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

Allow null lists when writing access control cluster ACL entries #13089

Merged
merged 2 commits into from
Jan 11, 2022
Merged

Allow null lists when writing access control cluster ACL entries #13089

merged 2 commits into from
Jan 11, 2022

Conversation

mlepage-google
Copy link
Contributor

Problem

Can't omit subjects or targets in chip-repl, when writing access control cluster ACL entries,
because those fields are sent with explicit null value, and decode expects a value to be an array/list.

Change overview

When decoding subjects or targets, check for explicit null value and treat it as an empty array/list.

Testing

Tested in chip-repl.

@mlepage-google
Allow list fields to be null when written
d4d4099 
When access control cluster is decoding TLV to write entries, if
subjects or targets are explicitly null, treat them as empty lists.
@boring-cyborg boring-cyborg bot added the app label Dec 16, 2021
@mlepage-google
Copy link
Contributor Author

Attn @harimau-qirex

@mlepage-google mlepage-google changed the title Allow list fields to be null when written Allow null lists when writing access control cluster ACL entries Dec 16, 2021
@github-actions
Copy link

github-actions bot commented Dec 16, 2021 •
edited
Loading

PR #13089: Size comparison from 8e40abc to d4d4099

Increases (3 builds for esp32, p6)
platform target config section 8e40abc d4d4099 change % change
esp32 all-clusters-app c3devkit (read only) 877072 877098 26 0.0
.flash.text 877072 877098 26 0.0
m5stack (read only) 937951 937983 32 0.0
.flash.text 932567 932599 32 0.0
p6 all-clusters-app default (read/write) 2384624 2384656 32 0.0
.text 1342888 1342920 32 0.0
Full report (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
platform target config section 8e40abc d4d4099 change % change
efr32 lighting-app BRD4161A (read only) 829456 829456 0 0.0
(read/write) 127352 127352 0 0.0
.bss 125472 125472 0 0.0
.data 1876 1876 0 0.0
.text 829448 829448 0 0.0
BRD4161A+rpc (read only) 817084 817084 0 0.0
(read/write) 144016 144016 0 0.0
.bss 142040 142040 0 0.0
.data 1976 1976 0 0.0
.text 817076 817076 0 0.0
window-app BRD4161A (read only) 802848 802848 0 0.0
(read/write) 126288 126288 0 0.0
.bss 124456 124456 0 0.0
.data 1832 1832 0 0.0
.text 802840 802840 0 0.0
esp32 all-clusters-app c3devkit (read only) 877072 877098 26 0.0
(read/write) 1313042 1313042 0 0.0
.dram0.bss 69784 69784 0 0.0
.dram0.data 14220 14220 0 0.0
.flash.rodata 175976 175976 0 0.0
.flash.text 877072 877098 26 0.0
.iram0.text 62254 62254 0 0.0
m5stack (read only) 937951 937983 32 0.0
(read/write) 442144 442144 0 0.0
.dram0.bss 74280 74280 0 0.0
.dram0.data 34056 34056 0 0.0
.flash.rodata 202800 202800 0 0.0
.flash.text 932567 932599 32 0.0
.iram0.text 122671 122671 0 0.0
k32w light k32w061+release (read/write) 648312 648312 0 0.0
.bss 76480 76480 0 0.0
.data 1904 1904 0 0.0
.text 564128 564128 0 0.0
lock k32w061+release (read/write) 633028 633028 0 0.0
.bss 76200 76200 0 0.0
.data 1860 1860 0 0.0
.text 549168 549168 0 0.0
linux chip-tool-ipv6only arm64 (read only) 6930700 6930700 0 0.0
(read/write) 323633 323633 0 0.0
.bss 54577 54577 0 0.0
.data 1096 1096 0 0.0
.data.rel.ro 208096 208096 0 0.0
.dynamic 560 560 0 0.0
.got 56160 56160 0 0.0
.init 24 24 0 0.0
.init_array 168 168 0 0.0
.rodata 379476 379476 0 0.0
.text 5863652 5863652 0 0.0
thermostat-no-ble arm64 (read only) 1993780 1993780 0 0.0
(read/write) 143937 143937 0 0.0
.bss 64321 64321 0 0.0
.data 880 880 0 0.0
.data.rel.ro 72000 72000 0 0.0
.dynamic 560 560 0 0.0
.got 3840 3840 0 0.0
.init 24 24 0 0.0
.init_array 288 288 0 0.0
.rodata 128196 128196 0 0.0
.text 1654112 1654112 0 0.0
mbed all-clusters-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2334104 2334104 0 0.0
.bss 189068 189068 0 0.0
.data 5264 5264 0 0.0
.text 1296680 1296680 0 0.0
lighting-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2328616 2328616 0 0.0
.bss 180896 180896 0 0.0
.data 5544 5544 0 0.0
.text 1291216 1291216 0 0.0
lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2301712 2301712 0 0.0
.bss 179944 179944 0 0.0
.data 5536 5536 0 0.0
.text 1264312 1264312 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) 2053688 2053688 0 0.0
.bss 156972 156972 0 0.0
.data 4864 4864 0 0.0
.text 1016288 1016288 0 0.0
nrfconnect lighting-app nrf52840dk_nrf52840 (read/write) 935579 935579 0 0.0
bss 118400 118400 0 0.0
rodata 108120 108120 0 0.0
text 631508 631508 0 0.0
nrf52840dk_nrf52840+rpc (read/write) 921979 921979 0 0.0
bss 115444 115444 0 0.0
rodata 101536 101536 0 0.0
text 626820 626820 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 859342 859342 0 0.0
bss 116684 116684 0 0.0
rodata 103044 103044 0 0.0
text 558948 558948 0 0.0
lock-app nrf52840dk_nrf52840 (read/write) 907723 907723 0 0.0
bss 117588 117588 0 0.0
rodata 103424 103424 0 0.0
text 609332 609332 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 831638 831638 0 0.0
bss 115900 115900 0 0.0
rodata 98388 98388 0 0.0
text 536816 536816 0 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) 912603 912603 0 0.0
bss 117496 117496 0 0.0
rodata 104768 104768 0 0.0
text 612884 612884 0 0.0
pump-controller-app nrf52840dk_nrf52840 (read/write) 905803 905803 0 0.0
bss 117376 117376 0 0.0
rodata 102896 102896 0 0.0
text 608080 608080 0 0.0
shell nrf52840dk_nrf52840 (read/write) 796079 796079 0 0.0
bss 109464 109464 0 0.0
rodata 78096 78096 0 0.0
text 532048 532048 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 708710 708710 0 0.0
bss 107352 107352 0 0.0
rodata 72396 72396 0 0.0
text 449544 449544 0 0.0
p6 all-clusters-app default (read/write) 2384624 2384656 32 0.0
.bss 117260 117260 0 0.0
.data 2544 2544 0 0.0
.text 1342888 1342920 32 0.0
light-app default (read/write) 2324008 2324008 0 0.0
.bss 106152 106152 0 0.0
.data 2384 2384 0 0.0
.text 1282272 1282272 0 0.0
lock-app default (read/write) 2296216 2296216 0 0.0
.bss 105032 105032 0 0.0
.data 2336 2336 0 0.0
.text 1254480 1254480 0 0.0
qpg lighting-app qpg6105+debug (read only) 531812 531812 0 0.0
(read/write) 146936 146936 0 0.0
.bss 86816 86816 0 0.0
.data 1004 1004 0 0.0
.text 526492 526492 0 0.0
lock-app qpg6105+debug (read only) 503492 503492 0 0.0
(read/write) 146940 146940 0 0.0
.bss 85952 85952 0 0.0
.data 952 952 0 0.0
.text 498172 498172 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) 830230 830230 0 0.0
bss 87040 87040 0 0.0
noinit 37160 37160 0 0.0
text 578418 578418 0 0.0

@bzbarsky-apple
Copy link
Contributor

Also, why wasn't this caught by yaml tests? Do we not have CI test coverage for this? We should.

bzbarsky-apple
bzbarsky-apple approved these changes Dec 17, 2021
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.

Approving just so you're not blocked, but this should really have yaml tests that exercise this stuff...

@stale
Copy link

stale bot commented Dec 24, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Dec 24, 2021
@stale
Copy link

stale bot commented Dec 31, 2021

This stale pull request has been automatically closed. Thank you for your contributions.

@stale stale bot closed this Dec 31, 2021
@bzbarsky-apple bzbarsky-apple reopened this Jan 7, 2022
@stale stale bot removed the stale Stale issue or PR label Jan 7, 2022
@github-actions
Copy link

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

PR #13089: Size comparison from 19f61eb to ef32aab

Increases (1 build for p6)
platform target config section 19f61eb ef32aab change % change
p6 all-clusters-app default (read/write) 2399760 2399776 16 0.0
.text 1358024 1358040 16 0.0
Full report (30 builds for efr32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
platform target config section 19f61eb ef32aab change % change
efr32 lighting-app BRD4161A (read only) 828916 828916 0 0.0
(read/write) 126996 126996 0 0.0
.bss 125120 125120 0 0.0
.data 1876 1876 0 0.0
.text 828908 828908 0 0.0
BRD4161A+rpc (read only) 816112 816112 0 0.0
(read/write) 143656 143656 0 0.0
.bss 141680 141680 0 0.0
.data 1976 1976 0 0.0
.text 816104 816104 0 0.0
window-app BRD4161A (read only) 802380 802380 0 0.0
(read/write) 125936 125936 0 0.0
.bss 124104 124104 0 0.0
.data 1832 1832 0 0.0
.text 802372 802372 0 0.0
k32w light k32w061+release (read/write) 655316 655316 0 0.0
.bss 76776 76776 0 0.0
.data 1848 1848 0 0.0
.text 570892 570892 0 0.0
lock k32w061+release (read/write) 659656 659656 0 0.0
.bss 77072 77072 0 0.0
.data 1868 1868 0 0.0
.text 574916 574916 0 0.0
linux chip-tool-ipv6only arm64 (read only) 7109708 7109708 0 0.0
(read/write) 327041 327041 0 0.0
.bss 54865 54865 0 0.0
.data 1096 1096 0 0.0
.data.rel.ro 209264 209264 0 0.0
.dynamic 560 560 0 0.0
.got 58136 58136 0 0.0
.init 24 24 0 0.0
.init_array 168 168 0 0.0
.rodata 389860 389860 0 0.0
.text 6022996 6022996 0 0.0
thermostat-no-ble arm64 (read only) 2031916 2031916 0 0.0
(read/write) 145137 145137 0 0.0
.bss 64657 64657 0 0.0
.data 880 880 0 0.0
.data.rel.ro 72656 72656 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 129004 129004 0 0.0
.text 1689136 1689136 0 0.0
mbed all-clusters-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2346888 2346888 0 0.0
.bss 188716 188716 0 0.0
.data 5312 5312 0 0.0
.text 1309464 1309464 0 0.0
lighting-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2330496 2330496 0 0.0
.bss 180544 180544 0 0.0
.data 5552 5552 0 0.0
.text 1293096 1293096 0 0.0
lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2303648 2303648 0 0.0
.bss 179592 179592 0 0.0
.data 5544 5544 0 0.0
.text 1266248 1266248 0 0.0
pigweed-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 1139712 1139712 0 0.0
.bss 11756 11756 0 0.0
.data 4368 4368 0 0.0
.text 103096 103096 0 0.0
shell CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2054432 2054432 0 0.0
.bss 157060 157060 0 0.0
.data 4864 4864 0 0.0
.text 1017032 1017032 0 0.0
nrfconnect lighting-app nrf52840dk_nrf52840 (read/write) 938287 938287 0 0.0
bss 119232 119232 0 0.0
rodata 108156 108156 0 0.0
text 633348 633348 0 0.0
nrf52840dk_nrf52840+rpc (read/write) 923751 923751 0 0.0
bss 116276 116276 0 0.0
rodata 100604 100604 0 0.0
text 628700 628700 0 0.0
nrf52840dongle_nrf52840 (read/write) 988963 988963 0 0.0
bss 122076 122076 0 0.0
rodata 112908 112908 0 0.0
text 665544 665544 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 848142 848142 0 0.0
bss 116020 116020 0 0.0
rodata 101328 101328 0 0.0
text 550272 550272 0 0.0
lock-app nrf52840dk_nrf52840 (read/write) 910431 910431 0 0.0
bss 118420 118420 0 0.0
rodata 103428 103428 0 0.0
text 611212 611212 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 820498 820498 0 0.0
bss 115236 115236 0 0.0
rodata 96652 96652 0 0.0
text 528176 528176 0 0.0
pigweed-app nrf52840dk_nrf52840 (read/write) 541835 541835 0 0.0
bss 52588 52588 0 0.0
rodata 50104 50104 0 0.0
text 376940 376940 0 0.0
pump-app nrf52840dk_nrf52840 (read/write) 911707 911707 0 0.0
bss 118180 118180 0 0.0
rodata 103640 103640 0 0.0
text 612424 612424 0 0.0
pump-controller-app nrf52840dk_nrf52840 (read/write) 908511 908511 0 0.0
bss 118208 118208 0 0.0
rodata 102900 102900 0 0.0
text 609960 609960 0 0.0
shell nrf52840dk_nrf52840 (read/write) 798063 798063 0 0.0
bss 109768 109768 0 0.0
rodata 78180 78180 0 0.0
text 533608 533608 0 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 710882 710882 0 0.0
bss 107656 107656 0 0.0
rodata 72484 72484 0 0.0
text 451288 451288 0 0.0
p6 all-clusters-app default (read/write) 2399760 2399776 16 0.0
.bss 116804 116804 0 0.0
.data 2592 2592 0 0.0
.text 1358024 1358040 16 0.0
light-app default (read/write) 2323744 2323744 0 0.0
.bss 105672 105672 0 0.0
.data 2384 2384 0 0.0
.text 1282008 1282008 0 0.0
lock-app default (read/write) 2295968 2295968 0 0.0
.bss 104552 104552 0 0.0
.data 2336 2336 0 0.0
.text 1254232 1254232 0 0.0
qpg lighting-app qpg6105+debug (read only) 533208 533208 0 0.0
(read/write) 146936 146936 0 0.0
.bss 86624 86624 0 0.0
.data 1004 1004 0 0.0
.text 527888 527888 0 0.0
lock-app qpg6105+debug (read only) 504984 504984 0 0.0
(read/write) 146940 146940 0 0.0
.bss 85760 85760 0 0.0
.data 952 952 0 0.0
.text 499664 499664 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) 834566 834566 0 0.0
bss 86924 86924 0 0.0
noinit 37160 37160 0 0.0
text 582774 582774 0 0.0

@mlepage-google
Copy link
Contributor Author

Just merged in master and confirmed this still works. Would like to get this in as it makes using the REPL easier, even though yes I will still continue to improve serialization to hopefully use more generated code.

@bzbarsky-apple
Copy link
Contributor

@mlepage-google I approved, but:

  1. The code is not spec-compliant. See the comments on the PR. Needs a followup to address. Please make sure that happens.
  2. Ideally needs YAML tests to ensure this actually works right. Please make sure this is tracked.

@mlepage-google
Copy link
Contributor Author

I made issue #13470 for more use of generated code in access control cluster.
I added requests for more test code in issue #10253.

@mlepage-google mlepage-google merged commit f9c4df8 into project-chip:master Jan 11, 2022
@mlepage-google mlepage-google deleted the null-lists-are-ok branch January 11, 2022 22:12
selissia pushed a commit to selissia/connectedhomeip that referenced this pull request Jan 28, 2022
@mlepage-google @selissia
Allow list fields to be null when written (project-chip#13089)
7dc74a1 
When access control cluster is decoding TLV to write entries, if
subjects or targets are explicitly null, treat them as empty lists.
step0035 pushed a commit to hank820/connectedhomeip that referenced this pull request Feb 8, 2022
@mlepage-google @step0035
Allow list fields to be null when written (project-chip#13089)
6d4e6cc 
When access control cluster is decoding TLV to write entries, if
subjects or targets are explicitly null, treat them as empty lists.
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

@andy31415 andy31415 andy31415 approved these changes

@anush-apple anush-apple Awaiting requested review from anush-apple

@austinh0 austinh0 Awaiting requested review from austinh0

@balducci-apple balducci-apple Awaiting requested review from balducci-apple

@Byungjoo-Lee Byungjoo-Lee Awaiting requested review from Byungjoo-Lee

@carol-apple carol-apple Awaiting requested review from carol-apple

@cecille cecille Awaiting requested review from cecille

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@chshu chshu Awaiting requested review from chshu

@chulspro chulspro Awaiting requested review from chulspro

@Damian-Nordic Damian-Nordic Awaiting requested review from Damian-Nordic

@dhrishi dhrishi Awaiting requested review from dhrishi

@electrocucaracha electrocucaracha Awaiting requested review from electrocucaracha

@emargolis emargolis Awaiting requested review from emargolis

@franck-apple franck-apple Awaiting requested review from franck-apple

@gjc13 gjc13 Awaiting requested review from gjc13

@hawk248 hawk248 Awaiting requested review from hawk248

@jelderton jelderton Awaiting requested review from jelderton

@jepenven-silabs jepenven-silabs Awaiting requested review from jepenven-silabs

@jmartinez-silabs jmartinez-silabs Awaiting requested review from jmartinez-silabs

@kghost kghost Awaiting requested review from kghost

@LuDuda LuDuda Awaiting requested review from LuDuda

@lzgrablic02 lzgrablic02 Awaiting requested review from lzgrablic02

@msandstedt msandstedt Awaiting requested review from msandstedt

@pan-apple pan-apple Awaiting requested review from pan-apple

@robszewczyk robszewczyk Awaiting requested review from robszewczyk

@sagar-apple sagar-apple Awaiting requested review from sagar-apple

@saurabhst saurabhst Awaiting requested review from saurabhst

@selissia selissia Awaiting requested review from selissia

@tcarmelveilleux tcarmelveilleux Awaiting requested review from tcarmelveilleux

@tecimovic tecimovic Awaiting requested review from tecimovic

@turon turon Awaiting requested review from turon

@vijs vijs Awaiting requested review from vijs

@vivien-apple vivien-apple Awaiting requested review from vivien-apple

@wbschiller wbschiller Awaiting requested review from wbschiller

@woody-apple woody-apple Awaiting requested review from woody-apple

@xylophone21 xylophone21 Awaiting requested review from xylophone21

@yufengwangca yufengwangca Awaiting requested review from yufengwangca

@yunhanw-google yunhanw-google Awaiting requested review from yunhanw-google

Assignees
No one assigned
Labels
app review - approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mlepage-google @bzbarsky-apple @andy31415