diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 88c1777b6c8224..9b4282fb036d88 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -43,6 +43,10 @@ #include #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE +#include + +// TODO: defaulting to codegen should eventually be an application choice and not +// hard-coded in the interaction model #include #endif @@ -1699,6 +1703,21 @@ CHIP_ERROR InteractionModelEngine::PushFront(SingleLinkedListNode *& 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 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 index 312f175778a314..4d42fc90282104 100644 --- a/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp +++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -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 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 index b486b953976328..70d5e6f41abdd6 100644 --- 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 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 index 649c6f02195a27..d5c0ea673fec80 100644 --- 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 index f38568319eb425..6bd3f439e8a417 100644 --- 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, ->GetStatusCode())`. This is a + /// convenience to make writing Invoke calls easier. + virtual std::optional 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 index f483f4b3de88a4..4ecc4d66706b0c 100644 --- 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 TestImCustomDataModel::Invoke(const InvokeRequest & request, + chip::TLV::TLVReader & input_arguments, CommandHandler * handler) { - return CHIP_ERROR_NOT_IMPLEMENTED; + return std::make_optional(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 index 6de6c9c34a2e21..7b1b1ad335b1e8 100644 --- 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 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 index 05fa16b956bb0c..0333b24239e0e7 100644 --- 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 CustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, + CommandHandler * handler) { - return CHIP_ERROR_NOT_IMPLEMENTED; + return std::make_optional(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 index cfb2edf5db83f3..9c23b395bb7810 100644 --- 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 Invoke(const DataModel::InvokeRequest & request, + chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override; EndpointId FirstEndpoint() override; EndpointId NextEndpoint(EndpointId before) override;