Dont store transition val #26867

step0035 · 2023-05-26T08:41:41Z

fix [BUG] don't store intermediate value into kvs when transitioning #26866
for attributes that supports transitioning, added a isTemp argument in their Set function
if the transition is not completed yet, the attribute value is temporary and don't store it into kvs
only store to kvs when the transition is completed
for all other attributes, set isTemp to false

Note

i only added this for levelcontrol and colorcontrol attributes, not sure about other clusters, please help to add it in if i missed out any

…ansitioning

CLAassistant · 2023-05-26T08:41:46Z

All committers have signed the CLA.

github-actions · 2023-05-26T08:56:18Z

PR #26867: Size comparison from ad5253a to 5e3e2a9

Increases (1 build for cc32xx)

platform	target	config	section	`ad5253a`	`5e3e2a9`	change
cc32xx	lock	CC3235SF_LAUNCHXL	(read only)	605842	605874	32
			.debug_info	20539281	20545386	6105
			.debug_line	`2715330`	`2715362`	32
			.debug_loclists	1532678	1532977	299
			.debug_rnglists	96806	96814	8
			.debug_str	`3266523`	3266540	17
			.strtab	484213	484215	2
			.text	499320	499352	32

Full report (1 build for cc32xx)

platform	target	config	section	`ad5253a`	`5e3e2a9`	change
cc32xx	lock	CC3235SF_LAUNCHXL		0	0	0
			(read only)	605842	605874	32
			(read/write)	204164	204164	0
			.ARM.attributes	44	44	0
			.ARM.exidx	8	8	0
			.bss	197576	197576	0
			.comment	206	206	0
			.data	1468	1468	0
			.debug_abbrev	955071	955071	0
			.debug_aranges	104360	104360	0
			.debug_frame	353300	353300	0
			.debug_info	20539281	20545386	6105
			.debug_line	`2715330`	`2715362`	32
			.debug_line_str	513	513	0
			.debug_loc	33340	33340	0
			.debug_loclists	1532678	1532977	299
			.debug_ranges	4984	4984	0
			.debug_rnglists	96806	96814	8
			.debug_str	`3266523`	3266540	17
			.ramVecs	780	780	0
			.resetVecs	64	64	0
			.rodata	104402	104402	0
			.shstrtab	265	265	0
			.stack	2048	2048	0
			.strtab	484213	484215	2
			.symtab	287456	287456	0
			.text	499320	499352	32

github-actions · 2023-05-26T09:25:58Z

PR #26867: Size comparison from ad5253a to bdfa143

Increases (16 builds for cc13x2_26x2, cc13x4_26x4, cc32xx, k32w, mbed, nrfconnect)

platform	target	config	section	`ad5253a`	`bdfa143`	change	% change
cc13x2_26x2	lock-mtd	LP_CC2652R7	(read only)	658123	658187	64	0.0
			.text	580516	580580	64	0.0
	pump-app	LP_CC2652R7	(read only)	648839	649087	248	0.0
			.rodata	79695	79735	40	0.1
			.text	568656	568864	208	0.0
	pump-controller-app	LP_CC2652R7	(read only)	634343	634383	40	0.0
			.text	558280	558320	40	0.0
cc13x4_26x4	lighting-app	LP_EM_CC1354P10_6	(read only)	747243	747491	248	0.0
			.rodata	79671	79711	40	0.1
			.text	667184	667392	208	0.0
	lock-ftd	LP_EM_CC1354P10_6	(read only)	739279	739327	48	0.0
			.text	662532	662580	48	0.0
	lock-mtd	LP_EM_CC1354P10_6	(read only)	727411	727459	48	0.0
			.text	624704	624752	48	0.0
	pump-app	LP_EM_CC1354P10_6	(read only)	680819	681035	216	0.0
			.rodata	76879	76919	40	0.1
			.text	603548	603724	176	0.0
	pump-controller-app	LP_EM_CC1354P10_6	(read only)	666267	666299	32	0.0
			.text	593148	593180	32	0.0
cc32xx	lock	CC3235SF_LAUNCHXL	(read only)	605842	605874	32	0.0
			.debug_info	20539281	20545386	6105	0.0
			.debug_line	`2715330`	`2715362`	32	0.0
			.debug_loclists	1532678	1532977	299	0.0
			.debug_rnglists	96806	96814	8	0.0
			.debug_str	`3266523`	3266540	17	0.0
			.strtab	484213	484215	2	0.0
			.text	499320	499352	32	0.0
k32w	contact	k32w0+release	(read only)	583644	583708	64	0.0
			.text	583108	583172	64	0.0
	light	k32w0+release	(read only)	582588	582716	128	0.0
			.text	582052	582180	128	0.0
	lock	k32w0+release	(read only)	556076	556108	32	0.0
			.text	555540	555572	32	0.0
mbed	lock-app	CY8CPROTO_062_4343W+release	(read/write)	`2495408`	`2495472`	64	0.0
			.text	1458092	1458156	64	0.0
nrfconnect	all-clusters-app	nrf52840dk_nrf52840	(read/write)	1178072	1178472	400	0.0
			rodata	133096	133128	32	0.0
			text	808648	809016	368	0.0
		nrf7002dk_nrf5340_cpuapp	(read/write)	`1437816`	`1438200`	384	0.0
			rodata	229112	229144	32	0.0
			text	779252	779612	360	0.0
	all-clusters-minimal-app	nrf52840dk_nrf52840	(read/write)	1124288	1124644	356	0.0
			rodata	109856	109892	36	0.0
			text	778944	779264	320	0.0

Full report (16 builds for cc13x2_26x2, cc13x4_26x4, cc32xx, k32w, mbed, nrfconnect)

platform	target	config	section	`ad5253a`	`bdfa143`	change	% change
cc13x2_26x2	lock-mtd	LP_CC2652R7	(read only)	658123	658187	64	0.0
			(read/write)	158713	158713	0	0.0
			.bss	80376	80376	0	0.0
			.data	3272	3272	0	0.0
			.rodata	77115	77115	0	0.0
			.text	580516	580580	64	0.0
	pump-app	LP_CC2652R7	(read only)	648839	649087	248	0.0
			(read/write)	153177	153177	0	0.0
			.bss	74600	74600	0	0.0
			.data	3264	3264	0	0.0
			.rodata	79695	79735	40	0.1
			.text	568656	568864	208	0.0
	pump-controller-app	LP_CC2652R7	(read only)	634343	634383	40	0.0
			(read/write)	153317	153317	0	0.0
			.bss	74744	74744	0	0.0
			.data	3260	3260	0	0.0
			.rodata	75575	75575	0	0.0
			.text	558280	558320	40	0.0
cc13x4_26x4	lighting-app	LP_EM_CC1354P10_6	(read only)	747243	747491	248	0.0
			(read/write)	170152	170152	0	0.0
			.bss	92488	92488	0	0.0
			.data	3464	3464	0	0.0
			.rodata	79671	79711	40	0.1
			.text	667184	667392	208	0.0
	lock-ftd	LP_EM_CC1354P10_6	(read only)	739279	739327	48	0.0
			(read/write)	175372	175372	0	0.0
			.bss	97712	97712	0	0.0
			.data	3460	3460	0	0.0
			.rodata	76355	76355	0	0.0
			.text	662532	662580	48	0.0
	lock-mtd	LP_EM_CC1354P10_6	(read only)	727411	727459	48	0.0
			(read/write)	169980	169980	0	0.0
			.bss	92320	92320	0	0.0
			.data	3460	3460	0	0.0
			.rodata	102319	102319	0	0.0
			.text	624704	624752	48	0.0
	pump-app	LP_EM_CC1354P10_6	(read only)	680819	681035	216	0.0
			(read/write)	164444	164444	0	0.0
			.bss	86544	86544	0	0.0
			.data	3452	3452	0	0.0
			.rodata	76879	76919	40	0.1
			.text	603548	603724	176	0.0
	pump-controller-app	LP_EM_CC1354P10_6	(read only)	666267	666299	32	0.0
			(read/write)	164576	164576	0	0.0
			.bss	86688	86688	0	0.0
			.data	3448	3448	0	0.0
			.rodata	72727	72727	0	0.0
			.text	593148	593180	32	0.0
cc32xx	lock	CC3235SF_LAUNCHXL	(blank)	0	0	0	0.0
			(read only)	605842	605874	32	0.0
			(read/write)	204164	204164	0	0.0
			.ARM.attributes	44	44	0	0.0
			.ARM.exidx	8	8	0	0.0
			.bss	197576	197576	0	0.0
			.comment	206	206	0	0.0
			.data	1468	1468	0	0.0
			.debug_abbrev	955071	955071	0	0.0
			.debug_aranges	104360	104360	0	0.0
			.debug_frame	353300	353300	0	0.0
			.debug_info	20539281	20545386	6105	0.0
			.debug_line	`2715330`	`2715362`	32	0.0
			.debug_line_str	513	513	0	0.0
			.debug_loc	33340	33340	0	0.0
			.debug_loclists	1532678	1532977	299	0.0
			.debug_ranges	4984	4984	0	0.0
			.debug_rnglists	96806	96814	8	0.0
			.debug_str	`3266523`	3266540	17	0.0
			.ramVecs	780	780	0	0.0
			.resetVecs	64	64	0	0.0
			.rodata	104402	104402	0	0.0
			.shstrtab	265	265	0	0.0
			.stack	2048	2048	0	0.0
			.strtab	484213	484215	2	0.0
			.symtab	287456	287456	0	0.0
			.text	499320	499352	32	0.0
k32w	contact	k32w0+release	(read only)	583644	583708	64	0.0
			(read/write)	82704	82704	0	0.0
			.bss	65888	65888	0	0.0
			.data	2192	2192	0	0.0
			.text	583108	583172	64	0.0
	light	k32w0+release	(read only)	582588	582716	128	0.0
			(read/write)	82356	82356	0	0.0
			.bss	65552	65552	0	0.0
			.data	2180	2180	0	0.0
			.text	582052	582180	128	0.0
	lock	k32w0+release	(read only)	556076	556108	32	0.0
			(read/write)	80352	80352	0	0.0
			.bss	63624	63624	0	0.0
			.data	2104	2104	0	0.0
			.text	555540	555572	32	0.0
mbed	lock-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0	0.0
			(read/write)	`2495408`	`2495472`	64	0.0
			.bss	216304	216304	0	0.0
			.data	5144	5144	0	0.0
			.text	1458092	1458156	64	0.0
nrfconnect	all-clusters-app	nrf52840dk_nrf52840	(read only)	4	4	0	0.0
			(read/write)	1178072	1178472	400	0.0
			bss	155621	155621	0	0.0
			rodata	133096	133128	32	0.0
			text	808648	809016	368	0.0
		nrf7002dk_nrf5340_cpuapp	(read only)	4	4	0	0.0
			(read/write)	`1437816`	`1438200`	384	0.0
			bss	135361	135361	0	0.0
			rodata	229112	229144	32	0.0
			text	779252	779612	360	0.0
	all-clusters-minimal-app	nrf52840dk_nrf52840	(read only)	4	4	0	0.0
			(read/write)	1124288	1124644	356	0.0
			bss	154773	154773	0	0.0
			rodata	109856	109892	36	0.0
			text	778944	779264	320	0.0

step0035 · 2023-05-26T09:37:27Z

will it be better if i create a separate set function for the affected attribute? Seems like there are a lot of build failures.

Damian-Nordic · 2023-05-26T09:52:09Z

Note that we solved this issue using DeferredAttributePersister not to make changes in the attribute storage: #23366. But maybe your approach is more straightforward, indeed.

tcarmelveilleux · 2023-05-26T18:52:24Z

src/app/util/attribute-table.cpp

@@ -54,18 +54,19 @@ EmberAfStatus emberAfWriteAttributeExternal(EndpointId endpoint, ClusterId clust
    case EmberAfAttributeWritePermission::AllowWriteNormal:
    case EmberAfAttributeWritePermission::AllowWriteOfReadOnly:
        return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType,
-                                  (extWritePermission == EmberAfAttributeWritePermission::AllowWriteOfReadOnly), false);
+                                  (extWritePermission == EmberAfAttributeWritePermission::AllowWriteOfReadOnly), false, false);
    default:
        return (EmberAfStatus) extWritePermission;
    }
 }

 EmberAfStatus emberAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,


Suggest using a new method emberAfWriteAttributeSkipNonVolatile to be much more explicit, which calls the version with isTemp.

This way, almost none of the accessors below need to be changed.

tcarmelveilleux · 2023-05-26T18:53:26Z

zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.cpp

The template for codegen has to be changed. You cannot manually change files in zzz_generated. See comment higher up (// THIS FILE IS GENERATED BY ZAP)

bzbarsky-apple

This is a spec violation if done in this blanket way. See https://github.com/project-chip/connectedhomeip/blob/master/src/app/DeferredAttributePersistenceProvider.h for how this should be handled.

step0035 · 2023-05-29T01:49:24Z

Thanks for the reviews, i will close this PR and look into DeferredAttributePersistenceProvider.

step0035 added 3 commits May 26, 2023 16:23

[levelcontrol] [colorcontrol] dont store intermediate level during tr…

56184d7

…ansitioning

restyle

612e93c

cleanup

5e3e2a9

github-actions bot added the app label May 26, 2023

pullapprove bot requested review from mlepage-google, mrjerryjohns, msandstedt, mspang, nivi-apple, robszewczyk, saurabhst, selissia, sharadb-amazon, tcarmelveilleux, tecimovic, turon, vijs, vivien-apple, woody-apple, xylophone21, younghak-hwang and yunhanw-google May 26, 2023 08:42

pullapprove bot added the review - pending label May 26, 2023

step0035 added 2 commits May 26, 2023 16:59

fix compile errors

60896af

add comments and restyle

bdfa143

tcarmelveilleux reviewed May 26, 2023

View reviewed changes

bzbarsky-apple requested changes May 26, 2023

View reviewed changes

step0035 closed this May 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dont store transition val #26867

Dont store transition val #26867

step0035 commented May 26, 2023 •

edited

Loading

CLAassistant commented May 26, 2023 •

edited

Loading

github-actions bot commented May 26, 2023

github-actions bot commented May 26, 2023 •

edited

Loading

step0035 commented May 26, 2023

Damian-Nordic commented May 26, 2023

tcarmelveilleux May 26, 2023

tcarmelveilleux May 26, 2023

bzbarsky-apple left a comment

step0035 commented May 29, 2023

Dont store transition val #26867

Dont store transition val #26867

Conversation

step0035 commented May 26, 2023 • edited Loading

Note

CLAassistant commented May 26, 2023 • edited Loading

github-actions bot commented May 26, 2023

github-actions bot commented May 26, 2023 • edited Loading

step0035 commented May 26, 2023

Damian-Nordic commented May 26, 2023

tcarmelveilleux May 26, 2023

Choose a reason for hiding this comment

tcarmelveilleux May 26, 2023

Choose a reason for hiding this comment

bzbarsky-apple left a comment

Choose a reason for hiding this comment

step0035 commented May 29, 2023

step0035 commented May 26, 2023 •

edited

Loading

CLAassistant commented May 26, 2023 •

edited

Loading

github-actions bot commented May 26, 2023 •

edited

Loading