Allow passing in a Path change listener to ember write functions (p…

…roject-chip#35455) * Cleaner usage: no need of a separate function that is used in one place only * Attempt an API update * Fix typos in the Accessors src * Fix typo and regen * More fixes on accessors * Update signature for emAfWriteAttributeExternal * Add a comment about all the checks being vague * Update src/app/util/af-types.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/af-types.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-storage.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/zap-templates/templates/app/attributes/Accessors-src.zapt Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/mock/CodegenEmberMocks.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/zap-templates/templates/app/attributes/Accessors-src.zapt Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/zap-templates/templates/app/attributes/Accessors-src.zapt Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * zap regen and restyle * Update src/app/zap-templates/templates/app/attributes/Accessors-src.zapt Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-table.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-storage.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-storage.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-table.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-table.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-storage.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/util/attribute-table.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Rename ChangedPathListener to AttributesChangedListener * Remove chip:: and chip::app * Update constructors of AttributePathParams and add nodiscard according to the linter to never call const methods without considering their return value * Restyled by clang-format * Remove auto-inserted include * Update again and zap regen: removed extra namespace prefixes in accessors.h/cpp * Add comment about uint8_t non-const usage... * Another rename given that the listener is now an attributes and not a path listener * Update src/app/util/attribute-table.h Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com> * Comment update to talk more about AttributesChangedListener * Restyle --------- Co-authored-by: Andrei Litvin <andreilitvin@google.com> Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
jmeg-sfy · Sep 10, 2024 · 7f78939 · 7f78939
1 parent 86e1974
commit 7f78939
Show file tree

Hide file tree

Showing 19 changed files with 9,092 additions and 8,259 deletions.
diff --git a/src/app/AttributePathParams.h b/src/app/AttributePathParams.h
@@ -30,6 +30,12 @@ class ReadClient;
 #endif // CHIP_CONFIG_ENABLE_READ_CLIENT
 struct AttributePathParams
 {
+    AttributePathParams() = default;
+
+    explicit AttributePathParams(EndpointId aEndpointId) :
+        AttributePathParams(aEndpointId, kInvalidClusterId, kInvalidAttributeId, kInvalidListIndex)
+    {}
+
     //
     // TODO: (Issue #10596) Need to ensure that we do not encode the NodeId over the wire
     // if it is either not 'set', or is set to a value that matches accessing fabric
@@ -50,9 +56,10 @@ struct AttributePathParams
         mClusterId(aClusterId), mAttributeId(aAttributeId), mEndpointId(aEndpointId), mListIndex(aListIndex)
     {}
 
-    AttributePathParams() {}
-
-    bool IsWildcardPath() const { return HasWildcardEndpointId() || HasWildcardClusterId() || HasWildcardAttributeId(); }
+    [[nodiscard]] bool IsWildcardPath() const
+    {
+        return HasWildcardEndpointId() || HasWildcardClusterId() || HasWildcardAttributeId();
+    }
 
     bool operator==(const AttributePathParams & aOther) const
     {
@@ -66,12 +73,12 @@ struct AttributePathParams
      * be wildcard. This does not verify that the attribute being targeted is actually of list type when the list index is not
      * wildcard.
      */
-    bool IsValidAttributePath() const { return HasWildcardListIndex() || !HasWildcardAttributeId(); }
+    [[nodiscard]] bool IsValidAttributePath() const { return HasWildcardListIndex() || !HasWildcardAttributeId(); }
 
-    inline bool HasWildcardEndpointId() const { return mEndpointId == kInvalidEndpointId; }
-    inline bool HasWildcardClusterId() const { return mClusterId == kInvalidClusterId; }
-    inline bool HasWildcardAttributeId() const { return mAttributeId == kInvalidAttributeId; }
-    inline bool HasWildcardListIndex() const { return mListIndex == kInvalidListIndex; }
+    [[nodiscard]] inline bool HasWildcardEndpointId() const { return mEndpointId == kInvalidEndpointId; }
+    [[nodiscard]] inline bool HasWildcardClusterId() const { return mClusterId == kInvalidClusterId; }
+    [[nodiscard]] inline bool HasWildcardAttributeId() const { return mAttributeId == kInvalidAttributeId; }
+    [[nodiscard]] inline bool HasWildcardListIndex() const { return mListIndex == kInvalidListIndex; }
     inline void SetWildcardEndpointId() { mEndpointId = kInvalidEndpointId; }
     inline void SetWildcardClusterId() { mClusterId = kInvalidClusterId; }
     inline void SetWildcardAttributeId()
@@ -80,7 +87,7 @@ struct AttributePathParams
         mListIndex   = kInvalidListIndex;
     }
 
-    bool IsAttributePathSupersetOf(const AttributePathParams & other) const
+    [[nodiscard]] bool IsAttributePathSupersetOf(const AttributePathParams & other) const
     {
         VerifyOrReturnError(HasWildcardEndpointId() || mEndpointId == other.mEndpointId, false);
         VerifyOrReturnError(HasWildcardClusterId() || mClusterId == other.mClusterId, false);
@@ -90,7 +97,7 @@ struct AttributePathParams
         return true;
     }
 
-    bool IsAttributePathSupersetOf(const ConcreteAttributePath & other) const
+    [[nodiscard]] bool IsAttributePathSupersetOf(const ConcreteAttributePath & other) const
     {
         VerifyOrReturnError(HasWildcardEndpointId() || mEndpointId == other.mEndpointId, false);
         VerifyOrReturnError(HasWildcardClusterId() || mClusterId == other.mClusterId, false);

diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp
@@ -386,8 +386,8 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat
     }
     else
     {
-        status = emAfWriteAttributeExternal(request.path.mEndpointId, request.path.mClusterId, request.path.mAttributeId,
-                                            dataBuffer.data(), (*attributeMetadata)->attributeType);
+        status =
+            emAfWriteAttributeExternal(request.path, EmberAfWriteDataInput(dataBuffer.data(), (*attributeMetadata)->attributeType));
     }
 
     if (status != Protocols::InteractionModel::Status::Success)

diff --git a/src/app/codegen-data-model-provider/tests/EmberReadWriteOverride.cpp b/src/app/codegen-data-model-provider/tests/EmberReadWriteOverride.cpp
@@ -17,6 +17,7 @@
 #include "EmberReadWriteOverride.h"
 
 #include <app/util/attribute-storage.h>
+#include <app/util/attribute-table.h>
 #include <app/util/ember-io-storage.h>
 #include <lib/support/Span.h>
 
@@ -100,8 +101,7 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord,
     return Status::Success;
 }
 
-Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID,
-                                  uint8_t * dataPtr, EmberAfAttributeType dataType)
+Status emAfWriteAttributeExternal(const chip::app::ConcreteAttributePath & path, const EmberAfWriteDataInput & input)
 {
     if (gEmberStatusCode != Status::Success)
     {
@@ -110,13 +110,13 @@ Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId clu
 
     // 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());
+    VerifyOrDie(input.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);
+    memcpy(gEmberIoBuffer, input.dataPtr, len);
     gEmberIoBufferFill = len;
 
     return Status::Success;
@@ -125,5 +125,6 @@ Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId clu
 Status emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, uint8_t * dataPtr,
                              EmberAfAttributeType dataType)
 {
-    return emAfWriteAttributeExternal(endpoint, cluster, attributeID, dataPtr, dataType);
+    return emAfWriteAttributeExternal(chip::app::ConcreteAttributePath(endpoint, cluster, attributeID),
+                                      EmberAfWriteDataInput(dataPtr, dataType));
 }
diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp
@@ -855,7 +855,7 @@ CHIP_ERROR Engine::InsertPathIntoDirtySet(const AttributePathParams & aAttribute
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR Engine::SetDirty(AttributePathParams & aAttributePath)
+CHIP_ERROR Engine::SetDirty(const AttributePathParams & aAttributePath)
 {
     BumpDirtySetGeneration();
 

diff --git a/src/app/reporting/Engine.h b/src/app/reporting/Engine.h
@@ -93,7 +93,7 @@ class Engine
     /**
      * Application marks mutated change path and would be sent out in later report.
      */
-    CHIP_ERROR SetDirty(AttributePathParams & aAttributePathParams);
+    CHIP_ERROR SetDirty(const AttributePathParams & aAttributePathParams);
 
     /**
      * @brief

diff --git a/src/app/reporting/reporting.cpp b/src/app/reporting/reporting.cpp
@@ -24,44 +24,23 @@
 using namespace chip;
 using namespace chip::app;
 
-namespace {
-
-void IncreaseClusterDataVersion(const ConcreteClusterPath & aConcreteClusterPath)
-{
-    DataVersion * version = emberAfDataVersionStorage(aConcreteClusterPath);
-    if (version == nullptr)
-    {
-        ChipLogError(DataManagement, "Endpoint %x, Cluster " ChipLogFormatMEI " not found in IncreaseClusterDataVersion!",
-                     aConcreteClusterPath.mEndpointId, ChipLogValueMEI(aConcreteClusterPath.mClusterId));
-    }
-    else
-    {
-        (*(version))++;
-        ChipLogDetail(DataManagement, "Endpoint %x, Cluster " ChipLogFormatMEI " update version to %" PRIx32,
-                      aConcreteClusterPath.mEndpointId, ChipLogValueMEI(aConcreteClusterPath.mClusterId), *(version));
-    }
-}
-
-} // namespace
-
 void MatterReportingAttributeChangeCallback(EndpointId endpoint, ClusterId clusterId, AttributeId attributeId)
 {
     // Attribute writes have asserted this already, but this assert should catch
     // applications notifying about changes from their end.
     assertChipStackLockedByCurrentThread();
 
-    AttributePathParams info;
-    info.mClusterId   = clusterId;
-    info.mAttributeId = attributeId;
-    info.mEndpointId  = endpoint;
-
-    IncreaseClusterDataVersion(ConcreteClusterPath(endpoint, clusterId));
-    InteractionModelEngine::GetInstance()->GetReportingEngine().SetDirty(info);
+    emberAfAttributeChanged(endpoint, clusterId, attributeId, emberAfGlobalInteractionModelAttributesChangedListener());
 }
 
 void MatterReportingAttributeChangeCallback(const ConcreteAttributePath & aPath)
 {
-    return MatterReportingAttributeChangeCallback(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId);
+    // Attribute writes have asserted this already, but this assert should catch
+    // applications notifying about changes from their end.
+    assertChipStackLockedByCurrentThread();
+
+    emberAfAttributeChanged(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId,
+                            emberAfGlobalInteractionModelAttributesChangedListener());
 }
 
 void MatterReportingAttributeChangeCallback(EndpointId endpoint)
@@ -70,10 +49,5 @@ void MatterReportingAttributeChangeCallback(EndpointId endpoint)
     // applications notifying about changes from their end.
     assertChipStackLockedByCurrentThread();
 
-    AttributePathParams info;
-    info.mEndpointId = endpoint;
-
-    // We are adding or enabling a whole endpoint, in this case, we do not touch the cluster data version.
-
-    InteractionModelEngine::GetInstance()->GetReportingEngine().SetDirty(info);
+    emberAfEndpointChanged(endpoint, emberAfGlobalInteractionModelAttributesChangedListener());
 }
diff --git a/src/app/util/af-types.h b/src/app/util/af-types.h
@@ -32,6 +32,7 @@
 
 #include <app/util/attribute-metadata.h> // EmberAfAttributeMetadata
 
+#include <app/AttributePathParams.h>
 #include <app/ConcreteAttributePath.h>
 #include <app/data-model/Nullable.h>
 #include <lib/core/DataModelTypes.h>
@@ -314,5 +315,15 @@ enum class MarkAttributeDirty
     kYes,
 };
 
+/// Notification object of a specific path being changed
+class AttributesChangedListener
+{
+public:
+    virtual ~AttributesChangedListener() = default;
+
+    /// Called when the set of attributes identified by AttributePathParams (which may contain wildcards) is to be considered dirty.
+    virtual void MarkDirty(const AttributePathParams & path) = 0;
+};
+
 } // namespace app
 } // namespace chip