Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Actions: Use an instance of the Actions cluster per endpoint instead of having global delegate array. #37799

Merged
merged 4 commits into from
Mar 4, 2025
Merged

Actions: Use an instance of the Actions cluster per endpoint instead of having global delegate array. #37799

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
880d2f2
Actions: Use instance of the Actions cluster per endpoint instead of …
jadhavrohit924 Feb 27, 2025
7cee1f4
Fix unit tests.
jadhavrohit924 Feb 27, 2025
bde7b06
Addressed review comments.
jadhavrohit924 Feb 28, 2025
6831f39
Add error logs.
jadhavrohit924 Mar 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions examples/all-clusters-app/all-clusters-common/src/bridged-actions-stub.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ 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)
{
if (index >= kActionList.size())
Expand Down Expand Up @@ -129,3 +134,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();
}
8 changes: 0 additions & 8 deletions examples/all-clusters-app/linux/main-common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down
163 changes: 38 additions & 125 deletions src/app/clusters/actions-server/actions-server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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>
Expand Down Expand Up @@ -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)))
{
Expand All @@ -220,139 +186,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);
}

Expand Down Expand Up @@ -426,8 +343,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() {}
23 changes: 19 additions & 4 deletions src/app/clusters/actions-server/actions-server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(Optional<EndpointId>(aEndpointId), Actions::Id),
CommandHandlerInterface(Optional<EndpointId>(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
Expand Down Expand Up @@ -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;
Expand Down
Loading
Loading