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 cluster command APIs to make it easier to fix list/struct support in commands #10168

Merged

Conversation

bzbarsky-apple
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple commented Oct 2, 2021

Review Note: Reviewing the commits individually is probably much nicer than reviewing the whole thing at once.

Problem

There are a few things this is laying the groundwork for:

  1. Simpler/saner encoding of responses to commands, using the ConcreteCommandPath this PR is adding and the command field structs that still need to be generated.
  2. Moving toward not needing to change cluster command callback signatures if something changes about the command fields, by encapsulating the fields in the "fields" struct.

Change overview

  1. Introduce a ConcreteCommandPath struct that represents what it's called. This will likely replace CommandPathParams in a bunch of places later on.
  2. Modify coden templates to work with the new function signatures.
  3. Modify cluster implementations to have the new function signatures.

Testing

No intended behavior changes. Certification tests still pass locally.

@pullapprove pullapprove bot requested a review from yunhanw-google October 2, 2021 06:18
@bzbarsky-apple bzbarsky-apple force-pushed the cluster-api-changes branch 3 times, most recently from e694578 to 2d95888 Compare October 3, 2021 05:19
@bzbarsky-apple
Copy link
Contributor Author

@bzbarsky-apple
Copy link
Contributor Author

Specific changes:

1) Codegen skeleton (empty so far) structs that represent command fields.
2) Modify codegen to pass the command path and the field struct to a
   command callback and change the order of the endpoint and
   CommandSender/CommandHandler arguments.
3) Modify codegen implementation of callbacks to have the new signature.

./scripts/tools/zap/generate.py -t src/app/common/templates/templates.json -o zzz_generated/app-common/app-common/zap-generated src/controller/data_model/controller-clusters.zap && ./scripts/tools/zap/generate.py -o zzz_generated/all-clusters-app/zap-generated examples/all-clusters-app/all-clusters-common/all-clusters-app.zap && ./gn_build.sh
This commit was generated by running:

  find src/app/clusters examples/tv-app/linux/include/content-launcher examples/window-app/common/src -name "*.cpp" | xargs perl -pi -e 'BEGIN { $/ = undef; } s/bool emberAf([a-zA-Z0-9]*)Cluster([a-zA-Z0-9]*)Callback\((?:chip::)?EndpointId (aEndpoint|endpoint|aEndpointId),\s*(?:chip::)?((?:app::)?CommandHandler \*(?: commandObj| command| apCommandObj|))([^)]*)\)/bool emberAf\1Cluster\2Callback(\4, const app::ConcreteCommandPath & commandPath, EndpointId \3\5, Commands::\2::DecodableType & commandData)/g;'

and then restyling the modified files.
Mostly:

* Include headers needed for ConcreteCommandPath and the command field structs.
* Make sure the namespaces are done right (add using declarations or
  namespace prefixes as needed).
* Resolve some name ambiguities due to name collisions between
  cluster-objects.h and af-structs.h.  We need to find a better solution for
  this.
@github-actions
Copy link

github-actions bot commented Oct 5, 2021

Size increase report for "gn_qpg-example-build" from db999f7

File Section File VM
chip-qpg6100-lighting-example.out .text 672 672
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,94893
.debug_str,0,6919
.strtab,0,6333
.debug_line,0,3751
.debug_loc,0,1681
.text,672,672
.debug_abbrev,0,532
.debug_frame,0,88
.symtab,0,16
.shstrtab,0,3
.debug_ranges,0,-152
[Unmapped],0,-672

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 5, 2021

Size increase report for "esp32-example-build" from db999f7

File Section File VM
chip-all-clusters-app.elf .flash.text 744 744
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,154259
.debug_str,0,12994
.strtab,0,11578
.debug_line,0,7862
.debug_loc,0,5879
.debug_abbrev,0,990
.flash.text,744,744
.debug_ranges,0,712
.shstrtab,0,-2
.debug_frame,0,-44
[Unmapped],0,-744


@github-actions
Copy link

github-actions bot commented Oct 5, 2021

Size increase report for "nrfconnect-example-build" from db999f7

File Section File VM
chip-lock.elf text 368 368
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,82515
.debug_str,0,4168
.strtab,0,3581
.debug_line,0,2719
.debug_abbrev,0,618
text,368,368
.debug_loc,0,184
.debug_frame,0,92
.symtab,0,16
.shstrtab,0,3
.debug_ranges,0,-88


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants