From 4fe0bdc0b5c14361fb0f9e958eff5291973f237d Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 22 Nov 2023 14:33:30 +0000 Subject: [PATCH 01/22] 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)); From 19dd4d7316831e60a1a0a502bb1e202059be5a82 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 22 Nov 2023 16:11:26 +0000 Subject: [PATCH 02/22] Restyle --- src/app/CommandHandler.cpp | 2 +- src/app/tests/TestCommandInteraction.cpp | 11 ++++++----- src/app/tests/integration/chip_im_responder.cpp | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index ab5e775a8c25f8..2e910fe0b9fa98 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -133,7 +133,7 @@ CHIP_ERROR CommandHandler::ValidateCommands(TLV::TLVReader & invokeRequestsReade commandRef = MakeOptional(ref); } - VerifyOrReturnError(!mCommandRefLookupTable.IsRequestedPathAndRefUnique(concretePath, commandRef), + VerifyOrReturnError(mCommandRefLookupTable.IsRequestedPathAndRefUnique(concretePath, commandRef), CHIP_ERROR_INVALID_ARGUMENT); ReturnErrorOnFailure(mCommandRefLookupTable.Add(concretePath, commandRef)); } diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index b6a9ead84ea9b0..ee38c3bf310dc1 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -274,7 +274,8 @@ class TestCommandInteraction { public: CommandHandlerWithOutstandingCommand(CommandHandler::Callback * apCallback, const ConcreteCommandPath & aRequestCommandPath, - const Optional & aRef) : CommandHandler(apCallback) + const Optional & aRef) : + CommandHandler(apCallback) { this->mCommandRefLookupTable.Add(aRequestCommandPath, aRef); } @@ -696,8 +697,8 @@ void TestCommandInteraction::TestCommandHandlerCommandDataEncoding(nlTestSuite * void TestCommandInteraction::TestCommandHandlerCommandEncodeFailure(nlTestSuite * apSuite, void * apContext) { - TestContext & ctx = *static_cast(apContext); - CHIP_ERROR err = CHIP_NO_ERROR; + 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); @@ -1104,8 +1105,8 @@ void TestCommandInteraction::TestCommandHandlerInvalidMessageAsync(nlTestSuite * void TestCommandInteraction::TestCommandHandlerCommandEncodeExternalFailure(nlTestSuite * apSuite, void * apContext) { - TestContext & ctx = *static_cast(apContext); - CHIP_ERROR err = CHIP_NO_ERROR; + 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); diff --git a/src/app/tests/integration/chip_im_responder.cpp b/src/app/tests/integration/chip_im_responder.cpp index 5ab1f88cd1ba51..e6716f57180dec 100644 --- a/src/app/tests/integration/chip_im_responder.cpp +++ b/src/app/tests/integration/chip_im_responder.cpp @@ -78,7 +78,7 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPat { static bool statusCodeFlipper = false; - if (ServerClusterCommandExists(aCommandPath) != Protocols::InteractionModel::Status::Success) + if (ServerClusterCommandExists(aRequestCommandPath) != Protocols::InteractionModel::Status::Success) { return; } From 56d5e5b012589bb8b316b4067e4dc2201d272b5b Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 22 Nov 2023 19:40:23 +0000 Subject: [PATCH 03/22] Fix CI --- src/app/CommandHandler.cpp | 22 +++++++++++++++++----- src/app/CommandHandler.h | 8 ++++---- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 2e910fe0b9fa98..d9974b9e34ab56 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -105,6 +105,12 @@ CHIP_ERROR CommandHandler::ValidateCommands(TLV::TLVReader & invokeRequestsReade bool commandRefExpected = false; TLV::Utilities::Count(invokeRequestsReader, commandCount, false /* recurse */); + if (IsGroupRequest()) + { + VerifyOrReturnError(commandCount == 1, CHIP_ERROR_INVALID_ARGUMENT); + return CHIP_NO_ERROR; + } + VerifyOrReturnError(commandCount <= CHIP_CONFIG_MAX_PATHS_PER_INVOKE, CHIP_ERROR_INVALID_ARGUMENT); commandRefExpected = commandCount > 1; @@ -158,6 +164,10 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa #if CHIP_CONFIG_IM_PRETTY_PRINT invokeRequestMessage.PrettyPrint(); #endif + if (mExchangeCtx->IsGroupExchangeContext()) + { + SetGroupRequest(true); + } VerifyOrReturnError(invokeRequestMessage.GetSuppressResponse(&mSuppressResponse) == CHIP_NO_ERROR, Status::InvalidAction); VerifyOrReturnError(invokeRequestMessage.GetTimedRequest(&mTimedRequest) == CHIP_NO_ERROR, Status::InvalidAction); @@ -175,7 +185,7 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa CommandDataIB::Parser commandData; VerifyOrReturnError(commandData.Init(invokeRequestsReader) == CHIP_NO_ERROR, Status::InvalidAction); Status status = Status::Success; - if (mExchangeCtx->IsGroupExchangeContext()) + if (IsGroupRequest()) { status = ProcessGroupCommandDataIB(commandData); } @@ -245,7 +255,7 @@ void CommandHandler::DecrementHoldOff() { ChipLogProgress(DataManagement, "Skipping command response: exchange context is null"); } - else if (!mExchangeCtx->IsGroupExchangeContext()) + else if (!IsGroupRequest()) { CHIP_ERROR err = SendCommandResponse(); if (err != CHIP_NO_ERROR) @@ -296,8 +306,6 @@ Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElem ConcreteCommandPath concretePath(0, 0, 0); TLV::TLVReader commandDataReader; - SetGroupRequest(false); - // NOTE: errors may occur before the concrete command path is even fully decoded. err = aCommandElement.GetPath(&commandPath); @@ -399,7 +407,6 @@ Status CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aComman Credentials::GroupDataProvider::GroupEndpoint mapping; Credentials::GroupDataProvider * groupDataProvider = Credentials::GetGroupDataProvider(); Credentials::GroupDataProvider::EndpointIterator * iterator; - SetGroupRequest(true); err = aCommandElement.GetPath(&commandPath); VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); @@ -548,12 +555,17 @@ CHIP_ERROR CommandHandler::AddClusterSpecificFailure(const ConcreteCommandPath & CHIP_ERROR CommandHandler::PrepareCommand(const ConcreteCommandPath & aRequestCommandPath, const ConcreteCommandPath & aCommandPath, bool aStartDataStruct) { + // Return prematurely in case of requests targeted to a group that should not add command for response purposes. + VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); auto * commandRefTableEntry = mCommandRefLookupTable.Find(aRequestCommandPath); return PrepareCommand(commandRefTableEntry, aCommandPath, aStartDataStruct); } CHIP_ERROR CommandHandler::PrepareCommand(const ConcreteCommandPath & aCommandPath, bool aStartDataStruct) { + // Return prematurely in case of requests targeted to a group that should not add command for response purposes. + VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); + // 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. diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 115c1fb3ae6b32..bcf4e63c0f64d3 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -436,15 +436,15 @@ class CommandHandler : public Messaging::ExchangeDelegate bool mSentStatusResponse = false; - State mState = State::Idle; - bool mGroupRequest = false; - Optional mRefForResponse; + State mState = State::Idle; - CommandRefLookupTable mCommandRefLookupTable; chip::System::PacketBufferTLVWriter mCommandMessageWriter; TLV::TLVWriter mBackupWriter; State mBackupState; + CommandRefLookupTable mCommandRefLookupTable; + Optional mRefForResponse; + bool mGroupRequest = false; 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 From a2e56dd333c23dac1e33e9e635ad97b21cc7cb28 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 23 Nov 2023 20:36:04 +0000 Subject: [PATCH 04/22] Address PR comments --- src/app/CommandHandler.cpp | 70 ++++++++------ src/app/CommandHandler.h | 95 +++++++++++++++---- src/app/CommandPathRegistry.h | 114 +++++++++++++++++++++++ src/app/CommandRefLookupTable.h | 106 --------------------- src/app/tests/TestCommandInteraction.cpp | 59 ++++++------ 5 files changed, 266 insertions(+), 178 deletions(-) create mode 100644 src/app/CommandPathRegistry.h delete mode 100644 src/app/CommandRefLookupTable.h diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index d9974b9e34ab56..13cdf4a5d138e9 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -98,20 +98,28 @@ void CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * ec, con mGoneAsync = true; } -CHIP_ERROR CommandHandler::ValidateCommands(TLV::TLVReader & invokeRequestsReader) +CHIP_ERROR CommandHandler::ValidateInvokeRequestsAndBuildRegistry(TLV::TLVReader & invokeRequestsReader) { CHIP_ERROR err = CHIP_NO_ERROR; size_t commandCount = 0; bool commandRefExpected = false; TLV::Utilities::Count(invokeRequestsReader, commandCount, false /* recurse */); + + // If this is a GroupRequest the only thing to check is that there is only one + // CommandDataIB. if (IsGroupRequest()) { VerifyOrReturnError(commandCount == 1, CHIP_ERROR_INVALID_ARGUMENT); return CHIP_NO_ERROR; } + // While technically any commandCount == 1 should already be unique and does not need + // any further validation, we do need to read and populate the registry to help + // in building the InvokeResponse. + + VerifyOrReturnError(commandCount <= MaxPathsPerInvoke(), CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(commandCount <= CHIP_CONFIG_MAX_PATHS_PER_INVOKE, CHIP_ERROR_INVALID_ARGUMENT); + // If there is more than one CommandDataIB, spec states that CommandRef must be provided. commandRefExpected = commandCount > 1; while (CHIP_NO_ERROR == (err = invokeRequestsReader.Next())) @@ -120,12 +128,13 @@ CHIP_ERROR CommandHandler::ValidateCommands(TLV::TLVReader & invokeRequestsReade CommandDataIB::Parser commandData; ReturnErrorOnFailure(commandData.Init(invokeRequestsReader)); + // First validating that we can get a ConcreteCommandPath CommandPathIB::Parser commandPath; ConcreteCommandPath concretePath(0, 0, 0); - ReturnErrorOnFailure(commandData.GetPath(&commandPath)); ReturnErrorOnFailure(commandPath.GetConcreteCommandPath(concretePath)); + // Trying to read CommandRef. If it is not populated, we will assume value of 0. Optional commandRef; uint16_t ref; err = commandData.GetRef(&ref); @@ -139,12 +148,10 @@ CHIP_ERROR CommandHandler::ValidateCommands(TLV::TLVReader & invokeRequestsReade commandRef = MakeOptional(ref); } - VerifyOrReturnError(mCommandRefLookupTable.IsRequestedPathAndRefUnique(concretePath, commandRef), - CHIP_ERROR_INVALID_ARGUMENT); - ReturnErrorOnFailure(mCommandRefLookupTable.Add(concretePath, commandRef)); + ReturnErrorOnFailure(GetCommandPathRegistry().Add(concretePath, commandRef)); } - // if we have exhausted this container + // It's OK/expected to have reached the end of the container without failure. if (CHIP_END_OF_TLV == err) { err = CHIP_NO_ERROR; @@ -175,8 +182,8 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa VerifyOrReturnError(mTimedRequest == isTimedInvoke, Status::UnsupportedAccess); invokeRequests.GetReader(&invokeRequestsReader); - VerifyOrReturnError(ValidateCommands(invokeRequestsReader) == CHIP_NO_ERROR, Status::InvalidAction); - // Reset the reader + VerifyOrReturnError(ValidateInvokeRequestsAndBuildRegistry(invokeRequestsReader) == CHIP_NO_ERROR, Status::InvalidAction); + // Reset the reader after validation, so that we can iterate the commands to actually process them. invokeRequests.GetReader(&invokeRequestsReader); while (CHIP_NO_ERROR == (err = invokeRequestsReader.Next())) @@ -552,37 +559,44 @@ 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) +CHIP_ERROR CommandHandler::PrepareInvokeResponseCommand(const ConcreteCommandPath & aRequestCommandPath, + const ConcreteCommandPath & aResponseCommandPath, bool aStartDataStruct) { - // Return prematurely in case of requests targeted to a group that should not add command for response purposes. + // Return early in case of requests targeted to a group, since they should not add a response. VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); - auto * commandRefTableEntry = mCommandRefLookupTable.Find(aRequestCommandPath); - return PrepareCommand(commandRefTableEntry, aCommandPath, aStartDataStruct); + + auto commandPathRegistryEntry = GetCommandPathRegistry().Find(aRequestCommandPath); + VerifyOrReturnValue(commandPathRegistryEntry.HasValue(), CHIP_ERROR_INCORRECT_STATE); + + return PrepareInvokeResponseCommand(commandPathRegistryEntry.Value(), aResponseCommandPath, aStartDataStruct); } -CHIP_ERROR CommandHandler::PrepareCommand(const ConcreteCommandPath & aCommandPath, bool aStartDataStruct) +CHIP_ERROR CommandHandler::PrepareCommand(const ConcreteCommandPath & aResponseCommandPath, bool aStartDataStruct) { - // Return prematurely in case of requests targeted to a group that should not add command for response purposes. + // Return early in case of requests targeted to a group, since they should not add a response. VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); // 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(); + size_t countOfPathRegistryEntries = GetCommandPathRegistry().Count(); - // 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 + // At this point application supports Batch Invoke Commands since CommandPathRegistry has more than 1 entry, + // but application is calling the deprecated PrepareCommand. We have no way to determine the associated CommandRef // to put into the InvokeResponse. - VerifyOrDieWithMsg(commandRefTableEntry, DataManagement, + VerifyOrDieWithMsg(countOfPathRegistryEntries > 1, DataManagement, "Seemingly device supports batch commands, but is calling the deprecated PrepareCommand API"); - return PrepareCommand(commandRefTableEntry, aCommandPath, aStartDataStruct); + + auto commandPathRegistryEntry = GetCommandPathRegistry().GetFirstEntry(); + VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); + VerifyOrReturnValue(commandPathRegistryEntry.HasValue(), CHIP_ERROR_INCORRECT_STATE); + + return PrepareInvokeResponseCommand(commandPathRegistryEntry.Value(), aResponseCommandPath, aStartDataStruct); } -CHIP_ERROR CommandHandler::PrepareCommand(const CommandRefLookupTable::CommandRefTableEntry * apCommandRefTableEntry, - const ConcreteCommandPath & aCommandPath, bool aStartDataStruct) +CHIP_ERROR CommandHandler::PrepareInvokeResponseCommand(const CommandPathRegistryEntry & apCommandPathRegistryEntry, + const ConcreteCommandPath & aCommandPath, bool aStartDataStruct) { - VerifyOrReturnError(apCommandRefTableEntry, CHIP_ERROR_INCORRECT_STATE); ReturnErrorOnFailure(AllocateBuffer()); mInvokeResponseBuilder.Checkpoint(mBackupWriter); @@ -592,7 +606,7 @@ CHIP_ERROR CommandHandler::PrepareCommand(const CommandRefLookupTable::CommandRe // VerifyOrReturnError(mState == State::Idle || mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE); - mRefForResponse = apCommandRefTableEntry->ref; + mRefForResponse = apCommandPathRegistryEntry.ref; MoveToState(State::Preparing); InvokeResponseIBs::Builder & invokeResponses = mInvokeResponseBuilder.GetInvokeResponses(); @@ -641,9 +655,9 @@ CHIP_ERROR CommandHandler::PrepareStatus(const ConcreteCommandPath & aCommandPat // 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; + auto commandPathRegistryEntry = GetCommandPathRegistry().Find(aCommandPath); + VerifyOrReturnError(commandPathRegistryEntry.HasValue(), CHIP_ERROR_INCORRECT_STATE); + mRefForResponse = commandPathRegistryEntry.Value().ref; MoveToState(State::Preparing); InvokeResponseIBs::Builder & invokeResponses = mInvokeResponseBuilder.GetInvokeResponses(); diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index bcf4e63c0f64d3..28887ebaa28ed5 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -30,7 +30,7 @@ #pragma once -#include "CommandRefLookupTable.h" +#include "CommandPathRegistry.h" #include #include @@ -173,6 +173,15 @@ class CommandHandler : public Messaging::ExchangeDelegate void OnInvokeCommandRequest(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader, System::PacketBufferHandle && payload, bool isTimedInvoke); + /** + * Checks that all CommandDataIB within InvokeRequests is correct as per spec. + * + * This also build a lookup table that is multipurpose. It helps validate that all elements + * in the invoke request are unique, but also used when later on when populating + * elements of the InvokeResponse. + */ + CHIP_ERROR ValidateInvokeRequestsAndBuildRegistry(TLV::TLVReader & invokeRequestsReader); + /** * Adds the given command status and returns any failures in adding statuses (e.g. out * of buffer space) to the caller @@ -192,12 +201,59 @@ 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 & aRequestCommandPath, const ConcreteCommandPath & aCommandPath, - bool aStartDataStruct = true); - [[deprecated("PrepareCommand now needs the requested command path. Without it device cannot support batch invoke")]] CHIP_ERROR + + /** + * This will add a new CommandDataIB element into InvokeResponses. It will put the + * aResponseCommandPath into CommandPath element within CommandDataIB. + * + * This call will fail if CommandHandler is already in the middle of building/sending + * InvokeResponseMessage. + * + * Upon success, the caller is expected to call `FinishCommand` once they have added + * all Data into Fields element of CommandDataIB. + * + * @param [in] aRequestCommandPath the concrete path of the command we are responding to. + * @param [in] aResponseCommandPath the concrete response path that we are sending to Requester. + * @param [in] aStartDataStruct starts the TLV container for the CommandFields element within + * CommandDataIB. + */ + CHIP_ERROR PrepareInvokeResponseCommand(const ConcreteCommandPath & aRequestCommandPath, + const ConcreteCommandPath & aResponseCommandPath, bool aStartDataStruct = true); + [[deprecated("PrepareCommand now needs the requested command path. Please use PrepareInvokeResponseCommand")]] CHIP_ERROR PrepareCommand(const ConcreteCommandPath & aCommandPath, bool aStartDataStruct = true); + + /** + * Finishes the CommandDataIB element within the InvokeResponses. + * + * Caller must have first successfully called `PrepareInvokeResponseCommand`. + * + * @param [in] aEndDataStruct end the TLV container for the CommandFields element within + * CommandDataIB. + */ CHIP_ERROR FinishCommand(bool aEndDataStruct = true); + + /** + * This will add a new CommandStatusIB element into InvokeResponses. It will put the + * aCommandPath into CommandPath element within CommandStatusIB. + * + * This call will fail if CommandHandler is already in the middle of building/sending + * InvokeResponseMessage. + * + * Upon success, the caller is expected to call `FinishStatus` once they have added + * data into Fields element of CommandStatusIB. + * + * @param [in] aCommandPath the concrete path of the command we are responding to. + */ CHIP_ERROR PrepareStatus(const ConcreteCommandPath & aCommandPath); + + /** + * Finishes the CommandStatusIB element within the InvokeResponses. + * + * Caller must have first successfully called `PrepareStatus`. + * + * @param [in] aEndDataStruct end the TLV container for the CommandFields element within + * CommandDataIB. + */ CHIP_ERROR FinishStatus(); TLV::TLVWriter * GetCommandDataIBTLVWriter(); @@ -320,7 +376,7 @@ class CommandHandler : public Messaging::ExchangeDelegate VerifyOrDie(false); } - enum class State + enum class State : uint8_t { Idle, ///< Default state that the object starts out in, where no work has commenced Preparing, ///< We are prepaing the command or status header. @@ -368,10 +424,8 @@ 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 PrepareInvokeResponseCommand(const CommandPathRegistryEntry & apCommandPathRegistryEntry, + const ConcreteCommandPath & aCommandPath, bool aStartDataStruct); CHIP_ERROR Finalize(System::PacketBufferHandle & commandPacket); @@ -407,8 +461,11 @@ class CommandHandler : public Messaging::ExchangeDelegate template CHIP_ERROR TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData) { + // Return early in case of requests targeted to a group, since they should not add a response. + VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); + ConcreteCommandPath path = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, CommandData::GetCommandId() }; - ReturnErrorOnFailure(PrepareCommand(aRequestCommandPath, path, false)); + ReturnErrorOnFailure(PrepareInvokeResponseCommand(aRequestCommandPath, path, false)); TLV::TLVWriter * writer = GetCommandDataIBTLVWriter(); VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE); ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields), aData)); @@ -426,24 +483,26 @@ class CommandHandler : public Messaging::ExchangeDelegate */ void SetGroupRequest(bool isGroupRequest) { mGroupRequest = isGroupRequest; } + CommandPathRegistryInterface & GetCommandPathRegistry() { return mCommandPathRegistry; } + + size_t MaxPathsPerInvoke() { return CHIP_CONFIG_MAX_PATHS_PER_INVOKE; } + Messaging::ExchangeHolder mExchangeCtx; Callback * mpCallback = nullptr; InvokeResponseMessage::Builder mInvokeResponseBuilder; TLV::TLVType mDataElementContainerType = TLV::kTLVType_NotSpecified; size_t mPendingWork = 0; - bool mSuppressResponse = false; - bool mTimedRequest = false; - - bool mSentStatusResponse = false; - - State mState = State::Idle; chip::System::PacketBufferTLVWriter mCommandMessageWriter; TLV::TLVWriter mBackupWriter; - State mBackupState; - CommandRefLookupTable mCommandRefLookupTable; + StaticCommandPathRegistry mCommandPathRegistry; Optional mRefForResponse; + State mState = State::Idle; + State mBackupState; + bool mSuppressResponse = false; + bool mTimedRequest = false; + bool mSentStatusResponse = false; bool mGroupRequest = false; bool mBufferAllocated = false; // If mGoneAsync is true, we have finished out initial processing of the diff --git a/src/app/CommandPathRegistry.h b/src/app/CommandPathRegistry.h new file mode 100644 index 00000000000000..f2aed2c61c1028 --- /dev/null +++ b/src/app/CommandPathRegistry.h @@ -0,0 +1,114 @@ +/* + * + * 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 +#include + +namespace chip { +namespace app { + +struct CommandPathRegistryEntry +{ + ConcreteCommandPath requestPath = ConcreteCommandPath(0, 0, 0); + Optional ref; +}; + +class CommandPathRegistryInterface +{ +public: + virtual ~CommandPathRegistryInterface(){}; + virtual Optional Find(const ConcreteCommandPath & requestPath) const = 0; + virtual Optional GetFirstEntry() const = 0; + virtual CHIP_ERROR Add(const ConcreteCommandPath & requestPath, const Optional & ref) = 0; + virtual size_t Count() const = 0; +}; + +/** + * @class StaticCommandPathRegistry + * + * @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 constraints. All commands need to be contained within a single InvokeRequest. In + * practice this is usually less than 60 commands (but could be much more with TCP transports or + * newer transports). + */ +template +class StaticCommandPathRegistry : public CommandPathRegistryInterface +{ +public: + ~StaticCommandPathRegistry() override{}; + + Optional Find(const ConcreteCommandPath & requestPath) const override + { + for (int i = 0; i < mCount; i++) + { + if (mTable[i].requestPath == requestPath) + { + return MakeOptional(mTable[i]); + } + } + return NullOptional; + } + + Optional GetFirstEntry() const override + { + if (mCount > 0) + { + return MakeOptional(mTable[0]); + } + return NullOptional; + } + + CHIP_ERROR Add(const ConcreteCommandPath & requestPath, const Optional & ref) override + { + if (mCount > N) + { + return CHIP_ERROR_NO_MEMORY; + } + for (int i = 0; i < mCount; i++) + { + if (mTable[i].requestPath == requestPath) + { + return CHIP_ERROR_DUPLICATE_KEY_ID; + } + if (mTable[i].ref.HasValue() && mTable[i].ref == ref) + { + return CHIP_ERROR_DUPLICATE_KEY_ID; + } + } + + mTable[mCount] = CommandPathRegistryEntry{ requestPath, ref }; + mCount++; + return CHIP_NO_ERROR; + } + + virtual size_t Count() const override { return mCount; } + +private: + size_t mCount = 0; + CommandPathRegistryEntry mTable[N]; +}; + +} // namespace app +} // namespace chip diff --git a/src/app/CommandRefLookupTable.h b/src/app/CommandRefLookupTable.h deleted file mode 100644 index 940751df76028e..00000000000000 --- a/src/app/CommandRefLookupTable.h +++ /dev/null @@ -1,106 +0,0 @@ -/* - * - * 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/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index ee38c3bf310dc1..215bc57d02f9fe 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -88,22 +88,22 @@ namespace app { CommandHandler::Handle asyncCommandHandle; -InteractionModel::Status ServerClusterCommandExists(const ConcreteCommandPath & aCommandPath) +InteractionModel::Status ServerClusterCommandExists(const ConcreteCommandPath & aRequestCommandPath) { // Mock cluster catalog, only support commands on one cluster on one endpoint. using InteractionModel::Status; - if (aCommandPath.mEndpointId != kTestEndpointId) + if (aRequestCommandPath.mEndpointId != kTestEndpointId) { return Status::UnsupportedEndpoint; } - if (aCommandPath.mClusterId != kTestClusterId) + if (aRequestCommandPath.mClusterId != kTestClusterId) { return Status::UnsupportedCluster; } - if (aCommandPath.mCommandId == kTestNonExistCommandId) + if (aRequestCommandPath.mCommandId == kTestNonExistCommandId) { return Status::UnsupportedCommand; } @@ -111,17 +111,18 @@ InteractionModel::Status ServerClusterCommandExists(const ConcreteCommandPath & return Status::Success; } -void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip::TLV::TLVReader & aReader, +void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPath, chip::TLV::TLVReader & aReader, CommandHandler * apCommandObj) { ChipLogDetail(Controller, "Received Cluster Command: Endpoint=%x Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI, - aCommandPath.mEndpointId, ChipLogValueMEI(aCommandPath.mClusterId), ChipLogValueMEI(aCommandPath.mCommandId)); + aRequestCommandPath.mEndpointId, ChipLogValueMEI(aRequestCommandPath.mClusterId), + ChipLogValueMEI(aRequestCommandPath.mCommandId)); // Duplicate what our normal command-field-decode code does, in terms of // checking for a struct and then entering it before getting the fields. if (aReader.GetType() != TLV::kTLVType_Structure) { - apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::InvalidAction); + apCommandObj->AddStatus(aRequestCommandPath, Protocols::InteractionModel::Status::InvalidAction); return; } @@ -130,7 +131,7 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip NL_TEST_ASSERT(gSuite, err == CHIP_NO_ERROR); err = aReader.Next(); - if (aCommandPath.mCommandId == kTestCommandIdNoData) + if (aRequestCommandPath.mCommandId == kTestCommandIdNoData) { NL_TEST_ASSERT(gSuite, err == CHIP_ERROR_END_OF_TLV); } @@ -155,13 +156,14 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip if (sendResponse) { - if (aCommandPath.mCommandId == kTestCommandIdNoData || aCommandPath.mCommandId == kTestCommandIdWithData) + if (aRequestCommandPath.mCommandId == kTestCommandIdNoData || aRequestCommandPath.mCommandId == kTestCommandIdWithData) { - apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::Success); + apCommandObj->AddStatus(aRequestCommandPath, Protocols::InteractionModel::Status::Success); } else { - apCommandObj->PrepareCommand(aCommandPath, aCommandPath); + const ConcreteCommandPath responseCommandPath = aRequestCommandPath; + apCommandObj->PrepareInvokeResponseCommand(aRequestCommandPath, responseCommandPath); chip::TLV::TLVWriter * writer = apCommandObj->GetCommandDataIBTLVWriter(); writer->PutBoolean(chip::TLV::ContextTag(1), true); apCommandObj->FinishCommand(); @@ -277,7 +279,7 @@ class TestCommandInteraction const Optional & aRef) : CommandHandler(apCallback) { - this->mCommandRefLookupTable.Add(aRequestCommandPath, aRef); + GetCommandPathRegistry().Add(aRequestCommandPath, aRef); } }; @@ -469,18 +471,18 @@ void TestCommandInteraction::AddInvokeResponseData(nlTestSuite * apSuite, void * { CHIP_ERROR err = CHIP_NO_ERROR; - chip::app::ConcreteCommandPath requestCommandPath(1, // Endpoint - 3, // ClusterId - 4 // CommandId - ); + constexpr EndpointId kTestEndpointId = 1; + constexpr ClusterId kTestClusterId = 3; + constexpr CommandId kTestCommandIdWithData = 4; + ConcreteCommandPath requestCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdWithData }; if (aNeedStatusCode) { apCommandHandler->AddStatus(requestCommandPath, Protocols::InteractionModel::Status::Success); } else { - ConcreteCommandPath path = { kTestEndpointId, kTestClusterId, aCommandId }; - err = apCommandHandler->PrepareCommand(requestCommandPath, path); + ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, aCommandId }; + err = apCommandHandler->PrepareInvokeResponseCommand(requestCommandPath, responseCommandPath); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); chip::TLV::TLVWriter * writer = apCommandHandler->GetCommandDataIBTLVWriter(); @@ -509,11 +511,13 @@ void TestCommandInteraction::TestCommandHandlerWithWrongState(nlTestSuite * apSu { TestContext & ctx = *static_cast(apContext); CHIP_ERROR err = CHIP_NO_ERROR; - ConcreteCommandPath path = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; + ConcreteCommandPath requestCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; + ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; - CommandHandlerWithOutstandingCommand commandHandler(&mockCommandHandlerDelegate, path, NullOptional); + CommandHandlerWithOutstandingCommand commandHandler(&mockCommandHandlerDelegate, requestCommandPath, + /* aRef = */ NullOptional); - err = commandHandler.PrepareCommand(path, path); + err = commandHandler.PrepareInvokeResponseCommand(requestCommandPath, responseCommandPath); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); TestExchangeDelegate delegate; @@ -558,16 +562,18 @@ void TestCommandInteraction::TestCommandHandlerWithSendEmptyCommand(nlTestSuite { TestContext & ctx = *static_cast(apContext); CHIP_ERROR err = CHIP_NO_ERROR; - ConcreteCommandPath path = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; + ConcreteCommandPath requestCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; + ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; - CommandHandlerWithOutstandingCommand commandHandler(&mockCommandHandlerDelegate, path, NullOptional); + CommandHandlerWithOutstandingCommand commandHandler(&mockCommandHandlerDelegate, requestCommandPath, + /* aRef = */ 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, path); + err = commandHandler.PrepareInvokeResponseCommand(requestCommandPath, responseCommandPath); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); err = commandHandler.FinishCommand(); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -597,7 +603,8 @@ void TestCommandInteraction::ValidateCommandHandlerWithSendCommand(nlTestSuite * CHIP_ERROR err = CHIP_NO_ERROR; System::PacketBufferHandle commandPacket; chip::app::ConcreteCommandPath requestCommandPath(kTestEndpointId, kTestClusterId, kTestCommandIdWithData); - CommandHandlerWithOutstandingCommand commandHandler(&mockCommandHandlerDelegate, requestCommandPath, NullOptional); + CommandHandlerWithOutstandingCommand commandHandler(&mockCommandHandlerDelegate, requestCommandPath, + /* aRef = */ NullOptional); TestExchangeDelegate delegate; auto exchange = ctx.NewExchangeToAlice(&delegate, false); @@ -666,7 +673,7 @@ void TestCommandInteraction::TestCommandHandlerCommandDataEncoding(nlTestSuite * CHIP_ERROR err = CHIP_NO_ERROR; auto path = MakeTestCommandPath(); auto requestCommandPath = ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId); - CommandHandlerWithOutstandingCommand commandHandler(nullptr, requestCommandPath, NullOptional); + CommandHandlerWithOutstandingCommand commandHandler(nullptr, requestCommandPath, /* aRef = */ NullOptional); System::PacketBufferHandle commandPacket; TestExchangeDelegate delegate; From 545b669893a4334d2d756dcc169d1e3bd2331c1c Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 23 Nov 2023 21:36:02 +0000 Subject: [PATCH 05/22] Fix CI --- src/app/CommandHandler.cpp | 8 +------- src/app/CommandPathRegistry.h | 4 ++-- .../network-commissioning/network-commissioning.cpp | 4 ++-- src/app/tests/integration/chip_im_responder.cpp | 2 +- 4 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 13cdf4a5d138e9..e8cb3d2a703a98 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -524,7 +524,7 @@ CHIP_ERROR CommandHandler::AddStatusInternal(const ConcreteCommandPath & aComman void CommandHandler::AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus, const char * context) { - // Return prematurely in case of requests targeted to a group that should not add the status for response purposes. + // Return early in case of requests targeted to a group, since they should not add a response. VerifyOrReturn(!IsGroupRequest()); VerifyOrDie(FallibleAddStatus(aCommandPath, aStatus, context) == CHIP_NO_ERROR); } @@ -562,9 +562,6 @@ CHIP_ERROR CommandHandler::AddClusterSpecificFailure(const ConcreteCommandPath & CHIP_ERROR CommandHandler::PrepareInvokeResponseCommand(const ConcreteCommandPath & aRequestCommandPath, const ConcreteCommandPath & aResponseCommandPath, bool aStartDataStruct) { - // Return early in case of requests targeted to a group, since they should not add a response. - VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); - auto commandPathRegistryEntry = GetCommandPathRegistry().Find(aRequestCommandPath); VerifyOrReturnValue(commandPathRegistryEntry.HasValue(), CHIP_ERROR_INCORRECT_STATE); @@ -573,9 +570,6 @@ CHIP_ERROR CommandHandler::PrepareInvokeResponseCommand(const ConcreteCommandPat CHIP_ERROR CommandHandler::PrepareCommand(const ConcreteCommandPath & aResponseCommandPath, bool aStartDataStruct) { - // Return early in case of requests targeted to a group, since they should not add a response. - VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); - // 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. diff --git a/src/app/CommandPathRegistry.h b/src/app/CommandPathRegistry.h index f2aed2c61c1028..b505071becce60 100644 --- a/src/app/CommandPathRegistry.h +++ b/src/app/CommandPathRegistry.h @@ -61,7 +61,7 @@ class StaticCommandPathRegistry : public CommandPathRegistryInterface Optional Find(const ConcreteCommandPath & requestPath) const override { - for (int i = 0; i < mCount; i++) + for (size_t i = 0; i < mCount; i++) { if (mTable[i].requestPath == requestPath) { @@ -86,7 +86,7 @@ class StaticCommandPathRegistry : public CommandPathRegistryInterface { return CHIP_ERROR_NO_MEMORY; } - for (int i = 0; i < mCount; i++) + for (size_t i = 0; i < mCount; i++) { if (mTable[i].requestPath == requestPath) { diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp index 5ea7435ff6db7e..9bf5ff00e57501 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.cpp +++ b/src/app/clusters/network-commissioning/network-commissioning.cpp @@ -815,7 +815,7 @@ void Instance::OnFinished(Status status, CharSpan debugText, ThreadScanResponseI size_t scanResponseArrayLength = 0; uint8_t extendedAddressBuffer[Thread::kSizeExtendedPanId]; - SuccessOrExit(err = commandHandle->PrepareCommand( + SuccessOrExit(err = commandHandle->PrepareInvokeResponseCommand( mPath, ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id))); VerifyOrExit((writer = commandHandle->GetCommandDataIBTLVWriter()) != nullptr, err = CHIP_ERROR_INCORRECT_STATE); @@ -927,7 +927,7 @@ void Instance::OnFinished(Status status, CharSpan debugText, WiFiScanResponseIte WiFiScanResponse scanResponse; size_t networksEncoded = 0; - SuccessOrExit(err = commandHandle->PrepareCommand( + SuccessOrExit(err = commandHandle->PrepareInvokeResponseCommand( mPath, ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id))); VerifyOrExit((writer = commandHandle->GetCommandDataIBTLVWriter()) != nullptr, err = CHIP_ERROR_INCORRECT_STATE); diff --git a/src/app/tests/integration/chip_im_responder.cpp b/src/app/tests/integration/chip_im_responder.cpp index e6716f57180dec..d5a4ca1c4238df 100644 --- a/src/app/tests/integration/chip_im_responder.cpp +++ b/src/app/tests/integration/chip_im_responder.cpp @@ -106,7 +106,7 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPat chip::TLV::TLVWriter * writer; - ReturnOnFailure(apCommandObj->PrepareCommand(aRequestCommandPath, path)); + ReturnOnFailure(apCommandObj->PrepareInvokeResponseCommand(aRequestCommandPath, path)); writer = apCommandObj->GetCommandDataIBTLVWriter(); ReturnOnFailure(writer->Put(chip::TLV::ContextTag(kTestFieldId1), kTestFieldValue1)); From d1bd66ccb3ecd9be232be10c24eddc56081dfea6 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 23 Nov 2023 21:45:31 +0000 Subject: [PATCH 06/22] Add some more comments --- src/app/CommandHandler.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index e8cb3d2a703a98..edac22e8f76432 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -128,13 +128,14 @@ CHIP_ERROR CommandHandler::ValidateInvokeRequestsAndBuildRegistry(TLV::TLVReader CommandDataIB::Parser commandData; ReturnErrorOnFailure(commandData.Init(invokeRequestsReader)); - // First validating that we can get a ConcreteCommandPath + // First validating that we can get a ConcreteCommandPath. CommandPathIB::Parser commandPath; ConcreteCommandPath concretePath(0, 0, 0); ReturnErrorOnFailure(commandData.GetPath(&commandPath)); ReturnErrorOnFailure(commandPath.GetConcreteCommandPath(concretePath)); - // Trying to read CommandRef. If it is not populated, we will assume value of 0. + // Following section is to grab the CommandRef if avaiable and perform + // any validation on it if we expect there to be a CommandRef Optional commandRef; uint16_t ref; err = commandData.GetRef(&ref); @@ -148,6 +149,8 @@ CHIP_ERROR CommandHandler::ValidateInvokeRequestsAndBuildRegistry(TLV::TLVReader commandRef = MakeOptional(ref); } + // Adding can fail if concretePath is not unique, or if commandRef is a value + // and is not unique, or if we have already added more paths then we support. ReturnErrorOnFailure(GetCommandPathRegistry().Add(concretePath, commandRef)); } From 06ffbdc4b2aca6904aaa9823722e8a77f962d8d3 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 23 Nov 2023 21:55:43 +0000 Subject: [PATCH 07/22] Restyle --- src/app/CommandHandler.h | 4 ++-- src/app/tests/TestCommandInteraction.cpp | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 28887ebaa28ed5..e91406d9fe809b 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -503,8 +503,8 @@ class CommandHandler : public Messaging::ExchangeDelegate bool mSuppressResponse = false; bool mTimedRequest = false; bool mSentStatusResponse = false; - bool mGroupRequest = false; - bool mBufferAllocated = false; + bool mGroupRequest = false; + 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 // time. diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 215bc57d02f9fe..1f563cfd2436c6 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -509,8 +509,8 @@ void TestCommandInteraction::TestCommandSenderWithWrongState(nlTestSuite * apSui void TestCommandInteraction::TestCommandHandlerWithWrongState(nlTestSuite * apSuite, void * apContext) { - TestContext & ctx = *static_cast(apContext); - CHIP_ERROR err = CHIP_NO_ERROR; + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; ConcreteCommandPath requestCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; @@ -560,8 +560,8 @@ void TestCommandInteraction::TestCommandSenderWithSendCommand(nlTestSuite * apSu void TestCommandInteraction::TestCommandHandlerWithSendEmptyCommand(nlTestSuite * apSuite, void * apContext) { - TestContext & ctx = *static_cast(apContext); - CHIP_ERROR err = CHIP_NO_ERROR; + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; ConcreteCommandPath requestCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdNoData }; From 75ef230bc2f356b451445d1d64bc99a0e7d04472 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 23 Nov 2023 23:17:24 +0000 Subject: [PATCH 08/22] Fix CI --- src/app/CommandHandler.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index e91406d9fe809b..46d556a957ce68 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -250,9 +250,6 @@ class CommandHandler : public Messaging::ExchangeDelegate * Finishes the CommandStatusIB element within the InvokeResponses. * * Caller must have first successfully called `PrepareStatus`. - * - * @param [in] aEndDataStruct end the TLV container for the CommandFields element within - * CommandDataIB. */ CHIP_ERROR FinishStatus(); TLV::TLVWriter * GetCommandDataIBTLVWriter(); From 39a22080f126303619afa669b28c012eed7bef85 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Fri, 24 Nov 2023 03:59:51 +0000 Subject: [PATCH 09/22] Add multi command unit test --- src/app/CommandHandler.cpp | 6 + src/app/CommandHandler.h | 16 +- src/app/CommandPathRegistry.h | 12 +- src/app/tests/TestCommandInteraction.cpp | 249 ++++++++++++++++++++++- 4 files changed, 272 insertions(+), 11 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index edac22e8f76432..abaa5fb790c3f1 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -45,6 +45,12 @@ using Status = Protocols::InteractionModel::Status; CommandHandler::CommandHandler(Callback * apCallback) : mExchangeCtx(*this), mpCallback(apCallback), mSuppressResponse(false) {} +CommandHandler::CommandHandler(Callback * apCallback, CommandPathRegistry * apCommandPathRegistry) : CommandHandler(apCallback) +{ + mMaxPathsPerInvoke = apCommandPathRegistry->MaxSize(); + mCommandPathRegistry = apCommandPathRegistry; +} + CHIP_ERROR CommandHandler::AllocateBuffer() { if (!mBufferAllocated) diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 46d556a957ce68..9a2af19cb8fa9f 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -160,6 +160,14 @@ class CommandHandler : public Messaging::ExchangeDelegate */ CommandHandler(Callback * apCallback); + /* + * Constructor to override number of supported paths per invoke. + * + * The callback and command path registry passed in has to outlive this CommandHandler object. + * Created for testing purposes. + */ + CommandHandler(Callback * apCallback, CommandPathRegistry * mCommandPathRegistry); + /* * Main entrypoint for this class to handle an invoke request. * @@ -480,9 +488,9 @@ class CommandHandler : public Messaging::ExchangeDelegate */ void SetGroupRequest(bool isGroupRequest) { mGroupRequest = isGroupRequest; } - CommandPathRegistryInterface & GetCommandPathRegistry() { return mCommandPathRegistry; } + CommandPathRegistry & GetCommandPathRegistry() { return *mCommandPathRegistry; } - size_t MaxPathsPerInvoke() { return CHIP_CONFIG_MAX_PATHS_PER_INVOKE; } + size_t MaxPathsPerInvoke() { return mMaxPathsPerInvoke; } Messaging::ExchangeHolder mExchangeCtx; Callback * mpCallback = nullptr; @@ -492,7 +500,9 @@ class CommandHandler : public Messaging::ExchangeDelegate chip::System::PacketBufferTLVWriter mCommandMessageWriter; TLV::TLVWriter mBackupWriter; - StaticCommandPathRegistry mCommandPathRegistry; + size_t mMaxPathsPerInvoke = CHIP_CONFIG_MAX_PATHS_PER_INVOKE; + BasicCommandPathRegistry mBasicCommandPathRegistry; + CommandPathRegistry * mCommandPathRegistry = &mBasicCommandPathRegistry; Optional mRefForResponse; State mState = State::Idle; diff --git a/src/app/CommandPathRegistry.h b/src/app/CommandPathRegistry.h index b505071becce60..c95014cfa67d17 100644 --- a/src/app/CommandPathRegistry.h +++ b/src/app/CommandPathRegistry.h @@ -33,18 +33,19 @@ struct CommandPathRegistryEntry Optional ref; }; -class CommandPathRegistryInterface +class CommandPathRegistry { public: - virtual ~CommandPathRegistryInterface(){}; + virtual ~CommandPathRegistry(){}; virtual Optional Find(const ConcreteCommandPath & requestPath) const = 0; virtual Optional GetFirstEntry() const = 0; virtual CHIP_ERROR Add(const ConcreteCommandPath & requestPath, const Optional & ref) = 0; virtual size_t Count() const = 0; + virtual size_t MaxSize() const = 0; }; /** - * @class StaticCommandPathRegistry + * @class BasicCommandPathRegistry * * @brief Allows looking up CommandRef using the requested ConcreteCommandPath. * @@ -54,10 +55,10 @@ class CommandPathRegistryInterface * newer transports). */ template -class StaticCommandPathRegistry : public CommandPathRegistryInterface +class BasicCommandPathRegistry : public CommandPathRegistry { public: - ~StaticCommandPathRegistry() override{}; + ~BasicCommandPathRegistry() override{}; Optional Find(const ConcreteCommandPath & requestPath) const override { @@ -104,6 +105,7 @@ class StaticCommandPathRegistry : public CommandPathRegistryInterface } virtual size_t Count() const override { return mCount; } + virtual size_t MaxSize() const override { return N; } private: size_t mCount = 0; diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 1f563cfd2436c6..65e09d38a56df0 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -69,6 +69,7 @@ namespace chip { namespace { bool isCommandDispatched = false; +size_t commandDispatchedCount = 0; bool sendResponse = true; bool asyncCommand = false; @@ -171,6 +172,7 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPat } chip::isCommandDispatched = true; + commandDispatchedCount++; } class MockCommandSenderCallback : public CommandSender::Callback @@ -220,6 +222,8 @@ class MockCommandHandlerCallback : public CommandHandler::Callback return ServerClusterCommandExists(aCommandPath); } + void ResetCounter() { onFinalCalledTimes = 0; } + int onFinalCalledTimes = 0; } mockCommandHandlerDelegate; @@ -246,7 +250,10 @@ class TestCommandInteraction static void TestCommandHandlerWithSendEmptyResponse(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerWithProcessReceivedEmptyDataMsg(nlTestSuite * apSuite, void * apContext); - static void TestCommandHandlerRejectMultipleCommands(nlTestSuite * apSuite, void * apContext); + static void TestCommandHandlerRejectMultipleIdenticalCommands(nlTestSuite * apSuite, void * apContext); + static void TestCommandHandlerRejectsMultipleCommandsWithIdenticalCommandRef(nlTestSuite * apSuite, void * apContext); + static void TestCommandHandlerRejectMultipleCommandsWhenHandlerOnlySupportsOne(nlTestSuite * apSuite, void * apContext); + static void TestCommandHandlerAcceptMultipleCommands(nlTestSuite * apSuite, void * apContext); #if CONFIG_BUILD_FOR_HOST_UNIT_TEST static void TestCommandHandlerReleaseWithExchangeClosed(nlTestSuite * apSuite, void * apContext); @@ -1100,6 +1107,8 @@ void TestCommandInteraction::TestCommandHandlerInvalidMessageAsync(nlTestSuite * // Decrease CommandHandler refcount and send response asyncCommandHandle = nullptr; + // Prevent breaking other tests. + asyncCommand = false; ctx.DrainAndServiceIO(); NL_TEST_ASSERT(apSuite, @@ -1360,7 +1369,7 @@ void TestCommandInteraction::TestCommandSenderAbruptDestruction(nlTestSuite * ap NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); } -void TestCommandInteraction::TestCommandHandlerRejectMultipleCommands(nlTestSuite * apSuite, void * apContext) +void TestCommandInteraction::TestCommandHandlerRejectMultipleIdenticalCommands(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); CHIP_ERROR err = CHIP_NO_ERROR; @@ -1416,6 +1425,236 @@ void TestCommandInteraction::TestCommandHandlerRejectMultipleCommands(nlTestSuit NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } +void TestCommandInteraction::TestCommandHandlerRejectsMultipleCommandsWithIdenticalCommandRef(nlTestSuite * apSuite, + void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + + isCommandDispatched = false; + mockCommandSenderDelegate.ResetCounter(); + + // Using commandSender to help build afterward we take the buffer to feed into standalone CommandHandler + app::CommandSender commandSender(&mockCommandSenderDelegate, &ctx.GetExchangeManager()); + + size_t numberOfCommandsToSend = 2; + { + CommandPathParams requestCommandPaths[] = { + MakeTestCommandPath(kTestCommandIdWithData), + MakeTestCommandPath(kTestCommandIdCommandSpecificResponse), + }; + + commandSender.AllocateBuffer(); + + // CommandSender does not support sending multiple commands with public API, so we craft a message manaully. + for (size_t i = 0; i < numberOfCommandsToSend; i++) + { + InvokeRequests::Builder & invokeRequests = commandSender.mInvokeRequestBuilder.GetInvokeRequests(); + CommandDataIB::Builder & invokeRequest = invokeRequests.CreateCommandData(); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequests.GetError()); + CommandPathIB::Builder & path = invokeRequest.CreatePath(); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequest.GetError()); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == path.Encode(requestCommandPaths[i])); + NL_TEST_ASSERT(apSuite, + CHIP_NO_ERROR == + invokeRequest.GetWriter()->StartContainer(TLV::ContextTag(CommandDataIB::Tag::kFields), + TLV::kTLVType_Structure, + commandSender.mDataElementContainerType)); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequest.GetWriter()->PutBoolean(chip::TLV::ContextTag(1), true)); + NL_TEST_ASSERT(apSuite, + CHIP_NO_ERROR == invokeRequest.GetWriter()->EndContainer(commandSender.mDataElementContainerType)); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequest.Ref(1)); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequest.EndOfCommandDataIB()); + } + + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == commandSender.mInvokeRequestBuilder.GetInvokeRequests().EndOfInvokeRequests()); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == commandSender.mInvokeRequestBuilder.EndOfInvokeRequestMessage()); + + commandSender.MoveToState(app::CommandSender::State::AddedCommand); + } + + CommandHandler commandHandler(&mockCommandHandlerDelegate); + TestExchangeDelegate delegate; + auto exchange = ctx.NewExchangeToAlice(&delegate, false); + commandHandler.mExchangeCtx.Grab(exchange); + + // Hackery to steal the InvokeRequest buffer from commandSender. + System::PacketBufferHandle commandDatabuf; + err = commandSender.Finalize(commandDatabuf); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + mockCommandHandlerDelegate.ResetCounter(); + commandDispatchedCount = 0; + + InteractionModel::Status status = commandHandler.ProcessInvokeRequest(std::move(commandDatabuf), false); + NL_TEST_ASSERT(apSuite, status == InteractionModel::Status::InvalidAction); + + NL_TEST_ASSERT(apSuite, commandDispatchedCount == 0); + + // + // Ordinarily, the ExchangeContext will close itself on a responder exchange when unwinding back from an + // OnMessageReceived callback and not having sent a subsequent message (as is the case when calling ProcessInvokeRequest + // above, which doesn't actually send back a response in these cases). Since that isn't the case in this artificial + // setup here (where we created a responder exchange that's not responding to anything), we need to explicitly close it + // out. This is not expected in normal application logic. + // + exchange->Close(); +} + +void TestCommandInteraction::TestCommandHandlerRejectMultipleCommandsWhenHandlerOnlySupportsOne(nlTestSuite * apSuite, + void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + + isCommandDispatched = false; + mockCommandSenderDelegate.ResetCounter(); + + // Using commandSender to help build afterward we take the buffer to feed into standalone CommandHandler + app::CommandSender commandSender(&mockCommandSenderDelegate, &ctx.GetExchangeManager()); + + size_t numberOfCommandsToSend = 2; + { + CommandPathParams requestCommandPaths[] = { + MakeTestCommandPath(kTestCommandIdWithData), + MakeTestCommandPath(kTestCommandIdCommandSpecificResponse), + }; + + commandSender.AllocateBuffer(); + + // CommandSender does not support sending multiple commands with public API, so we craft a message manaully. + for (size_t i = 0; i < numberOfCommandsToSend; i++) + { + InvokeRequests::Builder & invokeRequests = commandSender.mInvokeRequestBuilder.GetInvokeRequests(); + CommandDataIB::Builder & invokeRequest = invokeRequests.CreateCommandData(); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequests.GetError()); + CommandPathIB::Builder & path = invokeRequest.CreatePath(); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequest.GetError()); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == path.Encode(requestCommandPaths[i])); + NL_TEST_ASSERT(apSuite, + CHIP_NO_ERROR == + invokeRequest.GetWriter()->StartContainer(TLV::ContextTag(CommandDataIB::Tag::kFields), + TLV::kTLVType_Structure, + commandSender.mDataElementContainerType)); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequest.GetWriter()->PutBoolean(chip::TLV::ContextTag(1), true)); + NL_TEST_ASSERT(apSuite, + CHIP_NO_ERROR == invokeRequest.GetWriter()->EndContainer(commandSender.mDataElementContainerType)); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequest.Ref(static_cast(i))); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequest.EndOfCommandDataIB()); + } + + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == commandSender.mInvokeRequestBuilder.GetInvokeRequests().EndOfInvokeRequests()); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == commandSender.mInvokeRequestBuilder.EndOfInvokeRequestMessage()); + + commandSender.MoveToState(app::CommandSender::State::AddedCommand); + } + + BasicCommandPathRegistry<4> mBasicCommandPathRegistry; + CommandHandler commandHandler(&mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + TestExchangeDelegate delegate; + auto exchange = ctx.NewExchangeToAlice(&delegate, false); + commandHandler.mExchangeCtx.Grab(exchange); + + // Hackery to steal the InvokeRequest buffer from commandSender. + System::PacketBufferHandle commandDatabuf; + err = commandSender.Finalize(commandDatabuf); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + mockCommandHandlerDelegate.ResetCounter(); + sendResponse = true; + commandDispatchedCount = 0; + + InteractionModel::Status status = commandHandler.ProcessInvokeRequest(std::move(commandDatabuf), false); + NL_TEST_ASSERT(apSuite, status == InteractionModel::Status::Success); + + NL_TEST_ASSERT(apSuite, commandDispatchedCount == 2); + + // + // Ordinarily, the ExchangeContext will close itself on a responder exchange when unwinding back from an + // OnMessageReceived callback and not having sent a subsequent message (as is the case when calling ProcessInvokeRequest + // above, which doesn't actually send back a response in these cases). Since that isn't the case in this artificial + // setup here (where we created a responder exchange that's not responding to anything), we need to explicitly close it + // out. This is not expected in normal application logic. + // + exchange->Close(); +} + +void TestCommandInteraction::TestCommandHandlerAcceptMultipleCommands(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + + isCommandDispatched = false; + mockCommandSenderDelegate.ResetCounter(); + + // Using commandSender to help build afterward we take the buffer to feed into standalone CommandHandler + app::CommandSender commandSender(&mockCommandSenderDelegate, &ctx.GetExchangeManager()); + + size_t numberOfCommandsToSend = 2; + { + CommandPathParams requestCommandPaths[] = { + MakeTestCommandPath(kTestCommandIdWithData), + MakeTestCommandPath(kTestCommandIdCommandSpecificResponse), + }; + + commandSender.AllocateBuffer(); + + // CommandSender does not support sending multiple commands with public API, so we craft a message manaully. + for (size_t i = 0; i < numberOfCommandsToSend; i++) + { + InvokeRequests::Builder & invokeRequests = commandSender.mInvokeRequestBuilder.GetInvokeRequests(); + CommandDataIB::Builder & invokeRequest = invokeRequests.CreateCommandData(); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequests.GetError()); + CommandPathIB::Builder & path = invokeRequest.CreatePath(); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequest.GetError()); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == path.Encode(requestCommandPaths[i])); + NL_TEST_ASSERT(apSuite, + CHIP_NO_ERROR == + invokeRequest.GetWriter()->StartContainer(TLV::ContextTag(CommandDataIB::Tag::kFields), + TLV::kTLVType_Structure, + commandSender.mDataElementContainerType)); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequest.GetWriter()->PutBoolean(chip::TLV::ContextTag(1), true)); + NL_TEST_ASSERT(apSuite, + CHIP_NO_ERROR == invokeRequest.GetWriter()->EndContainer(commandSender.mDataElementContainerType)); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequest.Ref(static_cast(i))); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == invokeRequest.EndOfCommandDataIB()); + } + + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == commandSender.mInvokeRequestBuilder.GetInvokeRequests().EndOfInvokeRequests()); + NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == commandSender.mInvokeRequestBuilder.EndOfInvokeRequestMessage()); + + commandSender.MoveToState(app::CommandSender::State::AddedCommand); + } + + BasicCommandPathRegistry<4> mBasicCommandPathRegistry; + CommandHandler commandHandler(&mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + TestExchangeDelegate delegate; + auto exchange = ctx.NewExchangeToAlice(&delegate, false); + commandHandler.mExchangeCtx.Grab(exchange); + + // Hackery to steal the InvokeRequest buffer from commandSender. + System::PacketBufferHandle commandDatabuf; + err = commandSender.Finalize(commandDatabuf); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + mockCommandHandlerDelegate.ResetCounter(); + commandDispatchedCount = 0; + + InteractionModel::Status status = commandHandler.ProcessInvokeRequest(std::move(commandDatabuf), false); + NL_TEST_ASSERT(apSuite, status == InteractionModel::Status::Success); + + NL_TEST_ASSERT(apSuite, commandDispatchedCount == 2); + + // + // Ordinarily, the ExchangeContext will close itself on a responder exchange when unwinding back from an + // OnMessageReceived callback and not having sent a subsequent message (as is the case when calling ProcessInvokeRequest + // above, which doesn't actually send back a response in these cases). Since that isn't the case in this artificial + // setup here (where we created a responder exchange that's not responding to anything), we need to explicitly close it + // out. This is not expected in normal application logic. + // + exchange->Close(); +} + #if CONFIG_BUILD_FOR_HOST_UNIT_TEST // // This test needs a special unit-test only API being exposed in ExchangeContext to be able to correctly simulate @@ -1472,7 +1711,11 @@ const nlTest sTests[] = NL_TEST_DEF("TestCommandHandlerWithSendSimpleStatusCode", chip::app::TestCommandInteraction::TestCommandHandlerWithSendSimpleStatusCode), NL_TEST_DEF("TestCommandHandlerWithProcessReceivedNotExistCommand", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedNotExistCommand), NL_TEST_DEF("TestCommandHandlerWithProcessReceivedEmptyDataMsg", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg), - NL_TEST_DEF("TestCommandHandlerRejectMultipleCommands", chip::app::TestCommandInteraction::TestCommandHandlerRejectMultipleCommands), + NL_TEST_DEF("TestCommandHandlerRejectMultipleIdenticalCommands", chip::app::TestCommandInteraction::TestCommandHandlerRejectMultipleIdenticalCommands), + NL_TEST_DEF("TestCommandHandlerRejectsMultipleCommandsWithIdenticalCommandRef", chip::app::TestCommandInteraction::TestCommandHandlerRejectsMultipleCommandsWithIdenticalCommandRef), + NL_TEST_DEF("TestCommandHandlerRejectMultipleCommandsWhenHandlerOnlySupportsOne", chip::app::TestCommandInteraction::TestCommandHandlerRejectMultipleCommandsWhenHandlerOnlySupportsOne), + NL_TEST_DEF("TestCommandHandlerAcceptMultipleCommands", chip::app::TestCommandInteraction::TestCommandHandlerAcceptMultipleCommands), + #if CONFIG_BUILD_FOR_HOST_UNIT_TEST NL_TEST_DEF("TestCommandHandlerReleaseWithExchangeClosed", chip::app::TestCommandInteraction::TestCommandHandlerReleaseWithExchangeClosed), From 10f14cadf593eafbf7fa024895df3b076bd2b391 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Fri, 24 Nov 2023 13:59:12 +0000 Subject: [PATCH 10/22] Empty-Commit From 5d9eebfd0c5a5772223f12ac13d300de647a1e02 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Fri, 24 Nov 2023 14:01:19 +0000 Subject: [PATCH 11/22] Fix CI --- src/app/tests/TestCommandInteraction.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 65e09d38a56df0..6dac34f35aa370 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -68,7 +68,7 @@ void CheckForInvalidAction(nlTestSuite * apSuite, chip::Test::MessageCapturer & namespace chip { namespace { -bool isCommandDispatched = false; +bool isCommandDispatched = false; size_t commandDispatchedCount = 0; bool sendResponse = true; From dd197de0f01b57f83074d0c1de732359dc93fff0 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Fri, 24 Nov 2023 23:07:14 +0000 Subject: [PATCH 12/22] Address PR comments --- src/app/CommandHandler.cpp | 3 +- src/app/CommandHandler.h | 38 ++++++++++++++++++------ src/app/CommandPathRegistry.h | 10 ++++--- src/app/tests/TestCommandInteraction.cpp | 6 ++-- 4 files changed, 41 insertions(+), 16 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index abaa5fb790c3f1..a61e6f3c479d04 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -45,7 +45,8 @@ using Status = Protocols::InteractionModel::Status; CommandHandler::CommandHandler(Callback * apCallback) : mExchangeCtx(*this), mpCallback(apCallback), mSuppressResponse(false) {} -CommandHandler::CommandHandler(Callback * apCallback, CommandPathRegistry * apCommandPathRegistry) : CommandHandler(apCallback) +CommandHandler::CommandHandler(TestOnlyMarker aTestMarker, Callback * apCallback, CommandPathRegistry * apCommandPathRegistry) : + CommandHandler(apCallback) { mMaxPathsPerInvoke = apCommandPathRegistry->MaxSize(); mCommandPathRegistry = apCommandPathRegistry; diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 9a2af19cb8fa9f..d37362d6b09061 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -153,6 +153,10 @@ class CommandHandler : public Messaging::ExchangeDelegate uint32_t mMagic = 0; }; + class TestOnlyMarker + { + }; + /* * Constructor. * @@ -164,9 +168,9 @@ class CommandHandler : public Messaging::ExchangeDelegate * Constructor to override number of supported paths per invoke. * * The callback and command path registry passed in has to outlive this CommandHandler object. - * Created for testing purposes. + * For testing purposes. */ - CommandHandler(Callback * apCallback, CommandPathRegistry * mCommandPathRegistry); + CommandHandler(TestOnlyMarker aTestMarker, Callback * apCallback, CommandPathRegistry * apCommandPathRegistry); /* * Main entrypoint for this class to handle an invoke request. @@ -184,9 +188,8 @@ class CommandHandler : public Messaging::ExchangeDelegate /** * Checks that all CommandDataIB within InvokeRequests is correct as per spec. * - * This also build a lookup table that is multipurpose. It helps validate that all elements - * in the invoke request are unique, but also used when later on when populating - * elements of the InvokeResponse. + * This also builds a registry that to ensure that all commands can be responded + * to with the data required as per spec. */ CHIP_ERROR ValidateInvokeRequestsAndBuildRegistry(TLV::TLVReader & invokeRequestsReader); @@ -211,8 +214,9 @@ class CommandHandler : public Messaging::ExchangeDelegate Protocols::InteractionModel::Status ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke); /** - * This will add a new CommandDataIB element into InvokeResponses. It will put the - * aResponseCommandPath into CommandPath element within CommandDataIB. + * This will add a new CommandDataIB element into InvokeResponses for the associated + * aRequestCommandPath. This adds up until the `CommandFields` element within the + * `CommandDataIB`. * * This call will fail if CommandHandler is already in the middle of building/sending * InvokeResponseMessage. @@ -237,6 +241,21 @@ class CommandHandler : public Messaging::ExchangeDelegate * * @param [in] aEndDataStruct end the TLV container for the CommandFields element within * CommandDataIB. + * + * @return CHIP_ERROR_INCORRECT_STATE + * If device has not previously successfully called + * `PrepareInvokeResponseCommand`. + * @retval CHIP_ERROR_BUFFER_TOO_SMALL + * If writing the values needed to finish the InvokeReponseIB + * with the current contents of the InvokeResponseMessage + * would exceed the limit. When this error occurs, it is possible + * we have already closed some of the IB Builders that were + * previously started in `PrepareInvokeResponseCommand`. + * @return CHIP_ERROR_NO_MEMORY + * If TLVWriter attempted to allocate an output buffer failed due to + * lack of memory. + * @return other Other TLVWriter related errors. Typically occurs if + * `GetCommandDataIBTLVWriter()` was called and used incorrectly. */ CHIP_ERROR FinishCommand(bool aEndDataStruct = true); @@ -488,9 +507,9 @@ class CommandHandler : public Messaging::ExchangeDelegate */ void SetGroupRequest(bool isGroupRequest) { mGroupRequest = isGroupRequest; } - CommandPathRegistry & GetCommandPathRegistry() { return *mCommandPathRegistry; } + CommandPathRegistry & GetCommandPathRegistry() const { return *mCommandPathRegistry; } - size_t MaxPathsPerInvoke() { return mMaxPathsPerInvoke; } + size_t MaxPathsPerInvoke() const { return mMaxPathsPerInvoke; } Messaging::ExchangeHolder mExchangeCtx; Callback * mpCallback = nullptr; @@ -501,6 +520,7 @@ class CommandHandler : public Messaging::ExchangeDelegate chip::System::PacketBufferTLVWriter mCommandMessageWriter; TLV::TLVWriter mBackupWriter; size_t mMaxPathsPerInvoke = CHIP_CONFIG_MAX_PATHS_PER_INVOKE; + // TODO Allow flexibility in registration. BasicCommandPathRegistry mBasicCommandPathRegistry; CommandPathRegistry * mCommandPathRegistry = &mBasicCommandPathRegistry; Optional mRefForResponse; diff --git a/src/app/CommandPathRegistry.h b/src/app/CommandPathRegistry.h index c95014cfa67d17..6b7af76ad466f5 100644 --- a/src/app/CommandPathRegistry.h +++ b/src/app/CommandPathRegistry.h @@ -36,7 +36,8 @@ struct CommandPathRegistryEntry class CommandPathRegistry { public: - virtual ~CommandPathRegistry(){}; + virtual ~CommandPathRegistry() = default; + virtual Optional Find(const ConcreteCommandPath & requestPath) const = 0; virtual Optional GetFirstEntry() const = 0; virtual CHIP_ERROR Add(const ConcreteCommandPath & requestPath, const Optional & ref) = 0; @@ -58,8 +59,6 @@ template class BasicCommandPathRegistry : public CommandPathRegistry { public: - ~BasicCommandPathRegistry() override{}; - Optional Find(const ConcreteCommandPath & requestPath) const override { for (size_t i = 0; i < mCount; i++) @@ -93,7 +92,10 @@ class BasicCommandPathRegistry : public CommandPathRegistry { return CHIP_ERROR_DUPLICATE_KEY_ID; } - if (mTable[i].ref.HasValue() && mTable[i].ref == ref) + // No need to check if either has value. This is because if there is more than + // 1 entry in the table expectation is to have all entirely unique ref values + // so duplicate optional would mean we would want to error out. + if (mTable[i].ref == ref) { return CHIP_ERROR_DUPLICATE_KEY_ID; } diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 6dac34f35aa370..9b57d9e702dfba 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -83,6 +83,8 @@ constexpr CommandId kTestCommandIdWithData = 4; constexpr CommandId kTestCommandIdNoData = 5; constexpr CommandId kTestCommandIdCommandSpecificResponse = 6; constexpr CommandId kTestNonExistCommandId = 0; + +const app::CommandHandler::TestOnlyMarker kThisIsForTestOnly; } // namespace namespace app { @@ -1550,7 +1552,7 @@ void TestCommandInteraction::TestCommandHandlerRejectMultipleCommandsWhenHandler } BasicCommandPathRegistry<4> mBasicCommandPathRegistry; - CommandHandler commandHandler(&mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + CommandHandler commandHandler(kThisIsForTestOnly, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); TestExchangeDelegate delegate; auto exchange = ctx.NewExchangeToAlice(&delegate, false); commandHandler.mExchangeCtx.Grab(exchange); @@ -1627,7 +1629,7 @@ void TestCommandInteraction::TestCommandHandlerAcceptMultipleCommands(nlTestSuit } BasicCommandPathRegistry<4> mBasicCommandPathRegistry; - CommandHandler commandHandler(&mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + CommandHandler commandHandler(kThisIsForTestOnly, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); TestExchangeDelegate delegate; auto exchange = ctx.NewExchangeToAlice(&delegate, false); commandHandler.mExchangeCtx.Grab(exchange); From 8def68224b20acbec686307dbcc333d953a80914 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Fri, 24 Nov 2023 23:16:21 +0000 Subject: [PATCH 13/22] Fix comment --- src/app/CommandHandler.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index d37362d6b09061..7802672eed4e35 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -214,8 +214,8 @@ class CommandHandler : public Messaging::ExchangeDelegate Protocols::InteractionModel::Status ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke); /** - * This will add a new CommandDataIB element into InvokeResponses for the associated - * aRequestCommandPath. This adds up until the `CommandFields` element within the + * This adds a new CommandDataIB element into InvokeResponses for the associated + * aRequestCommandPath. This adds up until the `CommandFields` element within * `CommandDataIB`. * * This call will fail if CommandHandler is already in the middle of building/sending From 88783558695dc6bf4d38201629064462607d4973 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 28 Nov 2023 00:47:16 +0000 Subject: [PATCH 14/22] Address PR Comments --- src/app/CommandHandler.cpp | 25 +-- src/app/CommandHandler.h | 13 +- src/app/CommandPathRegistry.h | 2 +- src/app/tests/BUILD.gn | 1 + .../tests/TestBasicCommandPathRegistry.cpp | 147 ++++++++++++++++++ 5 files changed, 171 insertions(+), 17 deletions(-) create mode 100644 src/app/tests/TestBasicCommandPathRegistry.cpp diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index a61e6f3c479d04..eafdd3b0e9c663 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -111,7 +111,7 @@ CHIP_ERROR CommandHandler::ValidateInvokeRequestsAndBuildRegistry(TLV::TLVReader size_t commandCount = 0; bool commandRefExpected = false; - TLV::Utilities::Count(invokeRequestsReader, commandCount, false /* recurse */); + ReturnErrorOnFailure(TLV::Utilities::Count(invokeRequestsReader, commandCount, false /* recurse */)); // If this is a GroupRequest the only thing to check is that there is only one // CommandDataIB. @@ -135,14 +135,14 @@ CHIP_ERROR CommandHandler::ValidateInvokeRequestsAndBuildRegistry(TLV::TLVReader CommandDataIB::Parser commandData; ReturnErrorOnFailure(commandData.Init(invokeRequestsReader)); - // First validating that we can get a ConcreteCommandPath. + // First validate that we can get a ConcreteCommandPath. CommandPathIB::Parser commandPath; ConcreteCommandPath concretePath(0, 0, 0); ReturnErrorOnFailure(commandData.GetPath(&commandPath)); ReturnErrorOnFailure(commandPath.GetConcreteCommandPath(concretePath)); - // Following section is to grab the CommandRef if avaiable and perform - // any validation on it if we expect there to be a CommandRef + // Grab the CommandRef if there is one, and validate that it's there when it + // has to be. Optional commandRef; uint16_t ref; err = commandData.GetRef(&ref); @@ -153,11 +153,11 @@ CHIP_ERROR CommandHandler::ValidateInvokeRequestsAndBuildRegistry(TLV::TLVReader } if (err == CHIP_NO_ERROR) { - commandRef = MakeOptional(ref); + commandRef.SetValue(ref); } // Adding can fail if concretePath is not unique, or if commandRef is a value - // and is not unique, or if we have already added more paths then we support. + // and is not unique, or if we have already added more paths than we support. ReturnErrorOnFailure(GetCommandPathRegistry().Add(concretePath, commandRef)); } @@ -173,7 +173,6 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa { CHIP_ERROR err = CHIP_NO_ERROR; System::PacketBufferTLVReader reader; - TLV::TLVReader invokeRequestsReader; InvokeRequestMessage::Parser invokeRequestMessage; InvokeRequests::Parser invokeRequests; reader.Init(std::move(payload)); @@ -190,10 +189,15 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa VerifyOrReturnError(invokeRequestMessage.GetTimedRequest(&mTimedRequest) == CHIP_NO_ERROR, Status::InvalidAction); VerifyOrReturnError(invokeRequestMessage.GetInvokeRequests(&invokeRequests) == CHIP_NO_ERROR, Status::InvalidAction); VerifyOrReturnError(mTimedRequest == isTimedInvoke, Status::UnsupportedAccess); - invokeRequests.GetReader(&invokeRequestsReader); - VerifyOrReturnError(ValidateInvokeRequestsAndBuildRegistry(invokeRequestsReader) == CHIP_NO_ERROR, Status::InvalidAction); - // Reset the reader after validation, so that we can iterate the commands to actually process them. + { + TLV::TLVReader validationInvokeRequestsReader; + invokeRequests.GetReader(&validationInvokeRequestsReader); + VerifyOrReturnError(ValidateInvokeRequestsAndBuildRegistry(validationInvokeRequestsReader) == CHIP_NO_ERROR, + Status::InvalidAction); + } + + TLV::TLVReader invokeRequestsReader; invokeRequests.GetReader(&invokeRequestsReader); while (CHIP_NO_ERROR == (err = invokeRequestsReader.Next())) @@ -592,7 +596,6 @@ CHIP_ERROR CommandHandler::PrepareCommand(const ConcreteCommandPath & aResponseC "Seemingly device supports batch commands, but is calling the deprecated PrepareCommand API"); auto commandPathRegistryEntry = GetCommandPathRegistry().GetFirstEntry(); - VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); VerifyOrReturnValue(commandPathRegistryEntry.HasValue(), CHIP_ERROR_INCORRECT_STATE); return PrepareInvokeResponseCommand(commandPathRegistryEntry.Value(), aResponseCommandPath, aStartDataStruct); diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 7802672eed4e35..a0697fd6f78b4e 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -218,8 +218,9 @@ class CommandHandler : public Messaging::ExchangeDelegate * aRequestCommandPath. This adds up until the `CommandFields` element within * `CommandDataIB`. * - * This call will fail if CommandHandler is already in the middle of building/sending - * InvokeResponseMessage. + * This call will fail if CommandHandler is already in the middle of building a + * CommandStatusIB or CommandDataID (ei something has called Perpare*, without + * calling Finish*), or is already sending InvokeResponseMessage. * * Upon success, the caller is expected to call `FinishCommand` once they have added * all Data into Fields element of CommandDataIB. @@ -261,10 +262,11 @@ class CommandHandler : public Messaging::ExchangeDelegate /** * This will add a new CommandStatusIB element into InvokeResponses. It will put the - * aCommandPath into CommandPath element within CommandStatusIB. + * aCommandPath into the CommandPath element within CommandStatusIB. * - * This call will fail if CommandHandler is already in the middle of building/sending - * InvokeResponseMessage. + * This call will fail if CommandHandler is already in the middle of building a + * CommandStatusIB or CommandDataID (ei something has called Perpare*, without + * calling Finish*), or is already sending InvokeResponseMessage. * * Upon success, the caller is expected to call `FinishStatus` once they have added * data into Fields element of CommandStatusIB. @@ -520,6 +522,7 @@ class CommandHandler : public Messaging::ExchangeDelegate chip::System::PacketBufferTLVWriter mCommandMessageWriter; TLV::TLVWriter mBackupWriter; size_t mMaxPathsPerInvoke = CHIP_CONFIG_MAX_PATHS_PER_INVOKE; + // TODO(#30453): See if we can reduce this size for the default cases // TODO Allow flexibility in registration. BasicCommandPathRegistry mBasicCommandPathRegistry; CommandPathRegistry * mCommandPathRegistry = &mBasicCommandPathRegistry; diff --git a/src/app/CommandPathRegistry.h b/src/app/CommandPathRegistry.h index 6b7af76ad466f5..0d914ef1de3348 100644 --- a/src/app/CommandPathRegistry.h +++ b/src/app/CommandPathRegistry.h @@ -82,7 +82,7 @@ class BasicCommandPathRegistry : public CommandPathRegistry CHIP_ERROR Add(const ConcreteCommandPath & requestPath, const Optional & ref) override { - if (mCount > N) + if (mCount >= N) { return CHIP_ERROR_NO_MEMORY; } diff --git a/src/app/tests/BUILD.gn b/src/app/tests/BUILD.gn index 78e3bdb229e49c..4b20d3deabf485 100644 --- a/src/app/tests/BUILD.gn +++ b/src/app/tests/BUILD.gn @@ -133,6 +133,7 @@ chip_test_suite_using_nltest("tests") { "TestClusterInfo.cpp", "TestCommandInteraction.cpp", "TestCommandPathParams.cpp", + "TestBasicCommandPathRegistry.cpp", "TestDataModelSerialization.cpp", "TestDefaultOTARequestorStorage.cpp", "TestEventLoggingNoUTCTime.cpp", diff --git a/src/app/tests/TestBasicCommandPathRegistry.cpp b/src/app/tests/TestBasicCommandPathRegistry.cpp new file mode 100644 index 00000000000000..0a0b397e6d557b --- /dev/null +++ b/src/app/tests/TestBasicCommandPathRegistry.cpp @@ -0,0 +1,147 @@ +/* + * + * 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. + */ + +#include +#include + +#include + +namespace chip { +namespace app { +namespace TestBasicCommandPathRegistry { +namespace { + +size_t constexpr kQuickTestSize = 10; + +} // namespace + +void TestAddingSameConcretePath(nlTestSuite * apSuite, void * apContext) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + BasicCommandPathRegistry basicCommandPathRegistry; + + ConcreteCommandPath concretePath(0, 0, 0); + Optional commandRef; + uint16_t commandRefValue = 0; + + size_t idx = 0; + for (idx = 0; idx < kQuickTestSize && err == CHIP_NO_ERROR; idx++) + { + commandRef.SetValue(commandRefValue); + commandRefValue++; + err = basicCommandPathRegistry.Add(concretePath, commandRef); + } + + NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_DUPLICATE_KEY_ID); + NL_TEST_ASSERT(apSuite, basicCommandPathRegistry.Count() == 1); +} + +void TestAddingSameCommandRef(nlTestSuite * apSuite, void * apContext) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + BasicCommandPathRegistry basicCommandPathRegistry; + + Optional commandRef; + commandRef.SetValue(0); + + uint16_t endpointValue = 0; + + size_t idx = 0; + for (idx = 0; idx < kQuickTestSize && err == CHIP_NO_ERROR; idx++) + { + ConcreteCommandPath concretePath(endpointValue, 0, 0); + endpointValue++; + err = basicCommandPathRegistry.Add(concretePath, commandRef); + } + + NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_DUPLICATE_KEY_ID); + NL_TEST_ASSERT(apSuite, basicCommandPathRegistry.Count() == 1); +} + +void TestAddingMaxNumberOfEntries(nlTestSuite * apSuite, void * apContext) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + BasicCommandPathRegistry basicCommandPathRegistry; + + Optional commandRef; + uint16_t commandRefAndEndpointValue = 0; + + size_t idx = 0; + for (idx = 0; idx < kQuickTestSize && err == CHIP_NO_ERROR; idx++) + { + ConcreteCommandPath concretePath(commandRefAndEndpointValue, 0, 0); + commandRef.SetValue(commandRefAndEndpointValue); + commandRefAndEndpointValue++; + err = basicCommandPathRegistry.Add(concretePath, commandRef); + } + + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(apSuite, basicCommandPathRegistry.Count() == kQuickTestSize); +} + +void TestAddingTooManyEntries(nlTestSuite * apSuite, void * apContext) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + BasicCommandPathRegistry basicCommandPathRegistry; + size_t maxPlusOne = kQuickTestSize + 1; + + Optional commandRef; + uint16_t commandRefAndEndpointValue = 0; + + size_t idx = 0; + for (idx = 0; idx < maxPlusOne && err == CHIP_NO_ERROR; idx++) + { + ConcreteCommandPath concretePath(commandRefAndEndpointValue, 0, 0); + commandRef.SetValue(commandRefAndEndpointValue); + commandRefAndEndpointValue++; + err = basicCommandPathRegistry.Add(concretePath, commandRef); + } + + NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_NO_MEMORY); + NL_TEST_ASSERT(apSuite, basicCommandPathRegistry.Count() == kQuickTestSize); +} + +} // namespace TestBasicCommandPathRegistry +} // namespace app +} // namespace chip + +namespace { +// clang-format off +const nlTest sTests[] = +{ + NL_TEST_DEF("TestAddingSameConcretePath", chip::app::TestBasicCommandPathRegistry::TestAddingSameConcretePath), + NL_TEST_DEF("TestAddingSameCommandRef", chip::app::TestBasicCommandPathRegistry::TestAddingSameCommandRef), + NL_TEST_DEF("TestAddingMaxNumberOfEntries", chip::app::TestBasicCommandPathRegistry::TestAddingMaxNumberOfEntries), + NL_TEST_DEF("TestAddingTooManyEntries", chip::app::TestBasicCommandPathRegistry::TestAddingTooManyEntries), + + NL_TEST_SENTINEL() +}; +// clang-format on + +} // namespace + +int TestBasicCommandPathRegistry() +{ + nlTestSuite theSuite = { "CommandPathRegistry", &sTests[0], nullptr, nullptr }; + + nlTestRunner(&theSuite, nullptr); + + return (nlTestRunnerStats(&theSuite)); +} + +CHIP_REGISTER_TEST_SUITE(TestBasicCommandPathRegistry) From 5b753aecc6ca41d8f078c73ef596442d12dc166d Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 28 Nov 2023 01:08:38 +0000 Subject: [PATCH 15/22] Restyle --- src/app/tests/BUILD.gn | 2 +- src/app/tests/TestBasicCommandPathRegistry.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/tests/BUILD.gn b/src/app/tests/BUILD.gn index 4b20d3deabf485..577e9c69cdcffb 100644 --- a/src/app/tests/BUILD.gn +++ b/src/app/tests/BUILD.gn @@ -128,12 +128,12 @@ chip_test_suite_using_nltest("tests") { "TestAttributePersistenceProvider.cpp", "TestAttributeValueDecoder.cpp", "TestAttributeValueEncoder.cpp", + "TestBasicCommandPathRegistry.cpp", "TestBindingTable.cpp", "TestBuilderParser.cpp", "TestClusterInfo.cpp", "TestCommandInteraction.cpp", "TestCommandPathParams.cpp", - "TestBasicCommandPathRegistry.cpp", "TestDataModelSerialization.cpp", "TestDefaultOTARequestorStorage.cpp", "TestEventLoggingNoUTCTime.cpp", diff --git a/src/app/tests/TestBasicCommandPathRegistry.cpp b/src/app/tests/TestBasicCommandPathRegistry.cpp index 0a0b397e6d557b..61e859b95498f0 100644 --- a/src/app/tests/TestBasicCommandPathRegistry.cpp +++ b/src/app/tests/TestBasicCommandPathRegistry.cpp @@ -128,7 +128,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestAddingSameCommandRef", chip::app::TestBasicCommandPathRegistry::TestAddingSameCommandRef), NL_TEST_DEF("TestAddingMaxNumberOfEntries", chip::app::TestBasicCommandPathRegistry::TestAddingMaxNumberOfEntries), NL_TEST_DEF("TestAddingTooManyEntries", chip::app::TestBasicCommandPathRegistry::TestAddingTooManyEntries), - + NL_TEST_SENTINEL() }; // clang-format on From 00a075723ed60c62ecba257c5afb3113de17964f Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 28 Nov 2023 01:45:56 +0000 Subject: [PATCH 16/22] Prevent having similar issues with new PrepareCommand --- src/app/CommandHandler.cpp | 4 ++-- src/app/CommandHandler.h | 16 +++++++++++++--- .../network-commissioning.cpp | 12 ++++++++---- src/app/tests/TestCommandInteraction.cpp | 12 ++++++++---- src/app/tests/integration/chip_im_responder.cpp | 3 ++- 5 files changed, 33 insertions(+), 14 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index eafdd3b0e9c663..02801047508532 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -573,10 +573,10 @@ CHIP_ERROR CommandHandler::AddClusterSpecificFailure(const ConcreteCommandPath & return AddStatusInternal(aCommandPath, StatusIB(Status::Failure, aClusterStatus)); } -CHIP_ERROR CommandHandler::PrepareInvokeResponseCommand(const ConcreteCommandPath & aRequestCommandPath, +CHIP_ERROR CommandHandler::PrepareInvokeResponseCommand(const CommandHandler::PrepareParameters & aPrepareParameters, const ConcreteCommandPath & aResponseCommandPath, bool aStartDataStruct) { - auto commandPathRegistryEntry = GetCommandPathRegistry().Find(aRequestCommandPath); + auto commandPathRegistryEntry = GetCommandPathRegistry().Find(aPrepareParameters.mRequestCommandPath); VerifyOrReturnValue(commandPathRegistryEntry.HasValue(), CHIP_ERROR_INCORRECT_STATE); return PrepareInvokeResponseCommand(commandPathRegistryEntry.Value(), aResponseCommandPath, aStartDataStruct); diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index a0697fd6f78b4e..71c0fc271386ae 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -153,6 +153,14 @@ class CommandHandler : public Messaging::ExchangeDelegate uint32_t mMagic = 0; }; + // While this only contains one field for now, this is done in order to future proof if more information + // is needed when calling PrepareInvokeResponseCommand. Previously we wanted to add aRequestCommandPath but + // could not, as code outside of SDK is calling PrepareCommand. Now we can extend as needed a little easier. + struct PrepareParameters + { + ConcreteCommandPath mRequestCommandPath; + }; + class TestOnlyMarker { }; @@ -225,13 +233,14 @@ class CommandHandler : public Messaging::ExchangeDelegate * Upon success, the caller is expected to call `FinishCommand` once they have added * all Data into Fields element of CommandDataIB. * - * @param [in] aRequestCommandPath the concrete path of the command we are responding to. + * @param [in] aPrepareParameters struct containing paramters needs for preparing a command, such as request path. * @param [in] aResponseCommandPath the concrete response path that we are sending to Requester. * @param [in] aStartDataStruct starts the TLV container for the CommandFields element within * CommandDataIB. */ - CHIP_ERROR PrepareInvokeResponseCommand(const ConcreteCommandPath & aRequestCommandPath, + CHIP_ERROR PrepareInvokeResponseCommand(const PrepareParameters & aPrepareParameters, const ConcreteCommandPath & aResponseCommandPath, bool aStartDataStruct = true); + [[deprecated("PrepareCommand now needs the requested command path. Please use PrepareInvokeResponseCommand")]] CHIP_ERROR PrepareCommand(const ConcreteCommandPath & aCommandPath, bool aStartDataStruct = true); @@ -490,8 +499,9 @@ class CommandHandler : public Messaging::ExchangeDelegate // Return early in case of requests targeted to a group, since they should not add a response. VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); + PrepareParameters prepareParams = { aRequestCommandPath }; ConcreteCommandPath path = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, CommandData::GetCommandId() }; - ReturnErrorOnFailure(PrepareInvokeResponseCommand(aRequestCommandPath, path, false)); + ReturnErrorOnFailure(PrepareInvokeResponseCommand(prepareParams, path, false)); TLV::TLVWriter * writer = GetCommandDataIBTLVWriter(); VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE); ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields), aData)); diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp index 9bf5ff00e57501..e75c2d382a0a8e 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.cpp +++ b/src/app/clusters/network-commissioning/network-commissioning.cpp @@ -815,8 +815,10 @@ void Instance::OnFinished(Status status, CharSpan debugText, ThreadScanResponseI size_t scanResponseArrayLength = 0; uint8_t extendedAddressBuffer[Thread::kSizeExtendedPanId]; - SuccessOrExit(err = commandHandle->PrepareInvokeResponseCommand( - mPath, ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id))); + const CommandHandler::PrepareParameters prepareParams = { mPath }; + SuccessOrExit( + err = commandHandle->PrepareInvokeResponseCommand( + prepareParams, 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)); @@ -927,8 +929,10 @@ void Instance::OnFinished(Status status, CharSpan debugText, WiFiScanResponseIte WiFiScanResponse scanResponse; size_t networksEncoded = 0; - SuccessOrExit(err = commandHandle->PrepareInvokeResponseCommand( - mPath, ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id))); + const CommandHandler::PrepareParameters prepareParams = { mPath }; + SuccessOrExit( + err = commandHandle->PrepareInvokeResponseCommand( + prepareParams, 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 9b57d9e702dfba..9feb559837aead 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -165,8 +165,9 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPat } else { + const CommandHandler::PrepareParameters prepareParams = { aRequestCommandPath }; const ConcreteCommandPath responseCommandPath = aRequestCommandPath; - apCommandObj->PrepareInvokeResponseCommand(aRequestCommandPath, responseCommandPath); + apCommandObj->PrepareInvokeResponseCommand(prepareParams, responseCommandPath); chip::TLV::TLVWriter * writer = apCommandObj->GetCommandDataIBTLVWriter(); writer->PutBoolean(chip::TLV::ContextTag(1), true); apCommandObj->FinishCommand(); @@ -490,8 +491,9 @@ void TestCommandInteraction::AddInvokeResponseData(nlTestSuite * apSuite, void * } else { + const CommandHandler::PrepareParameters prepareParams = { requestCommandPath }; ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, aCommandId }; - err = apCommandHandler->PrepareInvokeResponseCommand(requestCommandPath, responseCommandPath); + err = apCommandHandler->PrepareInvokeResponseCommand(prepareParams, responseCommandPath); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); chip::TLV::TLVWriter * writer = apCommandHandler->GetCommandDataIBTLVWriter(); @@ -526,7 +528,8 @@ void TestCommandInteraction::TestCommandHandlerWithWrongState(nlTestSuite * apSu CommandHandlerWithOutstandingCommand commandHandler(&mockCommandHandlerDelegate, requestCommandPath, /* aRef = */ NullOptional); - err = commandHandler.PrepareInvokeResponseCommand(requestCommandPath, responseCommandPath); + const CommandHandler::PrepareParameters prepareParams = { requestCommandPath }; + err = commandHandler.PrepareInvokeResponseCommand(prepareParams, responseCommandPath); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); TestExchangeDelegate delegate; @@ -582,7 +585,8 @@ void TestCommandInteraction::TestCommandHandlerWithSendEmptyCommand(nlTestSuite auto exchange = ctx.NewExchangeToAlice(&delegate, false); commandHandler.mExchangeCtx.Grab(exchange); - err = commandHandler.PrepareInvokeResponseCommand(requestCommandPath, responseCommandPath); + const CommandHandler::PrepareParameters prepareParams = { requestCommandPath }; + err = commandHandler.PrepareInvokeResponseCommand(prepareParams, responseCommandPath); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); err = commandHandler.FinishCommand(); 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 d5a4ca1c4238df..0f373cde76ebaa 100644 --- a/src/app/tests/integration/chip_im_responder.cpp +++ b/src/app/tests/integration/chip_im_responder.cpp @@ -106,7 +106,8 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPat chip::TLV::TLVWriter * writer; - ReturnOnFailure(apCommandObj->PrepareInvokeResponseCommand(aRequestCommandPath, path)); + const CommandHandler::PrepareParameters prepareParams = { aRequestCommandPath }; + ReturnOnFailure(apCommandObj->PrepareInvokeResponseCommand(prepareParams, path)); writer = apCommandObj->GetCommandDataIBTLVWriter(); ReturnOnFailure(writer->Put(chip::TLV::ContextTag(kTestFieldId1), kTestFieldValue1)); From fab472fa3e632cbdc8dce8d82ae806f7bf09f970 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 28 Nov 2023 01:47:59 +0000 Subject: [PATCH 17/22] Restyle --- src/app/tests/TestCommandInteraction.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 9feb559837aead..e2b6380a539ccc 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -166,7 +166,7 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPat else { const CommandHandler::PrepareParameters prepareParams = { aRequestCommandPath }; - const ConcreteCommandPath responseCommandPath = aRequestCommandPath; + const ConcreteCommandPath responseCommandPath = aRequestCommandPath; apCommandObj->PrepareInvokeResponseCommand(prepareParams, responseCommandPath); chip::TLV::TLVWriter * writer = apCommandObj->GetCommandDataIBTLVWriter(); writer->PutBoolean(chip::TLV::ContextTag(1), true); @@ -492,7 +492,7 @@ void TestCommandInteraction::AddInvokeResponseData(nlTestSuite * apSuite, void * else { const CommandHandler::PrepareParameters prepareParams = { requestCommandPath }; - ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, aCommandId }; + ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, aCommandId }; err = apCommandHandler->PrepareInvokeResponseCommand(prepareParams, responseCommandPath); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); From d9647d2fa9e9231f79319ad12ddd1ff8901b8f4f Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 29 Nov 2023 19:01:40 +0000 Subject: [PATCH 18/22] Make API used by others outside of SDK better by having a parameter struct --- src/app/CommandHandler.cpp | 7 +++-- src/app/CommandHandler.h | 29 +++++++++++-------- .../network-commissioning.cpp | 8 ++--- src/app/tests/TestCommandInteraction.cpp | 18 ++++++------ .../tests/integration/chip_im_responder.cpp | 4 +-- 5 files changed, 36 insertions(+), 30 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 02801047508532..06f2ac942f2cd3 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -573,13 +573,14 @@ CHIP_ERROR CommandHandler::AddClusterSpecificFailure(const ConcreteCommandPath & return AddStatusInternal(aCommandPath, StatusIB(Status::Failure, aClusterStatus)); } -CHIP_ERROR CommandHandler::PrepareInvokeResponseCommand(const CommandHandler::PrepareParameters & aPrepareParameters, - const ConcreteCommandPath & aResponseCommandPath, bool aStartDataStruct) +CHIP_ERROR CommandHandler::PrepareInvokeResponseCommand(const ConcreteCommandPath & aResponseCommandPath, + const CommandHandler::InvokeResponseParameters & aPrepareParameters) { auto commandPathRegistryEntry = GetCommandPathRegistry().Find(aPrepareParameters.mRequestCommandPath); VerifyOrReturnValue(commandPathRegistryEntry.HasValue(), CHIP_ERROR_INCORRECT_STATE); - return PrepareInvokeResponseCommand(commandPathRegistryEntry.Value(), aResponseCommandPath, aStartDataStruct); + return PrepareInvokeResponseCommand(commandPathRegistryEntry.Value(), aResponseCommandPath, + aPrepareParameters.mStartOrEndDataStruct); } CHIP_ERROR CommandHandler::PrepareCommand(const ConcreteCommandPath & aResponseCommandPath, bool aStartDataStruct) diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 71c0fc271386ae..9b08e420025de8 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -153,12 +153,18 @@ class CommandHandler : public Messaging::ExchangeDelegate uint32_t mMagic = 0; }; - // While this only contains one field for now, this is done in order to future proof if more information - // is needed when calling PrepareInvokeResponseCommand. Previously we wanted to add aRequestCommandPath but - // could not, as code outside of SDK is calling PrepareCommand. Now we can extend as needed a little easier. - struct PrepareParameters + // Previously we kept adding arguments with default values individually as parameters. This is because there + // is legacy code outside of the SDK that would call PrepareCommand. With the new PrepareInvokeResponseCommand + // replacing PrepareCommand, we took this opportunity to create a new parameter structure to make it easier to + // add new parameters without there needing to be an ever increase parameter list with defaults. + struct InvokeResponseParameters { + InvokeResponseParameters(ConcreteCommandPath aRequestCommandPath, bool aStartOrEndDataStruct = true) : + mRequestCommandPath(aRequestCommandPath), mStartOrEndDataStruct(aStartOrEndDataStruct) + {} + ConcreteCommandPath mRequestCommandPath; + bool mStartOrEndDataStruct; }; class TestOnlyMarker @@ -233,13 +239,11 @@ class CommandHandler : public Messaging::ExchangeDelegate * Upon success, the caller is expected to call `FinishCommand` once they have added * all Data into Fields element of CommandDataIB. * - * @param [in] aPrepareParameters struct containing paramters needs for preparing a command, such as request path. * @param [in] aResponseCommandPath the concrete response path that we are sending to Requester. - * @param [in] aStartDataStruct starts the TLV container for the CommandFields element within - * CommandDataIB. + * @param [in] aPrepareParameters struct containing paramters needs for preparing a command, such as request path. */ - CHIP_ERROR PrepareInvokeResponseCommand(const PrepareParameters & aPrepareParameters, - const ConcreteCommandPath & aResponseCommandPath, bool aStartDataStruct = true); + CHIP_ERROR PrepareInvokeResponseCommand(const ConcreteCommandPath & aResponseCommandPath, + const InvokeResponseParameters & aPrepareParameters); [[deprecated("PrepareCommand now needs the requested command path. Please use PrepareInvokeResponseCommand")]] CHIP_ERROR PrepareCommand(const ConcreteCommandPath & aCommandPath, bool aStartDataStruct = true); @@ -499,9 +503,10 @@ class CommandHandler : public Messaging::ExchangeDelegate // Return early in case of requests targeted to a group, since they should not add a response. VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); - PrepareParameters prepareParams = { aRequestCommandPath }; - ConcreteCommandPath path = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, CommandData::GetCommandId() }; - ReturnErrorOnFailure(PrepareInvokeResponseCommand(prepareParams, path, false)); + InvokeResponseParameters prepareParams(aRequestCommandPath, /* aStartOrEndDataStruct = */ false); + ConcreteCommandPath responsePath = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, + CommandData::GetCommandId() }; + ReturnErrorOnFailure(PrepareInvokeResponseCommand(responsePath, prepareParams)); TLV::TLVWriter * writer = GetCommandDataIBTLVWriter(); VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE); ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields), aData)); diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp index e75c2d382a0a8e..5d9cee930677f8 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.cpp +++ b/src/app/clusters/network-commissioning/network-commissioning.cpp @@ -815,10 +815,10 @@ void Instance::OnFinished(Status status, CharSpan debugText, ThreadScanResponseI size_t scanResponseArrayLength = 0; uint8_t extendedAddressBuffer[Thread::kSizeExtendedPanId]; - const CommandHandler::PrepareParameters prepareParams = { mPath }; + const CommandHandler::InvokeResponseParameters prepareParams(mPath); SuccessOrExit( err = commandHandle->PrepareInvokeResponseCommand( - prepareParams, ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id))); + ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id), prepareParams)); VerifyOrExit((writer = commandHandle->GetCommandDataIBTLVWriter()) != nullptr, err = CHIP_ERROR_INCORRECT_STATE); SuccessOrExit(err = writer->Put(TLV::ContextTag(Commands::ScanNetworksResponse::Fields::kNetworkingStatus), status)); @@ -929,10 +929,10 @@ void Instance::OnFinished(Status status, CharSpan debugText, WiFiScanResponseIte WiFiScanResponse scanResponse; size_t networksEncoded = 0; - const CommandHandler::PrepareParameters prepareParams = { mPath }; + const CommandHandler::InvokeResponseParameters prepareParams(mPath); SuccessOrExit( err = commandHandle->PrepareInvokeResponseCommand( - prepareParams, ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id))); + ConcreteCommandPath(mPath.mEndpointId, NetworkCommissioning::Id, Commands::ScanNetworksResponse::Id), prepareParams)); 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 e2b6380a539ccc..255fd432ca281d 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -165,9 +165,9 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPat } else { - const CommandHandler::PrepareParameters prepareParams = { aRequestCommandPath }; - const ConcreteCommandPath responseCommandPath = aRequestCommandPath; - apCommandObj->PrepareInvokeResponseCommand(prepareParams, responseCommandPath); + const CommandHandler::InvokeResponseParameters prepareParams(aRequestCommandPath); + const ConcreteCommandPath responseCommandPath = aRequestCommandPath; + apCommandObj->PrepareInvokeResponseCommand(responseCommandPath, prepareParams); chip::TLV::TLVWriter * writer = apCommandObj->GetCommandDataIBTLVWriter(); writer->PutBoolean(chip::TLV::ContextTag(1), true); apCommandObj->FinishCommand(); @@ -491,9 +491,9 @@ void TestCommandInteraction::AddInvokeResponseData(nlTestSuite * apSuite, void * } else { - const CommandHandler::PrepareParameters prepareParams = { requestCommandPath }; + const CommandHandler::InvokeResponseParameters prepareParams(requestCommandPath); ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, aCommandId }; - err = apCommandHandler->PrepareInvokeResponseCommand(prepareParams, responseCommandPath); + err = apCommandHandler->PrepareInvokeResponseCommand(responseCommandPath, prepareParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); chip::TLV::TLVWriter * writer = apCommandHandler->GetCommandDataIBTLVWriter(); @@ -528,8 +528,8 @@ void TestCommandInteraction::TestCommandHandlerWithWrongState(nlTestSuite * apSu CommandHandlerWithOutstandingCommand commandHandler(&mockCommandHandlerDelegate, requestCommandPath, /* aRef = */ NullOptional); - const CommandHandler::PrepareParameters prepareParams = { requestCommandPath }; - err = commandHandler.PrepareInvokeResponseCommand(prepareParams, responseCommandPath); + const CommandHandler::InvokeResponseParameters prepareParams(requestCommandPath); + err = commandHandler.PrepareInvokeResponseCommand(responseCommandPath, prepareParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); TestExchangeDelegate delegate; @@ -585,8 +585,8 @@ void TestCommandInteraction::TestCommandHandlerWithSendEmptyCommand(nlTestSuite auto exchange = ctx.NewExchangeToAlice(&delegate, false); commandHandler.mExchangeCtx.Grab(exchange); - const CommandHandler::PrepareParameters prepareParams = { requestCommandPath }; - err = commandHandler.PrepareInvokeResponseCommand(prepareParams, responseCommandPath); + const CommandHandler::InvokeResponseParameters prepareParams(requestCommandPath); + err = commandHandler.PrepareInvokeResponseCommand(responseCommandPath, prepareParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); err = commandHandler.FinishCommand(); 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 0f373cde76ebaa..17d47a06ea803d 100644 --- a/src/app/tests/integration/chip_im_responder.cpp +++ b/src/app/tests/integration/chip_im_responder.cpp @@ -106,8 +106,8 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPat chip::TLV::TLVWriter * writer; - const CommandHandler::PrepareParameters prepareParams = { aRequestCommandPath }; - ReturnOnFailure(apCommandObj->PrepareInvokeResponseCommand(prepareParams, path)); + const CommandHandler::InvokeResponseParameters prepareParams(aRequestCommandPath); + ReturnOnFailure(apCommandObj->PrepareInvokeResponseCommand(path, prepareParams)); writer = apCommandObj->GetCommandDataIBTLVWriter(); ReturnOnFailure(writer->Put(chip::TLV::ContextTag(kTestFieldId1), kTestFieldValue1)); From a4ab8f00ff08c92269570da9ff60f0784bd30584 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 29 Nov 2023 19:03:32 +0000 Subject: [PATCH 19/22] Restyle --- src/app/tests/TestCommandInteraction.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 255fd432ca281d..0e589243026a7a 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -492,7 +492,7 @@ void TestCommandInteraction::AddInvokeResponseData(nlTestSuite * apSuite, void * else { const CommandHandler::InvokeResponseParameters prepareParams(requestCommandPath); - ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, aCommandId }; + ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, aCommandId }; err = apCommandHandler->PrepareInvokeResponseCommand(responseCommandPath, prepareParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); From 8a8ba8595777334427d92118d40db2bd535c3516 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 29 Nov 2023 22:20:47 +0000 Subject: [PATCH 20/22] Address CI Comments --- src/app/CommandHandler.cpp | 4 +++- src/app/CommandHandler.h | 41 ++++++++++++++++++++++++++------------ 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 06f2ac942f2cd3..1b4f8ea92998ba 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -593,7 +593,7 @@ CHIP_ERROR CommandHandler::PrepareCommand(const ConcreteCommandPath & aResponseC // At this point application supports Batch Invoke Commands since CommandPathRegistry has more than 1 entry, // but application is calling the deprecated PrepareCommand. We have no way to determine the associated CommandRef // to put into the InvokeResponse. - VerifyOrDieWithMsg(countOfPathRegistryEntries > 1, DataManagement, + VerifyOrDieWithMsg(countOfPathRegistryEntries == 1, DataManagement, "Seemingly device supports batch commands, but is calling the deprecated PrepareCommand API"); auto commandPathRegistryEntry = GetCommandPathRegistry().GetFirstEntry(); @@ -614,6 +614,8 @@ CHIP_ERROR CommandHandler::PrepareInvokeResponseCommand(const CommandPathRegistr // VerifyOrReturnError(mState == State::Idle || mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE); + // TODO(#30453): See if we can pass this back up the stack so caller can provide this instead of taking up + // space in CommandHanlder. mRefForResponse = apCommandPathRegistryEntry.ref; MoveToState(State::Preparing); diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 9b08e420025de8..17501e4e248c23 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -156,15 +156,25 @@ class CommandHandler : public Messaging::ExchangeDelegate // Previously we kept adding arguments with default values individually as parameters. This is because there // is legacy code outside of the SDK that would call PrepareCommand. With the new PrepareInvokeResponseCommand // replacing PrepareCommand, we took this opportunity to create a new parameter structure to make it easier to - // add new parameters without there needing to be an ever increase parameter list with defaults. + // add new parameters without there needing to be an ever increasing parameter list with defaults. struct InvokeResponseParameters { - InvokeResponseParameters(ConcreteCommandPath aRequestCommandPath, bool aStartOrEndDataStruct = true) : - mRequestCommandPath(aRequestCommandPath), mStartOrEndDataStruct(aStartOrEndDataStruct) + InvokeResponseParameters(ConcreteCommandPath aRequestCommandPath) : + mRequestCommandPath(aRequestCommandPath) {} + InvokeResponseParameters & SetStartOrEndDataStruct(bool aStartOrEndDataStruct) + { + mStartOrEndDataStruct = aStartOrEndDataStruct; + return *this; + } + ConcreteCommandPath mRequestCommandPath; - bool mStartOrEndDataStruct; + /** + * Should the method this is being provided to start/end the TLV container for the CommandFields element + * within CommandDataIB. + */ + bool mStartOrEndDataStruct = true; }; class TestOnlyMarker @@ -200,7 +210,8 @@ class CommandHandler : public Messaging::ExchangeDelegate System::PacketBufferHandle && payload, bool isTimedInvoke); /** - * Checks that all CommandDataIB within InvokeRequests is correct as per spec. + * Checks that all CommandDataIB within InvokeRequests satisfy the spec's general + * constraints for CommandDataIB. * * This also builds a registry that to ensure that all commands can be responded * to with the data required as per spec. @@ -233,14 +244,16 @@ class CommandHandler : public Messaging::ExchangeDelegate * `CommandDataIB`. * * This call will fail if CommandHandler is already in the middle of building a - * CommandStatusIB or CommandDataID (ei something has called Perpare*, without + * CommandStatusIB or CommandDataIB (i.e. something has called Prepare*, without * calling Finish*), or is already sending InvokeResponseMessage. * * Upon success, the caller is expected to call `FinishCommand` once they have added - * all Data into Fields element of CommandDataIB. + * all the fields into the CommandFields element of CommandDataIB. * * @param [in] aResponseCommandPath the concrete response path that we are sending to Requester. - * @param [in] aPrepareParameters struct containing paramters needs for preparing a command, such as request path. + * @param [in] aPrepareParameters struct containing paramters needs for preparing a command. Data + * such as request path, and if method should start the CommandField element within + * CommandDataIB. */ CHIP_ERROR PrepareInvokeResponseCommand(const ConcreteCommandPath & aResponseCommandPath, const InvokeResponseParameters & aPrepareParameters); @@ -254,12 +267,12 @@ class CommandHandler : public Messaging::ExchangeDelegate * Caller must have first successfully called `PrepareInvokeResponseCommand`. * * @param [in] aEndDataStruct end the TLV container for the CommandFields element within - * CommandDataIB. + * CommandDataIB. This should match the boolean passed into Prepare*. * * @return CHIP_ERROR_INCORRECT_STATE * If device has not previously successfully called * `PrepareInvokeResponseCommand`. - * @retval CHIP_ERROR_BUFFER_TOO_SMALL + * @return CHIP_ERROR_BUFFER_TOO_SMALL * If writing the values needed to finish the InvokeReponseIB * with the current contents of the InvokeResponseMessage * would exceed the limit. When this error occurs, it is possible @@ -271,6 +284,8 @@ class CommandHandler : public Messaging::ExchangeDelegate * @return other Other TLVWriter related errors. Typically occurs if * `GetCommandDataIBTLVWriter()` was called and used incorrectly. */ + // TODO(#30453): We should be able to eliminate the chances of OOM issues with reserve. + // This will be completed in a follow up PR. CHIP_ERROR FinishCommand(bool aEndDataStruct = true); /** @@ -278,11 +293,11 @@ class CommandHandler : public Messaging::ExchangeDelegate * aCommandPath into the CommandPath element within CommandStatusIB. * * This call will fail if CommandHandler is already in the middle of building a - * CommandStatusIB or CommandDataID (ei something has called Perpare*, without + * CommandStatusIB or CommandDataIB (i.e. something has called Prepare*, without * calling Finish*), or is already sending InvokeResponseMessage. * - * Upon success, the caller is expected to call `FinishStatus` once they have added - * data into Fields element of CommandStatusIB. + * Upon success, the caller is expected to call `FinishStatus` once they have encoded + * StatusIB. * * @param [in] aCommandPath the concrete path of the command we are responding to. */ From cd632a11fe2aa6b33dc3665fe559cda9ce08768b Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 29 Nov 2023 22:26:17 +0000 Subject: [PATCH 21/22] Restyle --- src/app/CommandHandler.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 17501e4e248c23..e2179c342eeb36 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -159,9 +159,7 @@ class CommandHandler : public Messaging::ExchangeDelegate // add new parameters without there needing to be an ever increasing parameter list with defaults. struct InvokeResponseParameters { - InvokeResponseParameters(ConcreteCommandPath aRequestCommandPath) : - mRequestCommandPath(aRequestCommandPath) - {} + InvokeResponseParameters(ConcreteCommandPath aRequestCommandPath) : mRequestCommandPath(aRequestCommandPath) {} InvokeResponseParameters & SetStartOrEndDataStruct(bool aStartOrEndDataStruct) { @@ -518,7 +516,9 @@ class CommandHandler : public Messaging::ExchangeDelegate // Return early in case of requests targeted to a group, since they should not add a response. VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); - InvokeResponseParameters prepareParams(aRequestCommandPath, /* aStartOrEndDataStruct = */ false); + InvokeResponseParameters prepareParams(aRequestCommandPath); + prepareParams.SetStartOrEndDataStruct(false); + ConcreteCommandPath responsePath = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, CommandData::GetCommandId() }; ReturnErrorOnFailure(PrepareInvokeResponseCommand(responsePath, prepareParams)); From 4a11ebd884d0e5c895ec478a450ecf595097c7eb Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 29 Nov 2023 18:20:31 -0500 Subject: [PATCH 22/22] Apply suggestions from code review Co-authored-by: Boris Zbarsky --- src/app/CommandHandler.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index e2179c342eeb36..037e2c5619f50e 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -169,7 +169,7 @@ class CommandHandler : public Messaging::ExchangeDelegate ConcreteCommandPath mRequestCommandPath; /** - * Should the method this is being provided to start/end the TLV container for the CommandFields element + * Whether the method this is being provided to should start/end the TLV container for the CommandFields element * within CommandDataIB. */ bool mStartOrEndDataStruct = true; @@ -250,7 +250,7 @@ class CommandHandler : public Messaging::ExchangeDelegate * * @param [in] aResponseCommandPath the concrete response path that we are sending to Requester. * @param [in] aPrepareParameters struct containing paramters needs for preparing a command. Data - * such as request path, and if method should start the CommandField element within + * such as request path, and whether this method should start the CommandFields element within * CommandDataIB. */ CHIP_ERROR PrepareInvokeResponseCommand(const ConcreteCommandPath & aResponseCommandPath,