Post merge review updates for CommandHandler updates

src/app/CommandHandler.cpp

@@ -113,6 +113,17 @@ Status CommandHandler::OnInvokeCommandRequest(CommandHandlerExchangeInterface &
     return status;
 }
 
+CHIP_ERROR CommandHandler::TryAddResponseData(const ConcreteCommandPath & path, CommandId commandId,
+                                              DataModel::EncoderToTLV & encoder)
+{
+    ConcreteCommandPath responseCommandPath = { path.mEndpointId, path.mClusterId, commandId };
+    ReturnErrorOnFailure(TryAddResponseDataPreEncode(path, 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 CommandHandler::ValidateInvokeRequestMessageAndBuildRegistry(InvokeRequestMessage::Parser & invokeRequestMessage)
 {
     CHIP_ERROR err          = CHIP_NO_ERROR;

src/app/CommandHandler.h

@@ -34,6 +34,7 @@
 #include <app/CommandPathRegistry.h>
 #include <app/ConcreteCommandPath.h>
 #include <app/data-model/Encode.h>
+#include <app/data-model/EncoderToTLV.h>
 #include <lib/core/CHIPCore.h>
 #include <lib/core/TLV.h>
 #include <lib/core/TLVDebug.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:
@@ -350,18 +325,16 @@ 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::ObjectEncoderToTLV<CommandData> encoder(aData);
+        return AddResponseData(aRequestCommandPath, CommandData::GetCommandId(), encoder);
     }
 
     /**
-     * API for adding a data response.  The encoded is generally expected to encode
+     * API for adding a data response.  The `encoder` is generally expected to encode
      * a ClusterName::Commands::CommandName::Type struct, but any
      * object should work.
      *
@@ -374,13 +347,12 @@ class CommandHandler
      * 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 commandId,
+                               DataModel::EncoderToTLV & encoder)
     {
         // Return early when response should not be sent out.
         VerifyOrReturnValue(ResponsesAccepted(), CHIP_NO_ERROR);
-        return TryAddingResponse(
-            [&]() -> CHIP_ERROR { return TryAddResponseDataViaEncoder(aRequestCommandPath, commandId, encoder); });
+        return TryAddingResponse([&]() -> CHIP_ERROR { return TryAddResponseData(aRequestCommandPath, commandId, encoder); });
     }
 
     /**
@@ -398,10 +370,10 @@ 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::ObjectEncoderToTLV<CommandData> encoder(aData);
+        return AddResponse(aRequestCommandPath, CommandData::GetCommandId(), encoder);
     }
 
     /**
@@ -410,9 +382,9 @@ class CommandHandler
      * The encoder 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 commandId, DataModel::EncoderToTLV & encoder)
     {
-        if (AddResponseDataViaEncoder(aRequestCommandPath, commandId, encoder) != CHIP_NO_ERROR)
+        if (AddResponseData(aRequestCommandPath, commandId, encoder) != CHIP_NO_ERROR)
         {
             AddStatus(aRequestCommandPath, Protocols::InteractionModel::Status::Failure);
         }
@@ -695,20 +667,11 @@ class CommandHandler
      * 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] path the concrete path of the command we are responding to
+     * @param [in] commandId the id of the command to encode
+     * @param [in] encoder the data to encode for the given commandId
      */
-    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 & path, CommandId commandId, DataModel::EncoderToTLV & encoder);
 
     void SetExchangeInterface(CommandHandlerExchangeInterface * commandResponder);
 

src/app/data-model/BUILD.gn

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

src/app/data-model/EncoderToTLV.h

@@ -0,0 +1,61 @@
+/*
+ *    Copyright (c) 2020-2021 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 EncoderToTLV
+{
+public:
+    virtual ~EncoderToTLV() = default;
+
+    virtual CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag) = 0;
+};
+
+/// An `EncoderToTLV` that uses `DataModel::Encode` to encode things.
+///
+/// Generally useful to encode things like <ClusterName>::Commands::<CommandName>::Type
+/// structures.
+template <typename T>
+class ObjectEncoderToTLV : public EncoderToTLV
+{
+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.
+    ObjectEncoderToTLV(const T & value) : mValue(value) {}
+
+    CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag) override { return DataModel::Encode(writer, tag, mValue); }
+
+private:
+    const T & mValue;
+};
+
+} // namespace DataModel
+} // namespace app
+} // namespace chip