Use pointer for subject descriptor in datamodel provider instead of s…

…td::optional (project-chip#36246) * Use a pointer for the subject descriptor. This seems to save about 88 bytes of flash on a test NRF board. * Restyle * Fix include * Fix include * Also fix PW rpc --------- Co-authored-by: Andrei Litvin <[email protected]>
espressif · Oct 28, 2024 · 2142870 · 2142870
1 parent 77b4780
commit 2142870
Show file tree

Hide file tree

Showing 12 changed files with 42 additions and 20 deletions.
diff --git a/examples/common/pigweed/rpc_services/Attributes.h b/examples/common/pigweed/rpc_services/Attributes.h
@@ -224,7 +224,7 @@ class Attributes : public pw_rpc::nanopb::Attributes::Service<Attributes>
         app::DataModel::ReadAttributeRequest request;
         request.path = path;
         request.operationFlags.Set(app::DataModel::OperationFlags::kInternal);
-        request.subjectDescriptor = subjectDescriptor;
+        request.subjectDescriptor = &subjectDescriptor;
 
         std::optional<app::DataModel::ClusterInfo> info = provider->GetClusterInfo(path);
         if (!info.has_value())

diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp
@@ -18,6 +18,7 @@
 #include <app/CommandHandlerImpl.h>
 
 #include <access/AccessControl.h>
+#include <access/SubjectDescriptor.h>
 #include <app-common/zap-generated/cluster-objects.h>
 #include <app/MessageDef/StatusIB.h>
 #include <app/RequiredPrivilege.h>
@@ -391,10 +392,11 @@ Status CommandHandlerImpl::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
     VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);
 
     {
+        Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor();
         DataModel::InvokeRequest request;
 
         request.path              = concretePath;
-        request.subjectDescriptor = GetSubjectDescriptor();
+        request.subjectDescriptor = &subjectDescriptor;
         request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, IsTimedInvoke());
 
         Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request);
@@ -513,10 +515,11 @@ Status CommandHandlerImpl::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo
         const ConcreteCommandPath concretePath(mapping.endpoint_id, clusterId, commandId);
 
         {
+            Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor();
             DataModel::InvokeRequest request;
 
             request.path              = concretePath;
-            request.subjectDescriptor = GetSubjectDescriptor();
+            request.subjectDescriptor = &subjectDescriptor;
             request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, IsTimedInvoke());
 
             Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request);

diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -1646,10 +1646,12 @@ void InteractionModelEngine::DispatchCommand(CommandHandlerImpl & apCommandObj,
 {
 #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
 
+    Access::SubjectDescriptor subjectDescriptor = apCommandObj.GetSubjectDescriptor();
+
     DataModel::InvokeRequest request;
     request.path = aCommandPath;
     request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, apCommandObj.IsTimedInvoke());
-    request.subjectDescriptor = apCommandObj.GetSubjectDescriptor();
+    request.subjectDescriptor = &subjectDescriptor;
 
     std::optional<DataModel::ActionReturnStatus> status = GetDataModelProvider()->Invoke(request, apPayload, &apCommandObj);
 
@@ -1702,7 +1704,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::ValidateCommandCanBe
 
 Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandAccess(const DataModel::InvokeRequest & aRequest)
 {
-    if (!aRequest.subjectDescriptor.has_value())
+    if (aRequest.subjectDescriptor == nullptr)
     {
         return Status::UnsupportedAccess; // we require a subject for invoke
     }

diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp
@@ -779,7 +779,7 @@ CHIP_ERROR WriteHandler::WriteClusterData(const Access::SubjectDescriptor & aSub
     DataModel::WriteAttributeRequest request;
 
     request.path                = aPath;
-    request.subjectDescriptor   = aSubject;
+    request.subjectDescriptor   = &aSubject;
     request.previousSuccessPath = mLastSuccessfullyWrittenPath;
     request.writeFlags.Set(DataModel::WriteFlags::kTimed, IsTimedWrite());
 

diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp
@@ -106,7 +106,7 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::ReadAttribute(const Data
     // ACL check for non-internal requests
     if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal))
     {
-        VerifyOrReturnError(request.subjectDescriptor.has_value(), CHIP_ERROR_INVALID_ARGUMENT);
+        VerifyOrReturnError(request.subjectDescriptor != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
 
         Access::RequestPath requestPath{ .cluster     = request.path.mClusterId,
                                          .endpoint    = request.path.mEndpointId,

diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp
@@ -159,7 +159,7 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat
 
     if (checkAcl)
     {
-        VerifyOrReturnError(request.subjectDescriptor.has_value(), Status::UnsupportedAccess);
+        VerifyOrReturnError(request.subjectDescriptor != nullptr, Status::UnsupportedAccess);
 
         Access::RequestPath requestPath{ .cluster     = request.path.mClusterId,
                                          .endpoint    = request.path.mEndpointId,

diff --git a/src/app/data-model-provider/OperationTypes.h b/src/app/data-model-provider/OperationTypes.h
@@ -55,15 +55,16 @@ struct OperationRequest
     ///  - operationFlags.Has(OperationFlags::kInternal) MUST NOT have this set
     ///
     /// NOTE: once kInternal flag is removed, this will become non-optional
-    std::optional<chip::Access::SubjectDescriptor> subjectDescriptor;
+    const chip::Access::SubjectDescriptor * subjectDescriptor = nullptr;
 
     /// Accessing fabric index is the subjectDescriptor fabric index (if any).
     /// This is a readability convenience function.
     ///
     /// Returns kUndefinedFabricIndex if no subject descriptor is available
     FabricIndex GetAccessingFabricIndex() const
     {
-        return subjectDescriptor.has_value() ? subjectDescriptor->fabricIndex : kUndefinedFabricIndex;
+        VerifyOrReturnValue(subjectDescriptor != nullptr, kUndefinedFabricIndex);
+        return subjectDescriptor->fabricIndex;
     }
 };
 

diff --git a/src/app/data-model-provider/tests/ReadTesting.h b/src/app/data-model-provider/tests/ReadTesting.h
@@ -136,7 +136,7 @@ class ReadOperation
     ReadOperation(const ConcreteAttributePath & path)
     {
         mRequest.path              = path;
-        mRequest.subjectDescriptor = kDenySubjectDescriptor;
+        mRequest.subjectDescriptor = &kDenySubjectDescriptor;
     }
 
     ReadOperation(EndpointId endpoint, ClusterId cluster, AttributeId attribute) :
@@ -146,7 +146,7 @@ class ReadOperation
     ReadOperation & SetSubjectDescriptor(const chip::Access::SubjectDescriptor & descriptor)
     {
         VerifyOrDie(mState == State::kInitializing);
-        mRequest.subjectDescriptor = descriptor;
+        mRequest.subjectDescriptor = &descriptor;
         return *this;
     }
 

diff --git a/src/app/data-model-provider/tests/WriteTesting.h b/src/app/data-model-provider/tests/WriteTesting.h
@@ -47,7 +47,7 @@ class WriteOperation
     WriteOperation(const ConcreteDataAttributePath & path)
     {
         mRequest.path              = path;
-        mRequest.subjectDescriptor = kDenySubjectDescriptor;
+        mRequest.subjectDescriptor = &kDenySubjectDescriptor;
     }
 
     WriteOperation(EndpointId endpoint, ClusterId cluster, AttributeId attribute) :
@@ -56,7 +56,7 @@ class WriteOperation
 
     WriteOperation & SetSubjectDescriptor(const chip::Access::SubjectDescriptor & descriptor)
     {
-        mRequest.subjectDescriptor = descriptor;
+        mRequest.subjectDescriptor = &descriptor;
         return *this;
     }
 
@@ -123,7 +123,11 @@ class WriteOperation
     AttributeValueDecoder DecoderFor(const T & value)
     {
         mTLVReader = ReadEncodedValue(value);
-        return AttributeValueDecoder(mTLVReader, mRequest.subjectDescriptor.value_or(kDenySubjectDescriptor));
+        if (mRequest.subjectDescriptor == nullptr)
+        {
+            AttributeValueDecoder(mTLVReader, kDenySubjectDescriptor);
+        }
+        return AttributeValueDecoder(mTLVReader, *mRequest.subjectDescriptor);
     }
 
 private:

diff --git a/src/app/reporting/Read-DataModel.cpp b/src/app/reporting/Read-DataModel.cpp
@@ -47,7 +47,7 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode
     {
         readRequest.readFlags.Set(DataModel::ReadFlags::kFabricFiltered);
     }
-    readRequest.subjectDescriptor = subjectDescriptor;
+    readRequest.subjectDescriptor = &subjectDescriptor;
     readRequest.path              = path;
 
     DataVersion version = 0;

diff --git a/src/app/tests/test-interaction-model-api.cpp b/src/app/tests/test-interaction-model-api.cpp
@@ -13,6 +13,7 @@
  *    See the License for the specific language governing permissions and
  *    limitations under the License.
  */
+#include "access/SubjectDescriptor.h"
 #include <app/tests/test-interaction-model-api.h>
 
 #include <app/InteractionModelEngine.h>
@@ -171,8 +172,13 @@ ActionReturnStatus TestImCustomDataModel::ReadAttribute(const ReadAttributeReque
 {
     AttributeEncodeState mutableState(&encoder.GetState()); // provide a state copy to start.
 
-    CHIP_ERROR err = ReadSingleClusterData(request.subjectDescriptor.value_or(Access::SubjectDescriptor()),
-                                           request.readFlags.Has(ReadFlags::kFabricFiltered), request.path,
+    Access::SubjectDescriptor subjectDescriptor;
+    if (request.subjectDescriptor != nullptr)
+    {
+        subjectDescriptor = *request.subjectDescriptor;
+    }
+
+    CHIP_ERROR err = ReadSingleClusterData(subjectDescriptor, request.readFlags.Has(ReadFlags::kFabricFiltered), request.path,
                                            TestOnlyAttributeValueEncoderAccessor(encoder).Builder(), &mutableState);
 
     // state must survive CHIP_ERRORs as it is used for chunking

diff --git a/src/controller/tests/data_model/DataModelFixtures.cpp b/src/controller/tests/data_model/DataModelFixtures.cpp
@@ -18,6 +18,7 @@
 
 #include "DataModelFixtures.h"
 
+#include <access/SubjectDescriptor.h>
 #include <app-common/zap-generated/cluster-objects.h>
 #include <app-common/zap-generated/ids/Clusters.h>
 #include <app/AttributeValueDecoder.h>
@@ -522,8 +523,13 @@ ActionReturnStatus CustomDataModel::ReadAttribute(const ReadAttributeRequest & r
     }
 #endif // CHIP_CONFIG_USE_EMBER_DATA_MODEL && CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
 
-    CHIP_ERROR err = ReadSingleClusterData(request.subjectDescriptor.value_or(Access::SubjectDescriptor()),
-                                           request.readFlags.Has(ReadFlags::kFabricFiltered), request.path,
+    Access::SubjectDescriptor subjectDescriptor;
+    if (request.subjectDescriptor != nullptr)
+    {
+        subjectDescriptor = *request.subjectDescriptor;
+    }
+
+    CHIP_ERROR err = ReadSingleClusterData(subjectDescriptor, request.readFlags.Has(ReadFlags::kFabricFiltered), request.path,
                                            TestOnlyAttributeValueEncoderAccessor(encoder).Builder(), &mutableState);
 
     // state must survive CHIP_ERRORs as it is used for chunking