Address review comments.

Author: bzbarsky-apple

src/app/util/af-types.h

@@ -297,3 +297,15 @@ typedef chip::Protocols::InteractionModel::Status (*EmberAfClusterPreAttributeCh
 #define MAX_INT16U_VALUE (0xFFFF)
 
 /** @} END addtogroup */
+
+namespace chip {
+namespace app {
+
+enum class MarkAttributeDirty
+{
+    kIfChanged,
+    kNo,
+};
+
+} // namespace app
+} // namespace chip

src/app/util/af.h

@@ -24,7 +24,6 @@
 
 #include <app/util/af-types.h>
 
-#include <app-common/zap-generated/attributes/Accessors.h>
 #include <app/util/endpoint-config-api.h>
 
 #include <lib/core/DataModelTypes.h>
@@ -100,7 +99,7 @@ bool emberAfContainsClient(chip::EndpointId endpoint, chip::ClusterId clusterId)
 chip::Protocols::InteractionModel::Status
 emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, uint8_t * dataPtr,
                       EmberAfAttributeType dataType,
-                      chip::app::MarkAttributeDirty markDirty = chip::app::MarkAttributeDirty::IfChanged);
+                      chip::app::MarkAttributeDirty markDirty = chip::app::MarkAttributeDirty::kIfChanged);
 
 /**
  * @brief Read the attribute value, performing all the checks.

src/app/util/attribute-table.cpp

@@ -102,6 +102,55 @@ static bool IsNullValue(const uint8_t * data, uint16_t dataLen, bool isAttribute
     return false;
 }
 
+namespace {
+/**
+ * Helper function to determine whether the attribute value for the given
+ * attribute is changing.  On success, the isChanging outparam will be set to
+ * whether the value is changing.
+ */
+Status AttributeValueIsChanging(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * newValueData,
+                                EmberAfAttributeType dataType, bool * isChanging)
+{
+    size_t valueSize = AttributeTypeSize(dataType);
+
+    constexpr size_t maxValueSize = 16; // ipv6adr
+    if (valueSize > maxValueSize)
+    {
+        // Very much unexpected
+        ChipLogError(Zcl, "Attribute type %d has too-large size %u", dataType, static_cast<unsigned>(valueSize));
+        return Status::ConstraintError;
+    }
+
+    // valueSize will be 0 when we have no size information for dataType.
+    // In that case, we can't usefully read the current value, since we
+    // don't know how big it is.
+    if (valueSize == 0)
+    {
+        // To be safe, claim changing.
+        *isChanging = true;
+        return Status::Success;
+    }
+
+    uint8_t oldValueBuffer[maxValueSize];
+    // Cast to uint16_t is safe, because we checked valueSize <= maxValueSize above.
+    if (emberAfReadAttribute(endpoint, cluster, attributeID, oldValueBuffer, static_cast<uint16_t>(valueSize)) == Status::Success)
+    {
+        if (memcmp(newValueData, oldValueBuffer, valueSize) == 0)
+        {
+            // Value has not changed.
+            *isChanging = false;
+            return Status::Success;
+        }
+    }
+
+    // We failed to read the old value, or the value is changing.  Either way,
+    // flag the value as changing to be safe.
+    *isChanging = true;
+    return Status::Success;
+}
+
+} // anonymous namespace
+
 Status emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * data,
                           EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType, MarkAttributeDirty markDirty)
 {
@@ -182,40 +231,24 @@ Status emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId at
     }
 
     // Check whether anything is actually changing, before we do any work here.
-    size_t valueSize = AttributeTypeSize(dataType);
-
-    constexpr size_t maxValueSize = 16; // ipv6adr
-    if (valueSize > maxValueSize)
+    bool valueChanging;
+    Status imStatus = AttributeValueIsChanging(endpoint, cluster, attributeID, data, dataType, &valueChanging);
+    if (imStatus != Status::Success)
     {
-        // Very much unexpected
-        ChipLogError(Zcl, "Attribute type %d has too-large size %u", dataType, static_cast<unsigned>(valueSize));
-        return Status::ConstraintError;
+        return imStatus;
     }
 
-    // valueSize will be 0 when we have no size information for dataType.
-    // In that case, we can't usefully read the current value, since we
-    // don't know how big it is.
-    if (valueSize != 0)
+    if (!valueChanging)
     {
-        uint8_t oldValueBuffer[maxValueSize];
-        // Cast to uint16_t is safe, because we checked valueSize <= maxValueSize above.
-        if (emberAfReadAttribute(endpoint, cluster, attributeID, oldValueBuffer, static_cast<uint16_t>(valueSize)) ==
-            Status::Success)
-        {
-            if (memcmp(data, oldValueBuffer, valueSize) == 0)
-            {
-                // Value has not changed.  Just do nothing.
-                return Status::Success;
-            }
-        }
+        // Just do nothing.
+        return Status::Success;
     }
 
     const app::ConcreteAttributePath attributePath(endpoint, cluster, attributeID);
 
     // Pre write attribute callback for all attribute changes,
     // regardless of cluster.
-    Protocols::InteractionModel::Status imStatus =
-        MatterPreAttributeChangeCallback(attributePath, dataType, emberAfAttributeSize(metadata), data);
+    imStatus = MatterPreAttributeChangeCallback(attributePath, dataType, emberAfAttributeSize(metadata), data);
     if (imStatus != Protocols::InteractionModel::Status::Success)
     {
         return imStatus;
@@ -252,7 +285,7 @@ Status emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId at
     // The callee will weed out attributes that do not need to be stored.
     emAfSaveAttributeToStorageIfNeeded(data, endpoint, cluster, metadata);
 
-    if (markDirty != MarkAttributeDirty::No)
+    if (markDirty != MarkAttributeDirty::kNo)
     {
         MatterReportingAttributeChangeCallback(endpoint, cluster, attributeID);
     }

src/app/util/attribute-table.h

@@ -64,4 +64,4 @@ chip::Protocols::InteractionModel::Status emAfWriteAttributeExternal(chip::Endpo
 chip::Protocols::InteractionModel::Status
 emAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, uint8_t * data,
                    EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType,
-                   chip::app::MarkAttributeDirty markDirty = chip::app::MarkAttributeDirty::IfChanged);
+                   chip::app::MarkAttributeDirty markDirty = chip::app::MarkAttributeDirty::kIfChanged);

src/app/zap-templates/templates/app/attributes/Accessors-src.zapt

@@ -117,7 +117,7 @@ Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEn
 
 Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name forceNotNullable=true forceNotOptional=true}} value)
 {
-    return Set(endpoint, value, MarkAttributeDirty::IfChanged);
+    return Set(endpoint, value, MarkAttributeDirty::kIfChanged);
 }
 
 {{#if isNullable}}
@@ -137,7 +137,7 @@ Protocols::InteractionModel::Status SetNull(chip::EndpointId endpoint, MarkAttri
 
 Protocols::InteractionModel::Status SetNull(chip::EndpointId endpoint)
 {
-    return SetNull(endpoint, MarkAttributeDirty::IfChanged);
+    return SetNull(endpoint, MarkAttributeDirty::kIfChanged);
 }
 
 Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name isArgument=true forceNotOptional=true}} value, MarkAttributeDirty markDirty)
@@ -151,7 +151,7 @@ Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEn
 
 Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name isArgument=true forceNotOptional=true}} value)
 {
-    return Set(endpoint, value, MarkAttributeDirty::IfChanged);
+    return Set(endpoint, value, MarkAttributeDirty::kIfChanged);
 }
 {{/if}}
 

src/app/zap-templates/templates/app/attributes/Accessors.zapt

@@ -17,11 +17,6 @@
 namespace chip {
 namespace app {
 
-enum class MarkAttributeDirty {
-    IfChanged,
-    No,
-};
-
 namespace Clusters {
 
 {{#zcl_clusters}}