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 <bzbarsky@apple.com>
* Address PR comments
* Fix CI and address some PR comments
* Restyled by clang-format
* Update src/app/CommandSender.cpp
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Update src/app/CommandSender.cpp
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Address PR comments
* Address comment
---------
Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
diff --git a/scripts/tools/check_includes_config.py b/scripts/tools/check_includes_config.py
index 42920c4..6943c26 100644
--- a/scripts/tools/check_includes_config.py
+++ b/scripts/tools/check_includes_config.py
@@ -161,6 +161,9 @@
'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'},
'src/lib/support/jsontlv/TlvToJson.h': {'string'},
diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn
index 73a0433..c0f3049 100644
--- a/src/app/BUILD.gn
+++ b/src/app/BUILD.gn
@@ -231,6 +231,9 @@
"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 7f77d6c..03ce278 100644
--- a/src/app/CommandSender.cpp
+++ b/src/app/CommandSender.cpp
@@ -75,6 +75,9 @@
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 @@
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 @@
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 @@
Close();
}
+void CommandSender::FlushNoCommandResponse()
+{
+ if (mpPendingResponseTracker && mUseExtendableCallback && mCallbackHandle.extendableCallback)
+ {
+ Optional<uint16_t> 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 @@
StatusIB statusIB;
{
- bool commandRefExpected = (mFinishedCommandCount > 1);
- bool hasDataResponse = false;
+ bool hasDataResponse = false;
TLV::TLVReader commandDataReader;
Optional<uint16_t> commandRef;
+ bool commandRefExpected = mpPendingResponseTracker && (mpPendingResponseTracker->Count() > 1);
CommandStatusIB::Parser commandStatus;
err = aInvokeResponse.GetStatus(&commandStatus);
@@ -409,6 +429,27 @@
}
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::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);
+ mRemoteMaxPathsPerInvoke = aConfigParams.remoteMaxPathsPerInvoke;
+ mBatchCommandsEnabled = (aConfigParams.remoteMaxPathsPerInvoke > 1);
+ }
+ else
+ {
+ VerifyOrReturnError(aConfigParams.remoteMaxPathsPerInvoke == 1, CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE);
+ }
return CHIP_NO_ERROR;
-#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 @@
//
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 @@
{
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 @@
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 fa213ac..ef1a5f9 100644
--- a/src/app/CommandSender.h
+++ b/src/app/CommandSender.h
@@ -32,6 +32,7 @@
#include <app/MessageDef/InvokeRequestMessage.h>
#include <app/MessageDef/InvokeResponseMessage.h>
#include <app/MessageDef/StatusIB.h>
+#include <app/PendingResponseTrackerImpl.h>
#include <app/data-model/Encode.h>
#include <lib/core/CHIPCore.h>
#include <lib/core/Optional.h>
@@ -75,6 +76,16 @@
Optional<uint16_t> 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,10 +138,22 @@
* @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().
*
* The CommandSender object MUST continue to exist after this call is completed. The application shall wait until it
@@ -229,12 +252,6 @@
// 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<uint16_t> commandRef;
// If the InvokeRequest needs to be in a state with a started data TLV struct container
bool startDataStruct = false;
@@ -278,6 +295,10 @@
bool endDataStruct = false;
};
+ class TestOnlyMarker
+ {
+ };
+
/*
* Constructor.
*
@@ -287,12 +308,20 @@
*/
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<Callback *>(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 @@
* 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 @@
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 @@
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 0000000..47bb37d
--- /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 <lib/core/CHIPError.h>
+#include <lib/core/Optional.h>
+
+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<uint16_t> PopPendingResponse() = 0;
+};
+
+} // namespace app
+} // namespace chip
diff --git a/src/app/PendingResponseTrackerImpl.cpp b/src/app/PendingResponseTrackerImpl.cpp
new file mode 100644
index 0000000..72f5fb8
--- /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 <app/PendingResponseTrackerImpl.h>
+
+#include <lib/support/CodeUtils.h>
+
+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<uint16_t> 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 0000000..8d6ade2
--- /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 <unordered_set>
+
+#include <app/PendingResponseTracker.h>
+
+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<uint16_t> PopPendingResponse() override;
+
+private:
+ std::unordered_set<uint16_t> mCommandReferenceSet;
+};
+
+} // namespace app
+} // namespace chip
diff --git a/src/app/tests/BUILD.gn b/src/app/tests/BUILD.gn
index 4dad961..be78dd8 100644
--- a/src/app/tests/BUILD.gn
+++ b/src/app/tests/BUILD.gn
@@ -147,6 +147,7 @@
"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 d67b389..6aab66e 100644
--- a/src/app/tests/TestCommandInteraction.cpp
+++ b/src/app/tests/TestCommandInteraction.cpp
@@ -85,7 +85,8 @@
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 @@
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 @@
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,25 @@
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<TestContext *>(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);
-
- commandSender.mBatchCommandsEnabled = true;
- commandSender.mRemoteMaxPathsPerInvoke = 2;
-
auto commandPathParams = MakeTestCommandPath();
err = commandSender.PrepareCommand(commandPathParams, prepareCommandParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
@@ -1314,8 +1325,47 @@
err = commandSender.FinishCommand(finishCommandParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
// Preparing second command.
- prepareCommandParams.SetCommandRef(1);
- finishCommandParams.SetCommandRef(1);
+ prepareCommandParams.SetCommandRef(0);
+ err = commandSender.PrepareCommand(commandPathParams, prepareCommandParams);
+ NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INVALID_ARGUMENT);
+
+ 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<TestContext *>(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);
+ 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(secondCommandRef);
+ finishCommandParams.SetCommandRef(secondCommandRef);
err = commandSender.PrepareCommand(commandPathParams, prepareCommandParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
writer = commandSender.GetCommandDataIBTLVWriter();
@@ -1650,7 +1700,7 @@
}
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 @@
}
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 @@
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 @@
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 * 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 @@
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 0000000..da62394
--- /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 <app/PendingResponseTrackerImpl.h>
+#include <lib/support/UnitTestRegistration.h>
+
+#include <algorithm>
+#include <nlunit-test.h>
+#include <vector>
+
+namespace {
+
+using namespace chip;
+
+void TestPendingResponseTracker_FillEntireTracker(nlTestSuite * inSuite, void * inContext)
+{
+ chip::app::PendingResponseTrackerImpl pendingResponseTracker;
+ for (uint16_t commandRef = 0; commandRef < std::numeric_limits<uint16_t>::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<uint16_t>::max() == pendingResponseTracker.Count());
+
+ for (uint16_t commandRef = 0; commandRef < std::numeric_limits<uint16_t>::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<uint16_t>::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<uint16_t> 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 2761137..9163caf 100644
--- a/src/lib/core/BUILD.gn
+++ b/src/lib/core/BUILD.gn
@@ -68,7 +68,7 @@
"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 09131ae..2de6a66 100644
--- a/src/lib/core/CHIPConfig.h
+++ b/src/lib/core/CHIPConfig.h
@@ -1703,12 +1703,15 @@
#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
/**
