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.

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;

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": []
+}

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

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;
 };

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());

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)
  {

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;
  }

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);

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);
 }
 

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

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