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

Add IM status response support and replace status report with status response for IM read/subscribe #9706

Merged

Conversation

yunhanw-google
Copy link
Contributor

@yunhanw-google yunhanw-google commented Sep 14, 2021

Problem

Interaction Model spec has introduced status response message and used it
for IM Read/Subscribe. Currently IM read/subscribe code still use status
report from secure channel, we need to update it with status response.
https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/master/src/data_model/Encoding-Specification.adoc#encoding-StatusResponseMessage

#9700

Change overview

-- Add status response message builder and parser and test
-- Replace status report with status response message for IM
read/subscribe

Testing

How was this tested? (at least one bullet point required)
-- Add Status response unit test
-- update in IM read/subscribe has been covered by existing test.

@yunhanw-google yunhanw-google changed the title Add IM status response support Add IM status response support and replace status report with status response for IM read/subscribe Sep 14, 2021
@yunhanw-google yunhanw-google force-pushed the feature/add_status_response branch from 8f3bc0e to 83607f4 Compare September 14, 2021 20:55
@yunhanw-google yunhanw-google force-pushed the feature/add_status_response branch from 83607f4 to 3a96af1 Compare September 15, 2021 04:52
@erjiaqing erjiaqing self-requested a review September 15, 2021 04:52
@erjiaqing erjiaqing dismissed their stale review September 15, 2021 04:53

code updated

@yunhanw-google yunhanw-google force-pushed the feature/add_status_response branch from 3a96af1 to e135688 Compare September 15, 2021 14:58
@yunhanw-google yunhanw-google force-pushed the feature/add_status_response branch 2 times, most recently from 3b8aefa to 6c8ebab Compare September 15, 2021 15:16
Copy link
Contributor

@msandstedt msandstedt left a comment

Choose a reason for hiding this comment

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

Two general comments: is protocolCode actually 32-bit? I thought it was 16-bit. Second, since we're already touching all of them, can we replace all of the intXX_t types with and format strings with typedefs and constexprs?

And apologies if protocol code is 32-bit. If so, can I be pointed to the part of the spec where this is defined? All of the protocol code examples imply these are 16-bit.

@msandstedt
Copy link
Contributor

Two general comments: is protocolCode actually 32-bit? I thought it was 16-bit. Second, since we're already touching all of them, can we replace all of the intXX_t types with and format strings with typedefs and constexprs?

And apologies if protocol code is 32-bit. If so, can I be pointed to the part of the spec where this is defined? All of the protocol code examples imply these are 16-bit.

Answered offline. Yes, ProtocolCode is 32bit now.

Typedefes and constexprs for all of the places we currently have uint32_t and PRIx32 would be nice, but don't need to block this PR.

Thanks.

@yunhanw-google
Copy link
Contributor Author

Two general comments: is protocolCode actually 32-bit? I thought it was 16-bit. Second, since we're already touching all of them, can we replace all of the intXX_t types with and format strings with typedefs and constexprs?
And apologies if protocol code is 32-bit. If so, can I be pointed to the part of the spec where this is defined? All of the protocol code examples imply these are 16-bit.

Answered offline. Yes, ProtocolCode is 32bit now.

Typedefes and constexprs for all of the places we currently have uint32_t and PRIx32 would be nice, but don't need to block this PR.

Thanks.
would continue to clean up status code in this issue
#9726

src/app/MessageDef/StatusResponse.cpp Outdated Show resolved Hide resolved
src/app/ReadClient.cpp Show resolved Hide resolved
src/app/CommandSender.cpp Outdated Show resolved Hide resolved
Problems:
Interaction Model spec has introduced status reponse message and used it
for IM Read/Subscribe. Currently IM read/subscribe code still use status
report from secure channel, we need to update it with status reponse.

Summary of Changes:
-- Add status reponse message builder and parser and test
-- Replace status report with status response message for IM
read/subscribe
@github-actions
Copy link

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

File Section File VM
chip-qpg6100-lighting-example.out .text 320 320
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,19472
.debug_line,0,3381
.debug_abbrev,0,2601
.debug_str,0,606
.debug_loc,0,560
.strtab,0,333
.text,320,320
.symtab,0,176
.debug_frame,0,156
.debug_aranges,0,64
.shstrtab,0,-1
[Unmapped],0,-320

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

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

File Section File VM
chip-lock.elf text 244 244
chip-lock.elf rodata 40 40
chip-lock.elf device_handles -4 -4
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,21016
.debug_line,0,2955
.debug_abbrev,0,2753
.debug_str,0,567
.debug_loc,0,553
.strtab,0,333
text,244,244
.symtab,0,176
.debug_frame,0,156
.debug_aranges,0,64
rodata,40,40
.shstrtab,0,-1
device_handles,-4,-4

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

sections,vmsize,filesize


@github-actions
Copy link

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

File Section File VM
chip-all-clusters-app.elf .flash.text 284 284
chip-all-clusters-app.elf .flash.rodata 80 80
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,52792
.debug_line,0,4347
[Unmapped],0,3732
.debug_abbrev,0,3584
.debug_loc,0,896
.debug_str,0,613
.strtab,0,333
.flash.text,284,284
.debug_frame,0,168
.flash.rodata,80,80
.symtab,0,80
.debug_aranges,0,64
.debug_ranges,0,48
.shstrtab,0,-1


@yufengwangca yufengwangca merged commit 1516dfd into project-chip:master Sep 16, 2021
mleisner pushed a commit to mleisner/connectedhomeip that referenced this pull request Sep 20, 2021
Problems:
Interaction Model spec has introduced status reponse message and used it
for IM Read/Subscribe. Currently IM read/subscribe code still use status
report from secure channel, we need to update it with status reponse.

Summary of Changes:
-- Add status reponse message builder and parser and test
-- Replace status report with status response message for IM
read/subscribe
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.

7 participants