Recheck and sync all clusters again against Chip XML/Specs #268
Conversation
| /** Used to remember if a state is already storednin an old scene to not store one again when sending another Off command */ | ||
| //globalSceneControl: OptionalAttribute(0x4000, TlvBoolean, { default: true }), | ||
| /** Used to remember if a state is already stored in an old scene to not store one again when sending another Off command */ | ||
| globalSceneControl: OptionalAttribute(0x4000, TlvBoolean, { default: true }), |
There was a problem hiding this comment.
No need to not add these optional ones
There was a problem hiding this comment.
Agreed with Ingo: globalSceneControl is not optional.
It should not be present when features.lightingLevelControl = false
and it is mandatory when features.lightingLevelControl = true
Let's keep those as commented out for now and we need to figure out a way to have the cluster definition changing based on a feature flag value.
There was a problem hiding this comment.
We have that already in other clusters the same way that we define things options if they are dependent on a featureflag. So i would do it the same here so that it can be used if the dev knows this dependency - until we have a better way. So "semantic optional" ;-)
There was a problem hiding this comment.
I added comments for the "semantic optional" approach in the two relevant clusters till now and also a comment.
For a clean splitup solution we need to either:
- enhance "BitFlag" used in the features to take a value to make is static ... downside would be that the feature struct is then duplicated, OR
- add a "featureDefaults" struct to the Cluster definition to allow to have the "feature definition" to be the same, but then set the feature flag directly. But in this case it should not be required anymore when instanziating the ClusterServer
I would propose to do that after code merge.
I we really do not come to an agreement I will comment the stuff out again and add an Todo - but this limits other Devs to use the fields and commands at all.
sync to XML fixes
sync to XML fixes
| /** Used to remember if a state is already storednin an old scene to not store one again when sending another Off command */ | ||
| //globalSceneControl: OptionalAttribute(0x4000, TlvBoolean, { default: true }), | ||
| /** Used to remember if a state is already stored in an old scene to not store one again when sending another Off command */ | ||
| globalSceneControl: OptionalAttribute(0x4000, TlvBoolean, { default: true }), |
There was a problem hiding this comment.
Agreed with Ingo: globalSceneControl is not optional.
It should not be present when features.lightingLevelControl = false
and it is mandatory when features.lightingLevelControl = true
Let's keep those as commented out for now and we need to figure out a way to have the cluster definition changing based on a feature flag value.
|
Content included in project-chip/matter.js#59 or as followup |
* Merging clusters with matter.js * fix * Adds already approved content from mfucci/node-matter#268 * Add fix for GroupsCluster * adjust GeneralDiagnostics to use all features
I did a full check of all clusters in as of now against the Chip XMLs and specs ... Here are the results.
I also streamlined all names tp be "without spaces" so that it easiuer readable in logging