Make sure that attribute changed information can be propagated out of ember

andreilitvin · andreilitvin · commit 9aeda71f0749 · 2024-08-06T11:35:30.000-04:00
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -25,6 +25,7 @@
 
 #include "InteractionModelEngine.h"
 #include "lib/core/CHIPError.h"
+#include "platform/LockTracker.h"
 
 #include <cinttypes>
 
@@ -1749,7 +1750,8 @@ DataModel::Provider * InteractionModelEngine::GetDataModelProvider()
 #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
     if (mDataModelProvider == nullptr)
     {
-        // Generally this is expected to be thread safe as these should be called within the CHIP processing loop.
+        // These should be called within the CHIP processing loop.
+        assertChipStackLockedByCurrentThread();
         SetDataModelProvider(CodegenDataModelProviderInstance());
     }
 #endif
diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp
@@ -375,18 +375,24 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat
         return Status::InvalidValue;
     }
 
+    AttributeChanged attributeChanged = AttributeChanged::kValueNotChanged;
+
     if (request.operationFlags.Has(DataModel::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 attribute types) - see attribute-table.h documentation for details.
         status = emberAfWriteAttribute(request.path.mEndpointId, request.path.mClusterId, request.path.mAttributeId,
-                                       dataBuffer.data(), (*attributeMetadata)->attributeType);
+                                       dataBuffer.data(), (*attributeMetadata)->attributeType,
+                                       MarkAttributeDirty::kNo, // Model handles its own dirty handling
+                                       &attributeChanged);
     }
     else
     {
         status = emAfWriteAttributeExternal(request.path.mEndpointId, request.path.mClusterId, request.path.mAttributeId,
-                                            dataBuffer.data(), (*attributeMetadata)->attributeType);
+                                            dataBuffer.data(), (*attributeMetadata)->attributeType,
+                                            MarkAttributeDirty::kNo, // Model handles its own dirty handling
+                                            &attributeChanged);
     }
 
     if (status != Protocols::InteractionModel::Status::Success)
@@ -398,11 +404,11 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat
     //
     // - Internal writes may need to be able to decide if to mark things dirty or not (see AAI as well)
     // - Changes-ommited paths should not be marked dirty (ember is not aware of that flag)
-    // - This likely maps to `MatterReportingAttributeChangeCallback` HOWEVER current ember write functions
-    //   will selectively call that one depending on old attribute state (i.e. calling every time is a
-    //   change in behavior)
-    emberAfIncreaseClusterDataVersion(request.path);
-    CurrentContext().dataModelChangeListener->MarkDirty(request.path);
+    if (attributeChanged == AttributeChanged::kValueChanged)
+    {
+        emberAfIncreaseClusterDataVersion(request.path);
+        CurrentContext().dataModelChangeListener->MarkDirty(request.path);
+    }
     return CHIP_NO_ERROR;
 }
 
diff --git a/src/app/codegen-data-model-provider/tests/EmberReadWriteOverride.cpp b/src/app/codegen-data-model-provider/tests/EmberReadWriteOverride.cpp
@@ -15,6 +15,7 @@
  *    limitations under the License.
  */
 #include "EmberReadWriteOverride.h"
+#include "app/util/af-types.h"
 
 #include <app/util/attribute-storage.h>
 #include <app/util/ember-io-storage.h>
@@ -101,7 +102,8 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord,
 }
 
 Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID,
-                                  uint8_t * dataPtr, EmberAfAttributeType dataType)
+                                  uint8_t * dataPtr, EmberAfAttributeType dataType, chip::app::MarkAttributeDirty markDirty,
+                                  chip::app::AttributeChanged * changed)
 {
     if (gEmberStatusCode != Status::Success)
     {
@@ -116,14 +118,22 @@ Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId clu
     // 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());
+
+    if (changed != nullptr)
+    {
+        *changed = (memcmp(gEmberIoBuffer, dataPtr, len) != 0) ? chip::app::AttributeChanged::kValueChanged
+                                                               : chip::app::AttributeChanged::kValueNotChanged;
+    }
+
     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)
+                             EmberAfAttributeType dataType, chip::app::MarkAttributeDirty markDirty,
+                             chip::app::AttributeChanged * changed)
 {
-    return emAfWriteAttributeExternal(endpoint, cluster, attributeID, dataPtr, dataType);
+    return emAfWriteAttributeExternal(endpoint, cluster, attributeID, dataPtr, dataType, markDirty, changed);
 }
diff --git a/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp
@@ -15,6 +15,7 @@
  *    limitations under the License.
  */
 
+#include "lib/support/logging/TextOnlyLogging.h"
 #include <vector>
 
 #include <pw_unit_test/framework.h>
@@ -808,6 +809,24 @@ void TestEmberScalarTypeWrite(const typename NumericAttributeTraits<T>::WorkingT
         model.ChangeListener().DirtyList().clear();
     }
 
+    // nullable test: write null to make sure content of buffer changed (otherwise it will be a noop for dirty checking)
+    {
+        TestWriteRequest test(
+            kAdminSubjectDescriptor,
+            ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZclType)));
+
+        using NumericType             = NumericAttributeTraits<T>;
+        using NullableType            = chip::app::DataModel::Nullable<typename NumericType::WorkingType>;
+        AttributeValueDecoder decoder = test.DecoderFor<NullableType>(NullableType());
+
+        // write should succeed
+        ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR);
+
+        // dirty: we changed the value to null
+        ASSERT_EQ(model.ChangeListener().DirtyList().size(), 1u);
+        EXPECT_EQ(model.ChangeListener().DirtyList()[0], test.request.path);
+    }
+
     // nullable test
     {
         TestWriteRequest test(
@@ -827,8 +846,9 @@ void TestEmberScalarTypeWrite(const typename NumericAttributeTraits<T>::WorkingT
         typename NumericAttributeTraits<T>::WorkingType actual = NumericAttributeTraits<T>::StorageToWorking(storage);
 
         ASSERT_EQ(actual, value);
-        ASSERT_EQ(model.ChangeListener().DirtyList().size(), 1u);
-        EXPECT_EQ(model.ChangeListener().DirtyList()[0], test.request.path);
+        // dirty a 2nd time when we moved from null to a real value
+        ASSERT_EQ(model.ChangeListener().DirtyList().size(), 2u);
+        EXPECT_EQ(model.ChangeListener().DirtyList()[1], test.request.path);
     }
 }
 
@@ -2436,7 +2456,7 @@ TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceTest)
     // AAI does not prevent read/write of regular attributes
     // validate that once AAI is added, we still can go through writing regular bits (i.e.
     // AAI returning "unknown" has fallback to ember)
-    TestEmberScalarTypeWrite<uint32_t, ZCL_INT32U_ATTRIBUTE_TYPE>(1234);
+    TestEmberScalarTypeWrite<uint32_t, ZCL_INT32U_ATTRIBUTE_TYPE>(4321);
     TestEmberScalarNullWrite<int64_t, ZCL_INT64S_ATTRIBUTE_TYPE>();
 }
 
diff --git a/src/app/util/af-types.h b/src/app/util/af-types.h
@@ -314,5 +314,11 @@ enum class MarkAttributeDirty
     kYes,
 };
 
+enum class AttributeChanged
+{
+    kValueNotChanged,
+    kValueChanged,
+};
+
 } // namespace app
 } // namespace chip
diff --git a/src/app/util/attribute-table-detail.h b/src/app/util/attribute-table-detail.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>
@@ -27,6 +28,8 @@
  * This will check attribute writeability and that
  * the provided data type matches the expected data type.
  */
-chip::Protocols::InteractionModel::Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId cluster,
-                                                                     chip::AttributeId attributeID, uint8_t * dataPtr,
-                                                                     EmberAfAttributeType dataType);
+chip::Protocols::InteractionModel::Status
+emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, uint8_t * dataPtr,
+                           EmberAfAttributeType dataType,
+                           chip::app::MarkAttributeDirty markDirty = chip::app::MarkAttributeDirty::kIfChanged,
+                           chip::app::AttributeChanged * changed   = nullptr);
diff --git a/src/app/util/attribute-table.cpp b/src/app/util/attribute-table.cpp
@@ -14,6 +14,7 @@
  *    See the License for the specific language governing permissions and
  *    limitations under the License.
  */
+#include "app/util/af-types.h"
 #include <app/util/attribute-table.h>
 
 #include <app/util/attribute-table-detail.h>
@@ -167,27 +168,34 @@ int8_t emberAfCompareValues(const uint8_t * val1, const uint8_t * val2, uint16_t
  * - 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);
+                          EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType, MarkAttributeDirty markDirty,
+                          AttributeChanged * wasDirty);
 } // anonymous namespace
 
 Status emAfWriteAttributeExternal(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
-                                  EmberAfAttributeType dataType)
+                                  EmberAfAttributeType dataType, MarkAttributeDirty markDirty, AttributeChanged * changed)
 {
-    return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, false /* override read-only */,
-                              MarkAttributeDirty::kIfChanged);
+    return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, false /* override read-only */, markDirty,
+                              changed);
 }
 
 Status emberAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
                              EmberAfAttributeType dataType)
 {
     return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, true /* override read-only */,
-                              MarkAttributeDirty::kIfChanged);
+                              MarkAttributeDirty::kIfChanged, nullptr);
 }
 
 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);
+    return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, true /* override read-only */, markDirty, nullptr);
+}
+
+Status emberAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
+                             EmberAfAttributeType dataType, MarkAttributeDirty markDirty, AttributeChanged * changed)
+{
+    return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, true /* override read-only */, markDirty, changed);
 }
 
 //------------------------------------------------------------------------------
@@ -320,8 +328,14 @@ Status AttributeValueIsChanging(EndpointId endpoint, ClusterId cluster, Attribut
 }
 
 Status emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * data,
-                          EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType, MarkAttributeDirty markDirty)
+                          EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType, MarkAttributeDirty markDirty,
+                          AttributeChanged * changeState)
 {
+    if (changeState)
+    {
+        *changeState = AttributeChanged::kValueNotChanged;
+    }
+
     const EmberAfAttributeMetadata * metadata = nullptr;
     EmberAfAttributeSearchRecord record;
     record.endpoint    = endpoint;
@@ -454,6 +468,12 @@ Status emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId at
         return status;
     }
 
+    // internal value has changed. Report to the caller.
+    if (changeState != nullptr)
+    {
+        *changeState = AttributeChanged::kValueChanged;
+    }
+
     // Save the attribute to persistent storage if needed
     // The callee will weed out attributes that do not need to be stored.
     emAfSaveAttributeToStorageIfNeeded(data, endpoint, cluster, metadata);
diff --git a/src/app/util/attribute-table.h b/src/app/util/attribute-table.h
@@ -56,6 +56,16 @@ chip::Protocols::InteractionModel::Status emberAfWriteAttribute(chip::EndpointId
                                                                 EmberAfAttributeType dataType,
                                                                 chip::app::MarkAttributeDirty markDirty);
 
+/**
+ * A version of emberAfWriteAttribute that returns back the attribute change state (i.e. if the in-memory
+ * value has actually changed or not).
+ */
+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::AttributeChanged * changed);
+
 /**
  * @brief Read the attribute value, performing all the checks.
  *
diff --git a/src/app/util/mock/CodegenEmberMocks.cpp b/src/app/util/mock/CodegenEmberMocks.cpp
@@ -14,6 +14,7 @@
  *    See the License for the specific language governing permissions and
  *    limitations under the License.
  */
+#include "app/util/af-types.h"
 #include <app/reporting/reporting.h>
 #include <app/util/attribute-storage.h>
 
@@ -35,13 +36,15 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord,
 }
 
 Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID,
-                                  uint8_t * dataPtr, EmberAfAttributeType dataType)
+                                  uint8_t * dataPtr, EmberAfAttributeType dataType, chip::app::MarkAttributeDirty markDirty,
+                                  chip::app::AttributeChanged * changed)
 {
     return Status::Success;
 }
 
 Status emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, uint8_t * dataPtr,
-                             EmberAfAttributeType dataType)
+                             EmberAfAttributeType dataType, chip::app::MarkAttributeDirty markDirty,
+                             chip::app::AttributeChanged * changed)
 {
-    return emAfWriteAttributeExternal(endpoint, cluster, attributeID, dataPtr, dataType);
+    return emAfWriteAttributeExternal(endpoint, cluster, attributeID, dataPtr, dataType, markDirty, changed);
 }
diff --git a/src/darwin/Framework/CHIP/ServerEndpoint/MTRIMDispatch.mm b/src/darwin/Framework/CHIP/ServerEndpoint/MTRIMDispatch.mm
@@ -40,7 +40,9 @@ void emberAfClusterInitCallback(EndpointId endpoint, ClusterId clusterId)
 }
 
 Protocols::InteractionModel::Status emAfWriteAttributeExternal(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
-    EmberAfAttributeType dataType)
+    EmberAfAttributeType dataType,
+    chip::app::MarkAttributeDirty markDirty,
+    chip::app::AttributeChanged * changed)
 {
     assertChipStackLockedByCurrentThread();
 

Original file line number	Diff line number	Diff line change
`@@ -25,6 +25,7 @@`
`25`	`25`
`26`	`26`	`#include "InteractionModelEngine.h"`
`27`	`27`	`#include "lib/core/CHIPError.h"`
	`28`	`+#include "platform/LockTracker.h"`
`28`	`29`
`29`	`30`	`#include <cinttypes>`
`30`	`31`
`@@ -1749,7 +1750,8 @@ DataModel::Provider * InteractionModelEngine::GetDataModelProvider()`
`1749`	`1750`	`#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE`
`1750`	`1751`	`if (mDataModelProvider == nullptr)`
`1751`	`1752`	`{`
`1752`		`- // Generally this is expected to be thread safe as these should be called within the CHIP processing loop.`
	`1753`	`+ // These should be called within the CHIP processing loop.`
	`1754`	`+ assertChipStackLockedByCurrentThread();`
`1753`	`1755`	`SetDataModelProvider(CodegenDataModelProviderInstance());`
`1754`	`1756`	`}`
`1755`	`1757`	`#endif`
Original file line number	Diff line number	Diff line change
`@@ -14,6 +14,7 @@`
`14`	`14`	`* See the License for the specific language governing permissions and`
`15`	`15`	`* limitations under the License.`
`16`	`16`	`*/`
	`17`	`+#include "app/util/af-types.h"`
`17`	`18`	`#include <app/reporting/reporting.h>`
`18`	`19`	`#include <app/util/attribute-storage.h>`
`19`	`20`
`@@ -35,13 +36,15 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord,`
`35`	`36`	`}`
`36`	`37`
`37`	`38`	`Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID,`
`38`		`- uint8_t * dataPtr, EmberAfAttributeType dataType)`
	`39`	`+ uint8_t * dataPtr, EmberAfAttributeType dataType, chip::app::MarkAttributeDirty markDirty,`
	`40`	`+ chip::app::AttributeChanged * changed)`
`39`	`41`	`{`
`40`	`42`	`return Status::Success;`
`41`	`43`	`}`
`42`	`44`
`43`	`45`	`Status emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, uint8_t * dataPtr,`
`44`		`- EmberAfAttributeType dataType)`
	`46`	`+ EmberAfAttributeType dataType, chip::app::MarkAttributeDirty markDirty,`
	`47`	`+ chip::app::AttributeChanged * changed)`
`45`	`48`	`{`
`46`		`- return emAfWriteAttributeExternal(endpoint, cluster, attributeID, dataPtr, dataType);`
	`49`	`+ return emAfWriteAttributeExternal(endpoint, cluster, attributeID, dataPtr, dataType, markDirty, changed);`
`47`	`50`	`}`
Original file line number	Diff line number	Diff line change
`@@ -40,7 +40,9 @@ void emberAfClusterInitCallback(EndpointId endpoint, ClusterId clusterId)`
`40`	`40`	`}`
`41`	`41`
`42`	`42`	`Protocols::InteractionModel::Status emAfWriteAttributeExternal(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,`
`43`		`- EmberAfAttributeType dataType)`
	`43`	`+ EmberAfAttributeType dataType,`
	`44`	`+ chip::app::MarkAttributeDirty markDirty,`
	`45`	`+ chip::app::AttributeChanged * changed)`
`44`	`46`	`{`
`45`	`47`	`assertChipStackLockedByCurrentThread();`
`46`	`48`