From 44311915659983f67aea7edd25fd4f0724cad1f0 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Fri, 19 Jan 2024 18:14:24 -0500 Subject: [PATCH] Upon running out of space in InvokeResponseMessage for additional responses, allocate another (#31516) Co-authored-by: Boris Zbarsky --- src/app/CommandHandler.cpp | 68 +++++++++- src/app/CommandHandler.h | 154 +++++++++++++++++------ src/app/CommandResponseSender.cpp | 15 ++- src/app/CommandResponseSender.h | 9 +- src/app/tests/TestCommandInteraction.cpp | 153 ++++++++++++++++++++-- 5 files changed, 342 insertions(+), 57 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index cb3c8eeb84edd9..6831a8440606b2 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -86,6 +86,7 @@ CHIP_ERROR CommandHandler::AllocateBuffer() ReturnErrorOnFailure(mInvokeResponseBuilder.GetError()); mBufferAllocated = true; + MoveToState(State::NewResponseMessage); } return CHIP_NO_ERROR; @@ -572,7 +573,7 @@ Status CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aComman return Status::Success; } -CHIP_ERROR CommandHandler::AddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus) +CHIP_ERROR CommandHandler::TryAddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus) { ReturnErrorOnFailure(PrepareStatus(aCommandPath)); CommandStatusIB::Builder & commandStatus = mInvokeResponseBuilder.GetInvokeResponses().GetInvokeResponse().GetStatus(); @@ -583,6 +584,11 @@ CHIP_ERROR CommandHandler::AddStatusInternal(const ConcreteCommandPath & aComman return FinishStatus(); } +CHIP_ERROR CommandHandler::AddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus) +{ + return TryAddingResponse([&]() -> CHIP_ERROR { return TryAddStatusInternal(aCommandPath, aStatus); }); +} + void CommandHandler::AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus, const char * context) { @@ -655,12 +661,22 @@ CHIP_ERROR CommandHandler::PrepareInvokeResponseCommand(const CommandPathRegistr { ReturnErrorOnFailure(AllocateBuffer()); - mInvokeResponseBuilder.Checkpoint(mBackupWriter); - mBackupState = mState; + if (!mInternalCallToAddResponseData && mState == State::AddedCommand) + { + // An attempt is being made to add CommandData InvokeResponse using primitive + // CommandHandler APIs. While not recommended, as this potentially leaves the + // CommandHandler in an incorrect state upon failure, this approach is permitted + // for legacy reasons. To maximize the likelihood of success, particularly when + // handling large amounts of data, we try to obtain a new, completely empty + // InvokeResponseMessage, as the existing one already has space occupied. + ReturnErrorOnFailure(FinalizeInvokeResponseMessageAndPrepareNext()); + } + + CreateBackupForResponseRollback(); // // We must not be in the middle of preparing a command, or having prepared or sent one. // - VerifyOrReturnError(mState == State::Idle || mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mState == State::NewResponseMessage || 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. @@ -711,7 +727,11 @@ 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 || mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mState == State::NewResponseMessage || mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE); + if (mState == State::AddedCommand) + { + CreateBackupForResponseRollback(); + } auto commandPathRegistryEntry = GetCommandPathRegistry().Find(aCommandPath); VerifyOrReturnError(commandPathRegistryEntry.HasValue(), CHIP_ERROR_INCORRECT_STATE); @@ -746,11 +766,26 @@ CHIP_ERROR CommandHandler::FinishStatus() return CHIP_NO_ERROR; } +void CommandHandler::CreateBackupForResponseRollback() +{ + VerifyOrReturn(mState == State::NewResponseMessage || mState == State::AddedCommand); + VerifyOrReturn(mInvokeResponseBuilder.GetInvokeResponses().GetError() == CHIP_NO_ERROR); + VerifyOrReturn(mInvokeResponseBuilder.GetError() == CHIP_NO_ERROR); + mInvokeResponseBuilder.Checkpoint(mBackupWriter); + mBackupState = mState; + mRollbackBackupValid = true; +} + CHIP_ERROR CommandHandler::RollbackResponse() { + VerifyOrReturnError(mRollbackBackupValid, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mState == State::Preparing || mState == State::AddingCommand, CHIP_ERROR_INCORRECT_STATE); + // TODO(#30453): Rollback of mInvokeResponseBuilder should handle resetting + // InvokeResponses. + mInvokeResponseBuilder.GetInvokeResponses().ResetError(); mInvokeResponseBuilder.Rollback(mBackupWriter); MoveToState(mBackupState); + mRollbackBackupValid = false; return CHIP_NO_ERROR; } @@ -801,6 +836,24 @@ CommandHandler::Handle::Handle(CommandHandler * handle) } } +CHIP_ERROR CommandHandler::FinalizeInvokeResponseMessageAndPrepareNext() +{ + ReturnErrorOnFailure(FinalizeInvokeResponseMessage(/* aHasMoreChunks = */ true)); + // After successfully finalizing InvokeResponseMessage, no buffer should remain + // allocated. + VerifyOrDie(!mBufferAllocated); + CHIP_ERROR err = AllocateBuffer(); + if (err != CHIP_NO_ERROR) + { + // TODO(#30453): Improve ResponseDropped calls to occur only when dropping is + // definitively guaranteed. + // Response dropping is not yet definitive as a subsequent call + // to AllocateBuffer might succeed. + mResponseSender.ResponseDropped(); + } + return err; +} + CHIP_ERROR CommandHandler::FinalizeInvokeResponseMessage(bool aHasMoreChunks) { System::PacketBufferHandle packet; @@ -817,6 +870,8 @@ CHIP_ERROR CommandHandler::FinalizeInvokeResponseMessage(bool aHasMoreChunks) ReturnErrorOnFailure(mInvokeResponseBuilder.EndOfInvokeResponseMessage()); ReturnErrorOnFailure(mCommandMessageWriter.Finalize(&packet)); mResponseSender.AddInvokeResponseToSend(std::move(packet)); + mBufferAllocated = false; + mRollbackBackupValid = false; return CHIP_NO_ERROR; } @@ -828,6 +883,9 @@ const char * CommandHandler::GetStateStr() const case State::Idle: return "Idle"; + case State::NewResponseMessage: + return "NewResponseMessage"; + case State::Preparing: return "Preparing"; diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 790a66f5c2d6f5..659828cc7d9eb8 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -41,6 +41,7 @@ #include #include #include +#include #include #include #include @@ -288,27 +289,6 @@ class CommandHandler // This will be completed in a follow up PR. CHIP_ERROR FinishCommand(bool aEndDataStruct = true); - /** - * This will add a new CommandStatusIB element into InvokeResponses. It will put the - * aCommandPath into the CommandPath element within CommandStatusIB. - * - * This call will fail if CommandHandler is already in the middle of building a - * 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 encoded - * StatusIB. - * - * @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`. - */ - CHIP_ERROR FinishStatus(); TLV::TLVWriter * GetCommandDataIBTLVWriter(); /** @@ -319,6 +299,54 @@ class CommandHandler */ FabricIndex GetAccessingFabricIndex() const; + /** + * @brief Best effort to add InvokeResponse to InvokeResponseMessage. + * + * Tries to add response using lambda. Upon failure to add response, attempts + * to rollback the InvokeResponseMessage to a known good state. If failure is due + * to insufficient space in the current InvokeResponseMessage: + * - Finalizes the current InvokeResponseMessage. + * - Allocates a new InvokeResponseMessage. + * - Reattempts to add the InvokeResponse to the new InvokeResponseMessage. + * + * @param [in] addResponseFunction A lambda function responsible for adding the + * response to the current InvokeResponseMessage. + */ + template + CHIP_ERROR TryAddingResponse(Function && addResponseFunction) + { + // Invalidate any existing rollback backups. The addResponseFunction is + // expected to create a new backup during either PrepareInvokeResponseCommand + // or PrepareStatus execution. Direct invocation of + // CreateBackupForResponseRollback is avoided since the buffer used by + // InvokeResponseMessage might not be allocated until a Prepare* function + // is called. + mRollbackBackupValid = false; + CHIP_ERROR err = addResponseFunction(); + if (err == CHIP_NO_ERROR) + { + return CHIP_NO_ERROR; + } + ReturnErrorOnFailure(RollbackResponse()); + // If we failed to add a command due to lack of space in the + // packet, we will make another attempt to add the response using + // an additional InvokeResponseMessage. + if (mState != State::AddedCommand || err != CHIP_ERROR_NO_MEMORY) + { + return err; + } + ReturnErrorOnFailure(FinalizeInvokeResponseMessageAndPrepareNext()); + err = addResponseFunction(); + if (err != CHIP_NO_ERROR) + { + // The return value of RollbackResponse is ignored, as we prioritize + // conveying the error generated by addResponseFunction to the + // caller. + RollbackResponse(); + } + return err; + } + /** * API for adding a data response. The template parameter T is generally * expected to be a ClusterName::Commands::CommandName::Type struct, but any @@ -332,15 +360,24 @@ class CommandHandler template CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData) { - // TryAddResponseData will ensure we are in the correct state when calling AddResponseData. - CHIP_ERROR err = TryAddResponseData(aRequestCommandPath, aData); - if (err != CHIP_NO_ERROR) - { - // The state guarantees that either we can rollback or we don't have to rollback the buffer, so we don't care about the - // return value of RollbackResponse. - RollbackResponse(); - } - return err; + // This method, templated with CommandData, captures all the components needs + // from CommandData with as little code as possible. This in theory should + // reduce compiled code size. + // + // TODO(#30453): Verify the accuracy of the theory outlined below. + // + // Theory on code reduction: Previously, non-essential code was unnecessarily + // templated, leading to compilation and duplication N times. The lambda + // function below mitigates this issue by isolating only the code segments + // that genuinely require templating, thereby minimizing duplicate compiled + // code. + ConcreteCommandPath responsePath = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, + CommandData::GetCommandId() }; + auto encodeCommandDataClosure = [&](TLV::TLVWriter & writer) -> CHIP_ERROR { + return DataModel::Encode(writer, TLV::ContextTag(CommandDataIB::Tag::kFields), aData); + }; + return TryAddingResponse( + [&]() -> CHIP_ERROR { return TryAddResponseData(aRequestCommandPath, responsePath, encodeCommandDataClosure); }); } /** @@ -416,6 +453,7 @@ class CommandHandler enum class State : uint8_t { Idle, ///< Default state that the object starts out in, where no work has commenced + NewResponseMessage, ///< mInvokeResponseBuilder is ready, with no responses added. Preparing, ///< We are prepaing the command or status header. AddingCommand, ///< In the process of adding a command. AddedCommand, ///< A command has been completely encoded and is awaiting transmission. @@ -427,7 +465,14 @@ class CommandHandler const char * GetStateStr() const; /** - * Rollback the state to before encoding the current ResponseData (before calling PrepareCommand / PrepareStatus) + * Create a backup to enable rolling back to the state prior to ResponseData encoding in the event of failure. + */ + void CreateBackupForResponseRollback(); + + /** + * Rollback the state to before encoding the current ResponseData (before calling PrepareInvokeResponseCommand / PrepareStatus) + * + * Requires CreateBackupForResponseRollback to be called at the start of PrepareInvokeResponseCommand / PrepareStatus */ CHIP_ERROR RollbackResponse(); @@ -461,12 +506,34 @@ class CommandHandler */ CHIP_ERROR AllocateBuffer(); + /** + * This will add a new CommandStatusIB element into InvokeResponses. It will put the + * aCommandPath into the CommandPath element within CommandStatusIB. + * + * This call will fail if CommandHandler is already in the middle of building a + * 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 encoded + * StatusIB. + * + * @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`. + */ + CHIP_ERROR FinishStatus(); + CHIP_ERROR PrepareInvokeResponseCommand(const CommandPathRegistryEntry & apCommandPathRegistryEntry, const ConcreteCommandPath & aCommandPath, bool aStartDataStruct); CHIP_ERROR FinalizeLastInvokeResponseMessage() { return FinalizeInvokeResponseMessage(/* aHasMoreChunks = */ false); } - CHIP_ERROR FinalizeIntermediateInvokeResponseMessage() { return FinalizeInvokeResponseMessage(/* aHasMoreChunks = */ true); } + CHIP_ERROR FinalizeInvokeResponseMessageAndPrepareNext(); CHIP_ERROR FinalizeInvokeResponseMessage(bool aHasMoreChunks); @@ -495,6 +562,8 @@ class CommandHandler Protocols::InteractionModel::Status ProcessGroupCommandDataIB(CommandDataIB::Parser & aCommandElement); CHIP_ERROR StartSendingCommandResponses(); + CHIP_ERROR TryAddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus); + CHIP_ERROR AddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus); /** @@ -503,10 +572,13 @@ class CommandHandler * * @param [in] aRequestCommandPath the concrete path of the command we are * responding to. - * @param [in] aData the data for the response. + * @param [in] aResponseCommandPath the concrete command response path. + * @param [in] encodeCommandDataFunction A lambda function responsible for + * encoding the CommandData field. */ - template - CHIP_ERROR TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData) + template + CHIP_ERROR TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, const ConcreteCommandPath & aResponseCommandPath, + Function && encodeCommandDataFunction) { // Return early in case of requests targeted to a group, since they should not add a response. VerifyOrReturnValue(!IsGroupRequest(), CHIP_NO_ERROR); @@ -514,12 +586,11 @@ class CommandHandler InvokeResponseParameters prepareParams(aRequestCommandPath); prepareParams.SetStartOrEndDataStruct(false); - ConcreteCommandPath responsePath = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, - CommandData::GetCommandId() }; - ReturnErrorOnFailure(PrepareInvokeResponseCommand(responsePath, prepareParams)); + ScopedChange internalCallToAddResponse(mInternalCallToAddResponseData, true); + ReturnErrorOnFailure(PrepareInvokeResponseCommand(aResponseCommandPath, prepareParams)); TLV::TLVWriter * writer = GetCommandDataIBTLVWriter(); VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE); - ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields), aData)); + ReturnErrorOnFailure(encodeCommandDataFunction(*writer)); return FinishCommand(/* aEndDataStruct = */ false); } @@ -557,12 +628,17 @@ class CommandHandler State mState = State::Idle; State mBackupState; + ScopedChangeOnly mInternalCallToAddResponseData{ false }; bool mSuppressResponse = false; bool mTimedRequest = false; bool mSentStatusResponse = false; bool mGroupRequest = false; bool mBufferAllocated = false; bool mReserveSpaceForMoreChunkMessages = false; + // TODO(#30453): We should introduce breaking change where calls to add CommandData + // need to use AddResponse, and not CommandHandler primitives directly using + // GetCommandDataIBTLVWriter. + bool mRollbackBackupValid = 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/CommandResponseSender.cpp b/src/app/CommandResponseSender.cpp index 69ab21ea02f2f3..6e69cd45a9538f 100644 --- a/src/app/CommandResponseSender.cpp +++ b/src/app/CommandResponseSender.cpp @@ -88,8 +88,7 @@ CHIP_ERROR CommandResponseSender::StartSendingCommandResponses() // failure, the caller bears the responsibility of closing the exchange. ReturnErrorOnFailure(SendCommandResponse()); - bool moreToSend = !mChunks.IsNull(); - if (moreToSend) + if (HasMoreToSend()) { MoveToState(State::AwaitingStatusResponse); mExchangeCtx->SetDelegate(this); @@ -103,12 +102,18 @@ CHIP_ERROR CommandResponseSender::StartSendingCommandResponses() CHIP_ERROR CommandResponseSender::SendCommandResponse() { - VerifyOrReturnError(!mChunks.IsNull(), CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(HasMoreToSend(), CHIP_ERROR_INCORRECT_STATE); + if (mChunks.IsNull()) + { + VerifyOrReturnError(mReportResponseDropped, CHIP_ERROR_INCORRECT_STATE); + SendStatusResponse(Status::ResourceExhausted); + mReportResponseDropped = false; + return CHIP_NO_ERROR; + } System::PacketBufferHandle commandResponsePayload = mChunks.PopHead(); - bool moreToSend = !mChunks.IsNull(); Messaging::SendFlags sendFlag = Messaging::SendMessageFlags::kNone; - if (moreToSend) + if (HasMoreToSend()) { sendFlag = Messaging::SendMessageFlags::kExpectResponse; mExchangeCtx->UseSuggestedResponseTimeout(app::kExpectedIMProcessingTime); diff --git a/src/app/CommandResponseSender.h b/src/app/CommandResponseSender.h index 9148f4f9df0508..0954a3e2ee0687 100644 --- a/src/app/CommandResponseSender.h +++ b/src/app/CommandResponseSender.h @@ -123,6 +123,11 @@ class CommandResponseSender : public Messaging::ExchangeDelegate mChunks.AddToEnd(std::move(aPacket)); } + /** + * @brief Called to indicate that response was dropped + */ + void ResponseDropped() { mReportResponseDropped = true; } + /** * @brief Registers a callback to be invoked when CommandResponseSender has finished sending responses. */ @@ -144,6 +149,7 @@ class CommandResponseSender : public Messaging::ExchangeDelegate const char * GetStateStr() const; CHIP_ERROR SendCommandResponse(); + bool HasMoreToSend() { return !mChunks.IsNull() || mReportResponseDropped; } void Close(); // A list of InvokeResponseMessages to be sent out by CommandResponseSender. @@ -153,7 +159,8 @@ class CommandResponseSender : public Messaging::ExchangeDelegate Messaging::ExchangeHolder mExchangeCtx; State mState = State::ReadyForInvokeResponses; - bool mCloseCalled = false; + bool mCloseCalled = false; + bool mReportResponseDropped = false; }; } // namespace app diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index cf0dd04b935a7b..02b4b01b116a9f 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -82,6 +82,7 @@ constexpr ClusterId kTestClusterId = 3; constexpr CommandId kTestCommandIdWithData = 4; constexpr CommandId kTestCommandIdNoData = 5; constexpr CommandId kTestCommandIdCommandSpecificResponse = 6; +constexpr CommandId kTestCommandIdFillResponseMessage = 7; constexpr CommandId kTestNonExistCommandId = 0; const app::CommandHandler::TestOnlyMarker kThisIsForTestOnly; @@ -91,6 +92,32 @@ namespace app { CommandHandler::Handle asyncCommandHandle; +struct ForcedSizeBuffer +{ + chip::Platform::ScopedMemoryBufferWithSize mBuffer; + + ForcedSizeBuffer(uint32_t size) + { + if (mBuffer.Alloc(size)) + { + // No significance with using 0x12, just using a value. + memset(mBuffer.Get(), 0x12, size); + } + } + + // No significance with using 0x12 as the CommandId, just using a value. + static constexpr chip::CommandId GetCommandId() { return 0x12; } + CHIP_ERROR Encode(TLV::TLVWriter & aWriter, TLV::Tag aTag) const + { + VerifyOrReturnError(mBuffer, CHIP_ERROR_NO_MEMORY); + + TLV::TLVType outerContainerType; + ReturnErrorOnFailure(aWriter.StartContainer(aTag, TLV::kTLVType_Structure, outerContainerType)); + ReturnErrorOnFailure(app::DataModel::Encode(aWriter, TLV::ContextTag(1), ByteSpan(mBuffer.Get(), mBuffer.AllocatedSize()))); + return aWriter.EndContainer(outerContainerType); + } +}; + InteractionModel::Status ServerClusterCommandExists(const ConcreteCommandPath & aRequestCommandPath) { // Mock cluster catalog, only support commands on one cluster on one endpoint. @@ -287,6 +314,12 @@ class TestCommandInteraction static void TestCommandHandlerRejectsMultipleCommandsWithIdenticalCommandRef(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerRejectMultipleCommandsWhenHandlerOnlySupportsOne(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerAcceptMultipleCommands(nlTestSuite * apSuite, void * apContext); + static void TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsStatusResponse(nlTestSuite * apSuite, + void * apContext); + static void TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponsePrimative(nlTestSuite * apSuite, + void * apContext); + static void TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponse(nlTestSuite * apSuite, + void * apContext); #if CONFIG_BUILD_FOR_HOST_UNIT_TEST static void TestCommandHandlerReleaseWithExchangeClosed(nlTestSuite * apSuite, void * apContext); @@ -342,7 +375,10 @@ class TestCommandInteraction static void AddInvalidInvokeRequestData(nlTestSuite * apSuite, void * apContext, CommandSender * apCommandSender, CommandId aCommandId = kTestCommandIdWithData); static void AddInvokeResponseData(nlTestSuite * apSuite, void * apContext, CommandHandler * apCommandHandler, - bool aNeedStatusCode, CommandId aCommandId = kTestCommandIdWithData); + bool aNeedStatusCode, CommandId aResponseCommandId = kTestCommandIdWithData, + CommandId aRequestCommandId = kTestCommandIdWithData); + static void FillCurrentInvokeResponseBuffer(nlTestSuite * apSuite, CommandHandler * apCommandHandler, + const ConcreteCommandPath & aRequestCommandPath, uint32_t aSizeToLeaveInBuffer); static void ValidateCommandHandlerEncodeInvokeResponseMessage(nlTestSuite * apSuite, void * apContext, bool aNeedStatusCode); }; @@ -511,14 +547,13 @@ void TestCommandInteraction::AddInvalidInvokeRequestData(nlTestSuite * apSuite, } void TestCommandInteraction::AddInvokeResponseData(nlTestSuite * apSuite, void * apContext, CommandHandler * apCommandHandler, - bool aNeedStatusCode, CommandId aCommandId) + bool aNeedStatusCode, CommandId aResponseCommandId, CommandId aRequestCommandId) { CHIP_ERROR err = CHIP_NO_ERROR; - constexpr EndpointId kTestEndpointId = 1; - constexpr ClusterId kTestClusterId = 3; - constexpr CommandId kTestCommandIdWithData = 4; - ConcreteCommandPath requestCommandPath = { kTestEndpointId, kTestClusterId, kTestCommandIdWithData }; + constexpr EndpointId kTestEndpointId = 1; + constexpr ClusterId kTestClusterId = 3; + ConcreteCommandPath requestCommandPath = { kTestEndpointId, kTestClusterId, aRequestCommandId }; if (aNeedStatusCode) { apCommandHandler->AddStatus(requestCommandPath, Protocols::InteractionModel::Status::Success); @@ -526,7 +561,7 @@ void TestCommandInteraction::AddInvokeResponseData(nlTestSuite * apSuite, void * else { const CommandHandler::InvokeResponseParameters prepareParams(requestCommandPath); - ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, aCommandId }; + ConcreteCommandPath responseCommandPath = { kTestEndpointId, kTestClusterId, aResponseCommandId }; err = apCommandHandler->PrepareInvokeResponseCommand(responseCommandPath, prepareParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -540,6 +575,28 @@ void TestCommandInteraction::AddInvokeResponseData(nlTestSuite * apSuite, void * } } +void TestCommandInteraction::FillCurrentInvokeResponseBuffer(nlTestSuite * apSuite, CommandHandler * apCommandHandler, + const ConcreteCommandPath & aRequestCommandPath, + uint32_t aSizeToLeaveInBuffer) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + + err = apCommandHandler->AllocateBuffer(); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + uint32_t remainingSize = apCommandHandler->mInvokeResponseBuilder.GetWriter()->GetRemainingFreeLength(); + + // TODO(#30453): Have this value derived from IM layer similar to GetSizeToEndInvokeResponseMessage() + // This number was derived by executing this method with aSizeToLeaveInBuffer = 100 and + // subsequently analyzing the remaining size to determine the overhead associated with calling + // AddResponseData with `ForcedSizeBuffer`. + uint32_t sizeNeededForAddingResponse = 27; + NL_TEST_ASSERT(apSuite, remainingSize > (aSizeToLeaveInBuffer + sizeNeededForAddingResponse)); + uint32_t sizeToFill = remainingSize - aSizeToLeaveInBuffer - sizeNeededForAddingResponse; + + err = apCommandHandler->AddResponseData(aRequestCommandPath, ForcedSizeBuffer(sizeToFill)); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); +} + void TestCommandInteraction::TestCommandSenderWithWrongState(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); @@ -1691,6 +1748,85 @@ void TestCommandInteraction::TestCommandHandlerAcceptMultipleCommands(nlTestSuit exchange->Close(); } +void TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsStatusResponse( + nlTestSuite * apSuite, void * apContext) +{ + BasicCommandPathRegistry<4> mBasicCommandPathRegistry; + CommandHandler commandHandler(kThisIsForTestOnly, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + commandHandler.mReserveSpaceForMoreChunkMessages = true; + ConcreteCommandPath requestCommandPath1 = { kTestEndpointId, kTestClusterId, kTestCommandIdFillResponseMessage }; + ConcreteCommandPath requestCommandPath2 = { kTestEndpointId, kTestClusterId, kTestCommandIdCommandSpecificResponse }; + Optional commandRef; + commandRef.SetValue(1); + mBasicCommandPathRegistry.Add(requestCommandPath1, commandRef); + commandRef.SetValue(2); + mBasicCommandPathRegistry.Add(requestCommandPath2, commandRef); + + uint32_t sizeToLeave = 0; + FillCurrentInvokeResponseBuffer(apSuite, &commandHandler, requestCommandPath1, sizeToLeave); + uint32_t remainingSize = commandHandler.mInvokeResponseBuilder.GetWriter()->GetRemainingFreeLength(); + NL_TEST_ASSERT(apSuite, remainingSize == sizeToLeave); + + AddInvokeResponseData(apSuite, apContext, &commandHandler, /* aNeedStatusCode = */ true, kTestCommandIdCommandSpecificResponse, + kTestCommandIdCommandSpecificResponse); + + remainingSize = commandHandler.mInvokeResponseBuilder.GetWriter()->GetRemainingFreeLength(); + NL_TEST_ASSERT(apSuite, remainingSize > sizeToLeave); +} + +void TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponsePrimative( + nlTestSuite * apSuite, void * apContext) +{ + BasicCommandPathRegistry<4> mBasicCommandPathRegistry; + CommandHandler commandHandler(kThisIsForTestOnly, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + commandHandler.mReserveSpaceForMoreChunkMessages = true; + ConcreteCommandPath requestCommandPath1 = { kTestEndpointId, kTestClusterId, kTestCommandIdFillResponseMessage }; + ConcreteCommandPath requestCommandPath2 = { kTestEndpointId, kTestClusterId, kTestCommandIdCommandSpecificResponse }; + Optional commandRef; + commandRef.SetValue(1); + mBasicCommandPathRegistry.Add(requestCommandPath1, commandRef); + commandRef.SetValue(2); + mBasicCommandPathRegistry.Add(requestCommandPath2, commandRef); + + uint32_t sizeToLeave = 0; + FillCurrentInvokeResponseBuffer(apSuite, &commandHandler, requestCommandPath1, sizeToLeave); + uint32_t remainingSize = commandHandler.mInvokeResponseBuilder.GetWriter()->GetRemainingFreeLength(); + NL_TEST_ASSERT(apSuite, remainingSize == sizeToLeave); + + AddInvokeResponseData(apSuite, apContext, &commandHandler, /* aNeedStatusCode = */ false, kTestCommandIdCommandSpecificResponse, + kTestCommandIdCommandSpecificResponse); + + remainingSize = commandHandler.mInvokeResponseBuilder.GetWriter()->GetRemainingFreeLength(); + NL_TEST_ASSERT(apSuite, remainingSize > sizeToLeave); +} + +void TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponse(nlTestSuite * apSuite, + void * apContext) +{ + BasicCommandPathRegistry<4> mBasicCommandPathRegistry; + CommandHandler commandHandler(kThisIsForTestOnly, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + commandHandler.mReserveSpaceForMoreChunkMessages = true; + ConcreteCommandPath requestCommandPath1 = { kTestEndpointId, kTestClusterId, kTestCommandIdFillResponseMessage }; + ConcreteCommandPath requestCommandPath2 = { kTestEndpointId, kTestClusterId, kTestCommandIdCommandSpecificResponse }; + Optional commandRef; + commandRef.SetValue(1); + mBasicCommandPathRegistry.Add(requestCommandPath1, commandRef); + commandRef.SetValue(2); + mBasicCommandPathRegistry.Add(requestCommandPath2, commandRef); + + uint32_t sizeToLeave = 0; + FillCurrentInvokeResponseBuffer(apSuite, &commandHandler, requestCommandPath1, sizeToLeave); + uint32_t remainingSize = commandHandler.mInvokeResponseBuilder.GetWriter()->GetRemainingFreeLength(); + NL_TEST_ASSERT(apSuite, remainingSize == sizeToLeave); + + uint32_t sizeToFill = 50; + CHIP_ERROR err = commandHandler.AddResponseData(requestCommandPath2, ForcedSizeBuffer(sizeToFill)); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + remainingSize = commandHandler.mInvokeResponseBuilder.GetWriter()->GetRemainingFreeLength(); + NL_TEST_ASSERT(apSuite, remainingSize > sizeToLeave); +} + #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 @@ -1751,6 +1887,9 @@ const nlTest sTests[] = NL_TEST_DEF("TestCommandHandlerRejectsMultipleCommandsWithIdenticalCommandRef", chip::app::TestCommandInteraction::TestCommandHandlerRejectsMultipleCommandsWithIdenticalCommandRef), NL_TEST_DEF("TestCommandHandlerRejectMultipleCommandsWhenHandlerOnlySupportsOne", chip::app::TestCommandInteraction::TestCommandHandlerRejectMultipleCommandsWhenHandlerOnlySupportsOne), NL_TEST_DEF("TestCommandHandlerAcceptMultipleCommands", chip::app::TestCommandInteraction::TestCommandHandlerAcceptMultipleCommands), + NL_TEST_DEF("TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsStatusResponse", chip::app::TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsStatusResponse), + NL_TEST_DEF("TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponsePrimative", chip::app::TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponsePrimative), + NL_TEST_DEF("TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponse", chip::app::TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhereSecondResponseIsDataResponse), #if CONFIG_BUILD_FOR_HOST_UNIT_TEST