Skip to content

Commit

Permalink
Post merge review updates for CommandHandler updates (#33658)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
andy31415 authored May 29, 2024
1 parent 1f1b750 commit 90c5cd4
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 87 deletions.
20 changes: 20 additions & 0 deletions src/app/CommandHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
115 changes: 28 additions & 87 deletions src/app/CommandHandler.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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:
Expand Down Expand Up @@ -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);

/**
Expand All @@ -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
Expand Down Expand Up @@ -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); });
}

/**
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);

Expand Down
1 change: 1 addition & 0 deletions src/app/data-model/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ source_set("data-model") {
"BasicTypes.h",
"DecodableList.h",
"Decode.h",
"EncodableToTLV.h",
"Encode.h",
"FabricScoped.h",
"FabricScopedPreEncodedValue.cpp",
Expand Down
62 changes: 62 additions & 0 deletions src/app/data-model/EncodableToTLV.h
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 90c5cd4

Please sign in to comment.