From 216860982694ab970bfedc21c8b755e8eb72314c Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 11 Dec 2024 15:06:35 -0500 Subject: [PATCH 01/15] Initial pass: replace all enumerate call implementations with equivalent bools --- src/app/CommandHandlerInterface.h | 53 ++++-------------- .../device-energy-management-server.cpp | 56 +++++++------------ .../device-energy-management-server.h | 2 +- .../energy-evse-server/energy-evse-server.cpp | 46 +++++---------- .../energy-evse-server/energy-evse-server.h | 2 +- .../network-commissioning.cpp | 49 ++++++---------- .../network-commissioning.h | 4 +- .../resource-monitoring-server.cpp | 15 +++-- .../resource-monitoring-server.h | 2 +- .../software-diagnostics-server.cpp | 19 +++---- 10 files changed, 80 insertions(+), 168 deletions(-) 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..87d446e886af02 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.cpp +++ b/src/app/clusters/network-commissioning/network-commissioning.cpp @@ -1360,47 +1360,30 @@ 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; 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..3a9e2f2cd51d3e 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,20 +168,15 @@ 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 chip { From 64be8747e964109f50f2d1678d6b31ba73102b22 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 11 Dec 2024 15:45:38 -0500 Subject: [PATCH 02/15] Start a bit of an implementation in codegen... likely still broken --- ...mber-global-attribute-access-interface.cpp | 86 +++---- .../ember-global-attribute-access-interface.h | 7 - .../codegen/CodegenDataModelProvider.cpp | 234 ++++++++---------- .../codegen/CodegenDataModelProvider.h | 61 +---- .../tests/TestCodegenModelViaMocks.cpp | 26 +- 5 files changed, 151 insertions(+), 263 deletions(-) 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/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index 95bac8fbbf82f3..30728d8b8c47a0 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,137 @@ 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->generatedCommandList != nullptr, DataModel::CommandEntry::kInvalid); - VerifyOrReturnValue(commandId != kInvalidCommandId, DataModel::CommandEntry::kInvalid); - return CommandEntryFrom(path, commandId); + auto * commandHandler = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId); + for (unsigned i = 0; cluster->generatedCommandList[i] != kInvalidCommandId; i++) + { + const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, cluster->generatedCommandList[i]); + if ((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath)) + { + mAcceptedCommandHint = i; + return CommandEntryFrom(path, commandPath.mCommandId); + } + } + return DataModel::CommandEntry::kInvalid; } DataModel::CommandEntry CodegenDataModelProvider::NextAcceptedCommand(const ConcreteCommandPath & before) { + const EmberAfCluster * cluster = FindServerCluster(before); + + VerifyOrReturnValue(cluster != nullptr, DataModel::CommandEntry::kInvalid); + VerifyOrReturnValue(cluster->generatedCommandList != nullptr, DataModel::CommandEntry::kInvalid); + + // 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); + unsigned beforeIdx; + for (beforeIdx = 0; cluster->generatedCommandList[beforeIdx] != kInvalidCommandId; beforeIdx++) + { + if (cluster->generatedCommandList[beforeIdx] == before.mCommandId) + { + break; // found it + } + } - EnumeratorCommandFinder handlerFinder(&CommandHandlerInterface::EnumerateAcceptedCommands); - CommandId commandId = FindCommand(before, handlerFinder, detail::EnumeratorCommandFinder::Operation::kFindNext, - mAcceptedCommandsIterator, AcceptedCommands); + VerifyOrReturnValue(cluster->generatedCommandList[beforeIdx] == before.mCommandId, DataModel::CommandEntry::kInvalid); - VerifyOrReturnValue(commandId != kInvalidCommandId, DataModel::CommandEntry::kInvalid); - return CommandEntryFrom(before, commandId); + // find the first "accepted" index out if thios + for (unsigned i = beforeIdx + 1; cluster->generatedCommandList[i] != kInvalidCommandId; i++) + { + const ConcreteCommandPath commandPath(before.mEndpointId, before.mClusterId, cluster->generatedCommandList[i]); + if ((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath)) + { + mAcceptedCommandHint = i; + return CommandEntryFrom(before, commandPath.mCommandId); + } + } + return DataModel::CommandEntry::kInvalid; } std::optional CodegenDataModelProvider::GetAcceptedCommandInfo(const ConcreteCommandPath & path) { + const EmberAfCluster * cluster = FindServerCluster(path); + + VerifyOrReturnValue(cluster != nullptr, std::nullopt); + VerifyOrReturnValue(cluster->generatedCommandList != nullptr, std::nullopt); + + // 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(path.mEndpointId, path.mClusterId); + for (unsigned i = 0; cluster->generatedCommandList[i] != kInvalidCommandId; i++) + { + if (cluster->generatedCommandList[i] != path.mCommandId) + { + continue; + } + + const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, cluster->generatedCommandList[i]); + VerifyOrReturnValue((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath), std::nullopt); - EnumeratorCommandFinder handlerFinder(&CommandHandlerInterface::EnumerateAcceptedCommands); - CommandId commandId = FindCommand(path, handlerFinder, detail::EnumeratorCommandFinder::Operation::kFindExact, - mAcceptedCommandsIterator, AcceptedCommands); + mAcceptedCommandHint = i; + return CommandEntryFrom(path, commandPath.mCommandId).info; + } - VerifyOrReturnValue(commandId != kInvalidCommandId, std::nullopt); - return CommandEntryFrom(path, commandId).info; + return std::nullopt; } 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(commandId != kInvalidCommandId, kInvalidCommandPath); - return ConcreteCommandPath(path.mEndpointId, path.mClusterId, commandId); + VerifyOrReturnValue(cluster != nullptr, ConcreteCommandPath()); + VerifyOrReturnValue(cluster->acceptedCommandList != nullptr, ConcreteCommandPath()); + + auto * commandHandler = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId); + for (unsigned i = 0; cluster->generatedCommandList[i] != kInvalidCommandId; i++) + { + const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, cluster->generatedCommandList[i]); + if ((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath)) + { + mGeneratedCommandHint = i; + return commandPath; + } + } + return {}; } ConcreteCommandPath CodegenDataModelProvider::NextGeneratedCommand(const ConcreteCommandPath & before) { - EnumeratorCommandFinder handlerFinder(&CommandHandlerInterface::EnumerateGeneratedCommands); + const EmberAfCluster * cluster = FindServerCluster(before); + + VerifyOrReturnValue(cluster != nullptr, ConcreteCommandPath()); + VerifyOrReturnValue(cluster->acceptedCommandList != nullptr, ConcreteCommandPath()); - CommandId commandId = FindCommand(before, handlerFinder, detail::EnumeratorCommandFinder::Operation::kFindNext, - mGeneratedCommandsIterator, GeneratedCommands); + // 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); + unsigned beforeIdx; + for (beforeIdx = 0; cluster->acceptedCommandList[beforeIdx] != kInvalidCommandId; beforeIdx++) + { + if (cluster->acceptedCommandList[beforeIdx] == before.mCommandId) + { + break; // found it + } + } - VerifyOrReturnValue(commandId != kInvalidCommandId, kInvalidCommandPath); - return ConcreteCommandPath(before.mEndpointId, before.mClusterId, commandId); + VerifyOrReturnValue(cluster->acceptedCommandList[beforeIdx] == before.mCommandId, ConcreteCommandPath()); + + // find the first "accepted" index out if thios + for (unsigned i = beforeIdx + 1; cluster->acceptedCommandList[i] != kInvalidCommandId; i++) + { + const ConcreteCommandPath commandPath(before.mEndpointId, before.mClusterId, cluster->acceptedCommandList[i]); + if ((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath)) + { + mAcceptedCommandHint = i; + return commandPath; + } + } + 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..2a818744757615 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. /// @@ -196,6 +141,8 @@ class CodegenDataModelProvider : public DataModel::Provider unsigned mAttributeIterationHint = 0; unsigned mDeviceTypeIterationHint = 0; unsigned mSemanticTagIterationHint = 0; + unsigned mAcceptedCommandHint = 0; + unsigned mGeneratedCommandHint = 0; EmberCommandListIterator mAcceptedCommandsIterator; EmberCommandListIterator mGeneratedCommandsIterator; @@ -236,10 +183,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..84e9b805dcd0ae 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; } From 74409ee7ee4572afdd4d79aec0e48b895c105d0c Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 11 Dec 2024 15:50:58 -0500 Subject: [PATCH 03/15] Fix up generated/accepted paths --- .../codegen/CodegenDataModelProvider.cpp | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index 30728d8b8c47a0..e38922c08a796b 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -727,12 +727,12 @@ DataModel::CommandEntry CodegenDataModelProvider::FirstAcceptedCommand(const Con const EmberAfCluster * cluster = FindServerCluster(path); VerifyOrReturnValue(cluster != nullptr, DataModel::CommandEntry::kInvalid); - VerifyOrReturnValue(cluster->generatedCommandList != nullptr, DataModel::CommandEntry::kInvalid); + VerifyOrReturnValue(cluster->acceptedCommandList != nullptr, DataModel::CommandEntry::kInvalid); auto * commandHandler = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId); - for (unsigned i = 0; cluster->generatedCommandList[i] != kInvalidCommandId; i++) + for (unsigned i = 0; cluster->acceptedCommandList[i] != kInvalidCommandId; i++) { - const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, cluster->generatedCommandList[i]); + const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, cluster->acceptedCommandList[i]); if ((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath)) { mAcceptedCommandHint = i; @@ -747,26 +747,26 @@ DataModel::CommandEntry CodegenDataModelProvider::NextAcceptedCommand(const Conc const EmberAfCluster * cluster = FindServerCluster(before); VerifyOrReturnValue(cluster != nullptr, DataModel::CommandEntry::kInvalid); - VerifyOrReturnValue(cluster->generatedCommandList != nullptr, DataModel::CommandEntry::kInvalid); + VerifyOrReturnValue(cluster->acceptedCommandList != nullptr, DataModel::CommandEntry::kInvalid); // 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); unsigned beforeIdx; - for (beforeIdx = 0; cluster->generatedCommandList[beforeIdx] != kInvalidCommandId; beforeIdx++) + for (beforeIdx = 0; cluster->acceptedCommandList[beforeIdx] != kInvalidCommandId; beforeIdx++) { - if (cluster->generatedCommandList[beforeIdx] == before.mCommandId) + if (cluster->acceptedCommandList[beforeIdx] == before.mCommandId) { break; // found it } } - VerifyOrReturnValue(cluster->generatedCommandList[beforeIdx] == before.mCommandId, DataModel::CommandEntry::kInvalid); + VerifyOrReturnValue(cluster->acceptedCommandList[beforeIdx] == before.mCommandId, DataModel::CommandEntry::kInvalid); // find the first "accepted" index out if thios - for (unsigned i = beforeIdx + 1; cluster->generatedCommandList[i] != kInvalidCommandId; i++) + for (unsigned i = beforeIdx + 1; cluster->acceptedCommandList[i] != kInvalidCommandId; i++) { - const ConcreteCommandPath commandPath(before.mEndpointId, before.mClusterId, cluster->generatedCommandList[i]); + const ConcreteCommandPath commandPath(before.mEndpointId, before.mClusterId, cluster->acceptedCommandList[i]); if ((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath)) { mAcceptedCommandHint = i; @@ -781,19 +781,19 @@ std::optional CodegenDataModelProvider::GetAcceptedComma const EmberAfCluster * cluster = FindServerCluster(path); VerifyOrReturnValue(cluster != nullptr, std::nullopt); - VerifyOrReturnValue(cluster->generatedCommandList != nullptr, std::nullopt); + VerifyOrReturnValue(cluster->acceptedCommandList != nullptr, std::nullopt); // 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(path.mEndpointId, path.mClusterId); - for (unsigned i = 0; cluster->generatedCommandList[i] != kInvalidCommandId; i++) + for (unsigned i = 0; cluster->acceptedCommandList[i] != kInvalidCommandId; i++) { - if (cluster->generatedCommandList[i] != path.mCommandId) + if (cluster->acceptedCommandList[i] != path.mCommandId) { continue; } - const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, cluster->generatedCommandList[i]); + const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, cluster->acceptedCommandList[i]); VerifyOrReturnValue((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath), std::nullopt); mAcceptedCommandHint = i; @@ -808,7 +808,7 @@ ConcreteCommandPath CodegenDataModelProvider::FirstGeneratedCommand(const Concre const EmberAfCluster * cluster = FindServerCluster(path); VerifyOrReturnValue(cluster != nullptr, ConcreteCommandPath()); - VerifyOrReturnValue(cluster->acceptedCommandList != nullptr, ConcreteCommandPath()); + VerifyOrReturnValue(cluster->generatedCommandList != nullptr, ConcreteCommandPath()); auto * commandHandler = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId); for (unsigned i = 0; cluster->generatedCommandList[i] != kInvalidCommandId; i++) @@ -828,26 +828,26 @@ ConcreteCommandPath CodegenDataModelProvider::NextGeneratedCommand(const Concret const EmberAfCluster * cluster = FindServerCluster(before); VerifyOrReturnValue(cluster != nullptr, ConcreteCommandPath()); - VerifyOrReturnValue(cluster->acceptedCommandList != 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); unsigned beforeIdx; - for (beforeIdx = 0; cluster->acceptedCommandList[beforeIdx] != kInvalidCommandId; beforeIdx++) + for (beforeIdx = 0; cluster->generatedCommandList[beforeIdx] != kInvalidCommandId; beforeIdx++) { - if (cluster->acceptedCommandList[beforeIdx] == before.mCommandId) + if (cluster->generatedCommandList[beforeIdx] == before.mCommandId) { break; // found it } } - VerifyOrReturnValue(cluster->acceptedCommandList[beforeIdx] == before.mCommandId, ConcreteCommandPath()); + VerifyOrReturnValue(cluster->generatedCommandList[beforeIdx] == before.mCommandId, ConcreteCommandPath()); // find the first "accepted" index out if thios - for (unsigned i = beforeIdx + 1; cluster->acceptedCommandList[i] != kInvalidCommandId; i++) + for (unsigned i = beforeIdx + 1; cluster->generatedCommandList[i] != kInvalidCommandId; i++) { - const ConcreteCommandPath commandPath(before.mEndpointId, before.mClusterId, cluster->acceptedCommandList[i]); + const ConcreteCommandPath commandPath(before.mEndpointId, before.mClusterId, cluster->generatedCommandList[i]); if ((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath)) { mAcceptedCommandHint = i; From db52df0d0fb10315104a58cdf250871ebc9aafec Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 11 Dec 2024 15:58:43 -0500 Subject: [PATCH 04/15] More fixes and match metadata with handler implementation --- .../codegen/CodegenDataModelProvider.cpp | 4 ++-- .../codegen/tests/TestCodegenModelViaMocks.cpp | 14 ++++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index e38922c08a796b..2b8c5aacd7c46c 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -814,7 +814,7 @@ ConcreteCommandPath CodegenDataModelProvider::FirstGeneratedCommand(const Concre for (unsigned i = 0; cluster->generatedCommandList[i] != kInvalidCommandId; i++) { const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, cluster->generatedCommandList[i]); - if ((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath)) + if ((commandHandler == nullptr) || commandHandler->GeneratesCommandId(commandPath)) { mGeneratedCommandHint = i; return commandPath; @@ -848,7 +848,7 @@ ConcreteCommandPath CodegenDataModelProvider::NextGeneratedCommand(const Concret for (unsigned i = beforeIdx + 1; cluster->generatedCommandList[i] != kInvalidCommandId; i++) { const ConcreteCommandPath commandPath(before.mEndpointId, before.mClusterId, cluster->generatedCommandList[i]); - if ((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath)) + if ((commandHandler == nullptr) || commandHandler->GeneratesCommandId(commandPath)) { mAcceptedCommandHint = i; return commandPath; diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp index 84e9b805dcd0ae..ee7fc427b0d158 100644 --- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp +++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp @@ -265,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), }), From 8718411fdef1a66f1393086888cc05bceb9fff69 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 11 Dec 2024 16:05:25 -0500 Subject: [PATCH 05/15] Unit tests pass --- .../tests/TestServerCommandDispatch.cpp | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/controller/tests/TestServerCommandDispatch.cpp b/src/controller/tests/TestServerCommandDispatch.cpp index 2389ce08e6baf2..406860d90a64ac 100644 --- a/src/controller/tests/TestServerCommandDispatch.cpp +++ b/src/controller/tests/TestServerCommandDispatch.cpp @@ -17,6 +17,7 @@ */ #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,19 @@ TEST_F(TestServerCommandDispatch, TestDataResponseHandlerOverride1) { TestClusterCommandHandler commandHandler; commandHandler.OverrideAcceptedCommands(); - TestDataResponseHelper(&testEndpoint2, true); + + // Clusters::UnitTesting::Commands::TestSimpleArgumentRequest::Id exists on Endpoint1 + TestDataResponseHelper(&testEndpoint1, true); } TEST_F(TestServerCommandDispatch, TestDataResponseHandlerOverride2) { TestClusterCommandHandler commandHandler; commandHandler.OverrideAcceptedCommands(); - TestDataResponseHelper(&testEndpoint3, true); + + // Clusters::UnitTesting::Commands::TestSimpleArgumentRequest::Id DOES NOT exist on endpoint1 + // so overriding accepting does nothing (there is no metadata to accept it) + TestDataResponseHelper(&testEndpoint3, false); } } // namespace From e51f0e9ebbe02062a76cfe17e5f6988222a76836 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 11 Dec 2024 16:05:38 -0500 Subject: [PATCH 06/15] Restyle --- src/controller/tests/TestServerCommandDispatch.cpp | 2 +- .../codegen/tests/TestCodegenModelViaMocks.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/controller/tests/TestServerCommandDispatch.cpp b/src/controller/tests/TestServerCommandDispatch.cpp index 406860d90a64ac..97c33456f38bd2 100644 --- a/src/controller/tests/TestServerCommandDispatch.cpp +++ b/src/controller/tests/TestServerCommandDispatch.cpp @@ -17,12 +17,12 @@ */ #include -#include #include #include #include #include #include +#include #include #include #include diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp index ee7fc427b0d158..44c3433f156e10 100644 --- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp +++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp @@ -266,7 +266,7 @@ class ScopedMockAccessControl const MockNodeConfig gTestNodeConfig({ MockEndpointConfig(kMockEndpoint1, { MockClusterConfig( - MockClusterId(1), + MockClusterId(1), { ClusterRevision::Id, FeatureMap::Id, }, /* attributes */ From 5fecd4379749174def3479d8091a592917aad248 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 11 Dec 2024 16:11:13 -0500 Subject: [PATCH 07/15] Fixed up comments --- src/controller/tests/TestServerCommandDispatch.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/controller/tests/TestServerCommandDispatch.cpp b/src/controller/tests/TestServerCommandDispatch.cpp index 97c33456f38bd2..6d742ba14b64f7 100644 --- a/src/controller/tests/TestServerCommandDispatch.cpp +++ b/src/controller/tests/TestServerCommandDispatch.cpp @@ -374,7 +374,8 @@ TEST_F(TestServerCommandDispatch, TestDataResponseHandlerOverride1) TestClusterCommandHandler commandHandler; commandHandler.OverrideAcceptedCommands(); - // Clusters::UnitTesting::Commands::TestSimpleArgumentRequest::Id exists on Endpoint1 + // Clusters::UnitTesting exists on testEndpoint1, so metadata about generated commands + // exists TestDataResponseHelper(&testEndpoint1, true); } @@ -383,7 +384,7 @@ TEST_F(TestServerCommandDispatch, TestDataResponseHandlerOverride2) TestClusterCommandHandler commandHandler; commandHandler.OverrideAcceptedCommands(); - // Clusters::UnitTesting::Commands::TestSimpleArgumentRequest::Id DOES NOT exist on endpoint1 + // Clusters::UnitTesting DOES NOT exist on testEndpoint3 // so overriding accepting does nothing (there is no metadata to accept it) TestDataResponseHelper(&testEndpoint3, false); } From 9db3dcbf11bb076724d30376edd346826078c73f Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 11 Dec 2024 16:24:00 -0500 Subject: [PATCH 08/15] Update upgrading document --- docs/upgrading.md | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/docs/upgrading.md b/docs/upgrading.md index e9e17239d50049..b49124bffb1838 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -68,14 +68,14 @@ independent of the InteractionModelEngine class. The following replacements exist: -- `chip::app::InteractionModelEngine::RegisterCommandHandler` replaced by - `chip::app::CommandHandlerInterfaceRegistry::Instance().RegisterCommandHandler` -- `chip::app::InteractionModelEngine::UnregisterCommandHandler` replaced by - `chip::app::CommandHandlerInterfaceRegistry::Instance().UnregisterCommandHandler` -- `chip::app::InteractionModelEngine::FindCommandHandler` replaced by - `chip::app::CommandHandlerInterfaceRegistry::Instance().GetCommandHandler` -- `chip::app::InteractionModelEngine::UnregisterCommandHandlers` replaced by - `chip::app::CommandHandlerInterfaceRegistry::Instance().UnregisterAllCommandHandlersForEndpoint` +- `chip::app::InteractionModelEngine::RegisterCommandHandler` replaced by + `chip::app::CommandHandlerInterfaceRegistry::Instance().RegisterCommandHandler` +- `chip::app::InteractionModelEngine::UnregisterCommandHandler` replaced by + `chip::app::CommandHandlerInterfaceRegistry::Instance().UnregisterCommandHandler` +- `chip::app::InteractionModelEngine::FindCommandHandler` replaced by + `chip::app::CommandHandlerInterfaceRegistry::Instance().GetCommandHandler` +- `chip::app::InteractionModelEngine::UnregisterCommandHandlers` replaced by + `chip::app::CommandHandlerInterfaceRegistry::Instance().UnregisterAllCommandHandlersForEndpoint` ### AttributeAccessInterface registration and removal @@ -84,14 +84,14 @@ A new object exists for the attribute access interface registry, accessible as Replacements for methods are: -- `registerAttributeAccessOverride` replaced by - `chip::app::AttributeAccessInterfaceRegistry::Instance().Register` -- `unregisterAttributeAccessOverride` replaced by - `chip::app::AttributeAccessInterfaceRegistry::Instance().Unregister` -- `unregisterAllAttributeAccessOverridesForEndpoint` replaced by - `chip::app::AttributeAccessInterfaceRegistry::Instance().UnregisterAllForEndpoint` -- `chip::app::GetAttributeAccessOverride` replaced by - `chip::app::AttributeAccessInterfaceRegistry::Instance().Get` +- `registerAttributeAccessOverride` replaced by + `chip::app::AttributeAccessInterfaceRegistry::Instance().Register` +- `unregisterAttributeAccessOverride` replaced by + `chip::app::AttributeAccessInterfaceRegistry::Instance().Unregister` +- `unregisterAllAttributeAccessOverridesForEndpoint` replaced by + `chip::app::AttributeAccessInterfaceRegistry::Instance().UnregisterAllForEndpoint` +- `chip::app::GetAttributeAccessOverride` replaced by + `chip::app::AttributeAccessInterfaceRegistry::Instance().Get` ### `ServerInitParams::dataModelProvider` in `Server::Init` and `FactoryInitParams` @@ -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. From bf63459c8b08421583e62c118430cc200d818172 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 11 Dec 2024 16:25:58 -0500 Subject: [PATCH 09/15] Fix network commissioning cluster --- .../network-commissioning.cpp | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp index 87d446e886af02..27d6225a2c6675 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.cpp +++ b/src/app/clusters/network-commissioning/network-commissioning.cpp @@ -1386,22 +1386,18 @@ bool Instance::AcceptsCommandId(const ConcreteCommandPath & commandPath) bool Instance::GeneratesCommandId(const ConcreteCommandPath & commandPath) { using namespace Clusters::NetworkCommissioning::Commands; - - if (mFeatureFlags.HasAny(Feature::kWiFiNetworkInterface, Feature::kThreadNetworkInterface)) + switch (commandPath.mCommandId) { - for (auto && cmd : { ScanNetworksResponse::Id, NetworkConfigResponse::Id, ConnectNetworkResponse::Id }) - { - VerifyOrExit(callback(cmd, context) == Loop::Continue, /**/); - } - } + case ScanNetworksResponse::Id: + case NetworkConfigResponse::Id: + case ConnectNetworkResponse::Id: + return mFeatureFlags.HasAny(Feature::kWiFiNetworkInterface, Feature::kThreadNetworkInterface); + case QueryIdentityResponse::Id: - if (mFeatureFlags.Has(Feature::kPerDeviceCredentials)) - { - VerifyOrExit(callback(QueryIdentityResponse::Id, context) == Loop::Continue, /**/); + return mFeatureFlags.Has(Feature::kPerDeviceCredentials); + default: + return false; } - -exit: - return CHIP_NO_ERROR; } bool NullNetworkDriver::GetEnabled() From 335bb89370f4bbe64b9928461eb801275e1ac0d0 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 11 Dec 2024 16:26:27 -0500 Subject: [PATCH 10/15] remove empty line --- src/app/clusters/network-commissioning/network-commissioning.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp index 27d6225a2c6675..7778e32a9755ca 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.cpp +++ b/src/app/clusters/network-commissioning/network-commissioning.cpp @@ -1393,7 +1393,6 @@ bool Instance::GeneratesCommandId(const ConcreteCommandPath & commandPath) case ConnectNetworkResponse::Id: return mFeatureFlags.HasAny(Feature::kWiFiNetworkInterface, Feature::kThreadNetworkInterface); case QueryIdentityResponse::Id: - return mFeatureFlags.Has(Feature::kPerDeviceCredentials); default: return false; From 7641bd31c383be07257dcb54d013e095aa640763 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 11 Dec 2024 16:27:49 -0500 Subject: [PATCH 11/15] One more compile fix. All clusters compiles now --- .../software-diagnostics-server.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 3a9e2f2cd51d3e..c6e99f69133bd3 100644 --- a/src/app/clusters/software-diagnostics-server/software-diagnostics-server.cpp +++ b/src/app/clusters/software-diagnostics-server/software-diagnostics-server.cpp @@ -177,7 +177,9 @@ bool SoftwareDiagnosticsCommandHandler::AcceptsCommandId(const ConcreteCommandPa default: return false; } -} // anonymous namespace +} + +} // namespace namespace chip { namespace app { From 8239d0e63aa1e0dbec0a52b1f1370fc6da817796 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Wed, 11 Dec 2024 21:28:32 +0000 Subject: [PATCH 12/15] Restyled by prettier-markdown --- docs/upgrading.md | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/docs/upgrading.md b/docs/upgrading.md index b49124bffb1838..8e2fbd424d539a 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -68,14 +68,14 @@ independent of the InteractionModelEngine class. The following replacements exist: -- `chip::app::InteractionModelEngine::RegisterCommandHandler` replaced by - `chip::app::CommandHandlerInterfaceRegistry::Instance().RegisterCommandHandler` -- `chip::app::InteractionModelEngine::UnregisterCommandHandler` replaced by - `chip::app::CommandHandlerInterfaceRegistry::Instance().UnregisterCommandHandler` -- `chip::app::InteractionModelEngine::FindCommandHandler` replaced by - `chip::app::CommandHandlerInterfaceRegistry::Instance().GetCommandHandler` -- `chip::app::InteractionModelEngine::UnregisterCommandHandlers` replaced by - `chip::app::CommandHandlerInterfaceRegistry::Instance().UnregisterAllCommandHandlersForEndpoint` +- `chip::app::InteractionModelEngine::RegisterCommandHandler` replaced by + `chip::app::CommandHandlerInterfaceRegistry::Instance().RegisterCommandHandler` +- `chip::app::InteractionModelEngine::UnregisterCommandHandler` replaced by + `chip::app::CommandHandlerInterfaceRegistry::Instance().UnregisterCommandHandler` +- `chip::app::InteractionModelEngine::FindCommandHandler` replaced by + `chip::app::CommandHandlerInterfaceRegistry::Instance().GetCommandHandler` +- `chip::app::InteractionModelEngine::UnregisterCommandHandlers` replaced by + `chip::app::CommandHandlerInterfaceRegistry::Instance().UnregisterAllCommandHandlersForEndpoint` ### AttributeAccessInterface registration and removal @@ -84,14 +84,14 @@ A new object exists for the attribute access interface registry, accessible as Replacements for methods are: -- `registerAttributeAccessOverride` replaced by - `chip::app::AttributeAccessInterfaceRegistry::Instance().Register` -- `unregisterAttributeAccessOverride` replaced by - `chip::app::AttributeAccessInterfaceRegistry::Instance().Unregister` -- `unregisterAllAttributeAccessOverridesForEndpoint` replaced by - `chip::app::AttributeAccessInterfaceRegistry::Instance().UnregisterAllForEndpoint` -- `chip::app::GetAttributeAccessOverride` replaced by - `chip::app::AttributeAccessInterfaceRegistry::Instance().Get` +- `registerAttributeAccessOverride` replaced by + `chip::app::AttributeAccessInterfaceRegistry::Instance().Register` +- `unregisterAttributeAccessOverride` replaced by + `chip::app::AttributeAccessInterfaceRegistry::Instance().Unregister` +- `unregisterAllAttributeAccessOverridesForEndpoint` replaced by + `chip::app::AttributeAccessInterfaceRegistry::Instance().UnregisterAllForEndpoint` +- `chip::app::GetAttributeAccessOverride` replaced by + `chip::app::AttributeAccessInterfaceRegistry::Instance().Get` ### `ServerInitParams::dataModelProvider` in `Server::Init` and `FactoryInitParams` From 41c329f78295b51feae198eb3963a125fb681e9f Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 11 Dec 2024 23:32:16 -0500 Subject: [PATCH 13/15] Remove O(N^2) cost using existing iterators --- .../codegen/CodegenDataModelProvider.cpp | 73 ++++++------------- .../codegen/CodegenDataModelProvider.h | 2 - 2 files changed, 23 insertions(+), 52 deletions(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index 2b8c5aacd7c46c..e15deaee7f15a4 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -730,14 +730,16 @@ DataModel::CommandEntry CodegenDataModelProvider::FirstAcceptedCommand(const Con VerifyOrReturnValue(cluster->acceptedCommandList != nullptr, DataModel::CommandEntry::kInvalid); auto * commandHandler = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId); - for (unsigned i = 0; cluster->acceptedCommandList[i] != kInvalidCommandId; i++) + + std::optional mCommand = mAcceptedCommandsIterator.First(cluster->acceptedCommandList); + while (mCommand.has_value()) { - const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, cluster->acceptedCommandList[i]); + const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, *mCommand); if ((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath)) { - mAcceptedCommandHint = i; return CommandEntryFrom(path, commandPath.mCommandId); } + mCommand = mAcceptedCommandsIterator.Next(cluster->acceptedCommandList, *mCommand); } return DataModel::CommandEntry::kInvalid; } @@ -749,29 +751,17 @@ DataModel::CommandEntry CodegenDataModelProvider::NextAcceptedCommand(const Conc VerifyOrReturnValue(cluster != nullptr, DataModel::CommandEntry::kInvalid); VerifyOrReturnValue(cluster->acceptedCommandList != nullptr, DataModel::CommandEntry::kInvalid); - // 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); - unsigned beforeIdx; - for (beforeIdx = 0; cluster->acceptedCommandList[beforeIdx] != kInvalidCommandId; beforeIdx++) - { - if (cluster->acceptedCommandList[beforeIdx] == before.mCommandId) - { - break; // found it - } - } - - VerifyOrReturnValue(cluster->acceptedCommandList[beforeIdx] == before.mCommandId, DataModel::CommandEntry::kInvalid); - // find the first "accepted" index out if thios - for (unsigned i = beforeIdx + 1; cluster->acceptedCommandList[i] != kInvalidCommandId; i++) + std::optional mCommand = mAcceptedCommandsIterator.Next(cluster->acceptedCommandList, before.mCommandId); + while (mCommand.has_value()) { - const ConcreteCommandPath commandPath(before.mEndpointId, before.mClusterId, cluster->acceptedCommandList[i]); + const ConcreteCommandPath commandPath(before.mEndpointId, before.mClusterId, *mCommand); if ((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath)) { - mAcceptedCommandHint = i; return CommandEntryFrom(before, commandPath.mCommandId); } + mCommand = mAcceptedCommandsIterator.Next(cluster->acceptedCommandList, *mCommand); } return DataModel::CommandEntry::kInvalid; } @@ -783,24 +773,14 @@ std::optional CodegenDataModelProvider::GetAcceptedComma VerifyOrReturnValue(cluster != nullptr, std::nullopt); VerifyOrReturnValue(cluster->acceptedCommandList != nullptr, std::nullopt); - // 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(path.mEndpointId, path.mClusterId); - for (unsigned i = 0; cluster->acceptedCommandList[i] != kInvalidCommandId; i++) - { - if (cluster->acceptedCommandList[i] != path.mCommandId) - { - continue; - } - const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, cluster->acceptedCommandList[i]); - VerifyOrReturnValue((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath), std::nullopt); + VerifyOrReturnValue(mAcceptedCommandsIterator.Exists(cluster->acceptedCommandList, path.mCommandId), std::nullopt); - mAcceptedCommandHint = i; - return CommandEntryFrom(path, commandPath.mCommandId).info; - } + const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, path.mCommandId); + VerifyOrReturnValue((commandHandler == nullptr) || commandHandler->AcceptsCommandId(commandPath), std::nullopt); - return std::nullopt; + return CommandEntryFrom(path, commandPath.mCommandId).info; } ConcreteCommandPath CodegenDataModelProvider::FirstGeneratedCommand(const ConcreteClusterPath & path) @@ -811,14 +791,16 @@ ConcreteCommandPath CodegenDataModelProvider::FirstGeneratedCommand(const Concre VerifyOrReturnValue(cluster->generatedCommandList != nullptr, ConcreteCommandPath()); auto * commandHandler = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId); - for (unsigned i = 0; cluster->generatedCommandList[i] != kInvalidCommandId; i++) + + std::optional mCommand = mGeneratedCommandsIterator.First(cluster->generatedCommandList); + while (mCommand.has_value()) { - const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, cluster->generatedCommandList[i]); + const ConcreteCommandPath commandPath(path.mEndpointId, path.mClusterId, *mCommand); if ((commandHandler == nullptr) || commandHandler->GeneratesCommandId(commandPath)) { - mGeneratedCommandHint = i; return commandPath; } + mCommand = mAcceptedCommandsIterator.Next(cluster->acceptedCommandList, *mCommand); } return {}; } @@ -833,27 +815,18 @@ ConcreteCommandPath CodegenDataModelProvider::NextGeneratedCommand(const Concret // 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); - unsigned beforeIdx; - for (beforeIdx = 0; cluster->generatedCommandList[beforeIdx] != kInvalidCommandId; beforeIdx++) - { - if (cluster->generatedCommandList[beforeIdx] == before.mCommandId) - { - break; // found it - } - } - VerifyOrReturnValue(cluster->generatedCommandList[beforeIdx] == before.mCommandId, ConcreteCommandPath()); - - // find the first "accepted" index out if thios - for (unsigned i = beforeIdx + 1; cluster->generatedCommandList[i] != kInvalidCommandId; i++) + std::optional mCommand = mGeneratedCommandsIterator.Next(cluster->generatedCommandList, before.mCommandId); + while (mCommand.has_value()) { - const ConcreteCommandPath commandPath(before.mEndpointId, before.mClusterId, cluster->generatedCommandList[i]); + const ConcreteCommandPath commandPath(before.mEndpointId, before.mClusterId, *mCommand); if ((commandHandler == nullptr) || commandHandler->GeneratesCommandId(commandPath)) { - mAcceptedCommandHint = i; return commandPath; } + mCommand = mGeneratedCommandsIterator.Next(cluster->generatedCommandList, *mCommand); } + return {}; } diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.h b/src/data-model-providers/codegen/CodegenDataModelProvider.h index 2a818744757615..e87be1fc5f28dd 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.h +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.h @@ -141,8 +141,6 @@ class CodegenDataModelProvider : public DataModel::Provider unsigned mAttributeIterationHint = 0; unsigned mDeviceTypeIterationHint = 0; unsigned mSemanticTagIterationHint = 0; - unsigned mAcceptedCommandHint = 0; - unsigned mGeneratedCommandHint = 0; EmberCommandListIterator mAcceptedCommandsIterator; EmberCommandListIterator mGeneratedCommandsIterator; From c71d10a5b27a75102585e37bb7ab4ee5e4cbb466 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 11 Dec 2024 23:34:30 -0500 Subject: [PATCH 14/15] Fix typo --- src/data-model-providers/codegen/CodegenDataModelProvider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index e15deaee7f15a4..4e6c1f0a25ee02 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -800,7 +800,7 @@ ConcreteCommandPath CodegenDataModelProvider::FirstGeneratedCommand(const Concre { return commandPath; } - mCommand = mAcceptedCommandsIterator.Next(cluster->acceptedCommandList, *mCommand); + mCommand = mAcceptedCommandsIterator.Next(cluster->generatedCommandList(), *mCommand); } return {}; } From 895f1389b3fede9b3ba3112c377da08e7a170cf5 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 11 Dec 2024 23:35:18 -0500 Subject: [PATCH 15/15] One more typo fix --- src/data-model-providers/codegen/CodegenDataModelProvider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index 4e6c1f0a25ee02..293a3c4fb1c43a 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -800,7 +800,7 @@ ConcreteCommandPath CodegenDataModelProvider::FirstGeneratedCommand(const Concre { return commandPath; } - mCommand = mAcceptedCommandsIterator.Next(cluster->generatedCommandList(), *mCommand); + mCommand = mGeneratedCommandsIterator.Next(cluster->generatedCommandList, *mCommand); } return {}; }