From 438992599b85da43565cae55a088ee53297cac06 Mon Sep 17 00:00:00 2001 From: Song Guo Date: Tue, 25 May 2021 23:50:22 +0800 Subject: [PATCH] [im] Fix error code cases in IM[TE3] (#7002) * [im] Return error when some cluster is not enabled on endpoint The current code may return wrong success or unexpected error code when one cluster is not enabled on endpoint, this commit added a function thay will query cluster from ember cluster catalog, the function will be replaced by cluster catalog. * Add tests * Add UT * Address comments * Address comments * Address comments * Update comments * Fix test --- src/app/CommandHandler.cpp | 22 +++-- src/app/CommandSender.cpp | 1 + src/app/InteractionModelEngine.h | 12 +++ src/app/tests/TestCommandInteraction.cpp | 43 ++++++++- .../tests/integration/chip_im_initiator.cpp | 93 +++++++++++++++++-- .../tests/integration/chip_im_responder.cpp | 7 ++ .../util/ember-compatibility-functions.cpp | 22 +++-- 7 files changed, 179 insertions(+), 21 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 0b40dc20e4eb88..4913ed423347f1 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -92,6 +92,8 @@ CHIP_ERROR CommandHandler::ProcessCommandDataElement(CommandDataElement::Parser err = commandPath.GetEndpointId(&endpointId); SuccessOrExit(err); + VerifyOrExit(ServerClusterCommandExists(clusterId, commandId, endpointId), err = CHIP_ERROR_INVALID_PROFILE_ID); + err = aCommandElement.GetData(&commandDataReader); if (CHIP_END_OF_TLV == err) { @@ -106,13 +108,21 @@ CHIP_ERROR CommandHandler::ProcessCommandDataElement(CommandDataElement::Parser exit: if (err != CHIP_NO_ERROR) { - // The Path is not present when there is an error to be conveyed back. ResponseCommandElement would only have status code, - // set the error with CHIP_NO_ERROR, then continue to process rest of commands - AddStatusCode(nullptr, GeneralStatusCode::kInvalidArgument, Protocols::SecureChannel::Id, - Protocols::SecureChannel::kProtocolCodeGeneralFailure); - err = CHIP_NO_ERROR; + chip::app::CommandPathParams returnStatusParam = { endpointId, + 0, // GroupId + clusterId, commandId, (chip::app::CommandPathFlags::kEndpointIdValid) }; + + // The Path is the path in the request if there are any error occurred before we dispatch the command to clusters. + // Currently, it could be failed to decode Path or failed to find cluster / command on desired endpoint. + // TODO: The behavior when receiving a malformed message is not clear in the Spec. (Spec#3259) + // TODO: The error code should be updated after #7072 added error codes required by IM. + AddStatusCode(&returnStatusParam, + err == CHIP_ERROR_INVALID_PROFILE_ID ? GeneralStatusCode::kNotFound : GeneralStatusCode::kInvalidArgument, + Protocols::SecureChannel::Id, Protocols::SecureChannel::kProtocolCodeGeneralFailure); } - return err; + // We have handled the error status above and put the error status in response, now return success status so we can process + // other commands in the invoke request. + return CHIP_NO_ERROR; } CHIP_ERROR CommandHandler::AddStatusCode(const CommandPathParams * apCommandPathParams, diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 2ed68d2ac31658..235ce294b1033a 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -163,6 +163,7 @@ CHIP_ERROR CommandSender::ProcessCommandDataElement(CommandDataElement::Parser & } else if (CHIP_END_OF_TLV == err) { + // TODO(Spec#3258): The endpoint id in response command is not clear, so we cannot do "ClientClusterCommandExists" check. err = aCommandElement.GetData(&commandDataReader); SuccessOrExit(err); // TODO(#4503): Should call callbacks of cluster that sends the command. diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 95b2cde836bcd0..2ed9e4967ce0a1 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -164,6 +164,18 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId, chip::TLV::TLVReader & aReader, Command * apCommandObj); + +/** + * Check whether the given cluster exists on the given endpoint and supports the given command. + * TODO: The implementation lives in ember-compatibility-functions.cpp, this should be replaced by IM command catalog look up + * function after we have a cluster catalog in interaction model engine. + * TODO: The endpoint id on response command (client side command) is unclear, so we don't have a ClientClusterCommandExists + * function. (Spec#3258) + * + * @retval True if the endpoint contains the server side of the given cluster and that cluster implements the given command, false + * otherwise. + */ +bool ServerClusterCommandExists(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId); CHIP_ERROR ReadSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVWriter & aWriter); CHIP_ERROR WriteSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader); } // namespace app diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 8f396169c3c2b6..5cbb314538d3cb 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -54,6 +54,12 @@ static secure_channel::MessageCounterManager gMessageCounterManager; static Transport::AdminId gAdminId = 0; static bool isCommandDispatched = false; +namespace { +constexpr EndpointId kTestEndpointId = 1; +constexpr ClusterId kTestClusterId = 3; +constexpr CommandId kTestCommandId = 4; +} // namespace + namespace app { void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId, @@ -64,6 +70,7 @@ void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aC aClusterId, // ClusterId aCommandId, // CommandId (chip::app::CommandPathFlags::kEndpointIdValid) }; + ChipLogDetail(Controller, "Received Cluster Command: Cluster=%" PRIx16 " Command=%" PRIx8 " Endpoint=%" PRIx8, aClusterId, aCommandId, aEndPointId); @@ -73,6 +80,12 @@ void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aC chip::isCommandDispatched = true; } +bool ServerClusterCommandExists(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId) +{ + // Mock cluster catalog, only support one command on one cluster on one endpoint. + return (aEndPointId == kTestEndpointId && aClusterId == kTestClusterId && aCommandId == kTestCommandId); +} + class TestCommandInteraction { public: @@ -81,6 +94,7 @@ class TestCommandInteraction static void TestCommandSenderWithSendCommand(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerWithSendEmptyCommand(nlTestSuite * apSuite, void * apContext); static void TestCommandSenderWithProcessReceivedMsg(nlTestSuite * apSuite, void * apContext); + static void TestCommandHandlerWithProcessReceivedNotExistCommand(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerWithSendSimpleCommandData(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerWithSendSimpleStatusCode(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerWithSendEmptyResponse(nlTestSuite * apSuite, void * apContext); @@ -89,7 +103,8 @@ class TestCommandInteraction private: static void GenerateReceivedCommand(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload, - bool aNeedCommandData); + bool aNeedCommandData, EndpointId aEndpointId = kTestEndpointId, + ClusterId aClusterId = kTestClusterId, CommandId aCommandId = kTestCommandId); static void AddCommandDataElement(nlTestSuite * apSuite, void * apContext, Command * apCommand, bool aNeedStatusCode, bool aIsEmptyResponse); static void ValidateCommandHandlerWithSendCommand(nlTestSuite * apSuite, void * apContext, bool aNeedStatusCode, @@ -106,7 +121,9 @@ class TestExchangeDelegate : public Messaging::ExchangeDelegate }; void TestCommandInteraction::GenerateReceivedCommand(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload, - bool aNeedCommandData) + bool aNeedCommandData, EndpointId aEndpointId, ClusterId aClusterId, + CommandId aCommandId) + { CHIP_ERROR err = CHIP_NO_ERROR; InvokeCommand::Builder invokeCommandBuilder; @@ -123,7 +140,7 @@ void TestCommandInteraction::GenerateReceivedCommand(nlTestSuite * apSuite, void NL_TEST_ASSERT(apSuite, commandList.GetError() == CHIP_NO_ERROR); CommandPath::Builder commandPathBuilder = commandDataElementBuilder.CreateCommandPathBuilder(); NL_TEST_ASSERT(apSuite, commandDataElementBuilder.GetError() == CHIP_NO_ERROR); - commandPathBuilder.EndpointId(1).ClusterId(3).CommandId(4).EndOfCommandPath(); + commandPathBuilder.EndpointId(aEndpointId).ClusterId(aClusterId).CommandId(aCommandId).EndOfCommandPath(); NL_TEST_ASSERT(apSuite, commandPathBuilder.GetError() == CHIP_NO_ERROR); if (aNeedCommandData) @@ -361,6 +378,25 @@ void TestCommandInteraction::TestCommandHandlerWithProcessReceivedMsg(nlTestSuit commandHandler.Shutdown(); } +void TestCommandInteraction::TestCommandHandlerWithProcessReceivedNotExistCommand(nlTestSuite * apSuite, void * apContext) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + app::CommandHandler commandHandler; + System::PacketBufferHandle commandDatabuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); + err = commandHandler.Init(&chip::gExchangeManager, nullptr); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + // Use some invalid endpoint / cluster / command. + GenerateReceivedCommand(apSuite, apContext, commandDatabuf, false /*aNeedCommandData*/, 0xDE /* endpoint */, + 0xADBE /* cluster */, 0xEF /* command */); + + // TODO: Need to find a way to get the response instead of only check if a function on key path is called. + // We should not reach CommandDispatch if requested command does not exist. + chip::isCommandDispatched = false; + err = commandHandler.ProcessCommandMessage(std::move(commandDatabuf), Command::CommandRoleId::HandlerId); + NL_TEST_ASSERT(apSuite, !chip::isCommandDispatched); + commandHandler.Shutdown(); +} + void TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg(nlTestSuite * apSuite, void * apContext) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -420,6 +456,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestCommandHandlerWithSendSimpleStatusCode", chip::app::TestCommandInteraction::TestCommandHandlerWithSendSimpleStatusCode), NL_TEST_DEF("TestCommandHandlerWithSendEmptyResponse", chip::app::TestCommandInteraction::TestCommandHandlerWithSendEmptyResponse), NL_TEST_DEF("TestCommandHandlerWithProcessReceivedMsg", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedMsg), + NL_TEST_DEF("TestCommandHandlerWithProcessReceivedNotExistCommand", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedNotExistCommand), NL_TEST_DEF("TestCommandHandlerWithProcessReceivedEmptyDataMsg", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg), NL_TEST_SENTINEL() }; diff --git a/src/app/tests/integration/chip_im_initiator.cpp b/src/app/tests/integration/chip_im_initiator.cpp index 09bceacca88384..5b9fffc3f3d24e 100644 --- a/src/app/tests/integration/chip_im_initiator.cpp +++ b/src/app/tests/integration/chip_im_initiator.cpp @@ -45,10 +45,11 @@ namespace { // Max value for the number of message request sent. -constexpr size_t kMaxCommandMessageCount = 3; -constexpr size_t kMaxReadMessageCount = 3; -constexpr int32_t gMessageIntervalSeconds = 1; -constexpr chip::Transport::AdminId gAdminId = 0; +constexpr size_t kMaxCommandMessageCount = 3; +constexpr size_t kTotalFailureCommandMessageCount = 1; +constexpr size_t kMaxReadMessageCount = 3; +constexpr int32_t gMessageIntervalSeconds = 1; +constexpr chip::Transport::AdminId gAdminId = 0; // The ReadClient object. chip::app::ReadClient * gpReadClient = nullptr; @@ -74,6 +75,16 @@ uint64_t gReadCount = 0; // Count of the number of CommandResponses received. uint64_t gReadRespCount = 0; +// Whether the last command successed. +enum class TestCommandResult : uint8_t +{ + kUndefined, + kSuccess, + kFailure +}; + +TestCommandResult gLastCommandResult = TestCommandResult::kUndefined; + std::condition_variable gCond; CHIP_ERROR SendCommandRequest(chip::app::CommandSender * commandSender) @@ -126,6 +137,43 @@ CHIP_ERROR SendCommandRequest(chip::app::CommandSender * commandSender) return err; } +CHIP_ERROR SendBadCommandRequest(chip::app::CommandSender * commandSender) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + + VerifyOrReturnError(commandSender != nullptr, CHIP_ERROR_INCORRECT_STATE); + + gLastMessageTime = chip::System::Timer::GetCurrentEpoch(); + + printf("\nSend invoke command request message to Node: %" PRIu64 "\n", chip::kTestDeviceNodeId); + + chip::app::CommandPathParams commandPathParams = { 0xDE, // Bad Endpoint + 0xADBE, // Bad GroupId + 0xEFCA, // Bad ClusterId + 0xFE, // Bad CommandId + chip::app::CommandPathFlags::kEndpointIdValid }; + + err = commandSender->PrepareCommand(&commandPathParams); + SuccessOrExit(err); + + err = commandSender->FinishCommand(); + SuccessOrExit(err); + + err = commandSender->SendCommandRequest(chip::kTestDeviceNodeId, gAdminId); + SuccessOrExit(err); + + if (err == CHIP_NO_ERROR) + { + gCommandCount++; + } + else + { + printf("Send invoke command request failed, err: %s\n", chip::ErrorStr(err)); + } +exit: + return err; +} + CHIP_ERROR SendReadRequest(void) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -220,6 +268,9 @@ class MockInteractionModelApp : public chip::app::InteractionModelDelegate printf("CommandResponseStatus with GeneralCode %d, ProtocolId %d, ProtocolCode %d, EndpointId %d, ClusterId %d, CommandId " "%d, CommandIndex %d", static_cast(aGeneralCode), aProtocolId, aProtocolCode, aEndpointId, aClusterId, aCommandId, aCommandIndex); + gLastCommandResult = (aGeneralCode == chip::Protocols::SecureChannel::GeneralStatusCode::kSuccess && aProtocolCode == 0) + ? TestCommandResult::kSuccess + : TestCommandResult::kFailure; return CHIP_NO_ERROR; } @@ -255,6 +306,12 @@ class MockInteractionModelApp : public chip::app::InteractionModelDelegate namespace chip { namespace app { +bool ServerClusterCommandExists(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId) +{ + // Always return true in test. + return true; +} + void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId, chip::TLV::TLVReader & aReader, Command * apCommandObj) { @@ -267,6 +324,8 @@ void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aC { chip::TLV::Debug::Dump(aReader, TLVPrettyPrinter); } + + gLastCommandResult = TestCommandResult::kSuccess; } CHIP_ERROR WriteSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader) @@ -347,13 +406,35 @@ int main(int argc, char * argv[]) if (err != CHIP_NO_ERROR) { printf("Send command request failed: %s\n", chip::ErrorStr(err)); - goto exit; + ExitNow(); } if (gCond.wait_for(lock, std::chrono::seconds(gMessageIntervalSeconds)) == std::cv_status::timeout) { printf("Invoke Command: No response received\n"); } + + VerifyOrExit(gLastCommandResult == TestCommandResult::kSuccess, err = CHIP_ERROR_INCORRECT_STATE); + } + + // Test with invalid endpoint / cluster / command combination. + { + chip::app::CommandSender * commandSender; + err = chip::app::InteractionModelEngine::GetInstance()->NewCommandSender(&commandSender); + SuccessOrExit(err); + err = SendBadCommandRequest(commandSender); + if (err != CHIP_NO_ERROR) + { + printf("Send command request failed: %s\n", chip::ErrorStr(err)); + ExitNow(); + } + + if (gCond.wait_for(lock, std::chrono::seconds(gMessageIntervalSeconds)) == std::cv_status::timeout) + { + printf("Invoke Command: No response received\n"); + } + + VerifyOrExit(gLastCommandResult == TestCommandResult::kFailure, err = CHIP_ERROR_INCORRECT_STATE); } // Connection has been established. Now send the ReadRequests. @@ -377,7 +458,7 @@ int main(int argc, char * argv[]) ShutdownChip(); exit: - if (err != CHIP_NO_ERROR || (gCommandRespCount != kMaxCommandMessageCount)) + if (err != CHIP_NO_ERROR || (gCommandRespCount != kMaxCommandMessageCount + kTotalFailureCommandMessageCount)) { printf("ChipCommandSender failed: %s\n", chip::ErrorStr(err)); exit(EXIT_FAILURE); diff --git a/src/app/tests/integration/chip_im_responder.cpp b/src/app/tests/integration/chip_im_responder.cpp index a357d41228221c..2be6b9b5daf95b 100644 --- a/src/app/tests/integration/chip_im_responder.cpp +++ b/src/app/tests/integration/chip_im_responder.cpp @@ -44,6 +44,13 @@ namespace chip { namespace app { + +bool ServerClusterCommandExists(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId) +{ + // The Mock cluster catalog -- only have one command on one cluster on one endpoint. + return (aEndPointId == kTestEndpointId && aClusterId == kTestClusterId && aCommandId == kTestCommandId); +} + void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId, chip::TLV::TLVReader & aReader, Command * apCommandObj) { diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp index f2c3fc8ee6e4fb..f55c66fe0b5978 100644 --- a/src/app/util/ember-compatibility-functions.cpp +++ b/src/app/util/ember-compatibility-functions.cpp @@ -21,9 +21,9 @@ * when calling ember callbacks. */ -#include - #include +#include +#include #include #include #include @@ -65,15 +65,17 @@ bool IMEmberAfSendDefaultResponseWithCallback(EmberAfStatus status) return false; } - chip::app::CommandPathParams returnStatusParam = { imCompatibilityEmberApsFrame.sourceEndpoint, + chip::app::CommandPathParams returnStatusParam = { imCompatibilityEmberApsFrame.destinationEndpoint, 0, // GroupId imCompatibilityEmberApsFrame.clusterId, imCompatibilityEmberAfCluster.commandId, (chip::app::CommandPathFlags::kEndpointIdValid) }; - CHIP_ERROR err = - currentCommandObject->AddStatusCode(&returnStatusParam, chip::Protocols::SecureChannel::GeneralStatusCode::kSuccess, - chip::Protocols::InteractionModel::Id, status); + CHIP_ERROR err = currentCommandObject->AddStatusCode(&returnStatusParam, + status == EMBER_ZCL_STATUS_SUCCESS + ? chip::Protocols::SecureChannel::GeneralStatusCode::kSuccess + : chip::Protocols::SecureChannel::GeneralStatusCode::kFailure, + chip::Protocols::InteractionModel::Id, status); return CHIP_NO_ERROR == err; } @@ -84,5 +86,13 @@ void ResetEmberAfObjects() } } // namespace Compatibility + +bool ServerClusterCommandExists(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId) +{ + // TODO: Currently, we are using cluster catalog from the ember library, this should be modified or replaced after several + // updates to Commands. + return emberAfContainsServer(aEndPointId, aClusterId); +} + } // namespace app } // namespace chip