Add support for using CommandHandlerInterface for accepted/generated command listing (project-chip#35753)

* Add support for using CommandHandlerInterface for accepted/generated commands.

Note that iteration is still O(N^2) which is not ideal, however at least
the use of this is infrequent and list of commands is somewhat short.

* Fix includes

* Comment update

* Comment update

* Fix includes

* comments

* Restyled by clang-format

* Update src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp

Co-authored-by: Boris Zbarsky <[email protected]>

* Update src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp

Co-authored-by: Terence Hampson <[email protected]>

* Make use of FindExact operation

* Use CommandEntryFrom

* Fix naming

* Added TODO on slow next iteration

* Update code flow to hopefully make it somewhat easier to read

* Remove extra include

* Make code even more readable I hope

* Comment update

* Fix includes

* Restyled by clang-format

* Update src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp

Co-authored-by: Boris Zbarsky <[email protected]>

* Restyled by clang-format

---------

Co-authored-by: Andrei Litvin <[email protected]>
Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
Co-authored-by: Terence Hampson <[email protected]>

Author: 5 people

src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp

@@ -17,11 +17,16 @@
 #include <app/codegen-data-model-provider/CodegenDataModelProvider.h>
 
 #include <app-common/zap-generated/attribute-type.h>
+#include <app/CommandHandlerInterface.h>
 #include <app/CommandHandlerInterfaceRegistry.h>
+#include <app/ConcreteClusterPath.h>
+#include <app/ConcreteCommandPath.h>
 #include <app/RequiredPrivilege.h>
+#include <app/data-model-provider/MetadataTypes.h>
 #include <app/util/IMClusterCommandHandler.h>
 #include <app/util/attribute-storage.h>
 #include <app/util/endpoint-config-api.h>
+#include <lib/core/CHIPError.h>
 #include <lib/core/DataModelTypes.h>
 
 #include <optional>
@@ -31,6 +36,113 @@ namespace chip {
 namespace app {
 namespace {
 
+/// 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<CommandId> FindCommandId(Operation operation, const ConcreteCommandPath & path);
+
+    /// Uses FindCommandId to find the given command and loads the command entry data
+    std::optional<DataModel::CommandEntry> FindCommandEntry(Operation operation, const ConcreteCommandPath & path);
+
+private:
+    HandlerCallbackFunction mCallback;
+    Operation mOperation;
+    CommandId mTarget;
+    std::optional<CommandId> mFound = std::nullopt;
+
+    Loop 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
+    }
+
+    static Loop HandlerCallbackFn(CommandId id, void * context)
+    {
+        auto self = static_cast<EnumeratorCommandFinder *>(context);
+        return self->HandlerCallback(id);
+    }
+};
+
+std::optional<CommandId> 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)
+    {
+        // 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());
+        return kInvalidCommandId;
+    }
+
+    return mFound.value_or(kInvalidCommandId);
+}
+
 /// Load the cluster information into the specified destination
 std::variant<CHIP_ERROR, DataModel::ClusterInfo> LoadClusterInfo(const ConcreteClusterPath & path, const EmberAfCluster & cluster)
 {
@@ -160,6 +272,20 @@ DataModel::CommandEntry CommandEntryFrom(const ConcreteClusterPath & clusterPath
     return entry;
 }
 
+std::optional<DataModel::CommandEntry> EnumeratorCommandFinder::FindCommandEntry(Operation operation,
+                                                                                 const ConcreteCommandPath & path)
+{
+
+    std::optional<CommandId> id = FindCommandId(operation, path);
+
+    if (!id.has_value())
+    {
+        return std::nullopt;
+    }
+
+    return (*id == kInvalidCommandId) ? DataModel::CommandEntry::kInvalid : CommandEntryFrom(path, *id);
+}
+
 const ConcreteCommandPath kInvalidCommandPath(kInvalidEndpointId, kInvalidClusterId, kInvalidCommandId);
 
 } // namespace
@@ -492,6 +618,15 @@ std::optional<DataModel::AttributeInfo> CodegenDataModelProvider::GetAttributeIn
 
 DataModel::CommandEntry CodegenDataModelProvider::FirstAcceptedCommand(const ConcreteClusterPath & path)
 {
+    auto handlerInterfaceValue = EnumeratorCommandFinder(&CommandHandlerInterface::EnumerateAcceptedCommands)
+                                     .FindCommandEntry(EnumeratorCommandFinder::Operation::kFindFirst,
+                                                       ConcreteCommandPath(path.mEndpointId, path.mClusterId, kInvalidCommandId));
+
+    if (handlerInterfaceValue.has_value())
+    {
+        return *handlerInterfaceValue;
+    }
+
     const EmberAfCluster * cluster = FindServerCluster(path);
 
     VerifyOrReturnValue(cluster != nullptr, DataModel::CommandEntry::kInvalid);
@@ -504,6 +639,16 @@ DataModel::CommandEntry CodegenDataModelProvider::FirstAcceptedCommand(const Con
 
 DataModel::CommandEntry CodegenDataModelProvider::NextAcceptedCommand(const ConcreteCommandPath & before)
 {
+    // TODO: `Next` redirecting to a callback is slow O(n^2).
+    //       see https://github.com/project-chip/connectedhomeip/issues/35790
+    auto handlerInterfaceValue = EnumeratorCommandFinder(&CommandHandlerInterface::EnumerateAcceptedCommands)
+                                     .FindCommandEntry(EnumeratorCommandFinder::Operation::kFindNext, before);
+
+    if (handlerInterfaceValue.has_value())
+    {
+        return *handlerInterfaceValue;
+    }
+
     const EmberAfCluster * cluster = FindServerCluster(before);
 
     VerifyOrReturnValue(cluster != nullptr, DataModel::CommandEntry::kInvalid);
@@ -516,6 +661,14 @@ DataModel::CommandEntry CodegenDataModelProvider::NextAcceptedCommand(const Conc
 
 std::optional<DataModel::CommandInfo> CodegenDataModelProvider::GetAcceptedCommandInfo(const ConcreteCommandPath & path)
 {
+    auto handlerInterfaceValue = EnumeratorCommandFinder(&CommandHandlerInterface::EnumerateAcceptedCommands)
+                                     .FindCommandEntry(EnumeratorCommandFinder::Operation::kFindExact, path);
+
+    if (handlerInterfaceValue.has_value())
+    {
+        return handlerInterfaceValue->IsValid() ? std::make_optional(handlerInterfaceValue->info) : std::nullopt;
+    }
+
     const EmberAfCluster * cluster = FindServerCluster(path);
 
     VerifyOrReturnValue(cluster != nullptr, std::nullopt);
@@ -526,17 +679,37 @@ std::optional<DataModel::CommandInfo> CodegenDataModelProvider::GetAcceptedComma
 
 ConcreteCommandPath CodegenDataModelProvider::FirstGeneratedCommand(const ConcreteClusterPath & path)
 {
-    const EmberAfCluster * cluster = FindServerCluster(path);
+    std::optional<CommandId> commandId =
+        EnumeratorCommandFinder(&CommandHandlerInterface::EnumerateGeneratedCommands)
+            .FindCommandId(EnumeratorCommandFinder::Operation::kFindFirst,
+                           ConcreteCommandPath(path.mEndpointId, path.mClusterId, kInvalidCommandId));
+    if (commandId.has_value())
+    {
+        return *commandId == kInvalidCommandId ? kInvalidCommandPath
+                                               : ConcreteCommandPath(path.mEndpointId, path.mClusterId, *commandId);
+    }
 
+    const EmberAfCluster * cluster = FindServerCluster(path);
     VerifyOrReturnValue(cluster != nullptr, kInvalidCommandPath);
 
-    std::optional<CommandId> commandId = mGeneratedCommandsIterator.First(cluster->generatedCommandList);
+    commandId = mGeneratedCommandsIterator.First(cluster->generatedCommandList);
     VerifyOrReturnValue(commandId.has_value(), kInvalidCommandPath);
     return ConcreteCommandPath(path.mEndpointId, path.mClusterId, *commandId);
 }
 
 ConcreteCommandPath CodegenDataModelProvider::NextGeneratedCommand(const ConcreteCommandPath & before)
 {
+    // TODO: `Next` redirecting to a callback is slow O(n^2).
+    //       see https://github.com/project-chip/connectedhomeip/issues/35790
+    auto nextId = EnumeratorCommandFinder(&CommandHandlerInterface::EnumerateGeneratedCommands)
+                      .FindCommandId(EnumeratorCommandFinder::Operation::kFindNext, before);
+
+    if (nextId.has_value())
+    {
+        return (*nextId == kInvalidCommandId) ? kInvalidCommandPath
+                                              : ConcreteCommandPath(before.mEndpointId, before.mClusterId, *nextId);
+    }
+
     const EmberAfCluster * cluster = FindServerCluster(before);
 
     VerifyOrReturnValue(cluster != nullptr, kInvalidCommandPath);

src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp

@@ -15,8 +15,6 @@
  *    limitations under the License.
  */
 
-#include <vector>
-
 #include <pw_unit_test/framework.h>
 
 #include <app/codegen-data-model-provider/tests/EmberInvokeOverride.h>
@@ -30,11 +28,14 @@
 #include <app/AttributeAccessInterfaceRegistry.h>
 #include <app/AttributeEncodeState.h>
 #include <app/AttributeValueDecoder.h>
+#include <app/CommandHandlerInterface.h>
+#include <app/CommandHandlerInterfaceRegistry.h>
 #include <app/ConcreteAttributePath.h>
 #include <app/ConcreteCommandPath.h>
 #include <app/GlobalAttributes.h>
 #include <app/MessageDef/ReportDataMessage.h>
 #include <app/codegen-data-model-provider/CodegenDataModelProvider.h>
+#include <app/data-model-provider/MetadataTypes.h>
 #include <app/data-model-provider/OperationTypes.h>
 #include <app/data-model-provider/StringBuilderAdapters.h>
 #include <app/data-model-provider/tests/ReadTesting.h>
@@ -62,6 +63,8 @@
 #include <lib/support/Span.h>
 #include <protocols/interaction_model/StatusCode.h>
 
+#include <vector>
+
 using namespace chip;
 using namespace chip::Test;
 using namespace chip::app;
@@ -183,6 +186,61 @@ class MockAccessControl : public Access::AccessControl::Delegate, public Access:
     bool IsDeviceTypeOnEndpoint(DeviceTypeId deviceType, EndpointId endpoint) override { return true; }
 };
 
+/// Overrides Enumerate*Commands in the CommandHandlerInterface to allow
+/// testing of behaviors when command enumeration is done in the interace.
+class CustomListCommandHandler : public CommandHandlerInterface
+{
+public:
+    CustomListCommandHandler(Optional<EndpointId> endpointId, ClusterId clusterId) : CommandHandlerInterface(endpointId, clusterId)
+    {
+        CommandHandlerInterfaceRegistry::Instance().RegisterCommandHandler(this);
+    }
+    ~CustomListCommandHandler() { CommandHandlerInterfaceRegistry::Instance().UnregisterCommandHandler(this); }
+
+    void InvokeCommand(HandlerContext & handlerContext) override { handlerContext.SetCommandNotHandled(); }
+
+    CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override
+    {
+        VerifyOrReturnError(mOverrideAccepted, CHIP_ERROR_NOT_IMPLEMENTED);
+
+        for (auto id : mAccepted)
+        {
+            if (callback(id, context) != Loop::Continue)
+            {
+                break;
+            }
+        }
+        return CHIP_NO_ERROR;
+    }
+
+    CHIP_ERROR EnumerateGeneratedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override
+    {
+        VerifyOrReturnError(mOverrideGenerated, CHIP_ERROR_NOT_IMPLEMENTED);
+
+        for (auto id : mGenerated)
+        {
+            if (callback(id, context) != Loop::Continue)
+            {
+                break;
+            }
+        }
+        return CHIP_NO_ERROR;
+    }
+
+    void SetOverrideAccepted(bool o) { mOverrideAccepted = o; }
+    void SetOverrideGenerated(bool o) { mOverrideGenerated = o; }
+
+    std::vector<CommandId> & AcceptedVec() { return mAccepted; }
+    std::vector<CommandId> & GeneratedVec() { return mGenerated; }
+
+private:
+    bool mOverrideAccepted  = false;
+    bool mOverrideGenerated = false;
+
+    std::vector<CommandId> mAccepted;
+    std::vector<CommandId> mGenerated;
+};
+
 class ScopedMockAccessControl
 {
 public:
@@ -1193,6 +1251,60 @@ TEST(TestCodegenModelViaMocks, IterateOverGeneratedCommands)
     }
 }
 
+TEST(TestCodegenModelViaMocks, CommandHandlerInterfaceAcceptedCommands)
+{
+
+    UseMockNodeConfig config(gTestNodeConfig);
+    CodegenDataModelProviderWithContext model;
+
+    // Command handler interface is capable to override accepted and generated commands.
+    // Validate that these work
+    CustomListCommandHandler handler(MakeOptional(kMockEndpoint1), MockClusterId(1));
+
+    // At this point, without overrides, there should be no accepted/generated commands
+    EXPECT_FALSE(model.FirstAcceptedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1))).IsValid());
+    EXPECT_FALSE(model.FirstGeneratedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1))).HasValidIds());
+    EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 1234)).has_value());
+
+    handler.SetOverrideAccepted(true);
+    handler.SetOverrideGenerated(true);
+
+    // with overrides, the list is still empty ...
+    EXPECT_FALSE(model.FirstAcceptedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1))).IsValid());
+    EXPECT_FALSE(model.FirstGeneratedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1))).HasValidIds());
+    EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 1234)).has_value());
+
+    // set some overrides
+    handler.AcceptedVec().push_back(1234);
+    handler.AcceptedVec().push_back(999);
+
+    handler.GeneratedVec().push_back(33);
+
+    DataModel::CommandEntry entry;
+
+    entry = model.FirstAcceptedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1)));
+    EXPECT_TRUE(entry.IsValid());
+    EXPECT_EQ(entry.path, ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 1234));
+
+    entry = model.NextAcceptedCommand(entry.path);
+    EXPECT_TRUE(entry.IsValid());
+    EXPECT_EQ(entry.path, ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 999));
+
+    entry = model.NextAcceptedCommand(entry.path);
+    EXPECT_FALSE(entry.IsValid());
+
+    ConcreteCommandPath path = model.FirstGeneratedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1)));
+    EXPECT_TRUE(path.HasValidIds());
+    EXPECT_EQ(path, ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 33));
+    path = model.NextGeneratedCommand(path);
+    EXPECT_FALSE(path.HasValidIds());
+
+    // Command finding should work
+    EXPECT_TRUE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 1234)).has_value());
+    EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 88)).has_value());
+    EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 33)).has_value());
+}
+
 TEST(TestCodegenModelViaMocks, EmberAttributeReadAclDeny)
 {
     UseMockNodeConfig config(gTestNodeConfig);