From 16d6475ef81f890371ab0d1b1f7e42ea4318d13a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 27 May 2024 21:21:43 -0400 Subject: [PATCH] Minimize API surface that is templated for `CommandHandler` (#33620) * Define a EncodeToTLV inteface for the command handler * Slight documentation update * Comments update --- src/app/CommandHandler.h | 92 ++++++++++++++++++++++++++++++---------- 1 file changed, 70 insertions(+), 22 deletions(-) diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index cce19879a6c8a9..96b7af873554ed 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -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 ::Commands::::Type +/// structures. +template +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: @@ -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 - CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData) + inline CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData) + { + DataModelEncoderToTLV 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); }); } /** @@ -350,9 +399,21 @@ class CommandHandler * @param [in] aData the data for the response. */ template - 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 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); } @@ -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 @@ -640,26 +700,14 @@ class CommandHandler * responding to. * @param [in] aData the data for the response. */ - template - 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); }