options.supportsHS should be aligned with meta.supportsHueAndSaturation

[sjorge]

While looking into Koenkk/zigbee2mqtt#17816 I spotted some confusing naming wrt options.supportsHS vs meta.supportsHueAndSaturation.

Both try to do very similar things:

• options: adjusts the exposes data to show support or lack of support for hs mode in addition to xy mode
• meta: disables reading of the HS color attributes (although we somehow still commands what doe HS operations)

This PR tries to adres this by:

• Switching to a more consistent naming
  • options.supportsHS to options.supportsHueAndSaturation (to bring them inline naming wise)
  • options.preferHS to options.preferHueAndSaturation (to be more consistent in the naming)
  • meta.enhancedHue to meta.supportsEnhancedHue (to be more consistent in the naming)
• Throw and error when meta.supportsHueAndSaturation == false in the toZigbee converter when using HS color (fixes bulbs like the trådfri cws ignoring commands triggered by a color.{hue,sat} payload
• passthru of options.supportsHueAndSaturation -> meta.supportsHueAndSaturation in lib/extend (to avoid forgetting to set it in both places or setting both to a conflicting value when using extends, exception being e.g. trådfri which does not use the extends)

Open Questions:

• default for meta.supportsHueAndSaturation is true default for options.supportHS (now options.supportsHueAndSaturation) was false, these should be aligned

Currently having the options default to false while most lights seem to supports it makes no sense, if most lights do not support it, we should change the default of the meta. In my test array of Hue, Hue BT, Tådfri, Innr, and Osram bulbs only Trådfri does not seem to support it, seeming to indicate that the meta default is the correct one?

[sjorge]

Tested if a random Hue bulb I had in my test box and an Osram one both still worked (Kind of annoying that the frontend uses {"color": "#hexhere"} for both color_hs and color_xy expose :/)

So tested with:

mosquitto_pub -t zigbee2mqtt/0x0017880104259333/set -m '{"color": {"hue": 128, "saturation": 100}}'
mosquitto_pub -t zigbee2mqtt/0x0017880104259333/set -m '{"color": {"x": 0.700, "y": 0.500}}'

I also read colorMode afterwards to see if it changed between 0 and 1, it did.

There is also some potential follow up work, not sure it's worth it or not. reading enhancedColorMode will show if we're in hs or ehs mode... but that's a lot of extra fiddling to lib/color and to not break home assistant we can't just expand the color_mode payload, so we need to introduce a enhanced_color_mode payload... that might confuse people.

[Koenkk]

supportHS is not a meta option, it is used in extend:

zigbee-herdsman-converters/src/lib/extend.js

Line 85 in 4649dcc

const exposes = [(options.supportsHS ? e.light_brightness_color(options.preferHS) : e.light_brightness_colorxy()),

[sjorge]

supportHS is not a meta option, it is used here:

zigbee-herdsman-converters/src/lib/extend.js

Line 85 in 4649dcc

const exposes = [(options.supportsHS ? e.light_brightness_color(options.preferHS) : e.light_brightness_colorxy()),

I'm happy to close this PR and do a new one just also adding supportsHS to the README.md, but it looks like supportsHS and supportsHueAndSaturation are really the same thing.

I'm dumb, I mixed up option and meta... let me revisit this in the afternoon, I do still think It's rather confusing and we should somehow better align these.

As at least some of these changes still make sense .e.g light.readColorAttributes() using the wrong default.
Also they are essentially trying to do the same thing, meta.supportsHueAndSaturation indicating if the firmware supports it which influences the toZigbee converter and light.readColorAttributes() and the other how to deal with the expose data.

[sjorge]

OK going to run some errands now, so have some time to think about this some more:

Before this PR in the current form:

• options.supportHS is used to indicate how the exposes data looks, just xy or xy + hs
• meta.supportsHueAndSaturation indictes if the device supports hs or not on the hw level
• converter POV we assumed all devices support HS by default
• exposes POV we assumed some devices support HS based on the options.supportsHS default in extend.js

Rework plan for this PR:

• undo the light.readColorAttributes() change, but updated README.md to reflect that the default is true
• make options.supportsHueAndSaturation (keeping the more consistent naming) also set meta.supportsHueAndSaturation in the extend

Indicating via options that there is no Hue/Saturation support would then do the same expose changes as before and in addition tell the converters/light.readColorAttributes() that there is no Hue/Saturation support, fixing the inconsistent views in exposes vs converter logic.

[sjorge]

Found another issue with some a tradfri bulb from my test set (this is on current master, not this PR)

Zigbee2MQTT:debug 2023-05-29 13:41:37: Received Zigbee message from '0x000d6ffffe197fe9', type 'readResponse', cluster 'lightingColorCtrl', data '{"colorMode":1,"currentX":30015,"currentY":26870}' from endpoint 1 with groupID 0
Zigbee2MQTT:info  2023-05-29 13:41:37: MQTT publish: topic 'zigbee2mqtt/0x000d6ffffe197fe9', payload '{"color":{"hue":32,"saturation":81,"x":0.458,"y":0.41},"color_mode":"xy","linkquality":225,"update":{"installed_version":318846322,"latest_version":587753009,"state":"available"},"update_available":true}'
Zigbee2MQTT:info  2023-05-29 13:41:37: Read result of 'lightingColorCtrl': {"colorMode":1,"currentX":30015,"currentY":26870}
Zigbee2MQTT:debug 2023-05-29 13:41:56: Received MQTT message on 'zigbee2mqtt/0x000d6ffffe197fe9/set' with data '{"color": {"hue": 128, "saturation": 100}}'
Zigbee2MQTT:debug 2023-05-29 13:41:56: Publishing 'set' 'color' to '0x000d6ffffe197fe9'
Zigbee2MQTT:info  2023-05-29 13:41:56: MQTT publish: topic 'zigbee2mqtt/0x000d6ffffe197fe9', payload '{"color":{"hue":128,"saturation":100,"x":0.1668,"y":0.6399},"color_mode":"hs","linkquality":229,"update":{"installed_version":318846322,"latest_version":587753009,"state":"available"},"update_available":true}'

The color does not change, as expected as this build has meta.supportsHueAdnSaturation set to false.

Zigbee2MQTT:debug 2023-05-29 13:42:59: Received MQTT message on 'zigbee2mqtt/0x000d6ffffe197fe9/get' with data '{"color":""}'
Zigbee2MQTT:debug 2023-05-29 13:42:59: Publishing get 'get' 'color' to '0x000d6ffffe197fe9'
Zigbee2MQTT:debug 2023-05-29 13:42:59: Received Zigbee message from '0x000d6ffffe197fe9', type 'readResponse', cluster 'lightingColorCtrl', data '{"colorMode":1,"currentX":30015,"currentY":26870}' from endpoint 1 with groupID 0
Zigbee2MQTT:info  2023-05-29 13:42:59: MQTT publish: topic 'zigbee2mqtt/0x000d6ffffe197fe9', payload '{"color":{"hue":32,"saturation":81,"x":0.458,"y":0.41},"color_mode":"xy","linkquality":225,"update":{"installed_version":318846322,"latest_version":587753009,"state":"available"},"update_available":true}'

We currently only handle these for the get converter as to not hit the UNSUPPORTED_ATTRIBUTE, we should also take care of this when calling set, we can fall back to converting hs to zy. I'll also take this with me into the next pass of this PR.

[sjorge]

OK hit another small roadblock, aside for philips.extend.light_* none of the regular extends set meta at all, there are two options forward:

• align defaults of meta.supportsHueAndSaturation with options.supportsHueAndSaturation(supportsHS) and make sure the non default case is set correctly in both options and meta for all devices
• somehow start setting meta from the general extends

The first one is the easiest, although I think the 2nd one is the cleanest, I am not sure how the extend + regular meta will interact.

Edit:

zigbee-herdsman-converters/src/index.js

Lines 71 to 74 in 4649dcc

	meta: extend.meta || definitionWithoutExtend.meta ? {
	...extend.meta,
	...definitionWithoutExtend.meta,
	} : undefined,

Looks to be safe to just set them in the extend, the non extend one will be merged into it and override any value present in both.

Option 2 it is

[sjorge]

@Koenkk this is refactor of the previous PR

• makes meta and options naming more consistent
• extend will pass through options.supportsHueAndSaturation to the meta property, as these should be in-sync (makes no sense otherwise)

There is one open question though, meta.supportsHueAndSaturation defaults to true but we set the default for options.supportsHueAndSaturation to false. I think those should be aligned too. So which one is the correct one?

I think the default for the options should be true instead of false, as it looks like most lighs seem to support it, as evidence by the very few lights where meta.supportsHueAndSaturation was set to false to avoid the UNSUPPORT_ATTRIBUTE erros on get calls for color.

Some tests done on a tradfri that only does XY and a hue one that does XY + (e)HS:

LCT012

Zigbee2MQTT:debug 2023-05-29 13:47:00: Received MQTT message on 'zigbee2mqtt/0x0017880104259333/set' with data '{"color": {"hue": 128, "saturation": 100}}'
Zigbee2MQTT:debug 2023-05-29 13:47:00: Publishing 'set' 'color' to '0x0017880104259333'
Zigbee2MQTT:info  2023-05-29 13:47:00: MQTT publish: topic 'zigbee2mqtt/0x0017880104259333', payload '{"color":{"hue":128,"saturation":100,"x":0.1668,"y":0.6399},"color_mode":"hs","linkquality":211,"state":"ON","update":{"installed_version":16786688,"latest_version":16786688,"state":"idle"},"update_available":false}'

TRADFRI bulb E27 CWS opal 600lm

Zigbee2MQTT:debug 2023-05-29 13:46:02: Received MQTT message on 'zigbee2mqtt/0x000d6ffffe197fe9/set' with data '{"color": {"hue": 128, "saturation": 100}}'
Zigbee2MQTT:debug 2023-05-29 13:46:02: Publishing 'set' 'color' to '0x000d6ffffe197fe9'
Zigbee2MQTT:warn  2023-05-29 13:46:02: This device does not support Hue/Saturation, falling back to X/Y!
Zigbee2MQTT:info  2023-05-29 13:46:02: MQTT publish: topic 'zigbee2mqtt/0x000d6ffffe197fe9', payload '{"color":{"hue":128,"saturation":100,"x":0.1668,"y":0.6399},"color_mode":"xy","linkquality":225,"update":{"installed_version":318846322,"latest_version":587753009,"state":"available"},"update_available":true}'

The tradfri now falls back to setting the color via XY and warning instead of just sending a command to the bulb which it ignores.

[Koenkk]

All looks good! I will merge it after the next release (probably friday)

[Koenkk]

Bedankt!