Implement the codegen-data-model Write support

.github/workflows/lint.yml

@@ -288,7 +288,17 @@ jobs:
                   type-safe setters
               if: always()
               run: |
-                  git grep -I -n 'emberAfWriteAttribute' -- './*' ':(exclude).github/workflows/lint.yml' ':(exclude)zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.cpp' ':(exclude)src/app/zap-templates/templates/app/attributes/Accessors-src.zapt' ':(exclude)src/app/util/attribute-table.cpp' ':(exclude)examples/common/pigweed/rpc_services/Attributes.h' ':(exclude)src/app/util/attribute-table.h' ':(exclude)src/app/util/ember-compatibility-functions.cpp' && exit 1 || exit 0
+                  git grep -I -n 'emberAfWriteAttribute' -- './*'                                            \
+                      ':(exclude).github/workflows/lint.yml'                                                 \
+                      ':(exclude)examples/common/pigweed/rpc_services/Attributes.h'                          \
+                      ':(exclude)src/app/codegen-data-model/CodegenDataModel_Write.cpp'                      \
+                      ':(exclude)src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp'                \
+                      ':(exclude)src/app/util/attribute-table.cpp'                                           \
+                      ':(exclude)src/app/util/attribute-table.h'                                             \
+                      ':(exclude)src/app/util/ember-compatibility-functions.cpp'                             \
+                      ':(exclude)src/app/zap-templates/templates/app/attributes/Accessors-src.zapt'          \
+                      ':(exclude)zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.cpp' \
+                      && exit 1 || exit 0
 
             # Run ruff python linter
             - name: Check for errors using ruff Python linter

src/app/codegen-data-model/CodegenDataModel_Read.cpp

@@ -37,7 +37,6 @@
 #include <app/util/attribute-storage.h>
 #include <app/util/ember-global-attribute-access-interface.h>
 #include <app/util/ember-io-storage.h>
-#include <app/util/ember-strings.h>
 #include <app/util/endpoint-config-api.h>
 #include <app/util/odd-sized-integers.h>
 #include <lib/core/CHIPError.h>
@@ -90,7 +89,12 @@ struct ShortPascalString
     using LengthType                        = uint8_t;
     static constexpr LengthType kNullLength = 0xFF;
 
-    static LengthType GetLength(const uint8_t * buffer) { return emberAfStringLength(buffer); }
+    static LengthType GetLength(const uint8_t * buffer)
+    {
+        // NOTE: we do NOT use emberAfLongStringLength because that will result in 0 length
+        //       for null strings
+        return *buffer;
+    }
 };
 
 /// Metadata of what a ember/pascal LONG string means (prepended by a u16 length)
@@ -118,7 +122,7 @@ template <class OUT, class ENCODING>
 std::optional<OUT> ExtractEmberString(ByteSpan data)
 {
     VerifyOrDie(sizeof(typename ENCODING::LengthType) <= data.size());
-    typename ENCODING::LengthType len = ENCODING::GetLength(data.data());
+    auto len = ENCODING::GetLength(data.data());
 
     if (len == ENCODING::kNullLength)
     {
@@ -238,7 +242,7 @@ CHIP_ERROR EncodeEmberValue(ByteSpan data, const EmberAfAttributeMetadata * meta
         return EncodeStringLike<ByteSpan, LongPascalString>(data, isNullable, encoder);
     default:
         ChipLogError(DataManagement, "Attribute type 0x%x not handled", static_cast<int>(metadata->attributeType));
-        return CHIP_IM_GLOBAL_STATUS(ConstraintError);
+        return CHIP_IM_GLOBAL_STATUS(Failure);
     }
 }
 
@@ -270,7 +274,7 @@ CHIP_ERROR CodegenDataModel::ReadAttribute(const InteractionModel::ReadAttribute
             ReturnErrorCodeIf(err != CHIP_ERROR_ACCESS_DENIED, err);
 
             // Implementation of 8.4.3.2 of the spec for path expansion
-            if (request.path.mExpanded && (err == CHIP_ERROR_ACCESS_DENIED))
+            if (request.path.mExpanded)
             {
                 return CHIP_NO_ERROR;
             }

src/app/codegen-data-model/CodegenDataModel_Write.cpp

@@ -23,10 +23,13 @@
 #include <app/RequiredPrivilege.h>
 #include <app/codegen-data-model/EmberMetadata.h>
 #include <app/data-model/FabricScoped.h>
+#include <app/reporting/reporting.h>
 #include <app/util/af-types.h>
 #include <app/util/attribute-metadata.h>
 #include <app/util/attribute-storage-detail.h>
 #include <app/util/attribute-storage-null-handling.h>
+#include <app/util/attribute-table-detail.h>
+#include <app/util/attribute-table.h>
 #include <app/util/ember-io-storage.h>
 #include <app/util/ember-strings.h>
 #include <app/util/odd-sized-integers.h>
@@ -92,14 +95,14 @@ struct LongPascalString
 static_assert(sizeof(ShortPascalString::LengthType) == 1);
 static_assert(sizeof(LongPascalString::LengthType) == 2);
 
-/// Encode the value stored in 'decoder' into an ember format string 'out'
+/// Convert the value stored in 'decoder' into an ember format span 'out'
 ///
-/// The value encoded will be of type T (e.g. CharSpan or ByteSpan) and it will be encoded
+/// The value converted will be of type T (e.g. CharSpan or ByteSpan) and it will be converted
 /// via the given ENCODING (i.e. ShortPascalString or LongPascalString)
 ///
-/// isNullable defines if the value of NULL is allowed to be encoded.
+/// isNullable defines if the value of NULL is allowed to be converted.
 template <typename T, class ENCODING>
-CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool isNullable, MutableByteSpan out)
+CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool isNullable, MutableByteSpan & out)
 {
     T workingValue;
 
@@ -112,6 +115,7 @@ CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool i
         {
             VerifyOrReturnError(out.size() >= sizeof(typename ENCODING::LengthType), CHIP_ERROR_BUFFER_TOO_SMALL);
             ENCODING::SetLength(out.data(), ENCODING::kNullLength);
+            out.reduce_size(sizeof(typename ENCODING::LengthType));
             return CHIP_NO_ERROR;
         }
 
@@ -123,7 +127,7 @@ CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool i
         ReturnErrorOnFailure(decoder.Decode(workingValue));
     }
 
-    typename ENCODING::LengthType len = static_cast<typename ENCODING::LengthType>(workingValue.size());
+    auto len = static_cast<typename ENCODING::LengthType>(workingValue.size());
     VerifyOrReturnError(out.size() >= sizeof(len) + len, CHIP_ERROR_BUFFER_TOO_SMALL);
 
     uint8_t * output_buffer = out.data();
@@ -132,15 +136,17 @@ CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool i
     output_buffer += sizeof(len);
 
     memcpy(output_buffer, workingValue.data(), workingValue.size());
+    output_buffer += workingValue.size();
 
+    out.reduce_size(output_buffer - out.data());
     return CHIP_NO_ERROR;
 }
 
-/// Encodes a numeric data value of type T from the given ember-encoded buffer `data`.
+/// Decodes a numeric data value of type T from the `decoder` into a ember-encoded buffer `out`
 ///
-/// isNullable defines if the value of NULL is allowed to be encoded.
+/// isNullable defines if the value of NULL is allowed to be decoded.
 template <typename T>
-CHIP_ERROR DecodeIntoEmberBuffer(AttributeValueDecoder & decoder, bool isNullable, MutableByteSpan out)
+CHIP_ERROR DecodeIntoEmberBuffer(AttributeValueDecoder & decoder, bool isNullable, MutableByteSpan & out)
 {
     using Traits = NumericAttributeTraits<T>;
     typename Traits::StorageType storageValue;
@@ -156,7 +162,7 @@ CHIP_ERROR DecodeIntoEmberBuffer(AttributeValueDecoder & decoder, bool isNullabl
         }
         else
         {
-            // This guards against trying to encode something that overlaps nullable, for example
+            // This guards against trying to decode something that overlaps nullable, for example
             // Nullable<uint8_t>(0xFF) is not representable because 0xFF is the encoding of NULL in ember
             // as well as odd-sized integers (e.g. full 32-bit value like 0x11223344 cannot be written
             // to a 3-byte odd-sized integger).
@@ -185,15 +191,20 @@ CHIP_ERROR DecodeIntoEmberBuffer(AttributeValueDecoder & decoder, bool isNullabl
     // The decoding + ToAttributeStoreRepresentation will result in data being
     // stored in native format/byteorder, suitable to directly be stored in the data store
     memcpy(out.data(), data, sizeof(storageValue));
+    out.reduce_size(sizeof(storageValue));
 
     return CHIP_NO_ERROR;
 }
 
 /// Read the data from "decoder" into an ember-formatted buffer "out"
 ///
+/// `out` is a in/out buffer:
+///    - its initial size determines the maximum size of the buffer
+///    - its output size reflects the actual data size
+///
 /// Uses the attribute `metadata` to determine how the data is to be encoded into out.
 CHIP_ERROR DecodeValueIntoEmberBuffer(AttributeValueDecoder & decoder, const EmberAfAttributeMetadata * metadata,
-                                      MutableByteSpan out)
+                                      MutableByteSpan & out)
 {
     VerifyOrReturnError(metadata != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
 
@@ -249,7 +260,7 @@ CHIP_ERROR DecodeValueIntoEmberBuffer(AttributeValueDecoder & decoder, const Emb
         return DecodeStringLikeIntoEmberBuffer<ByteSpan, LongPascalString>(decoder, isNullable, out);
     default:
         ChipLogError(DataManagement, "Attribute type 0x%x not handled", static_cast<int>(metadata->attributeType));
-        return CHIP_IM_GLOBAL_STATUS(ConstraintError);
+        return CHIP_IM_GLOBAL_STATUS(Failure);
     }
 }
 
@@ -258,7 +269,7 @@ CHIP_ERROR DecodeValueIntoEmberBuffer(AttributeValueDecoder & decoder, const Emb
 CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttributeRequest & request,
                                             AttributeValueDecoder & decoder)
 {
-    ChipLogDetail(DataManagement, "Writing attribute: Cluster=" ChipLogFormatMEI " Endpoint=%x AttributeId=" ChipLogFormatMEI,
+    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
@@ -293,7 +304,7 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu
 
     // All the global attributes that we do not have metadata for are
     // read-only. Specifically only the following list-based attributes match the
-    // "global attributes not in metadata" (see GlobalAttributes.h :: GlobalAttributesNotInMetadat):
+    // "global attributes not in metadata" (see GlobalAttributes.h :: GlobalAttributesNotInMetadata):
     //   - AttributeList
     //   - EventList
     //   - AcceptedCommands
@@ -321,45 +332,68 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu
         std::optional<InteractionModel::ClusterInfo> clusterInfo = GetClusterInfo(request.path);
         if (!clusterInfo.has_value())
         {
-            ChipLogError(DataManagement, "Unable to get cluster info for Endpoint %x, Cluster " ChipLogFormatMEI,
+            ChipLogError(DataManagement, "Unable to get cluster info for Endpoint 0x%x, Cluster " ChipLogFormatMEI,
                          request.path.mEndpointId, ChipLogValueMEI(request.path.mClusterId));
-            return CHIP_ERROR_NOT_FOUND;
+            return CHIP_IM_GLOBAL_STATUS(DataVersionMismatch);
         }
 
         if (request.path.mDataVersion.Value() != clusterInfo->dataVersion)
         {
-            ChipLogError(DataManagement, "Write Version mismatch for Endpoint %x, Cluster " ChipLogFormatMEI,
+            ChipLogError(DataManagement, "Write Version mismatch for Endpoint 0x%x, Cluster " ChipLogFormatMEI,
                          request.path.mEndpointId, ChipLogValueMEI(request.path.mClusterId));
             return CHIP_IM_GLOBAL_STATUS(DataVersionMismatch);
         }
     }
 
     AttributeAccessInterface * aai       = GetAttributeAccessOverride(request.path.mEndpointId, request.path.mClusterId);
     std::optional<CHIP_ERROR> aai_result = TryWriteViaAccessInterface(request.path, aai, decoder);
-
     if (aai_result.has_value())
     {
+        if (*aai_result == CHIP_NO_ERROR)
+        {
+            // TODO: change callbacks should likely be routed through the context `MarkDirty` only
+            //       however for now this is called directly because ember code does this call
+            //       inside emberAfWriteAttribute.
+            MatterReportingAttributeChangeCallback(request.path);
+            CurrentContext().dataModelChangeListener->MarkDirty(request.path);
+        }
         return *aai_result;
     }
 
-    ReturnErrorCodeIf(aai_result.has_value(), *aai_result);
+    MutableByteSpan dataBuffer = gEmberAttributeIOBufferSpan;
+    ReturnErrorOnFailure(DecodeValueIntoEmberBuffer(decoder, *attributeMetadata, dataBuffer));
 
-    ReturnErrorOnFailure(DecodeValueIntoEmberBuffer(decoder, *attributeMetadata, gEmberAttributeIOBufferSpan));
+    Protocols::InteractionModel::Status status;
 
-    EmberAfAttributeSearchRecord record;
-    record.endpoint    = request.path.mEndpointId;
-    record.clusterId   = request.path.mClusterId;
-    record.attributeId = request.path.mAttributeId;
+    if (request.operationFlags.Has(InteractionModel::OperationFlags::kInternal))
+    {
+        // Internal requests use the non-External interface that has less enforcement
+        // than the external version (e.g. does not check/enforce writable settings, does not
+        // validate atribute types) - see attribute-table.h documentation for details.
+        status = emberAfWriteAttribute(request.path.mEndpointId, request.path.mClusterId, request.path.mAttributeId,
+                                       gEmberAttributeIOBufferSpan.data(), (*attributeMetadata)->attributeType);
+    }
+    else
+    {
+        if (dataBuffer.size() > (*attributeMetadata)->size)
+        {
+            ChipLogDetail(Zcl, "Data to write exceedes the attribute size claimed.");
+            return CHIP_IM_GLOBAL_STATUS(InvalidValue);
+        }
 
-    Protocols::InteractionModel::Status status = emAfReadOrWriteAttribute(
-        &record, attributeMetadata, gEmberAttributeIOBufferSpan.data(), static_cast<uint16_t>(gEmberAttributeIOBufferSpan.size()),
-        /* write = */ true);
+        status = emAfWriteAttributeExternal(request.path.mEndpointId, request.path.mClusterId, request.path.mAttributeId,
+                                            gEmberAttributeIOBufferSpan.data(), (*attributeMetadata)->attributeType);
+    }
 
     if (status != Protocols::InteractionModel::Status::Success)
     {
         return CHIP_ERROR_IM_GLOBAL_STATUS_VALUE(status);
     }
 
+    // TODO: this may need more refinement:
+    //       - should internal requests be able to decide if something is marked dirty or not?
+    //       - changes-omitted paths should not be marked dirty (ember is not aware of these)
+    CurrentContext().dataModelChangeListener->MarkDirty(request.path);
     return CHIP_NO_ERROR;
 }
 

src/app/codegen-data-model/EmberMetadata.cpp

@@ -30,18 +30,23 @@ std::variant<const EmberAfCluster *,           // global attribute, data from a
              >
 FindAttributeMetadata(const ConcreteAttributePath & aPath)
 {
-    for (auto & attr : GlobalAttributesNotInMetadata)
+    if (IsGlobalAttribute(aPath.mAttributeId))
     {
-        if (attr == aPath.mAttributeId)
+        // Global list attribute check first: during path expansion a lot of attributes
+        // will actually be global attributes (so not too much of a performance hit)
+        for (auto & attr : GlobalAttributesNotInMetadata)
         {
-            const EmberAfCluster * cluster = emberAfFindServerCluster(aPath.mEndpointId, aPath.mClusterId);
-            if (cluster == nullptr)
+            if (attr == aPath.mAttributeId)
             {
-                return (emberAfFindEndpointType(aPath.mEndpointId) == nullptr) ? CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)
-                                                                               : CHIP_IM_GLOBAL_STATUS(UnsupportedCluster);
-            }
+                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 cluster;
+                return cluster;
+            }
         }
     }
     const EmberAfAttributeMetadata * metadata =

src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp

@@ -17,6 +17,7 @@
 #include "EmberReadWriteOverride.h"
 
 #include <app/util/attribute-storage.h>
+#include <app/util/ember-io-storage.h>
 #include <lib/support/Span.h>
 
 using chip::Protocols::InteractionModel::Status;
@@ -98,3 +99,31 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord,
 
     return Status::Success;
 }
+
+Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID,
+                                  uint8_t * dataPtr, EmberAfAttributeType dataType)
+{
+    if (gEmberStatusCode != Status::Success)
+    {
+        return gEmberStatusCode;
+    }
+
+    // ember here deduces the size of dataPtr. For testing however, we KNOW we read
+    // out of the ember IO buffer, so we try to use that
+    VerifyOrDie(dataPtr == chip::app::Compatibility::Internal::gEmberAttributeIOBufferSpan.data());
+
+    // In theory this should do type validation and sizes. This is NOT done for testing.
+    // copy over as much data as possible
+    // NOTE: we do NOT use (*metadata)->size since it is unclear if our mocks set that correctly
+    size_t len = std::min<size_t>(sizeof(gEmberIoBuffer), chip::app::Compatibility::Internal::gEmberAttributeIOBufferSpan.size());
+    memcpy(gEmberIoBuffer, dataPtr, len);
+    gEmberIoBufferFill = len;
+
+    return Status::Success;
+}
+
+Status emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, uint8_t * dataPtr,
+                             EmberAfAttributeType dataType)
+{
+    return emAfWriteAttributeExternal(endpoint, cluster, attributeID, dataPtr, dataType);
+}

src/app/codegen-data-model/tests/InteractionModelTemporaryOverrides.cpp

@@ -82,3 +82,8 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPat
 
 } // namespace app
 } // namespace chip
+
+void MatterReportingAttributeChangeCallback(const chip::app::ConcreteAttributePath & aPath)
+{
+    // TODO: should we add logic to track these calls for test purposes?
+}