Allow AttributeAccessInterface implementations to return a StatusIB (#…

…13950) * Propagate errors from AttributeAccessInterface::Write to the wire correctly. This allows implementations of Write to return a CHIP_ERROR representing a StatusIB and have that land on the wire as a StatusIB with the right values. * Propagate errors from AttributeAccessInterface::Read to the wire correctly. This allows implementations of Read to return a CHIP_ERROR representing a StatusIB and have that land on the wire as a StatusIB with the right values.
project-chip · Oct 17, 2023 · 1485418 · 1485418
1 parent 863fb4f
commit 1485418
Show file tree

Hide file tree

Showing 40 changed files with 1,290 additions and 90 deletions.
diff --git a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter b/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter
@@ -2607,6 +2607,8 @@ server cluster TestCluster = 1295 {
   readonly attribute LONG_OCTET_STRING listLongOctetString[] = 42;
   readonly attribute TestFabricScoped listFabricScoped[] = 43;
   attribute boolean timedWriteBoolean = 48;
+  attribute boolean generalErrorBoolean = 49;
+  attribute boolean clusterErrorBoolean = 50;
   attribute nullable boolean nullableBoolean = 32768;
   attribute nullable bitmap8 nullableBitmap8 = 32769;
   attribute nullable bitmap16 nullableBitmap16 = 32770;

diff --git a/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap b/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap
@@ -1718,7 +1718,7 @@
               "reportableChange": 0
             },
             {
-              "name": "CalendarType",
+              "name": "ActiveCalendarType",
               "code": 1,
               "mfgCode": null,
               "side": "server",
@@ -16549,6 +16549,36 @@
               "maxInterval": 65534,
               "reportableChange": 0
             },
+            {
+              "name": "general_error_boolean",
+              "code": 49,
+              "mfgCode": null,
+              "side": "server",
+              "included": 1,
+              "storageOption": "External",
+              "singleton": 0,
+              "bounded": 0,
+              "defaultValue": "",
+              "reportable": 1,
+              "minInterval": 1,
+              "maxInterval": 65534,
+              "reportableChange": 0
+            },
+            {
+              "name": "cluster_error_boolean",
+              "code": 50,
+              "mfgCode": null,
+              "side": "server",
+              "included": 1,
+              "storageOption": "External",
+              "singleton": 0,
+              "bounded": 0,
+              "defaultValue": "",
+              "reportable": 1,
+              "minInterval": 1,
+              "maxInterval": 65534,
+              "reportableChange": 0
+            },
             {
               "name": "unsupported",
               "code": 255,
@@ -17069,7 +17099,7 @@
               "singleton": 0,
               "bounded": 0,
               "defaultValue": "",
-              "reportable": 0,
+              "reportable": 1,
               "minInterval": 1,
               "maxInterval": 65534,
               "reportableChange": 0
@@ -17084,7 +17114,7 @@
               "singleton": 0,
               "bounded": 0,
               "defaultValue": "0",
-              "reportable": 0,
+              "reportable": 1,
               "minInterval": 1,
               "maxInterval": 65534,
               "reportableChange": 0
@@ -20465,5 +20495,6 @@
       "endpointVersion": 1,
       "deviceIdentifier": 256
     }
-  ]
-}
+  ],
+  "log": []
+}
diff --git a/src/app/MessageDef/AttributeReportIBs.cpp b/src/app/MessageDef/AttributeReportIBs.cpp
@@ -96,5 +96,29 @@ AttributeReportIBs::Builder & AttributeReportIBs::Builder::EndOfAttributeReportI
     EndOfContainer();
     return *this;
 }
+
+CHIP_ERROR AttributeReportIBs::Builder::EncodeAttributeStatus(const ConcreteReadAttributePath & aPath, const StatusIB & aStatus)
+{
+    AttributeReportIB::Builder & attributeReport = CreateAttributeReport();
+    ReturnErrorOnFailure(GetError());
+    AttributeStatusIB::Builder & attributeStatusIBBuilder = attributeReport.CreateAttributeStatus();
+    ReturnErrorOnFailure(attributeReport.GetError());
+    AttributePathIB::Builder & attributePathIBBuilder = attributeStatusIBBuilder.CreatePath();
+    ReturnErrorOnFailure(attributeStatusIBBuilder.GetError());
+
+    attributePathIBBuilder.Endpoint(aPath.mEndpointId)
+        .Cluster(aPath.mClusterId)
+        .Attribute(aPath.mAttributeId)
+        .EndOfAttributePathIB();
+    ReturnErrorOnFailure(attributePathIBBuilder.GetError());
+    StatusIB::Builder & statusIBBuilder = attributeStatusIBBuilder.CreateErrorStatus();
+    ReturnErrorOnFailure(attributeStatusIBBuilder.GetError());
+    statusIBBuilder.EncodeStatusIB(aStatus);
+    ReturnErrorOnFailure(statusIBBuilder.GetError());
+
+    ReturnErrorOnFailure(attributeStatusIBBuilder.EndOfAttributeStatusIB().GetError());
+    return attributeReport.EndOfAttributeReportIB().GetError();
+}
+
 } // namespace app
 } // namespace chip
diff --git a/src/app/MessageDef/AttributeReportIBs.h b/src/app/MessageDef/AttributeReportIBs.h
@@ -28,6 +28,8 @@
 #include "AttributeReportIB.h"
 
 #include <app/AppBuildConfig.h>
+#include <app/ConcreteAttributePath.h>
+#include <app/MessageDef/StatusIB.h>
 #include <app/util/basic-types.h>
 #include <lib/core/CHIPCore.h>
 #include <lib/core/CHIPTLV.h>
@@ -77,6 +79,11 @@ class Builder : public ArrayBuilder
      */
     AttributeReportIBs::Builder & EndOfAttributeReportIBs();
 
+    /**
+     * Encode an AttributeReportIB containing an AttributeStatus.
+     */
+    CHIP_ERROR EncodeAttributeStatus(const ConcreteReadAttributePath & aPath, const StatusIB & aStatus);
+
 private:
     AttributeReportIB::Builder mAttributeReport;
 };

diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp
@@ -173,7 +173,14 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData
             const ConcreteAttributePath concretePath =
                 ConcreteAttributePath(clusterInfo.mEndpointId, clusterInfo.mClusterId, clusterInfo.mAttributeId);
             MatterPreAttributeWriteCallback(concretePath);
+            TLV::TLVWriter backup;
+            mWriteResponseBuilder.Checkpoint(backup);
             err = WriteSingleClusterData(subjectDescriptor, clusterInfo, dataReader, this);
+            if (err != CHIP_NO_ERROR)
+            {
+                mWriteResponseBuilder.Rollback(backup);
+                err = AddStatus(concretePath, StatusIB(err));
+            }
             MatterPostAttributeWriteCallback(concretePath);
         }
         SuccessOrExit(err);
@@ -253,22 +260,24 @@ Status WriteHandler::ProcessWriteRequest(System::PacketBufferHandle && aPayload,
     return status;
 }
 
-CHIP_ERROR WriteHandler::AddStatus(const AttributePathParams & aAttributePathParams,
-                                   const Protocols::InteractionModel::Status aStatus)
+CHIP_ERROR WriteHandler::AddStatus(const ConcreteAttributePath & aPath, const Protocols::InteractionModel::Status aStatus)
+{
+    return AddStatus(aPath, StatusIB(aStatus));
+}
+
+CHIP_ERROR WriteHandler::AddStatus(const ConcreteAttributePath & aPath, const StatusIB & aStatus)
 {
     AttributeStatusIBs::Builder & writeResponses   = mWriteResponseBuilder.GetWriteResponses();
     AttributeStatusIB::Builder & attributeStatusIB = writeResponses.CreateAttributeStatus();
     ReturnErrorOnFailure(writeResponses.GetError());
 
     AttributePathIB::Builder & path = attributeStatusIB.CreatePath();
     ReturnErrorOnFailure(attributeStatusIB.GetError());
-    ReturnErrorOnFailure(path.Encode(aAttributePathParams));
+    ReturnErrorOnFailure(path.Encode(AttributePathParams(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId)));
 
-    StatusIB statusIB;
-    statusIB.mStatus                    = aStatus;
     StatusIB::Builder & statusIBBuilder = attributeStatusIB.CreateErrorStatus();
     ReturnErrorOnFailure(attributeStatusIB.GetError());
-    statusIBBuilder.EncodeStatusIB(statusIB);
+    statusIBBuilder.EncodeStatusIB(aStatus);
     ReturnErrorOnFailure(statusIBBuilder.GetError());
     attributeStatusIB.EndOfAttributeStatusIB();
     ReturnErrorOnFailure(attributeStatusIB.GetError());

diff --git a/src/app/WriteHandler.h b/src/app/WriteHandler.h
@@ -75,7 +75,8 @@ class WriteHandler
 
     CHIP_ERROR ProcessAttributeDataIBs(TLV::TLVReader & aAttributeDataIBsReader);
 
-    CHIP_ERROR AddStatus(const AttributePathParams & aAttributePathParams, const Protocols::InteractionModel::Status aStatus);
+    CHIP_ERROR AddStatus(const ConcreteAttributePath & aPath, const Protocols::InteractionModel::Status aStatus);
+    CHIP_ERROR AddStatus(const ConcreteAttributePath & aPath, const StatusIB & aStatus);
 
     CHIP_ERROR AddClusterSpecificSuccess(const AttributePathParams & aAttributePathParams, uint8_t aClusterStatus)
     {

diff --git a/src/app/clusters/test-cluster-server/test-cluster-server.cpp b/src/app/clusters/test-cluster-server/test-cluster-server.cpp
@@ -134,6 +134,12 @@ CHIP_ERROR TestAttrAccess::Read(const ConcreteReadAttributePath & aPath, Attribu
     case NullableStruct::Id: {
         return ReadNullableStruct(aEncoder);
     }
+    case GeneralErrorBoolean::Id: {
+        return StatusIB(Protocols::InteractionModel::Status::InvalidDataType).ToChipError();
+    }
+    case ClusterErrorBoolean::Id: {
+        return StatusIB(Protocols::InteractionModel::Status::Failure, 17).ToChipError();
+    }
     default: {
         break;
     }
@@ -164,6 +170,12 @@ CHIP_ERROR TestAttrAccess::Write(const ConcreteDataAttributePath & aPath, Attrib
     case NullableStruct::Id: {
         return WriteNullableStruct(aDecoder);
     }
+    case GeneralErrorBoolean::Id: {
+        return StatusIB(Protocols::InteractionModel::Status::InvalidDataType).ToChipError();
+    }
+    case ClusterErrorBoolean::Id: {
+        return StatusIB(Protocols::InteractionModel::Status::Failure, 17).ToChipError();
+    }
     default: {
         break;
     }

diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp
@@ -126,7 +126,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
                              "Error retrieving data from clusterId: " ChipLogFormatMEI ", err = %" CHIP_ERROR_FORMAT,
                              ChipLogValueMEI(pathForRetrieval.mClusterId), err.Format());
 
-                if (encodeState.AllowPartialData())
+                if (encodeState.AllowPartialData() && ((err == CHIP_ERROR_BUFFER_TOO_SMALL) || (err == CHIP_ERROR_NO_MEMORY)))
                 {
                     // Encoding is aborted but partial data is allowed, then we don't rollback and save the state for next chunk.
                     apReadHandler->SetAttributeEncodeState(encodeState);
@@ -137,6 +137,14 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
                     // attributeReportIB to avoid any partial data.
                     attributeReportIBs.Rollback(attributeBackup);
                     apReadHandler->SetAttributeEncodeState(AttributeValueEncoder::AttributeEncodeState());
+
+                    // Try to encode our error as a status response.
+                    err = attributeReportIBs.EncodeAttributeStatus(pathForRetrieval, StatusIB(err));
+                    if (err != CHIP_NO_ERROR)
+                    {
+                        // OK, just roll back again and give up.
+                        attributeReportIBs.Rollback(attributeBackup);
+                    }
                 }
             }
             SuccessOrExit(err);

diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp
@@ -107,12 +107,9 @@ void TestWriteInteraction::AddAttributeDataIB(nlTestSuite * apSuite, void * apCo
 void TestWriteInteraction::AddAttributeStatus(nlTestSuite * apSuite, void * apContext, WriteHandler & aWriteHandler)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
-    AttributePathParams attributePathParams;
-    attributePathParams.mEndpointId  = 2;
-    attributePathParams.mClusterId   = 3;
-    attributePathParams.mAttributeId = 4;
+    ConcreteAttributePath attributePath(2, 3, 4);
 
-    err = aWriteHandler.AddStatus(attributePathParams, Protocols::InteractionModel::Status::Success);
+    err = aWriteHandler.AddStatus(attributePath, Protocols::InteractionModel::Status::Success);
     NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 }
 
@@ -310,7 +307,7 @@ CHIP_ERROR WriteSingleClusterData(const Access::SubjectDescriptor & aSubjectDesc
     writer.CopyElement(TLV::AnonymousTag(), aReader);
     attributeDataTLVLen = writer.GetLengthWritten();
     return aWriteHandler->AddStatus(
-        AttributePathParams(aClusterInfo.mEndpointId, aClusterInfo.mClusterId, aClusterInfo.mAttributeId),
+        ConcreteAttributePath(aClusterInfo.mEndpointId, aClusterInfo.mClusterId, aClusterInfo.mAttributeId),
         Protocols::InteractionModel::Status::Success);
 }
 

diff --git a/src/app/tests/integration/chip_im_responder.cpp b/src/app/tests/integration/chip_im_responder.cpp
@@ -118,11 +118,8 @@ CHIP_ERROR WriteSingleClusterData(const Access::SubjectDescriptor & aSubjectDesc
                                   TLV::TLVReader & aReader, WriteHandler * apWriteHandler)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
-    AttributePathParams attributePathParams;
-    attributePathParams.mEndpointId  = 2;
-    attributePathParams.mClusterId   = 3;
-    attributePathParams.mAttributeId = 4;
-    err                              = apWriteHandler->AddStatus(attributePathParams, Protocols::InteractionModel::Status::Success);
+    ConcreteAttributePath attributePath(2, 3, 4);
+    err = apWriteHandler->AddStatus(attributePath, Protocols::InteractionModel::Status::Success);
     return err;
 }
 } // namespace app

diff --git a/src/app/tests/suites/TestCluster.yaml b/src/app/tests/suites/TestCluster.yaml
@@ -3709,3 +3709,35 @@ tests:
       attribute: "nullable_range_restricted_int16s"
       response:
           value: null
+
+    - label: "Write attribute that returns general status on write"
+      command: "writeAttribute"
+      attribute: "general_error_boolean"
+      arguments:
+          value: false
+      response:
+          error: INVALID_DATA_TYPE
+
+    - label: "Write attribute that returns cluster-specific status on write"
+      command: "writeAttribute"
+      attribute: "cluster_error_boolean"
+      arguments:
+          value: false
+      response:
+          # TODO: We don't have a way to represent cluster-specific status
+          # here yet.
+          error: FAILURE
+
+    - label: "Read attribute that returns general status on read"
+      command: "readAttribute"
+      attribute: "general_error_boolean"
+      response:
+          error: INVALID_DATA_TYPE
+
+    - label: "read attribute that returns cluster-specific status on read"
+      command: "readAttribute"
+      attribute: "cluster_error_boolean"
+      response:
+          # TODO: We don't have a way to represent cluster-specific status
+          # here yet.
+          error: FAILURE