Optimize out no-op writes in attribute-table. (#31162)

* Optimize out no-op writes in attribute-table.

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

Does best-effort optimizing out of string values that are unchanged, only for
strings that we know can fit into our fixed-size buffer.

Fixes #29136

* Address review comments.

* Fix things so going from null to empty string is considered a change.

Author: bzbarsky-apple

.github/workflows/tests.yaml

@@ -249,7 +249,7 @@ jobs:
                      --chip-tool ./out/linux-x64-chip-tool${CHIP_TOOL_VARIANT}-${BUILD_VARIANT}/chip-tool \
                      run \
                      --iterations 1 \
-                     --expected-failures 2 \
+                     --expected-failures 3 \
                      --keep-going \
                      --test-timeout-seconds 120 \
                      --all-clusters-app ./out/linux-x64-all-clusters-${BUILD_VARIANT}/chip-all-clusters-app \
@@ -400,7 +400,7 @@ jobs:
                      --chip-tool ./out/darwin-x64-chip-tool${CHIP_TOOL_VARIANT}-${BUILD_VARIANT}/chip-tool \
                      run \
                      --iterations 1 \
-                     --expected-failures 2 \
+                     --expected-failures 3 \
                      --keep-going \
                      --test-timeout-seconds 120 \
                      --all-clusters-app ./out/darwin-x64-all-clusters-${BUILD_VARIANT}/chip-all-clusters-app \

scripts/tests/chiptest/__init__.py

@@ -277,6 +277,7 @@ def _GetPurposefulFailureTests() -> Set[str]:
     """Tests that fail in YAML on purpose."""
     return {
         "TestPurposefulFailureEqualities.yaml",
+        "TestPurposefulFailureExtraReportingOnToggle.yaml",
         "TestPurposefulFailureNotNullConstraint.yaml",
     }
 

src/app/tests/suites/TestPurposefulFailureExtraReportingOnToggle.yaml

@@ -0,0 +1,58 @@
+# Copyright (c) 2024 Project CHIP Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+name:
+    Test that "passes" if turning on On/Off cluster reports Level Control things
+
+config:
+    nodeId: 0x12344321
+    cluster: "On/Off"
+    endpoint: 1
+    # We expect our test to time out, so set a timeout that's not too long, but
+    # long enough that if the server does report the attribute change we will
+    # almost certianly see it.
+    timeout: 5
+
+tests:
+    - label: "Wait for the commissioned device to be retrieved"
+      cluster: "DelayCommands"
+      command: "WaitForCommissionee"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: nodeId
+
+    - label: "Turn off the light"
+      command: "Off"
+
+    - label: "Subscribe LevelControl RemainingTime Attribute"
+      command: "subscribeAttribute"
+      cluster: "LevelControl"
+      attribute: "RemainingTime"
+      minInterval: 0
+      maxInterval: 5
+      response:
+          value: 0
+
+    - label: "Turn on the light to see attribute change, if any"
+      command: "On"
+
+    - label: "Check for attribute report"
+      command: "waitForReport"
+      cluster: "LevelControl"
+      attribute: "RemainingTime"
+      # This test should fail, since there should be no reporting for an
+      # attribute that did not actually change.
+      response:
+          value: 0

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/attribute-table-detail.h

@@ -30,37 +30,3 @@
 chip::Protocols::InteractionModel::Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId cluster,
                                                                      chip::AttributeId attributeID, uint8_t * dataPtr,
                                                                      EmberAfAttributeType dataType);
-
-/**
- * @brief write an attribute, performing all the checks.
- *
- * This function will attempt to write the attribute value from
- * the provided pointer. This function will only check that the
- * attribute exists. If it does it will write the value into
- * the attribute table for the given attribute.
- *
- * This function will not check to see if the attribute is
- * writable since the read only / writable characteristic
- * of an attribute only pertains to external devices writing
- * over the air. Because this function is being called locally
- * it assumes that the device knows what it is doing and has permission
- * to perform the given operation.
- *
- * if true is passed in for overrideReadOnlyAndDataType then the data type is
- * not checked and the read-only flag is ignored. This mode is meant for
- * testing or setting the initial value of the attribute on the device.
- *
- * this returns:
- * - Status::UnsupportedEndpoint: if endpoint isn't supported by the device.
- * - Status::UnsupportedCluster: if cluster isn't supported on the endpoint.
- * - Status::UnsupportedAttribute: if attribute isn't supported in the cluster.
- * - Status::InvalidDataType: if the data type passed in doesnt match the type
- *           stored in the attribute table
- * - Status::UnsupportedWrite: if the attribute isnt writable
- * - Status::ConstraintError: if the value is set out of the allowable range for
- *           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);

src/app/util/attribute-table.cpp

@@ -21,6 +21,7 @@
 #include <app/util/attribute-storage-detail.h>
 #include <app/util/attribute-storage.h>
 #include <app/util/config.h>
+#include <app/util/ember-strings.h>
 #include <app/util/generic-callbacks.h>
 #include <app/util/odd-sized-integers.h>
 #include <lib/core/CHIPConfig.h>
@@ -37,9 +38,9 @@
 using chip::Protocols::InteractionModel::Status;
 
 using namespace chip;
+using namespace chip::app;
 
 namespace {
-
 // Zigbee spec says types between signed 8 bit and signed 64 bit
 bool emberAfIsTypeSigned(EmberAfAttributeType dataType)
 {
@@ -134,18 +135,58 @@ int8_t emberAfCompareValues(const uint8_t * val1, const uint8_t * val2, uint16_t
     return 0;
 }
 
-} // namespace
+/**
+ * @brief write an attribute, performing all the checks.
+ *
+ * This function will attempt to write the attribute value from
+ * the provided pointer. This function will only check that the
+ * attribute exists. If it does it will write the value into
+ * the attribute table for the given attribute.
+ *
+ * This function will not check to see if the attribute is
+ * writable since the read only / writable characteristic
+ * of an attribute only pertains to external devices writing
+ * over the air. Because this function is being called locally
+ * it assumes that the device knows what it is doing and has permission
+ * to perform the given operation.
+ *
+ * if true is passed in for overrideReadOnlyAndDataType then the data type is
+ * not checked and the read-only flag is ignored. This mode is meant for
+ * testing or setting the initial value of the attribute on the device.
+ *
+ * this returns:
+ * - Status::UnsupportedEndpoint: if endpoint isn't supported by the device.
+ * - Status::UnsupportedCluster: if cluster isn't supported on the endpoint.
+ * - Status::UnsupportedAttribute: if attribute isn't supported in the cluster.
+ * - Status::InvalidDataType: if the data type passed in doesnt match the type
+ *           stored in the attribute table
+ * - Status::UnsupportedWrite: if the attribute isnt writable
+ * - Status::ConstraintError: if the value is set out of the allowable range for
+ *           the attribute
+ * - Status::Success: if the attribute was found and successfully written
+ */
+Status emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * data,
+                          EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType, MarkAttributeDirty markDirty);
+} // anonymous namespace
 
 Status emAfWriteAttributeExternal(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
                                   EmberAfAttributeType dataType)
 {
-    return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, false /* override read-only */);
+    return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, false /* override read-only */,
+                              MarkAttributeDirty::kIfChanged);
 }
 
 Status emberAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
                              EmberAfAttributeType dataType)
 {
-    return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, true /* override read-only */);
+    return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, true /* override read-only */,
+                              MarkAttributeDirty::kIfChanged);
+}
+
+Status emberAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
+                             EmberAfAttributeType dataType, MarkAttributeDirty markDirty)
+{
+    return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, true /* override read-only */, markDirty);
 }
 
 //------------------------------------------------------------------------------
@@ -207,8 +248,78 @@ 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,
+                                const EmberAfAttributeMetadata * metadata, uint8_t * newValueData, bool * isChanging)
+{
+    EmberAfAttributeType attributeType = metadata->attributeType;
+
+    // We don't know how to size our buffer for strings in general, but if the
+    // string happens to fit into our fixed-size buffer, great.
+    size_t valueSize               = metadata->size;
+    constexpr size_t kMaxValueSize = 16; // ipv6adr
+    if (valueSize > kMaxValueSize)
+    {
+        if (emberAfIsStringAttributeType(attributeType) || emberAfIsLongStringAttributeType(attributeType))
+        {
+            // It's a string that may not fit in our buffer.  Just claim it's
+            // changing, since we have no way to tell.
+            *isChanging = true;
+            return Status::Success;
+        }
+
+        // Very much unexpected
+        ChipLogError(Zcl, "Attribute type %d has too-large size %u", attributeType, static_cast<unsigned>(valueSize));
+        return Status::ConstraintError;
+    }
+
+    uint8_t oldValueBuffer[kMaxValueSize];
+    // Cast to uint16_t is safe, because we checked valueSize <= kMaxValueSize above.
+    if (emberAfReadAttribute(endpoint, cluster, attributeID, oldValueBuffer, static_cast<uint16_t>(valueSize)) != Status::Success)
+    {
+        // We failed to read the old value, so flag the value as changing to be safe.
+        *isChanging = true;
+        return Status::Success;
+    }
+
+    if (emberAfIsStringAttributeType(attributeType))
+    {
+        size_t oldLength = emberAfStringLength(oldValueBuffer);
+        size_t newLength = emberAfStringLength(newValueData);
+        // The first byte of the buffer is the string length, and
+        // oldLength/newLength refer to the number of bytes after that.  We want
+        // to include that first byte in our comparison, because null and empty
+        // string have different values there but both return 0 from
+        // emberAfStringLength.
+        *isChanging = (oldLength != newLength) || (memcmp(oldValueBuffer, newValueData, oldLength + 1) != 0);
+    }
+    else if (emberAfIsLongStringAttributeType(attributeType))
+    {
+        size_t oldLength = emberAfLongStringLength(oldValueBuffer);
+        size_t newLength = emberAfLongStringLength(newValueData);
+        // The first two bytes of the buffer are the string length, and
+        // oldLength/newLength refer to the number of bytes after that.  We want
+        // to include those first two bytes in our comparison, because null and
+        // empty string have different values there but both return 0 from
+        // emberAfLongStringLength.
+        *isChanging = (oldLength != newLength) || (memcmp(oldValueBuffer, newValueData, oldLength + 2) != 0);
+    }
+    else
+    {
+        *isChanging = (memcmp(newValueData, oldValueBuffer, valueSize) != 0);
+    }
+
+    return Status::Success;
+}
+
 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;
@@ -286,12 +397,25 @@ Status emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId at
         }
     }
 
+    // Check whether anything is actually changing, before we do any work here.
+    bool valueChanging;
+    Status imStatus = AttributeValueIsChanging(endpoint, cluster, attributeID, metadata, data, &valueChanging);
+    if (imStatus != Status::Success)
+    {
+        return imStatus;
+    }
+
+    if (!valueChanging)
+    {
+        // 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;
@@ -328,7 +452,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::kNo)
+    {
+        MatterReportingAttributeChangeCallback(endpoint, cluster, attributeID);
+    }
 
     // Post write attribute callback for all attributes changes, regardless
     // of cluster.
@@ -341,6 +468,8 @@ Status emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId at
     return Status::Success;
 }
 
+} // anonymous namespace
+
 Status emberAfReadAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr, uint16_t readLength)
 {
     const EmberAfAttributeMetadata * metadata = nullptr;

src/app/util/attribute-table.h

@@ -17,6 +17,7 @@
 
 #pragma once
 
+#include <app/util/af-types.h>
 #include <app/util/attribute-metadata.h>
 #include <lib/core/DataModelTypes.h>
 #include <protocols/interaction_model/StatusCode.h>
@@ -44,6 +45,17 @@ chip::Protocols::InteractionModel::Status emberAfWriteAttribute(chip::EndpointId
                                                                 chip::AttributeId attributeID, uint8_t * dataPtr,
                                                                 EmberAfAttributeType dataType);
 
+/**
+ * A version of emberAfWriteAttribute that allows controlling when the attribute
+ * should be marked dirty.  This is an overload, not an optional argument, to
+ * reduce codesize at all the callsites that want to write without doing
+ * anything special to control the dirty marking.
+ */
+chip::Protocols::InteractionModel::Status emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster,
+                                                                chip::AttributeId attributeID, uint8_t * dataPtr,
+                                                                EmberAfAttributeType dataType,
+                                                                chip::app::MarkAttributeDirty markDirty);
+
 /**
  * @brief Read the attribute value, performing all the checks.
  *