connectedhomeip icon indicating copy to clipboard operation
connectedhomeip copied to clipboard

Published 20 hours ago • verified publisher icon project-chip

[v1.0 cherrypick] change for on off transitiontime feature (#21569)

Open Jerry-ESP opened this issue 2 years ago • 9 comments

#21569

  • change for on off transitiontime feature

  • Fix subscription test to handle separate reports for different attributes better.

Co-authored-by: Restyled.io [email protected] Co-authored-by: Boris Zbarsky [email protected]

Jerry-ESP avatar Oct 13 '22 02:10 Jerry-ESP

PR #23158: Size comparison from 37d5bb947d9324b8b19df332917416fe7e127bac to 4e024af71c847ae52776b488a8af019d64dda591

Increases (9 builds for bl702, cc13x2_26x2, esp32, nrfconnect, psoc6)
platform target config section 37d5bb94 4e024af7 change % change
bl702 lighting-app bl702 .debug_line 5256312 5256320 8 0.0
bl702+rpc .debug_line 5630847 5630855 8 0.0
cc13x2_26x2 pump-controller-app LP_CC2652R7 (read only) 671807 671815 8 0.0
.text 585264 585272 8 0.0
shell LP_CC2652R7 (read only) 667614 667630 16 0.0
.text 580980 580996 16 0.0
esp32 all-clusters-app m5stack (read only) 1233031 1233035 4 0.0
(read/write) 564004 564012 8 0.0
.flash.rodata 314736 314744 8 0.0
.flash.text 1227647 1227651 4 0.0
nrfconnect all-clusters-minimal-app nrf52840dk_nrf52840 text 803172 803176 4 0.0
psoc6 all-clusters cy8ckit_062s2_43012 .debug_line 3670325 3670337 12 0.0
all-clusters-minimal cy8ckit_062s2_43012 .debug_line 3691041 3691053 12 0.0
light cy8ckit_062s2_43012 .debug_line 3260959 3260965 6 0.0
Decreases (11 builds for bl602, bl702, cc13x2_26x2, esp32, psoc6, telink)
platform target config section 37d5bb94 4e024af7 change % change
bl602 lighting-app bl602 .text 1068316 1068314 -2 -0.0
bl602+rpc .text 1099664 1099662 -2 -0.0
bl702 lighting-app bl702 .debug_info 37899319 37899318 -1 -0.0
.text 957132 957130 -2 -0.0
bl702+rpc .debug_info 41805937 41805936 -1 -0.0
.text 1030838 1030836 -2 -0.0
cc13x2_26x2 pump-controller-app LP_CC2652R7 (read/write) 177712 177704 -8 -0.0
shell LP_CC2652R7 (read/write) 186232 186216 -16 -0.0
esp32 all-clusters-app c3devkit (read only) 1223012 1223010 -2 -0.0
.flash.text 1223012 1223010 -2 -0.0
psoc6 all-clusters cy8ckit_062s2_43012 .debug_info 26822263 26822262 -1 -0.0
all-clusters-minimal cy8ckit_062s2_43012 .debug_info 26559045 26559043 -2 -0.0
telink lighting-app tlsr9518adk80d text 592828 592824 -4 -0.0
ota-requestor-app tlsr9518adk80d text 599014 599012 -2 -0.0
Full report (37 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
platform target config section 37d5bb94 4e024af7 change % change
bl602 lighting-app bl602 (read/write) 1388550 1388550 0 0.0
.bss 90529 90529 0 0.0
.data 9936 9936 0 0.0
.text 1068316 1068314 -2 -0.0
bl602+rpc (read/write) 1433762 1433762 0 0.0
.bss 97961 97961 0 0.0
.data 10320 10320 0 0.0
.text 1099664 1099662 -2 -0.0
bl702 lighting-app bl702 (read only) 3262 3262 0 0.0
(read/write) 1188539 1188539 0 0.0
.bleromro 6296 6296 0 0.0
.bleromrw 124 124 0 0.0
.boot2 688 688 0 0.0
.bss 67006 67006 0 0.0
.bss_psram 29696 29696 0 0.0
.comment 48 48 0 0.0
.data 4280 4280 0 0.0
.debug_abbrev 1506948 1506948 0 0.0
.debug_aranges 133168 133168 0 0.0
.debug_frame 486676 486676 0 0.0
.debug_info 37899319 37899318 -1 -0.0
.debug_line 5256312 5256320 8 0.0
.debug_loc 3363591 3363591 0 0.0
.debug_ranges 359936 359936 0 0.0
.debug_str 3456569 3456569 0 0.0
.hbn 509 509 0 0.0
.hbn_noinit 260 260 0 0.0
.init 342 342 0 0.0
.init_array 144 144 0 0.0
.psram 0 0 0 0.0
.riscv.attributes 47 47 0 0.0
.rodata 116616 116616 0 0.0
.rsvd 3188 3188 0 0.0
.shstrtab 293 293 0 0.0
.stack 2048 2048 0 0.0
.strtab 565170 565170 0 0.0
.symtab 171856 171856 0 0.0
.tcm_data 36 36 0 0.0
.tcmcode 3262 3262 0 0.0
.text 0 0 0 0.0
957132 957130 -2 -0.0
bl702+rpc (read only) 3262 3262 0 0.0
(read/write) 1284451 1284451 0 0.0
.bleromro 6296 6296 0 0.0
.bleromrw 124 124 0 0.0
.boot2 688 688 0 0.0
.bss 75038 75038 0 0.0
.bss_psram 29936 29936 0 0.0
.comment 48 48 0 0.0
.data 4800 4800 0 0.0
.debug_abbrev 1644527 1644527 0 0.0
.debug_aranges 140672 140672 0 0.0
.debug_frame 512052 512052 0 0.0
.debug_info 41805937 41805936 -1 -0.0
.debug_line 5630847 5630855 8 0.0
.debug_loc 3556271 3556271 0 0.0
.debug_ranges 382392 382392 0 0.0
.debug_str 3852536 3852536 0 0.0
.hbn 509 509 0 0.0
.hbn_noinit 260 260 0 0.0
.init 342 342 0 0.0
.init_array 160 160 0 0.0
.psram 0 0 0 0.0
.riscv.attributes 47 47 0 0.0
.rodata 130008 130008 0 0.0
.rsvd 3188 3188 0 0.0
.shstrtab 293 293 0 0.0
.stack 2048 2048 0 0.0
.strtab 624343 624343 0 0.0
.symtab 189664 189664 0 0.0
.tcm_data 36 36 0 0.0
.tcmcode 3262 3262 0 0.0
.text 0 0 0 0.0
1030838 1030836 -2 -0.0
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read only) 676619 676619 0 0.0
(read/write) 174916 174916 0 0.0
.bss 81228 81228 0 0.0
.data 3380 3380 0 0.0
.rodata 89603 89603 0 0.0
.text 586704 586704 0 0.0
all-clusters-minimal-app LP_CC2652R7 (read only) 640859 640859 0 0.0
(read/write) 157996 157996 0 0.0
.bss 80500 80500 0 0.0
.data 3380 3380 0 0.0
.rodata 78739 78739 0 0.0
.text 561800 561800 0 0.0
lock-ftd LP_CC2652R7 (read only) 678143 678143 0 0.0
(read/write) 170560 170560 0 0.0
.bss 78484 78484 0 0.0
.data 3304 3304 0 0.0
.rodata 77287 77287 0 0.0
.text 600376 600376 0 0.0
lock-mtd LP_CC2652R7 (read only) 661971 661971 0 0.0
(read/write) 182420 182420 0 0.0
.bss 74172 74172 0 0.0
.data 3304 3304 0 0.0
.rodata 103123 103123 0 0.0
.text 558368 558368 0 0.0
pump-app LP_CC2652R7 (read only) 687307 687307 0 0.0
(read/write) 162100 162100 0 0.0
.bss 78420 78420 0 0.0
.data 3296 3296 0 0.0
.rodata 90507 90507 0 0.0
.text 596316 596316 0 0.0
pump-controller-app LP_CC2652R7 (read only) 671807 671815 8 0.0
(read/write) 177712 177704 -8 -0.0
.bss 78532 78532 0 0.0
.data 3292 3292 0 0.0
.rodata 86063 86063 0 0.0
.text 585264 585272 8 0.0
shell LP_CC2652R7 (read only) 667614 667630 16 0.0
(read/write) 186232 186216 -16 -0.0
.bss 83540 83540 0 0.0
.data 3376 3376 0 0.0
.rodata 86318 86318 0 0.0
.text 580980 580996 16 0.0
cyw30739 light cyw930739m2evb_01 (read/write) 587586 587586 0 0.0
.app_xip_area 464212 464212 0 0.0
.bss 65792 65792 0 0.0
.data 760 760 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
lock cyw930739m2evb_01 (read/write) 594634 594634 0 0.0
.app_xip_area 465932 465932 0 0.0
.bss 71112 71112 0 0.0
.data 768 768 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
ota-requestor-no-progress-logging cyw930739m2evb_01 (read/write) 543322 543322 0 0.0
.app_xip_area 425004 425004 0 0.0
.bss 60784 60784 0 0.0
.data 716 716 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
efr32 lighting-app BRD4161A (read/write) 1109624 1109624 0 0.0
.bss 136340 136340 0 0.0
.data 2072 2072 0 0.0
.text 971192 971192 0 0.0
BRD4161A+rpc (read/write) 973204 973204 0 0.0
.bss 150852 150852 0 0.0
.data 2252 2252 0 0.0
.text 820080 820080 0 0.0
BRD4161A+rs911x (read/write) 1002920 1002920 0 0.0
.bss 169176 169176 0 0.0
.data 2064 2064 0 0.0
.text 831660 831660 0 0.0
lock-app BRD4161A+wf200 (read/write) 1150760 1150760 0 0.0
.bss 152264 152264 0 0.0
.data 2072 2072 0 0.0
.text 996404 996404 0 0.0
window-app BRD4161A (read/write) 1103220 1103220 0 0.0
.bss 137788 137788 0 0.0
.data 2096 2096 0 0.0
.text 963316 963316 0 0.0
esp32 all-clusters-app c3devkit (read only) 1223012 1223010 -2 -0.0
(read/write) 1788110 1788110 0 0.0
.dram0.bss 76944 76944 0 0.0
.dram0.data 13840 13840 0 0.0
.flash.rodata 257680 257680 0 0.0
.flash.text 1223012 1223010 -2 -0.0
.iram0.text 65204 65204 0 0.0
m5stack (read only) 1233031 1233035 4 0.0
(read/write) 564004 564012 8 0.0
.dram0.bss 82304 82304 0 0.0
.dram0.data 34296 34296 0 0.0
.flash.rodata 314736 314744 8 0.0
.flash.text 1227647 1227651 4 0.0
.iram0.text 123939 123939 0 0.0
k32w light k32w0+release (read/write) 641416 641416 0 0.0
.bss 74816 74816 0 0.0
.data 2064 2064 0 0.0
.text 561808 561808 0 0.0
lock k32w0+release (read/write) 635544 635544 0 0.0
.bss 75600 75600 0 0.0
.data 2080 2080 0 0.0
.text 555136 555136 0 0.0
linux chip-tool-ipv6only arm64 (read only) 10359148 10359148 0 0.0
(read/write) 706305 706305 0 0.0
.bss 33953 33953 0 0.0
.data 2768 2768 0 0.0
.data.rel.ro 650584 650584 0 0.0
.dynamic 560 560 0 0.0
.got 13904 13904 0 0.0
.init 24 24 0 0.0
.init_array 208 208 0 0.0
.rodata 504364 504364 0 0.0
.text 8199284 8199284 0 0.0
thermostat-no-ble arm64 (read only) 2388692 2388692 0 0.0
(read/write) 143585 143585 0 0.0
.bss 55361 55361 0 0.0
.data 1816 1816 0 0.0
.data.rel.ro 77208 77208 0 0.0
.dynamic 560 560 0 0.0
.got 5184 5184 0 0.0
.init 24 24 0 0.0
.init_array 440 440 0 0.0
.rodata 144124 144124 0 0.0
.text 2001728 2001728 0 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2455640 2455640 0 0.0
.bss 215044 215044 0 0.0
.data 5872 5872 0 0.0
.text 1418284 1418284 0 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read/write) 1182083 1182083 0 0.0
bss 143633 143633 0 0.0
rodata 144196 144196 0 0.0
text 815304 815304 0 0.0
all-clusters-minimal-app nrf52840dk_nrf52840 (read/write) 1160735 1160735 0 0.0
bss 142860 142860 0 0.0
rodata 135768 135768 0 0.0
text 803172 803176 4 0.0
psoc6 all-clusters cy8ckit_062s2_43012 (read only) 841968 841968 0 0.0
(read/write) 1744332 1744332 0 0.0
.ARM.attributes 46 46 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 188712 188712 0 0.0
.comment 204 204 0 0.0
.copy.table 24 24 0 0.0
.cy_m0p_image 6216 6216 0 0.0
.cy_sharedmem 8 8 0 0.0
.data 2664 2664 0 0.0
.debug_abbrev 1229274 1229274 0 0.0
.debug_aranges 111840 111840 0 0.0
.debug_frame 373436 373436 0 0.0
.debug_info 26822263 26822262 -1 -0.0
.debug_line 3670325 3670337 12 0.0
.debug_loc 3583707 3583707 0 0.0
.debug_ranges 340200 340200 0 0.0
.debug_str 3440007 3440007 0 0.0
.heap 841968 841968 0 0.0
.noinit 148 148 0 0.0
.ramVectors 736 736 0 0.0
.shstrtab 288 288 0 0.0
.stab 156 156 0 0.0
.stabstr 335 335 0 0.0
.stack_dummy 4096 4096 0 0.0
.strtab 569581 569581 0 0.0
.symtab 421104 421104 0 0.0
.text 1544568 1544568 0 0.0
.zero.table 8 8 0 0.0
text 0 0 0 0.0
all-clusters-minimal cy8ckit_062s2_43012 (read only) 842704 842704 0 0.0
(read/write) 1686932 1686932 0 0.0
.ARM.attributes 46 46 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 187976 187976 0 0.0
.comment 204 204 0 0.0
.copy.table 24 24 0 0.0
.cy_m0p_image 6216 6216 0 0.0
.cy_sharedmem 8 8 0 0.0
.data 2664 2664 0 0.0
.debug_abbrev 1221073 1221073 0 0.0
.debug_aranges 111312 111312 0 0.0
.debug_frame 376516 376516 0 0.0
.debug_info 26559045 26559043 -2 -0.0
.debug_line 3691041 3691053 12 0.0
.debug_loc 3571344 3571344 0 0.0
.debug_ranges 338816 338816 0 0.0
.debug_str 3429020 3429020 0 0.0
.heap 842704 842704 0 0.0
.noinit 148 148 0 0.0
.ramVectors 736 736 0 0.0
.shstrtab 288 288 0 0.0
.stab 156 156 0 0.0
.stabstr 335 335 0 0.0
.stack_dummy 4096 4096 0 0.0
.strtab 533670 533670 0 0.0
.symtab 407536 407536 0 0.0
.text 1487904 1487904 0 0.0
.zero.table 0 0 0 0.0
8 8 0 0.0
light cy8ckit_062s2_43012 (read only) 850896 850896 0 0.0
(read/write) 1605476 1605476 0 0.0
.ARM.attributes 46 46 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 179992 179992 0 0.0
.comment 204 204 0 0.0
.copy.table 24 24 0 0.0
.cy_m0p_image 6216 6216 0 0.0
.cy_sharedmem 8 8 0 0.0
.data 2456 2456 0 0.0
.debug_abbrev 1055129 1055129 0 0.0
.debug_aranges 103520 103520 0 0.0
.debug_frame 346844 346844 0 0.0
.debug_info 22023240 22023240 0 0.0
.debug_line 3260959 3260965 6 0.0
.debug_loc 3269410 3269410 0 0.0
.debug_ranges 304144 304144 0 0.0
.debug_str 3234552 3234552 0 0.0
.heap 850896 850896 0 0.0
.noinit 148 148 0 0.0
.ramVectors 736 736 0 0.0
.shstrtab 288 288 0 0.0
.stab 156 156 0 0.0
.stabstr 335 335 0 0.0
.stack_dummy 4096 4096 0 0.0
.strtab 470047 470047 0 0.0
.symtab 375984 375984 0 0.0
.text 1414640 1414640 0 0.0
.zero.table 0 0 0 0.0
8 8 0 0.0
lock cy8ckit_062s2_43012 (read only) 845864 845864 0 0.0
(read/write) 1643340 1643340 0 0.0
.ARM.attributes 46 46 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 185008 185008 0 0.0
.comment 204 204 0 0.0
.copy.table 24 24 0 0.0
.cy_m0p_image 6216 6216 0 0.0
.cy_sharedmem 8 8 0 0.0
.data 2472 2472 0 0.0
.debug_abbrev 1062548 1062548 0 0.0
.debug_aranges 104192 104192 0 0.0
.debug_frame 349668 349668 0 0.0
.debug_info 22402592 22402592 0 0.0
.debug_line 3269675 3269675 0 0.0
.debug_loc 3309282 3309282 0 0.0
.debug_ranges 307488 307488 0 0.0
.debug_str 3262007 3262007 0 0.0
.heap 845864 845864 0 0.0
.noinit 148 148 0 0.0
.ramVectors 736 736 0 0.0
.shstrtab 288 288 0 0.0
.stab 156 156 0 0.0
.stabstr 335 335 0 0.0
.stack_dummy 4096 4096 0 0.0
.strtab 476287 476287 0 0.0
.symtab 379216 379216 0 0.0
.text 1447472 1447472 0 0.0
.zero.table 0 0 0 0.0
8 8 0 0.0
qpg lighting-app qpg6105+debug (read/write) 1148140 1148140 0 0.0
.bss 110348 110348 0 0.0
.data 832 832 0 0.0
.text 595240 595240 0 0.0
lock-app qpg6105+debug (read/write) 1116060 1116060 0 0.0
.bss 106172 106172 0 0.0
.data 836 836 0 0.0
.text 563156 563156 0 0.0
telink light-switch-app tlsr9518adk80d (read/write) 813708 813708 0 0.0
bss 71372 71372 0 0.0
noinit 43488 43488 0 0.0
text 574610 574610 0 0.0
lighting-app tlsr9518adk80d (read/write) 835812 835812 0 0.0
bss 72228 72228 0 0.0
noinit 43488 43488 0 0.0
text 592828 592824 -4 -0.0
ota-requestor-app tlsr9518adk80d (read/write) 843772 843772 0 0.0
bss 73136 73136 0 0.0
noinit 43488 43488 0 0.0
text 599014 599012 -2 -0.0

github-actions[bot] avatar Oct 13 '22 03:10 github-actions[bot]

@Jerry-ESP - we generally try to keep v1.0 reasonably stable in terms of functionality. We allow example changes and platform changes as those have less chances of ripple effects and also allow significant bugs fixed (e.g. we had some storage leak issues).

This PR does not seem to fit exactly in any of the above categories. What is the side-effect if this is NOT merged into 1.0?

andy31415 avatar Oct 13 '22 15:10 andy31415

@Jerry-ESP - we generally try to keep v1.0 reasonably stable in terms of functionality. We allow example changes and platform changes as those have less chances of ripple effects and also allow significant bugs fixed (e.g. we had some storage leak issues).

This PR does not seem to fit exactly in any of the above categories. What is the side-effect if this is NOT merged into 1.0?

@andy31415 Without the fix, the OnOffTransitionTime feature won't work for 1.0 Matter products. The current implementation just passes the certification test, but the Off fade-out feature does not work as expect, the off command is triggered immediately regardless of the transition time.

The fix is requested by a device maker, who implements the OnOffTransitionTime feature in Matter 1.0 lighting products. It would be good to have in 1.0 branch, as long as it passes the CI, and no side effects.

chshu avatar Oct 14 '22 11:10 chshu

@chshu It's still not obvious to me that delaying the change to the "OnOff" attribute is spec-compliant here. Neither "1.6.4.1.1. Effect of On/Off Commands on the CurrentLevel Attribute" nor "1.6.5.9. OnOffTransitionTime Attribute" talks about doing that.

As far as I can tell what the spec requires is:

  1. The OnOff attribute becomes false.
  2. The CurrentLevel evolves over time.
  3. What happens to the physical light in the process is not really clearly defined, but application logic could prevent it from actually going off until the transition time elapses, without core changes.

Has a spec issue been raised about changing item 1 here, as this PR does?

bzbarsky-apple avatar Oct 14 '22 13:10 bzbarsky-apple

@bzbarsky-apple The OnOffTransitionTime feature is defined clearly in the spec, but it doesn't specify what should be done by the SDK, and what else should be done by Application. I think it's more like a code implementation tadeoff, rather than spec issue.

From the application perspective, it's straightforward to map any attribute change to physical directly in the callbacks. If the SDK doesn't delay the change to the "OnOff" attribute, the additional logic has to be added to all applications that support OnOffTransitionTime (each developer has to implement it once). Implementing the logic in the SDK seems more reasonable, but understand the status of v1.0_branch, we'd leave the decision to maintainers on whether cherry-pick it or not.

Another related question may worth discuss: when should the On/Off change report be sent to the subscriber? If the OffTransition is still on going, should it show Off status on the Phone APPs/Hubs, or wait until the OffTransition is done?

chshu avatar Oct 17 '22 11:10 chshu

The OnOffTransitionTime feature is defined clearly in the spec

Where in the spec does it clearly say that "OnOff" becoming false is deferred? @chshu

bzbarsky-apple avatar Oct 18 '22 03:10 bzbarsky-apple

The OnOffTransitionTime feature is defined clearly in the spec

Where in the spec does it clearly say that "OnOff" becoming false is deferred? @chshu

The OnOffTransitionTime feature is clear that the device needs to implement the fade-in/fade-out logic, but agree that the Spec doesn't mention about deferring OnOff attribute change, I thought the Spec leaves it to the code implementation, either the SDK or the Applicaiton has to do the deferring. The PR just adds the defferring logic in SDK, to make it easier for developers.

I will file a Spec issue for further discussion. Spec issue: https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/5677

chshu avatar Oct 18 '22 12:10 chshu

I thought the Spec leaves it to the code implementation

The spec for the "Off" command says:

On receipt of the Off command, a server SHALL set the OnOff attribute to FALSE.

How is that "left to code implementation"?

The fact is, the code changed involved is violating that SHALL requirement, as far as I can tell....

bzbarsky-apple avatar Oct 18 '22 14:10 bzbarsky-apple

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[bot] avatar Dec 20 '22 17:12 stale[bot]

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

stale[bot] avatar Jan 20 '23 07:01 stale[bot]