From 4fe0bdc0b5c14361fb0f9e958eff5291973f237d Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 22 Nov 2023 14:33:30 +0000 Subject: [PATCH] Update CommandHandler to support batch commands --- src/app/CommandHandler.cpp | 119 +++++++++++++++--- src/app/CommandHandler.h | 19 ++- src/app/CommandRefLookupTable.h | 106 ++++++++++++++++ .../network-commissioning.cpp | 4 +- src/app/tests/TestCommandInteraction.cpp | 77 +++++++----- .../tests/integration/chip_im_responder.cpp | 4 +- 6 files changed, 279 insertions(+), 50 deletions(-) create mode 100644 src/app/CommandRefLookupTable.h diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 8fd13bab7247f0..ab5e775a8c25f8 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -97,6 +98,54 @@ void CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * ec, con mGoneAsync = true; } +CHIP_ERROR CommandHandler::ValidateCommands(TLV::TLVReader & invokeRequestsReader) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + size_t commandCount = 0; + bool commandRefExpected = false; + + TLV::Utilities::Count(invokeRequestsReader, commandCount, false /* recurse */); + VerifyOrReturnError(commandCount <= CHIP_CONFIG_MAX_PATHS_PER_INVOKE, CHIP_ERROR_INVALID_ARGUMENT); + commandRefExpected = commandCount > 1; + + while (CHIP_NO_ERROR == (err = invokeRequestsReader.Next())) + { + VerifyOrReturnError(TLV::AnonymousTag() == invokeRequestsReader.GetTag(), CHIP_ERROR_INVALID_ARGUMENT); + CommandDataIB::Parser commandData; + ReturnErrorOnFailure(commandData.Init(invokeRequestsReader)); + + CommandPathIB::Parser commandPath; + ConcreteCommandPath concretePath(0, 0, 0); + + ReturnErrorOnFailure(commandData.GetPath(&commandPath)); + ReturnErrorOnFailure(commandPath.GetConcreteCommandPath(concretePath)); + + Optional commandRef; + uint16_t ref; + err = commandData.GetRef(&ref); + VerifyOrReturnError(err == CHIP_NO_ERROR || err == CHIP_END_OF_TLV, err); + if (err == CHIP_END_OF_TLV && commandRefExpected) + { + return CHIP_ERROR_INVALID_ARGUMENT; + } + if (err == CHIP_NO_ERROR) + { + commandRef = MakeOptional(ref); + } + + VerifyOrReturnError(!mCommandRefLookupTable.IsRequestedPathAndRefUnique(concretePath, commandRef), + CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorOnFailure(mCommandRefLookupTable.Add(concretePath, commandRef)); + } + + // if we have exhausted this container + if (CHIP_END_OF_TLV == err) + { + err = CHIP_NO_ERROR; + } + return err; +} + Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -116,13 +165,9 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa VerifyOrReturnError(mTimedRequest == isTimedInvoke, Status::UnsupportedAccess); invokeRequests.GetReader(&invokeRequestsReader); - { - // We don't support handling multiple commands but the protocol is ready to support it in the future, reject all of them and - // IM Engine will send a status response. - size_t commandCount = 0; - TLV::Utilities::Count(invokeRequestsReader, commandCount, false /* recurse */); - VerifyOrReturnError(commandCount == 1, Status::InvalidAction); - } + VerifyOrReturnError(ValidateCommands(invokeRequestsReader) == CHIP_NO_ERROR, Status::InvalidAction); + // Reset the reader + invokeRequests.GetReader(&invokeRequestsReader); while (CHIP_NO_ERROR == (err = invokeRequestsReader.Next())) { @@ -500,15 +545,43 @@ CHIP_ERROR CommandHandler::AddClusterSpecificFailure(const ConcreteCommandPath & return AddStatusInternal(aCommandPath, StatusIB(Status::Failure, aClusterStatus)); } +CHIP_ERROR CommandHandler::PrepareCommand(const ConcreteCommandPath & aRequestCommandPath, const ConcreteCommandPath & aCommandPath, + bool aStartDataStruct) +{ + auto * commandRefTableEntry = mCommandRefLookupTable.Find(aRequestCommandPath); + return PrepareCommand(commandRefTableEntry, aCommandPath, aStartDataStruct); +} + CHIP_ERROR CommandHandler::PrepareCommand(const ConcreteCommandPath & aCommandPath, bool aStartDataStruct) { + // Legacy code is calling the deprecated version of PrepareCommand. If we are in a case where + // there was a single command in the request, we can just assume this response is triggered by + // the single command. + auto * commandRefTableEntry = mCommandRefLookupTable.GetCommandRefTableEntryIfSingleRequest(); + + // At this point application supports Batch Invoke Commands since there does not exist a single command in table, + // but application is calling the depricated PrepareCommand. We have no way to determine the associated CommandRef + // to put into the InvokeResponse. + VerifyOrDieWithMsg(commandRefTableEntry, DataManagement, + "Seemingly device supports batch commands, but is calling the deprecated PrepareCommand API"); + return PrepareCommand(commandRefTableEntry, aCommandPath, aStartDataStruct); +} + +CHIP_ERROR CommandHandler::PrepareCommand(const CommandRefLookupTable::CommandRefTableEntry * apCommandRefTableEntry, + const ConcreteCommandPath & aCommandPath, bool aStartDataStruct) +{ + VerifyOrReturnError(apCommandRefTableEntry, CHIP_ERROR_INCORRECT_STATE); ReturnErrorOnFailure(AllocateBuffer()); mInvokeResponseBuilder.Checkpoint(mBackupWriter); + mBackupState = mState; // // We must not be in the middle of preparing a command, or having prepared or sent one. // - VerifyOrReturnError(mState == State::Idle, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mState == State::Idle || mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE); + + mRefForResponse = apCommandRefTableEntry->ref; + MoveToState(State::Preparing); InvokeResponseIBs::Builder & invokeResponses = mInvokeResponseBuilder.GetInvokeResponses(); InvokeResponseIB::Builder & invokeResponse = invokeResponses.CreateInvokeResponse(); @@ -536,10 +609,14 @@ CHIP_ERROR CommandHandler::FinishCommand(bool aStartDataStruct) { ReturnErrorOnFailure(commandData.GetWriter()->EndContainer(mDataElementContainerType)); } + + if (mRefForResponse.HasValue()) + { + ReturnErrorOnFailure(commandData.Ref(mRefForResponse.Value())); + } + ReturnErrorOnFailure(commandData.EndOfCommandDataIB()); ReturnErrorOnFailure(mInvokeResponseBuilder.GetInvokeResponses().GetInvokeResponse().EndOfInvokeResponseIB()); - ReturnErrorOnFailure(mInvokeResponseBuilder.GetInvokeResponses().EndOfInvokeResponses()); - ReturnErrorOnFailure(mInvokeResponseBuilder.EndOfInvokeResponseMessage()); MoveToState(State::AddedCommand); return CHIP_NO_ERROR; } @@ -550,7 +627,12 @@ CHIP_ERROR CommandHandler::PrepareStatus(const ConcreteCommandPath & aCommandPat // // We must not be in the middle of preparing a command, or having prepared or sent one. // - VerifyOrReturnError(mState == State::Idle, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mState == State::Idle || mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE); + + auto * commandRefTableEntry = mCommandRefLookupTable.Find(aCommandPath); + VerifyOrReturnError(commandRefTableEntry, CHIP_ERROR_INCORRECT_STATE); + mRefForResponse = commandRefTableEntry->ref; + MoveToState(State::Preparing); InvokeResponseIBs::Builder & invokeResponses = mInvokeResponseBuilder.GetInvokeResponses(); InvokeResponseIB::Builder & invokeResponse = invokeResponses.CreateInvokeResponse(); @@ -567,10 +649,15 @@ CHIP_ERROR CommandHandler::PrepareStatus(const ConcreteCommandPath & aCommandPat CHIP_ERROR CommandHandler::FinishStatus() { VerifyOrReturnError(mState == State::AddingCommand, CHIP_ERROR_INCORRECT_STATE); + + CommandStatusIB::Builder & commandStatus = mInvokeResponseBuilder.GetInvokeResponses().GetInvokeResponse().GetStatus(); + if (mRefForResponse.HasValue()) + { + ReturnErrorOnFailure(commandStatus.Ref(mRefForResponse.Value())); + } + ReturnErrorOnFailure(mInvokeResponseBuilder.GetInvokeResponses().GetInvokeResponse().GetStatus().EndOfCommandStatusIB()); ReturnErrorOnFailure(mInvokeResponseBuilder.GetInvokeResponses().GetInvokeResponse().EndOfInvokeResponseIB()); - ReturnErrorOnFailure(mInvokeResponseBuilder.GetInvokeResponses().EndOfInvokeResponses()); - ReturnErrorOnFailure(mInvokeResponseBuilder.EndOfInvokeResponseMessage()); MoveToState(State::AddedCommand); return CHIP_NO_ERROR; } @@ -579,9 +666,7 @@ CHIP_ERROR CommandHandler::RollbackResponse() { VerifyOrReturnError(mState == State::Preparing || mState == State::AddingCommand, CHIP_ERROR_INCORRECT_STATE); mInvokeResponseBuilder.Rollback(mBackupWriter); - // Note: We only support one command per request, so we reset the state to Idle here, need to review the states when adding - // supports of having multiple requests in the same transaction. - MoveToState(State::Idle); + MoveToState(mBackupState); return CHIP_NO_ERROR; } @@ -635,6 +720,8 @@ CommandHandler::Handle::Handle(CommandHandler * handle) CHIP_ERROR CommandHandler::Finalize(System::PacketBufferHandle & commandPacket) { VerifyOrReturnError(mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE); + ReturnErrorOnFailure(mInvokeResponseBuilder.GetInvokeResponses().EndOfInvokeResponses()); + ReturnErrorOnFailure(mInvokeResponseBuilder.EndOfInvokeResponseMessage()); return mCommandMessageWriter.Finalize(&commandPacket); } diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 65542ef0587f55..115c1fb3ae6b32 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -30,6 +30,8 @@ #pragma once +#include "CommandRefLookupTable.h" + #include #include #include @@ -190,7 +192,10 @@ class CommandHandler : public Messaging::ExchangeDelegate CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus); Protocols::InteractionModel::Status ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke); - CHIP_ERROR PrepareCommand(const ConcreteCommandPath & aCommandPath, bool aStartDataStruct = true); + CHIP_ERROR PrepareCommand(const ConcreteCommandPath & aRequestCommandPath, const ConcreteCommandPath & aCommandPath, + bool aStartDataStruct = true); + [[deprecated("PrepareCommand now needs the requested command path. Without it device cannot support batch invoke")]] CHIP_ERROR + PrepareCommand(const ConcreteCommandPath & aCommandPath, bool aStartDataStruct = true); CHIP_ERROR FinishCommand(bool aEndDataStruct = true); CHIP_ERROR PrepareStatus(const ConcreteCommandPath & aCommandPath); CHIP_ERROR FinishStatus(); @@ -363,6 +368,11 @@ class CommandHandler : public Messaging::ExchangeDelegate */ CHIP_ERROR AllocateBuffer(); + CHIP_ERROR ValidateCommands(TLV::TLVReader & invokeRequestsReader); + + CHIP_ERROR PrepareCommand(const CommandRefLookupTable::CommandRefTableEntry * apCommandRefTableEntry, + const ConcreteCommandPath & aCommandPath, bool aStartDataStruct); + CHIP_ERROR Finalize(System::PacketBufferHandle & commandPacket); /** @@ -398,7 +408,7 @@ class CommandHandler : public Messaging::ExchangeDelegate CHIP_ERROR TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData) { ConcreteCommandPath path = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, CommandData::GetCommandId() }; - ReturnErrorOnFailure(PrepareCommand(path, false)); + ReturnErrorOnFailure(PrepareCommand(aRequestCommandPath, path, false)); TLV::TLVWriter * writer = GetCommandDataIBTLVWriter(); VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE); ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields), aData)); @@ -428,8 +438,13 @@ class CommandHandler : public Messaging::ExchangeDelegate State mState = State::Idle; bool mGroupRequest = false; + Optional mRefForResponse; + + CommandRefLookupTable mCommandRefLookupTable; chip::System::PacketBufferTLVWriter mCommandMessageWriter; TLV::TLVWriter mBackupWriter; + State mBackupState; + bool mBufferAllocated = false; // If mGoneAsync is true, we have finished out initial processing of the // incoming invoke. After this point, our session could go away at any diff --git a/src/app/CommandRefLookupTable.h b/src/app/CommandRefLookupTable.h new file mode 100644 index 00000000000000..940751df76028e --- /dev/null +++ b/src/app/CommandRefLookupTable.h @@ -0,0 +1,106 @@ +/* + * + * Copyright (c) 2023 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include +#include + +namespace chip { +namespace app { + +/** + * @class CommandRefLookupTable + * + * @brief Allows looking up CommandRef using the requested ConcreteCommandPath. + * + * While there are faster implementations, right now batch commands are capped at a low number due to + * message size constrains. All commands need to be contained within a single InvokeRequest. In + * practice this is less than 60 commands. + * + */ +class CommandRefLookupTable +{ +public: + struct CommandRefTableEntry + { + ConcreteCommandPath requestPath = ConcreteCommandPath(0, 0, 0); + Optional ref; + }; + + /** + * IsRequestedPathAndRefUnique() is used to determine if requestPath and ref (if it has value) + * are unique and do NOT already exists in the table. This is to help validate incoming requests. + */ + bool IsRequestedPathAndRefUnique(const ConcreteCommandPath & requestPath, const Optional & ref) + { + for (int i = 0; i < mCount; i++) + { + if (mTable[i].requestPath == requestPath) + { + return false; + } + if (mTable[i].ref.HasValue() && mTable[i].ref == ref) + { + return false; + } + } + return true; + } + + const CommandRefTableEntry * Find(const ConcreteCommandPath & requestPath) + { + for (int i = 0; i < mCount; i++) + { + if (mTable[i].requestPath == requestPath) + { + return &mTable[i]; + } + } + return nullptr; + } + + CommandRefTableEntry * GetCommandRefTableEntryIfSingleRequest() + { + if (mCount == 1) + { + return &mTable[0]; + } + return nullptr; + } + + CHIP_ERROR Add(const ConcreteCommandPath & requestPath, const Optional & ref) + { + if (mCount > CHIP_CONFIG_MAX_PATHS_PER_INVOKE) + { + return CHIP_ERROR_NO_MEMORY; + } + + mTable[mCount] = CommandRefTableEntry{ requestPath, ref }; + mCount++; + return CHIP_NO_ERROR; + } + +private: + uint16_t mCount = 0; + CommandRefTableEntry mTable[CHIP_CONFIG_MAX_PATHS_PER_INVOKE]; +}; + +} // namespace app +} // namespace chip diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp index 471b6038b85b09..5ea7435ff6db7e 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.cpp +++ b/src/app/clusters/network-commissioning/network-commissioning.cpp @@ -816,7 +816,7 @@ void Instance::OnFinished(Status status, CharSpan debugText, ThreadScanResponseI uint8_t extendedAddressBuffer[Thread::kSizeExtendedPanId]; SuccessOrExit(err = commandHandle->PrepareCommand( - ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id))); + mPath, ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id))); VerifyOrExit((writer = commandHandle->GetCommandDataIBTLVWriter()) != nullptr, err = CHIP_ERROR_INCORRECT_STATE); SuccessOrExit(err = writer->Put(TLV::ContextTag(Commands::ScanNetworksResponse::Fields::kNetworkingStatus), status)); @@ -928,7 +928,7 @@ void Instance::OnFinished(Status status, CharSpan debugText, WiFiScanResponseIte size_t networksEncoded = 0; SuccessOrExit(err = commandHandle->PrepareCommand( - ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id))); + mPath, ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id))); VerifyOrExit((writer = commandHandle->GetCommandDataIBTLVWriter()) != nullptr, err = CHIP_ERROR_INCORRECT_STATE); SuccessOrExit(err = writer->Put(TLV::ContextTag(Commands::ScanNetworksResponse::Fields::kNetworkingStatus), status)); diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 718f7d38e567c2..b6a9ead84ea9b0 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -161,7 +161,7 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip } else { - apCommandObj->PrepareCommand(aCommandPath); + apCommandObj->PrepareCommand(aCommandPath, aCommandPath); chip::TLV::TLVWriter * writer = apCommandObj->GetCommandDataIBTLVWriter(); writer->PutBoolean(chip::TLV::ContextTag(1), true); apCommandObj->FinishCommand(); @@ -263,6 +263,23 @@ class TestCommandInteraction } private: + /** + * With the introduction of batch invoke commands, CommandHandler keeps track of incoming + * ConcreteCommandPath and the associated CommandRef. Normally this is populated as a part + * of OnMessageReceived from the incoming request. For some unit tests were we want to just + * test some of the processing messages we need to inject commands into the table of + * ConcreteCommandPath/CommandRef pairs. + */ + class CommandHandlerWithOutstandingCommand : public app::CommandHandler + { + public: + CommandHandlerWithOutstandingCommand(CommandHandler::Callback * apCallback, const ConcreteCommandPath & aRequestCommandPath, + const Optional & aRef) : CommandHandler(apCallback) + { + this->mCommandRefLookupTable.Add(aRequestCommandPath, aRef); + } + }; + // Generate an invoke request. If aCommandId is kTestCommandIdWithData, a // payload will be included. Otherwise no payload will be included. static void GenerateInvokeRequest(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload, @@ -451,18 +468,18 @@ void TestCommandInteraction::AddInvokeResponseData(nlTestSuite * apSuite, void * { CHIP_ERROR err = CHIP_NO_ERROR; + chip::app::ConcreteCommandPath requestCommandPath(1, // Endpoint + 3, // ClusterId + 4 // CommandId + ); if (aNeedStatusCode) { - chip::app::ConcreteCommandPath commandPath(1, // Endpoint - 3, // ClusterId - 4 // CommandId - ); - apCommandHandler->AddStatus(commandPath, Protocols::InteractionModel::Status::Success); + apCommandHandler->AddStatus(requestCommandPath, Protocols::InteractionModel::Status::Success); } else { ConcreteCommandPath path = { kTestEndpointId, kTestClusterId, aCommandId }; - err = apCommandHandler->PrepareCommand(path); + err = apCommandHandler->PrepareCommand(requestCommandPath, path); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); chip::TLV::TLVWriter * writer = apCommandHandler->GetCommandDataIBTLVWriter(); @@ -493,9 +510,9 @@ void TestCommandInteraction::TestCommandHandlerWithWrongState(nlTestSuite * apSu CHIP_ERROR err = CHIP_NO_ERROR; ConcreteCommandPath path = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; - app::CommandHandler commandHandler(&mockCommandHandlerDelegate); + CommandHandlerWithOutstandingCommand commandHandler(&mockCommandHandlerDelegate, path, NullOptional); - err = commandHandler.PrepareCommand(path); + err = commandHandler.PrepareCommand(path, path); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); TestExchangeDelegate delegate; @@ -542,14 +559,14 @@ void TestCommandInteraction::TestCommandHandlerWithSendEmptyCommand(nlTestSuite CHIP_ERROR err = CHIP_NO_ERROR; ConcreteCommandPath path = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; - app::CommandHandler commandHandler(&mockCommandHandlerDelegate); + CommandHandlerWithOutstandingCommand commandHandler(&mockCommandHandlerDelegate, path, NullOptional); System::PacketBufferHandle commandDatabuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); TestExchangeDelegate delegate; auto exchange = ctx.NewExchangeToAlice(&delegate, false); commandHandler.mExchangeCtx.Grab(exchange); - err = commandHandler.PrepareCommand(path); + err = commandHandler.PrepareCommand(path, path); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); err = commandHandler.FinishCommand(); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -577,8 +594,9 @@ void TestCommandInteraction::ValidateCommandHandlerWithSendCommand(nlTestSuite * { TestContext & ctx = *static_cast(apContext); CHIP_ERROR err = CHIP_NO_ERROR; - app::CommandHandler commandHandler(&mockCommandHandlerDelegate); System::PacketBufferHandle commandPacket; + chip::app::ConcreteCommandPath requestCommandPath(kTestEndpointId, kTestClusterId, kTestCommandIdWithData); + CommandHandlerWithOutstandingCommand commandHandler(&mockCommandHandlerDelegate, requestCommandPath, NullOptional); TestExchangeDelegate delegate; auto exchange = ctx.NewExchangeToAlice(&delegate, false); @@ -643,18 +661,18 @@ struct BadFields void TestCommandInteraction::TestCommandHandlerCommandDataEncoding(nlTestSuite * apSuite, void * apContext) { - TestContext & ctx = *static_cast(apContext); - CHIP_ERROR err = CHIP_NO_ERROR; - app::CommandHandler commandHandler(nullptr); + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + auto path = MakeTestCommandPath(); + auto requestCommandPath = ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId); + CommandHandlerWithOutstandingCommand commandHandler(nullptr, requestCommandPath, NullOptional); System::PacketBufferHandle commandPacket; TestExchangeDelegate delegate; auto exchange = ctx.NewExchangeToAlice(&delegate, false); commandHandler.mExchangeCtx.Grab(exchange); - auto path = MakeTestCommandPath(); - - commandHandler.AddResponse(ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId), Fields()); + commandHandler.AddResponse(requestCommandPath, Fields()); err = commandHandler.Finalize(commandPacket); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -680,16 +698,16 @@ void TestCommandInteraction::TestCommandHandlerCommandEncodeFailure(nlTestSuite { TestContext & ctx = *static_cast(apContext); CHIP_ERROR err = CHIP_NO_ERROR; - app::CommandHandler commandHandler(nullptr); + auto path = MakeTestCommandPath(); + auto requestCommandPath = ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId); + CommandHandlerWithOutstandingCommand commandHandler(nullptr, requestCommandPath, NullOptional); System::PacketBufferHandle commandPacket; TestExchangeDelegate delegate; auto exchange = ctx.NewExchangeToAlice(&delegate, false); commandHandler.mExchangeCtx.Grab(exchange); - auto path = MakeTestCommandPath(); - - commandHandler.AddResponse(ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId), BadFields()); + commandHandler.AddResponse(requestCommandPath, BadFields()); err = commandHandler.Finalize(commandPacket); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -1067,6 +1085,10 @@ void TestCommandInteraction::TestCommandHandlerInvalidMessageAsync(nlTestSuite * err = commandSender.SendCommandRequest(ctx.GetSessionBobToAlice()); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + // Warning! This test is not testing what the testname is suggesting. Because the invoke request is malformed, + // DispatchSingleClusterCommand is never called and asyncCommandHandle is never set. Possible fix to this test + // might be incoming when CommandSender is capable of sending multiple invoke command requests. + // Decrease CommandHandler refcount and send response asyncCommandHandle = nullptr; @@ -1084,19 +1106,18 @@ void TestCommandInteraction::TestCommandHandlerCommandEncodeExternalFailure(nlTe { TestContext & ctx = *static_cast(apContext); CHIP_ERROR err = CHIP_NO_ERROR; - app::CommandHandler commandHandler(nullptr); + auto path = MakeTestCommandPath(); + auto requestCommandPath = ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId); + CommandHandlerWithOutstandingCommand commandHandler(nullptr, requestCommandPath, NullOptional); System::PacketBufferHandle commandPacket; TestExchangeDelegate delegate; auto exchange = ctx.NewExchangeToAlice(&delegate, false); commandHandler.mExchangeCtx.Grab(exchange); - auto path = MakeTestCommandPath(); - - err = commandHandler.AddResponseData(ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId), BadFields()); + err = commandHandler.AddResponseData(requestCommandPath, BadFields()); NL_TEST_ASSERT(apSuite, err != CHIP_NO_ERROR); - commandHandler.AddStatus(ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId), - Protocols::InteractionModel::Status::Failure); + commandHandler.AddStatus(requestCommandPath, Protocols::InteractionModel::Status::Failure); err = commandHandler.Finalize(commandPacket); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); diff --git a/src/app/tests/integration/chip_im_responder.cpp b/src/app/tests/integration/chip_im_responder.cpp index e8fa74d9baa26c..5ab1f88cd1ba51 100644 --- a/src/app/tests/integration/chip_im_responder.cpp +++ b/src/app/tests/integration/chip_im_responder.cpp @@ -73,7 +73,7 @@ Protocols::InteractionModel::Status ServerClusterCommandExists(const ConcreteCom return Status::Success; } -void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip::TLV::TLVReader & aReader, +void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPath, chip::TLV::TLVReader & aReader, CommandHandler * apCommandObj) { static bool statusCodeFlipper = false; @@ -106,7 +106,7 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip chip::TLV::TLVWriter * writer; - ReturnOnFailure(apCommandObj->PrepareCommand(path)); + ReturnOnFailure(apCommandObj->PrepareCommand(aRequestCommandPath, path)); writer = apCommandObj->GetCommandDataIBTLVWriter(); ReturnOnFailure(writer->Put(chip::TLV::ContextTag(kTestFieldId1), kTestFieldValue1));