From 1107736508d9975eb19967cbd2cd21c98997d016 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 1 Apr 2022 20:31:55 -0400 Subject: [PATCH] Add a way for CommandHandlerInterface to provide a list of accepted/generated command ids. (#16947) * Add a way for CommandHandlerInterface to provide a list of accepted/generated command ids. * Address review comments * Fix shadowing warnings --- src/app/CommandHandlerInterface.h | 53 ++++++ src/app/util/attribute-storage.cpp | 2 +- src/app/util/attribute-storage.h | 2 +- .../util/ember-compatibility-functions.cpp | 114 +++++++++++-- .../tests/TestServerCommandDispatch.cpp | 158 +++++++++++++++++- 5 files changed, 303 insertions(+), 26 deletions(-) diff --git a/src/app/CommandHandlerInterface.h b/src/app/CommandHandlerInterface.h index c16ee865a8df78..6ec2547befa1d0 100644 --- a/src/app/CommandHandlerInterface.h +++ b/src/app/CommandHandlerInterface.h @@ -19,9 +19,12 @@ #pragma once #include +#include #include #include #include // So we can encode lists +#include +#include namespace chip { namespace app { @@ -98,6 +101,56 @@ 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. + * + * 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. + */ + virtual CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) + { + return CHIP_ERROR_NOT_IMPLEMENTED; + } + + /** + * Function that may be implemented to enumerate generated (response) + * commands for the given cluster. + * + * 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. + */ + virtual CHIP_ERROR EnumerateGeneratedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) + { + return CHIP_ERROR_NOT_IMPLEMENTED; + } + /** * Mechanism for keeping track of a chain of CommandHandlerInterface. */ diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index 925d891d693114..8c5b7b95add567 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -198,7 +198,7 @@ uint16_t emberAfGetDynamicIndexFromEndpoint(EndpointId id) return kEmberInvalidEndpointIndex; } -EmberAfStatus emberAfSetDynamicEndpoint(uint16_t index, EndpointId id, EmberAfEndpointType * ep, uint16_t deviceId, +EmberAfStatus emberAfSetDynamicEndpoint(uint16_t index, EndpointId id, const EmberAfEndpointType * ep, uint16_t deviceId, uint8_t deviceVersion, const Span & dataVersionStorage) { auto realIndex = index + FIXED_ENDPOINT_COUNT; diff --git a/src/app/util/attribute-storage.h b/src/app/util/attribute-storage.h index 1dcfbb5a9db4ea..9c7b227b9496b4 100644 --- a/src/app/util/attribute-storage.h +++ b/src/app/util/attribute-storage.h @@ -248,7 +248,7 @@ uint16_t emberAfGetDeviceIdForEndpoint(chip::EndpointId endpoint); // // The memory backing dataVersionStorage needs to stay alive until this dynamic // endpoint is cleared. -EmberAfStatus emberAfSetDynamicEndpoint(uint16_t index, chip::EndpointId id, EmberAfEndpointType * ep, uint16_t deviceId, +EmberAfStatus emberAfSetDynamicEndpoint(uint16_t index, chip::EndpointId id, const EmberAfEndpointType * ep, uint16_t deviceId, uint8_t deviceVersion, const chip::Span & dataVersionStorage); chip::EndpointId emberAfClearDynamicEndpoint(uint16_t index); uint16_t emberAfGetDynamicIndexFromEndpoint(chip::EndpointId id); diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp index 4123728334e3da..1ce9f152538db8 100644 --- a/src/app/util/ember-compatibility-functions.cpp +++ b/src/app/util/ember-compatibility-functions.cpp @@ -22,6 +22,7 @@ */ #include +#include #include #include #include @@ -237,7 +238,52 @@ Protocols::InteractionModel::Status ServerClusterCommandExists(const ConcreteCom return Status::UnsupportedCluster; } - for (const CommandId * cmd = cluster->acceptedCommandList; cmd != nullptr; cmd++) + auto * commandHandler = + InteractionModelEngine::GetInstance()->FindCommandHandler(aCommandPath.mEndpointId, aCommandPath.mClusterId); + if (commandHandler) + { + struct Context + { + bool commandExists; + CommandId targetCommand; + } context{ false, aCommandPath.mCommandId }; + + CHIP_ERROR err = commandHandler->EnumerateAcceptedCommands( + aCommandPath, + [](CommandId command, void * closure) -> Loop { + auto * ctx = static_cast(closure); + if (ctx->targetCommand == command) + { + ctx->commandExists = true; + return Loop::Break; + } + return Loop::Continue; + }, + &context); + + // We now have three cases: + // 1) handler returned CHIP_ERROR_NOT_IMPLEMENTED. In that case we + // should fall back to looking at cluster->acceptedCommandList + // 2) handler returned success. In that case, the handler is the source + // of truth about the set of accepted commands, and + // context.commandExists indicates whether a aCommandPath.mCommandId + // was in the set, and we should return either Success or + // UnsupportedCommand accordingly. + // 3) Some other status was returned. In this case we should probably + // err on the side of not allowing the command, since we have no idea + // whether to allow it or not. + if (err != CHIP_ERROR_NOT_IMPLEMENTED) + { + if (err == CHIP_NO_ERROR) + { + return context.commandExists ? Status::Success : Status::UnsupportedCommand; + } + + return Status::Failure; + } + } + + for (const CommandId * cmd = cluster->acceptedCommandList; cmd != nullptr && *cmd != kInvalidCommandId; cmd++) { if (*cmd == aCommandPath.mCommandId) { @@ -315,6 +361,13 @@ class GlobalAttributeReader : public MandatoryGlobalAttributeReader GlobalAttributeReader(const EmberAfCluster * aCluster) : MandatoryGlobalAttributeReader(aCluster) {} CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override; + +private: + typedef CHIP_ERROR (CommandHandlerInterface::*CommandListEnumerator)(const ConcreteClusterPath & cluster, + CommandHandlerInterface::CommandIdCallback callback, + void * context); + static CHIP_ERROR EncodeCommandList(const ConcreteClusterPath & aClusterPath, AttributeValueEncoder & aEncoder, + CommandListEnumerator aEnumerator, const CommandId * aClusterCommandList); }; CHIP_ERROR GlobalAttributeReader::Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) @@ -357,26 +410,57 @@ CHIP_ERROR GlobalAttributeReader::Read(const ConcreteReadAttributePath & aPath, return CHIP_NO_ERROR; }); case AcceptedCommandList::Id: - return aEncoder.EncodeList([this](const auto & encoder) { - for (const CommandId * cmd = mCluster->acceptedCommandList; cmd != nullptr && *cmd != kInvalidCommandId; cmd++) - { - ReturnErrorOnFailure(encoder.Encode(*cmd)); - } - return CHIP_NO_ERROR; - }); + return EncodeCommandList(aPath, aEncoder, &CommandHandlerInterface::EnumerateAcceptedCommands, + mCluster->acceptedCommandList); case GeneratedCommandList::Id: - return aEncoder.EncodeList([this](const auto & encoder) { - for (const CommandId * cmd = mCluster->generatedCommandList; cmd != nullptr && *cmd != kInvalidCommandId; cmd++) - { - ReturnErrorOnFailure(encoder.Encode(*cmd)); - } - return CHIP_NO_ERROR; - }); + return EncodeCommandList(aPath, aEncoder, &CommandHandlerInterface::EnumerateGeneratedCommands, + mCluster->generatedCommandList); default: return CHIP_NO_ERROR; } } +CHIP_ERROR GlobalAttributeReader::EncodeCommandList(const ConcreteClusterPath & aClusterPath, AttributeValueEncoder & aEncoder, + GlobalAttributeReader::CommandListEnumerator aEnumerator, + const CommandId * aClusterCommandList) +{ + return aEncoder.EncodeList([&](const auto & encoder) { + auto * commandHandler = + InteractionModelEngine::GetInstance()->FindCommandHandler(aClusterPath.mEndpointId, aClusterPath.mClusterId); + if (commandHandler) + { + struct Context + { + decltype(encoder) & commandIdEncoder; + CHIP_ERROR err; + } context{ encoder, CHIP_NO_ERROR }; + CHIP_ERROR err = (commandHandler->*aEnumerator)( + aClusterPath, + [](CommandId command, void * closure) -> Loop { + auto * ctx = static_cast(closure); + ctx->err = ctx->commandIdEncoder.Encode(command); + if (ctx->err != CHIP_NO_ERROR) + { + return Loop::Break; + } + return Loop::Continue; + }, + &context); + if (err != CHIP_ERROR_NOT_IMPLEMENTED) + { + return context.err; + } + // Else fall through to the list in aClusterCommandList. + } + + for (const CommandId * cmd = aClusterCommandList; cmd != nullptr && *cmd != kInvalidCommandId; cmd++) + { + ReturnErrorOnFailure(encoder.Encode(*cmd)); + } + return CHIP_NO_ERROR; + }); +} + // Helper function for trying to read an attribute value via an // AttributeAccessInterface. On failure, the read has failed. On success, the // aTriedEncode outparam is set to whether the AttributeAccessInterface tried to encode a value. diff --git a/src/controller/tests/TestServerCommandDispatch.cpp b/src/controller/tests/TestServerCommandDispatch.cpp index a824a57cd58912..93269695051dec 100644 --- a/src/controller/tests/TestServerCommandDispatch.cpp +++ b/src/controller/tests/TestServerCommandDispatch.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -41,6 +42,7 @@ using TestContext = chip::Test::AppContext; using namespace chip; +using namespace chip::app; using namespace chip::app::Clusters; namespace { @@ -67,8 +69,15 @@ class TestClusterCommandHandler : public chip::app::CommandHandlerInterface ~TestClusterCommandHandler() { chip::app::InteractionModelEngine::GetInstance()->UnregisterCommandHandler(this); } + void OverrideAcceptedCommands() { mOverrideAcceptedCommands = true; } + void ClaimNoCommands() { mClaimNoCommands = true; } + private: void InvokeCommand(chip::app::CommandHandlerInterface::HandlerContext & handlerContext) final; + CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) final; + + bool mOverrideAcceptedCommands = false; + bool mClaimNoCommands = false; }; void TestClusterCommandHandler::InvokeCommand(chip::app::CommandHandlerInterface::HandlerContext & handlerContext) @@ -100,6 +109,24 @@ void TestClusterCommandHandler::InvokeCommand(chip::app::CommandHandlerInterface }); } +CHIP_ERROR TestClusterCommandHandler::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, + CommandHandlerInterface::CommandIdCallback callback, void * context) +{ + if (!mOverrideAcceptedCommands) + { + return CHIP_ERROR_NOT_IMPLEMENTED; + } + + if (mClaimNoCommands) + { + return CHIP_NO_ERROR; + } + + // We just have one command id. + callback(TestCluster::Commands::TestSimpleArgumentRequest::Id, context); + return CHIP_NO_ERROR; +} + } // namespace namespace { @@ -110,8 +137,15 @@ class TestCommandInteraction TestCommandInteraction() {} static void TestNoHandler(nlTestSuite * apSuite, void * apContext); static void TestDataResponse(nlTestSuite * apSuite, void * apContext); + static void TestDataResponseNoCommand1(nlTestSuite * apSuite, void * apContext); + static void TestDataResponseNoCommand2(nlTestSuite * apSuite, void * apContext); + static void TestDataResponseNoCommand3(nlTestSuite * apSuite, void * apContext); + static void TestDataResponseHandlerOverride1(nlTestSuite * apSuite, void * apContext); + static void TestDataResponseHandlerOverride2(nlTestSuite * apSuite, void * apContext); private: + static void TestDataResponseHelper(nlTestSuite * apSuite, void * apContext, const EmberAfEndpointType * aEndpoint, + bool aExpectSuccess); }; // We want to send a TestSimpleArgumentRequest::Type, but get a @@ -177,23 +211,40 @@ DECLARE_DYNAMIC_ATTRIBUTE(chip::app::Clusters::Descriptor::Attributes::DeviceLis DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(testClusterAttrs) DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(); -constexpr CommandId testClusterCommands[] = { +constexpr CommandId testClusterCommands1[] = { TestCluster::Commands::TestSimpleArgumentRequest::Id, kInvalidCommandId, }; -DECLARE_DYNAMIC_CLUSTER_LIST_BEGIN(testEndpointClusters) -DECLARE_DYNAMIC_CLUSTER(chip::app::Clusters::TestCluster::Id, testClusterAttrs, testClusterCommands, nullptr), +DECLARE_DYNAMIC_CLUSTER_LIST_BEGIN(testEndpointClusters1) +DECLARE_DYNAMIC_CLUSTER(chip::app::Clusters::TestCluster::Id, testClusterAttrs, testClusterCommands1, nullptr), DECLARE_DYNAMIC_CLUSTER(chip::app::Clusters::Descriptor::Id, descriptorAttrs, nullptr, nullptr), DECLARE_DYNAMIC_CLUSTER_LIST_END; -DECLARE_DYNAMIC_ENDPOINT(testEndpoint, testEndpointClusters); +DECLARE_DYNAMIC_ENDPOINT(testEndpoint1, testEndpointClusters1); -void TestCommandInteraction::TestDataResponse(nlTestSuite * apSuite, void * apContext) +constexpr CommandId testClusterCommands2[] = { + kInvalidCommandId, +}; +DECLARE_DYNAMIC_CLUSTER_LIST_BEGIN(testEndpointClusters2) +DECLARE_DYNAMIC_CLUSTER(chip::app::Clusters::TestCluster::Id, testClusterAttrs, testClusterCommands2, nullptr), + DECLARE_DYNAMIC_CLUSTER(chip::app::Clusters::Descriptor::Id, descriptorAttrs, nullptr, nullptr), + DECLARE_DYNAMIC_CLUSTER_LIST_END; + +DECLARE_DYNAMIC_ENDPOINT(testEndpoint2, testEndpointClusters2); + +DECLARE_DYNAMIC_CLUSTER_LIST_BEGIN(testEndpointClusters3) +DECLARE_DYNAMIC_CLUSTER(chip::app::Clusters::TestCluster::Id, testClusterAttrs, nullptr, nullptr), + DECLARE_DYNAMIC_CLUSTER(chip::app::Clusters::Descriptor::Id, descriptorAttrs, nullptr, nullptr), + DECLARE_DYNAMIC_CLUSTER_LIST_END; + +DECLARE_DYNAMIC_ENDPOINT(testEndpoint3, testEndpointClusters3); + +void TestCommandInteraction::TestDataResponseHelper(nlTestSuite * apSuite, void * apContext, const EmberAfEndpointType * aEndpoint, + bool aExpectSuccess) { TestContext & ctx = *static_cast(apContext); FakeRequest request; auto sessionHandle = ctx.GetSessionBobToAlice(); - TestClusterCommandHandler commandHandler; bool onSuccessWasCalled = false; bool onFailureWasCalled = false; @@ -202,10 +253,13 @@ void TestCommandInteraction::TestDataResponse(nlTestSuite * apSuite, void * apCo // // Register descriptors for this endpoint since they are needed - // at command validation time to ensure the command actually exists on that endpoint. + // at command validation time to ensure the command actually exists on that + // endpoint. // - DataVersion dataVersionStorage[ArraySize(testEndpointClusters)]; - emberAfSetDynamicEndpoint(0, kTestEndpointId, &testEndpoint, 0, 0, Span(dataVersionStorage)); + // All our endpoints have the same number of clusters, so just pick one. + // + DataVersion dataVersionStorage[ArraySize(testEndpointClusters1)]; + emberAfSetDynamicEndpoint(0, kTestEndpointId, aEndpoint, 0, 0, Span(dataVersionStorage)); // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's // not safe to do so. @@ -241,17 +295,103 @@ void TestCommandInteraction::TestDataResponse(nlTestSuite * apSuite, void * apCo ctx.DrainAndServiceIO(); + NL_TEST_ASSERT(apSuite, onSuccessWasCalled == aExpectSuccess && onFailureWasCalled != aExpectSuccess); + NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + + onSuccessWasCalled = false; + onFailureWasCalled = false; + + auto readSuccessCb = [apSuite, &onSuccessWasCalled, aExpectSuccess](const ConcreteDataAttributePath &, + const DataModel::DecodableList & commandList) { + auto count = 0; + auto iter = commandList.begin(); + while (iter.Next()) + { + // We only expect 0 or 1 command ids here. + NL_TEST_ASSERT(apSuite, count == 0); + NL_TEST_ASSERT(apSuite, iter.GetValue() == TestCluster::Commands::TestSimpleArgumentRequest::Id); + ++count; + } + NL_TEST_ASSERT(apSuite, iter.GetStatus() == CHIP_NO_ERROR); + if (aExpectSuccess) + { + NL_TEST_ASSERT(apSuite, count == 1); + } + else + { + NL_TEST_ASSERT(apSuite, count == 0); + } + onSuccessWasCalled = true; + }; + auto readFailureCb = [&onFailureWasCalled](const ConcreteDataAttributePath *, CHIP_ERROR aError) { onFailureWasCalled = true; }; + + chip::Controller::ReadAttribute( + &ctx.GetExchangeManager(), sessionHandle, kTestEndpointId, readSuccessCb, readFailureCb); + + ctx.DrainAndServiceIO(); + NL_TEST_ASSERT(apSuite, onSuccessWasCalled && !onFailureWasCalled); NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); emberAfClearDynamicEndpoint(0); } +void TestCommandInteraction::TestDataResponse(nlTestSuite * apSuite, void * apContext) +{ + TestClusterCommandHandler commandHandler; + TestDataResponseHelper(apSuite, apContext, &testEndpoint1, true); +} + +void TestCommandInteraction::TestDataResponseNoCommand1(nlTestSuite * apSuite, void * apContext) +{ + // Check what happens if we don't claim our command id is supported, by + // overriding the acceptedCommandList with an empty list. + TestClusterCommandHandler commandHandler; + commandHandler.OverrideAcceptedCommands(); + commandHandler.ClaimNoCommands(); + TestDataResponseHelper(apSuite, apContext, &testEndpoint1, false); +} + +void TestCommandInteraction::TestDataResponseNoCommand2(nlTestSuite * apSuite, void * apContext) +{ + // Check what happens if we don't claim our command id is supported, by + // having an acceptedCommandList that ends immediately. + TestClusterCommandHandler commandHandler; + TestDataResponseHelper(apSuite, apContext, &testEndpoint2, false); +} + +void TestCommandInteraction::TestDataResponseNoCommand3(nlTestSuite * apSuite, void * apContext) +{ + // Check what happens if we don't claim our command id is supported, by + // having an acceptedCommandList that is null. + TestClusterCommandHandler commandHandler; + TestDataResponseHelper(apSuite, apContext, &testEndpoint3, false); +} + +void TestCommandInteraction::TestDataResponseHandlerOverride1(nlTestSuite * apSuite, void * apContext) +{ + TestClusterCommandHandler commandHandler; + commandHandler.OverrideAcceptedCommands(); + TestDataResponseHelper(apSuite, apContext, &testEndpoint2, true); +} + +void TestCommandInteraction::TestDataResponseHandlerOverride2(nlTestSuite * apSuite, void * apContext) +{ + TestClusterCommandHandler commandHandler; + commandHandler.OverrideAcceptedCommands(); + TestDataResponseHelper(apSuite, apContext, &testEndpoint3, true); +} + // clang-format off const nlTest sTests[] = { NL_TEST_DEF("TestNoHandler", TestCommandInteraction::TestNoHandler), NL_TEST_DEF("TestDataResponse", TestCommandInteraction::TestDataResponse), + NL_TEST_DEF("TestDataResponseNoCommand1", TestCommandInteraction::TestDataResponseNoCommand1), + NL_TEST_DEF("TestDataResponseNoCommand2", TestCommandInteraction::TestDataResponseNoCommand2), + NL_TEST_DEF("TestDataResponseNoCommand3", TestCommandInteraction::TestDataResponseNoCommand3), + NL_TEST_DEF("TestDataResponseHandlerOverride1", TestCommandInteraction::TestDataResponseHandlerOverride1), + NL_TEST_DEF("TestDataResponseHandlerOverride2", TestCommandInteraction::TestDataResponseHandlerOverride2), NL_TEST_SENTINEL() };