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

Fix AttributeCache forwarding callback #16566

Conversation

kpark-apple
Copy link
Contributor

@kpark-apple kpark-apple commented Mar 23, 2022

Problem

  • Fix AttributeCache forwarding OnAttributeData callback with incorrect data

Change overview

  • Fix forwarded OnAttributeData callback losing data by
    copying the TLV reader before its state changes for cache update.

Testing

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.

As a followup, could we add a test to src/app/tests/TestAttributeCache.cpp that would catch this? Probably adding an OnAttributeData to CacheValidator and doing some checks there....

src/app/AttributeCache.cpp Outdated Show resolved Hide resolved
src/app/AttributeCache.cpp Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Mar 23, 2022

PR #16566: Size comparison from 4a33c98 to f6599d4

Increases (3 builds for linux)
platform target config section 4a33c98 f6599d4 change % change
linux chip-tool debug (read only) 10179253 10179509 256 0.0
.text 8876325 8876581 256 0.0
chip-tool-ipv6only arm64 (read only) 9798524 9798732 208 0.0
.text 8250228 8250436 208 0.0
tv-app debug (read only) 2678521 2678777 256 0.0
.text 2296514 2296770 256 0.0
Full report (27 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
platform target config section 4a33c98 f6599d4 change % change
cyw30739 light cyw930739m2evb_01 (read/write) 603478 603478 0 0.0
.app_xip_area 510576 510576 0 0.0
.bss 75656 75656 0 0.0
.data 596 596 0 0.0
.rodata 0 0 0 0.0
.text 0 0 0 0.0
lock cyw930739m2evb_01 (read/write) 561274 561274 0 0.0
.app_xip_area 469900 469900 0 0.0
.bss 74160 74160 0 0.0
.data 560 560 0 0.0
.rodata 0 0 0 0.0
.text 0 0 0 0.0
ota-requestor-no-progress-logging cyw930739m2evb_01 (read/write) 572806 572806 0 0.0
.app_xip_area 471784 471784 0 0.0
.bss 83488 83488 0 0.0
.data 500 500 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
efr32 lighting-app BRD4161A (read only) 922300 922300 0 0.0
(read/write) 128756 128756 0 0.0
.bss 126768 126768 0 0.0
.data 1988 1988 0 0.0
.text 922292 922292 0 0.0
BRD4161A+rpc (read only) 951128 951128 0 0.0
(read/write) 144712 144712 0 0.0
.bss 142544 142544 0 0.0
.data 2168 2168 0 0.0
.text 951120 951120 0 0.0
window-app BRD4161A (read only) 852660 852660 0 0.0
(read/write) 126720 126720 0 0.0
.bss 124856 124856 0 0.0
.data 1864 1864 0 0.0
.text 852652 852652 0 0.0
esp32 all-clusters-app c3devkit (read only) 964156 964156 0 0.0
(read/write) 1393714 1393714 0 0.0
.dram0.bss 62072 62072 0 0.0
.dram0.data 14188 14188 0 0.0
.flash.rodata 198536 198536 0 0.0
.flash.text 964156 964156 0 0.0
.iram0.text 62016 62016 0 0.0
m5stack (read only) 1020687 1020687 0 0.0
(read/write) 461492 461492 0 0.0
.dram0.bss 67600 67600 0 0.0
.dram0.data 34016 34016 0 0.0
.flash.rodata 228040 228040 0 0.0
.flash.text 1015303 1015303 0 0.0
.iram0.text 123107 123107 0 0.0
k32w light k32w061+release (read/write) 701120 701120 0 0.0
.bss 77656 77656 0 0.0
.data 1868 1868 0 0.0
.text 615796 615796 0 0.0
lock k32w061+release (read/write) 701088 701088 0 0.0
.bss 77624 77624 0 0.0
.data 1908 1908 0 0.0
.text 615756 615756 0 0.0
linux all-clusters-app debug (read only) 2478329 2478329 0 0.0
(read/write) 143216 143216 0 0.0
.bss 57312 57312 0 0.0
.data 1264 1264 0 0.0
.data.rel.ro 78904 78904 0 0.0
.dynamic 592 592 0 0.0
.got 4168 4168 0 0.0
.init 27 27 0 0.0
.init_array 936 936 0 0.0
.rodata 211941 211941 0 0.0
.text 2102914 2102914 0 0.0
bridge-app debug+rpc (read only) 1753373 1753373 0 0.0
(read/write) 89424 89424 0 0.0
.bss 44456 44456 0 0.0
.data 1952 1952 0 0.0
.data.rel.ro 37944 37944 0 0.0
.dynamic 592 592 0 0.0
.got 3920 3920 0 0.0
.init 27 27 0 0.0
.init_array 544 544 0 0.0
.rodata 144556 144556 0 0.0
.text 1493605 1493605 0 0.0
chip-tool debug (read only) 10179253 10179509 256 0.0
(read/write) 354976 354976 0 0.0
.bss 22336 22336 0 0.0
.data 1072 1072 0 0.0
.data.rel.ro 325520 325520 0 0.0
.dynamic 608 608 0 0.0
.got 4784 4784 0 0.0
.init 27 27 0 0.0
.init_array 632 632 0 0.0
.rodata 520085 520085 0 0.0
.text 8876325 8876581 256 0.0
chip-tool-ipv6only arm64 (read only) 9798524 9798732 208 0.0
(read/write) 473169 473169 0 0.0
.bss 40625 40625 0 0.0
.data 1128 1128 0 0.0
.data.rel.ro 371936 371936 0 0.0
.dynamic 560 560 0 0.0
.got 55680 55680 0 0.0
.init 24 24 0 0.0
.init_array 192 192 0 0.0
.rodata 495636 495636 0 0.0
.text 8250228 8250436 208 0.0
door-lock-app debug (read only) 2005129 2005129 0 0.0
(read/write) 116832 116832 0 0.0
.bss 47584 47584 0 0.0
.data 992 992 0 0.0
.data.rel.ro 62856 62856 0 0.0
.dynamic 592 592 0 0.0
.got 4120 4120 0 0.0
.init 27 27 0 0.0
.init_array 664 664 0 0.0
.rodata 180988 180988 0 0.0
.text 1674482 1674482 0 0.0
lighting-app debug+rpc (read only) 2178777 2178777 0 0.0
(read/write) 123792 123792 0 0.0
.bss 48864 48864 0 0.0
.data 1472 1472 0 0.0
.data.rel.ro 67928 67928 0 0.0
.dynamic 608 608 0 0.0
.got 4168 4168 0 0.0
.init 27 27 0 0.0
.init_array 744 744 0 0.0
.rodata 175164 175164 0 0.0
.text 1844594 1844594 0 0.0
ota-provider-app debug (read only) 1946513 1946513 0 0.0
(read/write) 112560 112560 0 0.0
.bss 47456 47456 0 0.0
.data 1256 1256 0 0.0
.data.rel.ro 58200 58200 0 0.0
.dynamic 608 608 0 0.0
.got 4376 4376 0 0.0
.init 27 27 0 0.0
.init_array 616 616 0 0.0
.rodata 166987 166987 0 0.0
.text 1627810 1627810 0 0.0
ota-requestor-app debug (read only) 1969953 1969953 0 0.0
(read/write) 115592 115592 0 0.0
.bss 48480 48480 0 0.0
.data 1416 1416 0 0.0
.data.rel.ro 60264 60264 0 0.0
.dynamic 592 592 0 0.0
.got 4168 4168 0 0.0
.init 27 27 0 0.0
.init_array 640 640 0 0.0
.rodata 162980 162980 0 0.0
.text 1654962 1654962 0 0.0
shell debug (read only) 2427657 2427657 0 0.0
(read/write) 147272 147272 0 0.0
.bss 67240 67240 0 0.0
.data 784 784 0 0.0
.data.rel.ro 73536 73536 0 0.0
.dynamic 592 592 0 0.0
.got 4152 4152 0 0.0
.init 27 27 0 0.0
.init_array 920 920 0 0.0
.rodata 209362 209362 0 0.0
.text 2061570 2061570 0 0.0
thermostat-no-ble arm64 (read only) 2262692 2262692 0 0.0
(read/write) 148273 148273 0 0.0
.bss 62753 62753 0 0.0
.data 1024 1024 0 0.0
.data.rel.ro 77000 77000 0 0.0
.dynamic 560 560 0 0.0
.got 4480 4480 0 0.0
.init 24 24 0 0.0
.init_array 360 360 0 0.0
.rodata 139780 139780 0 0.0
.text 1899600 1899600 0 0.0
tv-app debug (read only) 2678521 2678777 256 0.0
(read/write) 247744 247744 0 0.0
.bss 164416 164416 0 0.0
.data 3104 3104 0 0.0
.data.rel.ro 74168 74168 0 0.0
.dynamic 592 592 0 0.0
.got 4552 4552 0 0.0
.init 27 27 0 0.0
.init_array 888 888 0 0.0
.rodata 207157 207157 0 0.0
.text 2296514 2296770 256 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2353636 2353636 0 0.0
.bss 184652 184652 0 0.0
.data 5752 5752 0 0.0
.text 1316236 1316236 0 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1138947 1138947 0 0.0
bss 142588 142588 0 0.0
rodata 141516 141516 0 0.0
text 780084 780084 0 0.0
p6 all-clusters-app default (read/write) 2493592 2493592 0 0.0
.bss 118072 118072 0 0.0
.data 2632 2632 0 0.0
.text 1451856 1451856 0 0.0
light-app default (read/write) 2396776 2396776 0 0.0
.bss 111544 111544 0 0.0
.data 2488 2488 0 0.0
.text 1355040 1355040 0 0.0
lock-app default (read/write) 2360312 2360312 0 0.0
.bss 111288 111288 0 0.0
.data 2448 2448 0 0.0
.text 1318576 1318576 0 0.0
telink lighting-app tlsr9518adk80d (read/write) 896954 896954 0 0.0
bss 87444 87444 0 0.0
noinit 37160 37160 0 0.0
text 634304 634304 0 0.0

@kpark-apple kpark-apple force-pushed the fix/kpark/20220323-attrib-cache-callback branch from f6599d4 to cee6900 Compare March 23, 2022 18:12
@kpark-apple
Copy link
Contributor Author

As a followup, could we add a test to src/app/tests/TestAttributeCache.cpp that would catch this? Probably adding an OnAttributeData to CacheValidator and doing some checks there....

I added new validator to the test.

* Fix forwarded OnAttributeData callback losing data by
  copying the TLV reader before its state changes for cache update.
@kpark-apple kpark-apple force-pushed the fix/kpark/20220323-attrib-cache-callback branch from cee6900 to df6df13 Compare March 23, 2022 18:55
@github-actions
Copy link

github-actions bot commented Mar 23, 2022

PR #16566: Size comparison from 2b7a67e to df6df13

Increases (3 builds for linux)
platform target config section 2b7a67e df6df13 change % change
linux chip-tool debug (read only) 10180709 10180789 80 0.0
.text 8877653 8877733 80 0.0
chip-tool-ipv6only arm64 (read only) 9799868 9799948 80 0.0
.text 8251428 8251508 80 0.0
tv-app debug (read only) 2679225 2679321 96 0.0
.text 2297218 2297314 96 0.0
Full report (27 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
platform target config section 2b7a67e df6df13 change % change
cyw30739 light cyw930739m2evb_01 (read/write) 603478 603478 0 0.0
.app_xip_area 510576 510576 0 0.0
.bss 75656 75656 0 0.0
.data 596 596 0 0.0
.rodata 0 0 0 0.0
.text 0 0 0 0.0
lock cyw930739m2evb_01 (read/write) 561274 561274 0 0.0
.app_xip_area 469900 469900 0 0.0
.bss 74160 74160 0 0.0
.data 560 560 0 0.0
.rodata 0 0 0 0.0
.text 0 0 0 0.0
ota-requestor-no-progress-logging cyw930739m2evb_01 (read/write) 573358 573358 0 0.0
.app_xip_area 472336 472336 0 0.0
.bss 83488 83488 0 0.0
.data 500 500 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
efr32 lighting-app BRD4161A (read only) 922876 922876 0 0.0
(read/write) 128748 128748 0 0.0
.bss 126760 126760 0 0.0
.data 1988 1988 0 0.0
.text 922868 922868 0 0.0
BRD4161A+rpc (read only) 951708 951708 0 0.0
(read/write) 144704 144704 0 0.0
.bss 142536 142536 0 0.0
.data 2168 2168 0 0.0
.text 951700 951700 0 0.0
window-app BRD4161A (read only) 852672 852672 0 0.0
(read/write) 126720 126720 0 0.0
.bss 124856 124856 0 0.0
.data 1864 1864 0 0.0
.text 852664 852664 0 0.0
esp32 all-clusters-app c3devkit (read only) 964156 964156 0 0.0
(read/write) 1393714 1393714 0 0.0
.dram0.bss 62072 62072 0 0.0
.dram0.data 14188 14188 0 0.0
.flash.rodata 198536 198536 0 0.0
.flash.text 964156 964156 0 0.0
.iram0.text 62016 62016 0 0.0
m5stack (read only) 1020691 1020691 0 0.0
(read/write) 461492 461492 0 0.0
.dram0.bss 67600 67600 0 0.0
.dram0.data 34016 34016 0 0.0
.flash.rodata 228040 228040 0 0.0
.flash.text 1015307 1015307 0 0.0
.iram0.text 123107 123107 0 0.0
k32w light k32w061+release (read/write) 701652 701652 0 0.0
.bss 77648 77648 0 0.0
.data 1868 1868 0 0.0
.text 616336 616336 0 0.0
lock k32w061+release (read/write) 701088 701088 0 0.0
.bss 77624 77624 0 0.0
.data 1908 1908 0 0.0
.text 615756 615756 0 0.0
linux all-clusters-app debug (read only) 2478313 2478313 0 0.0
(read/write) 143216 143216 0 0.0
.bss 57312 57312 0 0.0
.data 1264 1264 0 0.0
.data.rel.ro 78904 78904 0 0.0
.dynamic 592 592 0 0.0
.got 4168 4168 0 0.0
.init 27 27 0 0.0
.init_array 936 936 0 0.0
.rodata 211941 211941 0 0.0
.text 2102898 2102898 0 0.0
bridge-app debug+rpc (read only) 1753357 1753357 0 0.0
(read/write) 89424 89424 0 0.0
.bss 44456 44456 0 0.0
.data 1952 1952 0 0.0
.data.rel.ro 37944 37944 0 0.0
.dynamic 592 592 0 0.0
.got 3920 3920 0 0.0
.init 27 27 0 0.0
.init_array 544 544 0 0.0
.rodata 144556 144556 0 0.0
.text 1493589 1493589 0 0.0
chip-tool debug (read only) 10180709 10180789 80 0.0
(read/write) 354976 354976 0 0.0
.bss 22336 22336 0 0.0
.data 1072 1072 0 0.0
.data.rel.ro 325520 325520 0 0.0
.dynamic 608 608 0 0.0
.got 4784 4784 0 0.0
.init 27 27 0 0.0
.init_array 632 632 0 0.0
.rodata 520213 520213 0 0.0
.text 8877653 8877733 80 0.0
chip-tool-ipv6only arm64 (read only) 9799868 9799948 80 0.0
(read/write) 473185 473185 0 0.0
.bss 40625 40625 0 0.0
.data 1128 1128 0 0.0
.data.rel.ro 371960 371960 0 0.0
.dynamic 560 560 0 0.0
.got 55680 55680 0 0.0
.init 24 24 0 0.0
.init_array 192 192 0 0.0
.rodata 495764 495764 0 0.0
.text 8251428 8251508 80 0.0
door-lock-app debug (read only) 2005113 2005113 0 0.0
(read/write) 116832 116832 0 0.0
.bss 47584 47584 0 0.0
.data 992 992 0 0.0
.data.rel.ro 62856 62856 0 0.0
.dynamic 592 592 0 0.0
.got 4120 4120 0 0.0
.init 27 27 0 0.0
.init_array 664 664 0 0.0
.rodata 180988 180988 0 0.0
.text 1674466 1674466 0 0.0
lighting-app debug+rpc (read only) 2178761 2178761 0 0.0
(read/write) 123792 123792 0 0.0
.bss 48864 48864 0 0.0
.data 1472 1472 0 0.0
.data.rel.ro 67928 67928 0 0.0
.dynamic 608 608 0 0.0
.got 4168 4168 0 0.0
.init 27 27 0 0.0
.init_array 744 744 0 0.0
.rodata 175164 175164 0 0.0
.text 1844578 1844578 0 0.0
ota-provider-app debug (read only) 1946497 1946497 0 0.0
(read/write) 112560 112560 0 0.0
.bss 47456 47456 0 0.0
.data 1256 1256 0 0.0
.data.rel.ro 58200 58200 0 0.0
.dynamic 608 608 0 0.0
.got 4376 4376 0 0.0
.init 27 27 0 0.0
.init_array 616 616 0 0.0
.rodata 166987 166987 0 0.0
.text 1627794 1627794 0 0.0
ota-requestor-app debug (read only) 1973305 1973305 0 0.0
(read/write) 115784 115784 0 0.0
.bss 48448 48448 0 0.0
.data 1448 1448 0 0.0
.data.rel.ro 60424 60424 0 0.0
.dynamic 592 592 0 0.0
.got 4184 4184 0 0.0
.init 27 27 0 0.0
.init_array 640 640 0 0.0
.rodata 162980 162980 0 0.0
.text 1657698 1657698 0 0.0
shell debug (read only) 2427657 2427657 0 0.0
(read/write) 147272 147272 0 0.0
.bss 67240 67240 0 0.0
.data 784 784 0 0.0
.data.rel.ro 73536 73536 0 0.0
.dynamic 592 592 0 0.0
.got 4152 4152 0 0.0
.init 27 27 0 0.0
.init_array 920 920 0 0.0
.rodata 209362 209362 0 0.0
.text 2061570 2061570 0 0.0
thermostat-no-ble arm64 (read only) 2262692 2262692 0 0.0
(read/write) 148273 148273 0 0.0
.bss 62753 62753 0 0.0
.data 1024 1024 0 0.0
.data.rel.ro 77000 77000 0 0.0
.dynamic 560 560 0 0.0
.got 4480 4480 0 0.0
.init 24 24 0 0.0
.init_array 360 360 0 0.0
.rodata 139780 139780 0 0.0
.text 1899600 1899600 0 0.0
tv-app debug (read only) 2679225 2679321 96 0.0
(read/write) 247744 247744 0 0.0
.bss 164416 164416 0 0.0
.data 3104 3104 0 0.0
.data.rel.ro 74168 74168 0 0.0
.dynamic 592 592 0 0.0
.got 4552 4552 0 0.0
.init 27 27 0 0.0
.init_array 888 888 0 0.0
.rodata 207157 207157 0 0.0
.text 2297218 2297314 96 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2353636 2353636 0 0.0
.bss 184652 184652 0 0.0
.data 5752 5752 0 0.0
.text 1316236 1316236 0 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1139467 1139467 0 0.0
bss 142588 142588 0 0.0
rodata 141588 141588 0 0.0
text 780524 780524 0 0.0
p6 all-clusters-app default (read/write) 2493592 2493592 0 0.0
.bss 118072 118072 0 0.0
.data 2632 2632 0 0.0
.text 1451856 1451856 0 0.0
light-app default (read/write) 2396776 2396776 0 0.0
.bss 111544 111544 0 0.0
.data 2488 2488 0 0.0
.text 1355040 1355040 0 0.0
lock-app default (read/write) 2360312 2360312 0 0.0
.bss 111288 111288 0 0.0
.data 2448 2448 0 0.0
.text 1318576 1318576 0 0.0
telink lighting-app tlsr9518adk80d (read/write) 896954 896954 0 0.0
bss 87444 87444 0 0.0
noinit 37160 37160 0 0.0
text 634304 634304 0 0.0

Copy link
Contributor

@mrjerryjohns mrjerryjohns left a comment

Choose a reason for hiding this comment

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

Nice find! I guess when I wrote it, I never had quite tested folks actually using the OnAttributeData if they're using the cache...

@bzbarsky-apple bzbarsky-apple merged commit 035911e into project-chip:master Mar 24, 2022
@kpark-apple kpark-apple deleted the fix/kpark/20220323-attrib-cache-callback branch March 24, 2022 18:10
andrei-menzopol pushed a commit to andrei-menzopol/connectedhomeip that referenced this pull request Apr 14, 2022
* Fix forwarded OnAttributeData callback losing data by
  copying the TLV reader before its state changes for cache update.
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