Command handler interface mixin minimal change #38389
Draft
Conversation
This file contains hidden or 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
Co-authored-by: Andrei Litvin <[email protected]>
ratgr
force-pushed
the
command-handler-interface-mixin-minimal-change
branch
from
dc674ec to
9844128
Compare
…erface-mixin-minimal-change
|
PR #38389: Size comparison from 6367083 to d467f2b Full report (3 builds for cc32xx, stm32)
|
|
PR #38389: Size comparison from 6367083 to ea3b703 Increases above 0.2%:
Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Apr 17, 2025
| } // namespace detail | ||
|
|
||
| template <class TLambda> | ||
| struct SplittedLambda : detail::SplittedLambdaCallerImpl<decltype(&TLambda::operator())> |
There was a problem hiding this comment.
all these classes need documentation (including the Shim one): what is their intent, what do they do, what is their usage.
andy31415
reviewed
Apr 17, 2025
andy31415
reviewed
Apr 17, 2025
| template <> | ||
| struct ClusterMetadataProvider<DataModel::AttributeEntry, Clusters::AccessControl::Id> | ||
| { | ||
| static constexpr DataModel::AttributeEntry EntryFor(AttributeId commandId) |
There was a problem hiding this comment.
For constexpr purposes, this allows me to write:
EntryFor(Acl::Id)instead of
Acl::kMetadataEntryAt a first glance, this seems to replace a one-liner with another one-liner of similar length: we save 4 characters each time, however we add more complexity.
There was a problem hiding this comment.
Yes, I plan to add the [[deprecated]] attribute to this and all the other functions that are used, so we can retire them fast.
But this is all used to be able to provide a no friction update to the new CHI interface, later removing the SHIM
Hopefully we can go the other route, and just update
andy31415
reviewed
Apr 17, 2025
| @@ -232,5 +235,85 @@ class CommandHandlerInterface | |||
| CommandHandlerInterface * mNext = nullptr; | |||
| }; | |||
|
|
|||
| template <ClusterId... TClusterIds> | |||
| class CommandHandlerInterfaceShim : public CommandHandlerInterface | |||
There was a problem hiding this comment.
Need comments on intent here. Maybe should be located somewhere else than CommandHandlerInterface.h.
What is the usage here? is this to allow usage of the old interface implementations that return IDs only? Is this expected to be a long term solution? This feels like very high complexity.
There was a problem hiding this comment.
I see the descriptions are in the PR summary. That seems ok, however then I would maybe consider:
- separate all these out into some backwards-compatibility layer
- I believe they generally should not be used so maybe we should plan for their removal at some point in time
I would also generally consider not providing these shims at all and requesting an update as the update code does not seem as complicated.
There was a problem hiding this comment.
well, yes, I think it is a low effort update to just do it but this is an automatable change, to satisfy the requirements we discussed with Boris:
I got 3 drafts
- user makes no change at all (Command handler interface mixin no change #38435)
- user makes an automatable change (This change)
- user just replaces ID to kMetadataEntry
Once we select one I will add documentation to whatever needs to be added, and tidy up everything (moving things into their files, removing unnesesary code)
If you look at the code size changes this code is quite minimal, specially when the code is used only on the Network commissioning side
|
PR #38389: Size comparison from dc07b29 to 5015de7 Increases above 0.2%:
Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
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 one of 3 proposals I'm drafting for #37139
As an example this PR implements the new SHIM on the NetworkCommissioning cluster
Code TLDR
This code adds 3 big pieces of import to CHIP
Command Handler Interface Shim
On this change Command Handler Interface is updated to use the new builder based approach and a SHIM is provided to ease into updating
CommandHandlerInterfaceShim provides an interface compatible with old code, the templated ClusterIds are used for generation of a CompileTimeCalculated Switch statement with the clusters required for the Interface, if none are provided then it generates a Switch for all possible clusters
New implementation of AcceptedCommandEntryFor
As an interface to provide the old data, I added the AcceptedCommandEntryFor
it is templated with the ClusterIds and it dynamically at compile time generates a switch-like jump-table (or a simple branch depending on compiler optimizations) for the runtime selection of clusters
Generated Metadata
Adds more metadata through the specialization of the class
ClusterMetadataProvider<MetadataType, Cluster>as to avoid adding more unnecessary code to your build unless explicitly requested
Expected Use
Automated update
This change provides very low friction to update, this should be enough, however it might add too much code to your implementation
Manual Update
This change provides very low friction to update, this will generate a very minimal change to allow you to target your required cluster
Replace inheriting CommandHandlerInterface to CommandHandlerInterfaceShim<> with a list of the clusters are required for your use case.
Testing