From 329143234f15e2f609ff094cc4a5fe74eb60b779 Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Mon, 24 May 2021 17:08:12 -0700 Subject: [PATCH] Fix StatusCode Handling for empty invoke request (#7009) Problems: When receiving empty invoke request (it has path, no data inside), Status Code needs to be handled by SDK Consumer, instead of setting success status code. Summary of Change: -- If this is empty command data element in invoke request, skip the CHIP_END_OF_TLV error, the sdk consumer would handle this status code further. -- Add new unit test. Test: -- Add new unit test to handle empty command data in invoke request message. --- src/app/CommandHandler.cpp | 8 +--- src/app/tests/TestCommandInteraction.cpp | 59 ++++++++++++++++++------ 2 files changed, 48 insertions(+), 19 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index a74d8530597a8f..0b40dc20e4eb88 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -96,13 +96,9 @@ CHIP_ERROR CommandHandler::ProcessCommandDataElement(CommandDataElement::Parser if (CHIP_END_OF_TLV == err) { err = CHIP_NO_ERROR; - ChipLogDetail(DataManagement, "Add Status code for empty command, cluster Id is %d", clusterId); - // The Path is not present when the CommandDataElement is used with an empty response, ResponseCommandElement would only - // have status code, - AddStatusCode(nullptr, GeneralStatusCode::kSuccess, Protocols::SecureChannel::Id, - Protocols::SecureChannel::kProtocolCodeSuccess); + ChipLogDetail(DataManagement, "Received command without data for cluster %d", clusterId); } - else if (CHIP_NO_ERROR == err) + if (CHIP_NO_ERROR == err) { DispatchSingleClusterCommand(clusterId, commandId, endpointId, commandDataReader, this); } diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 310024af22fa3f..8f396169c3c2b6 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -52,14 +52,25 @@ static Messaging::ExchangeManager gExchangeManager; static TransportMgr gTransportManager; static secure_channel::MessageCounterManager gMessageCounterManager; static Transport::AdminId gAdminId = 0; +static bool isCommandDispatched = false; namespace app { void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId, chip::TLV::TLVReader & aReader, Command * apCommandObj) { + chip::app::CommandPathParams commandPathParams = { aEndPointId, // Endpoint + 0, // GroupId + aClusterId, // ClusterId + aCommandId, // CommandId + (chip::app::CommandPathFlags::kEndpointIdValid) }; ChipLogDetail(Controller, "Received Cluster Command: Cluster=%" PRIx16 " Command=%" PRIx8 " Endpoint=%" PRIx8, aClusterId, aCommandId, aEndPointId); + + apCommandObj->AddStatusCode(&commandPathParams, Protocols::SecureChannel::GeneralStatusCode::kSuccess, + Protocols::SecureChannel::Id, Protocols::SecureChannel::kProtocolCodeSuccess); + + chip::isCommandDispatched = true; } class TestCommandInteraction @@ -74,9 +85,11 @@ class TestCommandInteraction static void TestCommandHandlerWithSendSimpleStatusCode(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerWithSendEmptyResponse(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerWithProcessReceivedMsg(nlTestSuite * apSuite, void * apContext); + static void TestCommandHandlerWithProcessReceivedEmptyDataMsg(nlTestSuite * apSuite, void * apContext); private: - static void GenerateReceivedCommand(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload); + static void GenerateReceivedCommand(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload, + bool aNeedCommandData); static void AddCommandDataElement(nlTestSuite * apSuite, void * apContext, Command * apCommand, bool aNeedStatusCode, bool aIsEmptyResponse); static void ValidateCommandHandlerWithSendCommand(nlTestSuite * apSuite, void * apContext, bool aNeedStatusCode, @@ -92,7 +105,8 @@ class TestExchangeDelegate : public Messaging::ExchangeDelegate void OnResponseTimeout(Messaging::ExchangeContext * ec) override {} }; -void TestCommandInteraction::GenerateReceivedCommand(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload) +void TestCommandInteraction::GenerateReceivedCommand(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload, + bool aNeedCommandData) { CHIP_ERROR err = CHIP_NO_ERROR; InvokeCommand::Builder invokeCommandBuilder; @@ -112,16 +126,20 @@ void TestCommandInteraction::GenerateReceivedCommand(nlTestSuite * apSuite, void commandPathBuilder.EndpointId(1).ClusterId(3).CommandId(4).EndOfCommandPath(); NL_TEST_ASSERT(apSuite, commandPathBuilder.GetError() == CHIP_NO_ERROR); - chip::TLV::TLVWriter * pWriter = commandDataElementBuilder.GetWriter(); - chip::TLV::TLVType dummyType = chip::TLV::kTLVType_NotSpecified; - err = pWriter->StartContainer(chip::TLV::ContextTag(CommandDataElement::kCsTag_Data), chip::TLV::kTLVType_Structure, dummyType); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + if (aNeedCommandData) + { + chip::TLV::TLVWriter * pWriter = commandDataElementBuilder.GetWriter(); + chip::TLV::TLVType dummyType = chip::TLV::kTLVType_NotSpecified; + err = pWriter->StartContainer(chip::TLV::ContextTag(CommandDataElement::kCsTag_Data), chip::TLV::kTLVType_Structure, + dummyType); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - err = pWriter->PutBoolean(chip::TLV::ContextTag(1), true); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + err = pWriter->PutBoolean(chip::TLV::ContextTag(1), true); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - err = pWriter->EndContainer(dummyType); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + err = pWriter->EndContainer(dummyType); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + } commandDataElementBuilder.EndOfCommandDataElement(); NL_TEST_ASSERT(apSuite, commandDataElementBuilder.GetError() == CHIP_NO_ERROR); @@ -235,7 +253,7 @@ void TestCommandInteraction::TestCommandSenderWithSendCommand(nlTestSuite * apSu err = commandSender.SendCommandRequest(kTestDeviceNodeId, gAdminId); NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_NOT_CONNECTED); - GenerateReceivedCommand(apSuite, apContext, buf); + GenerateReceivedCommand(apSuite, apContext, buf, true /*aNeedCommandData*/); err = commandSender.ProcessCommandMessage(std::move(buf), Command::CommandRoleId::SenderId); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); commandSender.Shutdown(); @@ -277,7 +295,7 @@ void TestCommandInteraction::TestCommandSenderWithProcessReceivedMsg(nlTestSuite err = commandSender.Init(&gExchangeManager, nullptr); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - GenerateReceivedCommand(apSuite, apContext, buf); + GenerateReceivedCommand(apSuite, apContext, buf, true /*aNeedCommandData*/); err = commandSender.ProcessCommandMessage(std::move(buf), Command::CommandRoleId::SenderId); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); commandSender.Shutdown(); @@ -337,12 +355,26 @@ void TestCommandInteraction::TestCommandHandlerWithProcessReceivedMsg(nlTestSuit System::PacketBufferHandle commandDatabuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); err = commandHandler.Init(&chip::gExchangeManager, nullptr); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - GenerateReceivedCommand(apSuite, apContext, commandDatabuf); + GenerateReceivedCommand(apSuite, apContext, commandDatabuf, true /*aNeedCommandData*/); err = commandHandler.ProcessCommandMessage(std::move(commandDatabuf), Command::CommandRoleId::HandlerId); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); commandHandler.Shutdown(); } +void TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg(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); + chip::isCommandDispatched = false; + GenerateReceivedCommand(apSuite, apContext, commandDatabuf, false /*aNeedCommandData*/); + err = commandHandler.ProcessCommandMessage(std::move(commandDatabuf), Command::CommandRoleId::HandlerId); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR && chip::isCommandDispatched); + commandHandler.Shutdown(); +} + } // namespace app } // namespace chip @@ -388,6 +420,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("TestCommandHandlerWithProcessReceivedEmptyDataMsg", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg), NL_TEST_SENTINEL() }; // clang-format on