diff --git a/docs/upgrading.md b/docs/upgrading.md index e9e17239d50049..8e2fbd424d539a 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -107,3 +107,13 @@ To use default attribute persistence, you need to pass in a `PersistentStorageDelegate` to `CodegenDataModelProviderInstance`. See example changes in [36658](https://github.com/project-chip/connectedhomeip/pull/36658) ). + +### `EnumerateAcceptedCommands` and `EnumerateGeneratedCommands` in `CommandHandlerInterface` + +Commands are generally highly coupled with metadata. Code was changed to allow +the interfaces to filter out accepted/generated commands, however they cannot +control the actual list of commands anymore since they must reside in metadata +(e.g. ember/zap).provider + +Replace these functions with `AcceptsCommandId` and `GeneratesCommandId`. See +[36809](https://github.com/project-chip/connectedhomeip/pull/36809) for changes. diff --git a/src/app/CommandHandlerInterface.h b/src/app/CommandHandlerInterface.h index 4146556779756c..d6f138f0c08a64 100644 --- a/src/app/CommandHandlerInterface.h +++ b/src/app/CommandHandlerInterface.h @@ -101,55 +101,24 @@ class CommandHandlerInterface */ virtual void InvokeCommand(HandlerContext & handlerContext) = 0; - typedef Loop (*CommandIdCallback)(CommandId id, void * context); - /** - * Function that may be implemented to enumerate accepted (client-to-server) - * commands for the given cluster. - * - * If this function returns CHIP_ERROR_NOT_IMPLEMENTED, the list of accepted - * commands will come from the endpoint metadata for the cluster. - * - * If this function returns any other error, that will be treated as an - * error condition by the caller, and handling will depend on the caller. - * - * Otherwise the list of accepted commands will be the list of values passed - * to the provided callback. + * Returns true if the given command path can be processed by an InvokeCommand request. * - * The implementation _must_ pass the provided context to the callback. - * - * If the callback returns Loop::Break, there must be no more calls to it. - * This is used by callbacks that just look for a particular value in the - * list. + * Intent for this is that actual command metadata is available somewhere else, so generally code + * is aware of 'all possible commands that a cluster could accept/generate', however this + * allows a command handler interface to filter such commands to a subset. */ - virtual CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) - { - return CHIP_ERROR_NOT_IMPLEMENTED; - } + virtual bool AcceptsCommandId(const ConcreteCommandPath & commandPath) { return true; } /** - * Function that may be implemented to enumerate generated (response) - * commands for the given cluster. + * Returns true if the given command id will be returned as a generated command (i.e. as a response of some InvokeCommand + * request.) * - * If this function returns CHIP_ERROR_NOT_IMPLEMENTED, the list of - * generated commands will come from the endpoint metadata for the cluster. - * - * If this function returns any other error, that will be treated as an - * error condition by the caller, and handling will depend on the caller. - * - * Otherwise the list of generated commands will be the list of values - * passed to the provided callback. - * - * The implementation _must_ pass the provided context to the callback. - * - * If the callback returns Loop::Break, there must be no more calls to it. - * This is used by callbacks that just look for a particular value in the - * list. + * Intent for this is that actual command metadata is available somewhere else, so generally code + * is aware of 'all possible commands that a cluster could accept/generate', however this + * allows a command handler interface to filter such commands to a subset. */ - virtual CHIP_ERROR EnumerateGeneratedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) - { - return CHIP_ERROR_NOT_IMPLEMENTED; - } + virtual bool GeneratesCommandId(const ConcreteCommandPath & commandPath) { return true; } /** * Mechanism for keeping track of a chain of CommandHandlerInterface. diff --git a/src/app/clusters/device-energy-management-server/device-energy-management-server.cpp b/src/app/clusters/device-energy-management-server/device-energy-management-server.cpp index 0b7205dd83a33a..6ac81ff0d6b421 100644 --- a/src/app/clusters/device-energy-management-server/device-energy-management-server.cpp +++ b/src/app/clusters/device-energy-management-server/device-energy-management-server.cpp @@ -104,50 +104,32 @@ CHIP_ERROR Instance::Read(const ConcreteReadAttributePath & aPath, AttributeValu } // CommandHandlerInterface -CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) +bool Instance::AcceptsCommandId(const ConcreteCommandPath & commandPath) { using namespace Commands; - - if (HasFeature(Feature::kPowerAdjustment)) + switch (commandPath.mCommandId) { - for (auto && cmd : { - PowerAdjustRequest::Id, - CancelPowerAdjustRequest::Id, - }) - { - VerifyOrExit(callback(cmd, context) == Loop::Continue, /**/); - } - } - - if (HasFeature(Feature::kStartTimeAdjustment)) - { - VerifyOrExit(callback(StartTimeAdjustRequest::Id, context) == Loop::Continue, /**/); - } - - if (HasFeature(Feature::kPausable)) - { - VerifyOrExit(callback(PauseRequest::Id, context) == Loop::Continue, /**/); - VerifyOrExit(callback(ResumeRequest::Id, context) == Loop::Continue, /**/); - } + case PowerAdjustRequest::Id: + case CancelPowerAdjustRequest::Id: + return HasFeature(Feature::kPowerAdjustment); + case StartTimeAdjustRequest::Id: + return HasFeature(Feature::kStartTimeAdjustment); + case PauseRequest::Id: + case ResumeRequest::Id: + return HasFeature(Feature::kPausable); - if (HasFeature(Feature::kForecastAdjustment)) - { - VerifyOrExit(callback(ModifyForecastRequest::Id, context) == Loop::Continue, /**/); - } + case ModifyForecastRequest::Id: + return HasFeature(Feature::kForecastAdjustment); - if (HasFeature(Feature::kConstraintBasedAdjustment)) - { - VerifyOrExit(callback(RequestConstraintBasedForecast::Id, context) == Loop::Continue, /**/); - } + case RequestConstraintBasedForecast::Id: + return HasFeature(Feature::kConstraintBasedAdjustment); - if (HasFeature(Feature::kStartTimeAdjustment) || HasFeature(Feature::kForecastAdjustment) || - HasFeature(Feature::kConstraintBasedAdjustment)) - { - VerifyOrExit(callback(CancelRequest::Id, context) == Loop::Continue, /**/); + case CancelRequest::Id: + return HasFeature(Feature::kStartTimeAdjustment) || HasFeature(Feature::kForecastAdjustment) || + HasFeature(Feature::kConstraintBasedAdjustment); + default: + return false; } - -exit: - return CHIP_NO_ERROR; } void Instance::InvokeCommand(HandlerContext & handlerContext) diff --git a/src/app/clusters/device-energy-management-server/device-energy-management-server.h b/src/app/clusters/device-energy-management-server/device-energy-management-server.h index c58e5c5a73743a..f886612e69deb2 100644 --- a/src/app/clusters/device-energy-management-server/device-energy-management-server.h +++ b/src/app/clusters/device-energy-management-server/device-energy-management-server.h @@ -235,7 +235,7 @@ class Instance : public AttributeAccessInterface, public CommandHandlerInterface // CommandHandlerInterface void InvokeCommand(HandlerContext & handlerContext) override; - CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override; + bool AcceptsCommandId(const ConcreteCommandPath & commandPath) override; Protocols::InteractionModel::Status CheckOptOutAllowsRequest(AdjustmentCauseEnum adjustmentCause); void HandlePowerAdjustRequest(HandlerContext & ctx, const Commands::PowerAdjustRequest::DecodableType & commandData); diff --git a/src/app/clusters/energy-evse-server/energy-evse-server.cpp b/src/app/clusters/energy-evse-server/energy-evse-server.cpp index d4ae59307cfea1..7dab0d97c9772f 100644 --- a/src/app/clusters/energy-evse-server/energy-evse-server.cpp +++ b/src/app/clusters/energy-evse-server/energy-evse-server.cpp @@ -182,42 +182,26 @@ CHIP_ERROR Instance::Write(const ConcreteDataAttributePath & aPath, AttributeVal } // CommandHandlerInterface -CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) +bool Instance::AcceptsCommandId(const ConcreteCommandPath & commandPath) { using namespace Commands; - for (auto && cmd : { - Disable::Id, - EnableCharging::Id, - }) + switch (commandPath.mCommandId) { - VerifyOrExit(callback(cmd, context) == Loop::Continue, /**/); - } - - if (HasFeature(Feature::kV2x)) - { - VerifyOrExit(callback(EnableDischarging::Id, context) == Loop::Continue, /**/); - } - - if (HasFeature(Feature::kChargingPreferences)) - { - for (auto && cmd : { - SetTargets::Id, - GetTargets::Id, - ClearTargets::Id, - }) - { - VerifyOrExit(callback(cmd, context) == Loop::Continue, /**/); - } - } - - if (SupportsOptCmd(OptionalCommands::kSupportsStartDiagnostics)) - { - callback(StartDiagnostics::Id, context); + case Disable::Id: + case EnableCharging::Id: + return true; + case EnableDischarging::Id: + return HasFeature(Feature::kV2x); + case SetTargets::Id: + case GetTargets::Id: + case ClearTargets::Id: + return HasFeature(Feature::kChargingPreferences); + case StartDiagnostics::Id: + return SupportsOptCmd(OptionalCommands::kSupportsStartDiagnostics); + default: + return false; } - -exit: - return CHIP_NO_ERROR; } void Instance::InvokeCommand(HandlerContext & handlerContext) diff --git a/src/app/clusters/energy-evse-server/energy-evse-server.h b/src/app/clusters/energy-evse-server/energy-evse-server.h index 979e44c040a17b..b3852773cc66fc 100644 --- a/src/app/clusters/energy-evse-server/energy-evse-server.h +++ b/src/app/clusters/energy-evse-server/energy-evse-server.h @@ -200,7 +200,7 @@ class Instance : public AttributeAccessInterface, public CommandHandlerInterface // CommandHandlerInterface void InvokeCommand(HandlerContext & handlerContext) override; - CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override; + bool AcceptsCommandId(const ConcreteCommandPath & commandPath) override; void HandleDisable(HandlerContext & ctx, const Commands::Disable::DecodableType & commandData); void HandleEnableCharging(HandlerContext & ctx, const Commands::EnableCharging::DecodableType & commandData); diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp index ea73b8ff23d9eb..7778e32a9755ca 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.cpp +++ b/src/app/clusters/network-commissioning/network-commissioning.cpp @@ -1360,65 +1360,43 @@ void Instance::OnFailSafeTimerExpired() } } -CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) +bool Instance::AcceptsCommandId(const ConcreteCommandPath & commandPath) { using namespace Clusters::NetworkCommissioning::Commands; - if (mFeatureFlags.Has(Feature::kThreadNetworkInterface)) + switch (commandPath.mCommandId) { - for (auto && cmd : { - ScanNetworks::Id, - AddOrUpdateThreadNetwork::Id, - RemoveNetwork::Id, - ConnectNetwork::Id, - ReorderNetwork::Id, - }) - { - VerifyOrExit(callback(cmd, context) == Loop::Continue, /**/); - } - } - else if (mFeatureFlags.Has(Feature::kWiFiNetworkInterface)) - { - for (auto && cmd : { - ScanNetworks::Id, - AddOrUpdateWiFiNetwork::Id, - RemoveNetwork::Id, - ConnectNetwork::Id, - ReorderNetwork::Id, - }) - { - VerifyOrExit(callback(cmd, context) == Loop::Continue, /**/); - } - } + case AddOrUpdateThreadNetwork::Id: + return mFeatureFlags.Has(Feature::kThreadNetworkInterface); + case AddOrUpdateWiFiNetwork::Id: + return mFeatureFlags.Has(Feature::kWiFiNetworkInterface); + case ScanNetworks::Id: + case RemoveNetwork::Id: + case ConnectNetwork::Id: + case ReorderNetwork::Id: + return mFeatureFlags.HasAny(Feature::kThreadNetworkInterface, Feature::kWiFiNetworkInterface); + case QueryIdentity::Id: - if (mFeatureFlags.Has(Feature::kPerDeviceCredentials)) - { - VerifyOrExit(callback(QueryIdentity::Id, context) == Loop::Continue, /**/); + return mFeatureFlags.Has(Feature::kPerDeviceCredentials); + default: + return false; } - -exit: - return CHIP_NO_ERROR; } -CHIP_ERROR Instance::EnumerateGeneratedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) +bool Instance::GeneratesCommandId(const ConcreteCommandPath & commandPath) { using namespace Clusters::NetworkCommissioning::Commands; - - if (mFeatureFlags.HasAny(Feature::kWiFiNetworkInterface, Feature::kThreadNetworkInterface)) - { - for (auto && cmd : { ScanNetworksResponse::Id, NetworkConfigResponse::Id, ConnectNetworkResponse::Id }) - { - VerifyOrExit(callback(cmd, context) == Loop::Continue, /**/); - } - } - - if (mFeatureFlags.Has(Feature::kPerDeviceCredentials)) - { - VerifyOrExit(callback(QueryIdentityResponse::Id, context) == Loop::Continue, /**/); + switch (commandPath.mCommandId) + { + case ScanNetworksResponse::Id: + case NetworkConfigResponse::Id: + case ConnectNetworkResponse::Id: + return mFeatureFlags.HasAny(Feature::kWiFiNetworkInterface, Feature::kThreadNetworkInterface); + case QueryIdentityResponse::Id: + return mFeatureFlags.Has(Feature::kPerDeviceCredentials); + default: + return false; } - -exit: - return CHIP_NO_ERROR; } bool NullNetworkDriver::GetEnabled() diff --git a/src/app/clusters/network-commissioning/network-commissioning.h b/src/app/clusters/network-commissioning/network-commissioning.h index d727c9b6c2beb9..ca25531161b380 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.h +++ b/src/app/clusters/network-commissioning/network-commissioning.h @@ -59,8 +59,8 @@ class Instance : public CommandHandlerInterface, // CommandHandlerInterface void InvokeCommand(HandlerContext & ctx) override; - CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override; - CHIP_ERROR EnumerateGeneratedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override; + bool AcceptsCommandId(const ConcreteCommandPath & commandPath) override; + bool GeneratesCommandId(const ConcreteCommandPath & commandPath) override; // AttributeAccessInterface CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override; diff --git a/src/app/clusters/resource-monitoring-server/resource-monitoring-server.cpp b/src/app/clusters/resource-monitoring-server/resource-monitoring-server.cpp index 6c581ff72a9e19..116fdb5265813d 100644 --- a/src/app/clusters/resource-monitoring-server/resource-monitoring-server.cpp +++ b/src/app/clusters/resource-monitoring-server/resource-monitoring-server.cpp @@ -186,17 +186,16 @@ void Instance::InvokeCommand(HandlerContext & handlerContext) } } -// List the commands supported by this instance. -CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, - CommandHandlerInterface::CommandIdCallback callback, void * context) +// Commands supported by this instance. +bool Instance::AcceptsCommandId(const ConcreteCommandPath & commandPath) { - ChipLogDetail(Zcl, "resourcemonitoring: EnumerateAcceptedCommands"); - if (mResetConditionCommandSupported) + switch (commandPath.mCommandId) { - callback(ResourceMonitoring::Commands::ResetCondition::Id, context); + case ResourceMonitoring::Commands::ResetCondition::Id: + return mResetConditionCommandSupported; + default: + return false; } - - return CHIP_NO_ERROR; } CHIP_ERROR Instance::ReadReplaceableProductList(AttributeValueEncoder & aEncoder) diff --git a/src/app/clusters/resource-monitoring-server/resource-monitoring-server.h b/src/app/clusters/resource-monitoring-server/resource-monitoring-server.h index 45c96c39f30074..c6ebe644f42458 100644 --- a/src/app/clusters/resource-monitoring-server/resource-monitoring-server.h +++ b/src/app/clusters/resource-monitoring-server/resource-monitoring-server.h @@ -132,7 +132,7 @@ class Instance : public CommandHandlerInterface, public AttributeAccessInterface // CommandHandlerInterface void InvokeCommand(HandlerContext & ctx) override; - CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override; + bool AcceptsCommandId(const ConcreteCommandPath & commandPath) override; // AttributeAccessInterface CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override; diff --git a/src/app/clusters/software-diagnostics-server/software-diagnostics-server.cpp b/src/app/clusters/software-diagnostics-server/software-diagnostics-server.cpp index 9f9235a16f46d8..c6e99f69133bd3 100644 --- a/src/app/clusters/software-diagnostics-server/software-diagnostics-server.cpp +++ b/src/app/clusters/software-diagnostics-server/software-diagnostics-server.cpp @@ -62,7 +62,7 @@ class SoftwareDiagnosticsCommandHandler : public CommandHandlerInterface void InvokeCommand(HandlerContext & handlerContext) override; - CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override; + bool AcceptsCommandId(const ConcreteCommandPath & commandPath) override; }; SoftwareDiagosticsAttrAccess gAttrAccess; @@ -168,21 +168,18 @@ void SoftwareDiagnosticsCommandHandler::InvokeCommand(HandlerContext & handlerCo handlerContext.mCommandHandler.AddStatus(handlerContext.mRequestPath, status); } -CHIP_ERROR SoftwareDiagnosticsCommandHandler::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, - CommandIdCallback callback, void * context) +bool SoftwareDiagnosticsCommandHandler::AcceptsCommandId(const ConcreteCommandPath & commandPath) { - if (!DeviceLayer::GetDiagnosticDataProvider().SupportsWatermarks()) + switch (commandPath.mCommandId) { - // No commmands. - return CHIP_NO_ERROR; + case Commands::ResetWatermarks::Id: + return DeviceLayer::GetDiagnosticDataProvider().SupportsWatermarks(); + default: + return false; } - - callback(Commands::ResetWatermarks::Id, context); - - return CHIP_NO_ERROR; } -} // anonymous namespace +} // namespace namespace chip { namespace app { diff --git a/src/app/util/ember-global-attribute-access-interface.cpp b/src/app/util/ember-global-attribute-access-interface.cpp index 9878f41d45f83e..47ef5a75ea3896 100644 --- a/src/app/util/ember-global-attribute-access-interface.cpp +++ b/src/app/util/ember-global-attribute-access-interface.cpp @@ -13,6 +13,10 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "app/ConcreteCommandPath.h" +#include "lib/core/CHIPError.h" +#include "lib/core/DataModelTypes.h" +#include "lib/support/CodeUtils.h" #include #include @@ -58,12 +62,41 @@ CHIP_ERROR GlobalAttributeReader::Read(const ConcreteReadAttributePath & aPath, } return CHIP_NO_ERROR; }); - case AcceptedCommandList::Id: - return EncodeCommandList(aPath, aEncoder, &CommandHandlerInterface::EnumerateAcceptedCommands, - mCluster->acceptedCommandList); - case GeneratedCommandList::Id: - return EncodeCommandList(aPath, aEncoder, &CommandHandlerInterface::EnumerateGeneratedCommands, - mCluster->generatedCommandList); + case AcceptedCommandList::Id: { + auto * commandHandler = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(aPath.mEndpointId, aPath.mClusterId); + return aEncoder.EncodeList([&](const auto & encoder) { + if (mCluster->acceptedCommandList != nullptr) + { + for (unsigned i = 0; mCluster->acceptedCommandList[i] != kInvalidCommandId; i++) + { + const ConcreteCommandPath commandPath(aPath.mEndpointId, aPath.mClusterId, mCluster->acceptedCommandList[i]); + if ((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath)) + { + ReturnErrorOnFailure(encoder.Encode(commandPath.mCommandId)); + } + } + } + return CHIP_NO_ERROR; + }); + } + + case GeneratedCommandList::Id: { + auto * commandHandler = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(aPath.mEndpointId, aPath.mClusterId); + return aEncoder.EncodeList([&](const auto & encoder) { + if (mCluster->generatedCommandList != nullptr) + { + for (unsigned i = 0; mCluster->generatedCommandList[i] != kInvalidCommandId; i++) + { + const ConcreteCommandPath commandPath(aPath.mEndpointId, aPath.mClusterId, mCluster->generatedCommandList[i]); + if ((commandHandler == nullptr) || commandHandler->GeneratesCommandId(commandPath)) + { + ReturnErrorOnFailure(encoder.Encode(commandPath.mCommandId)); + } + } + } + return CHIP_NO_ERROR; + }); + } default: // This function is only called if attributeCluster is non-null in // ReadSingleClusterData, which only happens for attributes listed in @@ -75,47 +108,6 @@ CHIP_ERROR GlobalAttributeReader::Read(const ConcreteReadAttributePath & aPath, } } -CHIP_ERROR GlobalAttributeReader::EncodeCommandList(const ConcreteClusterPath & aClusterPath, AttributeValueEncoder & aEncoder, - GlobalAttributeReader::CommandListEnumerator aEnumerator, - const CommandId * aClusterCommandList) -{ - return aEncoder.EncodeList([&](const auto & encoder) { - auto * commandHandler = - CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(aClusterPath.mEndpointId, aClusterPath.mClusterId); - if (commandHandler) - { - struct Context - { - decltype(encoder) & commandIdEncoder; - CHIP_ERROR err; - } context{ encoder, CHIP_NO_ERROR }; - CHIP_ERROR err = (commandHandler->*aEnumerator)( - aClusterPath, - [](CommandId command, void * closure) -> Loop { - auto * ctx = static_cast(closure); - ctx->err = ctx->commandIdEncoder.Encode(command); - if (ctx->err != CHIP_NO_ERROR) - { - return Loop::Break; - } - return Loop::Continue; - }, - &context); - if (err != CHIP_ERROR_NOT_IMPLEMENTED) - { - return context.err; - } - // Else fall through to the list in aClusterCommandList. - } - - for (const CommandId * cmd = aClusterCommandList; cmd != nullptr && *cmd != kInvalidCommandId; cmd++) - { - ReturnErrorOnFailure(encoder.Encode(*cmd)); - } - return CHIP_NO_ERROR; - }); -} - } // namespace Compatibility } // namespace app } // namespace chip diff --git a/src/app/util/ember-global-attribute-access-interface.h b/src/app/util/ember-global-attribute-access-interface.h index d17e1b286dbd81..e7e8557c18fae9 100644 --- a/src/app/util/ember-global-attribute-access-interface.h +++ b/src/app/util/ember-global-attribute-access-interface.h @@ -42,13 +42,6 @@ class GlobalAttributeReader : public MandatoryGlobalAttributeReader GlobalAttributeReader(const EmberAfCluster * aCluster) : MandatoryGlobalAttributeReader(aCluster) {} CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override; - -private: - typedef CHIP_ERROR (CommandHandlerInterface::*CommandListEnumerator)(const ConcreteClusterPath & cluster, - CommandHandlerInterface::CommandIdCallback callback, - void * context); - static CHIP_ERROR EncodeCommandList(const ConcreteClusterPath & aClusterPath, AttributeValueEncoder & aEncoder, - CommandListEnumerator aEnumerator, const CommandId * aClusterCommandList); }; } // namespace Compatibility diff --git a/src/controller/tests/TestServerCommandDispatch.cpp b/src/controller/tests/TestServerCommandDispatch.cpp index 2389ce08e6baf2..6d742ba14b64f7 100644 --- a/src/controller/tests/TestServerCommandDispatch.cpp +++ b/src/controller/tests/TestServerCommandDispatch.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -69,7 +70,7 @@ class TestClusterCommandHandler : public chip::app::CommandHandlerInterface private: void InvokeCommand(chip::app::CommandHandlerInterface::HandlerContext & handlerContext) final; - CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) final; + bool AcceptsCommandId(const ConcreteCommandPath & path) override; bool mOverrideAcceptedCommands = false; bool mClaimNoCommands = false; @@ -104,28 +105,22 @@ void TestClusterCommandHandler::InvokeCommand(chip::app::CommandHandlerInterface }); } -CHIP_ERROR TestClusterCommandHandler::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, - CommandHandlerInterface::CommandIdCallback callback, void * context) +bool TestClusterCommandHandler::AcceptsCommandId(const ConcreteCommandPath & path) { if (!mOverrideAcceptedCommands) { - return CHIP_ERROR_NOT_IMPLEMENTED; + return true; // anything accepted } if (mClaimNoCommands) { - return CHIP_NO_ERROR; + return false; // deny all } // We just have one command id. - callback(Clusters::UnitTesting::Commands::TestSimpleArgumentRequest::Id, context); - return CHIP_NO_ERROR; + return path.mCommandId == Clusters::UnitTesting::Commands::TestSimpleArgumentRequest::Id; } -} // namespace - -namespace { - class DispatchTestDataModel : public CodegenDataModelProvider { public: @@ -139,12 +134,12 @@ class DispatchTestDataModel : public CodegenDataModelProvider class TestServerCommandDispatch : public chip::Test::AppContext { public: - void SetUp() + void SetUp() override { AppContext::SetUp(); mOldProvider = InteractionModelEngine::GetInstance()->SetDataModelProvider(&DispatchTestDataModel::Instance()); } - void TearDown() + void TearDown() override { InteractionModelEngine::GetInstance()->SetDataModelProvider(mOldProvider); AppContext::TearDown(); @@ -378,14 +373,20 @@ TEST_F(TestServerCommandDispatch, TestDataResponseHandlerOverride1) { TestClusterCommandHandler commandHandler; commandHandler.OverrideAcceptedCommands(); - TestDataResponseHelper(&testEndpoint2, true); + + // Clusters::UnitTesting exists on testEndpoint1, so metadata about generated commands + // exists + TestDataResponseHelper(&testEndpoint1, true); } TEST_F(TestServerCommandDispatch, TestDataResponseHandlerOverride2) { TestClusterCommandHandler commandHandler; commandHandler.OverrideAcceptedCommands(); - TestDataResponseHelper(&testEndpoint3, true); + + // Clusters::UnitTesting DOES NOT exist on testEndpoint3 + // so overriding accepting does nothing (there is no metadata to accept it) + TestDataResponseHelper(&testEndpoint3, false); } } // namespace diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index 95bac8fbbf82f3..293a3c4fb1c43a 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -41,81 +41,9 @@ namespace chip { namespace app { -namespace detail { - -Loop EnumeratorCommandFinder::HandlerCallback(CommandId id) -{ - switch (mOperation) - { - case Operation::kFindFirst: - mFound = id; - return Loop::Break; - case Operation::kFindExact: - if (mTarget == id) - { - mFound = id; // found it - return Loop::Break; - } - break; - case Operation::kFindNext: - if (mTarget == id) - { - // Once we found the ID, get the first - mOperation = Operation::kFindFirst; - } - break; - } - return Loop::Continue; // keep searching -} - -std::optional EnumeratorCommandFinder::FindCommandId(Operation operation, const ConcreteCommandPath & path) -{ - mOperation = operation; - mTarget = path.mCommandId; - - CommandHandlerInterface * interface = - CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId); - - if (interface == nullptr) - { - return std::nullopt; // no data: no interface - } - - CHIP_ERROR err = (interface->*mCallback)(path, HandlerCallbackFn, this); - if (err == CHIP_ERROR_NOT_IMPLEMENTED) - { - return std::nullopt; // no data provided by the interface - } - - if (err != CHIP_NO_ERROR) - { -#if CHIP_CONFIG_DATA_MODEL_EXTRA_LOGGING - // Report the error here since we lose actual error. This generally should NOT be possible as CommandHandlerInterface - // usually returns unimplemented or should just work for our use case (our callback never fails) - ChipLogError(DataManagement, "Enumerate error: %" CHIP_ERROR_FORMAT, err.Format()); -#endif - return kInvalidCommandId; - } - - return mFound.value_or(kInvalidCommandId); -} - -} // namespace detail - -using detail::EnumeratorCommandFinder; namespace { -const CommandId * AcceptedCommands(const EmberAfCluster & cluster) -{ - return cluster.acceptedCommandList; -} - -const CommandId * GeneratedCommands(const EmberAfCluster & cluster) -{ - return cluster.generatedCommandList; -} - /// Load the cluster information into the specified destination std::variant LoadClusterInfo(const ConcreteClusterPath & path, const EmberAfCluster & cluster) { @@ -749,35 +677,6 @@ const EmberAfCluster * CodegenDataModelProvider::FindServerCluster(const Concret return cluster; } -CommandId CodegenDataModelProvider::FindCommand(const ConcreteCommandPath & path, detail::EnumeratorCommandFinder & handlerFinder, - detail::EnumeratorCommandFinder::Operation operation, - CodegenDataModelProvider::EmberCommandListIterator & emberIterator, - CommandListGetter commandListGetter) -{ - - std::optional handlerCommandId = handlerFinder.FindCommandId(operation, path); - if (handlerCommandId.has_value()) - { - return *handlerCommandId; - } - - const EmberAfCluster * cluster = FindServerCluster(path); - VerifyOrReturnValue(cluster != nullptr, kInvalidCommandId); - - const CommandId * commandList = commandListGetter(*cluster); - - switch (operation) - { - case EnumeratorCommandFinder::Operation::kFindFirst: - return emberIterator.First(commandList).value_or(kInvalidCommandId); - case EnumeratorCommandFinder::Operation::kFindNext: - return emberIterator.Next(commandList, path.mCommandId).value_or(kInvalidCommandId); - case EnumeratorCommandFinder::Operation::kFindExact: - default: - return emberIterator.Exists(commandList, path.mCommandId) ? path.mCommandId : kInvalidCommandId; - } -} - DataModel::AttributeEntry CodegenDataModelProvider::NextAttribute(const ConcreteAttributePath & before) { const EmberAfCluster * cluster = FindServerCluster(before); @@ -825,58 +724,110 @@ std::optional CodegenDataModelProvider::GetAttributeIn DataModel::CommandEntry CodegenDataModelProvider::FirstAcceptedCommand(const ConcreteClusterPath & path) { - EnumeratorCommandFinder handlerFinder(&CommandHandlerInterface::EnumerateAcceptedCommands); + const EmberAfCluster * cluster = FindServerCluster(path); - CommandId commandId = - FindCommand(ConcreteCommandPath(path.mEndpointId, path.mClusterId, kInvalidCommandId), handlerFinder, - detail::EnumeratorCommandFinder::Operation::kFindFirst, mAcceptedCommandsIterator, AcceptedCommands); + VerifyOrReturnValue(cluster != nullptr, DataModel::CommandEntry::kInvalid); + VerifyOrReturnValue(cluster->acceptedCommandList != nullptr, DataModel::CommandEntry::kInvalid); - VerifyOrReturnValue(commandId != kInvalidCommandId, DataModel::CommandEntry::kInvalid); - return CommandEntryFrom(path, commandId); + auto * commandHandler = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId); + + std::optional mCommand = mAcceptedCommandsIterator.First(cluster->acceptedCommandList); + while (mCommand.has_value()) + { + const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, *mCommand); + if ((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath)) + { + return CommandEntryFrom(path, commandPath.mCommandId); + } + mCommand = mAcceptedCommandsIterator.Next(cluster->acceptedCommandList, *mCommand); + } + return DataModel::CommandEntry::kInvalid; } DataModel::CommandEntry CodegenDataModelProvider::NextAcceptedCommand(const ConcreteCommandPath & before) { + const EmberAfCluster * cluster = FindServerCluster(before); + + VerifyOrReturnValue(cluster != nullptr, DataModel::CommandEntry::kInvalid); + VerifyOrReturnValue(cluster->acceptedCommandList != nullptr, DataModel::CommandEntry::kInvalid); - EnumeratorCommandFinder handlerFinder(&CommandHandlerInterface::EnumerateAcceptedCommands); - CommandId commandId = FindCommand(before, handlerFinder, detail::EnumeratorCommandFinder::Operation::kFindNext, - mAcceptedCommandsIterator, AcceptedCommands); + auto * commandHandler = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(before.mEndpointId, before.mClusterId); - VerifyOrReturnValue(commandId != kInvalidCommandId, DataModel::CommandEntry::kInvalid); - return CommandEntryFrom(before, commandId); + std::optional mCommand = mAcceptedCommandsIterator.Next(cluster->acceptedCommandList, before.mCommandId); + while (mCommand.has_value()) + { + const ConcreteCommandPath commandPath(before.mEndpointId, before.mClusterId, *mCommand); + if ((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath)) + { + return CommandEntryFrom(before, commandPath.mCommandId); + } + mCommand = mAcceptedCommandsIterator.Next(cluster->acceptedCommandList, *mCommand); + } + return DataModel::CommandEntry::kInvalid; } std::optional CodegenDataModelProvider::GetAcceptedCommandInfo(const ConcreteCommandPath & path) { + const EmberAfCluster * cluster = FindServerCluster(path); + + VerifyOrReturnValue(cluster != nullptr, std::nullopt); + VerifyOrReturnValue(cluster->acceptedCommandList != nullptr, std::nullopt); + + auto * commandHandler = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId); + + VerifyOrReturnValue(mAcceptedCommandsIterator.Exists(cluster->acceptedCommandList, path.mCommandId), std::nullopt); - EnumeratorCommandFinder handlerFinder(&CommandHandlerInterface::EnumerateAcceptedCommands); - CommandId commandId = FindCommand(path, handlerFinder, detail::EnumeratorCommandFinder::Operation::kFindExact, - mAcceptedCommandsIterator, AcceptedCommands); + const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, path.mCommandId); + VerifyOrReturnValue((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath), std::nullopt); - VerifyOrReturnValue(commandId != kInvalidCommandId, std::nullopt); - return CommandEntryFrom(path, commandId).info; + return CommandEntryFrom(path, commandPath.mCommandId).info; } ConcreteCommandPath CodegenDataModelProvider::FirstGeneratedCommand(const ConcreteClusterPath & path) { - EnumeratorCommandFinder handlerFinder(&CommandHandlerInterface::EnumerateGeneratedCommands); - CommandId commandId = - FindCommand(ConcreteCommandPath(path.mEndpointId, path.mClusterId, kInvalidCommandId), handlerFinder, - detail::EnumeratorCommandFinder::Operation::kFindFirst, mGeneratedCommandsIterator, GeneratedCommands); + const EmberAfCluster * cluster = FindServerCluster(path); + + VerifyOrReturnValue(cluster != nullptr, ConcreteCommandPath()); + VerifyOrReturnValue(cluster->generatedCommandList != nullptr, ConcreteCommandPath()); + + auto * commandHandler = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId); - VerifyOrReturnValue(commandId != kInvalidCommandId, kInvalidCommandPath); - return ConcreteCommandPath(path.mEndpointId, path.mClusterId, commandId); + std::optional mCommand = mGeneratedCommandsIterator.First(cluster->generatedCommandList); + while (mCommand.has_value()) + { + const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, *mCommand); + if ((commandHandler == nullptr) || commandHandler->GeneratesCommandId(commandPath)) + { + return commandPath; + } + mCommand = mGeneratedCommandsIterator.Next(cluster->generatedCommandList, *mCommand); + } + return {}; } ConcreteCommandPath CodegenDataModelProvider::NextGeneratedCommand(const ConcreteCommandPath & before) { - EnumeratorCommandFinder handlerFinder(&CommandHandlerInterface::EnumerateGeneratedCommands); + const EmberAfCluster * cluster = FindServerCluster(before); + + VerifyOrReturnValue(cluster != nullptr, ConcreteCommandPath()); + VerifyOrReturnValue(cluster->generatedCommandList != nullptr, ConcreteCommandPath()); + + // TODO: this does NOT make use of the hint because the command list is NOT sized (it is value-terminated) + // and we have no way to check `is hint in bounds` + auto * commandHandler = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(before.mEndpointId, before.mClusterId); - CommandId commandId = FindCommand(before, handlerFinder, detail::EnumeratorCommandFinder::Operation::kFindNext, - mGeneratedCommandsIterator, GeneratedCommands); + std::optional mCommand = mGeneratedCommandsIterator.Next(cluster->generatedCommandList, before.mCommandId); + while (mCommand.has_value()) + { + const ConcreteCommandPath commandPath(before.mEndpointId, before.mClusterId, *mCommand); + if ((commandHandler == nullptr) || commandHandler->GeneratesCommandId(commandPath)) + { + return commandPath; + } + mCommand = mGeneratedCommandsIterator.Next(cluster->generatedCommandList, *mCommand); + } - VerifyOrReturnValue(commandId != kInvalidCommandId, kInvalidCommandPath); - return ConcreteCommandPath(before.mEndpointId, before.mClusterId, commandId); + return {}; } std::optional CodegenDataModelProvider::FirstDeviceType(EndpointId endpoint) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.h b/src/data-model-providers/codegen/CodegenDataModelProvider.h index 0493272dbbff47..e87be1fc5f28dd 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.h +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.h @@ -27,61 +27,6 @@ namespace chip { namespace app { -namespace detail { - -/// Handles going through callback-based enumeration of generated/accepted commands -/// for CommandHandlerInterface based items. -/// -/// Offers the ability to focus on some operation for finding a given -/// command id: -/// - FindFirst will return the first found element -/// - FindExact finds the element with the given id -/// - FindNext finds the element following the given id -class EnumeratorCommandFinder -{ -public: - using HandlerCallbackFunction = CHIP_ERROR (CommandHandlerInterface::*)(const ConcreteClusterPath &, - CommandHandlerInterface::CommandIdCallback, void *); - - enum class Operation - { - kFindFirst, // Find the first value in the list - kFindExact, // Find the given value - kFindNext // Find the value AFTER this value - }; - - EnumeratorCommandFinder(HandlerCallbackFunction callback) : - mCallback(callback), mOperation(Operation::kFindFirst), mTarget(kInvalidCommandId) - {} - - /// Find the given command ID that matches the given operation/path. - /// - /// If operation is kFindFirst, then path commandID is ignored. Otherwise it is used as a key to - /// kFindExact or kFindNext. - /// - /// Returns: - /// - std::nullopt if no command found using the command handler interface - /// - kInvalidCommandId if the find failed (but command handler interface does provide a list) - /// - valid id if command handler interface usage succeeds - std::optional FindCommandId(Operation operation, const ConcreteCommandPath & path); - -private: - HandlerCallbackFunction mCallback; - Operation mOperation; - CommandId mTarget; - std::optional mFound = std::nullopt; - - Loop HandlerCallback(CommandId id); - - static Loop HandlerCallbackFn(CommandId id, void * context) - { - auto self = static_cast(context); - return self->HandlerCallback(id); - } -}; - -} // namespace detail - /// An implementation of `InteractionModel::Model` that relies on code-generation /// via zap/ember. /// @@ -236,10 +181,6 @@ class CodegenDataModelProvider : public DataModel::Provider std::optional TryFindEndpointIndex(EndpointId id) const; using CommandListGetter = const CommandId *(const EmberAfCluster &); - - CommandId FindCommand(const ConcreteCommandPath & path, detail::EnumeratorCommandFinder & handlerFinder, - detail::EnumeratorCommandFinder::Operation operation, - CodegenDataModelProvider::EmberCommandListIterator & emberIterator, CommandListGetter commandListGetter); }; } // namespace app diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp index 302d26a8941cd0..44c3433f156e10 100644 --- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp +++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp @@ -220,32 +220,14 @@ class CustomListCommandHandler : public CommandHandlerInterface void InvokeCommand(HandlerContext & handlerContext) override { handlerContext.SetCommandNotHandled(); } - CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override + bool AcceptsCommandId(const app::ConcreteCommandPath & path) override { - VerifyOrReturnError(mOverrideAccepted, CHIP_ERROR_NOT_IMPLEMENTED); - - for (auto id : mAccepted) - { - if (callback(id, context) != Loop::Continue) - { - break; - } - } - return CHIP_NO_ERROR; + return mOverrideAccepted && std::find(mAccepted.cbegin(), mAccepted.cend(), path.mCommandId) != mAccepted.cend(); } - CHIP_ERROR EnumerateGeneratedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override + bool GeneratesCommandId(const app::ConcreteCommandPath & path) override { - VerifyOrReturnError(mOverrideGenerated, CHIP_ERROR_NOT_IMPLEMENTED); - - for (auto id : mGenerated) - { - if (callback(id, context) != Loop::Continue) - { - break; - } - } - return CHIP_NO_ERROR; + return mOverrideGenerated && std::find(mGenerated.cbegin(), mGenerated.cend(), path.mCommandId) != mGenerated.cend(); } void SetOverrideAccepted(bool o) { mOverrideAccepted = o; } @@ -283,11 +265,17 @@ class ScopedMockAccessControl // clang-format off const MockNodeConfig gTestNodeConfig({ MockEndpointConfig(kMockEndpoint1, { - MockClusterConfig(MockClusterId(1), { - ClusterRevision::Id, FeatureMap::Id, - }, { + MockClusterConfig( + MockClusterId(1), + { + ClusterRevision::Id, FeatureMap::Id, + }, /* attributes */ + { MockEventId(1), MockEventId(2), - }), + }, /* events */ + {100, 1234, 999, 2000, 3000}, /* acceptedCommands */ + {33, 44, 55, 66} /* generatedCommands */ + ), MockClusterConfig(MockClusterId(2), { ClusterRevision::Id, FeatureMap::Id, MockAttributeId(1), }),