Update MatterIDL codegen to not take into account `enabled` for client attributes #25737

andy31415 · 2023-03-17T18:15:07Z

We expect to generate accessors for all client side attributes, so for client side this switches
code generation loops to use zcl_attributes_server.

This also switches to workardouns for other settings because of different helper modes:

clusterRef for isGlobalAttributes
entryType for isArray
completely DROPS isReportable (since everything is reportable currently ...only added a TODO)

As a result, .matter files are now expected to have all attributes in client clusters, regardless of zap enabling/disabling on them.

After project-chip/zap#899 usage, we may be able to simplify these back...

Fixes #25684

github-actions · 2023-03-17T18:26:58Z

PR #25737: Size comparison from bbb8e93 to 568fd26

Full report (1 build for cc32xx)

platform	target	config	section	`bbb8e93`	`568fd26`
cc32xx	lock	CC3235SF_LAUNCHXL		0	0
			(read only)	645489	645489
			(read/write)	203848	203848
			.ARM.attributes	44	44
			.ARM.exidx	8	8
			.bss	197248	197248
			.comment	194	194
			.data	1480	1480
			.debug_abbrev	930289	930289
			.debug_aranges	87392	87392
			.debug_frame	300284	300284
			.debug_info	20246022	20246022
			.debug_line	2661031	2661031
			.debug_loc	`2804874`	`2804874`
			.debug_ranges	283152	283152
			.debug_str	3027037	3027037
			.ramVecs	780	780
			.resetVecs	64	64
			.rodata	105993	105993
			.shstrtab	232	232
			.stab	204	204
			.stabstr	441	441
			.stack	2048	2048
			.strtab	380331	380331
			.symtab	257376	257376
			.text	537376	537376

brdandu · 2023-03-17T18:24:12Z

src/app/zap-templates/templates/app/MatterIDL.zapt

-        {{~/if~}}
+  {{#if (is_client side)}}
+    {{#zcl_attributes_server}}
+      {{~#if clusterRef}}{{!Using `ClusterRef` to detect global attributes~}}


This works but if needed I am wondering if global attributes belong to a certain range. In that case you can also use if_compare if needed.

Using a numeric range seems even more hard-coded. Ideally I would have the helper, however isGlobalAttribute does not work.

brdandu · 2023-03-17T18:26:55Z

src/app/zap-templates/partials/idl/cluster_attribute.zapt

+  {{~/chip_access_elements~}}
+  {{~#if isNullable~}}
+    nullable {{!marker to place a space even with whitespace removal~}}
+  {{~/if~}} {{#if entryType}}{{entryType}}{{else}}{{type}}{{/if}}


You can use {{#if isArray}} if under zcl_attributes_server block helper

It did not work for me. isArray was always false (notice that isArray was already used in this method)

It should work with today's ZAP release.

src/app/zap-templates/partials/idl/cluster_attribute.zapt

brdandu · 2023-03-17T18:29:26Z

src/app/zap-templates/partials/idl/cluster_attribute.zapt

+  readonly {{!marker to place a space even with whitespace removal~}}
+{{~/unless~}}
+{{~!TODO: write only attributes should also be supported~}}
+{{~!TODO: non-reportable (nosubscribe) attributes should also be supported ~}}


This is supported using isReportable and isReportableAttribute under zcl_attributes_server

isReportable does not mean the same thing as Matter's nosubscribe. Most importantly, isReportable is a ZAP setting, while nosubscribe needs to be an XML thing attached to the attribute independent of config.

isReportable stopped working when I was using zcl_attributes_server. It was only working for the chip_* version.

src/app/zap-templates/templates/app/MatterIDL.zapt

brdandu · 2023-03-17T18:33:16Z

src/app/zap-templates/templates/app/MatterIDL.zapt

+    {{/zcl_attributes_server~}}
+  {{else}}
+    {{#chip_server_cluster_attributes}}
+      {{~#unless isGlobalAttribute~}}


Why do you need to switch to chip_server_cluster_attributes here?
Are you looking for if_basic_attribute by any chance?

chip_server_cluster_attributes was used before already before. I only switched the client-side currently. This piece of code is unchanged from what was before (except I moved the attribute generation into a common file to re-use it)

bzbarsky-apple · 2023-03-17T18:33:09Z

examples/light-switch-app/light-switch-common/light-switch-app.matter

+  readonly attribute int16u primary1X = 17;
+  readonly attribute int16u primary1Y = 18;


The light switch app most definitely does not do this.

What is this part of the .matter file used for in the light switch app, if anything? What is the goal of listing this at all, if it's just going to be a list of everything in the cluster? Why not just have a list of clusters at that point?

This is the client-side cluster version.
It looks to me like the zap file says that the light app has the client-side cluster enabled for light switch. I am not sure why it does that ... but it does.

We do have an oddity in codegen here: our client side always only uses src/controller/data_model/controller-clusters.matter however our codegen client and server side.

I believe ideally we should separate client and server IDLs, however that is likely a different change.

Light switches are required by spec to support certain client clusters: On/Off, and depending on the switch Level Control, etc.

our client side always only uses src/controller/data_model/controller-clusters.matter

Some of our clients use that. Not all. Light-switch does not.

Again, I think we need to clearly decide what "client side" means in .matter files.

Client-side in matter files represents what client side shows up as zap (i.e. side == client).
It was meant to be a representation of what zap contains as configuration.

The issue seems to also be that our usage of "side == client" under zap is unusual ... we seem to care about cluster enabling but not about underlying attributes/commands enabling (is this true?). I am not familiar enough with zap logic to understand this. Logic-wise it seems to me like client-side should be able to use whatever it wants where as server side has granular opinionated control about what it wants to support (i.e. we cannot just codegen everything)

The issue seems to also be that our usage of "side == client" under zap is unusual

Our usage in some specific clients.

We seem to care about cluster enabling but not about underlying attributes/commands enabling

That is the current state of things. Ideally, we would move to not caring about any of the ZAP state in those clients at all, since they are meant to be "everything" clients and should ideally just generate based on the XML. Ideally they would not reference a .zap or .matter file at all.

I am pretty sure Darwin could make that switch now; I can't speak for the other clients. We would need to think a bit about whether chip-tool should expose some of the clusters that are currently disabled in the client ZAP config (like the proxy bits)....

Logic-wise it seems to me like client-side should be able to use whatever it wants

For some of our APIs there are strongly typed callback types, etc, and those get codegenned.

I think it would be worth looking at something like light-switch-app to see what, if anything, is actually codegenned there based on the client config. If the answer is "nothing", so the only things using the client config are the "everything" clients, that would be useful information.

src/app/zap-templates/templates/app/MatterIDL.zapt

github-actions · 2023-03-17T18:56:16Z

PR #25737: Size comparison from bbb8e93 to 790b79a

Increases (1 build for cc32xx)

platform	target	config	section	`bbb8e93`	`790b79a`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_info	20246022	`2024602`	2	0.0

Full report (1 build for cc32xx)

platform	target	config	section	`bbb8e93`	`790b79a`	change
cc32xx	lock	CC3235SF_LAUNCHXL		0	0	0
			(read only)	645489	645489	0
			(read/write)	203848	203848	0
			.ARM.attributes	44	44	0
			.ARM.exidx	8	8	0
			.bss	197248	197248	0
			.comment	194	194	0
			.data	1480	1480	0
			.debug_abbrev	930289	930289	0
			.debug_aranges	87392	87392	0
			.debug_frame	300284	300284	0
			.debug_info	20246022	`2024602`	2
			.debug_line	2661031	2661031	0
			.debug_loc	`2804874`	`2804874`	0
			.debug_ranges	283152	283152	0
			.debug_str	3027037	3027037	0
			.ramVecs	780	780	0
			.resetVecs	64	64	0
			.rodata	105993	105993	0
			.shstrtab	232	232	0
			.stab	204	204	0
			.stabstr	441	441	0
			.stack	2048	2048	0
			.strtab	380331	380331	0
			.symtab	257376	257376	0
			.text	537376	537376	0

andy31415 · 2023-03-23T14:50:58Z

Closing as #25693 will fix this and makes sure java zap-codegen is in sync with matter IDL codegen.

andreilitvin added 9 commits March 17, 2023 12:23

Split out attribute codegen logic

3d43276

Minor changes to structure

1ad3d19

Indent seems to work out now

ce9bb55

Update some code logic and update to latest zap nightly

ac06e5e

Another update

02e9435

Progress, except array support

56e65d6

Templates significantly updated

0ae2e92

Zap regen after some alignment updates

adfecea

Fix alignment

568fd26

github-actions bot added app controller examples labels Mar 17, 2023

pullapprove bot requested review from amitnj, anush-apple, arkq, bzbarsky-apple, carol-apple, chrisdecenzo, chshu, chulspro, cliffamzn, CodeChronos928, Damian-Nordic, dhrishi, electrocucaracha, emargolis, franck-apple, gjc13, harimau-qirex and harsha-rajendran March 17, 2023 18:15

pullapprove bot added the review - pending label Mar 17, 2023

brdandu suggested changes Mar 17, 2023

View reviewed changes

bzbarsky-apple reviewed Mar 17, 2023

View reviewed changes

Replace chip_clusters with all_user_clusters

790b79a

This was referenced Mar 20, 2023

Enable all thermostat client attributes in controller clusters #25753

Merged

Move ClusterWriteMapping.java to be generated from matter.idl #25773

Merged

andy31415 closed this Mar 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update MatterIDL codegen to not take into account `enabled` for client attributes #25737

Update MatterIDL codegen to not take into account `enabled` for client attributes #25737

andy31415 commented Mar 17, 2023 •

edited

Loading

github-actions bot commented Mar 17, 2023

brdandu Mar 17, 2023

andy31415 Mar 17, 2023

brdandu Mar 17, 2023

andy31415 Mar 17, 2023

bzbarsky-apple Mar 17, 2023

brdandu Mar 17, 2023

bzbarsky-apple Mar 17, 2023

andy31415 Mar 17, 2023

brdandu Mar 17, 2023

andy31415 Mar 17, 2023

bzbarsky-apple Mar 17, 2023

andy31415 Mar 17, 2023

bzbarsky-apple Mar 17, 2023

bzbarsky-apple Mar 17, 2023

andy31415 Mar 20, 2023

bzbarsky-apple Mar 21, 2023 •

edited

Loading

github-actions bot commented Mar 17, 2023

andy31415 commented Mar 23, 2023

		readonly attribute int16u primary1X = 17;
		readonly attribute int16u primary1Y = 18;

Update MatterIDL codegen to not take into account enabled for client attributes #25737

Update MatterIDL codegen to not take into account enabled for client attributes #25737

Conversation

andy31415 commented Mar 17, 2023 • edited Loading

github-actions bot commented Mar 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzbarsky-apple Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Mar 17, 2023

andy31415 commented Mar 23, 2023

Update MatterIDL codegen to not take into account `enabled` for client attributes #25737

Update MatterIDL codegen to not take into account `enabled` for client attributes #25737

andy31415 commented Mar 17, 2023 •

edited

Loading

bzbarsky-apple Mar 21, 2023 •

edited

Loading