Update CommandHandlerInterface to have a bool accepts/generates instead of an iterator-based callback

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.

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.

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)

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);

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)

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);

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()

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;

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)

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;