-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move command handler registered list out of InteractionModelEngine and into a separate Registry
#34414
Merged
mergify
merged 12 commits into
project-chip:master
from
andy31415:command_handler_registry
Jul 23, 2024
Merged
Move command handler registered list out of InteractionModelEngine and into a separate Registry
#34414
mergify
merged 12 commits into
project-chip:master
from
andy31415:command_handler_registry
Jul 23, 2024
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
github-actions
bot
added
documentation
Improvements or additions to documentation
examples
controller
app
labels
Jul 19, 2024
pullapprove
bot
requested review from
andyg-apple,
anush-apple,
arkq,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chapongatien,
chrisdecenzo and
chshu
July 19, 2024 14:50
pullapprove
bot
requested review from
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang and
yufengwangca
July 19, 2024 14:50
PR #34414: Size comparison from fca7948 to e59ee29 Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
PR #34414: Size comparison from fca7948 to 07fdae9 Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
approved these changes
Jul 19, 2024
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
PR #34414: Size comparison from a10b47d to 84013df Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
tehampson
reviewed
Jul 23, 2024
tehampson
approved these changes
Jul 23, 2024
j-ororke
pushed a commit
to j-ororke/connectedhomeip
that referenced
this pull request
Jul 31, 2024
…d into a separate `Registry` (project-chip#34414) * Starting to define a commandhandlerregistry * Add missing files and replace some deprecated files usage * Replace all usages of IME to the command handler registry direct calls * Restyle * Fix wrong comment paste * Fix include * Remove unused include * Fix linter error * Update src/app/util/attribute-storage.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/controller/tests/TestServerCommandDispatch.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/controller/tests/TestServerCommandDispatch.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> --------- Co-authored-by: Andrei Litvin <andreilitvin@google.com> Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
app
controller
documentation
Improvements or additions to documentation
examples
github
review - approved
workflows
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 done for symmetry with the AttributeAccessInterface. The intent is to decouple usage (registering such interfaces now does not rely on the InteractionModelEngine) and allow eventual separation in DataModel processing (the processing of these are specific to data handling, same as attributes, and should not be separated out in the IME).
This has the downside that now separate IME instances cannot have separate command handler lists. I expect this to still be OK for now as for AttributeAccessInterface we did not have a separation to start with.
Changes
CommandHandlerInterfaceRegistry
with register/unregister functionsupgrade.md
with description of what functions to use to replace the old register/unregister methodsThe only odd TODO is that IME used to clear all registered command handler interfaces on shutdown, which seems somewhat reasonable for a list owned by the IME, however it does seem odd for a globally managed list. I kept that clear for now, however I expect we probably want to remove that clear in the future and eventually move it to
CodegenDataModel::Shutdown
once data models are in use.