From 8753139d6fa3248622de2fe0b3e4d859d25f8622 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 15 Feb 2024 21:59:39 -0500 Subject: [PATCH] CommandSender: For batch commands, track requests are responded to (#32105) * CommandSender: For batch commands, track requests are responded to * Rename * Restyled by whitespace * Restyled by clang-format * Restyled by gn * Self review updates * Quick fix * Quick fix * Restyled by whitespace * More fixes * Fix CI * Restyled by whitespace * Restyled by clang-format * Restyled by gn * Apply suggestions from code review Co-authored-by: Boris Zbarsky * Address PR comments * Fix CI and address some PR comments * Restyled by clang-format * Update src/app/CommandSender.cpp Co-authored-by: Boris Zbarsky * Update src/app/CommandSender.cpp Co-authored-by: Boris Zbarsky * Address PR comments * Address comment --------- Co-authored-by: Restyled.io Co-authored-by: Boris Zbarsky --- scripts/tools/check_includes_config.py | 3 + src/app/BUILD.gn | 3 + src/app/CommandSender.cpp | 85 +++++++++-- src/app/CommandSender.h | 63 ++++++-- src/app/PendingResponseTracker.h | 75 ++++++++++ src/app/PendingResponseTrackerImpl.cpp | 61 ++++++++ src/app/PendingResponseTrackerImpl.h | 44 ++++++ src/app/tests/BUILD.gn | 1 + src/app/tests/TestCommandInteraction.cpp | 83 ++++++++--- .../tests/TestPendingResponseTrackerImpl.cpp | 135 ++++++++++++++++++ src/lib/core/BUILD.gn | 2 +- src/lib/core/CHIPConfig.h | 11 +- 12 files changed, 517 insertions(+), 49 deletions(-) create mode 100644 src/app/PendingResponseTracker.h create mode 100644 src/app/PendingResponseTrackerImpl.cpp create mode 100644 src/app/PendingResponseTrackerImpl.h create mode 100644 src/app/tests/TestPendingResponseTrackerImpl.cpp diff --git a/scripts/tools/check_includes_config.py b/scripts/tools/check_includes_config.py index 42920c4014c133..6943c26ecb3081 100644 --- a/scripts/tools/check_includes_config.py +++ b/scripts/tools/check_includes_config.py @@ -160,6 +160,9 @@ 'src/tracing/json/json_tracing.cpp': {'string', 'sstream'}, 'src/tracing/json/json_tracing.h': {'fstream', 'unordered_map'}, + # Not intended for embedded clients + 'src/app/PendingResponseTrackerImpl.h': {'unordered_set'}, + # Not intended for embedded clients 'src/lib/support/jsontlv/JsonToTlv.cpp': {'sstream'}, 'src/lib/support/jsontlv/JsonToTlv.h': {'string'}, diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index 73a0433f73b262..c0f30493824f3a 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -231,6 +231,9 @@ static_library("app") { "FailSafeContext.cpp", "FailSafeContext.h", "OTAUserConsentCommon.h", + "PendingResponseTracker.h", + "PendingResponseTrackerImpl.cpp", + "PendingResponseTrackerImpl.h", "ReadHandler.cpp", "SafeAttributePersistenceProvider.h", "TimedRequest.cpp", diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 7f77d6c98c2bad..03ce2786b93d8d 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -75,6 +75,9 @@ CommandSender::CommandSender(ExtendableCallback * apExtendableCallback, Messagin mTimedRequest(aIsTimedRequest), mUseExtendableCallback(true) { assertChipStackLockedByCurrentThread(); +#if CHIP_CONFIG_COMMAND_SENDER_BUILTIN_SUPPORT_FOR_BATCHED_COMMANDS + mpPendingResponseTracker = &mNonTestPendingResponseTracker; +#endif // CHIP_CONFIG_COMMAND_SENDER_BUILTIN_SUPPORT_FOR_BATCHED_COMMANDS } CommandSender::~CommandSender() @@ -222,9 +225,9 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha if (aPayloadHeader.HasMessageType(MsgType::InvokeCommandResponse)) { + mInvokeResponseMessageCount++; err = ProcessInvokeResponse(std::move(aPayload), moreChunkedMessages); SuccessOrExit(err); - mInvokeResponseMessageCount++; if (moreChunkedMessages) { StatusResponse::Send(Status::Success, apExchangeContext, /*aExpectResponse = */ true); @@ -258,6 +261,10 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha if (mState != State::AwaitingResponse) { + if (err == CHIP_NO_ERROR) + { + FlushNoCommandResponse(); + } Close(); } // Else we got a response to a Timed Request and just sent the invoke. @@ -331,12 +338,25 @@ void CommandSender::OnResponseTimeout(Messaging::ExchangeContext * apExchangeCon Close(); } +void CommandSender::FlushNoCommandResponse() +{ + if (mpPendingResponseTracker && mUseExtendableCallback && mCallbackHandle.extendableCallback) + { + Optional commandRef = mpPendingResponseTracker->PopPendingResponse(); + while (commandRef.HasValue()) + { + NoResponseData noResponseData = { commandRef.Value() }; + mCallbackHandle.extendableCallback->OnNoResponse(this, noResponseData); + commandRef = mpPendingResponseTracker->PopPendingResponse(); + } + } +} + void CommandSender::Close() { mSuppressResponse = false; mTimedRequest = false; MoveToState(State::AwaitingDestruction); - OnDoneCallback(); } @@ -350,10 +370,10 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn StatusIB statusIB; { - bool commandRefExpected = (mFinishedCommandCount > 1); - bool hasDataResponse = false; + bool hasDataResponse = false; TLV::TLVReader commandDataReader; Optional commandRef; + bool commandRefExpected = mpPendingResponseTracker && (mpPendingResponseTracker->Count() > 1); CommandStatusIB::Parser commandStatus; err = aInvokeResponse.GetStatus(&commandStatus); @@ -409,6 +429,27 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn } ReturnErrorOnFailure(err); + if (commandRef.HasValue() && mpPendingResponseTracker != nullptr) + { + err = mpPendingResponseTracker->Remove(commandRef.Value()); + if (err != CHIP_NO_ERROR) + { + // This can happen for two reasons: + // 1. The current InvokeResponse is a duplicate (based on its commandRef). + // 2. The current InvokeResponse is for a request we never sent (based on its commandRef). + // Used when logging errors related to server violating spec. + [[maybe_unused]] ScopedNodeId remoteScopedNode; + if (mExchangeCtx.Get()->HasSessionHandle()) + { + remoteScopedNode = mExchangeCtx.Get()->GetSessionHandle()->GetPeer(); + } + ChipLogError(DataManagement, + "Received Unexpected Response from remote node " ChipLogFormatScopedNodeId ", commandRef=%u", + ChipLogValueScopedNodeId(remoteScopedNode), commandRef.Value()); + return err; + } + } + // When using ExtendableCallbacks, we are adhering to a different API contract where path // specific errors are sent to the OnResponse callback. For more information on the history // of this issue please see https://github.com/project-chip/connectedhomeip/issues/30991 @@ -430,17 +471,19 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn CHIP_ERROR CommandSender::SetCommandSenderConfig(CommandSender::ConfigParameters & aConfigParams) { -#if CHIP_CONFIG_SENDING_BATCH_COMMANDS_ENABLED VerifyOrReturnError(mState == State::Idle, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(aConfigParams.remoteMaxPathsPerInvoke > 0, CHIP_ERROR_INVALID_ARGUMENT); + if (mpPendingResponseTracker != nullptr) + { - mRemoteMaxPathsPerInvoke = aConfigParams.remoteMaxPathsPerInvoke; - mBatchCommandsEnabled = (aConfigParams.remoteMaxPathsPerInvoke > 1); - return CHIP_NO_ERROR; -#else - VerifyOrReturnError(aConfigParams.remoteMaxPathsPerInvoke == 1, CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE); + mRemoteMaxPathsPerInvoke = aConfigParams.remoteMaxPathsPerInvoke; + mBatchCommandsEnabled = (aConfigParams.remoteMaxPathsPerInvoke > 1); + } + else + { + VerifyOrReturnError(aConfigParams.remoteMaxPathsPerInvoke == 1, CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE); + } return CHIP_NO_ERROR; -#endif } CHIP_ERROR CommandSender::PrepareCommand(const CommandPathParams & aCommandPathParams, @@ -453,12 +496,19 @@ CHIP_ERROR CommandSender::PrepareCommand(const CommandPathParams & aCommandPathP // bool canAddAnotherCommand = (mState == State::AddedCommand && mBatchCommandsEnabled && mUseExtendableCallback); VerifyOrReturnError(mState == State::Idle || canAddAnotherCommand, CHIP_ERROR_INCORRECT_STATE); - VerifyOrReturnError(mFinishedCommandCount < mRemoteMaxPathsPerInvoke, CHIP_ERROR_MAXIMUM_PATHS_PER_INVOKE_EXCEEDED); + + if (mpPendingResponseTracker != nullptr) + { + size_t pendingCount = mpPendingResponseTracker->Count(); + VerifyOrReturnError(pendingCount < mRemoteMaxPathsPerInvoke, CHIP_ERROR_MAXIMUM_PATHS_PER_INVOKE_EXCEEDED); + } if (mBatchCommandsEnabled) { + VerifyOrReturnError(mpPendingResponseTracker != nullptr, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(aPrepareCommandParams.commandRef.HasValue(), CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(aPrepareCommandParams.commandRef.Value() == mFinishedCommandCount, CHIP_ERROR_INVALID_ARGUMENT); + uint16_t commandRef = aPrepareCommandParams.commandRef.Value(); + VerifyOrReturnError(!mpPendingResponseTracker->IsTracked(commandRef), CHIP_ERROR_INVALID_ARGUMENT); } InvokeRequests::Builder & invokeRequests = mInvokeRequestBuilder.GetInvokeRequests(); @@ -482,8 +532,10 @@ CHIP_ERROR CommandSender::FinishCommand(FinishCommandParameters & aFinishCommand { if (mBatchCommandsEnabled) { + VerifyOrReturnError(mpPendingResponseTracker != nullptr, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(aFinishCommandParams.commandRef.HasValue(), CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(aFinishCommandParams.commandRef.Value() == mFinishedCommandCount, CHIP_ERROR_INVALID_ARGUMENT); + uint16_t commandRef = aFinishCommandParams.commandRef.Value(); + VerifyOrReturnError(!mpPendingResponseTracker->IsTracked(commandRef), CHIP_ERROR_INVALID_ARGUMENT); } return FinishCommandInternal(aFinishCommandParams); @@ -511,7 +563,10 @@ CHIP_ERROR CommandSender::FinishCommandInternal(FinishCommandParameters & aFinis MoveToState(State::AddedCommand); - mFinishedCommandCount++; + if (mpPendingResponseTracker && aFinishCommandParams.commandRef.HasValue()) + { + mpPendingResponseTracker->Add(aFinishCommandParams.commandRef.Value()); + } if (aFinishCommandParams.timedInvokeTimeoutMs.HasValue()) { diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h index fa213ac0699832..ef1a5f97dd3e85 100644 --- a/src/app/CommandSender.h +++ b/src/app/CommandSender.h @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -75,6 +76,16 @@ class CommandSender final : public Messaging::ExchangeDelegate Optional commandRef; }; + // CommandSender::ExtendableCallback::OnNoResponse is public SDK API, so we cannot break + // source compatibility for it. To allow for additional values to be added at a future + // time without constantly changing the function's declaration parameter list, we are + // defining the struct NoResponseData and adding that to the parameter list to allow for + // future extendability. + struct NoResponseData + { + uint16_t commandRef; + }; + // CommandSender::ExtendableCallback::OnError is public SDK API, so we cannot break source // compatibility for it. To allow for additional values to be added at a future time // without constantly changing the function's declaration parameter list, we are @@ -127,9 +138,21 @@ class CommandSender final : public Messaging::ExchangeDelegate * @param[in] apCommandSender The command sender object that initiated the command transaction. * @param[in] aResponseData Information pertaining to the response. */ - ; virtual void OnResponse(CommandSender * commandSender, const ResponseData & aResponseData) {} + /** + * Called for each request that failed to receive a response after the server indicates completion of all requests. + * + * This callback may be omitted if clients have alternative ways to track non-responses. + * + * The CommandSender object MUST continue to exist after this call is completed. The application shall wait until it + * receives an OnDone call to destroy the object. + * + * @param apCommandSender The CommandSender object that initiated the transaction. + * @param aNoResponseData Details about the request without a response. + */ + virtual void OnNoResponse(CommandSender * commandSender, const NoResponseData & aNoResponseData) {} + /** * OnError will be called when a non-path-specific error occurs *after* a successful call to SendCommandRequest(). * @@ -229,12 +252,6 @@ class CommandSender final : public Messaging::ExchangeDelegate // early in PrepareCommand, even though it's not used until FinishCommand. This proactive // validation helps prevent unnecessary writing an InvokeRequest into the packet that later // needs to be undone. - // - // Currently, provided commandRefs for the first request must start at 0 and increment by one - // for each subsequent request. This requirement can be relaxed in the future if a compelling - // need arises. - // TODO(#30453): After introducing Request/Response tracking, remove statement above about - // this currently enforced requirement on commandRefs. Optional commandRef; // If the InvokeRequest needs to be in a state with a started data TLV struct container bool startDataStruct = false; @@ -278,6 +295,10 @@ class CommandSender final : public Messaging::ExchangeDelegate bool endDataStruct = false; }; + class TestOnlyMarker + { + }; + /* * Constructor. * @@ -287,12 +308,20 @@ class CommandSender final : public Messaging::ExchangeDelegate */ CommandSender(Callback * apCallback, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest = false, bool aSuppressResponse = false); - CommandSender(ExtendableCallback * apCallback, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest = false, - bool aSuppressResponse = false); CommandSender(std::nullptr_t, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest = false, bool aSuppressResponse = false) : CommandSender(static_cast(nullptr), apExchangeMgr, aIsTimedRequest, aSuppressResponse) {} + CommandSender(ExtendableCallback * apCallback, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest = false, + bool aSuppressResponse = 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) + { + mpPendingResponseTracker = apPendingResponseTracker; + } ~CommandSender(); /** @@ -307,11 +336,14 @@ class CommandSender final : public Messaging::ExchangeDelegate * based on how many paths the remote peer claims to support. * * @return CHIP_ERROR_INCORRECT_STATE - * If device has previously called `PrepareCommand`. + * If device has previously called `PrepareCommand`. * @return CHIP_ERROR_INVALID_ARGUMENT - * Invalid argument value. + * Invalid argument value. * @return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE - * Device has not enabled CHIP_CONFIG_SENDING_BATCH_COMMANDS_ENABLED. + * Device has not enabled batch command support. To enable: + * 1. Enable the CHIP_CONFIG_COMMAND_SENDER_BUILTIN_SUPPORT_FOR_BATCHED_COMMANDS + * configuration option. + * 2. Ensure you provide ExtendableCallback. */ CHIP_ERROR SetCommandSenderConfig(ConfigParameters & aConfigParams); @@ -497,6 +529,7 @@ class CommandSender final : public Messaging::ExchangeDelegate System::PacketBufferHandle && aPayload) override; void OnResponseTimeout(Messaging::ExchangeContext * apExchangeContext) override; + void FlushNoCommandResponse(); // // Called internally to signal the completion of all work on this object, gracefully close the // exchange (by calling into the base class) and finally, signal to the application that it's @@ -580,8 +613,12 @@ class CommandSender final : public Messaging::ExchangeDelegate chip::System::PacketBufferTLVWriter mCommandMessageWriter; +#if CHIP_CONFIG_COMMAND_SENDER_BUILTIN_SUPPORT_FOR_BATCHED_COMMANDS + PendingResponseTrackerImpl mNonTestPendingResponseTracker; +#endif // CHIP_CONFIG_COMMAND_SENDER_BUILTIN_SUPPORT_FOR_BATCHED_COMMANDS + PendingResponseTracker * mpPendingResponseTracker = nullptr; + uint16_t mInvokeResponseMessageCount = 0; - uint16_t mFinishedCommandCount = 0; uint16_t mRemoteMaxPathsPerInvoke = 1; State mState = State::Idle; diff --git a/src/app/PendingResponseTracker.h b/src/app/PendingResponseTracker.h new file mode 100644 index 00000000000000..47bb37dd026629 --- /dev/null +++ b/src/app/PendingResponseTracker.h @@ -0,0 +1,75 @@ +/* + * Copyright (c) 2024 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 + +namespace chip { +namespace app { + +/** + * @brief Interface for tracking responses to outbound InvokeRequests. + * + * This interface enables clients to: + * * Verify that received responses correspond to issued InvokeRequests. + * * Detect outstanding responses after the server indicates completion, helpful for identifying response omissions. + */ +class PendingResponseTracker +{ +public: + virtual ~PendingResponseTracker() = default; + + /** + * Start tracking the given `aCommandRef` + * + * @return CHIP_ERROR_INVALID_ARGUMENT if `aCommandRef` is already being tracked. + */ + virtual CHIP_ERROR Add(uint16_t aCommandRef) = 0; + + /** + * Removes tracking for the given `aCommandRef` + * + * @return CHIP_ERROR_KEY_NOT_FOUND if aCommandRef is not currently tracked. + */ + virtual CHIP_ERROR Remove(uint16_t aCommandRef) = 0; + + /** + * Checks if the given `aCommandRef` is being tracked. + */ + virtual bool IsTracked(uint16_t aCommandRef) = 0; + + /** + * Returns the number of pending responses. + */ + virtual size_t Count() = 0; + + /** + * Removes a pending response command reference from the tracker. + * + * Deletes an element from the tracker (order not guaranteed). This function can be called + * repeatedly to remove all tracked pending responses. + * + * @return NullOptional if the tracker is empty. + * @return Optional containing the CommandReference of a removed pending response. + */ + virtual Optional PopPendingResponse() = 0; +}; + +} // namespace app +} // namespace chip diff --git a/src/app/PendingResponseTrackerImpl.cpp b/src/app/PendingResponseTrackerImpl.cpp new file mode 100644 index 00000000000000..72f5fb8a197697 --- /dev/null +++ b/src/app/PendingResponseTrackerImpl.cpp @@ -0,0 +1,61 @@ +/* + * Copyright (c) 2024 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 + +namespace chip { +namespace app { + +CHIP_ERROR PendingResponseTrackerImpl::Add(uint16_t aCommandRef) +{ + VerifyOrReturnError(!IsTracked(aCommandRef), CHIP_ERROR_INVALID_ARGUMENT); + mCommandReferenceSet.insert(aCommandRef); + return CHIP_NO_ERROR; +} + +CHIP_ERROR PendingResponseTrackerImpl::Remove(uint16_t aCommandRef) +{ + VerifyOrReturnError(IsTracked(aCommandRef), CHIP_ERROR_KEY_NOT_FOUND); + mCommandReferenceSet.erase(aCommandRef); + return CHIP_NO_ERROR; +} + +bool PendingResponseTrackerImpl::IsTracked(uint16_t aCommandRef) +{ + return mCommandReferenceSet.find(aCommandRef) != mCommandReferenceSet.end(); +} + +size_t PendingResponseTrackerImpl::Count() +{ + return mCommandReferenceSet.size(); +} + +Optional PendingResponseTrackerImpl::PopPendingResponse() +{ + if (Count() == 0) + { + return NullOptional; + } + uint16_t commandRef = *mCommandReferenceSet.begin(); + mCommandReferenceSet.erase(mCommandReferenceSet.begin()); + return MakeOptional(commandRef); +} + +} // namespace app +} // namespace chip diff --git a/src/app/PendingResponseTrackerImpl.h b/src/app/PendingResponseTrackerImpl.h new file mode 100644 index 00000000000000..8d6ade2ae5bb69 --- /dev/null +++ b/src/app/PendingResponseTrackerImpl.h @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2024 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 + +namespace chip { +namespace app { + +/** + * @brief An implementation of PendingResponseTracker. + */ +class PendingResponseTrackerImpl : public PendingResponseTracker +{ +public: + CHIP_ERROR Add(uint16_t aCommandRef) override; + CHIP_ERROR Remove(uint16_t aCommandRef) override; + bool IsTracked(uint16_t aCommandRef) override; + size_t Count() override; + Optional PopPendingResponse() override; + +private: + std::unordered_set mCommandReferenceSet; +}; + +} // namespace app +} // namespace chip diff --git a/src/app/tests/BUILD.gn b/src/app/tests/BUILD.gn index 4dad96164804c0..be78dd8ade9e54 100644 --- a/src/app/tests/BUILD.gn +++ b/src/app/tests/BUILD.gn @@ -147,6 +147,7 @@ chip_test_suite_using_nltest("tests") { "TestNumericAttributeTraits.cpp", "TestOperationalStateClusterObjects.cpp", "TestPendingNotificationMap.cpp", + "TestPendingResponseTrackerImpl.cpp", "TestPowerSourceCluster.cpp", "TestReadInteraction.cpp", "TestReportingEngine.cpp", diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index d67b389b1df6aa..6aab66ec8549c3 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -85,7 +85,8 @@ constexpr CommandId kTestCommandIdCommandSpecificResponse = 6; constexpr CommandId kTestCommandIdFillResponseMessage = 7; constexpr CommandId kTestNonExistCommandId = 0; -const app::CommandHandler::TestOnlyMarker kThisIsForTestOnly; +const app::CommandHandler::TestOnlyMarker kCommandHandlerTestOnlyMarker; +const app::CommandSender::TestOnlyMarker kCommandSenderTestOnlyMarker; } // namespace namespace app { @@ -328,7 +329,8 @@ class TestCommandInteraction static void TestCommandSenderLegacyCallbackUnsupportedCommand(nlTestSuite * apSuite, void * apContext); static void TestCommandSenderExtendableCallbackUnsupportedCommand(nlTestSuite * apSuite, void * apContext); static void TestCommandSenderLegacyCallbackBuildingBatchCommandFails(nlTestSuite * apSuite, void * apContext); - static void TestCommandSenderExtendableCallbackBuildingBatchCommandFails(nlTestSuite * apSuite, void * apContext); + static void TestCommandSenderExtendableCallbackBuildingBatchDuplicateCommandRefFails(nlTestSuite * apSuite, void * apContext); + static void TestCommandSenderExtendableCallbackBuildingBatchCommandSuccess(nlTestSuite * apSuite, void * apContext); static void TestCommandSenderCommandSuccessResponseFlow(nlTestSuite * apSuite, void * apContext); static void TestCommandSenderCommandAsyncSuccessResponseFlow(nlTestSuite * apSuite, void * apContext); static void TestCommandSenderCommandFailureResponseFlow(nlTestSuite * apSuite, void * apContext); @@ -1271,8 +1273,12 @@ void TestCommandInteraction::TestCommandSenderLegacyCallbackBuildingBatchCommand prepareCommandParams.SetStartDataStruct(true).SetCommandRef(0); finishCommandParams.SetEndDataStruct(true).SetCommandRef(0); - commandSender.mBatchCommandsEnabled = true; - commandSender.mRemoteMaxPathsPerInvoke = 2; + CommandSender::ConfigParameters config; + config.SetRemoteMaxPathsPerInvoke(2); + err = commandSender.SetCommandSenderConfig(config); + NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE); + // Even though we got an error saying invalid argument we are going to attempt + // to add two commands. auto commandPathParams = MakeTestCommandPath(); err = commandSender.PrepareCommand(commandPathParams, prepareCommandParams); @@ -1291,20 +1297,64 @@ void TestCommandInteraction::TestCommandSenderLegacyCallbackBuildingBatchCommand NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } -void TestCommandInteraction::TestCommandSenderExtendableCallbackBuildingBatchCommandFails(nlTestSuite * apSuite, void * apContext) +void TestCommandInteraction::TestCommandSenderExtendableCallbackBuildingBatchDuplicateCommandRefFails(nlTestSuite * apSuite, + void * apContext) { TestContext & ctx = *static_cast(apContext); CHIP_ERROR err = CHIP_NO_ERROR; mockCommandSenderExtendedDelegate.ResetCounter(); - app::CommandSender commandSender(&mockCommandSenderExtendedDelegate, &ctx.GetExchangeManager()); + PendingResponseTrackerImpl pendingResponseTracker; + app::CommandSender commandSender(kCommandSenderTestOnlyMarker, &mockCommandSenderExtendedDelegate, &ctx.GetExchangeManager(), + &pendingResponseTracker); app::CommandSender::PrepareCommandParameters prepareCommandParams; app::CommandSender::FinishCommandParameters finishCommandParams; + + CommandSender::ConfigParameters config; + config.SetRemoteMaxPathsPerInvoke(2); + err = commandSender.SetCommandSenderConfig(config); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + prepareCommandParams.SetStartDataStruct(true).SetCommandRef(0); finishCommandParams.SetEndDataStruct(true).SetCommandRef(0); + auto commandPathParams = MakeTestCommandPath(); + err = commandSender.PrepareCommand(commandPathParams, prepareCommandParams); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + chip::TLV::TLVWriter * writer = commandSender.GetCommandDataIBTLVWriter(); + err = writer->PutBoolean(chip::TLV::ContextTag(1), true); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + err = commandSender.FinishCommand(finishCommandParams); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + // Preparing second command. + prepareCommandParams.SetCommandRef(0); + err = commandSender.PrepareCommand(commandPathParams, prepareCommandParams); + NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INVALID_ARGUMENT); - commandSender.mBatchCommandsEnabled = true; - commandSender.mRemoteMaxPathsPerInvoke = 2; + NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); +} +void TestCommandInteraction::TestCommandSenderExtendableCallbackBuildingBatchCommandSuccess(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + mockCommandSenderExtendedDelegate.ResetCounter(); + PendingResponseTrackerImpl pendingResponseTracker; + app::CommandSender commandSender(kCommandSenderTestOnlyMarker, &mockCommandSenderExtendedDelegate, &ctx.GetExchangeManager(), + &pendingResponseTracker); + app::CommandSender::PrepareCommandParameters prepareCommandParams; + app::CommandSender::FinishCommandParameters finishCommandParams; + + CommandSender::ConfigParameters config; + config.SetRemoteMaxPathsPerInvoke(2); + err = commandSender.SetCommandSenderConfig(config); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + // The specific values chosen here are arbitrary. This test primarily verifies that we can + // use a larger command reference value followed by a smaller one for subsequent command. + uint16_t firstCommandRef = 40; + uint16_t secondCommandRef = 2; + prepareCommandParams.SetStartDataStruct(true).SetCommandRef(firstCommandRef); + finishCommandParams.SetEndDataStruct(true).SetCommandRef(firstCommandRef); auto commandPathParams = MakeTestCommandPath(); err = commandSender.PrepareCommand(commandPathParams, prepareCommandParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -1314,8 +1364,8 @@ void TestCommandInteraction::TestCommandSenderExtendableCallbackBuildingBatchCom err = commandSender.FinishCommand(finishCommandParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); // Preparing second command. - prepareCommandParams.SetCommandRef(1); - finishCommandParams.SetCommandRef(1); + prepareCommandParams.SetCommandRef(secondCommandRef); + finishCommandParams.SetCommandRef(secondCommandRef); err = commandSender.PrepareCommand(commandPathParams, prepareCommandParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); writer = commandSender.GetCommandDataIBTLVWriter(); @@ -1650,7 +1700,7 @@ void TestCommandInteraction::TestCommandHandlerRejectMultipleCommandsWhenHandler } BasicCommandPathRegistry<4> mBasicCommandPathRegistry; - CommandHandler commandHandler(kThisIsForTestOnly, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + CommandHandler commandHandler(kCommandHandlerTestOnlyMarker, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); TestExchangeDelegate delegate; auto exchange = ctx.NewExchangeToAlice(&delegate, false); commandHandler.mResponseSender.SetExchangeContext(exchange); @@ -1724,7 +1774,7 @@ void TestCommandInteraction::TestCommandHandlerAcceptMultipleCommands(nlTestSuit } BasicCommandPathRegistry<4> mBasicCommandPathRegistry; - CommandHandler commandHandler(kThisIsForTestOnly, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + CommandHandler commandHandler(kCommandHandlerTestOnlyMarker, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); TestExchangeDelegate delegate; auto exchange = ctx.NewExchangeToAlice(&delegate, false); commandHandler.mResponseSender.SetExchangeContext(exchange); @@ -1756,7 +1806,7 @@ void TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhere nlTestSuite * apSuite, void * apContext) { BasicCommandPathRegistry<4> mBasicCommandPathRegistry; - CommandHandler commandHandler(kThisIsForTestOnly, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + CommandHandler commandHandler(kCommandHandlerTestOnlyMarker, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); commandHandler.mReserveSpaceForMoreChunkMessages = true; ConcreteCommandPath requestCommandPath1 = { kTestEndpointId, kTestClusterId, kTestCommandIdFillResponseMessage }; ConcreteCommandPath requestCommandPath2 = { kTestEndpointId, kTestClusterId, kTestCommandIdCommandSpecificResponse }; @@ -1782,7 +1832,7 @@ void TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhere nlTestSuite * apSuite, void * apContext) { BasicCommandPathRegistry<4> mBasicCommandPathRegistry; - CommandHandler commandHandler(kThisIsForTestOnly, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + CommandHandler commandHandler(kCommandHandlerTestOnlyMarker, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); commandHandler.mReserveSpaceForMoreChunkMessages = true; ConcreteCommandPath requestCommandPath1 = { kTestEndpointId, kTestClusterId, kTestCommandIdFillResponseMessage }; ConcreteCommandPath requestCommandPath2 = { kTestEndpointId, kTestClusterId, kTestCommandIdCommandSpecificResponse }; @@ -1808,7 +1858,7 @@ void TestCommandInteraction::TestCommandHandler_FillUpInvokeResponseMessageWhere void * apContext) { BasicCommandPathRegistry<4> mBasicCommandPathRegistry; - CommandHandler commandHandler(kThisIsForTestOnly, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); + CommandHandler commandHandler(kCommandHandlerTestOnlyMarker, &mockCommandHandlerDelegate, &mBasicCommandPathRegistry); commandHandler.mReserveSpaceForMoreChunkMessages = true; ConcreteCommandPath requestCommandPath1 = { kTestEndpointId, kTestClusterId, kTestCommandIdFillResponseMessage }; ConcreteCommandPath requestCommandPath2 = { kTestEndpointId, kTestClusterId, kTestCommandIdCommandSpecificResponse }; @@ -1902,7 +1952,8 @@ const nlTest sTests[] = NL_TEST_DEF("TestCommandSenderLegacyCallbackUnsupportedCommand", chip::app::TestCommandInteraction::TestCommandSenderLegacyCallbackUnsupportedCommand), NL_TEST_DEF("TestCommandSenderExtendableCallbackUnsupportedCommand", chip::app::TestCommandInteraction::TestCommandSenderExtendableCallbackUnsupportedCommand), NL_TEST_DEF("TestCommandSenderLegacyCallbackBuildingBatchCommandFails", chip::app::TestCommandInteraction::TestCommandSenderLegacyCallbackBuildingBatchCommandFails), - NL_TEST_DEF("TestCommandSenderExtendableCallbackBuildingBatchCommandFails", chip::app::TestCommandInteraction::TestCommandSenderExtendableCallbackBuildingBatchCommandFails), + NL_TEST_DEF("TestCommandSenderExtendableCallbackBuildingBatchDuplicateCommandRefFails", chip::app::TestCommandInteraction::TestCommandSenderExtendableCallbackBuildingBatchDuplicateCommandRefFails), + NL_TEST_DEF("TestCommandSenderExtendableCallbackBuildingBatchCommandSuccess", chip::app::TestCommandInteraction::TestCommandSenderExtendableCallbackBuildingBatchCommandSuccess), NL_TEST_DEF("TestCommandSenderCommandSuccessResponseFlow", chip::app::TestCommandInteraction::TestCommandSenderCommandSuccessResponseFlow), NL_TEST_DEF("TestCommandSenderCommandAsyncSuccessResponseFlow", chip::app::TestCommandInteraction::TestCommandSenderCommandAsyncSuccessResponseFlow), NL_TEST_DEF("TestCommandSenderCommandSpecificResponseFlow", chip::app::TestCommandInteraction::TestCommandSenderCommandSpecificResponseFlow), diff --git a/src/app/tests/TestPendingResponseTrackerImpl.cpp b/src/app/tests/TestPendingResponseTrackerImpl.cpp new file mode 100644 index 00000000000000..da6239434cd3b1 --- /dev/null +++ b/src/app/tests/TestPendingResponseTrackerImpl.cpp @@ -0,0 +1,135 @@ +/* + * Copyright (c) 2024 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 +#include +#include + +namespace { + +using namespace chip; + +void TestPendingResponseTracker_FillEntireTracker(nlTestSuite * inSuite, void * inContext) +{ + chip::app::PendingResponseTrackerImpl pendingResponseTracker; + for (uint16_t commandRef = 0; commandRef < std::numeric_limits::max(); commandRef++) + { + NL_TEST_ASSERT(inSuite, false == pendingResponseTracker.IsTracked(commandRef)); + NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == pendingResponseTracker.Add(commandRef)); + NL_TEST_ASSERT(inSuite, true == pendingResponseTracker.IsTracked(commandRef)); + } + + NL_TEST_ASSERT(inSuite, std::numeric_limits::max() == pendingResponseTracker.Count()); + + for (uint16_t commandRef = 0; commandRef < std::numeric_limits::max(); commandRef++) + { + NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == pendingResponseTracker.Remove(commandRef)); + NL_TEST_ASSERT(inSuite, false == pendingResponseTracker.IsTracked(commandRef)); + } + NL_TEST_ASSERT(inSuite, 0 == pendingResponseTracker.Count()); +} + +void TestPendingResponseTracker_FillSingleEntryInTracker(nlTestSuite * inSuite, void * inContext) +{ + chip::app::PendingResponseTrackerImpl pendingResponseTracker; + + // The value 40 is arbitrary; any value would work for this purpose. + uint16_t commandRefToSet = 40; + NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == pendingResponseTracker.Add(commandRefToSet)); + + for (uint16_t commandRef = 0; commandRef < std::numeric_limits::max(); commandRef++) + { + bool expectedIsSetResult = (commandRef == commandRefToSet); + NL_TEST_ASSERT(inSuite, expectedIsSetResult == pendingResponseTracker.IsTracked(commandRef)); + } +} + +void TestPendingResponseTracker_RemoveNonExistentEntryInTrackerFails(nlTestSuite * inSuite, void * inContext) +{ + chip::app::PendingResponseTrackerImpl pendingResponseTracker; + + // The value 40 is arbitrary; any value would work for this purpose. + uint16_t commandRef = 40; + NL_TEST_ASSERT(inSuite, false == pendingResponseTracker.IsTracked(commandRef)); + NL_TEST_ASSERT(inSuite, CHIP_ERROR_KEY_NOT_FOUND == pendingResponseTracker.Remove(commandRef)); +} + +void TestPendingResponseTracker_AddingSecondEntryFails(nlTestSuite * inSuite, void * inContext) +{ + chip::app::PendingResponseTrackerImpl pendingResponseTracker; + + // The value 40 is arbitrary; any value would work for this purpose. + uint16_t commandRef = 40; + NL_TEST_ASSERT(inSuite, false == pendingResponseTracker.IsTracked(commandRef)); + NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == pendingResponseTracker.Add(commandRef)); + NL_TEST_ASSERT(inSuite, true == pendingResponseTracker.IsTracked(commandRef)); + NL_TEST_ASSERT(inSuite, CHIP_ERROR_INVALID_ARGUMENT == pendingResponseTracker.Add(commandRef)); +} + +void TestPendingResponseTracker_PopFindsAllPendingRequests(nlTestSuite * inSuite, void * inContext) +{ + chip::app::PendingResponseTrackerImpl pendingResponseTracker; + + // The specific values in requestsToAdd are not significant; they are chosen arbitrarily for testing purposes. + std::vector requestsToAdd = { 0, 50, 2, 2000 }; + for (const uint16_t & commandRef : requestsToAdd) + { + NL_TEST_ASSERT(inSuite, false == pendingResponseTracker.IsTracked(commandRef)); + NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == pendingResponseTracker.Add(commandRef)); + NL_TEST_ASSERT(inSuite, true == pendingResponseTracker.IsTracked(commandRef)); + } + + NL_TEST_ASSERT(inSuite, requestsToAdd.size() == pendingResponseTracker.Count()); + + for (size_t i = 0; i < requestsToAdd.size(); i++) + { + auto commandRef = pendingResponseTracker.PopPendingResponse(); + NL_TEST_ASSERT(inSuite, true == commandRef.HasValue()); + bool expectedCommandRef = std::find(requestsToAdd.begin(), requestsToAdd.end(), commandRef.Value()) != requestsToAdd.end(); + NL_TEST_ASSERT(inSuite, true == expectedCommandRef); + } + NL_TEST_ASSERT(inSuite, 0 == pendingResponseTracker.Count()); + auto commandRef = pendingResponseTracker.PopPendingResponse(); + NL_TEST_ASSERT(inSuite, false == commandRef.HasValue()); +} + +} // namespace + +#define NL_TEST_DEF_FN(fn) NL_TEST_DEF("Test " #fn, fn) +/** + * Test Suite. It lists all the test functions. + */ +static const nlTest sTests[] = { NL_TEST_DEF_FN(TestPendingResponseTracker_FillEntireTracker), + NL_TEST_DEF_FN(TestPendingResponseTracker_FillSingleEntryInTracker), + NL_TEST_DEF_FN(TestPendingResponseTracker_RemoveNonExistentEntryInTrackerFails), + NL_TEST_DEF_FN(TestPendingResponseTracker_AddingSecondEntryFails), + NL_TEST_DEF_FN(TestPendingResponseTracker_PopFindsAllPendingRequests), + NL_TEST_SENTINEL() }; + +int TestPendingResponseTracker() +{ + nlTestSuite theSuite = { "CHIP PendingResponseTrackerImpl tests", &sTests[0], nullptr, nullptr }; + + // Run test suite against one context. + nlTestRunner(&theSuite, nullptr); + return nlTestRunnerStats(&theSuite); +} + +CHIP_REGISTER_TEST_SUITE(TestPendingResponseTracker) diff --git a/src/lib/core/BUILD.gn b/src/lib/core/BUILD.gn index 27611376c10069..9163cafd85fe0c 100644 --- a/src/lib/core/BUILD.gn +++ b/src/lib/core/BUILD.gn @@ -68,7 +68,7 @@ buildconfig_header("chip_buildconfig") { "CHIP_CONFIG_BIG_ENDIAN_TARGET=${chip_target_is_big_endian}", "CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_WRITE=${chip_tlv_validate_char_string_on_write}", "CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_READ=${chip_tlv_validate_char_string_on_read}", - "CHIP_CONFIG_SENDING_BATCH_COMMANDS_ENABLED=${chip_enable_sending_batch_commands}", + "CHIP_CONFIG_COMMAND_SENDER_BUILTIN_SUPPORT_FOR_BATCHED_COMMANDS=${chip_enable_sending_batch_commands}", ] visibility = [ ":chip_config_header" ] diff --git a/src/lib/core/CHIPConfig.h b/src/lib/core/CHIPConfig.h index 09131aed21cebd..2de6a662ee8c4d 100644 --- a/src/lib/core/CHIPConfig.h +++ b/src/lib/core/CHIPConfig.h @@ -1703,12 +1703,15 @@ extern const char CHIP_NON_PRODUCTION_MARKER[]; #endif /** - * @def CHIP_CONFIG_SENDING_BATCH_COMMANDS_ENABLED + * @def CHIP_CONFIG_COMMAND_SENDER_BUILTIN_SUPPORT_FOR_BATCHED_COMMANDS * - * @brief Device supports sending multiple batch commands in a single Invoke Request Message. + * @brief CommandSender will use built-in support to enable sending batch commands in a single Invoke Request Message. + * + * **Important:** This feature is code and RAM intensive. Enable only on platforms where these + * resources are not constrained. */ -#ifndef CHIP_CONFIG_SENDING_BATCH_COMMANDS_ENABLED -#define CHIP_CONFIG_SENDING_BATCH_COMMANDS_ENABLED 0 +#ifndef CHIP_CONFIG_COMMAND_SENDER_BUILTIN_SUPPORT_FOR_BATCHED_COMMANDS +#define CHIP_CONFIG_COMMAND_SENDER_BUILTIN_SUPPORT_FOR_BATCHED_COMMANDS 0 #endif /**