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

[mrp] Fix MRP after a recent refactoring #13413

Merged
merged 1 commit into from
Jan 10, 2022
Merged

[mrp] Fix MRP after a recent refactoring #13413

merged 1 commit into from
Jan 10, 2022

Conversation

Damian-Nordic
Copy link
Contributor

Problem

PR #11988 has broken StartTimer call in ReliableMessageMgr by passing a timestamp instead of a delay. Consequently, retransmissions stopped working.

Also, it was not caught because unit tests call the timeout function explicitly instead of relying on the system layer.

Change overview

Fix the issue and enhance unit tests.
Additionally, fix a crash on access to AsSecureSession() if a retransmission occurs during PASE or CASE.

Testing

Unit tests + manual smoke tests with nRF Connect lock running as sleepy-end device.

@Damian-Nordic Damian-Nordic requested a review from kghost January 10, 2022 14:56
@andy31415
Copy link
Contributor

fast track: small delta with solid unit test update.

@github-actions
Copy link

PR #13413: Size comparison from f2c2650 to f0db9f6

Increases (10 builds for k32w, linux, p6, qpg, telink)
platform target config section f2c2650 f0db9f6 change % change
k32w light k32w061+release (read/write) 655308 655340 32 0.0
.text 570884 570916 32 0.0
lock k32w061+release (read/write) 659632 659680 48 0.0
.text 574892 574940 48 0.0
linux chip-tool-ipv6only arm64 (read only) 7105196 7105308 112 0.0
.text 6019748 6019860 112 0.0
thermostat-no-ble arm64 (read only) 2033724 2033868 144 0.0
.text 1691088 1691232 144 0.0
p6 all-clusters-app default (read/write) 2401288 2401320 32 0.0
.text 1359552 1359584 32 0.0
light-app default (read/write) 2323696 2323728 32 0.0
.text 1281960 1281992 32 0.0
lock-app default (read/write) 2295920 2295952 32 0.0
.text 1254184 1254216 32 0.0
qpg lighting-app qpg6105+debug (read only) 533208 533248 40 0.0
.text 527888 527928 40 0.0
lock-app qpg6105+debug (read only) 504984 505024 40 0.0
.text 499664 499704 40 0.0
telink lighting-app tlsr9518adk80d (read/write) 834522 834546 24 0.0
text 582766 582796 30 0.0
Full report (11 builds for k32w, linux, p6, qpg, telink)
platform target config section f2c2650 f0db9f6 change % change
k32w light k32w061+release (read/write) 655308 655340 32 0.0
.bss 76776 76776 0 0.0
.data 1848 1848 0 0.0
.text 570884 570916 32 0.0
lock k32w061+release (read/write) 659632 659680 48 0.0
.bss 77072 77072 0 0.0
.data 1868 1868 0 0.0
.text 574892 574940 48 0.0
linux chip-tool-ipv6only arm64 (read only) 7105196 7105308 112 0.0
(read/write) 327009 327009 0 0.0
.bss 54865 54865 0 0.0
.data 1096 1096 0 0.0
.data.rel.ro 209392 209392 0 0.0
.dynamic 560 560 0 0.0
.got 57968 57968 0 0.0
.init 24 24 0 0.0
.init_array 168 168 0 0.0
.rodata 388900 388900 0 0.0
.text 6019748 6019860 112 0.0
thermostat-no-ble arm64 (read only) 2033724 2033868 144 0.0
(read/write) 145089 145089 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 4000 4000 0 0.0
.init 24 24 0 0.0
.init_array 296 296 0 0.0
.rodata 128988 128988 0 0.0
.text 1691088 1691232 144 0.0
p6 all-clusters-app default (read/write) 2401288 2401320 32 0.0
.bss 116812 116812 0 0.0
.data 2592 2592 0 0.0
.text 1359552 1359584 32 0.0
light-app default (read/write) 2323696 2323728 32 0.0
.bss 105672 105672 0 0.0
.data 2384 2384 0 0.0
.text 1281960 1281992 32 0.0
lock-app default (read/write) 2295920 2295952 32 0.0
.bss 104552 104552 0 0.0
.data 2336 2336 0 0.0
.text 1254184 1254216 32 0.0
qpg lighting-app qpg6105+debug (read only) 533208 533248 40 0.0
(read/write) 146936 146936 0 0.0
.bss 86624 86624 0 0.0
.data 1004 1004 0 0.0
.text 527888 527928 40 0.0
lock-app qpg6105+debug (read only) 504984 505024 40 0.0
(read/write) 146940 146940 0 0.0
.bss 85760 85760 0 0.0
.data 952 952 0 0.0
.text 499664 499704 40 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) 834522 834546 24 0.0
bss 86924 86924 0 0.0
noinit 37160 37160 0 0.0
text 582766 582796 30 0.0

@andy31415
Copy link
Contributor

/rebase

@Damian-Nordic @andy31415
[mrp] Fix MRP after a recent refactoring
d7e90ba 
PR 11988 has broken StartTimer call in ReliableMessageMgr
by passing a timestamp instead of a delay. Consequently,
retransmissions stopped working.

Also, it was not caught because unit tests call the timeout
function explicitly instead of relying on the system layer.

Fix the issue and enhance unit tests. Additionally, fix
a crash on access to AsSecureSession() if a retransmission
occurs during PASE or CASE.
@github-actions
Copy link

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

PR #13413: Size comparison from f2c2650 to d7e90ba

Increases (26 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
platform target config section f2c2650 d7e90ba change % change
efr32 lighting-app BRD4161A (read only) 829244 829276 32 0.0
.text 829236 829268 32 0.0
BRD4161A+rpc (read only) 816888 816920 32 0.0
.text 816880 816912 32 0.0
window-app BRD4161A (read only) 802708 802740 32 0.0
.text 802700 802732 32 0.0
esp32 all-clusters-app c3devkit (read only) 891640 891670 30 0.0
.flash.text 891640 891670 30 0.0
m5stack (read only) 951627 951659 32 0.0
.flash.text 946243 946275 32 0.0
k32w light k32w061+release (read/write) 655308 655340 32 0.0
.text 570884 570916 32 0.0
lock k32w061+release (read/write) 659632 659680 48 0.0
.text 574892 574940 48 0.0
linux chip-tool-ipv6only arm64 (read only) 7105196 7105308 112 0.0
.text 6019748 6019860 112 0.0
thermostat-no-ble arm64 (read only) 2033724 2033868 144 0.0
.text 1691088 1691232 144 0.0
mbed all-clusters-app CY8CPROTO_062_4343W+release (read/write) 2348240 2348304 64 0.0
.text 1310816 1310880 64 0.0
shell CY8CPROTO_062_4343W+release (read/write) 2054256 2054320 64 0.0
.text 1016856 1016920 64 0.0
nrfconnect lighting-app nrf52840dk_nrf52840 (read/write) 937115 937163 48 0.0
text 633332 633372 40 0.0
nrf52840dk_nrf52840+rpc (read/write) 923527 923559 32 0.0
text 628644 628684 40 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 848110 848142 32 0.0
text 550268 550308 40 0.0
lock-app nrf52840dk_nrf52840 (read/write) 909259 909307 48 0.0
text 611196 611236 40 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 820434 820466 32 0.0
text 528172 528212 40 0.0
pump-app nrf52840dk_nrf52840 (read/write) 910539 910571 32 0.0
text 612412 612452 40 0.0
pump-controller-app nrf52840dk_nrf52840 (read/write) 907339 907387 48 0.0
text 609944 609984 40 0.0
shell nrf52840dk_nrf52840 (read/write) 797935 797967 32 0.0
text 533508 533548 40 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 710750 710782 32 0.0
text 451188 451228 40 0.0
p6 all-clusters-app default (read/write) 2401288 2401320 32 0.0
.text 1359552 1359584 32 0.0
light-app default (read/write) 2323696 2323728 32 0.0
.text 1281960 1281992 32 0.0
lock-app default (read/write) 2295920 2295952 32 0.0
.text 1254184 1254216 32 0.0
qpg lighting-app qpg6105+debug (read only) 533208 533248 40 0.0
.text 527888 527928 40 0.0
lock-app qpg6105+debug (read only) 504984 505024 40 0.0
.text 499664 499704 40 0.0
telink lighting-app tlsr9518adk80d (read/write) 834522 834546 24 0.0
text 582766 582796 30 0.0
Full report (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
platform target config section f2c2650 d7e90ba change % change
efr32 lighting-app BRD4161A (read only) 829244 829276 32 0.0
(read/write) 126996 126996 0 0.0
.bss 125120 125120 0 0.0
.data 1876 1876 0 0.0
.text 829236 829268 32 0.0
BRD4161A+rpc (read only) 816888 816920 32 0.0
(read/write) 143656 143656 0 0.0
.bss 141680 141680 0 0.0
.data 1976 1976 0 0.0
.text 816880 816912 32 0.0
window-app BRD4161A (read only) 802708 802740 32 0.0
(read/write) 125936 125936 0 0.0
.bss 124104 124104 0 0.0
.data 1832 1832 0 0.0
.text 802700 802732 32 0.0
esp32 all-clusters-app c3devkit (read only) 891640 891670 30 0.0
(read/write) 1314026 1314026 0 0.0
.dram0.bss 69480 69480 0 0.0
.dram0.data 14236 14236 0 0.0
.flash.rodata 177248 177248 0 0.0
.flash.text 891640 891670 30 0.0
.iram0.text 62254 62254 0 0.0
m5stack (read only) 951627 951659 32 0.0
(read/write) 445604 445604 0 0.0
.dram0.bss 73968 73968 0 0.0
.dram0.data 34064 34064 0 0.0
.flash.rodata 206564 206564 0 0.0
.flash.text 946243 946275 32 0.0
.iram0.text 122671 122671 0 0.0
k32w light k32w061+release (read/write) 655308 655340 32 0.0
.bss 76776 76776 0 0.0
.data 1848 1848 0 0.0
.text 570884 570916 32 0.0
lock k32w061+release (read/write) 659632 659680 48 0.0
.bss 77072 77072 0 0.0
.data 1868 1868 0 0.0
.text 574892 574940 48 0.0
linux chip-tool-ipv6only arm64 (read only) 7105196 7105308 112 0.0
(read/write) 327009 327009 0 0.0
.bss 54865 54865 0 0.0
.data 1096 1096 0 0.0
.data.rel.ro 209392 209392 0 0.0
.dynamic 560 560 0 0.0
.got 57968 57968 0 0.0
.init 24 24 0 0.0
.init_array 168 168 0 0.0
.rodata 388900 388900 0 0.0
.text 6019748 6019860 112 0.0
thermostat-no-ble arm64 (read only) 2033724 2033868 144 0.0
(read/write) 145089 145089 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 4000 4000 0 0.0
.init 24 24 0 0.0
.init_array 296 296 0 0.0
.rodata 128988 128988 0 0.0
.text 1691088 1691232 144 0.0
mbed all-clusters-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2348240 2348304 64 0.0
.bss 188724 188724 0 0.0
.data 5312 5312 0 0.0
.text 1310816 1310880 64 0.0
lighting-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2330752 2330752 0 0.0
.bss 180544 180544 0 0.0
.data 5552 5552 0 0.0
.text 1293352 1293352 0 0.0
lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2303904 2303904 0 0.0
.bss 179592 179592 0 0.0
.data 5544 5544 0 0.0
.text 1266504 1266504 0 0.0
pigweed-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 1140008 1140008 0 0.0
.bss 11756 11756 0 0.0
.data 4368 4368 0 0.0
.text 103392 103392 0 0.0
shell CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2054256 2054320 64 0.0
.bss 157060 157060 0 0.0
.data 4864 4864 0 0.0
.text 1016856 1016920 64 0.0
nrfconnect lighting-app nrf52840dk_nrf52840 (read/write) 937115 937163 48 0.0
bss 118112 118112 0 0.0
rodata 108120 108120 0 0.0
text 633332 633372 40 0.0
nrf52840dk_nrf52840+rpc (read/write) 923527 923559 32 0.0
bss 115156 115156 0 0.0
rodata 101548 101548 0 0.0
text 628644 628684 40 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 848110 848142 32 0.0
bss 116004 116004 0 0.0
rodata 101296 101296 0 0.0
text 550268 550308 40 0.0
lock-app nrf52840dk_nrf52840 (read/write) 909259 909307 48 0.0
bss 117300 117300 0 0.0
rodata 103392 103392 0 0.0
text 611196 611236 40 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 820434 820466 32 0.0
bss 115220 115220 0 0.0
rodata 96620 96620 0 0.0
text 528172 528212 40 0.0
pigweed-app nrf52840dk_nrf52840 (read/write) 542351 542351 0 0.0
bss 52588 52588 0 0.0
rodata 50668 50668 0 0.0
text 376892 376892 0 0.0
pump-app nrf52840dk_nrf52840 (read/write) 910539 910571 32 0.0
bss 117060 117060 0 0.0
rodata 103608 103608 0 0.0
text 612412 612452 40 0.0
pump-controller-app nrf52840dk_nrf52840 (read/write) 907339 907387 48 0.0
bss 117088 117088 0 0.0
rodata 102864 102864 0 0.0
text 609944 609984 40 0.0
shell nrf52840dk_nrf52840 (read/write) 797935 797967 32 0.0
bss 109768 109768 0 0.0
rodata 78148 78148 0 0.0
text 533508 533548 40 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 710750 710782 32 0.0
bss 107656 107656 0 0.0
rodata 72448 72448 0 0.0
text 451188 451228 40 0.0
p6 all-clusters-app default (read/write) 2401288 2401320 32 0.0
.bss 116812 116812 0 0.0
.data 2592 2592 0 0.0
.text 1359552 1359584 32 0.0
light-app default (read/write) 2323696 2323728 32 0.0
.bss 105672 105672 0 0.0
.data 2384 2384 0 0.0
.text 1281960 1281992 32 0.0
lock-app default (read/write) 2295920 2295952 32 0.0
.bss 104552 104552 0 0.0
.data 2336 2336 0 0.0
.text 1254184 1254216 32 0.0
qpg lighting-app qpg6105+debug (read only) 533208 533248 40 0.0
(read/write) 146936 146936 0 0.0
.bss 86624 86624 0 0.0
.data 1004 1004 0 0.0
.text 527888 527928 40 0.0
lock-app qpg6105+debug (read only) 504984 505024 40 0.0
(read/write) 146940 146940 0 0.0
.bss 85760 85760 0 0.0
.data 952 952 0 0.0
.text 499664 499704 40 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) 834522 834546 24 0.0
bss 86924 86924 0 0.0
noinit 37160 37160 0 0.0
text 582766 582796 30 0.0

@andy31415 andy31415 merged commit 0307df3 into project-chip:master Jan 10, 2022
step0035 pushed a commit to hank820/connectedhomeip that referenced this pull request Feb 8, 2022
@Damian-Nordic @step0035
[mrp] Fix MRP after a recent refactoring (project-chip#13413)
37ca8d7 
PR 11988 has broken StartTimer call in ReliableMessageMgr
by passing a timestamp instead of a delay. Consequently,
retransmissions stopped working.

Also, it was not caught because unit tests call the timeout
function explicitly instead of relying on the system layer.

Fix the issue and enhance unit tests. Additionally, fix
a crash on access to AsSecureSession() if a retransmission
occurs during PASE or CASE.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yufengwangca yufengwangca yufengwangca approved these changes

@andy31415 andy31415 andy31415 approved these changes

@kghost kghost Awaiting requested review from kghost

Assignees
No one assigned
Labels
controller fast track transport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Damian-Nordic @andy31415 @yufengwangca