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

Fix #13377 for compatibility with #13330 #13382

Merged
merged 1 commit into from
Jan 7, 2022
Merged

Fix #13377 for compatibility with #13330 #13382

merged 1 commit into from
Jan 7, 2022
+3 −3

Conversation

msandstedt
Copy link
Contributor

Problem

The pull requests #13330 and #13377 were merged around the same time,
and have caused a small build breakage.

Change overview

Amend #13377 to work with #13330.

Testing

Tested to build without error.

@msandstedt
Fix project-chip#13377 for compatibility with project-chip#13330
8f3933a 
The pull requests project-chip#13330 and project-chip#13377 were merged around the same time,
and have caused a small build breakage.  This fixes it.
@msandstedt msandstedt added the hotfix urgent fix needed, can bypass review label Jan 7, 2022
bzbarsky-apple
bzbarsky-apple approved these changes Jan 7, 2022
View reviewed changes
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

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

OK to fix build, but as a followup we should figure out whether this is in fact always a SecureSession session, right? @turon

turon
turon approved these changes Jan 7, 2022
View reviewed changes
Copy link
Contributor

@turon turon left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Was about to post the same.

@turon
Copy link
Contributor

turon commented Jan 7, 2022 •
edited
Loading

OK to fix build, but as a followup we should figure out whether this is in fact always a SecureSession session, right? @turon

Yes, I had the same question for @kghost regarding the new API. Specifically, what happens when AsSecureSession is called on an unsecured session? This use case simply needs to print info about the session that should be there regardless of session type (session ids, peer node id, is the session secure or not). We should have a simple API for that that doesn't require a number of conditionals and branches to print the common data.

@msandstedt
Copy link
Contributor Author

msandstedt commented Jan 7, 2022 •
edited
Loading

I guess I would defend this as no better or worse than the original code, which was extracting the chip::Optional GetLocalSessionId.Value without checking whether it exists first. It seems sort of like the same underlying assumptions are at play in either case.

edit: Not that this is great. I agree a pretty printer that does the right thing would be much better, and I don't claim this can't break. I'm only claiming that it's probably no more likely to break than the previous code.

@turon
Copy link
Contributor

turon commented Jan 7, 2022 •
edited
Loading

I guess I would defend this as no better or worse than the original code, which was extracting the chip::Optional GetLocalSessionId.Value without checking whether it exists first. It seems sort of like the same underlying assumptions are at play in either case.

Yes, basically the entire stack now calls AsSecureSession() when a secure session is assumed / required, which will VerifyOrDie(GetSessionType() == SessionType::kSecure);.

I'll prep a follow-up that uses a switch(session->GetSessionType()).

@github-actions
Copy link

github-actions bot commented Jan 7, 2022 •
edited
Loading

PR #13382: Size comparison from 585cdcb to 8f3933a

Increases above 0.2%:

platform target config section 585cdcb 8f3933a change % change
mbed pigweed-app CY8CPROTO_062_4343W+release .text 103096 103392 296 0.3
nrfconnect lighting-app nrf52840dk_nrf52840+rpc rodata 100572 101548 976 1.0
pigweed-app nrf52840dk_nrf52840 rodata 50104 50668 564 1.1
Increases (10 builds for efr32, linux, mbed, nrfconnect)
platform target config section 585cdcb 8f3933a change % change
efr32 lighting-app BRD4161A (read only) 828876 829196 320 0.0
.text 828868 829188 320 0.0
BRD4161A+rpc (read only) 816072 816840 768 0.1
.text 816064 816832 768 0.1
window-app BRD4161A (read only) 802324 802660 336 0.0
.text 802316 802652 336 0.0
linux chip-tool-ipv6only arm64 (read only) 7039468 7039660 192 0.0
.text 5961492 5961684 192 0.0
thermostat-no-ble arm64 (read only) 2033468 2033708 240 0.0
.text 1690800 1691040 240 0.0
mbed lighting-app CY8CPROTO_062_4343W+release (read/write) 2330392 2330688 296 0.0
.text 1292992 1293288 296 0.0
lock-app CY8CPROTO_062_4343W+release (read/write) 2303544 2303840 296 0.0
.text 1266144 1266440 296 0.0
pigweed-app CY8CPROTO_062_4343W+release (read/write) 1139712 1140008 296 0.0
.text 103096 103392 296 0.3
nrfconnect lighting-app nrf52840dk_nrf52840+rpc (read/write) 922551 923479 928 0.1
rodata 100572 101548 976 1.0
pigweed-app nrf52840dk_nrf52840 (read/write) 541835 542351 516 0.1
rodata 50104 50668 564 1.1
Decreases (2 builds for nrfconnect)
platform target config section 585cdcb 8f3933a change % change
nrfconnect lighting-app nrf52840dk_nrf52840+rpc text 628644 628604 -40 -0.0
pigweed-app nrf52840dk_nrf52840 text 376940 376892 -48 -0.0
Full report (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
platform target config section 585cdcb 8f3933a change % change
efr32 lighting-app BRD4161A (read only) 828876 829196 320 0.0
(read/write) 126996 126996 0 0.0
.bss 125120 125120 0 0.0
.data 1876 1876 0 0.0
.text 828868 829188 320 0.0
BRD4161A+rpc (read only) 816072 816840 768 0.1
(read/write) 143656 143656 0 0.0
.bss 141680 141680 0 0.0
.data 1976 1976 0 0.0
.text 816064 816832 768 0.1
window-app BRD4161A (read only) 802324 802660 336 0.0
(read/write) 125936 125936 0 0.0
.bss 124104 124104 0 0.0
.data 1832 1832 0 0.0
.text 802316 802652 336 0.0
esp32 all-clusters-app c3devkit (read only) 891834 891834 0 0.0
(read/write) 1314010 1314010 0 0.0
.dram0.bss 69464 69464 0 0.0
.dram0.data 14236 14236 0 0.0
.flash.rodata 177248 177248 0 0.0
.flash.text 891834 891834 0 0.0
.iram0.text 62254 62254 0 0.0
m5stack (read only) 951779 951779 0 0.0
(read/write) 445592 445592 0 0.0
.dram0.bss 73960 73960 0 0.0
.dram0.data 34064 34064 0 0.0
.flash.rodata 206560 206560 0 0.0
.flash.text 946395 946395 0 0.0
.iram0.text 122671 122671 0 0.0
k32w light k32w061+release (read/write) 655308 655308 0 0.0
.bss 76776 76776 0 0.0
.data 1848 1848 0 0.0
.text 570884 570884 0 0.0
lock k32w061+release (read/write) 659584 659584 0 0.0
.bss 77072 77072 0 0.0
.data 1868 1868 0 0.0
.text 574844 574844 0 0.0
linux chip-tool-ipv6only arm64 (read only) 7039468 7039660 192 0.0
(read/write) 325985 325985 0 0.0
.bss 54865 54865 0 0.0
.data 1096 1096 0 0.0
.data.rel.ro 209296 209296 0 0.0
.dynamic 560 560 0 0.0
.got 57040 57040 0 0.0
.init 24 24 0 0.0
.init_array 168 168 0 0.0
.rodata 384404 384404 0 0.0
.text 5961492 5961684 192 0.0
thermostat-no-ble arm64 (read only) 2033468 2033708 240 0.0
(read/write) 145105 145105 0 0.0
.bss 64657 64657 0 0.0
.data 880 880 0 0.0
.data.rel.ro 72624 72624 0 0.0
.dynamic 560 560 0 0.0
.got 4008 4008 0 0.0
.init 24 24 0 0.0
.init_array 296 296 0 0.0
.rodata 128988 128988 0 0.0
.text 1690800 1691040 240 0.0
mbed all-clusters-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2348496 2348496 0 0.0
.bss 188724 188724 0 0.0
.data 5312 5312 0 0.0
.text 1311072 1311072 0 0.0
lighting-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2330392 2330688 296 0.0
.bss 180544 180544 0 0.0
.data 5552 5552 0 0.0
.text 1292992 1293288 296 0.0
lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2303544 2303840 296 0.0
.bss 179592 179592 0 0.0
.data 5544 5544 0 0.0
.text 1266144 1266440 296 0.0
pigweed-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 1139712 1140008 296 0.0
.bss 11756 11756 0 0.0
.data 4368 4368 0 0.0
.text 103096 103392 296 0.3
shell CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2054256 2054256 0 0.0
.bss 157060 157060 0 0.0
.data 4864 4864 0 0.0
.text 1016856 1016856 0 0.0
nrfconnect lighting-app nrf52840dk_nrf52840 (read/write) 937083 937083 0 0.0
bss 118112 118112 0 0.0
rodata 108120 108120 0 0.0
text 633292 633292 0 0.0
nrf52840dk_nrf52840+rpc (read/write) 922551 923479 928 0.1
bss 115156 115156 0 0.0
rodata 100572 101548 976 1.0
text 628644 628604 -40 -0.0
nrf5340dk_nrf5340_cpuapp (read/write) 848062 848062 0 0.0
bss 116004 116004 0 0.0
rodata 101296 101296 0 0.0
text 550228 550228 0 0.0
lock-app nrf52840dk_nrf52840 (read/write) 909227 909227 0 0.0
bss 117300 117300 0 0.0
rodata 103392 103392 0 0.0
text 611160 611160 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 820386 820386 0 0.0
bss 115220 115220 0 0.0
rodata 96620 96620 0 0.0
text 528132 528132 0 0.0
pigweed-app nrf52840dk_nrf52840 (read/write) 541835 542351 516 0.1
bss 52588 52588 0 0.0
rodata 50104 50668 564 1.1
text 376940 376892 -48 -0.0
pump-app nrf52840dk_nrf52840 (read/write) 910491 910491 0 0.0
bss 117060 117060 0 0.0
rodata 103608 103608 0 0.0
text 612372 612372 0 0.0
pump-controller-app nrf52840dk_nrf52840 (read/write) 907307 907307 0 0.0
bss 117088 117088 0 0.0
rodata 102864 102864 0 0.0
text 609908 609908 0 0.0
shell nrf52840dk_nrf52840 (read/write) 797919 797919 0 0.0
bss 109768 109768 0 0.0
rodata 78148 78148 0 0.0
text 533496 533496 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 710734 710734 0 0.0
bss 107656 107656 0 0.0
rodata 72448 72448 0 0.0
text 451172 451172 0 0.0
p6 all-clusters-app default (read/write) 2401624 2401624 0 0.0
.bss 116804 116804 0 0.0
.data 2592 2592 0 0.0
.text 1359888 1359888 0 0.0
light-app default (read/write) 2323648 2323648 0 0.0
.bss 105672 105672 0 0.0
.data 2384 2384 0 0.0
.text 1281912 1281912 0 0.0
lock-app default (read/write) 2295872 2295872 0 0.0
.bss 104552 104552 0 0.0
.data 2336 2336 0 0.0
.text 1254136 1254136 0 0.0
qpg lighting-app qpg6105+debug (read only) 533160 533160 0 0.0
(read/write) 146936 146936 0 0.0
.bss 86624 86624 0 0.0
.data 1004 1004 0 0.0
.text 527840 527840 0 0.0
lock-app qpg6105+debug (read only) 504936 504936 0 0.0
(read/write) 146940 146940 0 0.0
.bss 85760 85760 0 0.0
.data 952 952 0 0.0
.text 499616 499616 0 0.0
persistent-storage-app qpg6105+debug (read only) 106448 106448 0 0.0
(read/write) 146938 146938 0 0.0
.bss 36146 36146 0 0.0
.data 288 288 0 0.0
.text 101128 101128 0 0.0
telink lighting-app tlsr9518adk80d (read/write) 834466 834466 0 0.0
bss 86924 86924 0 0.0
noinit 37160 37160 0 0.0
text 582710 582710 0 0.0

@andy31415 andy31415 merged commit c25c4b3 into project-chip:master Jan 7, 2022
andy31415 added a commit to andy31415/connectedhomeip that referenced this pull request Jan 10, 2022
@andy31415
Roll pigweed forward (was rolled back by project-chip#13382)
2861502
andy31415 added a commit that referenced this pull request Jan 10, 2022
@andy31415
Roll pigweed forward (was rolled back by #13382) (#13424)
416b98e
step0035 pushed a commit to hank820/connectedhomeip that referenced this pull request Feb 8, 2022
@msandstedt @step0035
Fix project-chip#13377 for compatibility with project-chip#13330 (pro…
79320b1 
…ject-chip#13382)

The pull requests project-chip#13330 and project-chip#13377 were merged around the same time,
and have caused a small build breakage.  This fixes it.
step0035 pushed a commit to hank820/connectedhomeip that referenced this pull request Feb 8, 2022
@andy31415 @step0035
Roll pigweed forward (was rolled back by project-chip#13382) (project…
2bf37d2 
…-chip#13424)
@msandstedt msandstedt deleted the 13330-tree-fix branch March 31, 2022 13:23
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

@saurabhst saurabhst saurabhst approved these changes

@turon turon turon approved these changes

Assignees
No one assigned
Labels
examples hotfix urgent fix needed, can bypass review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@msandstedt @turon @saurabhst @bzbarsky-apple @andy31415