Generate XML using Alchemy tool for closure cluster and device type #37371
Conversation
|
Review changes with Changed Files
|
jmartinez-silabs
changed the title
| <cluster code="0x0104"/> | ||
| <cluster code="0x0106"/> |
There was a problem hiding this comment.
In the spec.... you can't just randomly reference a data type from cluster X in cluster Y. I mean, you can write that, and Alchemy seems to "do something" with it as here, but it's broken, because data type names are not unique; so they are always scoped to the cluster they are defined in, except for global data types, which do have unique names.
There was a problem hiding this comment.
For backwards compatibility purposes, alchemy allows this because existing clusters use each other's structs, but now that we have proper global struct support, we should use that instead.
You could do this most easily by moving the shared struct definitions to closures.adoc, and making sure you have references to them everywhere they're used.
| <cluster code="0x0104"/> | ||
| <cluster code="0x0106"/> |
There was a problem hiding this comment.
Why does this not apply to all the cluster IDs for Closure Dimension?
There was a problem hiding this comment.
Bug in alchemy; this is the first time we've had a set of cluster aliases reference a type from a different cluster. Fixed in 1.5.4.
| <attribute code="0x0000" side="server" define="COUNTDOWN_TIME" type="elapsed_s" isNullable="true" max="259200" optional="true"> | ||
| <description>CountdownTime</description> | ||
| <optionalConform/> |
There was a problem hiding this comment.
Maybe this is an alchemy question, but why do we have both optional="true" and optionalConform here? @hasty
There was a problem hiding this comment.
Because the optional attribute predated the embedding of DM-style conformance. The "optional" attribute essentially just means "not mandatory"
There was a problem hiding this comment.
Well, do we plan to keep the "optional" XML attribute in the long term? If not, we should make sure ZAP handles that and just stop emitting it. If we plan to keep it, we should not output the conformance bit when it's pure-optional. No matter what, we should not have two sources of truth here...
| <item name="AtPedestrian" value="0x04"/> | ||
| </enum> | ||
|
|
||
| <struct name="OverallStateStruct" apiMaturity="provisional"> |
There was a problem hiding this comment.
Shouldn't the enums also be tagged with apiMaturity="provisional"?
There was a problem hiding this comment.
The XSD for ZCL only allows apiMaturity on clusters and structs, so alchemy doesn't generate that attribute. We could update the XSD, of course. Is there anything downstream that's expecting this? I only see it on clusters, structs and attributes in the existing XML.
There was a problem hiding this comment.
Everything can be marked provisional in the spec, so anything should be able to be marked provisional in the XML. If the XSD does not support that, the XSD is broken. If the tooling we have on PRs does not support that (e.g. does not look at it), that tooling is broken.... @andy31415
| <item fieldId="2" name="Speed" type="ThreeLevelAutoEnum" optional="true" min="0x00" max="0x03"/> | ||
| </struct> | ||
|
|
||
| <cluster> |
There was a problem hiding this comment.
Shouldn't this have apiMaturity="provisional"?
There was a problem hiding this comment.
Alchemy used to do this if a cluster was new to the file, but it's been changed to only generate apiMaturity when the cluster is marked provisional in the spec. It's not marked provisional, so no apiMaturity.
There was a problem hiding this comment.
Shouldn't anything that's in ifdefs in the spec be provisional automatically @hasty ?
| <cluster> | ||
| <domain>Closures</domain> | ||
| <name>Closure 1st Dimension</name> | ||
| <code>0x0105</code> |
There was a problem hiding this comment.
I do wonder how the order of these clusters was determined; it's not ID order and it's not name order... it's a little odd. @hasty is this something Alchemy chose somehow?
There was a problem hiding this comment.
When it's going through the list of clusters it couldn't find in the file, it ends up going in the order the clusters were processed. Since this is done in parallel, it's essentially whatever order the scheduler saw fit to process them. Fixed in v1.5.4
| <include cluster="Proxy Discovery" client="false" server="true" clientLocked="true" serverLocked="true"/> | ||
| <include cluster="Proxy Valid" client="false" server="true" clientLocked="true" serverLocked="true"/> | ||
| <include cluster="Pulse Width Modulation" client="false" server="true" clientLocked="true" serverLocked="true"/> | ||
| <include cluster="Proxy Configuration" client="false" server="true" clientLocked="true" serverLocked="true"></include> |
There was a problem hiding this comment.
Why all these irrelevant changes to the empty tag syntax? They make it very hard to review what changes are real here and what changes are just noise.
If this needs to happen, it should happen in a separate PR with no actual changes to the XML semantics in it.
There was a problem hiding this comment.
The existing file has inconsistent handling of empty tags; trying to replicate that when serializing to XML would be difficult and kinda pointless.
We should do a separate PR that just fixes these; in the meantime, I'd suggest discarding the unrelated changes.
| <clusters> | ||
| <include cluster="Groups" client="false" server="false" clientLocked="true" serverLocked="false"></include> | ||
| <include cluster="Identify" client="false" server="false" clientLocked="false" serverLocked="false"></include> | ||
| <include cluster="Scenes Management" client="false" server="false" clientLocked="true" serverLocked="false"></include> |
There was a problem hiding this comment.
This is missing all the actual Closure clusters, no? Probably because the spec is broken and does not list the right cluster IDs here....
There was a problem hiding this comment.
Yeah, by this point alchemy has stopped trying to match by name, because it gets too complicated when reconciling with the base device type. Fixing the spec will fix this.
| <deviceType> | ||
| <name>MA-heatcool</name> | ||
| <domain>CHIP</domain> | ||
| <typeName>Heating/Cooling Unit</typeName> |
There was a problem hiding this comment.
Why is this being removed? Seems unrelated to this PR. If this is "general cleanup", it should happen in a separate PR before this one.
There was a problem hiding this comment.
Agreed. I assume this was a device type at some point, but it's not in the spec.
| @@ -150,9 +152,15 @@ | |||
| "manufacturersXml": "../../../../src/app/zap-templates/zcl/data-model/manufacturers.xml", | |||
| "options": { | |||
| "text": { | |||
| "defaultResponsePolicy": ["Always", "Conditional", "Never"] | |||
| "defaultResponsePolicy": [ | |||
There was a problem hiding this comment.
This change is making your PR fail CI, quite apart from making it harder to review.
There was a problem hiding this comment.
I can probably fix this; just alchemy's JSON formatting out of sync with what restyled expects.
|
PR #37371: Size comparison from 92f9f0b to 92871d7 Full report (3 builds for cc32xx, stm32)
|
|
As closure dimension cluster need more discussion and fixes on the spec side. We are splitting this PR into two PR's
|
Changelog
Testing
Manually verified the generated parameters with the specification