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