OnOff Cluster does not enforce EffectVariant value requirements correctly

[agatah2333]

Reproduction steps

The type for Field 1 of Command 0x40 in the OnOff cluster should be DelayedAllOffEffectVariantEnum, but it is currently written as enum8. The range of values is inconsistent.

The range of DelayedAllOffEffectVariantEnum is 0-2, result in

./chip-tool onoff off-with-effect 0 4   0x654325 1

This command could work incorrectly due to the missing constraints.

Bug prevalence

each time

GitHub hash of the SDK that was being used

5bb5c9e

Platform

other

Platform Version(s)

1.3

Type

Spec Compliance Issue, Security Issue, Core SDK Performance Improvement, Core SDK Interopability Issue

Anything else?

No response

[bzbarsky-apple]

The type for Field 1 of Command 0x40 in the OnOff cluster should be DelayedAllOffEffectVariantEnum

That's not what the spec I am looking at says. It says enum8.

That said, while the XML is correct, this spec requirement:

This field ... SHALL contain one of the non-reserved values listed in either DelayedAllOffEffectVariantEnum or DyingLightEffectVariantEnum.

does not seem to be enforced. And that said, the spec really needs to define the behavior here more clearly: what happens if EffectIdentifier is DyingLight but EffectVariant is set to 1?

@jmartinez-silabs @Rob-Houtepen-Signify

[Rob-Houtepen-Signify]

@bzbarsky-apple
The spec says: If the server does not support the given variant, it SHALL use the default variant.
This allows future extensions.

[agatah2333]

Thanks for your reply! I have a follow-up question: If a given value, such as 4, exceeds the range of the DelayedAllOffEffectVariantEnum, what is the corresponding default value?

Another observation:

./chip-tool onoff off-with-effect 0 10   0x654325 1

This command could work incorrectly due to the missing enum8 constraints.

[Rob-Houtepen-Signify]

@agatah2333 Good catch. What is the default variant is lost in translation from Zigbee to Matter. But variant with value 0x00 must be considered as the default.

[Rob-Houtepen-Signify]

@agatah2333 I'm not sure whether I catch your point on the command example. Are you saying that today chip-tool allows (and sends) the variant parameter with value 10? I would say that is good for testing. :-)

[jmartinez-silabs]

@agatah2333 @bzbarsky-apple @Rob-Houtepen-Signify
The sdk does indeed allow commands with an invalid variant parameter as shown here
./chip-tool onoff off-with-effect 0 10 0x654325 1

We should add a check to the command handler to return a constraint error here.

Lastly, off effect is implemented such that the user must register a callback to their effect implementation. The invalid parameter can be caught there too by each users implementation.

[agatah2333]

@agatah2333 I'm not sure whether I catch your point on the command example. Are you saying that today chip-tool allows (and sends) the variant parameter with value 10? I would say that is good for testing. :-)

Yes! That is exactly what I mean. Thanks for trying to understand!

I viewed the XML files, and it seems most clusters have well-defined minimum or maximum values. However, when running the chip-tool, you can see many values exceed these limits, as mentioned in the previous issue I raised in

• Binding cluster does not enforce "min 1" constraint on the Group field of TargetStruct #34215
• [1.3] The Groups XML files contain issues regarding group_id #34213 (comment)

Since Matter uses a data model to detail the constraints of the range of parameter value, is there any automated way to generate these constraints when using ZAP to generate the code?

[jmartinez-silabs]

@Rob-Houtepen-Signify @bzbarsky-apple

From the cluster spec.

This field is used to indicate which variant of the effect, indicated in the EffectIdentifier field, SHOULD be triggered. If the server does not support the given variant, it SHALL use the default variant. This field is dependent on the value of the EffectIdentifier field and SHALL contain one of the non-reserved values listed in either DelayedAllOffEffectVariantEnum or DyingLightEffectVariantEnum.

I understand from your comment above in this ticket :

The spec says: If the server does not support the given variant, it SHALL use the default variant.
Shall be implemented in a way, to allow future extension, that unsupported/unknown values shall default to the default EffectVariant.

That any unsupported/undefined value should apply a default effect variant

However, from the second sentence of the spec, I understand that the received Value for the EffectVariant shall be a value listed in either enums (DelayedAllOffEffectVariantEnum / DyingLightEffectVariantEnum).

Can you clarify, if an unknown value would classified as an unsupported value or not?

As I see it, a server could not support, lets say, DelayedOffSlowFade effectVariant but "know" about it (the enum value) and therefore defer to the default EffectVariant.
On the other hand, a server wouldn't know about an un-listed value in the enum. Shall it also default to its default effectVariant or shall the command be refused with a constraint error as the value isn't defined in the server VariantEnum

[bzbarsky-apple]

That is the question, yes. I think the spec was aiming for "use default for any unrecognized value", but I don't know this 100% for sure.

[Rob-Houtepen-Signify]

@bzbarsky-apple Yes, "use default for any unrecognized value" was the intended behavior.
You don't want some 'older' light to NOT turn off just because it does not recognize a newly defined effect / variant. :-)

[Rob-Houtepen-Signify]

Note that the defaults are the effects / variants with value 0x00. (This is lost in translation from ZCL.)

[jmartinez-silabs]

@Rob-Houtepen-Signify @bzbarsky-apple Thanks, I will apply in the SDK PR.

Would it be valuable to clarify in the spec