Add DataModel::Provider Invoke usage (#35540)

* Switch out invoke logic so that codegen data model provider can fully use it * Restyle * Update include paths * Update src/app/InteractionModelEngine.cpp Co-authored-by: Terence Hampson <[email protected]> * Update documentation on invoke * More documentation * More documentation again * Update src/app/data-model-provider/Provider.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/data-model-provider/Provider.h Co-authored-by: Boris Zbarsky <[email protected]> * Comment cleanup * Update src/app/data-model-provider/Provider.h Co-authored-by: Terence Hampson <[email protected]> --------- Co-authored-by: Terence Hampson <[email protected]> Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Sep 17, 2024 · 31894f6 · 31894f6
1 parent 85b2fd3
commit 31894f6
Show file tree

Hide file tree

Showing 9 changed files with 67 additions and 36 deletions.
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -43,6 +43,10 @@
 #include <lib/support/FibonacciUtils.h>
 
 #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
+#include <app/data-model-provider/ActionReturnStatus.h>
+
+// TODO: defaulting to codegen should eventually be an application choice and not
+//       hard-coded in the interaction model
 #include <app/codegen-data-model-provider/Instance.h>
 #endif
 
@@ -1699,6 +1703,21 @@ CHIP_ERROR InteractionModelEngine::PushFront(SingleLinkedListNode<T> *& aObjectL
 void InteractionModelEngine::DispatchCommand(CommandHandlerImpl & apCommandObj, const ConcreteCommandPath & aCommandPath,
                                              TLV::TLVReader & apPayload)
 {
+#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
+
+    DataModel::InvokeRequest request;
+    request.path = aCommandPath;
+
+    std::optional<DataModel::ActionReturnStatus> status = GetDataModelProvider()->Invoke(request, apPayload, &apCommandObj);
+
+    // Provider indicates that handler status or data was already set (or will be set asynchronously) by
+    // returning std::nullopt. If any other value is returned, it is requesting that a status is set. This
+    // includes CHIP_NO_ERROR: in this case CHIP_NO_ERROR would mean set a `status success on the command`
+    if (status.has_value())
+    {
+        apCommandObj.AddStatus(aCommandPath, status->GetStatusCode());
+    }
+#else
     CommandHandlerInterface * handler =
         CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(aCommandPath.mEndpointId, aCommandPath.mClusterId);
 
@@ -1717,6 +1736,7 @@ void InteractionModelEngine::DispatchCommand(CommandHandlerImpl & apCommandObj,
     }
 
     DispatchSingleClusterCommand(aCommandPath, apPayload, &apCommandObj);
+#endif // CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
 }
 
 Protocols::InteractionModel::Status InteractionModelEngine::CommandExists(const ConcreteCommandPath & aCommandPath)

diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp
@@ -17,6 +17,7 @@
 #include <app/codegen-data-model-provider/CodegenDataModelProvider.h>
 
 #include <app-common/zap-generated/attribute-type.h>
+#include <app/CommandHandlerInterfaceRegistry.h>
 #include <app/RequiredPrivilege.h>
 #include <app/util/IMClusterCommandHandler.h>
 #include <app/util/attribute-storage.h>
@@ -229,20 +230,28 @@ bool CodegenDataModelProvider::EmberCommandListIterator::Exists(const CommandId
     return (*mCurrentHint == toCheck);
 }
 
-DataModel::ActionReturnStatus CodegenDataModelProvider::Invoke(const DataModel::InvokeRequest & request,
-                                                               TLV::TLVReader & input_arguments, CommandHandler * handler)
+std::optional<DataModel::ActionReturnStatus> CodegenDataModelProvider::Invoke(const DataModel::InvokeRequest & request,
+                                                                              TLV::TLVReader & input_arguments,
+                                                                              CommandHandler * handler)
 {
-    // TODO: CommandHandlerInterface support is currently
-    //       residing in InteractionModelEngine itself. We may want to separate this out
-    //       into its own registry, similar to attributes, so that IM is decoupled from actual storage of things.
-    //
-    //       Open issue at https://github.com/project-chip/connectedhomeip/issues/34258
-
-    // Ember dispatching automatically uses `handler` to set an appropriate result or status
-    // This never fails (as handler error is encoded as needed).
-    DispatchSingleClusterCommand(request.path, input_arguments, handler);
+    CommandHandlerInterface * handler_interface =
+        CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(request.path.mEndpointId, request.path.mClusterId);
+
+    if (handler_interface)
+    {
+        CommandHandlerInterface::HandlerContext context(*handler, request.path, input_arguments);
+        handler_interface->InvokeCommand(context);
 
-    return CHIP_NO_ERROR;
+        // If the command was handled, don't proceed any further and return successfully.
+        if (context.mCommandHandled)
+        {
+            return std::nullopt;
+        }
+    }
+
+    // Ember always sets the return in the handler
+    DispatchSingleClusterCommand(request.path, input_arguments, handler);
+    return std::nullopt;
 }
 
 EndpointId CodegenDataModelProvider::FirstEndpoint()

diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider.h b/src/app/codegen-data-model-provider/CodegenDataModelProvider.h
@@ -73,8 +73,8 @@ class CodegenDataModelProvider : public chip::app::DataModel::Provider
                                                 AttributeValueEncoder & encoder) override;
     DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request,
                                                  AttributeValueDecoder & decoder) override;
-    DataModel::ActionReturnStatus Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
-                                         CommandHandler * handler) override;
+    std::optional<DataModel::ActionReturnStatus> Invoke(const DataModel::InvokeRequest & request,
+                                                        chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override;
 
     /// attribute tree iteration
     EndpointId FirstEndpoint() override;

diff --git a/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp
@@ -2460,7 +2460,7 @@ TEST(TestCodegenModelViaMocks, EmberInvokeTest)
         const uint32_t kDispatchCountPre = chip::Test::DispatchCount();
 
         // Using a handler set to nullptr as it is not used by the impl
-        ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ nullptr), CHIP_NO_ERROR);
+        ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ nullptr), std::nullopt);
 
         EXPECT_EQ(chip::Test::DispatchCount(), kDispatchCountPre + 1); // single dispatch
         EXPECT_EQ(chip::Test::GetLastDispatchPath(), kCommandPath);    // for the right path
@@ -2474,7 +2474,7 @@ TEST(TestCodegenModelViaMocks, EmberInvokeTest)
         const uint32_t kDispatchCountPre = chip::Test::DispatchCount();
 
         // Using a handler set to nullpotr as it is not used by the impl
-        ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ nullptr), CHIP_NO_ERROR);
+        ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ nullptr), std::nullopt);
 
         EXPECT_EQ(chip::Test::DispatchCount(), kDispatchCountPre + 1); // single dispatch
         EXPECT_EQ(chip::Test::GetLastDispatchPath(), kCommandPath);    // for the right path

diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h
@@ -88,18 +88,20 @@ class Provider : public ProviderMetadataTree
     virtual ActionReturnStatus WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) = 0;
 
     /// `handler` is used to send back the reply.
+    ///    - returning `std::nullopt` means that return value was placed in handler directly.
+    ///      This includes cases where command handling and value return will be done asynchronously.
     ///    - returning a value other than Success implies an error reply (error and data are mutually exclusive)
     ///
-    /// Returning anything other than CHIP_NO_ERROR or Status::Success (i.e. success without a return code)
-    /// means that the invoke will be considered to be returning the given path-specific status WITHOUT any data (any data
-    /// that was sent via CommandHandler is to be rolled back/discarded).
-    ///
-    /// This is because only one of the following may be encoded in a response:
-    ///    - data (as CommandDataIB) which is assumed a "response as a success"
-    ///    - status (as a CommandStatusIB) which is considered a final status, usually an error however
-    ///      cluster-specific success statuses also exist.
-    virtual ActionReturnStatus Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
-                                      CommandHandler * handler) = 0;
+    /// Return value expectations:
+    ///   - if a response has been placed into `handler` then std::nullopt MUST be returned. In particular
+    ///     note that CHIP_NO_ERROR is NOT the same as std::nullopt:
+    ///        > CHIP_NO_ERROR means handler had no status set and we expect the caller to AddStatus(success)
+    ///        > std::nullopt means that handler has added an appropriate data/status response
+    ///   - if a value is returned (not nullopt) then the handler response MUST NOT be filled. The caller
+    ///     will then issue `handler->AddStatus(request.path, <return_value>->GetStatusCode())`. This is a
+    ///     convenience to make writing Invoke calls easier.
+    virtual std::optional<ActionReturnStatus> Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
+                                                     CommandHandler * handler) = 0;
 
 private:
     InteractionModelContext mContext = { nullptr };

diff --git a/src/app/tests/test-interaction-model-api.cpp b/src/app/tests/test-interaction-model-api.cpp
@@ -173,10 +173,10 @@ ActionReturnStatus TestImCustomDataModel::WriteAttribute(const WriteAttributeReq
     return CHIP_ERROR_NOT_IMPLEMENTED;
 }
 
-ActionReturnStatus TestImCustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
-                                                 CommandHandler * handler)
+std::optional<ActionReturnStatus> TestImCustomDataModel::Invoke(const InvokeRequest & request,
+                                                                chip::TLV::TLVReader & input_arguments, CommandHandler * handler)
 {
-    return CHIP_ERROR_NOT_IMPLEMENTED;
+    return std::make_optional<ActionReturnStatus>(CHIP_ERROR_NOT_IMPLEMENTED);
 }
 
 EndpointId TestImCustomDataModel::FirstEndpoint()

diff --git a/src/app/tests/test-interaction-model-api.h b/src/app/tests/test-interaction-model-api.h
@@ -122,8 +122,8 @@ class TestImCustomDataModel : public DataModel::Provider
                                                 AttributeValueEncoder & encoder) override;
     DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request,
                                                  AttributeValueDecoder & decoder) override;
-    DataModel::ActionReturnStatus Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
-                                         CommandHandler * handler) override;
+    std::optional<DataModel::ActionReturnStatus> Invoke(const DataModel::InvokeRequest & request,
+                                                        chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override;
 
     EndpointId FirstEndpoint() override;
     EndpointId NextEndpoint(EndpointId before) override;

diff --git a/src/controller/tests/data_model/DataModelFixtures.cpp b/src/controller/tests/data_model/DataModelFixtures.cpp
@@ -525,10 +525,10 @@ ActionReturnStatus CustomDataModel::WriteAttribute(const WriteAttributeRequest &
     return CHIP_ERROR_NOT_IMPLEMENTED;
 }
 
-ActionReturnStatus CustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
-                                           CommandHandler * handler)
+std::optional<ActionReturnStatus> CustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
+                                                          CommandHandler * handler)
 {
-    return CHIP_ERROR_NOT_IMPLEMENTED;
+    return std::make_optional<ActionReturnStatus>(CHIP_ERROR_NOT_IMPLEMENTED);
 }
 
 EndpointId CustomDataModel::FirstEndpoint()

diff --git a/src/controller/tests/data_model/DataModelFixtures.h b/src/controller/tests/data_model/DataModelFixtures.h
@@ -119,8 +119,8 @@ class CustomDataModel : public DataModel::Provider
                                                 AttributeValueEncoder & encoder) override;
     DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request,
                                                  AttributeValueDecoder & decoder) override;
-    DataModel::ActionReturnStatus Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
-                                         CommandHandler * handler) override;
+    std::optional<DataModel::ActionReturnStatus> Invoke(const DataModel::InvokeRequest & request,
+                                                        chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override;
 
     EndpointId FirstEndpoint() override;
     EndpointId NextEndpoint(EndpointId before) override;