Skip to content

Commit

Permalink
Fix StatusCode Handling for empty invoke request (project-chip#7009)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
yunhanw-google authored May 25, 2021
1 parent 1a21a3f commit 21354c3
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 19 deletions.
8 changes: 2 additions & 6 deletions src/app/CommandHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
59 changes: 46 additions & 13 deletions src/app/tests/TestCommandInteraction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,25 @@ static Messaging::ExchangeManager gExchangeManager;
static TransportMgr<Transport::UDP> 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
Expand All @@ -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,
Expand All @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 21354c3

Please sign in to comment.