Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Matter 1.4 #1657

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Matter 1.4 #1657

wants to merge 1 commit into from

Conversation

lauckhart
Copy link
Collaborator

This includes application-level upgrades to Matter 1.4. Upgrades the model, cluster definitions, endpoint types and behaviors. Some protocol-level features are still TODO.

One notable change is that Basic Information "unique ID" is mandatory in 1.4. We now generate one unconditionally unless provided as input. This required tweaking current and deprecated codepaths and numerous tests but should be largely transparent to users.

Extends conformance and constraint parsers with new flags and syntax, plus a couple of bits that were used previously but we didn't support. Some of these were complicated and consisted of ad-hoc syntax only used in one or two places... But validation of the final model now sits at 100%.

Includes many upgrades to spec ingestion to handle special cases, mostly due to malformatting. Replaces some hard-coded fixes with automatic detection/repair of errors that have become more prevalent as the spec grows.

Tweaks codegen for derived enums to generate union types that accept values from extended enums. This appears to have been unnecessary because the specification was changed to include the redundant values, possibly because the equivalent fix was deemed infeasible in CHIP and/or alchemy. However it does improve consistency so leaving in.

Includes a few minor CHIP test tweaks. The new changes appear to have been transparent to CHIP but some test edits were no longer necessary and I fixed a few tests that had been broken previously.

Finally, includes a large chunk of code to handle ANSI text processing. A bit of a rathole... Came about because I wanted to make test error reporting more legible, which requires complex nested merging of styled text from multiple sources. And I'd gotten tired of the mix of hand-coded styling and use of a 3rd-party library that has typing issues. Over time we can migrate to this new code and drop the 3rd-party dependency, but will require some package reorganization because it currently lives in "tooling" which is not available everywhere. So just approaching on an "as needed" basis for now.

Copy link

semanticdiff-com bot commented Jan 24, 2025

Review changes with  SemanticDiff

This includes application-level upgrades to Matter 1.4.  Upgrades the model, cluster definitions, endpoint types and behaviors.  Some protocol-level features are still TODO.

One notable change is that Basic Information "unique ID" is mandatory in 1.4.  We now generate one unconditionally unless provided as input.  This required tweaking current and deprecated codepaths and numerous tests but should be largely transparent to users.

Extends conformance and constraint parsers with new flags and syntax, plus a couple of bits that were used previously but we didn't support.  Some of these were complicated and consisted of ad-hoc syntax only used in one or two places...  But validation of the final model now sits at 100%.

Includes many upgrades to spec ingestion to handle special cases, mostly due to malformatting.  Replaces some hard-coded fixes with automatic detection/repair of errors that have become more prevalent as the spec grows.

Tweaks codegen for derived enums to generate union types that accept values from extended enums.  This appears to have been unnecessary because the specification was changed to include the redundant values, possibly because the equivalent fix was deemed infeasible in CHIP and/or alchemy.  However it does improve consistency so leaving in.

Includes a few minor CHIP test tweaks.  The new changes appear to have been transparent to CHIP but some test edits were no longer necessary and I fixed a few tests that had been broken previously.

Finally, includes a large chunk of code to handle ANSI text processing.  A bit of a rathole...  Came about because I wanted to make test error reporting more legible, which requires complex nested merging of styled text from multiple sources.  And I'd gotten tired of the mix of hand-coded styling and use of a 3rd-party library that has typing issues.  Over time we can migrate to this new code and drop the 3rd-party dependency, but will require some package reorganization because it currently lives in "tooling" which is not available everywhere.  So just approaching on an "as needed" basis for now.
children: [
{ tag: "field", name: "Decrement", id: 0 },
{ tag: "field", name: "Increment", id: 1 },
],
},

// In 1.4 they created ColorLoopDirectionEnum but left the type of ColorLoopDirection attribute as uint8
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -135,14 +140,30 @@ LocalMatter.children.push({
],
},

// In 1.4 the spec removed default values for these. Unsure why but bring them back as they are mandatory
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact was removed because of the fact that no one changed it and in fact these defaults make o sense because no light will ever be able to support these values this. So the default was removed to ensure manufactuers set meaningful values here. So I would also tend to remove to make sure implementors set meaningful values here

// Set the correct type of MoveMode because just in the description
{
tag: "command",
id: 0x4b,
name: "MoveColorTemperature",

children: [
{ tag: "field", name: "MoveMode", id: 0x0, type: "MoveMode", conformance: "M", constraint: "desc" },
{ tag: "field", name: "MoveMode", id: 0x0, type: "MoveModeEnum", conformance: "M", constraint: "desc" },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this not be in specs in 1.4 correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only edited these lines because the types changed and they otherwise break the model. I generally don't review the overrides to see which are unnecessary anymore

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, see my comment ... we can also do that after merging here or incorporate it ... both ok for me

@@ -153,7 +174,7 @@ LocalMatter.children.push({
name: "StepColorTemperature",

children: [
{ tag: "field", name: "StepMode", id: 0x0, type: "StepMode", conformance: "M", constraint: "desc" },
{ tag: "field", name: "StepMode", id: 0x0, type: "StepModeEnum", conformance: "M", constraint: "desc" },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also this should be correct in 1.4 specs

@@ -21,7 +21,7 @@ LocalMatter.children.push({
tag: "field",
name: "Ranges",

// 1.3 spec mislabels the "access" column as "default"
// 1.3 spec mislabels the "access" column as "default" (still present in 1.4 as well)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requirements: OccupancySensorRequirements,
behaviors: SupportedBehaviors(
OccupancySensorRequirements.server.mandatory.Identify,
OccupancySensorRequirements.server.mandatory.OccupancySensing
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is removed? it should be still mandatory

@@ -37,7 +40,7 @@ export const PowerSourceEndpointDefinition = MutableEndpoint({
deviceRevision: 1,
deviceClass: DeviceClassification.Utility,
requirements: PowerSourceRequirements,
behaviors: SupportedBehaviors(PowerSourceRequirements.server.mandatory.PowerSource)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or this? Or has definition changed?

@Apollon77
Copy link
Collaborator

@lauckhart For overrides I also assume:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants