From 880d2f2236d7b2b8fd4d39ba4290561d5a58e55c Mon Sep 17 00:00:00 2001 From: Rohit Jadhav Date: Thu, 27 Feb 2025 11:44:14 +0530 Subject: [PATCH 1/4] Actions: Use instance of the Actions cluster per endpoint instead of having global delegate array. --- .../src/bridged-actions-stub.cpp | 12 ++ .../all-clusters-app/linux/main-common.cpp | 8 - .../actions-server/actions-server.cpp | 162 +++++------------- .../clusters/actions-server/actions-server.h | 23 ++- src/app/tests/TestActionsCluster.cpp | 54 +++--- 5 files changed, 103 insertions(+), 156 deletions(-) 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 index b44193d996b788..05829e863244b3 100644 --- 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,9 @@ using namespace chip::app::Clusters::Actions; using namespace chip::app::Clusters::Actions::Attributes; using namespace chip::Protocols::InteractionModel; +Clusters::Actions::ActionsDelegateImpl * sActionsDelegateImpl = nullptr; +Clusters::Actions::ActionsServer * sActionsServer = nullptr; + CHIP_ERROR ActionsDelegateImpl::ReadActionAtIndex(uint16_t index, ActionStructStorage & action) { if (index >= kActionList.size()) @@ -129,3 +132,12 @@ Status ActionsDelegateImpl::HandleDisableActionWithDuration(uint16_t actionId, u // Not implemented return Status::NotFound; } + +void emberAfActionsClusterInitCallback(EndpointId endpoint) +{ + VerifyOrReturn(endpoint == 1); + VerifyOrReturn(sActionsDelegateImpl == nullptr && sActionsServer == nullptr); + sActionsDelegateImpl = new Actions::ActionsDelegateImpl; + sActionsServer = new Actions::ActionsServer(endpoint, sActionsDelegateImpl); + sActionsServer->Init(); +} diff --git a/examples/all-clusters-app/linux/main-common.cpp b/examples/all-clusters-app/linux/main-common.cpp index b9026a291817fb..c5a7c2189303a0 100644 --- a/examples/all-clusters-app/linux/main-common.cpp +++ b/examples/all-clusters-app/linux/main-common.cpp @@ -54,7 +54,6 @@ #include #include #include -#include #include #include #include @@ -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 index e02f094d85105f..3172394718963a 100644 --- a/src/app/clusters/actions-server/actions-server.cpp +++ b/src/app/clusters/actions-server/actions-server.cpp @@ -32,44 +32,25 @@ 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; + CommandHandlerInterfaceRegistry::Instance().UnregisterCommandHandler(this); + AttributeAccessInterfaceRegistry::Instance().Unregister(this); } -} // namespace - -ActionsServer ActionsServer::sInstance; - -void ActionsServer::SetDefaultDelegate(EndpointId aEndpoint, Delegate * aDelegate) +CHIP_ERROR ActionsServer::Init() { - if (aEndpoint < kActionsDelegateTableSize) - { - gDelegateTable[aEndpoint] = aDelegate; - } -} + // Check if the cluster has been selected in zap + VerifyOrDie(emberAfContainsServer(mEndpointId, Actions::Id) == true); -ActionsServer & ActionsServer::Instance() -{ - 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,13 +107,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) { @@ -147,14 +125,11 @@ 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 +142,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 @@ -201,10 +174,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 +191,90 @@ 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 = Status::InvalidInState; + 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 = Status::InvalidInState; + 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 = Status::InvalidInState; + 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 = Status::InvalidInState; + 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 = Status::InvalidInState; + 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 = Status::InvalidInState; + 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 = Status::InvalidInState; + 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 = Status::InvalidInState; + 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 = Status::InvalidInState; + 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 = Status::InvalidInState; + 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 = Status::InvalidInState; + 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 = Status::InvalidInState; + status = mDelegate->HandleDisableActionWithDuration(commandData.actionID, commandData.duration, commandData.invokeID); ctx.mCommandHandler.AddStatus(ctx.mRequestPath, status); } @@ -426,8 +348,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 index 2cacbe868739b6..34d0989f213c36 100644 --- 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::Missing(), Actions::Id), - CommandHandlerInterface(Optional::Missing(), Actions::Id) + ActionsServer(EndpointId aEndpointId, Delegate * aDelegate) : + AttributeAccessInterface(Optional(aEndpointId), Actions::Id), + CommandHandlerInterface(Optional(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 index f7034a075dc2b0..43ae6ff1ec3ffd 100644 --- a/src/app/tests/TestActionsCluster.cpp +++ b/src/app/tests/TestActionsCluster.cpp @@ -44,17 +44,11 @@ namespace app { using namespace Clusters::Actions; using namespace chip::Protocols::InteractionModel; -class TestActionsCluster : public ::testing::Test -{ -public: - static void SetUpTestSuite() { ASSERT_EQ(chip::Platform::MemoryInit(), CHIP_NO_ERROR); } - static void TearDownTestSuite() { chip::Platform::MemoryShutdown(); } -}; +class TestActionsDelegateImpl; class TestActionsDelegateImpl : public Clusters::Actions::Delegate { public: - EndpointId endpointId = 0; static constexpr uint8_t kMaxActionNameLength = 128u; static constexpr uint8_t kMaxEndpointListNameLength = 128u; static constexpr uint16_t kEndpointListMaxSize = 256u; @@ -176,7 +170,26 @@ class TestActionsDelegateImpl : public Clusters::Actions::Delegate } }; -TestActionsDelegateImpl delegate; +static TestActionsDelegateImpl * sActionsDelegateImpl; +static ActionsServer * sActionsServer; + +class TestActionsCluster : public ::testing::Test +{ +public: + static void SetUpTestSuite() + { + ASSERT_EQ(chip::Platform::MemoryInit(), CHIP_NO_ERROR); + sActionsDelegateImpl = new TestActionsDelegateImpl; + sActionsServer = new ActionsServer(0, sActionsDelegateImpl); + sActionsServer->Init(); + } + static void TearDownTestSuite() + { + sActionsServer->Shutdown(); + chip::Platform::MemoryShutdown(); + } +}; + TEST_F(TestActionsCluster, TestActionListConstraints) { // Test 1: Action name length constraint @@ -186,6 +199,7 @@ TEST_F(TestActionsCluster, TestActionListConstraints) ActionStructStorage actionWithLongName(1, CharSpan::fromCharString(longName), ActionTypeEnum::kScene, 0, BitMask(), ActionStateEnum::kInactive); + TestActionsDelegateImpl delegate = *sActionsDelegateImpl; // Add action and verify it was added successfully EXPECT_EQ(delegate.AddTestAction(actionWithLongName), CHIP_NO_ERROR); @@ -221,6 +235,7 @@ TEST_F(TestActionsCluster, TestEndpointListConstraints) EndpointListStorage epListWithLongName(1, CharSpan::fromCharString(longName), EndpointListTypeEnum::kOther, DataModel::List(endpoints)); + TestActionsDelegateImpl delegate = *sActionsDelegateImpl; // Add endpoint list and verify it was added successfully EXPECT_EQ(delegate.AddTestEndpointList(epListWithLongName), CHIP_NO_ERROR); @@ -264,8 +279,8 @@ TEST_F(TestActionsCluster, TestEndpointListConstraints) TEST_F(TestActionsCluster, TestActionListAttributeAccess) { - ActionsServer::Instance().SetDefaultDelegate(delegate.endpointId, &delegate); - delegate.mNumActions = 0; + TestActionsDelegateImpl delegate = *sActionsDelegateImpl; + delegate.mNumActions = 0; // Add test actions ActionStructStorage action1(1, CharSpan::fromCharString("FirstAction"), ActionTypeEnum::kScene, 0, BitMask(), @@ -286,14 +301,14 @@ TEST_F(TestActionsCluster, TestActionListAttributeAccess) AttributeReportIBs::Builder builder; builder.Init(&tlvWriter); - ConcreteAttributePath path(delegate.endpointId, Clusters::Actions::Id, Clusters::Actions::Attributes::ActionList::Id); + ConcreteAttributePath path(0, Clusters::Actions::Id, Clusters::Actions::Attributes::ActionList::Id); ConcreteReadAttributePath readPath(path); chip::DataVersion dataVersion(0); Access::SubjectDescriptor subjectDescriptor; AttributeValueEncoder encoder(builder, subjectDescriptor, path, dataVersion); // Read the action list using the Actions cluster's Read function - EXPECT_EQ(ActionsServer::Instance().Read(readPath, encoder), CHIP_NO_ERROR); + EXPECT_EQ(sActionsServer->Read(readPath, encoder), CHIP_NO_ERROR); TLV::TLVReader reader; reader.Init(buf); @@ -342,15 +357,12 @@ TEST_F(TestActionsCluster, TestActionListAttributeAccess) EXPECT_EQ(action.endpointListID, 1); EXPECT_EQ(action.supportedCommands.Raw(), 0); EXPECT_EQ(action.state, ActionStateEnum::kActive); - - // Cleanup - ActionsServer::Instance().SetDefaultDelegate(delegate.endpointId, nullptr); } TEST_F(TestActionsCluster, TestEndpointListAttributeAccess) { - ActionsServer::Instance().SetDefaultDelegate(delegate.endpointId, &delegate); - delegate.mNumEndpointLists = 0; + TestActionsDelegateImpl delegate = *sActionsDelegateImpl; + delegate.mNumEndpointLists = 0; // Add test endpoint lists const EndpointId endpoints1[] = { 1, 2 }; @@ -376,13 +388,14 @@ TEST_F(TestActionsCluster, TestEndpointListAttributeAccess) AttributeReportIBs::Builder builder; builder.Init(&tlvWriter); - ConcreteAttributePath path(delegate.endpointId, Clusters::Actions::Id, Clusters::Actions::Attributes::EndpointLists::Id); + ConcreteAttributePath path(0, Clusters::Actions::Id, Clusters::Actions::Attributes::EndpointLists::Id); ConcreteReadAttributePath readPath(path); chip::DataVersion dataVersion(0); Access::SubjectDescriptor subjectDescriptor; AttributeValueEncoder encoder(builder, subjectDescriptor, path, dataVersion); - EXPECT_EQ(ActionsServer::Instance().Read(path, encoder), CHIP_NO_ERROR); + // Read the endpoint lists using the Actions cluster's Read function + EXPECT_EQ(sActionsServer->Read(path, encoder), CHIP_NO_ERROR); TLV::TLVReader reader; reader.Init(buf); @@ -443,9 +456,6 @@ TEST_F(TestActionsCluster, TestEndpointListAttributeAccess) EXPECT_EQ(endpoints2[i++], it.GetValue()); } } - - // Cleanup - ActionsServer::Instance().SetDefaultDelegate(delegate.endpointId, nullptr); } } // namespace app From 7cee1f49a20f08c39e53a61ae938a788ee12f41a Mon Sep 17 00:00:00 2001 From: Rohit Jadhav Date: Thu, 27 Feb 2025 16:20:24 +0530 Subject: [PATCH 2/4] Fix unit tests. --- .../src/bridged-actions-stub.cpp | 2 + .../actions-server/actions-server.cpp | 5 --- src/app/tests/TestActionsCluster.cpp | 41 +++++++++---------- 3 files changed, 21 insertions(+), 27 deletions(-) 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 index 05829e863244b3..ef161dadd54060 100644 --- 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,8 +24,10 @@ using namespace chip::app::Clusters::Actions; using namespace chip::app::Clusters::Actions::Attributes; using namespace chip::Protocols::InteractionModel; +namespace { Clusters::Actions::ActionsDelegateImpl * sActionsDelegateImpl = nullptr; Clusters::Actions::ActionsServer * sActionsServer = nullptr; +} // namespace CHIP_ERROR ActionsDelegateImpl::ReadActionAtIndex(uint16_t index, ActionStructStorage & action) { diff --git a/src/app/clusters/actions-server/actions-server.cpp b/src/app/clusters/actions-server/actions-server.cpp index 3172394718963a..ded1927ae49213 100644 --- a/src/app/clusters/actions-server/actions-server.cpp +++ b/src/app/clusters/actions-server/actions-server.cpp @@ -45,9 +45,6 @@ void ActionsServer::Shutdown() CHIP_ERROR ActionsServer::Init() { - // Check if the cluster has been selected in zap - VerifyOrDie(emberAfContainsServer(mEndpointId, Actions::Id) == true); - ReturnErrorOnFailure(CommandHandlerInterfaceRegistry::Instance().RegisterCommandHandler(this)); VerifyOrReturnError(AttributeAccessInterfaceRegistry::Instance().Register(this), CHIP_ERROR_INCORRECT_STATE); return CHIP_NO_ERROR; @@ -111,7 +108,6 @@ CHIP_ERROR ActionsServer::ReadActionListAttribute(const ConcreteReadAttributePat { ActionStructStorage action; CHIP_ERROR err = mDelegate->ReadActionAtIndex(i, action); - if (err == CHIP_ERROR_PROVIDER_LIST_EXHAUSTED) { return CHIP_NO_ERROR; @@ -128,7 +124,6 @@ CHIP_ERROR ActionsServer::ReadEndpointListAttribute(const ConcreteReadAttributeP for (uint16_t i = 0; i < kMaxEndpointListLength; i++) { EndpointListStorage epList; - CHIP_ERROR err = mDelegate->ReadEndpointListAtIndex(i, epList); if (err == CHIP_ERROR_PROVIDER_LIST_EXHAUSTED) { diff --git a/src/app/tests/TestActionsCluster.cpp b/src/app/tests/TestActionsCluster.cpp index 43ae6ff1ec3ffd..a38161f426cb70 100644 --- a/src/app/tests/TestActionsCluster.cpp +++ b/src/app/tests/TestActionsCluster.cpp @@ -33,19 +33,12 @@ #include #include -#ifdef CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT -#undef CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT -#define CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT 1 -#endif - namespace chip { namespace app { using namespace Clusters::Actions; using namespace chip::Protocols::InteractionModel; -class TestActionsDelegateImpl; - class TestActionsDelegateImpl : public Clusters::Actions::Delegate { public: @@ -170,8 +163,8 @@ class TestActionsDelegateImpl : public Clusters::Actions::Delegate } }; -static TestActionsDelegateImpl * sActionsDelegateImpl; -static ActionsServer * sActionsServer; +static TestActionsDelegateImpl * sActionsDelegateImpl = nullptr; +static ActionsServer * sActionsServer = nullptr; class TestActionsCluster : public ::testing::Test { @@ -180,12 +173,14 @@ class TestActionsCluster : public ::testing::Test { ASSERT_EQ(chip::Platform::MemoryInit(), CHIP_NO_ERROR); sActionsDelegateImpl = new TestActionsDelegateImpl; - sActionsServer = new ActionsServer(0, sActionsDelegateImpl); + sActionsServer = new ActionsServer(1, sActionsDelegateImpl); sActionsServer->Init(); } static void TearDownTestSuite() { sActionsServer->Shutdown(); + delete sActionsDelegateImpl; + delete sActionsServer; chip::Platform::MemoryShutdown(); } }; @@ -193,13 +188,14 @@ class TestActionsCluster : public ::testing::Test TEST_F(TestActionsCluster, TestActionListConstraints) { // Test 1: Action name length constraint + TestActionsDelegateImpl delegate = *sActionsDelegateImpl; + delegate.mNumActions = 0; char longName[kActionNameMaxSize + 10]; memset(longName, 'A', sizeof(longName)); longName[sizeof(longName) - 1] = '\0'; ActionStructStorage actionWithLongName(1, CharSpan::fromCharString(longName), ActionTypeEnum::kScene, 0, BitMask(), ActionStateEnum::kInactive); - TestActionsDelegateImpl delegate = *sActionsDelegateImpl; // Add action and verify it was added successfully EXPECT_EQ(delegate.AddTestAction(actionWithLongName), CHIP_NO_ERROR); @@ -227,6 +223,8 @@ TEST_F(TestActionsCluster, TestActionListConstraints) TEST_F(TestActionsCluster, TestEndpointListConstraints) { // Test 1: Endpoint list name length constraint + TestActionsDelegateImpl delegate = *sActionsDelegateImpl; + delegate.mNumEndpointLists = 0; char longName[kEndpointListNameMaxSize + 10]; memset(longName, 'B', sizeof(longName)); longName[sizeof(longName) - 1] = '\0'; @@ -235,7 +233,6 @@ TEST_F(TestActionsCluster, TestEndpointListConstraints) EndpointListStorage epListWithLongName(1, CharSpan::fromCharString(longName), EndpointListTypeEnum::kOther, DataModel::List(endpoints)); - TestActionsDelegateImpl delegate = *sActionsDelegateImpl; // Add endpoint list and verify it was added successfully EXPECT_EQ(delegate.AddTestEndpointList(epListWithLongName), CHIP_NO_ERROR); @@ -279,8 +276,8 @@ TEST_F(TestActionsCluster, TestEndpointListConstraints) TEST_F(TestActionsCluster, TestActionListAttributeAccess) { - TestActionsDelegateImpl delegate = *sActionsDelegateImpl; - delegate.mNumActions = 0; + TestActionsDelegateImpl * delegate = sActionsDelegateImpl; + delegate->mNumActions = 0; // Add test actions ActionStructStorage action1(1, CharSpan::fromCharString("FirstAction"), ActionTypeEnum::kScene, 0, BitMask(), @@ -288,8 +285,8 @@ TEST_F(TestActionsCluster, TestActionListAttributeAccess) ActionStructStorage action2(2, CharSpan::fromCharString("SecondAction"), ActionTypeEnum::kScene, 1, BitMask(), ActionStateEnum::kActive); - EXPECT_EQ(delegate.AddTestAction(action1), CHIP_NO_ERROR); - EXPECT_EQ(delegate.AddTestAction(action2), CHIP_NO_ERROR); + EXPECT_EQ(delegate->AddTestAction(action1), CHIP_NO_ERROR); + EXPECT_EQ(delegate->AddTestAction(action2), CHIP_NO_ERROR); // Test reading actions through attribute reader uint8_t buf[1024]; @@ -301,7 +298,7 @@ TEST_F(TestActionsCluster, TestActionListAttributeAccess) AttributeReportIBs::Builder builder; builder.Init(&tlvWriter); - ConcreteAttributePath path(0, Clusters::Actions::Id, Clusters::Actions::Attributes::ActionList::Id); + ConcreteAttributePath path(1, Clusters::Actions::Id, Clusters::Actions::Attributes::ActionList::Id); ConcreteReadAttributePath readPath(path); chip::DataVersion dataVersion(0); Access::SubjectDescriptor subjectDescriptor; @@ -361,8 +358,8 @@ TEST_F(TestActionsCluster, TestActionListAttributeAccess) TEST_F(TestActionsCluster, TestEndpointListAttributeAccess) { - TestActionsDelegateImpl delegate = *sActionsDelegateImpl; - delegate.mNumEndpointLists = 0; + TestActionsDelegateImpl * delegate = sActionsDelegateImpl; + delegate->mNumEndpointLists = 0; // Add test endpoint lists const EndpointId endpoints1[] = { 1, 2 }; @@ -373,8 +370,8 @@ TEST_F(TestActionsCluster, TestEndpointListAttributeAccess) EndpointListStorage epList2(2, CharSpan::fromCharString("SecondList"), EndpointListTypeEnum::kOther, DataModel::List(endpoints2, 3)); - EXPECT_EQ(delegate.AddTestEndpointList(epList1), CHIP_NO_ERROR); - EXPECT_EQ(delegate.AddTestEndpointList(epList2), CHIP_NO_ERROR); + EXPECT_EQ(delegate->AddTestEndpointList(epList1), CHIP_NO_ERROR); + EXPECT_EQ(delegate->AddTestEndpointList(epList2), CHIP_NO_ERROR); // Test reading endpoint lists through attribute reader TLV::TLVWriter writer; @@ -388,7 +385,7 @@ TEST_F(TestActionsCluster, TestEndpointListAttributeAccess) AttributeReportIBs::Builder builder; builder.Init(&tlvWriter); - ConcreteAttributePath path(0, Clusters::Actions::Id, Clusters::Actions::Attributes::EndpointLists::Id); + ConcreteAttributePath path(1, Clusters::Actions::Id, Clusters::Actions::Attributes::EndpointLists::Id); ConcreteReadAttributePath readPath(path); chip::DataVersion dataVersion(0); Access::SubjectDescriptor subjectDescriptor; From bde7b06a5be5f90cdbaad93ad9a7098b8558d367 Mon Sep 17 00:00:00 2001 From: Rohit Jadhav Date: Fri, 28 Feb 2025 13:56:57 +0530 Subject: [PATCH 3/4] Addressed review comments. --- .../src/bridged-actions-stub.cpp | 13 +++--- .../actions-server/actions-server.cpp | 45 +++++++------------ .../clusters/actions-server/actions-server.h | 8 ++-- src/app/tests/TestActionsCluster.cpp | 2 +- 4 files changed, 30 insertions(+), 38 deletions(-) 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 index ef161dadd54060..3613007b54c6e0 100644 --- 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 @@ -25,8 +25,8 @@ using namespace chip::app::Clusters::Actions::Attributes; using namespace chip::Protocols::InteractionModel; namespace { -Clusters::Actions::ActionsDelegateImpl * sActionsDelegateImpl = nullptr; -Clusters::Actions::ActionsServer * sActionsServer = nullptr; +std::unique_ptr sActionsDelegateImpl; +std::unique_ptr sActionsServer; } // namespace CHIP_ERROR ActionsDelegateImpl::ReadActionAtIndex(uint16_t index, ActionStructStorage & action) @@ -138,8 +138,11 @@ Status ActionsDelegateImpl::HandleDisableActionWithDuration(uint16_t actionId, u void emberAfActionsClusterInitCallback(EndpointId endpoint) { VerifyOrReturn(endpoint == 1); - VerifyOrReturn(sActionsDelegateImpl == nullptr && sActionsServer == nullptr); - sActionsDelegateImpl = new Actions::ActionsDelegateImpl; - sActionsServer = new Actions::ActionsServer(endpoint, sActionsDelegateImpl); + VerifyOrDie(emberAfContainsServer(endpoint, Actions::Id)); + VerifyOrReturn(!sActionsDelegateImpl && !sActionsServer); + + sActionsDelegateImpl = std::make_unique(); + sActionsServer = std::make_unique(endpoint, *sActionsDelegateImpl.get()); + sActionsServer->Init(); } diff --git a/src/app/clusters/actions-server/actions-server.cpp b/src/app/clusters/actions-server/actions-server.cpp index ded1927ae49213..5f6d6163e758e5 100644 --- a/src/app/clusters/actions-server/actions-server.cpp +++ b/src/app/clusters/actions-server/actions-server.cpp @@ -107,7 +107,7 @@ CHIP_ERROR ActionsServer::ReadActionListAttribute(const ConcreteReadAttributePat for (uint16_t i = 0; i < kMaxActionListLength; i++) { ActionStructStorage action; - CHIP_ERROR err = mDelegate->ReadActionAtIndex(i, action); + CHIP_ERROR err = mDelegate.ReadActionAtIndex(i, action); if (err == CHIP_ERROR_PROVIDER_LIST_EXHAUSTED) { return CHIP_NO_ERROR; @@ -124,7 +124,7 @@ CHIP_ERROR ActionsServer::ReadEndpointListAttribute(const ConcreteReadAttributeP for (uint16_t i = 0; i < kMaxEndpointListLength; i++) { EndpointListStorage epList; - CHIP_ERROR err = mDelegate->ReadEndpointListAtIndex(i, epList); + CHIP_ERROR err = mDelegate.ReadEndpointListAtIndex(i, epList); if (err == CHIP_ERROR_PROVIDER_LIST_EXHAUSTED) { return CHIP_NO_ERROR; @@ -137,7 +137,7 @@ CHIP_ERROR ActionsServer::ReadEndpointListAttribute(const ConcreteReadAttributeP bool ActionsServer::HaveActionWithId(EndpointId aEndpointId, uint16_t aActionId, uint16_t & aActionIndex) { - return mDelegate->HaveActionWithId(aActionId, aActionIndex); + return mDelegate.HaveActionWithId(aActionId, aActionIndex); } template @@ -170,7 +170,7 @@ void ActionsServer::HandleCommand(HandlerContext & handlerContext, FuncT func) if (actionIndex != kMaxActionListLength) { ActionStructStorage action; - mDelegate->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))) { @@ -186,90 +186,79 @@ void ActionsServer::HandleCommand(HandlerContext & handlerContext, FuncT func) void ActionsServer::HandleInstantAction(HandlerContext & ctx, const Commands::InstantAction::DecodableType & commandData) { - Status status = Status::InvalidInState; - status = mDelegate->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) { - Status status = Status::InvalidInState; - status = mDelegate->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) { - Status status = Status::InvalidInState; - status = mDelegate->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) { - Status status = Status::InvalidInState; - status = mDelegate->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) { - Status status = Status::InvalidInState; - status = mDelegate->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) { - Status status = Status::InvalidInState; - status = mDelegate->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) { - Status status = Status::InvalidInState; - status = mDelegate->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) { - Status status = Status::InvalidInState; - status = mDelegate->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) { - Status status = Status::InvalidInState; - status = mDelegate->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) { - Status status = Status::InvalidInState; - status = mDelegate->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) { - Status status = Status::InvalidInState; - status = mDelegate->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) { - Status status = Status::InvalidInState; - status = mDelegate->HandleDisableActionWithDuration(commandData.actionID, commandData.duration, commandData.invokeID); + Status status = mDelegate.HandleDisableActionWithDuration(commandData.actionID, commandData.duration, commandData.invokeID); ctx.mCommandHandler.AddStatus(ctx.mRequestPath, status); } diff --git a/src/app/clusters/actions-server/actions-server.h b/src/app/clusters/actions-server/actions-server.h index 34d0989f213c36..aaf08eb6bf2b99 100644 --- a/src/app/clusters/actions-server/actions-server.h +++ b/src/app/clusters/actions-server/actions-server.h @@ -114,9 +114,9 @@ class ActionsServer : public AttributeAccessInterface, public CommandHandlerInte { public: // Register for the Actions cluster on all endpoints. - ActionsServer(EndpointId aEndpointId, Delegate * aDelegate) : - AttributeAccessInterface(Optional(aEndpointId), Actions::Id), - CommandHandlerInterface(Optional(aEndpointId), Actions::Id), mDelegate(aDelegate), mEndpointId(aEndpointId) + ActionsServer(EndpointId aEndpointId, Delegate & aDelegate) : + AttributeAccessInterface(MakeOptional(aEndpointId), Actions::Id), + CommandHandlerInterface(MakeOptional(aEndpointId), Actions::Id), mDelegate(aDelegate), mEndpointId(aEndpointId) {} ~ActionsServer(); @@ -165,7 +165,7 @@ class ActionsServer : public AttributeAccessInterface, public CommandHandlerInte void EndpointListModified(EndpointId aEndpoint); private: - Delegate * mDelegate; + Delegate & mDelegate; EndpointId mEndpointId; static ActionsServer sInstance; static constexpr size_t kMaxEndpointListLength = 256u; diff --git a/src/app/tests/TestActionsCluster.cpp b/src/app/tests/TestActionsCluster.cpp index a38161f426cb70..b4f28addc3cc69 100644 --- a/src/app/tests/TestActionsCluster.cpp +++ b/src/app/tests/TestActionsCluster.cpp @@ -173,7 +173,7 @@ class TestActionsCluster : public ::testing::Test { ASSERT_EQ(chip::Platform::MemoryInit(), CHIP_NO_ERROR); sActionsDelegateImpl = new TestActionsDelegateImpl; - sActionsServer = new ActionsServer(1, sActionsDelegateImpl); + sActionsServer = new ActionsServer(1, *sActionsDelegateImpl); sActionsServer->Init(); } static void TearDownTestSuite() From 6831f3908e2bcedaa30265235a63963a2f400364 Mon Sep 17 00:00:00 2001 From: Rohit Jadhav Date: Tue, 4 Mar 2025 12:39:59 +0530 Subject: [PATCH 4/4] Add error logs. --- .../all-clusters-common/src/bridged-actions-stub.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 index 3613007b54c6e0..0e564759ee7be2 100644 --- 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 @@ -137,8 +137,10 @@ Status ActionsDelegateImpl::HandleDisableActionWithDuration(uint16_t actionId, u void emberAfActionsClusterInitCallback(EndpointId endpoint) { - VerifyOrReturn(endpoint == 1); - VerifyOrDie(emberAfContainsServer(endpoint, Actions::Id)); + 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();