Skip to content

Commit 31894f6

Browse files
andy31415tehampsonandreilitvinbzbarsky-apple
authoredSep 17, 2024
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 <thampson@google.com> * Update documentation on invoke * More documentation * More documentation again * Update src/app/data-model-provider/Provider.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/data-model-provider/Provider.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Comment cleanup * Update src/app/data-model-provider/Provider.h Co-authored-by: Terence Hampson <thampson@google.com> --------- Co-authored-by: Terence Hampson <thampson@google.com> Co-authored-by: Andrei Litvin <andreilitvin@google.com> Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
1 parent 85b2fd3 commit 31894f6

9 files changed

+67
-36
lines changed
 

‎src/app/InteractionModelEngine.cpp

+20
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@
4343
#include <lib/support/FibonacciUtils.h>
4444

4545
#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
46+
#include <app/data-model-provider/ActionReturnStatus.h>
47+
48+
// TODO: defaulting to codegen should eventually be an application choice and not
49+
// hard-coded in the interaction model
4650
#include <app/codegen-data-model-provider/Instance.h>
4751
#endif
4852

@@ -1699,6 +1703,21 @@ CHIP_ERROR InteractionModelEngine::PushFront(SingleLinkedListNode<T> *& aObjectL
16991703
void InteractionModelEngine::DispatchCommand(CommandHandlerImpl & apCommandObj, const ConcreteCommandPath & aCommandPath,
17001704
TLV::TLVReader & apPayload)
17011705
{
1706+
#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
1707+
1708+
DataModel::InvokeRequest request;
1709+
request.path = aCommandPath;
1710+
1711+
std::optional<DataModel::ActionReturnStatus> status = GetDataModelProvider()->Invoke(request, apPayload, &apCommandObj);
1712+
1713+
// Provider indicates that handler status or data was already set (or will be set asynchronously) by
1714+
// returning std::nullopt. If any other value is returned, it is requesting that a status is set. This
1715+
// includes CHIP_NO_ERROR: in this case CHIP_NO_ERROR would mean set a `status success on the command`
1716+
if (status.has_value())
1717+
{
1718+
apCommandObj.AddStatus(aCommandPath, status->GetStatusCode());
1719+
}
1720+
#else
17021721
CommandHandlerInterface * handler =
17031722
CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(aCommandPath.mEndpointId, aCommandPath.mClusterId);
17041723

@@ -1717,6 +1736,7 @@ void InteractionModelEngine::DispatchCommand(CommandHandlerImpl & apCommandObj,
17171736
}
17181737

17191738
DispatchSingleClusterCommand(aCommandPath, apPayload, &apCommandObj);
1739+
#endif // CHIP_CONFIG_USE_DATA_MODEL_INTERFACE
17201740
}
17211741

17221742
Protocols::InteractionModel::Status InteractionModelEngine::CommandExists(const ConcreteCommandPath & aCommandPath)

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

+21-12
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include <app/codegen-data-model-provider/CodegenDataModelProvider.h>
1818

1919
#include <app-common/zap-generated/attribute-type.h>
20+
#include <app/CommandHandlerInterfaceRegistry.h>
2021
#include <app/RequiredPrivilege.h>
2122
#include <app/util/IMClusterCommandHandler.h>
2223
#include <app/util/attribute-storage.h>
@@ -229,20 +230,28 @@ bool CodegenDataModelProvider::EmberCommandListIterator::Exists(const CommandId
229230
return (*mCurrentHint == toCheck);
230231
}
231232

232-
DataModel::ActionReturnStatus CodegenDataModelProvider::Invoke(const DataModel::InvokeRequest & request,
233-
TLV::TLVReader & input_arguments, CommandHandler * handler)
233+
std::optional<DataModel::ActionReturnStatus> CodegenDataModelProvider::Invoke(const DataModel::InvokeRequest & request,
234+
TLV::TLVReader & input_arguments,
235+
CommandHandler * handler)
234236
{
235-
// TODO: CommandHandlerInterface support is currently
236-
// residing in InteractionModelEngine itself. We may want to separate this out
237-
// into its own registry, similar to attributes, so that IM is decoupled from actual storage of things.
238-
//
239-
// Open issue at https://github.com/project-chip/connectedhomeip/issues/34258
240-
241-
// Ember dispatching automatically uses `handler` to set an appropriate result or status
242-
// This never fails (as handler error is encoded as needed).
243-
DispatchSingleClusterCommand(request.path, input_arguments, handler);
237+
CommandHandlerInterface * handler_interface =
238+
CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(request.path.mEndpointId, request.path.mClusterId);
239+
240+
if (handler_interface)
241+
{
242+
CommandHandlerInterface::HandlerContext context(*handler, request.path, input_arguments);
243+
handler_interface->InvokeCommand(context);
244244

245-
return CHIP_NO_ERROR;
245+
// If the command was handled, don't proceed any further and return successfully.
246+
if (context.mCommandHandled)
247+
{
248+
return std::nullopt;
249+
}
250+
}
251+
252+
// Ember always sets the return in the handler
253+
DispatchSingleClusterCommand(request.path, input_arguments, handler);
254+
return std::nullopt;
246255
}
247256

248257
EndpointId CodegenDataModelProvider::FirstEndpoint()

‎src/app/codegen-data-model-provider/CodegenDataModelProvider.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,8 @@ class CodegenDataModelProvider : public chip::app::DataModel::Provider
7373
AttributeValueEncoder & encoder) override;
7474
DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request,
7575
AttributeValueDecoder & decoder) override;
76-
DataModel::ActionReturnStatus Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
77-
CommandHandler * handler) override;
76+
std::optional<DataModel::ActionReturnStatus> Invoke(const DataModel::InvokeRequest & request,
77+
chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override;
7878

7979
/// attribute tree iteration
8080
EndpointId FirstEndpoint() override;

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -2460,7 +2460,7 @@ TEST(TestCodegenModelViaMocks, EmberInvokeTest)
24602460
const uint32_t kDispatchCountPre = chip::Test::DispatchCount();
24612461

24622462
// Using a handler set to nullptr as it is not used by the impl
2463-
ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ nullptr), CHIP_NO_ERROR);
2463+
ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ nullptr), std::nullopt);
24642464

24652465
EXPECT_EQ(chip::Test::DispatchCount(), kDispatchCountPre + 1); // single dispatch
24662466
EXPECT_EQ(chip::Test::GetLastDispatchPath(), kCommandPath); // for the right path
@@ -2474,7 +2474,7 @@ TEST(TestCodegenModelViaMocks, EmberInvokeTest)
24742474
const uint32_t kDispatchCountPre = chip::Test::DispatchCount();
24752475

24762476
// Using a handler set to nullpotr as it is not used by the impl
2477-
ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ nullptr), CHIP_NO_ERROR);
2477+
ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ nullptr), std::nullopt);
24782478

24792479
EXPECT_EQ(chip::Test::DispatchCount(), kDispatchCountPre + 1); // single dispatch
24802480
EXPECT_EQ(chip::Test::GetLastDispatchPath(), kCommandPath); // for the right path

‎src/app/data-model-provider/Provider.h

+12-10
Original file line numberDiff line numberDiff line change
@@ -88,18 +88,20 @@ class Provider : public ProviderMetadataTree
8888
virtual ActionReturnStatus WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) = 0;
8989

9090
/// `handler` is used to send back the reply.
91+
/// - returning `std::nullopt` means that return value was placed in handler directly.
92+
/// This includes cases where command handling and value return will be done asynchronously.
9193
/// - returning a value other than Success implies an error reply (error and data are mutually exclusive)
9294
///
93-
/// Returning anything other than CHIP_NO_ERROR or Status::Success (i.e. success without a return code)
94-
/// means that the invoke will be considered to be returning the given path-specific status WITHOUT any data (any data
95-
/// that was sent via CommandHandler is to be rolled back/discarded).
96-
///
97-
/// This is because only one of the following may be encoded in a response:
98-
/// - data (as CommandDataIB) which is assumed a "response as a success"
99-
/// - status (as a CommandStatusIB) which is considered a final status, usually an error however
100-
/// cluster-specific success statuses also exist.
101-
virtual ActionReturnStatus Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
102-
CommandHandler * handler) = 0;
95+
/// Return value expectations:
96+
/// - if a response has been placed into `handler` then std::nullopt MUST be returned. In particular
97+
/// note that CHIP_NO_ERROR is NOT the same as std::nullopt:
98+
/// > CHIP_NO_ERROR means handler had no status set and we expect the caller to AddStatus(success)
99+
/// > std::nullopt means that handler has added an appropriate data/status response
100+
/// - if a value is returned (not nullopt) then the handler response MUST NOT be filled. The caller
101+
/// will then issue `handler->AddStatus(request.path, <return_value>->GetStatusCode())`. This is a
102+
/// convenience to make writing Invoke calls easier.
103+
virtual std::optional<ActionReturnStatus> Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
104+
CommandHandler * handler) = 0;
103105

104106
private:
105107
InteractionModelContext mContext = { nullptr };

‎src/app/tests/test-interaction-model-api.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -173,10 +173,10 @@ ActionReturnStatus TestImCustomDataModel::WriteAttribute(const WriteAttributeReq
173173
return CHIP_ERROR_NOT_IMPLEMENTED;
174174
}
175175

176-
ActionReturnStatus TestImCustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
177-
CommandHandler * handler)
176+
std::optional<ActionReturnStatus> TestImCustomDataModel::Invoke(const InvokeRequest & request,
177+
chip::TLV::TLVReader & input_arguments, CommandHandler * handler)
178178
{
179-
return CHIP_ERROR_NOT_IMPLEMENTED;
179+
return std::make_optional<ActionReturnStatus>(CHIP_ERROR_NOT_IMPLEMENTED);
180180
}
181181

182182
EndpointId TestImCustomDataModel::FirstEndpoint()

‎src/app/tests/test-interaction-model-api.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -122,8 +122,8 @@ class TestImCustomDataModel : public DataModel::Provider
122122
AttributeValueEncoder & encoder) override;
123123
DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request,
124124
AttributeValueDecoder & decoder) override;
125-
DataModel::ActionReturnStatus Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
126-
CommandHandler * handler) override;
125+
std::optional<DataModel::ActionReturnStatus> Invoke(const DataModel::InvokeRequest & request,
126+
chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override;
127127

128128
EndpointId FirstEndpoint() override;
129129
EndpointId NextEndpoint(EndpointId before) override;

‎src/controller/tests/data_model/DataModelFixtures.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -525,10 +525,10 @@ ActionReturnStatus CustomDataModel::WriteAttribute(const WriteAttributeRequest &
525525
return CHIP_ERROR_NOT_IMPLEMENTED;
526526
}
527527

528-
ActionReturnStatus CustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
529-
CommandHandler * handler)
528+
std::optional<ActionReturnStatus> CustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
529+
CommandHandler * handler)
530530
{
531-
return CHIP_ERROR_NOT_IMPLEMENTED;
531+
return std::make_optional<ActionReturnStatus>(CHIP_ERROR_NOT_IMPLEMENTED);
532532
}
533533

534534
EndpointId CustomDataModel::FirstEndpoint()

‎src/controller/tests/data_model/DataModelFixtures.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -119,8 +119,8 @@ class CustomDataModel : public DataModel::Provider
119119
AttributeValueEncoder & encoder) override;
120120
DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request,
121121
AttributeValueDecoder & decoder) override;
122-
DataModel::ActionReturnStatus Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments,
123-
CommandHandler * handler) override;
122+
std::optional<DataModel::ActionReturnStatus> Invoke(const DataModel::InvokeRequest & request,
123+
chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override;
124124

125125
EndpointId FirstEndpoint() override;
126126
EndpointId NextEndpoint(EndpointId before) override;

0 commit comments

Comments
 (0)