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.

Sign up for GitHub

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

Jump to bottom

[nwprov] Implement AcceptedCommandList and GeneratedCommandList #17387

Merged
merged 1 commit into from
Apr 18, 2022
Merged

[nwprov] Implement AcceptedCommandList and GeneratedCommandList #17387

merged 1 commit into from
Apr 18, 2022

Conversation

erjiaqing
Copy link
Contributor

@erjiaqing erjiaqing commented Apr 14, 2022 •
edited
Loading

Problem

Fixes #16990
Fixes #17104
Fixes #17105

Change overview

Implement AcceptedCommandList and GeneratedCommandList

Testing

Updated src/controller/python/test/test_scripts/network_commissioning.py

@github-actions
Copy link

github-actions bot commented Apr 14, 2022 •
edited
Loading

PR #17387: Size comparison from 21024aa to a8a020e

Increases (22 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
platform target config section 21024aa a8a020e change % change
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read only) 683351 683543 192 0.0
.rodata 102975 103023 48 0.0
.text 579896 580040 144 0.0
lock-ftd LP_CC2652R7 (read only) 639823 640023 200 0.0
.rodata 79951 80007 56 0.1
.text 559380 559524 144 0.0
lock-mtd LP_CC2652R7 (read only) 588567 588767 200 0.0
.rodata 79831 79887 56 0.1
.text 508244 508388 144 0.0
pump-app LP_CC2652R7 (read only) 648571 648763 192 0.0
.rodata 75435 75483 48 0.1
.text 572648 572792 144 0.0
pump-controller-app LP_CC2652R7 (read only) 641871 642063 192 0.0
.rodata 78767 78815 48 0.1
.text 562616 562760 144 0.0
cyw30739 light cyw930739m2evb_01 (read/write) 618458 618650 192 0.0
.app_xip_area 525160 525352 192 0.0
lock cyw930739m2evb_01 (read/write) 576058 576258 200 0.0
.app_xip_area 484296 484496 200 0.0
ota-requestor-no-progress-logging cyw930739m2evb_01 (read/write) 565234 565426 192 0.0
.app_xip_area 463844 464036 192 0.0
efr32 lighting-app BRD4161A (read only) 907396 907644 248 0.0
.text 907388 907636 248 0.0
BRD4161A+rpc (read only) 941772 942020 248 0.0
.text 941764 942012 248 0.0
window-app BRD4161A (read only) 844052 844300 248 0.0
.text 844044 844292 248 0.0
esp32 all-clusters-app c3devkit (read only) 979666 979856 190 0.0
(read/write) 1397410 1397450 40 0.0
.flash.rodata 201432 201472 40 0.0
.flash.text 979666 979856 190 0.0
m5stack (read only) 1035075 1035215 140 0.0
(read/write) 465164 465220 56 0.0
.flash.rodata 231024 231080 56 0.0
.flash.text 1029691 1029831 140 0.0
k32w light k32w061+release (read/write) 686876 687068 192 0.0
.text 600904 601096 192 0.0
lock k32w061+release (read/write) 691320 691520 200 0.0
.text 604812 605012 200 0.0
linux thermostat-no-ble arm64 (read only) 2353652 2354612 960 0.0
.rodata 144612 144660 48 0.0
.text 1980384 1981296 912 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read/write) 2368820 2369004 184 0.0
.text 1331420 1331604 184 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1162867 1163059 192 0.0
rodata 147092 147140 48 0.0
text 800624 800768 144 0.0
p6 all-clusters-app default (read/write) 2514624 2514872 248 0.0
.text 1472888 1473136 248 0.0
light-app default (read/write) 2415080 2415320 240 0.0
.text 1373344 1373584 240 0.0
lock-app default (read/write) 2378648 2378888 240 0.0
.text 1336912 1337152 240 0.0
telink lighting-app tlsr9518adk80d (read/write) 800852 801068 216 0.0
text 569742 569924 182 0.0
Decreases (1 build for cc13x2_26x2)
platform target config section 21024aa a8a020e change % change
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read/write) 169840 169648 -192 -0.1
Full report (23 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
platform target config section 21024aa a8a020e change % change
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read only) 683351 683543 192 0.0
(read/write) 169840 169648 -192 -0.1
.bss 76176 76176 0 0.0
.data 3380 3380 0 0.0
.rodata 102975 103023 48 0.0
.text 579896 580040 144 0.0
lock-ftd LP_CC2652R7 (read only) 639823 640023 200 0.0
(read/write) 151220 151220 0 0.0
.bss 74152 74152 0 0.0
.data 3212 3212 0 0.0
.rodata 79951 80007 56 0.1
.text 559380 559524 144 0.0
lock-mtd LP_CC2652R7 (read only) 588567 588767 200 0.0
(read/write) 146940 146940 0 0.0
.bss 69872 69872 0 0.0
.data 3212 3212 0 0.0
.rodata 79831 79887 56 0.1
.text 508244 508388 144 0.0
pump-app LP_CC2652R7 (read only) 648571 648763 192 0.0
(read/write) 152516 152516 0 0.0
.bss 74648 74648 0 0.0
.data 3244 3244 0 0.0
.rodata 75435 75483 48 0.1
.text 572648 572792 144 0.0
pump-controller-app LP_CC2652R7 (read only) 641871 642063 192 0.0
(read/write) 152184 152184 0 0.0
.bss 74352 74352 0 0.0
.data 3208 3208 0 0.0
.rodata 78767 78815 48 0.1
.text 562616 562760 144 0.0
cyw30739 light cyw930739m2evb_01 (read/write) 618458 618650 192 0.0
.app_xip_area 525160 525352 192 0.0
.bss 75964 75964 0 0.0
.data 684 684 0 0.0
.rodata 0 0 0 0.0
.text 0 0 0 0.0
lock cyw930739m2evb_01 (read/write) 576058 576258 200 0.0
.app_xip_area 484296 484496 200 0.0
.bss 74460 74460 0 0.0
.data 648 648 0 0.0
.rodata 0 0 0 0.0
.text 0 0 0 0.0
ota-requestor-no-progress-logging cyw930739m2evb_01 (read/write) 565234 565426 192 0.0
.app_xip_area 463844 464036 192 0.0
.bss 83792 83792 0 0.0
.data 564 564 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
efr32 lighting-app BRD4161A (read only) 907396 907644 248 0.0
(read/write) 133152 133152 0 0.0
.bss 131112 131112 0 0.0
.data 2040 2040 0 0.0
.text 907388 907636 248 0.0
BRD4161A+rpc (read only) 941772 942020 248 0.0
(read/write) 149836 149836 0 0.0
.bss 147592 147592 0 0.0
.data 2244 2244 0 0.0
.text 941764 942012 248 0.0
window-app BRD4161A (read only) 844052 844300 248 0.0
(read/write) 131156 131156 0 0.0
.bss 129208 129208 0 0.0
.data 1948 1948 0 0.0
.text 844044 844292 248 0.0
esp32 all-clusters-app c3devkit (read only) 979666 979856 190 0.0
(read/write) 1397410 1397450 40 0.0
.dram0.bss 62632 62632 0 0.0
.dram0.data 14420 14420 0 0.0
.flash.rodata 201432 201472 40 0.0
.flash.text 979666 979856 190 0.0
.iram0.text 62016 62016 0 0.0
m5stack (read only) 1035075 1035215 140 0.0
(read/write) 465164 465220 56 0.0
.dram0.bss 68152 68152 0 0.0
.dram0.data 34152 34152 0 0.0
.flash.rodata 231024 231080 56 0.0
.flash.text 1029691 1029831 140 0.0
.iram0.text 123107 123107 0 0.0
k32w light k32w061+release (read/write) 686876 687068 192 0.0
.bss 78136 78136 0 0.0
.data 2036 2036 0 0.0
.text 600904 601096 192 0.0
lock k32w061+release (read/write) 691320 691520 200 0.0
.bss 78712 78712 0 0.0
.data 1996 1996 0 0.0
.text 604812 605012 200 0.0
linux chip-tool-no-interactive-ipv6only arm64 (read only) 10343140 10343140 0 0.0
(read/write) 492273 492273 0 0.0
.bss 41025 41025 0 0.0
.data 1168 1168 0 0.0
.data.rel.ro 388840 388840 0 0.0
.dynamic 560 560 0 0.0
.got 57440 57440 0 0.0
.init 24 24 0 0.0
.init_array 184 184 0 0.0
.rodata 513204 513204 0 0.0
.text 8726516 8726516 0 0.0
thermostat-no-ble arm64 (read only) 2353652 2354612 960 0.0
(read/write) 151137 151137 0 0.0
.bss 63169 63169 0 0.0
.data 1424 1424 0 0.0
.data.rel.ro 78752 78752 0 0.0
.dynamic 560 560 0 0.0
.got 4768 4768 0 0.0
.init 24 24 0 0.0
.init_array 368 368 0 0.0
.rodata 144612 144660 48 0.0
.text 1980384 1981296 912 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2368820 2369004 184 0.0
.bss 185244 185244 0 0.0
.data 5840 5840 0 0.0
.text 1331420 1331604 184 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1162867 1163059 192 0.0
bss 136536 136536 0 0.0
rodata 147092 147140 48 0.0
text 800624 800768 144 0.0
p6 all-clusters-app default (read/write) 2514624 2514872 248 0.0
.bss 118648 118648 0 0.0
.data 2768 2768 0 0.0
.text 1472888 1473136 248 0.0
light-app default (read/write) 2415080 2415320 240 0.0
.bss 112144 112144 0 0.0
.data 2576 2576 0 0.0
.text 1373344 1373584 240 0.0
lock-app default (read/write) 2378648 2378888 240 0.0
.bss 111888 111888 0 0.0
.data 2536 2536 0 0.0
.text 1336912 1337152 240 0.0
telink lighting-app tlsr9518adk80d (read/write) 800852 801068 216 0.0
bss 69996 69996 0 0.0
noinit 40416 40416 0 0.0
text 569742 569924 182 0.0

andy31415
andy31415 approved these changes Apr 14, 2022
View reviewed changes
Copy link
Contributor

@andy31415 andy31415 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, however the spec says:

[options="header",valign="middle"]
|===
| ID       | Name                 | Element               | Type             | Constraint | Quality | Access  | Default | Conformance
| 0xFFFD  s| ClusterRevision      | attribute             | uint16           | 1 to max   | F       | R V     |         | M
| 0xFFFC  s| FeatureMap           | attribute             | map32            |            | F       | R V     |       0 | M
| 0xFFFB  s| AttributeList        | attribute             | list[attrib-id]  |            | F       | R V     |         | M
| 0xFFFA  s| EventList            | attribute             | list[event-id]   |            | F       | R V     |         | M
| 0xFFF9  s| AcceptedCommandList  | attribute             | list[command-id] |            | F       | R V     |         | M
| 0xFFF8  s| GeneratedCommandList | attribute             | list[command-id] |            | F       | R V     |         | M
| 0xFE    s| FabricIndex          | struct or event field | fabric-idx       | 1 to max   |         | R V F   |         | fabric-scoped
|===

...

==== AcceptedCommandList
This attribute is a list of client generated commands which are supported by this cluster server instance.

Each instance of a cluster SHALL support this attribute.

This attribute SHALL be a list of the command IDs for client generated commands that are supported and processed by the server.

For each client request command in this list that mandates a response from the server, the response command SHALL be in the `GeneratedCommandList`.

So each of these must be for every single cluster.
Should ember generate these instead for most clusters?

I understand that network commissioning is special since we are using features and should control the state. What is the plan for all other clusters?

Do we need to add these accepted/generated command list to the zap files?
I see these in NetworkCommissioing in the window-app.matter, however I do not see them in examples/all-clusters-app/all-clusters-common/all-clusters-app.matter

@erjiaqing
Copy link
Contributor Author

This works, however the spec says:

[options="header",valign="middle"]
|===
| ID       | Name                 | Element               | Type             | Constraint | Quality | Access  | Default | Conformance
| 0xFFFD  s| ClusterRevision      | attribute             | uint16           | 1 to max   | F       | R V     |         | M
| 0xFFFC  s| FeatureMap           | attribute             | map32            |            | F       | R V     |       0 | M
| 0xFFFB  s| AttributeList        | attribute             | list[attrib-id]  |            | F       | R V     |         | M
| 0xFFFA  s| EventList            | attribute             | list[event-id]   |            | F       | R V     |         | M
| 0xFFF9  s| AcceptedCommandList  | attribute             | list[command-id] |            | F       | R V     |         | M
| 0xFFF8  s| GeneratedCommandList | attribute             | list[command-id] |            | F       | R V     |         | M
| 0xFE    s| FabricIndex          | struct or event field | fabric-idx       | 1 to max   |         | R V F   |         | fabric-scoped
|===

...

==== AcceptedCommandList
This attribute is a list of client generated commands which are supported by this cluster server instance.

Each instance of a cluster SHALL support this attribute.

This attribute SHALL be a list of the command IDs for client generated commands that are supported and processed by the server.

For each client request command in this list that mandates a response from the server, the response command SHALL be in the `GeneratedCommandList`.

So each of these must be for every single cluster. Should ember generate these instead for most clusters?

I understand that network commissioning is special since we are using features and should control the state. What is the plan for all other clusters?

Do we need to add these accepted/generated command list to the zap files? I see these in NetworkCommissioing in the window-app.matter, however I do not see them in examples/all-clusters-app/all-clusters-common/all-clusters-app.matter

Ember is already generating this, they were introduced in #14299

However, the network commissioning is using a dynamic approach since the platforms and the apps are so different, so we made the cluster build / run time configurable, in this case, generated command list is not enough.

@bzbarsky-apple
Copy link
Contributor

Do we need to add these accepted/generated command list to the zap files?

For the record: no. IM auto-synthesizes them from data structures codegen spits out.

@bzbarsky-apple bzbarsky-apple merged commit dc0b934 into project-chip:master Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anush-apple anush-apple Awaiting requested review from anush-apple

@austinh0 austinh0 Awaiting requested review from austinh0

@Byungjoo-Lee Byungjoo-Lee Awaiting requested review from Byungjoo-Lee

@andy31415 andy31415 Awaiting requested review from andy31415

@carol-apple carol-apple Awaiting requested review from carol-apple

@cecille cecille Awaiting requested review from cecille

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@chshu chshu Awaiting requested review from chshu

@chulspro chulspro Awaiting requested review from chulspro

@Damian-Nordic Damian-Nordic Awaiting requested review from Damian-Nordic

@bzbarsky-apple bzbarsky-apple Awaiting requested review from bzbarsky-apple

@dhrishi dhrishi Awaiting requested review from dhrishi

@electrocucaracha electrocucaracha Awaiting requested review from electrocucaracha

@emargolis emargolis Awaiting requested review from emargolis

@franck-apple franck-apple Awaiting requested review from franck-apple

@gjc13 gjc13 Awaiting requested review from gjc13

@hawk248 hawk248 Awaiting requested review from hawk248

@holbrookt holbrookt Awaiting requested review from holbrookt

@harsha-rajendran harsha-rajendran Awaiting requested review from harsha-rajendran

@isiu-apple isiu-apple Awaiting requested review from isiu-apple

@jelderton jelderton Awaiting requested review from jelderton

@jepenven-silabs jepenven-silabs Awaiting requested review from jepenven-silabs

@jmartinez-silabs jmartinez-silabs Awaiting requested review from jmartinez-silabs

@lazarkov lazarkov Awaiting requested review from lazarkov

@LuDuda LuDuda Awaiting requested review from LuDuda

@lzgrablic02 lzgrablic02 Awaiting requested review from lzgrablic02

@mlepage-google mlepage-google Awaiting requested review from mlepage-google

@mrjerryjohns mrjerryjohns Awaiting requested review from mrjerryjohns

@msandstedt msandstedt Awaiting requested review from msandstedt

@sagar-apple sagar-apple Awaiting requested review from sagar-apple

@saurabhst saurabhst Awaiting requested review from saurabhst

@selissia selissia Awaiting requested review from selissia

@tcarmelveilleux tcarmelveilleux Awaiting requested review from tcarmelveilleux

@tecimovic tecimovic Awaiting requested review from tecimovic

@vijs vijs Awaiting requested review from vijs

@vivien-apple vivien-apple Awaiting requested review from vivien-apple

@wbschiller wbschiller Awaiting requested review from wbschiller

@woody-apple woody-apple Awaiting requested review from woody-apple

@xylophone21 xylophone21 Awaiting requested review from xylophone21

@yunhanw-google yunhanw-google Awaiting requested review from yunhanw-google

Assignees
No one assigned
Labels
app controller review - approved
Projects
None yet
Milestone
No milestone
3 participants
@erjiaqing @bzbarsky-apple @andy31415