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

[chip-tool] Add Optional endpoint id for the test command #11740

Merged
merged 2 commits into from
Nov 12, 2021
Merged

[chip-tool] Add Optional endpoint id for the test command #11740

merged 2 commits into from
Nov 12, 2021

Conversation

vivien-apple
Copy link
Contributor

Problem

It is useful to override the endpoint id uses by chip-tool for the test command

Change overview

  • Add an optional endpoint id

@vivien-apple vivien-apple self-assigned this Nov 12, 2021
@woody-apple
Copy link
Contributor

Fast tracking, given this is adding functionality to a test tool.

@restyled-io restyled-io bot mentioned this pull request Nov 12, 2021
Closed
@vivien-apple vivien-apple force-pushed the CI_ChipToolAddAWayToOverrideEndpointId branch from 5f32d8d to 55db910 Compare November 12, 2021 17:44
bzbarsky-apple
bzbarsky-apple reviewed Nov 12, 2021
View reviewed changes
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple 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.

I don't understand how this can work given the code as it landed in PR #11729. That code will error out (with a very confusing message) if an optional argument is actually passed, and optional arguments are not initialized in that code [Edit: this claim is wrong].

I feel like the approach in #11642 at least answers those questions, and that answer is that you use named optional args and make the names look like something that looks like a command-line-arg naming convention, so in this case "--endpoint-id" or so.

Seems like we should fix that, and then this approach, with Optional, seems fine.

@github-actions
Copy link

github-actions bot commented Nov 12, 2021 •
edited
Loading

PR #11740: Size comparison from 5d4bade to 55db910

Increases above 0.2%:

platform target config section 5d4bade 55db910 change % change
linux chip-tool debug (read only) 4613309 4661421 48112 1.0
.text 4051605 4099717 48112 1.2
Increases (1 build for linux)
platform target config section 5d4bade 55db910 change % change
linux chip-tool debug (read only) 4613309 4661421 48112 1.0
.text 4051605 4099717 48112 1.2
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
platform target config section 5d4bade 55db910 change % change
efr32 lighting-app BRD4161A (read only) 748640 748640 0 0.0
(read/write) 115940 115940 0 0.0
.bss 114140 114140 0 0.0
.data 1800 1800 0 0.0
.text 748632 748632 0 0.0
BRD4161A+rpc (read only) 736188 736188 0 0.0
(read/write) 132568 132568 0 0.0
.bss 130644 130644 0 0.0
.data 1924 1924 0 0.0
.text 736180 736180 0 0.0
lock-app BRD4161A (read only) 725480 725480 0 0.0
(read/write) 113724 113724 0 0.0
.bss 111964 111964 0 0.0
.data 1756 1756 0 0.0
.text 725472 725472 0 0.0
window-app BRD4161A (read only) 726376 726376 0 0.0
(read/write) 114044 114044 0 0.0
.bss 112284 112284 0 0.0
.data 1760 1760 0 0.0
.text 726368 726368 0 0.0
esp32 all-clusters-app c3devkit (read only) 823478 823478 0 0.0
(read/write) 1222346 1222346 0 0.0
.dram0.bss 56016 56016 0 0.0
.dram0.data 14092 14092 0 0.0
.flash.rodata 166992 166992 0 0.0
.flash.text 823478 823478 0 0.0
.iram0.text 61394 61394 0 0.0
m5stack (read only) 894551 894551 0 0.0
(read/write) 420520 420520 0 0.0
.dram0.bss 61104 61104 0 0.0
.dram0.data 34056 34056 0 0.0
.flash.rodata 194036 194036 0 0.0
.flash.text 889167 889167 0 0.0
.iram0.text 122987 122987 0 0.0
k32w lighting-app k32w061+se05x+release (read/write) 701568 701568 0 0.0
.bss 77508 77508 0 0.0
.data 1908 1908 0 0.0
.text 616352 616352 0 0.0
lock-app k32w061+debug (read/write) 592784 592784 0 0.0
.bss 68028 68028 0 0.0
.data 1876 1876 0 0.0
.text 517080 517080 0 0.0
shell k32w061+debug (read/write) 658468 658468 0 0.0
.bss 78820 78820 0 0.0
.data 1844 1844 0 0.0
.text 572004 572004 0 0.0
linux all-clusters-app debug (read only) 1712537 1712537 0 0.0
(read/write) 126336 126336 0 0.0
.bss 57680 57680 0 0.0
.data 1042 1042 0 0.0
.data.rel.ro 62336 62336 0 0.0
.dynamic 592 592 0 0.0
.got 4112 4112 0 0.0
.init 27 27 0 0.0
.init_array 552 552 0 0.0
.rodata 140053 140053 0 0.0
.text 1438434 1438434 0 0.0
bridge-app debug+rpc (read only) 1306573 1306573 0 0.0
(read/write) 77456 77456 0 0.0
.bss 42896 42896 0 0.0
.data 1568 1568 0 0.0
.data.rel.ro 27992 27992 0 0.0
.dynamic 592 592 0 0.0
.got 3984 3984 0 0.0
.init 27 27 0 0.0
.init_array 408 408 0 0.0
.rodata 111668 111668 0 0.0
.text 1097909 1097909 0 0.0
chip-tool debug (read only) 4613309 4661421 48112 1.0
(read/write) 164008 164008 0 0.0
.bss 41192 41192 0 0.0
.data 2272 2272 0 0.0
.data.rel.ro 115040 115040 0 0.0
.dynamic 592 592 0 0.0
.got 4416 4416 0 0.0
.init 27 27 0 0.0
.init_array 472 472 0 0.0
.rodata 259594 259594 0 0.0
.text 4051605 4099717 48112 1.2
lighting-app debug+rpc (read only) 1573345 1573345 0 0.0
(read/write) 110224 110224 0 0.0
.bss 48080 48080 0 0.0
.data 1234 1234 0 0.0
.data.rel.ro 55584 55584 0 0.0
.dynamic 608 608 0 0.0
.got 4136 4136 0 0.0
.init 27 27 0 0.0
.init_array 536 536 0 0.0
.rodata 129681 129681 0 0.0
.text 1308946 1308946 0 0.0
ota-provider-app debug (read only) 1261809 1261809 0 0.0
(read/write) 75208 75208 0 0.0
.bss 44512 44512 0 0.0
.data 784 784 0 0.0
.data.rel.ro 24808 24808 0 0.0
.dynamic 592 592 0 0.0
.got 4016 4016 0 0.0
.init 27 27 0 0.0
.init_array 448 448 0 0.0
.rodata 113127 113127 0 0.0
.text 1051810 1051810 0 0.0
ota-requestor-app debug (read only) 1346625 1346625 0 0.0
(read/write) 78976 78976 0 0.0
.bss 46976 46976 0 0.0
.data 848 848 0 0.0
.data.rel.ro 26056 26056 0 0.0
.dynamic 592 592 0 0.0
.got 3992 3992 0 0.0
.init 27 27 0 0.0
.init_array 472 472 0 0.0
.rodata 123984 123984 0 0.0
.text 1123218 1123218 0 0.0
shell debug (read only) 788969 788969 0 0.0
(read/write) 65064 65064 0 0.0
.bss 23400 23400 0 0.0
.data 242 242 0 0.0
.data.rel.ro 36928 36928 0 0.0
.dynamic 592 592 0 0.0
.got 3528 3528 0 0.0
.init 27 27 0 0.0
.init_array 344 344 0 0.0
.rodata 77903 77903 0 0.0
.text 609170 609170 0 0.0
tv-app debug (read only) 1851361 1851361 0 0.0
(read/write) 407928 407928 0 0.0
.bss 339752 339752 0 0.0
.data 2768 2768 0 0.0
.data.rel.ro 59728 59728 0 0.0
.dynamic 592 592 0 0.0
.got 4432 4432 0 0.0
.init 27 27 0 0.0
.init_array 616 616 0 0.0
.rodata 156749 156749 0 0.0
.text 1549506 1549506 0 0.0
mbed all-clusters-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2290920 2290920 0 0.0
.bss 179612 179612 0 0.0
.data 5216 5216 0 0.0
.heap 851616 851616 0 0.0
.text 1253520 1253520 0 0.0
lighting-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2273728 2273728 0 0.0
.bss 172556 172556 0 0.0
.data 5576 5576 0 0.0
.heap 858312 858312 0 0.0
.text 1236328 1236328 0 0.0
lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2249424 2249424 0 0.0
.bss 171436 171436 0 0.0
.data 5568 5568 0 0.0
.heap 859440 859440 0 0.0
.text 1212024 1212024 0 0.0
pigweed-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 1139744 1139744 0 0.0
.bss 11752 11752 0 0.0
.data 4368 4368 0 0.0
.heap 1020328 1020328 0 0.0
.text 103128 103128 0 0.0
shell CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2048376 2048376 0 0.0
.bss 155912 155912 0 0.0
.data 4968 4968 0 0.0
.heap 875568 875568 0 0.0
.text 1010976 1010976 0 0.0
nrfconnect lighting-app nrf52840dk_nrf52840 (read/write) 864075 864075 0 0.0
bss 110964 110964 0 0.0
rodata 97100 97100 0 0.0
text 580388 580388 0 0.0
nrf52840dk_nrf52840+rpc (read/write) 826451 826451 0 0.0
bss 107316 107316 0 0.0
rodata 88292 88292 0 0.0
text 554560 554560 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 789118 789118 0 0.0
bss 112336 112336 0 0.0
rodata 92360 92360 0 0.0
text 509856 509856 0 0.0
lock-app nrf52840dk_nrf52840 (read/write) 839311 839311 0 0.0
bss 109988 109988 0 0.0
rodata 93168 93168 0 0.0
text 560692 560692 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 764606 764606 0 0.0
bss 111400 111400 0 0.0
rodata 88472 88472 0 0.0
text 490252 490252 0 0.0
pigweed-app nrf52840dk_nrf52840 (read/write) 497327 497327 0 0.0
bss 51824 51824 0 0.0
rodata 45780 45780 0 0.0
text 339436 339436 0 0.0
pump-app nrf52840dk_nrf52840 (read/write) 845387 845387 0 0.0
bss 110128 110128 0 0.0
rodata 94876 94876 0 0.0
text 564852 564852 0 0.0
pump-controller-app nrf52840dk_nrf52840 (read/write) 839167 839167 0 0.0
bss 110024 110024 0 0.0
rodata 93168 93168 0 0.0
text 560428 560428 0 0.0
shell nrf52840dk_nrf52840 (read/write) 775483 775483 0 0.0
bss 108736 108736 0 0.0
rodata 72160 72160 0 0.0
text 520008 520008 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 690538 690538 0 0.0
bss 109720 109720 0 0.0
rodata 66804 66804 0 0.0
text 440616 440616 0 0.0
p6 all-clusters-app default (read/write) 2299408 2299408 0 0.0
.bss 112608 112608 0 0.0
.data 2520 2520 0 0.0
.heap 918216 918216 0 0.0
.text 1257672 1257672 0 0.0
lock-app default (read/write) 2213448 2213448 0 0.0
.bss 101296 101296 0 0.0
.data 2400 2400 0 0.0
.heap 929648 929648 0 0.0
.text 1171712 1171712 0 0.0
qpg lighting-app qpg6100+debug (read only) 493180 493180 0 0.0
(read/write) 114144 114144 0 0.0
.bss 50640 50640 0 0.0
.data 1008 1008 0 0.0
.text 487860 487860 0 0.0
lock-app qpg6100+debug (read only) 467924 467924 0 0.0
(read/write) 114140 114140 0 0.0
.bss 49576 49576 0 0.0
.data 964 964 0 0.0
.text 462604 462604 0 0.0
persistent-storage-app qpg6100+debug (read only) 105408 105408 0 0.0
(read/write) 114142 114142 0 0.0
.bss 8978 8978 0 0.0
.data 272 272 0 0.0
.text 100088 100088 0 0.0
telink lighting-app tlsr9518adk80d (read/write) 666414 666414 0 0.0
bss 69320 69320 0 0.0
noinit 33216 33216 0 0.0
text 460906 460906 0 0.0

@vivien-apple
Copy link
Contributor Author

I don't understand how this can work given the code as it landed in PR #11729. That code will error out (with a very confusing message) if an optional argument is actually passed, and optional arguments are not initialized in that code.

I feel like the approach in #11642 at least answers those questions, and that answer is that you use named optional args and make the names look like something that looks like a command-line-arg naming convention, so in this case "--endpoint-id" or so.

Seems like we should fix that, and then this approach, with Optional, seems fine.

The code just assume that if you have arguments A, B, C where B and C are optionals, that if you want to use C you also need to provide B.

I agree, that adding a way to specify the argument is good, and I spoke with @krypton36 about adding support for it already as a way to make #11729 better.

@bzbarsky-apple
Copy link
Contributor

The code just assume that if you have arguments A, B, C where B and C are optionals, that if you want to use C you also need to provide B.

Right, but that's a pretty weird thing to require.

In any case, I see I was misreading it and we do initialize the optionals, so I guess this is ok as-is for now.

@andy31415 andy31415 merged commit 2aae643 into project-chip:master Nov 12, 2021
@vivien-apple vivien-apple mentioned this pull request Nov 12, 2021
Closed
@mikaelhm mikaelhm mentioned this pull request Nov 12, 2021
Closed
PSONALl pushed a commit to PSONALl/connectedhomeip that referenced this pull request Dec 3, 2021
@vivien-apple
[chip-tool] Add Optional endpoint id for the test command (project-ch…
0f543f5 
…ip#11740)

* [chip-tool] Add optional endpoint id for the test command

* Update generated content
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bzbarsky-apple bzbarsky-apple bzbarsky-apple approved these changes

@andy31415 andy31415 andy31415 approved these changes

Assignees

@vivien-apple vivien-apple

Labels
examples fast track
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vivien-apple @woody-apple @bzbarsky-apple @andy31415