From ec8547afbd314fd104c54985b7900336958b331a Mon Sep 17 00:00:00 2001 From: Pradip De Date: Fri, 14 Jun 2024 16:59:16 -0700 Subject: [PATCH] Changes to CommandSender and CommandHandler for supporting commands (#33889) with large payloads. --- src/app/CommandHandlerExchangeInterface.h | 12 +++++++++++ src/app/CommandHandlerImpl.cpp | 4 +++- src/app/CommandResponseSender.cpp | 16 +++++++++++++++ src/app/CommandResponseSender.h | 2 ++ src/app/CommandSender.cpp | 25 ++++++++++++++++++----- src/app/CommandSender.h | 14 +++++++------ src/app/tests/TestCommandInteraction.cpp | 2 ++ 7 files changed, 63 insertions(+), 12 deletions(-) diff --git a/src/app/CommandHandlerExchangeInterface.h b/src/app/CommandHandlerExchangeInterface.h index f39a469f35a440..99458ce68ec68b 100644 --- a/src/app/CommandHandlerExchangeInterface.h +++ b/src/app/CommandHandlerExchangeInterface.h @@ -102,6 +102,18 @@ class CommandHandlerExchangeInterface * Called by CommandHandler to relay this information to the requester. */ virtual void ResponseDropped() = 0; + + /** + * @brief Gets the maximum size of a packet buffer to encode a Command + * Response message. This size depends on the underlying session used + * by the exchange. + * + * The size returned here is the size not including the prepended headers. + * + * Called by CommandHandler when allocating buffer for encoding the Command + * response. + */ + virtual size_t GetCommandResponseMaxBufferSize() = 0; }; } // namespace app diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp index d57fab2b00f78a..8c2572db590c17 100644 --- a/src/app/CommandHandlerImpl.cpp +++ b/src/app/CommandHandlerImpl.cpp @@ -67,7 +67,9 @@ CHIP_ERROR CommandHandlerImpl::AllocateBuffer() { mCommandMessageWriter.Reset(); - System::PacketBufferHandle commandPacket = System::PacketBufferHandle::New(chip::app::kMaxSecureSduLengthBytes); + const size_t commandBufferMaxSize = mpResponder->GetCommandResponseMaxBufferSize(); + auto commandPacket = System::PacketBufferHandle::New(commandBufferMaxSize); + VerifyOrReturnError(!commandPacket.IsNull(), CHIP_ERROR_NO_MEMORY); mCommandMessageWriter.Init(std::move(commandPacket)); diff --git a/src/app/CommandResponseSender.cpp b/src/app/CommandResponseSender.cpp index 430fa5f0fa523b..f23786e28dd318 100644 --- a/src/app/CommandResponseSender.cpp +++ b/src/app/CommandResponseSender.cpp @@ -226,6 +226,22 @@ void CommandResponseSender::OnInvokeCommandRequest(Messaging::ExchangeContext * } } +size_t CommandResponseSender::GetCommandResponseMaxBufferSize() +{ + if (!mExchangeCtx || !mExchangeCtx->HasSessionHandle()) + { + ChipLogError(DataManagement, "Session not available. Unable to infer session-specific buffer capacities."); + return kMaxSecureSduLengthBytes; + } + + if (mExchangeCtx->GetSessionHandle()->AllowsLargePayload()) + { + return kMaxLargeSecureSduLengthBytes; + } + + return kMaxSecureSduLengthBytes; +} + #if CHIP_WITH_NLFAULTINJECTION void CommandResponseSender::TestOnlyInvokeCommandRequestWithFaultsInjected(Messaging::ExchangeContext * ec, diff --git a/src/app/CommandResponseSender.h b/src/app/CommandResponseSender.h index 1925f01b54bf8c..0e03a87f1e8cab 100644 --- a/src/app/CommandResponseSender.h +++ b/src/app/CommandResponseSender.h @@ -124,6 +124,8 @@ class CommandResponseSender : public Messaging::ExchangeDelegate, void ResponseDropped() override { mReportResponseDropped = true; } + size_t GetCommandResponseMaxBufferSize() override; + /* * Main entrypoint for this class to handle an invoke request. * diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index fe263eb8923cc7..a91d0fd76ea038 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -55,18 +55,19 @@ CHIP_ERROR GetRef(ParserT aParser, Optional & aRef, bool commandRefReq } // namespace CommandSender::CommandSender(Callback * apCallback, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest, - bool aSuppressResponse) : + bool aSuppressResponse, bool aAllowLargePayload) : mExchangeCtx(*this), - mCallbackHandle(apCallback), mpExchangeMgr(apExchangeMgr), mSuppressResponse(aSuppressResponse), mTimedRequest(aIsTimedRequest) + mCallbackHandle(apCallback), mpExchangeMgr(apExchangeMgr), mSuppressResponse(aSuppressResponse), mTimedRequest(aIsTimedRequest), + mAllowLargePayload(aAllowLargePayload) { assertChipStackLockedByCurrentThread(); } CommandSender::CommandSender(ExtendableCallback * apExtendableCallback, Messaging::ExchangeManager * apExchangeMgr, - bool aIsTimedRequest, bool aSuppressResponse) : + bool aIsTimedRequest, bool aSuppressResponse, bool aAllowLargePayload) : mExchangeCtx(*this), mCallbackHandle(apExtendableCallback), mpExchangeMgr(apExchangeMgr), mSuppressResponse(aSuppressResponse), - mTimedRequest(aIsTimedRequest), mUseExtendableCallback(true) + mTimedRequest(aIsTimedRequest), mUseExtendableCallback(true), mAllowLargePayload(aAllowLargePayload) { assertChipStackLockedByCurrentThread(); #if CHIP_CONFIG_COMMAND_SENDER_BUILTIN_SUPPORT_FOR_BATCHED_COMMANDS @@ -85,7 +86,15 @@ CHIP_ERROR CommandSender::AllocateBuffer() { mCommandMessageWriter.Reset(); - System::PacketBufferHandle commandPacket = System::PacketBufferHandle::New(chip::app::kMaxSecureSduLengthBytes); + System::PacketBufferHandle commandPacket; + if (mAllowLargePayload) + { + commandPacket = System::PacketBufferHandle::New(kMaxLargeSecureSduLengthBytes); + } + else + { + commandPacket = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes); + } VerifyOrReturnError(!commandPacket.IsNull(), CHIP_ERROR_NO_MEMORY); mCommandMessageWriter.Init(std::move(commandPacket)); @@ -139,6 +148,12 @@ CHIP_ERROR CommandSender::TestOnlyCommandSenderTimedRequestFlagWithNoTimedInvoke CHIP_ERROR CommandSender::SendCommandRequest(const SessionHandle & session, Optional timeout) { + // If the command is expected to be large, ensure that the underlying + // session supports it. + if (mAllowLargePayload) + { + VerifyOrReturnError(session->AllowsLargePayload(), CHIP_ERROR_INCORRECT_STATE); + } if (mTimedRequest != mTimedInvokeTimeoutMs.HasValue()) { diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h index 5542d2469bb743..e34dbe7b494df6 100644 --- a/src/app/CommandSender.h +++ b/src/app/CommandSender.h @@ -308,18 +308,19 @@ class CommandSender final : public Messaging::ExchangeDelegate * If callbacks are passed the only one that will be called in a group sesttings is the onDone */ CommandSender(Callback * apCallback, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest = false, - bool aSuppressResponse = false); + bool aSuppressResponse = false, bool aAllowLargePayload = false); CommandSender(std::nullptr_t, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest = false, - bool aSuppressResponse = false) : - CommandSender(static_cast(nullptr), apExchangeMgr, aIsTimedRequest, aSuppressResponse) + bool aSuppressResponse = false, bool aAllowLargePayload = false) : + CommandSender(static_cast(nullptr), apExchangeMgr, aIsTimedRequest, aSuppressResponse, aAllowLargePayload) {} CommandSender(ExtendableCallback * apCallback, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest = false, - bool aSuppressResponse = false); + bool aSuppressResponse = false, bool aAllowLargePayload = false); // TODO(#32138): After there is a macro that is always defined for all unit tests, the constructor with // TestOnlyMarker should only be compiled if that macro is defined. CommandSender(TestOnlyMarker aTestMarker, ExtendableCallback * apCallback, Messaging::ExchangeManager * apExchangeMgr, - PendingResponseTracker * apPendingResponseTracker, bool aIsTimedRequest = false, bool aSuppressResponse = false) : - CommandSender(apCallback, apExchangeMgr, aIsTimedRequest, aSuppressResponse) + PendingResponseTracker * apPendingResponseTracker, bool aIsTimedRequest = false, bool aSuppressResponse = false, + bool aAllowLargePayload = false) : + CommandSender(apCallback, apExchangeMgr, aIsTimedRequest, aSuppressResponse, aAllowLargePayload) { mpPendingResponseTracker = apPendingResponseTracker; } @@ -636,6 +637,7 @@ class CommandSender final : public Messaging::ExchangeDelegate bool mBufferAllocated = false; bool mBatchCommandsEnabled = false; bool mUseExtendableCallback = false; + bool mAllowLargePayload = false; }; } // namespace app diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 22c7ccc1a9e9b4..0573db4e9aa861 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -330,6 +330,8 @@ class MockCommandResponder : public CommandHandlerExchangeInterface void AddInvokeResponseToSend(System::PacketBufferHandle && aPacket) override { mChunks.AddToEnd(std::move(aPacket)); } void ResponseDropped() override { mResponseDropped = true; } + size_t GetCommandResponseMaxBufferSize() override { return kMaxSecureSduLengthBytes; } + System::PacketBufferHandle mChunks; bool mResponseDropped = false; };