Post merge review updates for CommandHandler updates (project-chip#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

Author: andy31415

src/app/CommandHandler.cpp

@@ -113,6 +113,26 @@ Status CommandHandler::OnInvokeCommandRequest(CommandHandlerExchangeInterface &
     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;

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 @@ class CommandHandler
      * 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 @@ class CommandHandler
     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 @@ class CommandHandler
      * @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 @@ class CommandHandler
      * @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);
         }
@@ -666,49 +639,17 @@ class CommandHandler
 
     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);
 

src/app/data-model/BUILD.gn

@@ -18,6 +18,7 @@ source_set("data-model") {
     "BasicTypes.h",
     "DecodableList.h",
     "Decode.h",
+    "EncodableToTLV.h",
     "Encode.h",
     "FabricScoped.h",
     "FabricScopedPreEncodedValue.cpp",

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