Skip to content
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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

andy31415
Copy link
Contributor

@andy31415 andy31415 commented Dec 11, 2024

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.

Copy link

Review changes with  SemanticDiff

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Dec 11, 2024
Copy link

github-actions bot commented Dec 11, 2024

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)
platform target config section 41a9dea 8239d0e change % change
bl602 lighting-app bl602+mfd+littlefs+rpc FLASH 1353340 1353068 -272 -0.0
RAM 104112 104120 8 0.0
bl702 lighting-app bl702+eth FLASH 651826 651826 0 0.0
RAM 25353 25361 8 0.0
bl702+wifi FLASH 829154 829142 -12 -0.0
RAM 14093 14085 -8 -0.1
bl706+mfd+rpc+littlefs FLASH 1057626 1057358 -268 -0.0
RAM 23933 23925 -8 -0.0
bl702l lighting-app bl702l+mfd+littlefs FLASH 979000 978732 -268 -0.0
RAM 16596 16588 -8 -0.0
cc13x4_26x4 lighting-app LP_EM_CC1354P10_6 FLASH 839760 839504 -256 -0.0
RAM 123664 123656 -8 -0.0
lock-ftd LP_EM_CC1354P10_6 FLASH 825308 825028 -280 -0.0
RAM 125552 125544 -8 -0.0
pump-app LP_EM_CC1354P10_6 FLASH 772096 771832 -264 -0.0
RAM 114020 114020 0 0.0
pump-controller-app LP_EM_CC1354P10_6 FLASH 756284 756028 -256 -0.0
RAM 114228 114220 -8 -0.0
cc32xx air-purifier CC3235SF_LAUNCHXL FLASH 631050 630666 -384 -0.1
RAM 205824 205832 8 0.0
lock CC3235SF_LAUNCHXL FLASH 669646 669326 -320 -0.0
RAM 205968 205976 8 0.0
cyw30739 light CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 681505 681241 -264 -0.0
RAM 78724 78716 -8 -0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 701349 701093 -256 -0.0
RAM 81364 81356 -8 -0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 701349 701093 -256 -0.0
RAM 81364 81356 -8 -0.0
CYW930739M2EVB-02 unknown 2040 2040 0 0.0
FLASH 658293 658021 -272 -0.0
RAM 73792 73784 -8 -0.0
light-switch CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 618065 617793 -272 -0.0
RAM 71708 71708 0 0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 637693 637429 -264 -0.0
RAM 74252 74252 0 0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 637693 637429 -264 -0.0
RAM 74252 74252 0 0.0
lock CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 637465 637201 -264 -0.0
RAM 74724 74716 -8 -0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 657173 656909 -264 -0.0
RAM 77268 77260 -8 -0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 657173 656909 -264 -0.0
RAM 77268 77260 -8 -0.0
thermostat CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 613925 613661 -264 -0.0
RAM 68812 68804 -8 -0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 633777 633513 -264 -0.0
RAM 71444 71436 -8 -0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 633777 633513 -264 -0.0
RAM 71444 71436 -8 -0.0
efr32 lock-app BRD4187C FLASH 932340 932060 -280 -0.0
RAM 160192 160188 -4 -0.0
BRD4338a FLASH 746144 745640 -504 -0.1
RAM 233320 233336 16 0.0
window-app BRD4187C FLASH 1024784 1024312 -472 -0.0
RAM 128296 128304 8 0.0
esp32 all-clusters-app c3devkit DRAM 95360 95360 0 0.0
FLASH 1543480 1543124 -356 -0.0
IRAM 82542 82542 0 0.0
m5stack DRAM 116312 116320 8 0.0
FLASH 1550134 1549798 -336 -0.0
IRAM 117039 117039 0 0.0
linux air-purifier-app debug unknown 4720 4720 0 0.0
FLASH 2715063 2713377 -1686 -0.1
RAM 129800 129832 32 0.0
all-clusters-app debug unknown 5560 5560 0 0.0
FLASH 6007064 6004430 -2634 -0.0
RAM 523544 523576 32 0.0
all-clusters-minimal-app debug unknown 5456 5456 0 0.0
FLASH 5344804 5343264 -1540 -0.0
RAM 242600 242632 32 0.0
bridge-app debug unknown 5440 5440 0 0.0
FLASH 4684372 4682764 -1608 -0.0
RAM 218416 218448 32 0.0
chip-tool debug unknown 5992 5992 0 0.0
FLASH 12847832 12847082 -750 -0.0
RAM 582474 582506 32 0.0
chip-tool-ipv6only arm64 unknown 21352 21344 -8 -0.0
FLASH 10982560 10981168 -1392 -0.0
RAM 633392 633400 8 0.0
fabric-admin debug unknown 5816 5816 0 0.0
FLASH 11255157 11254407 -750 -0.0
RAM 582850 582882 32 0.0
fabric-bridge-app debug unknown 4696 4696 0 0.0
FLASH 4509948 4508308 -1640 -0.0
RAM 205600 205632 32 0.0
fabric-sync debug unknown 4936 4936 0 0.0
FLASH 5608837 5607205 -1632 -0.0
RAM 472584 472616 32 0.0
lighting-app debug+rpc+ui unknown 6104 6104 0 0.0
FLASH 5621073 5619457 -1616 -0.0
RAM 228792 228824 32 0.0
lock-app debug unknown 5376 5376 0 0.0
FLASH 4733612 4732008 -1604 -0.0
RAM 204776 204808 32 0.0
ota-provider-app debug unknown 4752 4752 0 0.0
FLASH 4359350 4357858 -1492 -0.0
RAM 198448 198480 32 0.0
ota-requestor-app debug unknown 4688 4688 0 0.0
FLASH 4498342 4496850 -1492 -0.0
RAM 203032 203064 32 0.0
shell debug unknown 4248 4248 0 0.0
FLASH 3030077 3029005 -1072 -0.0
RAM 160424 160456 32 0.0
thermostat-no-ble arm64 unknown 9536 9528 -8 -0.1
FLASH 4103472 4101448 -2024 -0.0
RAM 243040 243048 8 0.0
tv-app debug unknown 5704 5704 0 0.0
FLASH 5958773 5957269 -1504 -0.0
RAM 596016 596048 32 0.0
tv-casting-app debug unknown 5288 5288 0 0.0
FLASH 11054589 11053053 -1536 -0.0
RAM 692184 692216 32 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 FLASH 917616 917244 -372 -0.0
RAM 143292 143280 -12 -0.0
nrf7002dk_nrf5340_cpuapp FLASH 890104 889764 -340 -0.0
RAM 141487 141475 -12 -0.0
all-clusters-minimal-app nrf52840dk_nrf52840 FLASH 851760 851512 -248 -0.0
RAM 142200 142188 -12 -0.0
nxp contact k32w0+release FLASH 585440 585160 -280 -0.0
RAM 71080 71072 -8 -0.0
mcxw71+release FLASH 600048 599768 -280 -0.0
RAM 63176 63168 -8 -0.0
light k32w0+release FLASH 612396 612132 -264 -0.0
RAM 70472 70464 -8 -0.0
k32w1+release FLASH 686576 686320 -256 -0.0
RAM 48808 48800 -8 -0.0
lock mcxw71+release FLASH 762928 762664 -264 -0.0
RAM 70844 70836 -8 -0.0
psoc6 all-clusters cy8ckit_062s2_43012 FLASH 1646364 1645588 -776 -0.0
RAM 212104 212112 8 0.0
all-clusters-minimal cy8ckit_062s2_43012 FLASH 1554108 1553628 -480 -0.0
RAM 208904 208912 8 0.0
light cy8ckit_062s2_43012 FLASH 1469436 1468932 -504 -0.0
RAM 200880 200888 8 0.0
lock cy8ckit_062s2_43012 FLASH 1467164 1466676 -488 -0.0
RAM 225240 225248 8 0.0
qpg lighting-app qpg6105+debug FLASH 664008 663752 -256 -0.0
RAM 105424 105424 0 0.0
lock-app qpg6105+debug FLASH 621796 621540 -256 -0.0
RAM 99868 99868 0 0.0
stm32 light STM32WB5MM-DK FLASH 484720 484456 -264 -0.1
RAM 144880 144872 -8 -0.0
telink bridge-app tlsr9258a FLASH 682920 682742 -178 -0.0
RAM 91208 91196 -12 -0.0
contact-sensor-app tlsr9528a_retention FLASH 623350 623178 -172 -0.0
RAM 31440 31428 -12 -0.0
light-app-ota-compress-lzma-shell-factory-data tl3218x FLASH 772180 772000 -180 -0.0
RAM 49300 49288 -12 -0.0
light-switch-app-ota-compress-lzma-shell-factory-data tlsr9528a FLASH 710774 710596 -178 -0.0
RAM 73504 73492 -12 -0.0
lighting-app-ota-factory-data tlsr9118bdk40d FLASH 627794 627614 -180 -0.0
RAM 142140 142128 -12 -0.0
lighting-app-ota-rpc-factory-data-4mb tlsr9518adk80d FLASH 813808 813628 -180 -0.0
RAM 99684 99672 -12 -0.0
tizen all-clusters-app arm unknown 4988 4984 -4 -0.1
FLASH 1732528 1730928 -1600 -0.1
RAM 90744 90752 8 0.0
chip-tool-ubsan arm unknown 10804 10800 -4 -0.0
FLASH 17969958 17970294 336 0.0
RAM 7840924 7840820 -104 -0.0

@andy31415 andy31415 marked this pull request as ready for review December 12, 2024 04:32
Copy link

github-actions bot commented Dec 12, 2024

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)
platform target config section 41a9dea 895f138 change % change
bl602 lighting-app bl602+mfd+littlefs+rpc FLASH 1353340 1353322 -18 -0.0
RAM 104112 104112 0 0.0
bl702 lighting-app bl702+eth FLASH 651826 651826 0 0.0
RAM 25353 25353 0 0.0
bl702+wifi FLASH 829154 829142 -12 -0.0
RAM 14093 14077 -16 -0.1
bl706+mfd+rpc+littlefs FLASH 1057626 1057612 -14 -0.0
RAM 23933 23917 -16 -0.1
bl702l lighting-app bl702l+mfd+littlefs FLASH 979000 978986 -14 -0.0
RAM 16596 16580 -16 -0.1
cc13x4_26x4 lighting-app LP_EM_CC1354P10_6 FLASH 839760 839696 -64 -0.0
RAM 123664 123648 -16 -0.0
lock-ftd LP_EM_CC1354P10_6 FLASH 825308 825220 -88 -0.0
RAM 125552 125536 -16 -0.0
pump-app LP_EM_CC1354P10_6 FLASH 772096 772024 -72 -0.0
RAM 114020 114012 -8 -0.0
pump-controller-app LP_EM_CC1354P10_6 FLASH 756284 756204 -80 -0.0
RAM 114228 114212 -16 -0.0
cc32xx air-purifier CC3235SF_LAUNCHXL FLASH 631050 630954 -96 -0.0
RAM 205824 205824 0 0.0
lock CC3235SF_LAUNCHXL FLASH 669646 669614 -32 -0.0
RAM 205968 205968 0 0.0
cyw30739 light CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 681505 681433 -72 -0.0
RAM 78724 78708 -16 -0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 701349 701285 -64 -0.0
RAM 81364 81348 -16 -0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 701349 701285 -64 -0.0
RAM 81364 81348 -16 -0.0
CYW930739M2EVB-02 unknown 2040 2040 0 0.0
FLASH 658293 658213 -80 -0.0
RAM 73792 73776 -16 -0.0
light-switch CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 618065 617977 -88 -0.0
RAM 71708 71700 -8 -0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 637693 637613 -80 -0.0
RAM 74252 74244 -8 -0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 637693 637613 -80 -0.0
RAM 74252 74244 -8 -0.0
lock CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 637465 637385 -80 -0.0
RAM 74724 74708 -16 -0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 657173 657093 -80 -0.0
RAM 77268 77252 -16 -0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 657173 657093 -80 -0.0
RAM 77268 77252 -16 -0.0
thermostat CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 613925 613853 -72 -0.0
RAM 68812 68796 -16 -0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 633777 633705 -72 -0.0
RAM 71444 71428 -16 -0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 633777 633705 -72 -0.0
RAM 71444 71428 -16 -0.0
efr32 lock-app BRD4187C FLASH 932340 932252 -88 -0.0
RAM 160192 160180 -12 -0.0
BRD4338a FLASH 746144 745928 -216 -0.0
RAM 233320 233320 0 0.0
window-app BRD4187C FLASH 1024784 1024600 -184 -0.0
RAM 128296 128296 0 0.0
esp32 all-clusters-app c3devkit DRAM 95360 95352 -8 -0.0
FLASH 1543480 1543236 -244 -0.0
IRAM 82542 82542 0 0.0
m5stack DRAM 116312 116304 -8 -0.0
FLASH 1550134 1550006 -128 -0.0
IRAM 117039 117039 0 0.0
linux air-purifier-app debug unknown 4720 4720 0 0.0
FLASH 2715063 2713981 -1082 -0.0
RAM 129800 129800 0 0.0
all-clusters-app debug unknown 5560 5560 0 0.0
FLASH 6007064 6005034 -2030 -0.0
RAM 523544 523544 0 0.0
all-clusters-minimal-app debug unknown 5456 5456 0 0.0
FLASH 5344804 5343868 -936 -0.0
RAM 242600 242600 0 0.0
bridge-app debug unknown 5440 5440 0 0.0
FLASH 4684372 4683368 -1004 -0.0
RAM 218416 218416 0 0.0
chip-tool debug unknown 5992 5992 0 0.0
FLASH 12847832 12847812 -20 -0.0
RAM 582474 582474 0 0.0
chip-tool-ipv6only arm64 unknown 21352 21344 -8 -0.0
FLASH 10982560 1098227 -288 -0.0
RAM 633392 633392 0 0.0
fabric-admin debug unknown 5816 5816 0 0.0
FLASH 11255157 11255137 -20 -0.0
RAM 582850 582850 0 0.0
fabric-bridge-app debug unknown 4696 4696 0 0.0
FLASH 4509948 4508912 -1036 -0.0
RAM 205600 205600 0 0.0
fabric-sync debug unknown 4936 4936 0 0.0
FLASH 5608837 5609013 176 0.0
RAM 472584 472584 0 0.0
lighting-app debug+rpc+ui unknown 6104 6104 0 0.0
FLASH 5621073 5620065 -1008 -0.0
RAM 228792 228792 0 0.0
lock-app debug unknown 5376 5376 0 0.0
FLASH 4733612 4732612 -1000 -0.0
RAM 204776 204776 0 0.0
ota-provider-app debug unknown 4752 4752 0 0.0
FLASH 4359350 4358462 -888 -0.0
RAM 198448 198448 0 0.0
ota-requestor-app debug unknown 4688 4688 0 0.0
FLASH 4498342 4497454 -888 -0.0
RAM 203032 203032 0 0.0
shell debug unknown 4248 4248 0 0.0
FLASH 3030077 3029613 -464 -0.0
RAM 160424 160424 0 0.0
thermostat-no-ble arm64 unknown 9536 9528 -8 -0.1
FLASH 4103472 4102424 -1048 -0.0
RAM 243040 243040 0 0.0
tv-app debug unknown 5704 5704 0 0.0
FLASH 5958773 5957989 -784 -0.0
RAM 596016 596016 0 0.0
tv-casting-app debug unknown 5288 5288 0 0.0
FLASH 11054589 11053661 -928 -0.0
RAM 692184 692184 0 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 FLASH 917616 917428 -188 -0.0
RAM 143292 143280 -12 -0.0
nrf7002dk_nrf5340_cpuapp FLASH 890104 889892 -212 -0.0
RAM 141487 141475 -12 -0.0
all-clusters-minimal-app nrf52840dk_nrf52840 FLASH 851760 851696 -64 -0.0
RAM 142200 142188 -12 -0.0
nxp contact k32w0+release FLASH 585440 585352 -88 -0.0
RAM 71080 71064 -16 -0.0
mcxw71+release FLASH 600048 599960 -88 -0.0
RAM 63176 63160 -16 -0.0
light k32w0+release FLASH 612396 612324 -72 -0.0
RAM 70472 70456 -16 -0.0
k32w1+release FLASH 686576 686512 -64 -0.0
RAM 48808 48792 -16 -0.0
lock mcxw71+release FLASH 762928 762856 -72 -0.0
RAM 70844 70828 -16 -0.0
psoc6 all-clusters cy8ckit_062s2_43012 FLASH 1646364 1645876 -488 -0.0
RAM 212104 212104 0 0.0
all-clusters-minimal cy8ckit_062s2_43012 FLASH 1554108 1553900 -208 -0.0
RAM 208904 208904 0 0.0
light cy8ckit_062s2_43012 FLASH 1469436 1469220 -216 -0.0
RAM 200880 200880 0 0.0
lock cy8ckit_062s2_43012 FLASH 1467164 1466964 -200 -0.0
RAM 225240 225240 0 0.0
qpg lighting-app qpg6105+debug FLASH 664008 663944 -64 -0.0
RAM 105424 105416 -8 -0.0
lock-app qpg6105+debug FLASH 621796 621716 -80 -0.0
RAM 99868 99860 -8 -0.0
stm32 light STM32WB5MM-DK FLASH 484720 484648 -72 -0.0
RAM 144880 144864 -16 -0.0
telink bridge-app tlsr9258a FLASH 682920 682850 -70 -0.0
RAM 91208 91196 -12 -0.0
contact-sensor-app tlsr9528a_retention FLASH 623350 623286 -64 -0.0
RAM 31440 31428 -12 -0.0
light-app-ota-compress-lzma-shell-factory-data tl3218x FLASH 772180 772108 -72 -0.0
RAM 49300 49288 -12 -0.0
light-switch-app-ota-compress-lzma-shell-factory-data tlsr9528a FLASH 710774 710704 -70 -0.0
RAM 73504 73492 -12 -0.0
lighting-app-ota-factory-data tlsr9118bdk40d FLASH 627794 627722 -72 -0.0
RAM 142140 142128 -12 -0.0
lighting-app-ota-rpc-factory-data-4mb tlsr9518adk80d FLASH 813808 813736 -72 -0.0
RAM 99684 99672 -12 -0.0
tizen all-clusters-app arm unknown 4988 4984 -4 -0.1
FLASH 1732528 1731384 -1144 -0.1
RAM 90744 90744 0 0.0
chip-tool-ubsan arm unknown 10804 10800 -4 -0.0
FLASH 17969958 17972702 2744 0.0
RAM 7840924 7842264 1340 0.0

Comment on lines -62 to 64
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;

Copy link
Contributor

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.

Copy link
Contributor Author

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.

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 review - pending
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants