Optimize out no-op writes in attribute-table.

This way we don't mark things dirty if they have not changed.

Does not optimize out string values that are unchanged, because we have no good
way to tell how much space we need to read the old value.

Fixes project-chip#29136

Author: bzbarsky-apple

examples/chef/esp32/main/CMakeLists.txt

@@ -71,6 +71,7 @@ set(SRC_DIRS_LIST
                       "${CMAKE_SOURCE_DIR}/../common/clusters/channel/"
                       "${CMAKE_SOURCE_DIR}/../common/clusters/keypad-input/"
                       "${CMAKE_SOURCE_DIR}/../common/clusters/audio-output/"
+                      "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/zzz_generated/app-common/app-common/zap-generated"
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/zzz_generated/app-common/app-common/zap-generated/attributes"
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/src/app/server"
                       "${CMAKE_SOURCE_DIR}/third_party/connectedhomeip/src/app/util"

src/app/chip_data_model.cmake

@@ -135,6 +135,7 @@ function(chip_configure_data_model APP_TARGET)
     target_sources(${APP_TARGET} ${SCOPE}
         ${CHIP_APP_BASE_DIR}/../../zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.cpp
         ${CHIP_APP_BASE_DIR}/../../zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp
+        ${CHIP_APP_BASE_DIR}/../../zzz_generated/app-common/app-common/zap-generated/attribute-type.cpp
         ${CHIP_APP_BASE_DIR}/util/attribute-storage.cpp
         ${CHIP_APP_BASE_DIR}/util/attribute-table.cpp
         ${CHIP_APP_BASE_DIR}/util/binding-table.cpp

src/app/chip_data_model.gni

@@ -199,6 +199,7 @@ template("chip_data_model") {
       "${_app_root}/util/binding-table.h",
       "${_app_root}/util/generic-callback-stubs.cpp",
       "${_app_root}/util/privilege-storage.cpp",
+      "${chip_root}/zzz_generated/app-common/app-common/zap-generated/attribute-type.cpp",
       "${chip_root}/zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.cpp",
     ]
 

src/app/common/templates/templates.json

@@ -40,6 +40,11 @@
             "name": "ZCL attribute-type header",
             "output": "attribute-type.h"
         },
+        {
+            "path": "../../zap-templates/templates/app/attribute-type-src.zapt",
+            "name": "ZCL attribute-type implementation",
+            "output": "attribute-type.cpp"
+        },
         {
             "path": "../../zap-templates/templates/app/callback.zapt",
             "name": "ZCL callback header",

src/app/util/af.h

@@ -24,6 +24,7 @@
 
 #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>
@@ -96,9 +97,10 @@ bool emberAfContainsClient(chip::EndpointId endpoint, chip::ClusterId clusterId)
  * data type (as Accessors.h/cpp have this correct by default).
  * TODO: this not checking seems off - what if this is run without Accessors.h ?
  */
-chip::Protocols::InteractionModel::Status emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster,
-                                                                chip::AttributeId attributeID, uint8_t * dataPtr,
-                                                                EmberAfAttributeType dataType);
+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);
 
 /**
  * @brief Read the attribute value, performing all the checks.

src/app/util/attribute-table.cpp

@@ -16,6 +16,7 @@
  */
 #include <app/util/attribute-table.h>
 
+#include <app-common/zap-generated/attribute-type.h>
 #include <app/util/attribute-storage.h>
 #include <app/util/config.h>
 #include <app/util/generic-callbacks.h>
@@ -28,6 +29,7 @@
 using chip::Protocols::InteractionModel::Status;
 
 using namespace chip;
+using namespace chip::app;
 
 Status emAfWriteAttributeExternal(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
                                   EmberAfAttributeType dataType)
@@ -36,9 +38,9 @@ Status emAfWriteAttributeExternal(EndpointId endpoint, ClusterId cluster, Attrib
 }
 
 Status emberAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
-                             EmberAfAttributeType dataType)
+                             EmberAfAttributeType dataType, MarkAttributeDirty markDirty)
 {
-    return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, true /* override read-only */);
+    return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, true /* override read-only */, markDirty);
 }
 
 //------------------------------------------------------------------------------
@@ -101,7 +103,7 @@ static bool IsNullValue(const uint8_t * data, uint16_t dataLen, bool isAttribute
 }
 
 Status emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * data,
-                          EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType)
+                          EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType, MarkAttributeDirty markDirty)
 {
     const EmberAfAttributeMetadata * metadata = nullptr;
     EmberAfAttributeSearchRecord record;
@@ -179,6 +181,35 @@ 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)
+    {
+        // 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)
+    {
+        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;
+            }
+        }
+    }
+
     const app::ConcreteAttributePath attributePath(endpoint, cluster, attributeID);
 
     // Pre write attribute callback for all attribute changes,
@@ -221,7 +252,10 @@ 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);
 
-    MatterReportingAttributeChangeCallback(endpoint, cluster, attributeID);
+    if (markDirty != MarkAttributeDirty::No)
+    {
+        MatterReportingAttributeChangeCallback(endpoint, cluster, attributeID);
+    }
 
     // Post write attribute callback for all attributes changes, regardless
     // of cluster.

src/app/util/attribute-table.h

@@ -61,6 +61,7 @@ chip::Protocols::InteractionModel::Status emAfWriteAttributeExternal(chip::Endpo
  *           the attribute
  * - Status::Success: if the attribute was found and successfully written
  */
-chip::Protocols::InteractionModel::Status emAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster,
-                                                             chip::AttributeId attributeID, uint8_t * data,
-                                                             EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType);
+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);

src/app/zap-templates/templates/app/attribute-type-src.zapt

@@ -0,0 +1,26 @@
+{{> header}}
+
+#include <app-common/zap-generated/attribute-type.h>
+
+namespace chip {
+namespace app {
+
+size_t AttributeTypeSize(uint8_t attributeType)
+{
+    switch (attributeType)
+    {
+{{#zcl_atomics}}
+    case ZCL_{{asDelimitedMacro name}}_ATTRIBUTE_TYPE:
+        {{#if size}}
+        return {{size}};
+        {{else}}
+        return 0;
+        {{/if}}
+{{/zcl_atomics}}
+    default:
+        return 0;
+    }
+}
+
+} // namespace app
+} // namespace chip

src/app/zap-templates/templates/app/attribute-type.zapt

@@ -3,9 +3,22 @@
 // Prevent multiple inclusion
 #pragma once
 
+#include <cstddef>
+#include <cstdint>
+
 // ZCL attribute types
 enum {
 {{#zcl_atomics}}
 {{ident}}ZCL_{{asDelimitedMacro name}}_ATTRIBUTE_TYPE = {{asHex atomicId 2}}, // {{description}}
 {{/zcl_atomics}}
 };
+
+namespace chip {
+namespace app {
+
+// Returns 0 for attribute types without a well-defined size (like strings).
+{{! TODO: Figure out whether we can use EmberAfAttributeType here? }}
+size_t AttributeTypeSize(uint8_t attributeType);
+
+} // namespace app
+} // namespace chip

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

@@ -85,7 +85,8 @@ Protocols::InteractionModel::Status Get(chip::EndpointId endpoint, {{accessorGet
     return status;
     {{/if}}
 }
-Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name forceNotNullable=true forceNotOptional=true}} value)
+
+Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name forceNotNullable=true forceNotOptional=true}} value, MarkAttributeDirty markDirty)
 {
     {{~#if (isString type)}}
     {{~#*inline "lengthType"}}uint{{#if (isShortString type)}}8{{else}}16{{/if}}_t{{/inline}}
@@ -100,7 +101,7 @@ Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEn
       Encoding::LittleEndian::Put16(zclString, length);
     {{/if}}
     memcpy(&zclString[{{>sizingBytes}}], value.data(), value.size());
-    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, zclString, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE);
+    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, zclString, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE, markDirty);
     {{else}}
     using Traits = NumericAttributeTraits<{{accessorTraitType type}}>;
     if (!Traits::CanRepresentValue(/* isNullable = */ {{isNullable}}, value))
@@ -110,32 +111,47 @@ Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEn
     Traits::StorageType storageValue;
     Traits::WorkingToStorage(value, storageValue);
     uint8_t * writable = Traits::ToAttributeStoreRepresentation(storageValue);
-    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, writable, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE);
+    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, writable, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE, markDirty);
     {{/if}}
 }
 
+Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name forceNotNullable=true forceNotOptional=true}} value)
+{
+    return Set(endpoint, value, MarkAttributeDirty::IfChanged);
+}
+
 {{#if isNullable}}
-Protocols::InteractionModel::Status SetNull(chip::EndpointId endpoint)
+Protocols::InteractionModel::Status SetNull(chip::EndpointId endpoint, MarkAttributeDirty markDirty)
 {
     {{#if (isString type)}}
     uint8_t zclString[{{>sizingBytes}}] = { {{#if (isShortString type)}}0xFF{{else}}0xFF, 0xFF{{/if}} };
-    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, zclString, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE);
+    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, zclString, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE, markDirty);
     {{else}}
     using Traits = NumericAttributeTraits<{{accessorTraitType type}}>;
     Traits::StorageType value;
     Traits::SetNull(value);
     uint8_t * writable = Traits::ToAttributeStoreRepresentation(value);
-    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, writable, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE);
+    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, writable, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE, markDirty);
     {{/if}}
 }
 
-Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name isArgument=true forceNotOptional=true}} value)
+Protocols::InteractionModel::Status SetNull(chip::EndpointId endpoint)
+{
+    return SetNull(endpoint, MarkAttributeDirty::IfChanged);
+}
+
+Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name isArgument=true forceNotOptional=true}} value, MarkAttributeDirty markDirty)
 {
   if (value.IsNull()) {
-    return SetNull(endpoint);
+    return SetNull(endpoint, markDirty);
   }
 
-  return Set(endpoint, value.Value());
+  return Set(endpoint, value.Value(), markDirty);
+}
+
+Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name isArgument=true forceNotOptional=true}} value)
+{
+    return Set(endpoint, value, MarkAttributeDirty::IfChanged);
 }
 {{/if}}
 

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

@@ -16,6 +16,12 @@
 
 namespace chip {
 namespace app {
+
+enum class MarkAttributeDirty {
+    IfChanged,
+    No,
+};
+
 namespace Clusters {
 
 {{#zcl_clusters}}
@@ -28,10 +34,15 @@ namespace Attributes {
 {{#unless (isStrEqual storagePolicy "attributeAccessInterface")}}
 namespace {{asUpperCamelCase label}} {
 Protocols::InteractionModel::Status Get(chip::EndpointId endpoint, {{accessorGetterType this}} value); // {{type}}
+{{! NOTE: Adding an optional arg instead of an overload can break API
+    consumers that are using the function type (e.g. in templates). }}
 Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name forceNotNullable=true forceNotOptional=true}} value);
+Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name forceNotNullable=true forceNotOptional=true}} value, MarkAttributeDirty markDirty);
 {{#if isNullable}}
 Protocols::InteractionModel::Status SetNull(chip::EndpointId endpoint);
+Protocols::InteractionModel::Status SetNull(chip::EndpointId endpoint, MarkAttributeDirty markDirty);
 Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name isArgument=true forceNotOptional=true}} value);
+Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name isArgument=true forceNotOptional=true}} value, MarkAttributeDirty markDirty);
 {{/if}}
 } // namespace {{asUpperCamelCase label}}
 

src/controller/python/test/test_scripts/base.py

@@ -749,9 +749,9 @@ def OnValueChange(path: Attribute.TypedAttributePath, transaction: Attribute.Sub
 
         #
         # Now write the attribute from fabric2, give it some time before checking if the report
-        # was received.
+        # was received.  Use a different value from before, so there is an actual change.
         #
-        await self.devCtrl2.WriteAttribute(nodeid, [(1, Clusters.UnitTesting.Attributes.Int8u(4))])
+        await self.devCtrl2.WriteAttribute(nodeid, [(1, Clusters.UnitTesting.Attributes.Int8u(5))])
         time.sleep(2)
 
         sub.Shutdown()
@@ -761,7 +761,8 @@ def OnValueChange(path: Attribute.TypedAttributePath, transaction: Attribute.Sub
             return False
 
         #
-        # Do the same test again, but reversing the roles of fabric1 and fabric2.
+        # Do the same test again, but reversing the roles of fabric1 and fabric2.  And again
+        # writing a different value, so there is an actual value change.
         #
         self.logger.info("Testing fabric-isolated CASE eviction (reverse)")
 
@@ -773,7 +774,7 @@ def OnValueChange(path: Attribute.TypedAttributePath, transaction: Attribute.Sub
             self.devCtrl.CloseSession(nodeid)
             await self.devCtrl.ReadAttribute(nodeid, [(Clusters.BasicInformation.Attributes.ClusterRevision)])
 
-        await self.devCtrl.WriteAttribute(nodeid, [(1, Clusters.UnitTesting.Attributes.Int8u(4))])
+        await self.devCtrl.WriteAttribute(nodeid, [(1, Clusters.UnitTesting.Attributes.Int8u(6))])
         time.sleep(2)
 
         sub.Shutdown()