Don't encode fabric-index in fabric scoped structs for a write. #15268

bzbarsky-apple · 2022-02-16T18:10:02Z

Problem

We are encoding (uselessly) fabric indices for a write.

Change overview

Stop doing that, at least when the WriteClient is doing the encoding itself.

Some notes:

I would really prefer it if fabric-scoped things don't have an Encode() at all and have separate EncodeForWrite and EncodeForRead APIs, so when you are encoding them you have to think about what you are doing. That's a slightly more invasive change in terms of needing to change some code that currently calls Encode to call EncodeForRead, but absolutely doable.
Alternately, we could make DataModel::Encode (and presumably the struct's Encode) just take a boolean for whether it's an encode for a write. If we default it to false, that is more or less equivalent to the proposed change. More slightly inscrutable booleans floating around (esp. in all the Encode methods that have nothing to do with structs), a little less template duplication involved.
Or we could add a boolean to DataMode::Encode but only default it for things that are not fabric scoped, while requiring it for things that are fabric-scoped, so consumers have to think about it. This might have the least copy/paste code and should be a little simpler to get to than separate EncodeForRead/EncodeForWrite methods that only exist for fabric-scoped structs, since all Encode signatures would allow passing in the boolean.

Testing

Checked that if I do:

chip-tool accesscontrol write acl '[{"fabricIndex": 0, "privilege": 0, "authMode": 0, "subjects": [], "targets": []}]' 17 0

without this change the recipient sees:

Data = 
{
	0x0 = 0, 
	0x1 = 0, 
	0x2 = 0, 
	0x3 = [
		
	],
	0x4 = [
		
	],
},

and with this change the tag 0x0 is gone.

github-actions · 2022-02-16T18:32:39Z

PR #15268: Size comparison from 6e55dee to 26ad641

Increases (38 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)

platform	target	config	section	`6e55dee`	`26ad641`	change
cyw30739	light	cyw930739m2evb_01	(read/write)	597258	597314	56
			.app_xip_area	501176	501232	56
	lock	cyw930739m2evb_01	(read/write)	555202	555246	44
			.app_xip_area	460688	460732	44
	ota-requestor	cyw930739m2evb_01	(read/write)	576310	576366	56
			.app_xip_area	472364	472420	56
efr32	lighting-app	BRD4161A	(read only)	914108	914188	80
			.text	914100	914180	80
		BRD4161A+rpc	(read only)	942792	942872	80
			.text	942784	942864	80
	window-app	BRD4161A	(read only)	847960	848008	48
			.text	847952	848000	48
esp32	all-clusters-app	c3devkit	(read only)	947604	947634	30
			.flash.text	947604	947634	30
		m5stack	(read only)	997331	997403	72
			.flash.text	991947	992019	72
k32w	light	k32w061+release	(read/write)	691232	691280	48
			.text	605136	605184	48
	lock	k32w061+release	(read/write)	693848	693896	48
			.text	607496	607544	48
linux	all-clusters-app	debug	(read only)	2375169	2375713	544
			.text	`2006050`	2006594	544
	bridge-app	debug+rpc	(read only)	1730253	1730557	304
			.text	`1471829`	1472133	304
	chip-tool	debug	(read only)	`8578997`	8579429	432
			.text	7602885	7603317	432
	chip-tool-ipv6only	arm64	(read only)	`8332196`	`8332628`	432
			.text	7133748	7134180	432
	door-lock-app	debug	(read only)	1941953	1942273	320
			.text	1619698	1620018	320
	lighting-app	debug+rpc	(read only)	`2068217`	2068649	432
			.text	1747090	1747522	432
	ota-provider-app	debug	(read only)	1875417	1875737	320
			.text	1566178	1566498	320
	ota-requestor-app	debug	(read only)	`1888521`	1888937	416
			.text	1586018	1586434	416
	shell	debug	(read only)	2349865	2350393	528
			.text	1985874	`1986402`	528
	thermostat-no-ble	arm64	(read only)	`2162044`	2162476	432
			.text	1809888	1810320	432
	tv-app	debug	(read only)	2537161	2537593	432
			.text	2165650	2166082	432
mbed	all-clusters-app	CY8CPROTO_062_4343W+release	(read/write)	2428412	2428476	64
			.text	1390984	1391048	64
	lighting-app	CY8CPROTO_062_4343W+release	(read/write)	2391612	2391676	64
			.text	1354184	1354248	64
	shell	CY8CPROTO_062_4343W+release	(read/write)	2317820	2317884	64
			.text	1280392	1280456	64
nrfconnect	lighting-app	nrf52840dk_nrf52840	(read/write)	`1022191`	1022255	64
			text	699036	699108	72
		nrf52840dk_nrf52840+rpc	(read/write)	991487	991551	64
			text	678936	679008	72
		nrf52840dongle_nrf52840	(read/write)	1037011	1037059	48
			text	702928	702984	56
		nrf5340dk_nrf5340_cpuapp	(read/write)	928806	928886	80
			text	614056	614128	72
	lock-app	nrf52840dk_nrf52840	(read/write)	950895	950927	32
			text	641228	641268	40
		nrf5340dk_nrf5340_cpuapp	(read/write)	858358	858406	48
			text	557024	557068	44
	pump-app	nrf52840dk_nrf52840	(read/write)	949347	949379	32
			text	641048	641088	40
	pump-controller-app	nrf52840dk_nrf52840	(read/write)	945419	945467	48
			text	637288	637328	40
p6	all-clusters-app	default	(read/write)	2485872	2485952	80
			.text	1444136	1444216	80
	light-app	default	(read/write)	2392760	2392824	64
			.text	1351024	`1351088`	64
	lock-app	default	(read/write)	`2356240`	`2356288`	48
			.text	1314504	1314552	48
qpg	lighting-app	qpg6105+debug	(read only)	598744	598808	64
			.text	593424	593488	64
	lock-app	qpg6105+debug	(read only)	564352	564408	56
			.text	559032	559088	56
telink	lighting-app	tlsr9518adk80d	(read/write)	877490	877514	24
			text	617978	618002	24

Full report (43 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)

platform	target	config	section	`6e55dee`	`26ad641`	change
cyw30739	light	cyw930739m2evb_01	(read/write)	597258	597314	56
			.app_xip_area	501176	501232	56
			.bss	78780	78780	0
			.data	644	644	0
			.rodata	0	0	0
			.text	0	0	0
	lock	cyw930739m2evb_01	(read/write)	555202	555246	44
			.app_xip_area	460688	460732	44
			.bss	77252	77252	0
			.data	608	608	0
			.rodata	0	0	0
			.text	0	0	0
	ota-requestor	cyw930739m2evb_01	(read/write)	576310	576366	56
			.app_xip_area	472364	472420	56
			.bss	86356	86356	0
			.data	552	552	0
			.rodata	0	0	0
			.text	112	112	0
efr32	lighting-app	BRD4161A	(read only)	914108	914188	80
			(read/write)	129524	129524	0
			.bss	127488	127488	0
			.data	2036	2036	0
			.text	914100	914180	80
		BRD4161A+rpc	(read only)	942792	942872	80
			(read/write)	146440	146440	0
			.bss	144264	144264	0
			.data	2176	2176	0
			.text	942784	942864	80
	window-app	BRD4161A	(read only)	847960	848008	48
			(read/write)	127416	127416	0
			.bss	125512	125512	0
			.data	1904	1904	0
			.text	847952	848000	48
esp32	all-clusters-app	c3devkit	(read only)	947604	947634	30
			(read/write)	1401130	1401130	0
			.dram0.bss	68296	68296	0
			.dram0.data	14268	14268	0
			.flash.rodata	199664	199664	0
			.flash.text	947604	947634	30
			.iram0.text	62056	62056	0
		m5stack	(read only)	997331	997403	72
			(read/write)	466380	466380	0
			.dram0.bss	73432	73432	0
			.dram0.data	34064	34064	0
			.flash.rodata	226756	226756	0
			.flash.text	991947	992019	72
			.iram0.text	123399	123399	0
k32w	light	k32w061+release	(read/write)	691232	691280	48
			.bss	78384	78384	0
			.data	1912	1912	0
			.text	605136	605184	48
	lock	k32w061+release	(read/write)	693848	693896	48
			.bss	78600	78600	0
			.data	1952	1952	0
			.text	607496	607544	48
linux	all-clusters-app	debug	(read only)	2375169	2375713	544
			(read/write)	151072	151072	0
			.bss	65376	65376	0
			.data	1328	1328	0
			.data.rel.ro	78664	78664	0
			.dynamic	592	592	0
			.got	4160	4160	0
			.init	27	27	0
			.init_array	920	920	0
			.rodata	206181	206181	0
			.text	`2006050`	2006594	544
	bridge-app	debug+rpc	(read only)	1730253	1730557	304
			(read/write)	94536	94536	0
			.bss	49296	49296	0
			.data	2034	2034	0
			.data.rel.ro	38080	38080	0
			.dynamic	592	592	0
			.got	3952	3952	0
			.init	27	27	0
			.init_array	560	560	0
			.rodata	142092	142092	0
			.text	`1471829`	1472133	304
	chip-tool	debug	(read only)	`8578997`	8579429	432
			(read/write)	254816	254816	0
			.bss	40712	40712	0
			.data	1184	1184	0
			.data.rel.ro	206896	206896	0
			.dynamic	608	608	0
			.got	4784	4784	0
			.init	27	27	0
			.init_array	624	624	0
			.rodata	459701	459701	0
			.text	7602885	7603317	432
	chip-tool-ipv6only	arm64	(read only)	`8332196`	`8332628`	432
			(read/write)	362609	362609	0
			.bss	58961	58961	0
			.data	1216	1216	0
			.data.rel.ro	249368	249368	0
			.dynamic	560	560	0
			.got	49272	49272	0
			.init	24	24	0
			.init_array	200	200	0
			.rodata	435756	435756	0
			.text	7133748	7134180	432
	door-lock-app	debug	(read only)	1941953	1942273	320
			(read/write)	120504	120504	0
			.bss	51984	51984	0
			.data	1010	1010	0
			.data.rel.ro	62080	62080	0
			.dynamic	592	592	0
			.got	4136	4136	0
			.init	27	27	0
			.init_array	672	672	0
			.rodata	173394	173394	0
			.text	1619698	1620018	320
	lighting-app	debug+rpc	(read only)	`2068217`	2068649	432
			(read/write)	125784	125784	0
			.bss	53024	53024	0
			.data	1400	1400	0
			.data.rel.ro	65832	65832	0
			.dynamic	608	608	0
			.got	4168	4168	0
			.init	27	27	0
			.init_array	720	720	0
			.rodata	166033	166033	0
			.text	1747090	1747522	432
	ota-provider-app	debug	(read only)	1875417	1875737	320
			(read/write)	116216	116216	0
			.bss	51840	51840	0
			.data	1224	1224	0
			.data.rel.ro	57480	57480	0
			.dynamic	608	608	0
			.got	4392	4392	0
			.init	27	27	0
			.init_array	624	624	0
			.rodata	158275	158275	0
			.text	1566178	1566498	320
	ota-requestor-app	debug	(read only)	`1888521`	1888937	416
			(read/write)	117632	117632	0
			.bss	52256	52256	0
			.data	1160	1160	0
			.data.rel.ro	58744	58744	0
			.dynamic	592	592	0
			.got	4200	4200	0
			.init	27	27	0
			.init_array	632	632	0
			.rodata	153044	153044	0
			.text	1586018	1586434	416
	shell	debug	(read only)	2349865	2350393	528
			(read/write)	153424	153424	0
			.bss	73696	73696	0
			.data	832	832	0
			.data.rel.ro	73224	73224	0
			.dynamic	592	592	0
			.got	4168	4168	0
			.init	27	27	0
			.init_array	904	904	0
			.rodata	207186	207186	0
			.text	1985874	`1986402`	528
	thermostat-no-ble	arm64	(read only)	`2162044`	2162476	432
			(read/write)	150737	150737	0
			.bss	67489	67489	0
			.data	1032	1032	0
			.data.rel.ro	75024	75024	0
			.dynamic	560	560	0
			.got	4208	4208	0
			.init	24	24	0
			.init_array	336	336	0
			.rodata	133532	133532	0
			.text	1809888	1810320	432
	tv-app	debug	(read only)	2537161	2537593	432
			(read/write)	151456	151456	0
			.bss	69184	69184	0
			.data	3200	3200	0
			.data.rel.ro	73008	73008	0
			.dynamic	592	592	0
			.got	4552	4552	0
			.init	27	27	0
			.init_array	888	888	0
			.rodata	198197	198197	0
			.text	2165650	2166082	432
mbed	all-clusters-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0
			(read/write)	2428412	2428476	64
			.bss	195916	195916	0
			.data	5328	5328	0
			.text	1390984	1391048	64
	lighting-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0
			(read/write)	2391612	2391676	64
			.bss	188448	188448	0
			.data	5632	5632	0
			.text	1354184	1354248	64
	lock-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0
			(read/write)	2327056	2327056	0
			.bss	187424	187424	0
			.data	5608	5608	0
			.text	1289656	1289656	0
	pigweed-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0
			(read/write)	1139840	1139840	0
			.bss	11796	11796	0
			.data	4368	4368	0
			.text	103224	103224	0
	shell	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0
			(read/write)	2317820	2317884	64
			.bss	185972	185972	0
			.data	5440	5440	0
			.text	1280392	1280456	64
nrfconnect	lighting-app	nrf52840dk_nrf52840	(read/write)	`1022191`	1022255	64
			bss	123548	123548	0
			rodata	120604	120604	0
			text	699036	699108	72
		nrf52840dk_nrf52840+rpc	(read/write)	991487	991551	64
			bss	120736	120736	0
			rodata	112132	112132	0
			text	678936	679008	72
		nrf52840dongle_nrf52840	(read/write)	1037011	1037059	48
			bss	124768	124768	0
			rodata	119420	119420	0
			text	702928	702984	56
		nrf5340dk_nrf5340_cpuapp	(read/write)	928806	928886	80
			bss	120112	120112	0
			rodata	113848	113848	0
			text	614056	614128	72
	lock-app	nrf52840dk_nrf52840	(read/write)	950895	950927	32
			bss	121752	121752	0
			rodata	109300	109300	0
			text	641228	641268	40
		nrf5340dk_nrf5340_cpuapp	(read/write)	858358	858406	48
			bss	118344	118344	0
			rodata	102472	102472	0
			text	557024	557068	44
	pigweed-app	nrf52840dk_nrf52840	(read/write)	527595	527595	0
			bss	53632	53632	0
			rodata	49976	49976	0
			text	361016	361016	0
	pump-app	nrf52840dk_nrf52840	(read/write)	949347	949379	32
			bss	121472	121472	0
			rodata	108256	108256	0
			text	641048	641088	40
	pump-controller-app	nrf52840dk_nrf52840	(read/write)	945419	945467	48
			bss	121476	121476	0
			rodata	107952	107952	0
			text	637288	637328	40
	shell	nrf52840dk_nrf52840	(read/write)	810799	810799	0
			bss	113324	113324	0
			rodata	79364	79364	0
			text	540456	540456	0
p6	all-clusters-app	default	(read/write)	2485872	2485952	80
			.bss	124232	124232	0
			.data	2672	2672	0
			.text	1444136	1444216	80
	light-app	default	(read/write)	2392760	2392824	64
			.bss	113912	113912	0
			.data	2528	2528	0
			.text	1351024	`1351088`	64
	lock-app	default	(read/write)	`2356240`	`2356288`	48
			.bss	113640	113640	0
			.data	2488	2488	0
			.text	1314504	1314552	48
qpg	lighting-app	qpg6105+debug	(read only)	598744	598808	64
			(read/write)	146940	146940	0
			.bss	90744	90744	0
			.data	1112	1112	0
			.text	593424	593488	64
	lock-app	qpg6105+debug	(read only)	564352	564408	56
			(read/write)	146940	146940	0
			.bss	90728	90728	0
			.data	1064	1064	0
			.text	559032	559088	56
	persistent-storage-app	qpg6105+debug	(read only)	99520	99520	0
			(read/write)	146940	146940	0
			.bss	24004	24004	0
			.data	176	176	0
			.text	94200	94200	0
telink	lighting-app	tlsr9518adk80d	(read/write)	877490	877514	24
			bss	87520	87520	0
			noinit	37160	37160	0
			text	617978	618002	24

bzbarsky-apple · 2022-02-16T18:53:15Z

@mrjerryjohns is going to take over this and do the EncodeForWrite and EncodeForRead thing, since he needs something very similar anyway for fabric-sensitive bits.

This builds upon bzbarsky's original PR project-chip#15268 based upon discussions with him to create separate EncodeForWrite/EncodeForRead APIs that only apply to fabric-scoped structs. This is then used (for now) to omit encoding the fabric index on writes. Tests: Ran chip-tool to write an ACL entry and confirmed that that the fabric index field is omitted on writes.

* Separate DataModel APIs for encoding fabric-sensitive structs This builds upon bzbarsky's original PR #15268 based upon discussions with him to create separate EncodeForWrite/EncodeForRead APIs that only apply to fabric-scoped structs. This is then used (for now) to omit encoding the fabric index on writes. Tests: Ran chip-tool to write an ACL entry and confirmed that that the fabric index field is omitted on writes. * WIP * Review feedback * Tests were using fabric index 0 for fake fabric testing, but that trips the invalid fabric checker in AttributeValueEncoder::EncodeListItem... * Review feedback

Don't encode fabric-index in fabric scoped structs for a write.

26ad641

boring-cyborg bot added app examples labels Feb 16, 2022

bzbarsky-apple marked this pull request as draft February 16, 2022 18:10

bzbarsky-apple mentioned this pull request Feb 16, 2022

Don't encode fabric index for fabric-scoped structs when it's set to 0. #15265

Closed

bzbarsky-apple closed this Feb 16, 2022

bzbarsky-apple deleted the encode-indicate-write branch February 16, 2022 18:53

mrjerryjohns mentioned this pull request Feb 16, 2022

Separate DataModel APIs for encoding fabric-sensitive structs #15284

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't encode fabric-index in fabric scoped structs for a write. #15268

Don't encode fabric-index in fabric scoped structs for a write. #15268

bzbarsky-apple commented Feb 16, 2022

github-actions bot commented Feb 16, 2022 •

edited

Loading

bzbarsky-apple commented Feb 16, 2022

Don't encode fabric-index in fabric scoped structs for a write. #15268

Don't encode fabric-index in fabric scoped structs for a write. #15268

Conversation

bzbarsky-apple commented Feb 16, 2022

Problem

Change overview

Testing

github-actions bot commented Feb 16, 2022 • edited Loading

bzbarsky-apple commented Feb 16, 2022

github-actions bot commented Feb 16, 2022 •

edited

Loading