Actions: Use an instance of the Actions cluster per endpoint instead of having global delegate array. (project-chip#37799)

jadhavrohit924 · gmarcosb · commit 66c6de7369ed · 2025-03-04T16:01:29.000-07:00
* Actions: Use instance of the Actions cluster per endpoint instead of having global delegate array.

* Fix unit tests.

* Addressed review comments.

* Add error logs.
diff --git a/examples/all-clusters-app/all-clusters-common/src/bridged-actions-stub.cpp b/examples/all-clusters-app/all-clusters-common/src/bridged-actions-stub.cpp
@@ -24,6 +24,11 @@ using namespace chip::app::Clusters::Actions;
 using namespace chip::app::Clusters::Actions::Attributes;
 using namespace chip::Protocols::InteractionModel;
 
+namespace {
+std::unique_ptr<Clusters::Actions::ActionsDelegateImpl> sActionsDelegateImpl;
+std::unique_ptr<Clusters::Actions::ActionsServer> sActionsServer;
+} // namespace
+
 CHIP_ERROR ActionsDelegateImpl::ReadActionAtIndex(uint16_t index, ActionStructStorage & action)
 {
     if (index >= kActionList.size())
@@ -129,3 +134,17 @@ Status ActionsDelegateImpl::HandleDisableActionWithDuration(uint16_t actionId, u
     // Not implemented
     return Status::NotFound;
 }
+
+void emberAfActionsClusterInitCallback(EndpointId endpoint)
+{
+    VerifyOrReturn(endpoint == 1,
+                   ChipLogError(Zcl, "Actions cluster delegate is not implemented for endpoint with id %d.", endpoint));
+    VerifyOrReturn(emberAfContainsServer(endpoint, Actions::Id) == true,
+                   ChipLogError(Zcl, "Endpoint %d does not support Actions cluster.", endpoint));
+    VerifyOrReturn(!sActionsDelegateImpl && !sActionsServer);
+
+    sActionsDelegateImpl = std::make_unique<Actions::ActionsDelegateImpl>();
+    sActionsServer       = std::make_unique<Actions::ActionsServer>(endpoint, *sActionsDelegateImpl.get());
+
+    sActionsServer->Init();
+}
diff --git a/examples/all-clusters-app/linux/main-common.cpp b/examples/all-clusters-app/linux/main-common.cpp
@@ -54,7 +54,6 @@
 #include <app/clusters/valve-configuration-and-control-server/valve-configuration-and-control-server.h>
 #include <app/server/Server.h>
 #include <app/util/attribute-storage.h>
-#include <bridged-actions-stub.h>
 #include <lib/support/CHIPMem.h>
 #include <platform/DeviceInstanceInfoProvider.h>
 #include <platform/DiagnosticDataProvider.h>
@@ -87,7 +86,6 @@ Clusters::TemperatureControl::AppSupportedTemperatureLevelsDelegate sAppSupporte
 Clusters::ModeSelect::StaticSupportedModesManager sStaticSupportedModesManager;
 Clusters::ValveConfigurationAndControl::ValveControlDelegate sValveDelegate;
 Clusters::TimeSynchronization::ExtendedTimeSyncDelegate sTimeSyncDelegate;
-Clusters::Actions::ActionsDelegateImpl sActionsDelegateImpl;
 
 // Please refer to https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/master/src/namespaces
 constexpr const uint8_t kNamespaceCommon   = 7;
@@ -342,12 +340,6 @@ void emberAfThermostatClusterInitCallback(EndpointId endpoint)
     SetDefaultDelegate(endpoint, &delegate);
 }
 
-void emberAfActionsClusterInitCallback(EndpointId endpoint)
-{
-    VerifyOrReturn(endpoint == 1);
-    Clusters::Actions::ActionsServer::Instance().SetDefaultDelegate(endpoint, &sActionsDelegateImpl);
-}
-
 Status emberAfExternalAttributeReadCallback(EndpointId endpoint, ClusterId clusterId,
                                             const EmberAfAttributeMetadata * attributeMetadata, uint8_t * buffer,
                                             uint16_t maxReadLength)
diff --git a/src/app/clusters/actions-server/actions-server.cpp b/src/app/clusters/actions-server/actions-server.cpp
@@ -32,44 +32,22 @@ using namespace chip::app::Clusters::Actions;
 using namespace chip::app::Clusters::Actions::Attributes;
 using namespace chip::Protocols::InteractionModel;
 
-namespace {
-static constexpr size_t kActionsDelegateTableSize =
-    MATTER_DM_ACTIONS_CLUSTER_SERVER_ENDPOINT_COUNT + CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT;
-static_assert(kActionsDelegateTableSize <= kEmberInvalidEndpointIndex, "Actions Delegate table size error");
-
-// TODO: We should not use global array, instead we can use one cluster instance per endpoint.
-Delegate * gDelegateTable[kActionsDelegateTableSize] = { nullptr };
-
-Delegate * GetDelegate(EndpointId aEndpoint)
+ActionsServer::~ActionsServer()
 {
-    return (aEndpoint >= kActionsDelegateTableSize ? nullptr : gDelegateTable[aEndpoint]);
+    Shutdown();
 }
 
-CHIP_ERROR ValidateDelegate(Delegate * aDelegate, EndpointId aEndpoint)
+void ActionsServer::Shutdown()
 {
-    if (aDelegate == nullptr)
-    {
-        ChipLogError(Zcl, "Actions delegate is null for endpoint: %d !!!", aEndpoint);
-        return CHIP_ERROR_INCORRECT_STATE;
-    }
-    return CHIP_NO_ERROR;
-}
-
-} // namespace
-
-ActionsServer ActionsServer::sInstance;
-
-void ActionsServer::SetDefaultDelegate(EndpointId aEndpoint, Delegate * aDelegate)
-{
-    if (aEndpoint < kActionsDelegateTableSize)
-    {
-        gDelegateTable[aEndpoint] = aDelegate;
-    }
+    CommandHandlerInterfaceRegistry::Instance().UnregisterCommandHandler(this);
+    AttributeAccessInterfaceRegistry::Instance().Unregister(this);
 }
 
-ActionsServer & ActionsServer::Instance()
+CHIP_ERROR ActionsServer::Init()
 {
-    return sInstance;
+    ReturnErrorOnFailure(CommandHandlerInterfaceRegistry::Instance().RegisterCommandHandler(this));
+    VerifyOrReturnError(AttributeAccessInterfaceRegistry::Instance().Register(this), CHIP_ERROR_INCORRECT_STATE);
+    return CHIP_NO_ERROR;
 }
 
 void ActionsServer::OnStateChanged(EndpointId aEndpoint, uint16_t aActionId, uint32_t aInvokeId, ActionStateEnum aActionState)
@@ -126,14 +104,10 @@ CHIP_ERROR ActionsServer::Read(const ConcreteReadAttributePath & aPath, Attribut
 CHIP_ERROR ActionsServer::ReadActionListAttribute(const ConcreteReadAttributePath & aPath,
                                                   const AttributeValueEncoder::ListEncodeHelper & aEncoder)
 {
-    Delegate * delegate = GetDelegate(aPath.mEndpointId);
-    ReturnErrorOnFailure(ValidateDelegate(delegate, aPath.mEndpointId));
-
     for (uint16_t i = 0; i < kMaxActionListLength; i++)
     {
         ActionStructStorage action;
-        CHIP_ERROR err = delegate->ReadActionAtIndex(i, action);
-
+        CHIP_ERROR err = mDelegate.ReadActionAtIndex(i, action);
         if (err == CHIP_ERROR_PROVIDER_LIST_EXHAUSTED)
         {
             return CHIP_NO_ERROR;
@@ -147,14 +121,10 @@ CHIP_ERROR ActionsServer::ReadActionListAttribute(const ConcreteReadAttributePat
 CHIP_ERROR ActionsServer::ReadEndpointListAttribute(const ConcreteReadAttributePath & aPath,
                                                     const AttributeValueEncoder::ListEncodeHelper & aEncoder)
 {
-    Delegate * delegate = GetDelegate(aPath.mEndpointId);
-    ReturnErrorOnFailure(ValidateDelegate(delegate, aPath.mEndpointId));
-
     for (uint16_t i = 0; i < kMaxEndpointListLength; i++)
     {
         EndpointListStorage epList;
-
-        CHIP_ERROR err = delegate->ReadEndpointListAtIndex(i, epList);
+        CHIP_ERROR err = mDelegate.ReadEndpointListAtIndex(i, epList);
         if (err == CHIP_ERROR_PROVIDER_LIST_EXHAUSTED)
         {
             return CHIP_NO_ERROR;
@@ -167,9 +137,7 @@ CHIP_ERROR ActionsServer::ReadEndpointListAttribute(const ConcreteReadAttributeP
 
 bool ActionsServer::HaveActionWithId(EndpointId aEndpointId, uint16_t aActionId, uint16_t & aActionIndex)
 {
-    Delegate * delegate = GetDelegate(aEndpointId);
-    VerifyOrReturnValue(ValidateDelegate(delegate, aEndpointId) == CHIP_NO_ERROR, false);
-    return delegate->HaveActionWithId(aActionId, aActionIndex);
+    return mDelegate.HaveActionWithId(aActionId, aActionIndex);
 }
 
 template <typename RequestT, typename FuncT>
@@ -201,10 +169,8 @@ void ActionsServer::HandleCommand(HandlerContext & handlerContext, FuncT func)
         }
         if (actionIndex != kMaxActionListLength)
         {
-            Delegate * delegate = GetDelegate(handlerContext.mRequestPath.mEndpointId);
-            ReturnOnFailure(ValidateDelegate(delegate, handlerContext.mRequestPath.mEndpointId));
             ActionStructStorage action;
-            delegate->ReadActionAtIndex(actionIndex, action);
+            mDelegate.ReadActionAtIndex(actionIndex, action);
             // Check if the command bit is set in the SupportedCommands of an ations.
             if (!(action.supportedCommands.Raw() & (1 << handlerContext.mRequestPath.mCommandId)))
             {
@@ -220,139 +186,79 @@ void ActionsServer::HandleCommand(HandlerContext & handlerContext, FuncT func)
 
 void ActionsServer::HandleInstantAction(HandlerContext & ctx, const Commands::InstantAction::DecodableType & commandData)
 {
-    Delegate * delegate = GetDelegate(ctx.mRequestPath.mEndpointId);
-    Status status       = Status::InvalidInState;
-    if (delegate != nullptr)
-    {
-        status = delegate->HandleInstantAction(commandData.actionID, commandData.invokeID);
-    }
+    Status status = mDelegate.HandleInstantAction(commandData.actionID, commandData.invokeID);
     ctx.mCommandHandler.AddStatus(ctx.mRequestPath, status);
 }
 
 void ActionsServer::HandleInstantActionWithTransition(HandlerContext & ctx,
                                                       const Commands::InstantActionWithTransition::DecodableType & commandData)
 {
-    Delegate * delegate = GetDelegate(ctx.mRequestPath.mEndpointId);
-    Status status       = Status::InvalidInState;
-    if (delegate != nullptr)
-    {
-        status =
-            delegate->HandleInstantActionWithTransition(commandData.actionID, commandData.transitionTime, commandData.invokeID);
-    }
+    Status status =
+        mDelegate.HandleInstantActionWithTransition(commandData.actionID, commandData.transitionTime, commandData.invokeID);
     ctx.mCommandHandler.AddStatus(ctx.mRequestPath, status);
 }
 
 void ActionsServer::HandleStartAction(HandlerContext & ctx, const Commands::StartAction::DecodableType & commandData)
 {
-    Delegate * delegate = GetDelegate(ctx.mRequestPath.mEndpointId);
-    Status status       = Status::InvalidInState;
-    if (delegate != nullptr)
-    {
-        status = delegate->HandleStartAction(commandData.actionID, commandData.invokeID);
-    }
+    Status status = mDelegate.HandleStartAction(commandData.actionID, commandData.invokeID);
     ctx.mCommandHandler.AddStatus(ctx.mRequestPath, status);
 }
 
 void ActionsServer::HandleStartActionWithDuration(HandlerContext & ctx,
                                                   const Commands::StartActionWithDuration::DecodableType & commandData)
 {
-    Delegate * delegate = GetDelegate(ctx.mRequestPath.mEndpointId);
-    Status status       = Status::InvalidInState;
-    if (delegate != nullptr)
-    {
-        status = delegate->HandleStartActionWithDuration(commandData.actionID, commandData.duration, commandData.invokeID);
-    }
+    Status status = mDelegate.HandleStartActionWithDuration(commandData.actionID, commandData.duration, commandData.invokeID);
     ctx.mCommandHandler.AddStatus(ctx.mRequestPath, status);
 }
 
 void ActionsServer::HandleStopAction(HandlerContext & ctx, const Commands::StopAction::DecodableType & commandData)
 {
-    Delegate * delegate = GetDelegate(ctx.mRequestPath.mEndpointId);
-    Status status       = Status::InvalidInState;
-    if (delegate != nullptr)
-    {
-        status = delegate->HandleStopAction(commandData.actionID, commandData.invokeID);
-    }
+    Status status = mDelegate.HandleStopAction(commandData.actionID, commandData.invokeID);
     ctx.mCommandHandler.AddStatus(ctx.mRequestPath, status);
 }
 
 void ActionsServer::HandlePauseAction(HandlerContext & ctx, const Commands::PauseAction::DecodableType & commandData)
 {
-    Delegate * delegate = GetDelegate(ctx.mRequestPath.mEndpointId);
-    Status status       = Status::InvalidInState;
-    if (delegate != nullptr)
-    {
-        status = delegate->HandlePauseAction(commandData.actionID, commandData.invokeID);
-    }
+    Status status = mDelegate.HandlePauseAction(commandData.actionID, commandData.invokeID);
     ctx.mCommandHandler.AddStatus(ctx.mRequestPath, status);
 }
 
 void ActionsServer::HandlePauseActionWithDuration(HandlerContext & ctx,
                                                   const Commands::PauseActionWithDuration::DecodableType & commandData)
 {
-    Delegate * delegate = GetDelegate(ctx.mRequestPath.mEndpointId);
-    Status status       = Status::InvalidInState;
-    if (delegate != nullptr)
-    {
-        status = delegate->HandlePauseActionWithDuration(commandData.actionID, commandData.duration, commandData.invokeID);
-    }
+    Status status = mDelegate.HandlePauseActionWithDuration(commandData.actionID, commandData.duration, commandData.invokeID);
     ctx.mCommandHandler.AddStatus(ctx.mRequestPath, status);
 }
 
 void ActionsServer::HandleResumeAction(HandlerContext & ctx, const Commands::ResumeAction::DecodableType & commandData)
 {
-    Delegate * delegate = GetDelegate(ctx.mRequestPath.mEndpointId);
-    Status status       = Status::InvalidInState;
-    if (delegate != nullptr)
-    {
-        status = delegate->HandleResumeAction(commandData.actionID, commandData.invokeID);
-    }
+    Status status = mDelegate.HandleResumeAction(commandData.actionID, commandData.invokeID);
     ctx.mCommandHandler.AddStatus(ctx.mRequestPath, status);
 }
 
 void ActionsServer::HandleEnableAction(HandlerContext & ctx, const Commands::EnableAction::DecodableType & commandData)
 {
-    Delegate * delegate = GetDelegate(ctx.mRequestPath.mEndpointId);
-    Status status       = Status::InvalidInState;
-    if (delegate != nullptr)
-    {
-        status = delegate->HandleEnableAction(commandData.actionID, commandData.invokeID);
-    }
+    Status status = mDelegate.HandleEnableAction(commandData.actionID, commandData.invokeID);
     ctx.mCommandHandler.AddStatus(ctx.mRequestPath, status);
 }
 
 void ActionsServer::HandleEnableActionWithDuration(HandlerContext & ctx,
                                                    const Commands::EnableActionWithDuration::DecodableType & commandData)
 {
-    Delegate * delegate = GetDelegate(ctx.mRequestPath.mEndpointId);
-    Status status       = Status::InvalidInState;
-    if (delegate != nullptr)
-    {
-        status = delegate->HandleEnableActionWithDuration(commandData.actionID, commandData.duration, commandData.invokeID);
-    }
+    Status status = mDelegate.HandleEnableActionWithDuration(commandData.actionID, commandData.duration, commandData.invokeID);
     ctx.mCommandHandler.AddStatus(ctx.mRequestPath, status);
 }
 
 void ActionsServer::HandleDisableAction(HandlerContext & ctx, const Commands::DisableAction::DecodableType & commandData)
 {
-    Delegate * delegate = GetDelegate(ctx.mRequestPath.mEndpointId);
-    Status status       = Status::InvalidInState;
-    if (delegate != nullptr)
-    {
-        status = delegate->HandleDisableAction(commandData.actionID, commandData.invokeID);
-    }
+    Status status = mDelegate.HandleDisableAction(commandData.actionID, commandData.invokeID);
     ctx.mCommandHandler.AddStatus(ctx.mRequestPath, status);
 }
 
 void ActionsServer::HandleDisableActionWithDuration(HandlerContext & ctx,
                                                     const Commands::DisableActionWithDuration::DecodableType & commandData)
 {
-    Delegate * delegate = GetDelegate(ctx.mRequestPath.mEndpointId);
-    Status status       = Status::InvalidInState;
-    if (delegate != nullptr)
-    {
-        status = delegate->HandleDisableActionWithDuration(commandData.actionID, commandData.duration, commandData.invokeID);
-    }
+    Status status = mDelegate.HandleDisableActionWithDuration(commandData.actionID, commandData.duration, commandData.invokeID);
     ctx.mCommandHandler.AddStatus(ctx.mRequestPath, status);
 }
 
@@ -426,8 +332,4 @@ void ActionsServer::EndpointListModified(EndpointId aEndpoint)
     MarkDirty(aEndpoint, Attributes::EndpointLists::Id);
 }
 
-void MatterActionsPluginServerInitCallback()
-{
-    AttributeAccessInterfaceRegistry::Instance().Register(&ActionsServer::Instance());
-    CommandHandlerInterfaceRegistry::Instance().RegisterCommandHandler(&ActionsServer::Instance());
-}
+void MatterActionsPluginServerInitCallback() {}
diff --git a/src/app/clusters/actions-server/actions-server.h b/src/app/clusters/actions-server/actions-server.h
@@ -114,11 +114,24 @@ class ActionsServer : public AttributeAccessInterface, public CommandHandlerInte
 {
 public:
     // Register for the Actions cluster on all endpoints.
-    ActionsServer() :
-        AttributeAccessInterface(Optional<EndpointId>::Missing(), Actions::Id),
-        CommandHandlerInterface(Optional<EndpointId>::Missing(), Actions::Id)
+    ActionsServer(EndpointId aEndpointId, Delegate & aDelegate) :
+        AttributeAccessInterface(MakeOptional(aEndpointId), Actions::Id),
+        CommandHandlerInterface(MakeOptional(aEndpointId), Actions::Id), mDelegate(aDelegate), mEndpointId(aEndpointId)
     {}
-    static ActionsServer & Instance();
+
+    ~ActionsServer();
+
+    /**
+     * Initialise the Actions server instance.
+     * @return Returns an error if the given endpoint and cluster have not been enabled in zap, if the
+     * AttributeAccessInterface or AttributeAccessInterface registration fails returns an error.
+     */
+    CHIP_ERROR Init();
+
+    /**
+     * Unregisters the CommandHandlerInterface and AttributeAccessInterface.
+     */
+    void Shutdown();
 
     /**
      * @brief
@@ -152,6 +165,8 @@ class ActionsServer : public AttributeAccessInterface, public CommandHandlerInte
     void EndpointListModified(EndpointId aEndpoint);
 
 private:
+    Delegate & mDelegate;
+    EndpointId mEndpointId;
     static ActionsServer sInstance;
     static constexpr size_t kMaxEndpointListLength = 256u;
     static constexpr size_t kMaxActionListLength   = 256u;
diff --git a/src/app/tests/TestActionsCluster.cpp b/src/app/tests/TestActionsCluster.cpp
