-
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
Update CommandHandlerInterface
to have a bool accepts/generates
instead of an iterator-based callback
#36809
base: master
Are you sure you want to change the base?
Update CommandHandlerInterface
to have a bool accepts/generates
instead of an iterator-based callback
#36809
Conversation
PR #36809: Size comparison from 41a9dea to 8239d0e Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
PR #36809: Size comparison from 41a9dea to 895f138 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override; | ||
CHIP_ERROR EnumerateGeneratedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override; | ||
bool AcceptsCommandId(const ConcreteCommandPath & commandPath) override; | ||
bool GeneratesCommandId(const ConcreteCommandPath & commandPath) override; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the ability of CHI to not require the ZAP metadata to include a command. The AcceptedCommandList should not need to be exhaustive and include commands not generated. Otherwise we are coupling MORE, and also requiring that ZAP metadata includes all commands, when it was not required before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fair concern. Keeping this comment open as a blocker for the PR to see what we do here.
Ideally I would like CHI to be fully stand alone then, but then we need some interface that is similar to DataModel::Provider
capabilities (i.e. return iteration AND metadata). Still need to determine what to do here.
CommandHandlerInterface is still highly tied to metadata that is outside of it: commands still need invoke privileges and qualities (timed invoke, supports large objects, fabric scoped) and cannot function independently.
This changes the logic from using CHI to iterate through commands and allowing it to report commands that are missing in metadata to be able to filter-out commands that do exist in metadata.