Minimize API surface that is templated for CommandHandler
#33620
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andyg-apple,
anush-apple,
arkq,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
fessehaeve,
harimau-qirex,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kiel-apple,
kkasperczyk-no,
kpschoedel and
ksperling-apple
pullapprove
bot
requested review from
sharadb-amazon,
tcarmelveilleux,
tecimovic,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
|
PR #33620: Size comparison from 6b83689 to a32fb0c Increases above 0.2%:
Increases (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, esp32, linux, nrfconnect, stm32)
Decreases (79 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
jmartinez-silabs
approved these changes
May 27, 2024
cecille
approved these changes
May 27, 2024
tehampson
approved these changes
May 27, 2024
|
PR #33620: Size comparison from 6b83689 to 46d905a Increases above 0.2%:
Increases (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, esp32, linux, nrfconnect, stm32)
Decreases (79 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
| public: | ||
| virtual ~EncoderToTLV() = default; | ||
|
|
||
| virtual CHIP_ERROR Encode(TLV::TLVWriter &, TLV::Tag tag) = 0; |
There was a problem hiding this comment.
Suggest naming the writer arg
| class DataModelEncoderToTLV : public EncoderToTLV | ||
| { | ||
| public: | ||
| DataModelEncoderToTLV(const T & value) : mValue(value) {} |
There was a problem hiding this comment.
This has an ownership time consideration. Consider documenting.
| public: | ||
| DataModelEncoderToTLV(const T & value) : mValue(value) {} | ||
|
|
||
| virtual CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag) { return DataModel::Encode(writer, tag, mValue); } |
There was a problem hiding this comment.
Please remove virtual and specify override
| @@ -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) | |||
There was a problem hiding this comment.
I looked to reduce actual template explosion of typed function code and wanted to leave it up to the caller to generate the equivalent 'encoder' logic.
I will check to see if inline has any effect on code sizes ... I would probably be biased to use whatever uses the least flash.
| @@ -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 | |||
There was a problem hiding this comment.
Why is this CommandHandler-specific? And if it is, why is it in the chip::app namespace?
There was a problem hiding this comment.
Was the minimal delta. The correct thing is probably for this to live close to DataModel::Encode since it ends up being like that. I will move it as a followup.
| virtual CHIP_ERROR Encode(TLV::TLVWriter &, TLV::Tag tag) = 0; | ||
| }; | ||
|
|
||
| /// An `EncoderToTLV` the uses `DataModel::Encode` to encode things. |
| } | ||
|
|
||
| /** | ||
| * API for adding a data response. The encoded is generally expected to encode |
There was a problem hiding this comment.
What is "the encoded"?
| * 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, |
There was a problem hiding this comment.
Why does this need the "ViaEncoder" suffix? Why not just call this AddResponseData? It has different arguments from the template version, so confusion should not be an issue.
There was a problem hiding this comment.
It did not have different arguments originally (when I did not notice we need a commandId). I will change them back.
| * The encoder would generally encode a ClusterName::Commands::CommandName::Type with | ||
| * the corresponding `GetCommandId` call. | ||
| */ | ||
| void AddResponseViaEncoder(const ConcreteCommandPath & aRequestCommandPath, CommandId commandId, EncoderToTLV & encoder) |
There was a problem hiding this comment.
Again, why the "ViaEncoder" suffix?
There was a problem hiding this comment.
I had it without ViaEncoder at first however then <template typename T>void AddResponse(const ConcreteCommandPath&, T> and void AddResponse(const ConcreteCommandPath&, EncoderToTLV &encoder) seemed ambigous.
In the mean time I had to add commandId, so I could rename them as well I guess. This was iterated a few times, can be more.
| @@ -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, | |||
There was a problem hiding this comment.
Again, why the ViaEncoder suffix?
| // 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(); |
There was a problem hiding this comment.
Since this stuff is no longer templated:
- We no longer need this fine-grained slicing into things that do or do not depend on the now-nonexistent template argument.
- We do not need this thing in the header. It shouldn't be.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is in preparation to simplify CommandHandler usage in the CommandHandlerInterface: even though command handler has 50+ methods and 20+ data members, the external interface seems to be limited to 6 calls: addstatus/response/responsedata and some getters (fabricindex and subjectedescriptor) and an early flush call:
As a result, it seems highly desirable to have some
AddResposeimplementation that is non-templated and this also addresses a subset #31627 (and may save some flash as a result as the surface that is templated may become smaller even if that is not a primary goal of this PR).