Skip to content

Commit

Permalink
Fix incorrectly reported NoReponse when single batch command sent to … (
Browse files Browse the repository at this point in the history 
project-chip#32362)

* Fix incorrectly reported NoReponse when single batch command sent to legacy device

* Add tests

* Add more tests

* Restyled by clang-format

---------

Co-authored-by: Restyled.io <commits@restyled.io>
  • Loading branch information
@tehampson @restyled-commits @Sarthak-Shaha
2 people authored and Sarthak-Shaha committed Mar 5, 2024
1 parent f4d5d0d commit 7f679d3
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 17 deletions.
25 changes: 17 additions & 8 deletions src/app/CommandSender.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ namespace {
// Gets the CommandRef if available. Error returned if we expected CommandRef and it wasn't
// provided in the response.
template <typename ParserT>
CHIP_ERROR GetRef(ParserT aParser, Optional<uint16_t> & aRef, bool commandRefExpected)
CHIP_ERROR GetRef(ParserT aParser, Optional<uint16_t> & aRef, bool commandRefRequired)
{
CHIP_ERROR err = CHIP_NO_ERROR;
uint16_t ref;
Expand All @@ -40,7 +40,7 @@ CHIP_ERROR GetRef(ParserT aParser, Optional<uint16_t> & aRef, bool commandRefExp
VerifyOrReturnError(err == CHIP_NO_ERROR || err == CHIP_END_OF_TLV, err);
if (err == CHIP_END_OF_TLV)
{
if (commandRefExpected)
if (commandRefRequired)
{
return CHIP_ERROR_INVALID_ARGUMENT;
}
Expand Down Expand Up @@ -364,7 +364,7 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn
bool hasDataResponse = false;
TLV::TLVReader commandDataReader;
Optional<uint16_t> commandRef;
bool commandRefExpected = mpPendingResponseTracker && (mpPendingResponseTracker->Count() > 1);
bool commandRefRequired = (mFinishedCommandCount > 1);

CommandStatusIB::Parser commandStatus;
err = aInvokeResponse.GetStatus(&commandStatus);
Expand All @@ -379,7 +379,7 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn
StatusIB::Parser status;
commandStatus.GetErrorStatus(&status);
ReturnErrorOnFailure(status.DecodeStatusIB(statusIB));
ReturnErrorOnFailure(GetRef(commandStatus, commandRef, commandRefExpected));
ReturnErrorOnFailure(GetRef(commandStatus, commandRef, commandRefRequired));
}
else if (CHIP_END_OF_TLV == err)
{
Expand All @@ -391,7 +391,7 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn
ReturnErrorOnFailure(commandPath.GetClusterId(&clusterId));
ReturnErrorOnFailure(commandPath.GetCommandId(&commandId));
commandData.GetFields(&commandDataReader);
ReturnErrorOnFailure(GetRef(commandData, commandRef, commandRefExpected));
ReturnErrorOnFailure(GetRef(commandData, commandRef, commandRefRequired));
err = CHIP_NO_ERROR;
hasDataResponse = true;
}
Expand Down Expand Up @@ -430,7 +430,7 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn
// 2. The current InvokeResponse is for a request we never sent (based on its commandRef).
// Used when logging errors related to server violating spec.
[[maybe_unused]] ScopedNodeId remoteScopedNode;
if (mExchangeCtx.Get()->HasSessionHandle())
if (mExchangeCtx.Get() && mExchangeCtx.Get()->HasSessionHandle())
{
remoteScopedNode = mExchangeCtx.Get()->GetSessionHandle()->GetPeer();
}
Expand All @@ -441,6 +441,15 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn
}
}

if (!commandRef.HasValue() && !commandRefRequired && mpPendingResponseTracker != nullptr &&
mpPendingResponseTracker->Count() == 1)
{
// We have sent out a single invoke request. As per spec, server in this case doesn't need to provide the CommandRef
// in the response. This is allowed to support communicating with a legacy server. In this case we assume the response
// is associated with the only command we sent out.
commandRef = mpPendingResponseTracker->PopPendingResponse();
}

// When using ExtendableCallbacks, we are adhering to a different API contract where path
// specific errors are sent to the OnResponse callback. For more information on the history
// of this issue please see https://github.com/project-chip/connectedhomeip/issues/30991
Expand Down Expand Up @@ -490,8 +499,7 @@ CHIP_ERROR CommandSender::PrepareCommand(const CommandPathParams & aCommandPathP

if (mpPendingResponseTracker != nullptr)
{
size_t pendingCount = mpPendingResponseTracker->Count();
VerifyOrReturnError(pendingCount < mRemoteMaxPathsPerInvoke, CHIP_ERROR_MAXIMUM_PATHS_PER_INVOKE_EXCEEDED);
VerifyOrReturnError(mFinishedCommandCount < mRemoteMaxPathsPerInvoke, CHIP_ERROR_MAXIMUM_PATHS_PER_INVOKE_EXCEEDED);
}

if (mBatchCommandsEnabled)
Expand Down Expand Up @@ -553,6 +561,7 @@ CHIP_ERROR CommandSender::FinishCommandInternal(FinishCommandParameters & aFinis
ReturnErrorOnFailure(commandData.EndOfCommandDataIB());

MoveToState(State::AddedCommand);
mFinishedCommandCount++;

if (mpPendingResponseTracker && aFinishCommandParams.commandRef.HasValue())
{
Expand Down
1 change: 1 addition & 0 deletions src/app/CommandSender.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,7 @@ class CommandSender final : public Messaging::ExchangeDelegate
PendingResponseTracker * mpPendingResponseTracker = nullptr;

uint16_t mInvokeResponseMessageCount = 0;
uint16_t mFinishedCommandCount = 0;
uint16_t mRemoteMaxPathsPerInvoke = 1;

State mState = State::Idle;
Expand Down
101 changes: 92 additions & 9 deletions src/app/tests/TestCommandInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,11 @@ class MockCommandSenderExtendableCallback : public CommandSender::ExtendableCall
aResponseData.path.mClusterId, aResponseData.path.mCommandId, aResponseData.path.mEndpointId);
onResponseCalledTimes++;
}
void OnNoResponse(CommandSender * commandSender, const CommandSender::NoResponseData & aNoResponseData) override
{
ChipLogError(Controller, "NoResponse received for command associated with CommandRef %u", aNoResponseData.commandRef);
onNoResponseCalledTimes++;
}
void OnError(const CommandSender * apCommandSender, const CommandSender::ErrorData & aErrorData) override
{
ChipLogError(Controller, "OnError happens with %" CHIP_ERROR_FORMAT, aErrorData.error.Format());
Expand All @@ -266,15 +271,17 @@ class MockCommandSenderExtendableCallback : public CommandSender::ExtendableCall

void ResetCounter()
{
onResponseCalledTimes = 0;
onErrorCalledTimes = 0;
onFinalCalledTimes = 0;
onResponseCalledTimes = 0;
onNoResponseCalledTimes = 0;
onErrorCalledTimes = 0;
onFinalCalledTimes = 0;
}

int onResponseCalledTimes = 0;
int onErrorCalledTimes = 0;
int onFinalCalledTimes = 0;
CHIP_ERROR mError = CHIP_NO_ERROR;
int onResponseCalledTimes = 0;
int onNoResponseCalledTimes = 0;
int onErrorCalledTimes = 0;
int onFinalCalledTimes = 0;
CHIP_ERROR mError = CHIP_NO_ERROR;
} mockCommandSenderExtendedDelegate;

class MockCommandHandlerCallback : public CommandHandler::Callback
Expand Down Expand Up @@ -303,6 +310,9 @@ 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 TestCommandSenderExtendableApiWithProcessReceivedMsg(nlTestSuite * apSuite, void * apContext);
static void TestCommandSenderExtendableApiWithProcessReceivedMsgContainingInvalidCommandRef(nlTestSuite * apSuite,
void * apContext);
static void TestCommandHandlerWithProcessReceivedNotExistCommand(nlTestSuite * apSuite, void * apContext);
static void TestCommandHandlerEncodeSimpleCommandData(nlTestSuite * apSuite, void * apContext);
static void TestCommandHandlerCommandDataEncoding(nlTestSuite * apSuite, void * apContext);
Expand Down Expand Up @@ -377,7 +387,7 @@ class TestCommandInteraction
// payload will be included. Otherwise no payload will be included.
static void GenerateInvokeResponse(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload,
CommandId aCommandId, ClusterId aClusterId = kTestClusterId,
EndpointId aEndpointId = kTestEndpointId);
EndpointId aEndpointId = kTestEndpointId, Optional<uint16_t> aCommandRef = NullOptional);
static void AddInvokeRequestData(nlTestSuite * apSuite, void * apContext, CommandSender * apCommandSender,
CommandId aCommandId = kTestCommandIdWithData);
static void AddInvalidInvokeRequestData(nlTestSuite * apSuite, void * apContext, CommandSender * apCommandSender,
Expand Down Expand Up @@ -463,7 +473,8 @@ void TestCommandInteraction::GenerateInvokeRequest(nlTestSuite * apSuite, void *
}

void TestCommandInteraction::GenerateInvokeResponse(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload,
CommandId aCommandId, ClusterId aClusterId, EndpointId aEndpointId)
CommandId aCommandId, ClusterId aClusterId, EndpointId aEndpointId,
Optional<uint16_t> aCommandRef)

{
CHIP_ERROR err = CHIP_NO_ERROR;
Expand Down Expand Up @@ -505,6 +516,11 @@ void TestCommandInteraction::GenerateInvokeResponse(nlTestSuite * apSuite, void
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
}

if (aCommandRef.HasValue())
{
NL_TEST_ASSERT(apSuite, commandDataIBBuilder.Ref(aCommandRef.Value()) == CHIP_NO_ERROR);
}

commandDataIBBuilder.EndOfCommandDataIB();
NL_TEST_ASSERT(apSuite, commandDataIBBuilder.GetError() == CHIP_NO_ERROR);

Expand Down Expand Up @@ -750,6 +766,71 @@ void TestCommandInteraction::TestCommandSenderWithProcessReceivedMsg(nlTestSuite
NL_TEST_ASSERT(apSuite, moreChunkedMessages == false);
}

void TestCommandInteraction::TestCommandSenderExtendableApiWithProcessReceivedMsg(nlTestSuite * apSuite, void * apContext)
{
TestContext & ctx = *static_cast<TestContext *>(apContext);
CHIP_ERROR err = CHIP_NO_ERROR;

mockCommandSenderExtendedDelegate.ResetCounter();
PendingResponseTrackerImpl pendingResponseTracker;
app::CommandSender commandSender(kCommandSenderTestOnlyMarker, &mockCommandSenderExtendedDelegate, &ctx.GetExchangeManager(),
&pendingResponseTracker);

uint16_t mockCommandRef = 1;
pendingResponseTracker.Add(mockCommandRef);
commandSender.mFinishedCommandCount = 1;

System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);

GenerateInvokeResponse(apSuite, apContext, buf, kTestCommandIdWithData);
bool moreChunkedMessages = false;
err = commandSender.ProcessInvokeResponse(std::move(buf), moreChunkedMessages);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
NL_TEST_ASSERT(apSuite, moreChunkedMessages == false);

commandSender.FlushNoCommandResponse();

NL_TEST_ASSERT(apSuite,
mockCommandSenderExtendedDelegate.onResponseCalledTimes == 1 &&
mockCommandSenderExtendedDelegate.onFinalCalledTimes == 0 &&
mockCommandSenderExtendedDelegate.onNoResponseCalledTimes == 0 &&
mockCommandSenderExtendedDelegate.onErrorCalledTimes == 0);
}

void TestCommandInteraction::TestCommandSenderExtendableApiWithProcessReceivedMsgContainingInvalidCommandRef(nlTestSuite * apSuite,
void * apContext)
{
TestContext & ctx = *static_cast<TestContext *>(apContext);
CHIP_ERROR err = CHIP_NO_ERROR;

mockCommandSenderExtendedDelegate.ResetCounter();
PendingResponseTrackerImpl pendingResponseTracker;
app::CommandSender commandSender(kCommandSenderTestOnlyMarker, &mockCommandSenderExtendedDelegate, &ctx.GetExchangeManager(),
&pendingResponseTracker);

uint16_t mockCommandRef = 1;
pendingResponseTracker.Add(mockCommandRef);
commandSender.mFinishedCommandCount = 1;

System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);

uint16_t invalidResponseCommandRef = 2;
GenerateInvokeResponse(apSuite, apContext, buf, kTestCommandIdWithData, kTestClusterId, kTestEndpointId,
MakeOptional(invalidResponseCommandRef));
bool moreChunkedMessages = false;
err = commandSender.ProcessInvokeResponse(std::move(buf), moreChunkedMessages);
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_KEY_NOT_FOUND);
NL_TEST_ASSERT(apSuite, moreChunkedMessages == false);

commandSender.FlushNoCommandResponse();

NL_TEST_ASSERT(apSuite,
mockCommandSenderExtendedDelegate.onResponseCalledTimes == 0 &&
mockCommandSenderExtendedDelegate.onFinalCalledTimes == 0 &&
mockCommandSenderExtendedDelegate.onNoResponseCalledTimes == 1 &&
mockCommandSenderExtendedDelegate.onErrorCalledTimes == 0);
}

void TestCommandInteraction::ValidateCommandHandlerEncodeInvokeResponseMessage(nlTestSuite * apSuite, void * apContext,
bool aNeedStatusCode)
{
Expand Down Expand Up @@ -1961,6 +2042,8 @@ const nlTest sTests[] =
NL_TEST_DEF("TestCommandSenderWithSendCommand", chip::app::TestCommandInteraction::TestCommandSenderWithSendCommand),
NL_TEST_DEF("TestCommandHandlerWithSendEmptyCommand", chip::app::TestCommandInteraction::TestCommandHandlerWithSendEmptyCommand),
NL_TEST_DEF("TestCommandSenderWithProcessReceivedMsg", chip::app::TestCommandInteraction::TestCommandSenderWithProcessReceivedMsg),
NL_TEST_DEF("TestCommandSenderExtendableApiWithProcessReceivedMsg", chip::app::TestCommandInteraction::TestCommandSenderExtendableApiWithProcessReceivedMsg),
NL_TEST_DEF("TestCommandSenderExtendableApiWithProcessReceivedMsgContainingInvalidCommandRef", chip::app::TestCommandInteraction::TestCommandSenderExtendableApiWithProcessReceivedMsgContainingInvalidCommandRef),
NL_TEST_DEF("TestCommandHandlerEncodeSimpleCommandData", chip::app::TestCommandInteraction::TestCommandHandlerEncodeSimpleCommandData),
NL_TEST_DEF("TestCommandHandlerCommandDataEncoding", chip::app::TestCommandInteraction::TestCommandHandlerCommandDataEncoding),
NL_TEST_DEF("TestCommandHandlerCommandEncodeFailure", chip::app::TestCommandInteraction::TestCommandHandlerCommandEncodeFailure),
Expand Down

0 comments on commit 7f679d3

Please sign in to comment.