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

Switch server-side command decoding to using the cluster-objects infrastructure #10296

Merged
merged 2 commits into from
Oct 8, 2021
Merged

Switch server-side command decoding to using the cluster-objects infrastructure #10296

merged 2 commits into from
Oct 8, 2021

Conversation

bzbarsky-apple
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple commented Oct 6, 2021 •
edited
Loading

Problem

We are passing in structs that are not initialized yet.

Change overview

Fill in the struct members. Get the values we used to decode directly from the struct members.

Testing

Exercised by our existing tests. The only observable behavior changes are the change to expect UTF-8 strings in the payload for CHAR_STRING.

@bzbarsky-apple bzbarsky-apple force-pushed the struct-fields-codegen branch 2 times, most recently from 610e133 to 305d5ae Compare October 7, 2021 04:24
@kpschoedel
Copy link
Contributor

PR #10296: Size comparison from 305d5ae to d48d754

Increases above 1.0% from 305d5ae to d48d754:

platform target config section d48d754 305d5ae change % change
linux all-clusters-app debug .text 1273570 1316834 43264 3.4
tv-app debug .text 1435698 1461330 25632 1.8
bridge-app debug+rpc .text 1034565 1047173 12608 1.2
lighting-app debug+rpc .text 1219378 1243298 23920 2.0
34 builds
platform target config section d48d754 305d5ae change % change
efr32 lighting-app BRD4161A .bss 117956 117956 0 0.0
.data 1796 1796 0 0.0
.text 780200 780308 108 0.0
lock-app BRD4161A .bss 115388 115388 0 0.0
.data 1756 1756 0 0.0
.text 758452 758752 300 0.0
window-app BRD4161A .bss 117036 117036 0 0.0
.data 1760 1760 0 0.0
.text 759864 759996 132 0.0
lighting-app BRD4161A+rpc .bss 131292 131292 0 0.0
.data 1848 1848 0 0.0
.text 758956 759056 100 0.0
esp32 all-clusters-app c3devkit .dram0.bss 65344 65344 0 0.0
.dram0.data 16240 16240 0 0.0
.flash.rodata 200280 200424 144 0.1
.flash.text 873858 875622 1764 0.2
.iram0.text 57330 57330 0 0.0
m5stack .dram0.bss 67856 67856 0 0.0
.dram0.data 32092 32092 0 0.0
.flash.rodata 208836 208980 144 0.1
.flash.text 907943 906091 -1852 -0.2
.iram0.text 125115 125115 0 0.0
k32w lock-app k32w061+debug .bss 68604 68604 0 0.0
.data 1860 1860 0 0.0
.text 515252 514856 -396 -0.1
shell k32w061+debug .bss 55064 55064 0 0.0
.data 672 672 0 0.0
.text 355596 355596 0 0.0
lighting-app k32w061+se05x+release .bss 78544 78544 0 0.0
.data 1896 1896 0 0.0
.text 614596 614008 -588 -0.1
linux all-clusters-app debug .bss 58064 58064 0 0.0
.data 1010 1010 0 0.0
.data.rel.ro 58368 58368 0 0.0
.dynamic 592 592 0 0.0
.got 4096 4096 0 0.0
.init 27 27 0 0.0
.init_array 512 512 0 0.0
.rodata 134389 134805 416 0.3
.text 1273570 1316834 43264 3.4
chip-tool debug .bss 18960 18960 0 0.0
.data 1584 1584 0 0.0
.data.rel.ro 77168 77168 0 0.0
.dynamic 592 592 0 0.0
.got 4344 4344 0 0.0
.init 27 27 0 0.0
.init_array 408 408 0 0.0
.rodata 173304 173304 0 0.0
.text 2409621 2411349 1728 0.1
ota-provider-app debug .bss 37376 37376 0 0.0
.data 752 752 0 0.0
.data.rel.ro 22808 22808 0 0.0
.dynamic 592 592 0 0.0
.got 3968 3968 0 0.0
.init 27 27 0 0.0
.init_array 424 424 0 0.0
.rodata 106640 107056 416 0.4
.text 972066 974978 2912 0.3
ota-requestor-app debug .bss 204664 204664 0 0.0
.data 704 704 0 0.0
.data.rel.ro 23520 23520 0 0.0
.dynamic 592 592 0 0.0
.got 4080 4080 0 0.0
.init 27 27 0 0.0
.init_array 480 480 0 0.0
.rodata 113374 113374 0 0.0
.text 1037010 1037330 320 0.0
shell debug .bss 16104 16104 0 0.0
.data 242 242 0 0.0
.data.rel.ro 35192 35192 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 70255 70255 0 0.0
.text 571010 571010 0 0.0
tv-app debug .bss 218256 218256 0 0.0
.data 2032 2032 0 0.0
.data.rel.ro 55408 55408 0 0.0
.dynamic 592 592 0 0.0
.got 4416 4416 0 0.0
.init 27 27 0 0.0
.init_array 608 608 0 0.0
.rodata 149392 149936 544 0.4
.text 1435698 1461330 25632 1.8
bridge-app debug+rpc .bss 54416 54416 0 0.0
.data 976 976 0 0.0
.data.rel.ro 25480 25480 0 0.0
.dynamic 592 592 0 0.0
.got 3952 3952 0 0.0
.init 27 27 0 0.0
.init_array 400 400 0 0.0
.rodata 108120 108488 368 0.3
.text 1034565 1047173 12608 1.2
lighting-app debug+rpc .bss 42936 42936 0 0.0
.data 1106 1106 0 0.0
.data.rel.ro 51840 51840 0 0.0
.dynamic 608 608 0 0.0
.got 4128 4128 0 0.0
.init 27 27 0 0.0
.init_array 536 536 0 0.0
.rodata 123601 123985 384 0.3
.text 1219378 1243298 23920 2.0
mbed lighting-app CY8CPROTO_062_4343W+release .bss 172340 172340 0 0.0
.data 5368 5368 0 0.0
.heap 858736 858736 0 0.0
.text 1157224 1156656 -568 -0.0
lock-app CY8CPROTO_062_4343W+release .bss 170844 170844 0 0.0
.data 5344 5344 0 0.0
.heap 860256 860256 0 0.0
.text 1134048 1133600 -448 -0.0
nrfconnect lighting-app nrf52840dk_nrf52840 bss 112308 112308 0 0.0
rodata 96656 96540 -116 -0.1
text 579968 579200 -768 -0.1
lock-app nrf52840dk_nrf52840 bss 110908 110908 0 0.0
rodata 93040 92920 -120 -0.1
text 560508 560076 -432 -0.1
pigweed-app nrf52840dk_nrf52840 bss 51768 51768 0 0.0
rodata 45744 45744 0 0.0
text 338904 338904 0 0.0
pump-app nrf52840dk_nrf52840 bss 111332 111332 0 0.0
rodata 94152 94032 -120 -0.1
text 563900 563344 -556 -0.1
pump-controller-app nrf52840dk_nrf52840 bss 110832 110832 0 0.0
rodata 92860 92744 -116 -0.1
text 559284 558784 -500 -0.1
shell nrf52840dk_nrf52840 bss 107316 107316 0 0.0
rodata 71152 71152 0 0.0
text 518688 518688 0 0.0
lighting-app nrf52840dk_nrf52840+rpc bss 108544 108544 0 0.0
rodata 87400 87284 -116 -0.1
text 552604 551836 -768 -0.1
nrf5340dk_nrf5340_cpuapp bss 113684 113684 0 0.0
rodata 91900 91780 -120 -0.1
text 509476 508712 -764 -0.1
lock-app nrf5340dk_nrf5340_cpuapp bss 112284 112284 0 0.0
rodata 88296 88176 -120 -0.1
text 490016 489584 -432 -0.1
shell nrf5340dk_nrf5340_cpuapp bss 108300 108300 0 0.0
rodata 65796 65796 0 0.0
text 439320 439320 0 0.0
p6 lock-app default .bss 67784 67784 0 0.0
.data 2416 2416 0 0.0
.heap 963144 963144 0 0.0
.text 1125408 1125696 288 0.0
qpg lighting-app qpg6100+debug .bss 53544 53544 0 0.0
.data 1000 1000 0 0.0
.text 486748 486168 -580 -0.1
lock-app qpg6100+debug .bss 52064 52064 0 0.0
.data 956 956 0 0.0
.text 462092 461688 -404 -0.1
persistent-storage-app qpg6100+debug .bss 17802 17802 0 0.0
.data 284 284 0 0.0
.text 102712 102712 0 0.0
telink lighting-app tlsr9518adk80d bss 71080 71080 0 0.0
noinit 33216 33216 0 0.0
text 459562 460198 636 0.1

@bzbarsky-apple
Copy link
Contributor Author

@yunhanw-google @vivien-apple if one of you could look this over, I would really appreciated it, just to make sure I am not missing something in the codegen bits.

@bzbarsky-apple bzbarsky-apple linked an issue Oct 7, 2021 that may be closed by this pull request
S1: Update IMClusterCommandHandler.cpp to use generated cluster command objects #8971
Closed
@bzbarsky-apple
Switch command decoding to using the cluster-objects infrastructure.
aea5b03 
For now we are still passing the existing arguments to cluster
callbacks, so we don't need to update cluster implementations as part
of this change.

The change to how we encode CHAR_STRING command arguments is needed
because the new setup expects to decode them as TLV UTF-8 strings, not
TLV octet strings.
@bzbarsky-apple
Regenerate generated files
7f187c0
@github-actions
Copy link

github-actions bot commented Oct 7, 2021

Size increase report for "gn_qpg-example-build" from 8dc009b

File Section File VM
chip-qpg6100-lighting-example.out .text -596 -596
Full report output 
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_info,0,647293
.debug_str,0,303452
.debug_loc,0,298290
.debug_line,0,180444
.debug_ranges,0,69832
.debug_frame,0,25904
.strtab,0,7060
.debug_aranges,0,5464
.debug_abbrev,0,2865
.symtab,0,1904
[Unmapped],0,596
.text,-596,-596

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

@github-actions
Copy link

github-actions bot commented Oct 7, 2021

Size increase report for "esp32-example-build" from 8dc009b

File Section File VM
chip-all-clusters-app.elf .flash.text 1744 1744
chip-all-clusters-app.elf .flash.rodata 144 144
Full report output 
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,513144
.debug_str,0,294274
.debug_line,0,235778
.debug_loc,0,226055
.debug_ranges,0,69992
.debug_frame,0,33452
.strtab,0,12045
.debug_aranges,0,5456
.debug_abbrev,0,3185
.symtab,0,1936
.flash.text,1744,1744
.flash.rodata,144,144
.shstrtab,0,-1
[Unmapped],0,-1888

@github-actions
Copy link

github-actions bot commented Oct 7, 2021

Size increase report for "nrfconnect-example-build" from 8dc009b

File Section File VM
chip-lock.elf rodata -120 -120
chip-lock.elf text -432 -432
Full report output 
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,652750
.debug_loc,0,305729
.debug_str,0,304867
.debug_line,0,188860
.debug_ranges,0,72144
.debug_frame,0,25856
.debug_aranges,0,5464
.strtab,0,3402
.debug_abbrev,0,2890
.symtab,0,848
.shstrtab,0,-2
rodata,-120,-120
text,-432,-432

@bzbarsky-apple bzbarsky-apple merged commit 1598c2e into project-chip:master Oct 8, 2021
@bzbarsky-apple bzbarsky-apple deleted the struct-fields-codegen branch October 8, 2021 05:02
erjiaqing added a commit to erjiaqing/connectedhomeip that referenced this pull request Oct 8, 2021
@erjiaqing
[hotfix] Re-run codegen for project-chip#10296
9542309
yunhanw-google pushed a commit that referenced this pull request Oct 8, 2021
@erjiaqing
[hotfix] Re-run codegen for #10296 (#10343)
4617f82
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this pull request Oct 8, 2021
@bzbarsky-apple
Fix build failure caused by merge issue.
a6fe63f 
Merge problem from combination of
project-chip#10296 and
project-chip#10299
@bzbarsky-apple bzbarsky-apple mentioned this pull request Oct 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saurabhst saurabhst saurabhst approved these changes

@yunhanw-google yunhanw-google yunhanw-google approved these changes

@woody-apple woody-apple woody-apple approved these changes

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

@andy31415 andy31415 Awaiting requested review from andy31415

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

@austinh0 austinh0 Awaiting requested review from austinh0

@balducci-apple balducci-apple Awaiting requested review from balducci-apple

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

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

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

@emargolis emargolis Awaiting requested review from emargolis

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

@hawk248 hawk248 Awaiting requested review from hawk248

@holbrookt holbrookt Awaiting requested review from holbrookt

@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

@kghost kghost Awaiting requested review from kghost

@LuDuda LuDuda Awaiting requested review from LuDuda

@mrjerryjohns mrjerryjohns Awaiting requested review from mrjerryjohns

@msandstedt msandstedt Awaiting requested review from msandstedt

@pan-apple pan-apple Awaiting requested review from pan-apple

@robszewczyk robszewczyk Awaiting requested review from robszewczyk

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

@shana-apple shana-apple Awaiting requested review from shana-apple

@tcarmelveilleux tcarmelveilleux Awaiting requested review from tcarmelveilleux

@yufengwangca yufengwangca Awaiting requested review from yufengwangca

Assignees
No one assigned
Labels
app examples review - approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

S1: Update IMClusterCommandHandler.cpp to use generated cluster command objects
5 participants
@bzbarsky-apple @kpschoedel @saurabhst @yunhanw-google @woody-apple