Post merge review updates for CommandHandler updates (#33658)
* Several updates: comments and remove inline
* Restyle
* Move TryAddResponseData into the cpp instead of the header
* Add override, virtual was a copy and paste
* Name argument
* Argument renaming and comment update
* Move EncoderToTLV into DataModel as it looks like a more generic place, maybe we end up re-using it
* Restyle
* Update copyright year
* Renames based on review comments
* More renames of args
* Fix compile
* Slight comment update
* More comment update after self-review
* More comment update after self-review
* Some renaming
* Restyle
* One more rename: EncodableType
* EncodeTo should be const
diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp
index 6077e09..3096854 100644
--- a/src/app/CommandHandler.cpp
+++ b/src/app/CommandHandler.cpp
@@ -113,6 +113,26 @@
return status;
}
+CHIP_ERROR CommandHandler::TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, CommandId aResponseCommandId,
+ DataModel::EncodableToTLV & aEncodable)
+{
+ ConcreteCommandPath responseCommandPath = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId,
+ aResponseCommandId };
+
+ InvokeResponseParameters prepareParams(aRequestCommandPath);
+ prepareParams.SetStartOrEndDataStruct(false);
+
+ {
+ ScopedChange<bool> internalCallToAddResponse(mInternalCallToAddResponseData, true);
+ ReturnErrorOnFailure(PrepareInvokeResponseCommand(responseCommandPath, prepareParams));
+ }
+
+ TLV::TLVWriter * writer = GetCommandDataIBTLVWriter();
+ VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE);
+ ReturnErrorOnFailure(aEncodable.EncodeTo(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields)));
+ return FinishCommand(/* aEndDataStruct = */ false);
+}
+
CHIP_ERROR CommandHandler::ValidateInvokeRequestMessageAndBuildRegistry(InvokeRequestMessage::Parser & invokeRequestMessage)
{
CHIP_ERROR err = CHIP_NO_ERROR;
diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h
index b06064c..ae41ec1 100644
--- a/src/app/CommandHandler.h
+++ b/src/app/CommandHandler.h
@@ -33,6 +33,7 @@
#include <app/CommandHandlerExchangeInterface.h>
#include <app/CommandPathRegistry.h>
#include <app/ConcreteCommandPath.h>
+#include <app/data-model/EncodableToTLV.h>
#include <app/data-model/Encode.h>
#include <lib/core/CHIPCore.h>
#include <lib/core/TLV.h>
@@ -56,32 +57,6 @@
namespace chip {
namespace app {
-/// Defines an abstract class of something that can be encoded
-/// into a TLV with a given data tag
-class EncoderToTLV
-{
-public:
- virtual ~EncoderToTLV() = default;
-
- virtual CHIP_ERROR Encode(TLV::TLVWriter &, TLV::Tag tag) = 0;
-};
-
-/// An `EncoderToTLV` the uses `DataModel::Encode` to encode things.
-///
-/// Generally useful to encode things like <ClusterName>::Commands::<CommandName>::Type
-/// structures.
-template <typename T>
-class DataModelEncoderToTLV : public EncoderToTLV
-{
-public:
- DataModelEncoderToTLV(const T & value) : mValue(value) {}
-
- virtual CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag) { return DataModel::Encode(writer, tag, mValue); }
-
-private:
- const T & mValue;
-};
-
class CommandHandler
{
public:
@@ -267,7 +242,7 @@
* Adds the given command status and returns any failures in adding statuses (e.g. out
* of buffer space) to the caller
*/
- CHIP_ERROR FallibleAddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus,
+ CHIP_ERROR FallibleAddStatus(const ConcreteCommandPath & aRequestCommandPath, const Protocols::InteractionModel::Status aStatus,
const char * context = nullptr);
/**
@@ -277,9 +252,9 @@
void AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus,
const char * context = nullptr);
- CHIP_ERROR AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus);
+ CHIP_ERROR AddClusterSpecificSuccess(const ConcreteCommandPath & aRequestCommandPath, ClusterStatus aClusterStatus);
- CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus);
+ CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aRequestCommandPath, ClusterStatus aClusterStatus);
/**
* This adds a new CommandDataIB element into InvokeResponses for the associated
@@ -350,37 +325,34 @@
* @param [in] aRequestCommandPath the concrete path of the command we are
* responding to.
* @param [in] aData the data for the response.
- *
- * NOTE: this is a convenience function for `AddResponseDataViaEncoder`
*/
template <typename CommandData>
- inline CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
+ CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
{
- DataModelEncoderToTLV<CommandData> encoder(aData);
- return AddResponseDataViaEncoder(aRequestCommandPath, CommandData::GetCommandId(), encoder);
+ DataModel::EncodableType<CommandData> encoder(aData);
+ return AddResponseData(aRequestCommandPath, CommandData::GetCommandId(), encoder);
}
/**
- * API for adding a data response. The encoded is generally expected to encode
- * a ClusterName::Commands::CommandName::Type struct, but any
- * object should work.
+ * API for adding a data response. The `aEncodable` is generally expected to encode
+ * a ClusterName::Commands::CommandName::Type struct, however any object should work.
*
* @param [in] aRequestCommandPath the concrete path of the command we are
* responding to.
- * @param [in] commandId the command whose content is being encoded.
- * @param [in] encoder - an encoder that places the command data structure for `commandId`
- * into a TLV Writer.
+ * @param [in] aResponseCommandId the command whose content is being encoded.
+ * @param [in] aEncodable - an encodable that places the command data structure
+ * for `aResponseCommandId` into a TLV Writer.
*
* Most applications are likely to use `AddResponseData` as a more convenient
* one-call that auto-sets command ID and creates the underlying encoders.
*/
- CHIP_ERROR AddResponseDataViaEncoder(const ConcreteCommandPath & aRequestCommandPath, CommandId commandId,
- EncoderToTLV & encoder)
+ CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, CommandId aResponseCommandId,
+ DataModel::EncodableToTLV & aEncodable)
{
// Return early when response should not be sent out.
VerifyOrReturnValue(ResponsesAccepted(), CHIP_NO_ERROR);
return TryAddingResponse(
- [&]() -> CHIP_ERROR { return TryAddResponseDataViaEncoder(aRequestCommandPath, commandId, encoder); });
+ [&]() -> CHIP_ERROR { return TryAddResponseData(aRequestCommandPath, aResponseCommandId, aEncodable); });
}
/**
@@ -398,21 +370,22 @@
* @param [in] aData the data for the response.
*/
template <typename CommandData>
- inline void AddResponse(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
+ void AddResponse(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
{
- DataModelEncoderToTLV<CommandData> encoder(aData);
- return AddResponseViaEncoder(aRequestCommandPath, CommandData::GetCommandId(), encoder);
+ DataModel::EncodableType<CommandData> encodable(aData);
+ return AddResponse(aRequestCommandPath, CommandData::GetCommandId(), encodable);
}
/**
- * API for adding a response with a given encoder of TLV data.
+ * API for adding a response with a given encodable of TLV data.
*
- * The encoder would generally encode a ClusterName::Commands::CommandName::Type with
+ * The encodable would generally encode a ClusterName::Commands::CommandName::Type with
* the corresponding `GetCommandId` call.
*/
- void AddResponseViaEncoder(const ConcreteCommandPath & aRequestCommandPath, CommandId commandId, EncoderToTLV & encoder)
+ void AddResponse(const ConcreteCommandPath & aRequestCommandPath, CommandId aResponseCommandId,
+ DataModel::EncodableToTLV & aEncodable)
{
- if (AddResponseDataViaEncoder(aRequestCommandPath, commandId, encoder) != CHIP_NO_ERROR)
+ if (AddResponseData(aRequestCommandPath, aResponseCommandId, aEncodable) != CHIP_NO_ERROR)
{
AddStatus(aRequestCommandPath, Protocols::InteractionModel::Status::Failure);
}
@@ -667,48 +640,16 @@
CHIP_ERROR AddStatusInternal(const ConcreteCommandPath & aCommandPath, const StatusIB & aStatus);
/**
- * Non-templated function called before DataModel::Encode when attempting to add a response,
- * which does all the work needed before encoding the actual type-dependent data into the buffer.
- *
- * **Important:** If this function fails, the TLV buffer may be left in an inconsistent state.
- * Callers should create snapshots as necessary before invoking this function and implement
- * rollback mechanisms if needed.
- *
- * **Usage:** This function is intended to be called exclusively by TryAddResponseData. It was
- * factored out to optimize code size.
- *
- * @param aRequestCommandPath The concrete path of the command being responded to.
- * @param aResponseCommandPath The concrete path of the command response.
- */
- CHIP_ERROR TryAddResponseDataPreEncode(const ConcreteCommandPath & aRequestCommandPath,
- const ConcreteCommandPath & aResponseCommandPath)
- {
- InvokeResponseParameters prepareParams(aRequestCommandPath);
- prepareParams.SetStartOrEndDataStruct(false);
-
- ScopedChange<bool> internalCallToAddResponse(mInternalCallToAddResponseData, true);
- return PrepareInvokeResponseCommand(aResponseCommandPath, prepareParams);
- }
-
- /**
* If this function fails, it may leave our TLV buffer in an inconsistent state.
* Callers should snapshot as needed before calling this function, and roll back
* as needed afterward.
*
- * @param [in] aRequestCommandPath the concrete path of the command we are
- * responding to.
- * @param [in] aData the data for the response.
+ * @param [in] aRequestCommandPath the concrete path of the command we are responding to
+ * @param [in] aResponseCommandId the id of the command to encode
+ * @param [in] aEncodable the data to encode for the given aResponseCommandId
*/
- CHIP_ERROR TryAddResponseDataViaEncoder(const ConcreteCommandPath & aRequestCommandPath, CommandId commandId,
- EncoderToTLV & encoder)
- {
- ConcreteCommandPath responseCommandPath = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, commandId };
- ReturnErrorOnFailure(TryAddResponseDataPreEncode(aRequestCommandPath, responseCommandPath));
- TLV::TLVWriter * writer = GetCommandDataIBTLVWriter();
- VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE);
- ReturnErrorOnFailure(encoder.Encode(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields)));
- return FinishCommand(/* aEndDataStruct = */ false);
- }
+ CHIP_ERROR TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, CommandId aResponseCommandId,
+ DataModel::EncodableToTLV & aEncodable);
void SetExchangeInterface(CommandHandlerExchangeInterface * commandResponder);
diff --git a/src/app/data-model/BUILD.gn b/src/app/data-model/BUILD.gn
index 6f38c05..d61b30a 100644
--- a/src/app/data-model/BUILD.gn
+++ b/src/app/data-model/BUILD.gn
@@ -18,6 +18,7 @@
"BasicTypes.h",
"DecodableList.h",
"Decode.h",
+ "EncodableToTLV.h",
"Encode.h",
"FabricScoped.h",
"FabricScopedPreEncodedValue.cpp",
diff --git a/src/app/data-model/EncodableToTLV.h b/src/app/data-model/EncodableToTLV.h
new file mode 100644
index 0000000..d8b0391
--- /dev/null
+++ b/src/app/data-model/EncodableToTLV.h
@@ -0,0 +1,62 @@
+/*
+ * 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 <app/data-model/Encode.h>
+#include <lib/core/CHIPError.h>
+#include <lib/core/TLV.h>
+
+namespace chip {
+namespace app {
+namespace DataModel {
+
+/// Defines an abstract class of something that can be encoded
+/// into a TLV with a given data tag
+class EncodableToTLV
+{
+public:
+ virtual ~EncodableToTLV() = default;
+
+ virtual CHIP_ERROR EncodeTo(TLV::TLVWriter & writer, TLV::Tag tag) const = 0;
+};
+
+/// An `EncodableToTLV` that uses `DataModel::Encode` to encode things in one call.
+///
+/// Applicable to any type for which `chip::app::DataModel::Encode` works. In
+/// particular, types like <ClusterName>::Commands::<CommandName>::Type
+/// can be used as a type here.
+template <typename T>
+class EncodableType : public EncodableToTLV
+{
+public:
+ /// Encodes the given value via `DataModel::Encode` when the underlying
+ /// encode is called.
+ ///
+ /// LIFETIME NOTE: uses a reference to value, so value must live longer than
+ /// this object.
+ EncodableType(const T & value) : mValue(value) {}
+
+ CHIP_ERROR EncodeTo(TLV::TLVWriter & writer, TLV::Tag tag) const override { return DataModel::Encode(writer, tag, mValue); }
+
+private:
+ const T & mValue;
+};
+
+} // namespace DataModel
+} // namespace app
+} // namespace chip
