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

Stop passing individual field arguments to server cluster command callbacks #10354

Merged

Conversation

bzbarsky-apple
Copy link
Contributor

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

REVIEW NOTE: Reviewing the individual changesets may me easier than reviewing the whole thing squashed together: there are some mass-change changesets in there that are search-and-replace, and some changesets that have manual changes that need careful review.

Problem

We are passing both the field structs and individual fields.

Change overview

Just pass the structs.

This requires some manual fixups for types that were mis-handled by the individual fields (lists and CHAR_STRING) to get the code to compile.

Fixes #8974
Fixes #8977

Testing

No intended observable behavior changes so far and passes our existing tests.

@woody-apple
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple force-pushed the stop-passing-individual-fields branch from 386e6c5 to 342381b Compare October 11, 2021 22:35
Copy link
Contributor

@jmeg-sfy jmeg-sfy left a comment

Choose a reason for hiding this comment

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

Good job !

Question how command callbacks with optional parameters will be handle ?
how this work to test the presence of an arg in CommandData ?

Thanks to answer if you see my message

@erjiaqing
Copy link
Contributor

Good job !

Question how command callbacks with optional parameters will be handle ? how this work to test the presence of an arg in CommandData ?

Thanks to answer if you see my message

@mrjerryjohns

I remembered that optional parameters will be represented by chip::Optional. Anyway, this will be the work for ClusterObjects instead of command handlers (of course, the handler should have their own logic for optional fields) AFAIK.

@bzbarsky-apple bzbarsky-apple force-pushed the stop-passing-individual-fields branch from bd9df30 to c2f16b5 Compare October 12, 2021 14:12
@bzbarsky-apple
Copy link
Contributor Author

Question how command callbacks with optional parameters will be handle ?
how this work to test the presence of an arg in CommandData ?

@jmeg-sfy There are fundamentally two options here, on the decoding side:

  1. Just set the missing optional parameters to their spec-defined default values when decoding.
  2. Represent optional parameters of type T as Optional<T> and have cluster logic explicitly deal with the case when they are missing.

I don't think we're 100% decided yet on which is better. Any thoughts would be much appreciated.

On the encoding side I think we want to use Optional<T> so we don't take up bytes on the wire sending things that don't need to be sent.

@github-actions
Copy link

Size increase report for "gn_qpg-example-build" from 9cfc21b

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

Files found only in the build output:
    report.csv

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'

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

sections,vmsize,filesize
.debug_loc,0,3036
[Unmapped],0,1336
.debug_ranges,0,32
.debug_line,0,20
.symtab,0,16
.shstrtab,0,-2
.debug_aranges,0,-16
.debug_abbrev,0,-171
.debug_frame,0,-192
.debug_str,0,-264
.strtab,0,-342
.debug_info,0,-377
.text,-1336,-1336


@github-actions
Copy link

Size increase report for "esp32-example-build" from 9cfc21b

File Section File VM
chip-all-clusters-app.elf .flash.text -2108 -2108
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_str,0,708
.debug_ranges,0,248
.debug_line,0,128
.debug_abbrev,0,92
.shstrtab,0,-2
.debug_aranges,0,-16
.debug_frame,0,-92
.strtab,0,-354
.debug_info,0,-1480
[Unmapped],0,-1988
.flash.text,-2108,-2108
.debug_loc,0,-6596


@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 9cfc21b

File Section File VM
chip-lock.elf device_handles 8 8
chip-lock.elf text -600 -600
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_loc,0,1967
.debug_info,0,1209
.symtab,0,32
.debug_ranges,0,8
device_handles,8,8
.shstrtab,0,1
.debug_aranges,0,-16
.debug_line,0,-38
.debug_frame,0,-148
.debug_str,0,-151
.debug_abbrev,0,-175
.strtab,0,-229
text,-600,-600


@bzbarsky-apple bzbarsky-apple merged commit 4d722f7 into project-chip:master Oct 12, 2021
@bzbarsky-apple bzbarsky-apple deleted the stop-passing-individual-fields branch October 12, 2021 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants