SceneHandlers are now stored inside a linked list, reworked the logic…

… for applying scene handlers, added stubs to allow testing

src/app/clusters/scenes/ExtensionFieldSetsImpl.cpp

@@ -110,7 +110,7 @@ CHIP_ERROR ExtensionFieldSetsImpl::InsertFieldSet(const ExtensionFieldSet & fiel
     return CHIP_ERROR_NO_MEMORY;
 }
 
-CHIP_ERROR ExtensionFieldSetsImpl::GetFieldSetAtPosition(ExtensionFieldSet & fieldSet, uint8_t position)
+CHIP_ERROR ExtensionFieldSetsImpl::GetFieldSetAtPosition(ExtensionFieldSet & fieldSet, uint8_t position) const
 {
     VerifyOrReturnError(position < mFieldSetsCount, CHIP_ERROR_BUFFER_TOO_SMALL);
 

src/app/clusters/scenes/ExtensionFieldSetsImpl.h

@@ -136,7 +136,7 @@ class ExtensionFieldSetsImpl : public ExtensionFieldSets
     uint8_t GetFieldSetCount() const override { return mFieldSetsCount; };
 
     CHIP_ERROR InsertFieldSet(const ExtensionFieldSet & field);
-    CHIP_ERROR GetFieldSetAtPosition(ExtensionFieldSet & field, uint8_t position);
+    CHIP_ERROR GetFieldSetAtPosition(ExtensionFieldSet & field, uint8_t position) const;
     CHIP_ERROR RemoveFieldAtPosition(uint8_t position);
 
     // implementation

src/app/clusters/scenes/SceneTable.h

@@ -20,6 +20,7 @@
 #include <app/clusters/scenes/ExtensionFieldSets.h>
 #include <lib/support/CHIPMemString.h>
 #include <lib/support/CommonIterator.h>
+#include <lib/support/IntrusiveList.h>
 #include <lib/support/PersistentData.h>
 #include <lib/support/Span.h>
 
@@ -39,16 +40,15 @@ static constexpr uint8_t kMaxScenePerFabric = CHIP_CONFIG_SCENES_MAX_PER_FABRIC;
 static constexpr size_t kIteratorsMax       = CHIP_CONFIG_MAX_SCENES_CONCURRENT_ITERATORS;
 static constexpr size_t kSceneNameMax       = CHIP_CONFIG_SCENES_CLUSTER_MAXIMUM_NAME_LENGTH;
 
-// Handler's are meant to retrieve a single cluster's extension field set, therefore the maximum number allowed is the maximal
-// number of extension field set.
-static constexpr uint8_t kMaxSceneHandlers = CHIP_CONFIG_SCENES_MAX_SCENE_HANDLERS;
-
 /// @brief Abstract class allowing different Endpoints interactions with the ExtensionFieldSets added to and retrieved from the
 /// scene Table. The Scene Handler's are meant as interface between various clusters and the Scene table. The expected behaviour of
 /// the table with the handler is: Once a scene command involving extension field set is received, the Scene Table will go through
 /// the list of handlers to either retrieve, populate or apply Extension field sets. Each handler is meant to retrieve an extension
 /// field set for a single cluster however it is also possible to use a single generic handler that handles all of them.
-class SceneHandler
+///
+/// @note If more than one handler is implemented for a specific <endpoint, cluster> pair, ONLY THE FIRST HANDLER FOR THAT SPECIFIC
+/// PAIR will get called on executing extension field sets related ations on the scene table.
+class SceneHandler : public IntrusiveListNodeBase<>
 {
 public:
     SceneHandler(){};
@@ -258,13 +258,13 @@ class SceneTable
     virtual CHIP_ERROR RemoveSceneTableEntryAtPosition(FabricIndex fabric_index, SceneIndex scene_idx)                = 0;
 
     // SceneHandlers
-    virtual CHIP_ERROR RegisterHandler(SceneHandler * handler)   = 0;
-    virtual CHIP_ERROR UnregisterHandler(SceneHandler * handler) = 0;
-    virtual CHIP_ERROR UnregisterAllHandlers()                   = 0;
+    virtual void RegisterHandler(SceneHandler * handler)   = 0;
+    virtual void UnregisterHandler(SceneHandler * handler) = 0;
+    virtual void UnregisterAllHandlers()                   = 0;
 
     // Extension field sets operation
-    virtual CHIP_ERROR SceneSaveEFS(SceneTableEntry & scene)                                    = 0;
-    virtual CHIP_ERROR SceneApplyEFS(FabricIndex fabric_index, const SceneStorageId & scene_id) = 0;
+    virtual CHIP_ERROR SceneSaveEFS(SceneTableEntry & scene)        = 0;
+    virtual CHIP_ERROR SceneApplyEFS(const SceneTableEntry & scene) = 0;
 
     // Fabrics
     virtual CHIP_ERROR RemoveFabric(FabricIndex fabric_index) = 0;
@@ -276,11 +276,11 @@ class SceneTable
 
     // Handlers
     virtual bool HandlerListEmpty() { return (mNumHandlers == 0); }
-    virtual bool HandlerListFull() { return (mNumHandlers >= kMaxSceneHandlers); }
     virtual uint8_t GetHandlerNum() { return this->mNumHandlers; }
 
-    SceneHandler * mHandlers[kMaxSceneHandlers] = { nullptr };
-    uint8_t mNumHandlers                        = 0;
+    // SceneHandler * mHandlers[kMaxSceneHandlers] = { nullptr };
+    IntrusiveList<SceneHandler> mHandlerList;
+    uint8_t mNumHandlers = 0;
 };
 
 } // namespace scenes

src/app/clusters/scenes/SceneTableImpl.cpp

@@ -16,6 +16,7 @@
  */
 
 #include <app/clusters/scenes/SceneTableImpl.h>
+#include <app/util/attribute-storage.h>
 #include <lib/support/DefaultStorageKeyAllocator.h>
 #include <stdlib.h>
 
@@ -314,6 +315,7 @@ CHIP_ERROR DefaultSceneTableImpl::Init(PersistentStorageDelegate * storage)
 
 void DefaultSceneTableImpl::Finish()
 {
+    UnregisterAllHandlers();
     mSceneEntryIterators.ReleaseAll();
 }
 
@@ -329,6 +331,7 @@ CHIP_ERROR DefaultSceneTableImpl::SetSceneTableEntry(FabricIndex fabric_index, c
 
     return fabric.SaveScene(mStorage, entry);
 }
+
 CHIP_ERROR DefaultSceneTableImpl::GetSceneTableEntry(FabricIndex fabric_index, SceneStorageId scene_id, SceneTableEntry & entry)
 {
     FabricSceneData fabric(fabric_index);
@@ -343,6 +346,7 @@ CHIP_ERROR DefaultSceneTableImpl::GetSceneTableEntry(FabricIndex fabric_index, S
 
     return CHIP_NO_ERROR;
 }
+
 CHIP_ERROR DefaultSceneTableImpl::RemoveSceneTableEntry(FabricIndex fabric_index, SceneStorageId scene_id)
 {
     FabricSceneData fabric(fabric_index);
@@ -371,124 +375,81 @@ CHIP_ERROR DefaultSceneTableImpl::RemoveSceneTableEntryAtPosition(FabricIndex fa
     return fabric.RemoveScene(mStorage, scene.mStorageId);
 }
 
-/// @brief Register a handler in the handler list
+/// @brief Register a handler in the handler linked list
 /// @param handler Cluster specific handler for extension field sets interaction
-/// @return CHIP_NO_ERROR if handler was registered, CHIP_ERROR_NO_MEMORY if the handler list is full
-CHIP_ERROR DefaultSceneTableImpl::RegisterHandler(SceneHandler * handler)
+void DefaultSceneTableImpl::RegisterHandler(SceneHandler * handler)
 {
-    uint8_t firstEmptyPosition = kInvalidPosition;
 
-    for (uint8_t i = 0; i < kMaxSceneHandlers; i++)
-    {
-        if (this->mHandlers[i] == handler)
-        {
-            this->mHandlers[i] = handler;
-            return CHIP_NO_ERROR;
-        }
-        if (this->mHandlers[i] == nullptr && firstEmptyPosition == kInvalidPosition)
-        {
-            firstEmptyPosition = i;
-        }
-    }
-
-    // if found, insert at found position, otherwise at first free possition, otherwise return error
-    if (firstEmptyPosition < kMaxSceneHandlers)
-    {
-        this->mHandlers[firstEmptyPosition] = handler;
-        this->mNumHandlers++;
-        return CHIP_NO_ERROR;
-    }
-
-    return CHIP_ERROR_NO_MEMORY;
+    mHandlerList.PushFront(handler);
+    mNumHandlers++;
 }
 
-CHIP_ERROR DefaultSceneTableImpl::UnregisterHandler(SceneHandler * handler)
+void DefaultSceneTableImpl::UnregisterHandler(SceneHandler * handler)
 {
-    uint8_t position = kInvalidPosition;
-
     // Verify list is populated and handler is not null
-    VerifyOrReturnValue(!this->HandlerListEmpty() && !(handler == nullptr), CHIP_NO_ERROR);
-
-    // Finds the position of the Handler to unregister
-    for (uint8_t i = 0; i < this->mNumHandlers; i++)
-    {
-        if (this->mHandlers[i] == handler)
-        {
-            position = i;
-            break;
-        }
-    }
-
-    // Verify Handler was found
-    VerifyOrReturnValue(position < kMaxSceneHandlers, CHIP_NO_ERROR);
-
-    uint8_t nextPos = static_cast<uint8_t>(position + 1);
-    uint8_t moveNum = static_cast<uint8_t>(kMaxSceneHandlers - nextPos);
-
-    // TODO: Implement general array management methods
-    // Compress array after removal
-    if (moveNum)
-    {
-        memmove(&this->mHandlers[position], &this->mHandlers[nextPos], sizeof(this->mHandlers[position]) * moveNum);
-    }
-
-    this->mNumHandlers--;
-    // Clear last occupied position
-    this->mHandlers[mNumHandlers] = nullptr;
-
-    return CHIP_NO_ERROR;
+    VerifyOrReturn(!this->HandlerListEmpty() && !(handler == nullptr));
+    mHandlerList.Remove(handler);
+    mNumHandlers--;
 }
 
-CHIP_ERROR DefaultSceneTableImpl::UnregisterAllHandlers()
+void DefaultSceneTableImpl::UnregisterAllHandlers()
 {
-    for (uint8_t i = 0; i < this->mNumHandlers; i++)
+    while (!mHandlerList.Empty())
     {
-        ReturnErrorOnFailure(this->UnregisterHandler(this->mHandlers[0]));
+        IntrusiveList<SceneHandler>::Iterator foo = mHandlerList.begin();
+        SceneHandler * handle                     = &(*foo);
+        mHandlerList.Remove((handle));
+        mNumHandlers--;
     }
-
-    return CHIP_NO_ERROR;
 }
 
+/// @brief Gets the field sets for the clusters implemented on a specific endpoint and store them in an EFS (extension field set).
+/// Does so by going through the SceneHandler list and calling the first handler the list find for each specific clusters.
+/// @param scene Scene in which the EFS gets populated
 CHIP_ERROR DefaultSceneTableImpl::SceneSaveEFS(SceneTableEntry & scene)
 {
+
     if (!this->HandlerListEmpty())
     {
-        for (uint8_t i = 0; i < this->mNumHandlers; i++)
+        uint8_t clusterCount = 0;
+        clusterId cArray[kMaxClusterPerScenes];
+        Span<clusterId> cSpan(cArray);
+        clusterCount = GetClustersFromEndpoint(scene.mStorageId.mEndpointId, cArray, kMaxClusterPerScenes);
+        cSpan.reduce_size(clusterCount);
+        for (clusterId cluster : cSpan)
         {
-            clusterId cArray[kMaxClusterPerScenes];
-            Span<clusterId> cSpan(cArray);
-            if (this->mHandlers[i] != nullptr)
+            ExtensionFieldSet EFS;
+            MutableByteSpan EFSSpan = MutableByteSpan(EFS.mBytesBuffer, kMaxFieldBytesPerCluster);
+            EFS.mID                 = cluster;
+
+            IntrusiveList<SceneHandler>::Iterator iter = mHandlerList.begin();
+            while (iter != mHandlerList.end())
             {
-                this->mHandlers[i]->GetSupportedClusters(scene.mStorageId.mEndpointId, cSpan);
-                for (clusterId cluster : cSpan)
+                if (iter->SupportsCluster(scene.mStorageId.mEndpointId, cluster))
                 {
-                    ExtensionFieldSet EFS;
-                    MutableByteSpan EFSSpan = MutableByteSpan(EFS.mBytesBuffer, kMaxFieldBytesPerCluster);
-
-                    EFS.mID = cluster;
-                    ReturnErrorOnFailure(this->mHandlers[i]->SerializeSave(scene.mStorageId.mEndpointId, EFS.mID, EFSSpan));
+                    ReturnErrorOnFailure(iter->SerializeSave(scene.mStorageId.mEndpointId, EFS.mID, EFSSpan));
                     EFS.mUsedBytes = static_cast<uint8_t>(EFSSpan.size());
                     ReturnErrorOnFailure(scene.mStorageData.mExtensionFieldSets.InsertFieldSet(EFS));
+                    break;
                 }
+                iter++;
             }
         }
     }
 
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR DefaultSceneTableImpl::SceneApplyEFS(FabricIndex fabric_index, const SceneStorageId & scene_id)
+/// @brief Retrieves the values of extension field sets on a scene and applies them to each cluster on the endpoint of the scene.
+/// Does so by iterating through mHandlerList for each cluster in the EFS and calling the FIRST handler found that supports the
+/// cluster. Does so by going through the SceneHandler list and calling the first handler the list find for each specific clusters.
+/// @param scene Scene providing the EFSs (extension field sets)
+CHIP_ERROR DefaultSceneTableImpl::SceneApplyEFS(const SceneTableEntry & scene)
 {
-    FabricSceneData fabric(fabric_index);
-    SceneTableData scene(fabric_index);
     ExtensionFieldSet EFS;
     TransitionTimeMs time;
     clusterId cluster;
 
-    ReturnErrorOnFailure(fabric.Load(mStorage));
-    VerifyOrReturnError(fabric.Find(scene_id, scene.index) == CHIP_NO_ERROR, CHIP_ERROR_NOT_FOUND);
-    ReturnErrorOnFailure(scene.Load(mStorage));
-
     if (!this->HandlerListEmpty())
     {
         for (uint8_t i = 0; i < scene.mStorageData.mExtensionFieldSets.GetFieldSetCount(); i++)
@@ -500,9 +461,14 @@ CHIP_ERROR DefaultSceneTableImpl::SceneApplyEFS(FabricIndex fabric_index, const
 
             if (!EFS.IsEmpty())
             {
-                for (uint8_t j = 0; j < this->mNumHandlers; j++)
+                IntrusiveList<SceneHandler>::Iterator iter = mHandlerList.begin();
+                while (iter != mHandlerList.end())
                 {
-                    ReturnErrorOnFailure(this->mHandlers[j]->ApplyScene(scene.mStorageId.mEndpointId, cluster, EFSSpan, time));
+                    if (iter->SupportsCluster(scene.mStorageId.mEndpointId, cluster))
+                    {
+                        ReturnErrorOnFailure(iter->ApplyScene(scene.mStorageId.mEndpointId, cluster, EFSSpan, time));
+                        break;
+                    }
                 }
             }
         }
@@ -529,6 +495,13 @@ CHIP_ERROR DefaultSceneTableImpl::RemoveFabric(FabricIndex fabric_index)
     return fabric.Delete(mStorage);
 }
 
+/// @brief wrapper function around emberAfGetClustersFromEndpoint to allow testing, shimed in test configuration because
+/// emberAfGetClusterFromEndpoint relies on <app/util/attribute-storage.h>, which relies on zap generated files
+uint8_t DefaultSceneTableImpl::GetClustersFromEndpoint(EndpointId endpoint, ClusterId * clusterList, uint8_t listLen)
+{
+    return emberAfGetClustersFromEndpoint(endpoint, clusterList, listLen, true);
+}
+
 DefaultSceneTableImpl::SceneEntryIterator * DefaultSceneTableImpl::IterateSceneEntries(FabricIndex fabric_index)
 {
     VerifyOrReturnError(IsInitialized(), nullptr);

src/app/clusters/scenes/SceneTableImpl.h

@@ -200,13 +200,14 @@ class DefaultSceneTableImpl : public SceneTable<scenes::ExtensionFieldSetsImpl>
     CHIP_ERROR RemoveSceneTableEntryAtPosition(FabricIndex fabric_index, SceneIndex scene_idx) override;
 
     // SceneHandlers
-    CHIP_ERROR RegisterHandler(SceneHandler * handler) override;
-    CHIP_ERROR UnregisterHandler(SceneHandler * handler) override;
-    CHIP_ERROR UnregisterAllHandlers() override;
+    void RegisterHandler(SceneHandler * handler) override;
+    void UnregisterHandler(SceneHandler * handler) override;
+    void UnregisterAllHandlers() override;
 
     // Extension field sets operation
+
     CHIP_ERROR SceneSaveEFS(SceneTableEntry & scene) override;
-    CHIP_ERROR SceneApplyEFS(FabricIndex fabric_index, const SceneStorageId & scene_id) override;
+    CHIP_ERROR SceneApplyEFS(const SceneTableEntry & scene) override;
 
     // Fabrics
     CHIP_ERROR RemoveFabric(FabricIndex fabric_index) override;
@@ -215,6 +216,9 @@ class DefaultSceneTableImpl : public SceneTable<scenes::ExtensionFieldSetsImpl>
     SceneEntryIterator * IterateSceneEntries(FabricIndex fabric_index) override;
 
 protected:
+    // wrapper function around emberAfGetClustersFromEndpoint to allow override when testing
+    virtual uint8_t GetClustersFromEndpoint(EndpointId endpoint, ClusterId * clusterList, uint8_t listLen);
+
     class SceneEntryIteratorImpl : public SceneEntryIterator
     {
     public:

src/app/server/BUILD.gn

@@ -50,7 +50,6 @@ static_library("server") {
 
   public_deps = [
     "${chip_root}/src/app",
-    "${chip_root}/src/app/clusters/scenes",
     "${chip_root}/src/lib/address_resolve",
     "${chip_root}/src/lib/dnssd",
     "${chip_root}/src/messaging",

src/app/server/Server.cpp

@@ -162,8 +162,6 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams)
     mGroupsProvider = initParams.groupDataProvider;
     SetGroupDataProvider(mGroupsProvider);
 
-    mSceneTable = initParams.sceneTable;
-
     mTestEventTriggerDelegate = initParams.testEventTriggerDelegate;
 
     deviceInfoprovider = DeviceLayer::GetDeviceInfoProvider();
@@ -537,8 +535,6 @@ KvsPersistentStorageDelegate CommonCaseDeviceServerInitParams::sKvsPersistenStor
 PersistentStorageOperationalKeystore CommonCaseDeviceServerInitParams::sPersistentStorageOperationalKeystore;
 Credentials::PersistentStorageOpCertStore CommonCaseDeviceServerInitParams::sPersistentStorageOpCertStore;
 Credentials::GroupDataProviderImpl CommonCaseDeviceServerInitParams::sGroupDataProvider;
-// TODO: complete scene cluster implementation before uncommenting
-// scenes::DefaultSceneTableImpl CommonCaseDeviceServerInitParams::sSceneTable;
 IgnoreCertificateValidityPolicy CommonCaseDeviceServerInitParams::sDefaultCertValidityPolicy;
 #if CHIP_CONFIG_ENABLE_SESSION_RESUMPTION
 SimpleSessionResumptionStorage CommonCaseDeviceServerInitParams::sSessionResumptionStorage;