Skip to content

Commit

Permalink
Minimize API surface that is templated for CommandHandler (#33620)
Browse files Browse the repository at this point in the history
* Define a EncodeToTLV inteface for the command handler

* Slight documentation update

* Comments update
  • Loading branch information
andy31415 authored and pull[bot] committed Oct 4, 2024
1 parent 546111f commit ad66d9c
Showing 1 changed file with 70 additions and 22 deletions.
92 changes: 70 additions & 22 deletions src/app/CommandHandler.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,32 @@
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 @@ -325,14 +351,37 @@ 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>
CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
inline CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
{
DataModelEncoderToTLV<CommandData> encoder(aData);
return AddResponseDataViaEncoder(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.
*
* @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.
*
* 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)
{
// Return early when response should not be sent out.
VerifyOrReturnValue(ResponsesAccepted(), CHIP_NO_ERROR);

return TryAddingResponse([&]() -> CHIP_ERROR { return TryAddResponseData(aRequestCommandPath, aData); });
return TryAddingResponse(
[&]() -> CHIP_ERROR { return TryAddResponseDataViaEncoder(aRequestCommandPath, commandId, encoder); });
}

/**
Expand All @@ -350,9 +399,21 @@ class CommandHandler
* @param [in] aData the data for the response.
*/
template <typename CommandData>
void AddResponse(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
inline void AddResponse(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
{
if (AddResponseData(aRequestCommandPath, aData) != CHIP_NO_ERROR)
DataModelEncoderToTLV<CommandData> encoder(aData);
return AddResponseViaEncoder(aRequestCommandPath, CommandData::GetCommandId(), encoder);
}

/**
* API for adding a response with a given encoder of TLV data.
*
* The encoder would generally encode a ClusterName::Commands::CommandName::Type with
* the corresponding `GetCommandId` call.
*/
void AddResponseViaEncoder(const ConcreteCommandPath & aRequestCommandPath, CommandId commandId, EncoderToTLV & encoder)
{
if (AddResponseDataViaEncoder(aRequestCommandPath, commandId, encoder) != CHIP_NO_ERROR)
{
AddStatus(aRequestCommandPath, Protocols::InteractionModel::Status::Failure);
}
Expand Down Expand Up @@ -630,7 +691,6 @@ class CommandHandler
return PrepareInvokeResponseCommand(aResponseCommandPath, prepareParams);
}

// TODO(#31627): It would be awesome if we could remove this template all together.
/**
* 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
Expand All @@ -640,26 +700,14 @@ class CommandHandler
* responding to.
* @param [in] aData the data for the response.
*/
template <typename CommandData>
CHIP_ERROR TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
CHIP_ERROR TryAddResponseDataViaEncoder(const ConcreteCommandPath & aRequestCommandPath, CommandId commandId,
EncoderToTLV & encoder)
{
// This method, templated with CommandData, captures all the components needs
// from CommandData with as little code as possible.
//
// Previously, non-essential code was unnecessarily templated, leading to
// compilation and duplication N times. By isolating only the code segments
// that genuinely require templating, minimizes duplicate compiled code.
ConcreteCommandPath responseCommandPath = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId,
CommandData::GetCommandId() };
ConcreteCommandPath responseCommandPath = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, commandId };
ReturnErrorOnFailure(TryAddResponseDataPreEncode(aRequestCommandPath, responseCommandPath));
TLV::TLVWriter * writer = GetCommandDataIBTLVWriter();
VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE);
ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields), aData));

// FinishCommand technically should be refactored out as it is not a command that needs templating.
// But, because there is only a single function call, keeping it here takes less code. If there is
// ever more code between DataModel::Encode and the end of this function, it should be broken out into
// TryAddResponseDataPostEncode.
ReturnErrorOnFailure(encoder.Encode(*writer, TLV::ContextTag(CommandDataIB::Tag::kFields)));
return FinishCommand(/* aEndDataStruct = */ false);
}

Expand Down

0 comments on commit ad66d9c

Please sign in to comment.