Define an ActionReturnStatus for DataModel::Provider action returns (…

…project-chip#34708) * In progress * ActionReturnStatus implementation * Start making use of ActionReturnStatus * Things seem to compile (but not yet passing) * nice log formatting * Propper formatting and comparisons in tests. Mock tests pass * Restyle * Restyle * Fix typo * Add missing files * Added some unit tests that pass * More tests * Restyle * Update src/app/codegen-data-model-provider/EmberMetadata.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/data-model-provider/ActionReturnStatus.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/data-model-provider/ActionReturnStatus.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/data-model-provider/ActionReturnStatus.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/lib/core/CHIPError.h Co-authored-by: Boris Zbarsky <[email protected]> * Rename IsOutOfSpaceError and do not document specifics * Document invoke return codes * Use the new out of space method in checked * Restyle * Allow ClusterStatusCode to be constructed from a CHIP_ERROR * Format action statuses as c_str. HOWEVER this wastes 32 bytes of BSS * Fix error formatting * Restyle * Fix includes to be system paths * Update src/app/data-model-provider/Provider.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/data-model-provider/Provider.h Co-authored-by: Boris Zbarsky <[email protected]> * Fix status success and chip_no_error equivalence and add unit tests * Added more tests --------- Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
w8floosh · Aug 4, 2024 · ec11920 · ec11920
1 parent ec239e5
commit ec11920
Show file tree

Hide file tree

Showing 30 changed files with 729 additions and 152 deletions.
diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp
@@ -229,8 +229,8 @@ bool CodegenDataModelProvider::EmberCommandListIterator::Exists(const CommandId
     return (*mCurrentHint == toCheck);
 }
 
-CHIP_ERROR CodegenDataModelProvider::Invoke(const DataModel::InvokeRequest & request, TLV::TLVReader & input_arguments,
-                                            CommandHandler * handler)
+DataModel::ActionReturnStatus CodegenDataModelProvider::Invoke(const DataModel::InvokeRequest & request,
+                                                               TLV::TLVReader & input_arguments, CommandHandler * handler)
 {
     // TODO: CommandHandlerInterface support is currently
     //       residing in InteractionModelEngine itself. We may want to separate this out

diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider.h b/src/app/codegen-data-model-provider/CodegenDataModelProvider.h
@@ -16,6 +16,7 @@
  */
 #pragma once
 
+#include "app/data-model-provider/ActionReturnStatus.h"
 #include <app/data-model-provider/Provider.h>
 
 #include <app/util/af-types.h>
@@ -68,10 +69,12 @@ class CodegenDataModelProvider : public chip::app::DataModel::Provider
     /// Generic model implementations
     CHIP_ERROR Shutdown() override { return CHIP_NO_ERROR; }
 
-    CHIP_ERROR ReadAttribute(const DataModel::ReadAttributeRequest & request, AttributeValueEncoder & encoder) override;
-    CHIP_ERROR WriteAttribute(const DataModel::WriteAttributeRequest & request, AttributeValueDecoder & decoder) override;
-    CHIP_ERROR Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
-                      CommandHandler * handler) override;
+    DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request,
+                                                AttributeValueEncoder & encoder) override;
+    DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request,
+                                                 AttributeValueDecoder & decoder) override;
+    DataModel::ActionReturnStatus Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
+                                         CommandHandler * handler) override;
 
     /// attribute tree iteration
     EndpointId FirstEndpoint() override;

diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp
@@ -47,7 +47,9 @@
 namespace chip {
 namespace app {
 namespace {
+
 using namespace chip::app::Compatibility::Internal;
+using Protocols::InteractionModel::Status;
 
 /// Attempts to read via an attribute access interface (AAI)
 ///
@@ -258,7 +260,8 @@ CHIP_ERROR EncodeEmberValue(ByteSpan data, const EmberAfAttributeMetadata * meta
 ///    - validate ACL (only for non-internal requests)
 ///    - Try to read attribute via the AttributeAccessInterface
 ///    - Try to read the value from ember RAM storage
-CHIP_ERROR CodegenDataModelProvider::ReadAttribute(const DataModel::ReadAttributeRequest & request, AttributeValueEncoder & encoder)
+DataModel::ActionReturnStatus CodegenDataModelProvider::ReadAttribute(const DataModel::ReadAttributeRequest & request,
+                                                                      AttributeValueEncoder & encoder)
 {
     ChipLogDetail(DataManagement,
                   "Reading attribute: Cluster=" ChipLogFormatMEI " Endpoint=0x%x AttributeId=" ChipLogFormatMEI " (expanded=%d)",
@@ -290,12 +293,12 @@ CHIP_ERROR CodegenDataModelProvider::ReadAttribute(const DataModel::ReadAttribut
     auto metadata = Ember::FindAttributeMetadata(request.path);
 
     // Explicit failure in finding a suitable metadata
-    if (const CHIP_ERROR * err = std::get_if<CHIP_ERROR>(&metadata))
+    if (const Status * status = std::get_if<Status>(&metadata))
     {
-        VerifyOrDie((*err == CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)) || //
-                    (*err == CHIP_IM_GLOBAL_STATUS(UnsupportedCluster)) ||  //
-                    (*err == CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute)));
-        return *err;
+        VerifyOrDie((*status == Status::UnsupportedEndpoint) || //
+                    (*status == Status::UnsupportedCluster) ||  //
+                    (*status == Status::UnsupportedAttribute));
+        return *status;
     }
 
     // Read via AAI

diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp
@@ -43,6 +43,7 @@ namespace app {
 namespace {
 
 using namespace chip::app::Compatibility::Internal;
+using Protocols::InteractionModel::Status;
 
 /// Attempts to write via an attribute access interface (AAI)
 ///
@@ -266,16 +267,16 @@ CHIP_ERROR DecodeValueIntoEmberBuffer(AttributeValueDecoder & decoder, const Emb
 
 } // namespace
 
-CHIP_ERROR CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttributeRequest & request,
-                                                    AttributeValueDecoder & decoder)
+DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttributeRequest & request,
+                                                                       AttributeValueDecoder & decoder)
 {
     ChipLogDetail(DataManagement, "Writing attribute: Cluster=" ChipLogFormatMEI " Endpoint=0x%x AttributeId=" ChipLogFormatMEI,
                   ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId, ChipLogValueMEI(request.path.mAttributeId));
 
     // ACL check for non-internal requests
     if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal))
     {
-        ReturnErrorCodeIf(!request.subjectDescriptor.has_value(), CHIP_IM_GLOBAL_STATUS(UnsupportedAccess));
+        ReturnErrorCodeIf(!request.subjectDescriptor.has_value(), Status::UnsupportedAccess);
 
         Access::RequestPath requestPath{ .cluster = request.path.mClusterId, .endpoint = request.path.mEndpointId };
         CHIP_ERROR err = Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath,
@@ -286,18 +287,19 @@ CHIP_ERROR CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttrib
             ReturnErrorCodeIf(err != CHIP_ERROR_ACCESS_DENIED, err);
 
             // TODO: when wildcard/group writes are supported, handle them to discard rather than fail with status
-            return CHIP_IM_GLOBAL_STATUS(UnsupportedAccess);
+            return Status::UnsupportedAccess;
         }
     }
 
     auto metadata = Ember::FindAttributeMetadata(request.path);
 
-    if (const CHIP_ERROR * err = std::get_if<CHIP_ERROR>(&metadata))
+    // Explicit failure in finding a suitable metadata
+    if (const Status * status = std::get_if<Status>(&metadata))
     {
-        VerifyOrDie((*err == CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)) || //
-                    (*err == CHIP_IM_GLOBAL_STATUS(UnsupportedCluster)) ||  //
-                    (*err == CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute)));
-        return *err;
+        VerifyOrDie((*status == Status::UnsupportedEndpoint) || //
+                    (*status == Status::UnsupportedCluster) ||  //
+                    (*status == Status::UnsupportedAttribute));
+        return *status;
     }
 
     const EmberAfAttributeMetadata ** attributeMetadata = std::get_if<const EmberAfAttributeMetadata *>(&metadata);
@@ -316,15 +318,15 @@ CHIP_ERROR CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttrib
     // Internal is allowed to bypass timed writes and read-only.
     if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal))
     {
-        VerifyOrReturnError(!isReadOnly, CHIP_IM_GLOBAL_STATUS(UnsupportedWrite));
+        VerifyOrReturnError(!isReadOnly, Status::UnsupportedWrite);
 
         VerifyOrReturnError(!(*attributeMetadata)->MustUseTimedWrite() || request.writeFlags.Has(DataModel::WriteFlags::kTimed),
-                            CHIP_IM_GLOBAL_STATUS(NeedsTimedInteraction));
+                            Status::NeedsTimedInteraction);
     }
 
     // Extra check: internal requests can bypass the read only check, however global attributes
     // have no underlying storage, so write still cannot be done
-    VerifyOrReturnError(attributeMetadata != nullptr, CHIP_IM_GLOBAL_STATUS(UnsupportedWrite));
+    VerifyOrReturnError(attributeMetadata != nullptr, Status::UnsupportedWrite);
 
     if (request.path.mDataVersion.HasValue())
     {
@@ -333,14 +335,14 @@ CHIP_ERROR CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttrib
         {
             ChipLogError(DataManagement, "Unable to get cluster info for Endpoint 0x%x, Cluster " ChipLogFormatMEI,
                          request.path.mEndpointId, ChipLogValueMEI(request.path.mClusterId));
-            return CHIP_IM_GLOBAL_STATUS(DataVersionMismatch);
+            return Status::DataVersionMismatch;
         }
 
         if (request.path.mDataVersion.Value() != clusterInfo->dataVersion)
         {
             ChipLogError(DataManagement, "Write Version mismatch for Endpoint 0x%x, Cluster " ChipLogFormatMEI,
                          request.path.mEndpointId, ChipLogValueMEI(request.path.mClusterId));
-            return CHIP_IM_GLOBAL_STATUS(DataVersionMismatch);
+            return Status::DataVersionMismatch;
         }
     }
 
@@ -367,7 +369,7 @@ CHIP_ERROR CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttrib
     if (dataBuffer.size() > (*attributeMetadata)->size)
     {
         ChipLogDetail(Zcl, "Data to write exceeds the attribute size claimed.");
-        return CHIP_IM_GLOBAL_STATUS(InvalidValue);
+        return Status::InvalidValue;
     }
 
     if (request.operationFlags.Has(DataModel::OperationFlags::kInternal))
@@ -386,7 +388,7 @@ CHIP_ERROR CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttrib
 
     if (status != Protocols::InteractionModel::Status::Success)
     {
-        return CHIP_ERROR_IM_GLOBAL_STATUS_VALUE(status);
+        return status;
     }
 
     // TODO: this WILL requre updates

diff --git a/src/app/codegen-data-model-provider/EmberMetadata.cpp b/src/app/codegen-data-model-provider/EmberMetadata.cpp
@@ -24,9 +24,11 @@ namespace chip {
 namespace app {
 namespace Ember {
 
+using Protocols::InteractionModel::Status;
+
 std::variant<const EmberAfCluster *,           // global attribute, data from a cluster
              const EmberAfAttributeMetadata *, // a specific attribute stored by ember
-             CHIP_ERROR                        // error, this will NEVER be CHIP_NO_ERROR
+             Status                            // one of Status::Unsupported*
              >
 FindAttributeMetadata(const ConcreteAttributePath & aPath)
 {
@@ -41,8 +43,8 @@ FindAttributeMetadata(const ConcreteAttributePath & aPath)
                 const EmberAfCluster * cluster = emberAfFindServerCluster(aPath.mEndpointId, aPath.mClusterId);
                 if (cluster == nullptr)
                 {
-                    return (emberAfFindEndpointType(aPath.mEndpointId) == nullptr) ? CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)
-                                                                                   : CHIP_IM_GLOBAL_STATUS(UnsupportedCluster);
+                    return (emberAfFindEndpointType(aPath.mEndpointId) == nullptr) ? Status::UnsupportedEndpoint
+                                                                                   : Status::UnsupportedCluster;
                 }
 
                 return cluster;
@@ -57,18 +59,18 @@ FindAttributeMetadata(const ConcreteAttributePath & aPath)
         const EmberAfEndpointType * type = emberAfFindEndpointType(aPath.mEndpointId);
         if (type == nullptr)
         {
-            return CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint);
+            return Status::UnsupportedEndpoint;
         }
 
         const EmberAfCluster * cluster = emberAfFindClusterInType(type, aPath.mClusterId, CLUSTER_MASK_SERVER);
         if (cluster == nullptr)
         {
-            return CHIP_IM_GLOBAL_STATUS(UnsupportedCluster);
+            return Status::UnsupportedCluster;
         }
 
         // Since we know the attribute is unsupported and the endpoint/cluster are
         // OK, this is the only option left.
-        return CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute);
+        return Status::UnsupportedAttribute;
     }
 
     return metadata;

diff --git a/src/app/codegen-data-model-provider/EmberMetadata.h b/src/app/codegen-data-model-provider/EmberMetadata.h
@@ -18,6 +18,7 @@
 
 #include <app/util/af-types.h>
 #include <lib/core/CHIPError.h>
+#include <protocols/interaction_model/StatusCode.h>
 
 #include <variant>
 
@@ -31,13 +32,13 @@ namespace Ember {
 /// Possible return values:
 ///    - EmberAfCluster (NEVER null)           - Only for GlobalAttributesNotInMetaData
 ///    - EmberAfAttributeMetadata (NEVER null) - if the attribute is known to ember datastore
-///    - CHIP_ERROR, only specifically for unknown attributes, may only be one of:
-///        - CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint);
-///        - CHIP_IM_GLOBAL_STATUS(UnsupportedCluster);
-///        - CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute);
-std::variant<const EmberAfCluster *,           // global attribute, data from a cluster
-             const EmberAfAttributeMetadata *, // a specific attribute stored by ember
-             CHIP_ERROR                        // error, (CHIP_IM_GLOBAL_STATUS(Unsupported*))
+///    - Status, only specifically for unknown attributes, may only be one of:
+///        - Status::UnsupportedEndpoint
+///        - Status::UnsupportedCluster
+///        - Status::UnsupportedAttribute
+std::variant<const EmberAfCluster *,             // global attribute, data from a cluster
+             const EmberAfAttributeMetadata *,   // a specific attribute stored by ember
+             Protocols::InteractionModel::Status // one of Status::Unsupported*
              >
 FindAttributeMetadata(const ConcreteAttributePath & aPath);
 

diff --git a/src/app/codegen-data-model-provider/tests/BUILD.gn b/src/app/codegen-data-model-provider/tests/BUILD.gn
@@ -57,5 +57,8 @@ chip_test_suite("tests") {
 
   cflags = [ "-Wconversion" ]
 
-  public_deps = [ ":mock_model" ]
+  public_deps = [
+    ":mock_model",
+    "${chip_root}/src/app/data-model-provider:string-builder-adapters",
+  ]
 }