Use enum class for GeneralCommissioning::RegulatoryLocationType

[vivien-apple]

Problem

RegulatoryLocationType from

connectedhomeip/src/app/zap-templates/zcl/data-model/chip/general-commissioning-cluster.xml

Line 26 in d24eff1

<enum name="RegulatoryLocationType" type="ENUM8">

is still beeing generated as a weakly typed enum.

Change overview

• Remove it from the list of executions in isWeaklyTypedEnum
• Update the code accordingly

[vivien-apple]

I'm not 100% happy with this patch, but it may let me ask a question.

Basically I have added a lot of static_assert on the fact that RegulatoryLocationType is stored as an uint8_t.

This is because src/include/platform/ConfigurationManager.h expects an uint8_t for it. Now, I can update src/include/platform/ConfigurationManager.h to use a GeneralCommissioning::RegulatoryLocationType instead of an uint8_t and it will makes consumer lives much easier...

But I have some doubts here. The reason is that GeneralCommissioning::RegulatoryLocationType and then all the code that uses src/include/platform/ConfigurationManager.h will depends on generated code.
Please note that his is already the case since ConfigurationManager.h previously depends on <app-common/zap-generated/enums.h> but it will spread it onto much more places.

Do someone has any opinion on that ?

[andy31415]

I'm not 100% happy with this patch, but it may let me ask a question.

Basically I have added a lot of static_assert on the fact that RegulatoryLocationType is stored as an uint8_t.

This is because src/include/platform/ConfigurationManager.h expects an uint8_t for it. Now, I can update src/include/platform/ConfigurationManager.h to use a GeneralCommissioning::RegulatoryLocationType instead of an uint8_t and it will makes consumer lives much easier...

But I have some doubts here. The reason is that GeneralCommissioning::RegulatoryLocationType and then all the code that uses src/include/platform/ConfigurationManager.h will depends on generated code. Please note that his is already the case since ConfigurationManager.h previously depends on <app-common/zap-generated/enums.h> but it will spread it onto much more places.

Do someone has any opinion on that ?

I believe a long term strategy would be to make sure our tooling supports separate generated files in some way, so that we have have some generic includes shared. Original zap logic was that "app generates" however we have a lot of shared code at various levels. I think we should find a way to generate what we need where we need.

I am not sure that is attainable though in the medium term, especially within a 'end of january FC' date.

A proposed alternative:

• make platform::ConfigurationManager have an own type like class RegulatoryLocation { public: uint8_t Raw() const; private: uint8_t value;}
• have ember code have a method that converts the enum type into regulatoryLocation in one place. That way we can assert sizes in only one location (some emberRegulatoryTypeToPlatform method)z

[bzbarsky-apple]

So my take on this is that having configuration manager depend on non-app-specific generated code (i.e. "depend on the spec-defined types") should be fine. This is not app-specific code; this is code generated based on just XML without taking any .zap files into account.

[bzbarsky-apple]

Oh, and using to_underlying instead of static_cast, plus making sure -Wconversion is used as needed will allow removing all those static_asserts, right?

[github-actions]

PR #13374: Size comparison from 36a759d to 00ba343

Full report (1 build for telink)

platform	target	config	section	36a759d	00ba343	change	% change
telink	lighting-app	tlsr9518adk80d	(read/write)	834102	834102	0	0.0
			bss	86864	86864	0	0.0
			noinit	37160	37160	0	0.0
			text	582240	582240	0	0.0

[vivien-apple]

I did use to_underlying and it makes the code much cleaner. That's probably enough for now.

[vivien-apple]

I believe a long term strategy would be to make sure our tooling supports separate generated files in some way, so that we have have some generic includes shared. Original zap logic was that "app generates" however we have a lot of shared code at various levels. I think we should find a way to generate what we need where we need.

app-common is already separated from "app" specific files. Is that "OK" to have more places depends on it ?

[github-actions]

PR #13374: Size comparison from 36a759d to eb84cf8

Increases (2 builds for linux)

platform	target	config	section	36a759d	eb84cf8	change	% change
linux	chip-tool-ipv6only	arm64	(read only)	7033996	7034028	32	0.0
			.text	5956932	5956964	32	0.0
	thermostat-no-ble	arm64	(read only)	2031036	2031100	64	0.0
			.text	1689280	1689344	64	0.0

Full report (14 builds for efr32, k32w, linux, p6, qpg, telink)

platform	target	config	section	36a759d	eb84cf8	change	% change
efr32	lighting-app	BRD4161A	(read only)	831968	831968	0	0.0
			(read/write)	127088	127088	0	0.0
			.bss	125208	125208	0	0.0
			.data	1876	1876	0	0.0
			.text	831960	831960	0	0.0
		BRD4161A+rpc	(read only)	819148	819148	0	0.0
			(read/write)	143744	143744	0	0.0
			.bss	141768	141768	0	0.0
			.data	1976	1976	0	0.0
			.text	819140	819140	0	0.0
	window-app	BRD4161A	(read only)	805416	805416	0	0.0
			(read/write)	126024	126024	0	0.0
			.bss	124192	124192	0	0.0
			.data	1832	1832	0	0.0
			.text	805408	805408	0	0.0
k32w	light	k32w061+release	(read/write)	655772	655772	0	0.0
			.bss	76864	76864	0	0.0
			.data	1848	1848	0	0.0
			.text	571260	571260	0	0.0
	lock	k32w061+release	(read/write)	660032	660032	0	0.0
			.bss	77160	77160	0	0.0
			.data	1868	1868	0	0.0
			.text	575204	575204	0	0.0
linux	chip-tool-ipv6only	arm64	(read only)	7033996	7034028	32	0.0
			(read/write)	325073	325073	0	0.0
			.bss	54241	54241	0	0.0
			.data	1096	1096	0	0.0
			.data.rel.ro	209064	209064	0	0.0
			.dynamic	560	560	0	0.0
			.got	56992	56992	0	0.0
			.init	24	24	0	0.0
			.init_array	168	168	0	0.0
			.rodata	384292	384292	0	0.0
			.text	5956932	5956964	32	0.0
	thermostat-no-ble	arm64	(read only)	2031036	2031100	64	0.0
			(read/write)	144193	144193	0	0.0
			.bss	64033	64033	0	0.0
			.data	880	880	0	0.0
			.data.rel.ro	72392	72392	0	0.0
			.dynamic	560	560	0	0.0
			.got	3952	3952	0	0.0
			.init	24	24	0	0.0
			.init_array	296	296	0	0.0
			.rodata	128844	128844	0	0.0
			.text	1689280	1689344	64	0.0
p6	all-clusters-app	default	(read/write)	2404872	2404872	0	0.0
			.bss	117020	117020	0	0.0
			.data	2592	2592	0	0.0
			.text	1363136	1363136	0	0.0
	light-app	default	(read/write)	2326832	2326832	0	0.0
			.bss	105888	105888	0	0.0
			.data	2384	2384	0	0.0
			.text	1285096	1285096	0	0.0
	lock-app	default	(read/write)	2299032	2299032	0	0.0
			.bss	104768	104768	0	0.0
			.data	2336	2336	0	0.0
			.text	1257296	1257296	0	0.0
qpg	lighting-app	qpg6105+debug	(read only)	533268	533268	0	0.0
			(read/write)	146936	146936	0	0.0
			.bss	86688	86688	0	0.0
			.data	1004	1004	0	0.0
			.text	527948	527948	0	0.0
	lock-app	qpg6105+debug	(read only)	505044	505044	0	0.0
			(read/write)	146940	146940	0	0.0
			.bss	85824	85824	0	0.0
			.data	952	952	0	0.0
			.text	499724	499724	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)	834102	834102	0	0.0
			bss	86864	86864	0	0.0
			noinit	37160	37160	0	0.0
			text	582240	582240	0	0.0

[andy31415]

I believe a long term strategy would be to make sure our tooling supports separate generated files in some way, so that we have have some generic includes shared. Original zap logic was that "app generates" however we have a lot of shared code at various levels. I think we should find a way to generate what we need where we need.

app-common is already separated from "app" specific files. Is that "OK" to have more places depends on it ?

I would prefer to not have 'app' in names - ideally things that are SDK should be somewhere inside SDK that is not src/app as that seems too high level. Platform is probably one of the lowest levels we have, so it should depend on other very low level (core or core/support ... or maybe some new 'chip-types' library).

[bzbarsky-apple]

We've already moved the manually-defined app-specified types into lib/core/DataModelTypes.h. I would argue that the ones that are auto-generated types are not different and should live under lib/core.