Enable the use of the ServerClusterInterfaceRegistry in Codegen Provider

src/app/server-cluster/DefaultServerCluster.cpp

@@ -106,15 +106,6 @@ void DefaultServerCluster::NotifyAttributeChanged(AttributeId attributeId)
     mContext->interactionContext->dataModelChangeListener->MarkDirty({ path.mEndpointId, path.mClusterId, attributeId });
 }
 
-void DefaultServerCluster::NotifyAllAttributesChanged()
-{
-    IncreaseDataVersion();
-
-    VerifyOrReturn(mContext != nullptr);
-    const ConcreteClusterPath path = GetPath();
-    mContext->interactionContext->dataModelChangeListener->MarkDirty({ path.mEndpointId, path.mClusterId });
-}
-
 BitFlags<ClusterQualityFlags> DefaultServerCluster::GetClusterFlags() const
 {
     return {};

src/app/server-cluster/DefaultServerCluster.h

@@ -95,12 +95,6 @@ class DefaultServerCluster : public ServerClusterInterface
     /// notify that the attribute has changed.
     void NotifyAttributeChanged(AttributeId attributeId);
 
-    /// A "bulk update" notification, that notifies a wildcard attribute change.
-    ///
-    /// Increases cluster data version and if a cluster context is available, it will notify
-    /// that the cluster has changed.
-    void NotifyAllAttributesChanged();
-
 private:
     DataVersion mDataVersion; // will be random-initialized as per spec
 };

src/app/server-cluster/ServerClusterContext.h

@@ -34,9 +34,11 @@ namespace app {
 /// for every cluster maintaining such a context.
 struct ServerClusterContext
 {
-    DataModel::Provider * provider                          = nullptr; /// underlying provider that the cluster operates in
-    PersistentStorageDelegate * storage                     = nullptr; /// read/write persistent storage
-    DataModel::InteractionModelContext * interactionContext = nullptr; /// outside-world communication
+    DataModel::Provider * const provider                          = nullptr; /// underlying provider that the cluster operates in
+    PersistentStorageDelegate * const storage                     = nullptr; /// read/write persistent storage
+    DataModel::InteractionModelContext * const interactionContext = nullptr; /// outside-world communication
+
+    ServerClusterContext(ServerClusterContext &&) = default;
 
     bool operator!=(const ServerClusterContext & other) const
     {

src/app/server-cluster/ServerClusterInterface.h

@@ -45,7 +45,7 @@ class ServerClusterInterface
 
     /// Starts up the server cluster interface.
     ///
-    /// The `context` lifetime is guaranteed to be valid
+    /// The `context` lifetime must be guaranteed to last
     /// until `Shutdown` is called.
     virtual CHIP_ERROR Startup(ServerClusterContext * context) = 0;
 

src/app/server-cluster/testing/EmptyProvider.cpp

@@ -35,34 +35,36 @@ CHIP_ERROR EmptyProvider::Endpoints(ListBuilder<app::DataModel::EndpointEntry> &
 
 CHIP_ERROR EmptyProvider::SemanticTags(EndpointId endpointId, ListBuilder<SemanticTag> & builder)
 {
-    return CHIP_NO_ERROR;
+    return CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint);
 }
 CHIP_ERROR EmptyProvider::DeviceTypes(EndpointId endpointId, ListBuilder<app::DataModel::DeviceTypeEntry> & builder)
 {
-    return CHIP_NO_ERROR;
+    return CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint);
 }
 
 CHIP_ERROR EmptyProvider::ClientClusters(EndpointId endpointId, ListBuilder<ClusterId> & builder)
 {
-    return CHIP_NO_ERROR;
+    return CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint);
 }
 CHIP_ERROR EmptyProvider::ServerClusters(EndpointId endpointId, ListBuilder<app::DataModel::ServerClusterEntry> & builder)
 {
-    return CHIP_NO_ERROR;
+    return CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint);
 }
 
 CHIP_ERROR EmptyProvider::Attributes(const app::ConcreteClusterPath & path, ListBuilder<app::DataModel::AttributeEntry> & builder)
 {
-    return CHIP_NO_ERROR;
+    return CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint);
 }
+
 CHIP_ERROR EmptyProvider::GeneratedCommands(const app::ConcreteClusterPath & path, ListBuilder<CommandId> & builder)
 {
-    return CHIP_NO_ERROR;
+    return CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint);
 }
+
 CHIP_ERROR EmptyProvider::AcceptedCommands(const app::ConcreteClusterPath & path,
                                            ListBuilder<app::DataModel::AcceptedCommandEntry> & builder)
 {
-    return CHIP_NO_ERROR;
+    return CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint);
 }
 
 void EmptyProvider::Temporary_ReportAttributeChanged(const app::AttributePathParams & path) {}

src/app/server-cluster/testing/EmptyProvider.h

@@ -22,10 +22,12 @@
 namespace chip {
 namespace Test {
 
-/// A provider that is emtpy - everything returns null
+/// A provider that is emtpy - it contains no endpoints and most
+/// calls fail with `Unsupported Endpoint`
 ///
-/// Over time this should be replaced with some code-generated/controllable provider
-/// to allow for better testing.
+/// This is a bare minimum implentation to have somethign that claims to be a `Provider`
+/// however it has no real implementation that is useful. Over time this should be replaced
+/// with some code-generated/controllable provider to allow for better testing.
 class EmptyProvider : public app::DataModel::Provider
 {
 public:

src/app/server-cluster/testing/TestServerClusterContext.h

@@ -42,20 +42,35 @@ class NullActionContext : public app::DataModel::ActionContext
 ///   At thist time, `interactionContext::actionContext::CurrentExchange` WILL return nullptr
 ///   in the existing implementation as the exchange is too heavy of an object
 ///   to create for testing
-class TestServerClusterContext : public app::ServerClusterContext
+class TestServerClusterContext
 {
 public:
-    TestServerClusterContext()
+    TestServerClusterContext() :
+        mContext{
+            .provider           = &mTestProvider,
+            .storage            = &mTestStorage,
+            .interactionContext = &mTestContext,
+        }
     {
         mTestContext.eventsGenerator         = &mTestEventsGenerator;
         mTestContext.dataModelChangeListener = &mTestDataModelChangeListener;
         mTestContext.actionContext           = &mNullActionContext;
-
-        provider           = &mTestProvider;
-        storage            = &mTestStorage;
-        interactionContext = &mTestContext;
     }
 
+    /// Get a stable pointer to the underlying context
+    app::ServerClusterContext * Get() { return &mContext; }
+
+    /// Create a new context bound to this test context
+    app::ServerClusterContext Create()
+    {
+        return {
+            .provider           = &mTestProvider,
+            .storage            = &mTestStorage,
+            .interactionContext = &mTestContext,
+
+        };
+    };
+
     LogOnlyEvents & EventsGenerator() { return mTestEventsGenerator; }
     TestProviderChangeListener & ChangeListener() { return mTestDataModelChangeListener; }
     TestPersistentStorageDelegate & StorageDelegate() { return mTestStorage; }
@@ -68,6 +83,8 @@ class TestServerClusterContext : public app::ServerClusterContext
     TestPersistentStorageDelegate mTestStorage;
 
     app::DataModel::InteractionModelContext mTestContext;
+
+    app::ServerClusterContext mContext;
 };
 
 } // namespace Test

src/app/server-cluster/tests/TestDefaultServerCluster.cpp

@@ -14,6 +14,7 @@
  *    See the License for the specific language governing permissions and
  *    limitations under the License.
  */
+#include "app/server-cluster/ServerClusterContext.h"
 #include <pw_unit_test/framework.h>
 
 #include <access/Privilege.h>
@@ -66,7 +67,6 @@ class FakeDefaultServerCluster : public DefaultServerCluster
 
     void TestIncreaseDataVersion() { IncreaseDataVersion(); }
     void TestNotifyAttributeChanged(AttributeId attributeId) { NotifyAttributeChanged(attributeId); }
-    void TestNotifyAllAttributesChanged() { NotifyAllAttributesChanged(); }
 
 private:
     ConcreteClusterPath mPath;
@@ -190,7 +190,7 @@ TEST(TestDefaultServerCluster, NotifyAttributeChanged)
 
     // Create a ServerClusterContext and verify that attribute change notifications are processed.
     TestServerClusterContext context;
-    ASSERT_EQ(cluster.Startup(&context), CHIP_NO_ERROR);
+    ASSERT_EQ(cluster.Startup(context.Get()), CHIP_NO_ERROR);
 
     oldVersion = cluster.GetDataVersion();
     cluster.TestNotifyAttributeChanged(234);
@@ -199,28 +199,3 @@ TEST(TestDefaultServerCluster, NotifyAttributeChanged)
     ASSERT_EQ(context.ChangeListener().DirtyList().size(), 1u);
     ASSERT_EQ(context.ChangeListener().DirtyList()[0], AttributePathParams(kEndpointId, kClusterId, 234));
 }
-
-TEST(TestDefaultServerCluster, NotifyAllAttributesChanged)
-{
-    constexpr EndpointId kEndpointId = 321;
-    constexpr ClusterId kClusterId   = 1122;
-    FakeDefaultServerCluster cluster({ kEndpointId, kClusterId });
-
-    // When no ServerClusterContext is set, only the data version should change.
-    DataVersion oldVersion = cluster.GetDataVersion();
-
-    cluster.TestNotifyAllAttributesChanged();
-    ASSERT_NE(cluster.GetDataVersion(), oldVersion);
-
-    // Create a ServerClusterContext and verify that attribute change notifications are processed.
-    TestServerClusterContext context;
-    ASSERT_EQ(cluster.Startup(&context), CHIP_NO_ERROR);
-
-    oldVersion = cluster.GetDataVersion();
-    cluster.TestNotifyAllAttributesChanged();
-    ASSERT_NE(cluster.GetDataVersion(), oldVersion);
-
-    // When all attributes are changed, a wildcard should be used in the list.
-    ASSERT_EQ(context.ChangeListener().DirtyList().size(), 1u);
-    ASSERT_EQ(context.ChangeListener().DirtyList()[0], AttributePathParams(kEndpointId, kClusterId));
-}

src/data-model-providers/codegen/CodegenDataModelProvider.cpp

@@ -40,13 +40,15 @@
 #include <app/util/persistence/AttributePersistenceProvider.h>
 #include <app/util/persistence/DefaultAttributePersistenceProvider.h>
 #include <data-model-providers/codegen/EmberMetadata.h>
+#include <iterator>
 #include <lib/core/CHIPError.h>
 #include <lib/core/DataModelTypes.h>
 #include <lib/support/CodeUtils.h>
 #include <lib/support/ScopedBuffer.h>
 #include <lib/support/SpanSearchValue.h>
 
 #include <cstdint>
+#include <iterator>
 #include <optional>
 
 namespace chip {
@@ -164,16 +166,11 @@ CHIP_ERROR CodegenDataModelProvider::Startup(DataModel::InteractionModelContext
 
     InitDataModelForTesting();
 
-    {
-        ServerClusterContext clusterContext;
-        clusterContext.provider           = this;
-        clusterContext.storage            = mPersistentStorageDelegate;
-        clusterContext.interactionContext = &mContext;
-
-        ReturnErrorOnFailure(mRegistry.SetContext(clusterContext));
-    }
-
-    return CHIP_NO_ERROR;
+    return mRegistry.SetContext(ServerClusterContext{
+        .provider           = this,
+        .storage            = mPersistentStorageDelegate,
+        .interactionContext = &mContext,
+    });
 }
 
 std::optional<DataModel::ActionReturnStatus> CodegenDataModelProvider::InvokeCommand(const DataModel::InvokeRequest & request,
@@ -267,25 +264,21 @@ CHIP_ERROR CodegenDataModelProvider::ServerClusters(EndpointId endpointId,
     VerifyOrReturnValue(endpoint->clusterCount > 0, CHIP_NO_ERROR);
     VerifyOrReturnValue(endpoint->cluster != nullptr, CHIP_NO_ERROR);
 
-    // We have 2 managed lists:
-    //   - ember clusters, that have ember metadata AND ember version
-    //   - `ServerClusterInterfaceRegistry` clusters which MAY have an ember version
-    //     however may also not have one (we should accept server clusters registered
-    //     completely outside ember).
+    // We build the cluster list by merging two lists:
+    //   - mRegistry items from ServerClusterInterfaces
+    //   - ember metadata clusters
     //
-    // As a result, we are merging the lists here. The first list is the registry
-    // as the cluster version from that is authoritative (regardless of what ember
-    // claims). Secondly we add any additional ember clusters.
+    // This is done because `ServerClusterInterface` allows full control for all its metadata,
+    // in particular `data version` and `flags`.
     //
-    // This uses some RAM, however we assume clusters are in the 10s of items only.
-    // so this overflow seems ok.
+    // To allow cluster implementations to be incrementally converted to storing their own data versions,
+    // instead of relying on the out-of-band emberAfDataVersionStorage, first check for clusters that are
+    // using the new data version storage and are registered via ServerClusterInterfaceRegistry, then fill
+    // in the data versions for the rest via the out-of-band mechanism.
 
     // assume the clusters on endpoint does not change in between these two loops
-    size_t registryClusterCount = 0;
-    for (auto * _ : mRegistry.ClustersOnEndpoint(endpointId))
-    {
-        registryClusterCount++;
-    }
+    auto clusters               = mRegistry.ClustersOnEndpoint(endpointId);
+    size_t registryClusterCount = std::distance(clusters.begin(), clusters.end());
 
     ReturnErrorOnFailure(builder.EnsureAppendCapacity(registryClusterCount));
 

src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp

@@ -22,6 +22,7 @@
 #include <lib/core/DataModelTypes.h>
 #include <lib/support/CHIPMem.h>
 #include <lib/support/CodeUtils.h>
+#include <optional>
 
 namespace chip {
 namespace app {
@@ -31,7 +32,7 @@ ServerClusterInterfaceRegistry::~ServerClusterInterfaceRegistry()
     while (mRegistrations != nullptr)
     {
         ServerClusterRegistration * next = mRegistrations->next;
-        if (mContextIsValid)
+        if (mContext.has_value())
         {
             mRegistrations->serverClusterInterface->Shutdown();
         }
@@ -55,9 +56,9 @@ CHIP_ERROR ServerClusterInterfaceRegistry::Register(ServerClusterRegistration &
     // items. We preserve this however we may want to make this optional at some point in time.
     VerifyOrReturnError(Get(path) == nullptr, CHIP_ERROR_DUPLICATE_KEY_ID);
 
-    if (mContextIsValid)
+    if (mContext.has_value())
     {
-        ReturnErrorOnFailure(entry.serverClusterInterface->Startup(&mContext));
+        ReturnErrorOnFailure(entry.serverClusterInterface->Startup(&*mContext));
     }
 
     entry.next     = mRegistrations;
@@ -93,7 +94,7 @@ ServerClusterInterface * ServerClusterInterfaceRegistry::Unregister(const Concre
             }
 
             current->next = nullptr; // Make sure current does not look like part of a list.
-            if (mContextIsValid)
+            if (mContext.has_value())
             {
                 current->serverClusterInterface->Shutdown();
             }
@@ -137,7 +138,7 @@ void ServerClusterInterfaceRegistry::UnregisterAllFromEndpoint(EndpointId endpoi
             ServerClusterRegistration * actual_next = current->next;
 
             current->next = nullptr; // Make sure current does not look like part of a list.
-            if (mContextIsValid)
+            if (mContext.has_value())
             {
                 current->serverClusterInterface->Shutdown();
             }
@@ -178,22 +179,21 @@ ServerClusterInterface * ServerClusterInterfaceRegistry::Get(const ConcreteClust
     return nullptr;
 }
 
-CHIP_ERROR ServerClusterInterfaceRegistry::SetContext(const ServerClusterContext & context)
+CHIP_ERROR ServerClusterInterfaceRegistry::SetContext(ServerClusterContext && context)
 {
-    if (mContextIsValid)
+    if (mContext.has_value())
     {
         // if there is no difference, do not re-initialize.
-        VerifyOrReturnError(mContext != context, CHIP_NO_ERROR);
+        VerifyOrReturnError(*mContext != context, CHIP_NO_ERROR);
         ClearContext();
     }
 
-    mContext         = context;
-    mContextIsValid  = true;
+    mContext.emplace(std::move(context));
     bool had_failure = false;
 
     for (ServerClusterRegistration * registration = mRegistrations; registration != nullptr; registration = registration->next)
     {
-        CHIP_ERROR err = registration->serverClusterInterface->Startup(&mContext);
+        CHIP_ERROR err = registration->serverClusterInterface->Startup(&*mContext);
         if (err != CHIP_NO_ERROR)
         {
 #if CHIP_ERROR_LOGGING
@@ -219,7 +219,7 @@ CHIP_ERROR ServerClusterInterfaceRegistry::SetContext(const ServerClusterContext
 
 void ServerClusterInterfaceRegistry::ClearContext()
 {
-    if (!mContextIsValid)
+    if (!mContext.has_value())
     {
         return;
     }
@@ -228,8 +228,7 @@ void ServerClusterInterfaceRegistry::ClearContext()
         registration->serverClusterInterface->Shutdown();
     }
 
-    mContext        = ServerClusterContext{};
-    mContextIsValid = false;
+    mContext.reset();
 }
 
 } // namespace app