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 RemoveFabric responses to follow the spec. #14688

Merged
merged 2 commits into from
Feb 3, 2022
Merged

Fix RemoveFabric responses to follow the spec. #14688

merged 2 commits into from
Feb 3, 2022

Conversation

bzbarsky-apple
Copy link
Contributor

It should be returning a NOCResponse, not a status.

Problem

Incorrect response means the client can't parse it.

Change overview

  1. Fix up some of the cluster XML to better match the spec.
  2. Fix the response from RemoveFabric to be a NOCResponse, especially on success.

Testing

Added a YAML test for removing a nonexistent fabric. @mrjerryjohns was going to see if he can add a cirque test for removing an existing one.

@bzbarsky-apple
Fix RemoveFabric to follow the spec.
04c6d82 
It should be returning a NOCResponse, not a status.
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

PR #14688: Size comparison from 63a5c52 to 04c6d82

Increases (30 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
platform target config section 63a5c52 04c6d82 change % change
cyw30739 light cyw930739m2evb_01 (read/write) 577526 577770 244 0.0
.app_xip_area 484756 485000 244 0.1
lock cyw930739m2evb_01 (read/write) 535150 535390 240 0.0
.app_xip_area 443948 444188 240 0.1
ota-requestor cyw930739m2evb_01 (read/write) 559706 559902 196 0.0
.app_xip_area 459192 459388 196 0.0
efr32 lighting-app BRD4161A (read only) 843824 844128 304 0.0
.text 843816 844120 304 0.0
window-app BRD4161A (read only) 816076 816364 288 0.0
.text 816068 816356 288 0.0
lighting-app BRD4161A+rpc (read only) 831200 831504 304 0.0
.text 831192 831496 304 0.0
esp32 all-clusters-app c3devkit (read only) 934022 934166 144 0.0
.flash.text 934022 934166 144 0.0
m5stack (read only) 982071 982187 116 0.0
.flash.text 976687 976803 116 0.0
k32w light k32w061+release (read/write) 661708 661900 192 0.0
.text 578844 579036 192 0.0
lock k32w061+release (read/write) 662892 663116 224 0.0
.text 579668 579892 224 0.0
linux chip-tool-ipv6only arm64 (read only) 7088532 7094068 5536 0.1
(read/write) 290881 291041 160 0.1
.data.rel.ro 186160 186296 136 0.1
.got 45232 45264 32 0.1
.rodata 390460 390508 48 0.0
.text 6097780 6102900 5120 0.1
thermostat-no-ble arm64 (read only) 2086380 2088092 1712 0.1
.rodata 131004 131084 80 0.1
.text 1737648 1739280 1632 0.1
mbed all-clusters-app CY8CPROTO_062_4343W+release (read/write) 2385832 2385960 128 0.0
.text 1348432 1348560 128 0.0
lighting-app CY8CPROTO_062_4343W+release (read/write) 2338640 2338832 192 0.0
.text 1301240 1301432 192 0.0
lock-app CY8CPROTO_062_4343W+release (read/write) 2302296 2302552 256 0.0
.text 1264896 1265152 256 0.0
shell CY8CPROTO_062_4343W+release (read/write) 2292804 2292996 192 0.0
.text 1255376 1255568 192 0.0
nrfconnect lighting-app nrf52840dk_nrf52840 (read/write) 980927 981119 192 0.0
text 668068 668260 192 0.0
lock-app nrf52840dk_nrf52840 (read/write) 913143 913383 240 0.0
text 614068 614296 228 0.0
pump-app nrf52840dk_nrf52840 (read/write) 915927 916167 240 0.0
text 616524 616752 228 0.0
pump-controller-app nrf52840dk_nrf52840 (read/write) 911135 911359 224 0.0
text 612576 612804 228 0.0
lighting-app nrf52840dk_nrf52840+rpc (read/write) 966487 966679 192 0.0
text 664508 664704 196 0.0
nrf52840dongle_nrf52840 (read/write) 996995 997219 224 0.0
text 673460 673688 228 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 890126 890318 192 0.0
text 584256 584452 196 0.0
lock-app nrf5340dk_nrf5340_cpuapp (read/write) 823210 823434 224 0.0
text 531040 531268 228 0.0
p6 all-clusters-app default (read/write) 2446944 2447168 224 0.0
.text 1405208 1405432 224 0.0
light-app default (read/write) 2340536 2340840 304 0.0
.text 1298800 1299104 304 0.0
lock-app default (read/write) 2305560 2305848 288 0.0
.text 1263824 1264112 288 0.0
qpg lighting-app qpg6105+debug (read only) 571104 571304 200 0.0
.text 565784 565984 200 0.0
lock-app qpg6105+debug (read only) 516840 517056 216 0.0
.text 511520 511736 216 0.0
telink lighting-app tlsr9518adk80d (read/write) 845494 845686 192 0.0
text 592264 592460 196 0.0
Full report (34 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
platform target config section 63a5c52 04c6d82 change % change
cyw30739 light cyw930739m2evb_01 (read/write) 577526 577770 244 0.0
.app_xip_area 484756 485000 244 0.1
.bss 75516 75516 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) 535150 535390 240 0.0
.app_xip_area 443948 444188 240 0.1
.bss 73988 73988 0 0.0
.data 560 560 0 0.0
.rodata 0 0 0 0.0
.text 0 0 0 0.0
ota-requestor cyw930739m2evb_01 (read/write) 559706 559902 196 0.0
.app_xip_area 459192 459388 196 0.0
.bss 82972 82972 0 0.0
.data 504 504 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
efr32 lighting-app BRD4161A (read only) 843824 844128 304 0.0
(read/write) 125056 125056 0 0.0
.bss 123160 123160 0 0.0
.data 1892 1892 0 0.0
.text 843816 844120 304 0.0
window-app BRD4161A (read only) 816076 816364 288 0.0
(read/write) 123684 123684 0 0.0
.bss 121836 121836 0 0.0
.data 1848 1848 0 0.0
.text 816068 816356 288 0.0
lighting-app BRD4161A+rpc (read only) 831200 831504 304 0.0
(read/write) 141712 141712 0 0.0
.bss 139720 139720 0 0.0
.data 1992 1992 0 0.0
.text 831192 831496 304 0.0
esp32 all-clusters-app c3devkit (read only) 934022 934166 144 0.0
(read/write) 1401482 1401482 0 0.0
.dram0.bss 70320 70320 0 0.0
.dram0.data 14268 14268 0 0.0
.flash.rodata 198000 198000 0 0.0
.flash.text 934022 934166 144 0.0
.iram0.text 62056 62056 0 0.0
m5stack (read only) 982071 982187 116 0.0
(read/write) 465832 465832 0 0.0
.dram0.bss 75072 75072 0 0.0
.dram0.data 34024 34024 0 0.0
.flash.rodata 224608 224608 0 0.0
.flash.text 976687 976803 116 0.0
.iram0.text 123399 123399 0 0.0
k32w light k32w061+release (read/write) 661708 661900 192 0.0
.bss 75212 75212 0 0.0
.data 1852 1852 0 0.0
.text 578844 579036 192 0.0
lock k32w061+release (read/write) 662892 663116 224 0.0
.bss 75532 75532 0 0.0
.data 1892 1892 0 0.0
.text 579668 579892 224 0.0
linux chip-tool-ipv6only arm64 (read only) 7088532 7094068 5536 0.1
(read/write) 290881 291041 160 0.1
.bss 54577 54577 0 0.0
.data 1128 1128 0 0.0
.data.rel.ro 186160 186296 136 0.1
.dynamic 560 560 0 0.0
.got 45232 45264 32 0.1
.init 24 24 0 0.0
.init_array 184 184 0 0.0
.rodata 390460 390508 48 0.0
.text 6097780 6102900 5120 0.1
thermostat-no-ble arm64 (read only) 2086380 2088092 1712 0.1
(read/write) 148865 148865 0 0.0
.bss 66177 66177 0 0.0
.data 952 952 0 0.0
.data.rel.ro 74624 74624 0 0.0
.dynamic 560 560 0 0.0
.got 4136 4136 0 0.0
.init 24 24 0 0.0
.init_array 336 336 0 0.0
.rodata 131004 131084 80 0.1
.text 1737648 1739280 1632 0.1
mbed all-clusters-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2385832 2385960 128 0.0
.bss 189236 189236 0 0.0
.data 5288 5288 0 0.0
.text 1348432 1348560 128 0.0
lighting-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2338640 2338832 192 0.0
.bss 178152 178152 0 0.0
.data 5568 5568 0 0.0
.text 1301240 1301432 192 0.0
lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2302296 2302552 256 0.0
.bss 178032 178032 0 0.0
.data 5544 5544 0 0.0
.text 1264896 1265152 256 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) 2292804 2292996 192 0.0
.bss 175316 175316 0 0.0
.data 5368 5368 0 0.0
.text 1255376 1255568 192 0.0
nrfconnect lighting-app nrf52840dk_nrf52840 (read/write) 980927 981119 192 0.0
bss 118524 118524 0 0.0
rodata 116548 116548 0 0.0
text 668068 668260 192 0.0
lock-app nrf52840dk_nrf52840 (read/write) 913143 913383 240 0.0
bss 116884 116884 0 0.0
rodata 104804 104804 0 0.0
text 614068 614296 228 0.0
pigweed-app nrf52840dk_nrf52840 (read/write) 541779 541779 0 0.0
bss 52588 52588 0 0.0
rodata 50048 50048 0 0.0
text 376940 376940 0 0.0
pump-app nrf52840dk_nrf52840 (read/write) 915927 916167 240 0.0
bss 116640 116640 0 0.0
rodata 105324 105324 0 0.0
text 616524 616752 228 0.0
pump-controller-app nrf52840dk_nrf52840 (read/write) 911135 911359 224 0.0
bss 116648 116648 0 0.0
rodata 104428 104428 0 0.0
text 612576 612804 228 0.0
shell nrf52840dk_nrf52840 (read/write) 798379 798379 0 0.0
bss 109772 109772 0 0.0
rodata 78352 78352 0 0.0
text 533752 533752 0 0.0
lighting-app nrf52840dk_nrf52840+rpc (read/write) 966487 966679 192 0.0
bss 115568 115568 0 0.0
rodata 108020 108020 0 0.0
text 664508 664704 196 0.0
nrf52840dongle_nrf52840 (read/write) 996995 997219 224 0.0
bss 119696 119696 0 0.0
rodata 115380 115380 0 0.0
text 673460 673688 228 0.0
nrf5340dk_nrf5340_cpuapp (read/write) 890126 890318 192 0.0
bss 115312 115312 0 0.0
rodata 109824 109824 0 0.0
text 584256 584452 196 0.0
lock-app nrf5340dk_nrf5340_cpuapp (read/write) 823210 823434 224 0.0
bss 113700 113700 0 0.0
rodata 98012 98012 0 0.0
text 531040 531268 228 0.0
p6 all-clusters-app default (read/write) 2446944 2447168 224 0.0
.bss 117388 117388 0 0.0
.data 2576 2576 0 0.0
.text 1405208 1405432 224 0.0
light-app default (read/write) 2340536 2340840 304 0.0
.bss 103348 103348 0 0.0
.data 2400 2400 0 0.0
.text 1298800 1299104 304 0.0
lock-app default (read/write) 2305560 2305848 288 0.0
.bss 103068 103068 0 0.0
.data 2360 2360 0 0.0
.text 1263824 1264112 288 0.0
qpg lighting-app qpg6105+debug (read only) 571104 571304 200 0.0
(read/write) 146940 146940 0 0.0
.bss 87496 87496 0 0.0
.data 1056 1056 0 0.0
.text 565784 565984 200 0.0
lock-app qpg6105+debug (read only) 516840 517056 216 0.0
(read/write) 146940 146940 0 0.0
.bss 86936 86936 0 0.0
.data 992 992 0 0.0
.text 511520 511736 216 0.0
persistent-storage-app qpg6105+debug (read only) 107140 107140 0 0.0
(read/write) 146940 146940 0 0.0
.bss 38504 38504 0 0.0
.data 288 288 0 0.0
.text 101820 101820 0 0.0
telink lighting-app tlsr9518adk80d (read/write) 845494 845686 192 0.0
bss 85552 85552 0 0.0
noinit 37160 37160 0 0.0
text 592264 592460 196 0.0

@tcarmelveilleux tcarmelveilleux changed the title Fix RemoveFabric to follow the spec. Fix RemoveFabric responses to follow the spec. Feb 2, 2022
@andy31415 andy31415 merged commit 5c0c71b into project-chip:master Feb 3, 2022
@bzbarsky-apple bzbarsky-apple deleted the fix-remove-fabric branch February 3, 2022 21:47
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this pull request Feb 4, 2022
@bzbarsky-apple
Fix merge error causing test failures.
058b7db 
Merging project-chip#14487
violated the API contract that was established in
project-chip#14688, so we
started failing tests.  The two PRs had never been run through CI
together.
@bzbarsky-apple bzbarsky-apple mentioned this pull request Feb 4, 2022
Merged
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this pull request Feb 4, 2022
@bzbarsky-apple
Fix merge error causing test failures.
c0460b2 
Merging project-chip#14487
violated the API contract that was established in
project-chip#14688, so we
started failing tests.  The two PRs had never been run through CI
together.
bzbarsky-apple added a commit that referenced this pull request Feb 4, 2022
@bzbarsky-apple
Fix merge error causing test failures. (#14773)
1fafe32 
Merging #14487
violated the API contract that was established in
#14688, so we
started failing tests.  The two PRs had never been run through CI
together.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcarmelveilleux tcarmelveilleux tcarmelveilleux approved these changes

@andy31415 andy31415 andy31415 approved these changes

@jmartinez-silabs jmartinez-silabs jmartinez-silabs 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

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

@gjc13 gjc13 Awaiting requested review from gjc13

@harimau-qirex harimau-qirex Awaiting requested review from harimau-qirex

@hawk248 hawk248 Awaiting requested review from hawk248

@isiu-apple isiu-apple Awaiting requested review from isiu-apple

@jelderton jelderton Awaiting requested review from jelderton

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

@kghost kghost Awaiting requested review from kghost

@kpschoedel kpschoedel Awaiting requested review from kpschoedel

@LuDuda LuDuda Awaiting requested review from LuDuda

@lzgrablic02 lzgrablic02 Awaiting requested review from lzgrablic02

@mlepage-google mlepage-google Awaiting requested review from mlepage-google

@msandstedt msandstedt Awaiting requested review from msandstedt

@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

@tecimovic tecimovic Awaiting requested review from tecimovic

@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

@harsha-rajendran harsha-rajendran Awaiting requested review from harsha-rajendran

Assignees
No one assigned
Labels
app controller examples fast track review - pending
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bzbarsky-apple @tcarmelveilleux @andy31415 @jmartinez-silabs