From f65773e03a99da0e8ff005d2bbc41e42c4a1eb7b Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Fri, 6 Dec 2024 12:28:59 -0800 Subject: [PATCH] Use ProviderChangeListener directly to report attrbiute change --- .../CodegenDataModelProvider.h | 17 ++++++++- .../CodegenDataModelProvider_Write.cpp | 36 ++++++++++--------- src/app/data-model-provider/MetadataTypes.h | 18 ---------- src/app/data-model-provider/Provider.h | 9 +++++ src/app/reporting/reporting.cpp | 14 +++++--- src/app/tests/test-interaction-model-api.cpp | 14 ++++++++ src/app/tests/test-interaction-model-api.h | 2 +- .../tests/data_model/DataModelFixtures.cpp | 14 ++++++++ .../tests/data_model/DataModelFixtures.h | 2 +- 9 files changed, 84 insertions(+), 42 deletions(-) diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider.h b/src/app/codegen-data-model-provider/CodegenDataModelProvider.h index 0493272dbbff47..d99bcab9b03e1f 100644 --- a/src/app/codegen-data-model-provider/CodegenDataModelProvider.h +++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider.h @@ -80,6 +80,17 @@ class EnumeratorCommandFinder } }; +class DefaultProviderChangeListener : public DataModel::ProviderChangeListener +{ +public: + explicit DefaultProviderChangeListener(DataModel::Provider & provider) : mProvider(provider) {} + + void MarkDirty(const AttributePathParams & path) override; + +private: + DataModel::Provider & mProvider; +}; + } // namespace detail /// An implementation of `InteractionModel::Model` that relies on code-generation @@ -126,6 +137,8 @@ class CodegenDataModelProvider : public DataModel::Provider }; public: + CodegenDataModelProvider() : mChangeListener(*this) {} + /// clears out internal caching. Especially useful in unit tests, /// where path caching does not really apply (the same path may result in different outcomes) void Reset() @@ -185,7 +198,7 @@ class CodegenDataModelProvider : public DataModel::Provider ConcreteCommandPath FirstGeneratedCommand(const ConcreteClusterPath & cluster) override; ConcreteCommandPath NextGeneratedCommand(const ConcreteCommandPath & before) override; - void Temporary_ReportAttributeChanged(const AttributePathParams & path) override; + DataModel::ProviderChangeListener & GetAttributeChangeReporter() override { return mChangeListener; } private: // Iteration is often done in a tight loop going through all values. @@ -221,6 +234,8 @@ class CodegenDataModelProvider : public DataModel::Provider // Ember requires a persistence provider, so we make sure we can always have something PersistentStorageDelegate * mPersistentStorageDelegate = nullptr; + detail::DefaultProviderChangeListener mChangeListener; + /// Finds the specified ember cluster /// /// Effectively the same as `emberAfFindServerCluster` except with some caching capabilities diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp index 10e03ae4844487..43f896e1d92dcf 100644 --- a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp +++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp @@ -88,6 +88,26 @@ std::optional TryWriteViaAccessInterface(const ConcreteDataAttribute } // namespace +namespace detail { + +void DefaultProviderChangeListener::MarkDirty(const AttributePathParams & path) +{ + ContextAttributesChangeListener change_listener(mProvider.CurrentContext()); + if (path.mClusterId != kInvalidClusterId) + { + emberAfAttributeChanged(path.mEndpointId, path.mClusterId, path.mAttributeId, &change_listener); + } + else + { + // When the path has wildcard cluster Id, call the emberAfEndpointChanged to mark attributes on the given endpoint + // as having changing, but do NOT increase/alter any cluster data versions, as this happens when a bridged endpoint is + // added or removed from a bridge and the cluster data is not changed during the process. + emberAfEndpointChanged(path.mEndpointId, &change_listener); + } +} + +} // namespace detail + DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttributeRequest & request, AttributeValueDecoder & decoder) { @@ -261,21 +281,5 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat return CHIP_NO_ERROR; } -void CodegenDataModelProvider::Temporary_ReportAttributeChanged(const AttributePathParams & path) -{ - ContextAttributesChangeListener change_listener(CurrentContext()); - if (path.mClusterId != kInvalidClusterId) - { - emberAfAttributeChanged(path.mEndpointId, path.mClusterId, path.mAttributeId, &change_listener); - } - else - { - // When the path has wildcard cluster Id, call the emberAfEndpointChanged to mark attributes on the given endpoint - // as having changing, but do NOT increase/alter any cluster data versions, as this happens when a bridged endpoint is - // added or removed from a bridge and the cluster data is not changed during the process. - emberAfEndpointChanged(path.mEndpointId, &change_listener); - } -} - } // namespace app } // namespace chip diff --git a/src/app/data-model-provider/MetadataTypes.h b/src/app/data-model-provider/MetadataTypes.h index 9eeaa7d05049f1..7de97ba6acf714 100644 --- a/src/app/data-model-provider/MetadataTypes.h +++ b/src/app/data-model-provider/MetadataTypes.h @@ -221,24 +221,6 @@ class ProviderMetadataTree // returned as responses. virtual ConcreteCommandPath FirstGeneratedCommand(const ConcreteClusterPath & cluster) = 0; virtual ConcreteCommandPath NextGeneratedCommand(const ConcreteCommandPath & before) = 0; - - /// Workaround function to report attribute change. - /// - /// When this is invoked, the caller is expected to increment the cluster data version, and the attribute path - /// should be marked as `dirty` by the data model provider listener so that the reporter can notify the subscriber - /// of attribute changes. - /// This function should be invoked when attribute managed by attribute access interface is modified but not - /// through an actual Write interaction. - /// For example, if the LastNetworkingStatus attribute changes because the NetworkCommissioning driver detects a - /// network connection status change and calls SetLastNetworkingStatusValue(). The data model provider can recognize - /// this change by invoking this function at the point of change. - /// - /// This is a workaround function as we cannot notify the attribute change to the data model provider. The provider - /// should own its data and versions. - /// - /// TODO: We should remove this function when the AttributeAccessInterface/CommandHandlerInterface is able to report - /// the attribute changes. - virtual void Temporary_ReportAttributeChanged(const AttributePathParams & path) = 0; }; } // namespace DataModel diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index c547b75b81984f..f0d011057ee519 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -97,6 +97,15 @@ class Provider : public ProviderMetadataTree virtual std::optional Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, CommandHandler * handler) = 0; + /// Returns a reference to the ProviderChangeListener used for reporting attribute changes. + /// + /// This is used to notify listeners about changes in attributes managed by the provider. + /// Listeners can be used to react to updates in the underlying data model. + /// + /// It is expected that implementations provide a valid reference, ensuring that + /// attribute change reporting is functional throughout the lifecycle of the Provider. + virtual ProviderChangeListener & GetAttributeChangeReporter() = 0; + private: InteractionModelContext mContext = { nullptr }; }; diff --git a/src/app/reporting/reporting.cpp b/src/app/reporting/reporting.cpp index d0e07544149d6b..ced5bf790c343e 100644 --- a/src/app/reporting/reporting.cpp +++ b/src/app/reporting/reporting.cpp @@ -30,8 +30,9 @@ void MatterReportingAttributeChangeCallback(EndpointId endpoint, ClusterId clust // applications notifying about changes from their end. assertChipStackLockedByCurrentThread(); - InteractionModelEngine::GetInstance()->GetDataModelProvider()->Temporary_ReportAttributeChanged( - AttributePathParams(endpoint, clusterId, attributeId)); + DataModel::ProviderChangeListener & changeListener = + InteractionModelEngine::GetInstance()->GetDataModelProvider()->GetAttributeChangeReporter(); + changeListener.MarkDirty(AttributePathParams(endpoint, clusterId, attributeId)); } void MatterReportingAttributeChangeCallback(const ConcreteAttributePath & aPath) @@ -40,8 +41,9 @@ void MatterReportingAttributeChangeCallback(const ConcreteAttributePath & aPath) // applications notifying about changes from their end. assertChipStackLockedByCurrentThread(); - InteractionModelEngine::GetInstance()->GetDataModelProvider()->Temporary_ReportAttributeChanged( - AttributePathParams(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId)); + DataModel::ProviderChangeListener & changeListener = + InteractionModelEngine::GetInstance()->GetDataModelProvider()->GetAttributeChangeReporter(); + changeListener.MarkDirty(AttributePathParams(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId)); } void MatterReportingAttributeChangeCallback(EndpointId endpoint) @@ -50,5 +52,7 @@ void MatterReportingAttributeChangeCallback(EndpointId endpoint) // applications notifying about changes from their end. assertChipStackLockedByCurrentThread(); - InteractionModelEngine::GetInstance()->GetDataModelProvider()->Temporary_ReportAttributeChanged(AttributePathParams(endpoint)); + DataModel::ProviderChangeListener & changeListener = + InteractionModelEngine::GetInstance()->GetDataModelProvider()->GetAttributeChangeReporter(); + changeListener.MarkDirty(AttributePathParams(endpoint)); } diff --git a/src/app/tests/test-interaction-model-api.cpp b/src/app/tests/test-interaction-model-api.cpp index 71e7d93036005a..c06028657887fd 100644 --- a/src/app/tests/test-interaction-model-api.cpp +++ b/src/app/tests/test-interaction-model-api.cpp @@ -60,6 +60,14 @@ class TestOnlyAttributeValueDecoderAccessor AttributeValueDecoder & mDecoder; }; +class TestProviderChangeListener : public DataModel::ProviderChangeListener +{ +public: + explicit TestProviderChangeListener() {} + + void MarkDirty(const AttributePathParams & path) override {} +}; + // strong defintion in TestCommandInteraction.cpp __attribute__((weak)) void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPath, chip::TLV::TLVReader & aReader, CommandHandler * apCommandObj) @@ -149,6 +157,12 @@ std::optional TestImCustomDataModel::Invoke(const InvokeRequ return std::make_optional(CHIP_ERROR_NOT_IMPLEMENTED); } +ProviderChangeListener & TestImCustomDataModel::GetAttributeChangeReporter() +{ + static TestProviderChangeListener changeListener; + return changeListener; +} + DataModel::EndpointEntry TestImCustomDataModel::FirstEndpoint() { return CodegenDataModelProviderInstance(nullptr /* delegate */)->FirstEndpoint(); diff --git a/src/app/tests/test-interaction-model-api.h b/src/app/tests/test-interaction-model-api.h index 2120ec4304eb8e..a9479dcd77e9ce 100644 --- a/src/app/tests/test-interaction-model-api.h +++ b/src/app/tests/test-interaction-model-api.h @@ -109,6 +109,7 @@ class TestImCustomDataModel : public DataModel::Provider AttributeValueDecoder & decoder) override; std::optional Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override; + DataModel::ProviderChangeListener & GetAttributeChangeReporter() override; DataModel::EndpointEntry FirstEndpoint() override; DataModel::EndpointEntry NextEndpoint(EndpointId before) override; @@ -131,7 +132,6 @@ class TestImCustomDataModel : public DataModel::Provider std::optional GetAcceptedCommandInfo(const ConcreteCommandPath & path) override; ConcreteCommandPath FirstGeneratedCommand(const ConcreteClusterPath & cluster) override; ConcreteCommandPath NextGeneratedCommand(const ConcreteCommandPath & before) override; - void Temporary_ReportAttributeChanged(const AttributePathParams & path) override {} }; } // namespace app diff --git a/src/controller/tests/data_model/DataModelFixtures.cpp b/src/controller/tests/data_model/DataModelFixtures.cpp index c0054043fe4bbf..f2a1281c09b5f6 100644 --- a/src/controller/tests/data_model/DataModelFixtures.cpp +++ b/src/controller/tests/data_model/DataModelFixtures.cpp @@ -67,6 +67,14 @@ class TestOnlyAttributeValueDecoderAccessor AttributeValueDecoder & mDecoder; }; +class TestProviderChangeListener : public DataModel::ProviderChangeListener +{ +public: + explicit TestProviderChangeListener() {} + + void MarkDirty(const AttributePathParams & path) override {} +}; + namespace DataModelTests { ScopedChangeOnly gReadResponseDirective(ReadResponseDirective::kSendDataResponse); @@ -474,6 +482,12 @@ std::optional CustomDataModel::Invoke(const InvokeRequest & return std::nullopt; // handler status is set by the dispatch } +ProviderChangeListener & CustomDataModel::GetAttributeChangeReporter() +{ + static TestProviderChangeListener changeListener; + return changeListener; +} + DataModel::EndpointEntry CustomDataModel::FirstEndpoint() { return CodegenDataModelProviderInstance(nullptr /* delegate */)->FirstEndpoint(); diff --git a/src/controller/tests/data_model/DataModelFixtures.h b/src/controller/tests/data_model/DataModelFixtures.h index 181bdf8fa3bb5c..6d885b4fb822fa 100644 --- a/src/controller/tests/data_model/DataModelFixtures.h +++ b/src/controller/tests/data_model/DataModelFixtures.h @@ -121,6 +121,7 @@ class CustomDataModel : public DataModel::Provider AttributeValueDecoder & decoder) override; std::optional Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override; + DataModel::ProviderChangeListener & GetAttributeChangeReporter() override; DataModel::EndpointEntry FirstEndpoint() override; DataModel::EndpointEntry NextEndpoint(EndpointId before) override; @@ -143,7 +144,6 @@ class CustomDataModel : public DataModel::Provider std::optional GetAcceptedCommandInfo(const ConcreteCommandPath & path) override; ConcreteCommandPath FirstGeneratedCommand(const ConcreteClusterPath & cluster) override; ConcreteCommandPath NextGeneratedCommand(const ConcreteCommandPath & before) override; - void Temporary_ReportAttributeChanged(const AttributePathParams & path) override {} }; } // namespace DataModelTests