search

Koenkk / zigbee-herdsman-converters

Collection of device converters to be used with zigbee-herdsman
MIT License
918 stars 3.04k forks source link

Colors slightly off for !Hue and scene_add vs direct set differences #2017

Closed sjorge closed 3 years ago

sjorge commented 3 years ago

So I was messing with the RB 250 C as this is seems to be an odd bulb in the bunch that does not do enhancedHue... It also needs redfix.

FF0000 -> with redfix applied = dark red

hue:0, sat:100 -> off red, so I guess it also needs the red fix applied somehow... but... I noticed the following

Zigbee2MQTT:debug 2021-01-03 18:43:56: Received MQTT message on 'zigbee2mqtt/office/light/desk/bulb/set/color' with data '{"hue": 0, "saturation": 107}'
Zigbee2MQTT:debug 2021-01-03 18:43:56: Publishing 'set' 'color' to 'office/light/desk/bulb'                                                                                                                                                   Zigbee2MQTT:info  2021-01-03 18:43:56: MQTT publish: topic 'zigbee2mqtt/office/light/desk/bulb', payload '{"brightness":254,"color":{"hue":0,"saturation":107,"x":0.7006,"y":0.2993},"color_mode":1,"color_temp":37,"linkquality":63,"state":"
ON"}'

Zigbee2MQTT:debug 2021-01-03 18:44:21: Received MQTT message on 'zigbee2mqtt/office/light/desk/bulb/set/color' with data '{"hue": 0, "saturation": 108}'
Zigbee2MQTT:debug 2021-01-03 18:44:21: Publishing 'set' 'color' to 'office/light/desk/bulb'
Zigbee2MQTT:error 2021-01-03 18:44:21: Publish 'set' 'color' to 'office/light/desk/bulb' failed: 'RangeError [ERR_OUT_OF_RANGE]: Command 0x14b457fffe2bd760/1 lightingColorCtrl.moveToHueAndSaturation({"transtime":0,"hue":0,"saturation":255.99136000000001,"direction":0}, {"timeout":10000,"disableResponse":false,"disableRecovery":false,"disableDefaultResponse":false,"direction":0,"srcEndpoint":null,"reservedBits":0,"manufacturerCode":null,"transactionSequenceNumber":null}) failed (The value of "value" is out of range. It must be >= 0 and <= 255. Received 255.99136000000001)'
Zigbee2MQTT:debug 2021-01-03 18:44:21: RangeError [ERR_OUT_OF_RANGE]: The value of "value" is out of range. It must be >= 0 and <= 255. Received 255.99136000000001
    at writeU_Int8 (internal/buffer.js:735:11)
    at Buffer.writeUInt8 (internal/buffer.js:745:10)
    at BuffaloZcl.writeUInt8 (/opt/zigbee2mqtt/node_modules/zigbee-herdsman/dist/buffalo/buffalo.js:30:21)
    at BuffaloZcl.write (/opt/zigbee2mqtt/node_modules/zigbee-herdsman/dist/buffalo/buffalo.js:206:18)
    at BuffaloZcl.write (/opt/zigbee2mqtt/node_modules/zigbee-herdsman/dist/zcl/buffaloZcl.js:303:26)
    at ZclFrame.writePayloadCluster (/opt/zigbee2mqtt/node_modules/zigbee-herdsman/dist/zcl/zclFrame.js:144:21)
    at ZclFrame.toBuffer (/opt/zigbee2mqtt/node_modules/zigbee-herdsman/dist/zcl/zclFrame.js:74:18)
    at ZStackAdapter.<anonymous> (/opt/zigbee2mqtt/node_modules/zigbee-herdsman/dist/adapter/z-stack/adapter/zStackAdapter.js:317:163)
    at Generator.next (<anonymous>)
    at /opt/zigbee2mqtt/node_modules/zigbee-herdsman/dist/adapter/z-stack/adapter/zStackAdapter.js:27:71

Running the following...

mosquitto_pub -t zigbee2mqtt/office/light/desk/bulb/set/color -m '{"hue": 0, "saturation": 100}' ; sleep 3 ; mosquitto_pub -t zigbee2mqtt/office/light/desk/bulb/set/color -m '{"hue": 0, "saturation": 107}' ; sleep 3; mosquitto_pub -t zigbee2mqtt/office/light/desk/bulb/set/color -m '#FF0000'

Makes it go from off red, nearly dark red, dark red

The difference between the last 2 is very very tiny and I usually don't even notice it... I wonder if some how our saturation calculation is wrong for non enhancedHue ?

sjorge commented 3 years ago

Looks like utils.gammaCorrectHSV() is what is causing it, if I skip this step for the RB 250 C the colors look very close to there RGB values when set via XY, with that call colors seems slightly off.

Edit: multiplying by 2.55 instead of 2.54 (with gammaCorrectHSV still) yields even closer results for hue+sat vs xy.

sjorge commented 3 years ago

ZCL seems to hint that 2.54 is indeed correct here...

0x0001 CurrentSaturation uint8 0x00 – 0xfe RPS 0x00 M0
Koenkk commented 3 years ago

Changing https://github.com/Koenkk/zigbee-herdsman-converters/blob/c9ed6bb75a9682cf60b41e4e7647c653480f9a6c/converters/toZigbee.js#L735 to value.saturation = Math.min(255, hsv.s * (2.54)); should fix the error

sjorge commented 3 years ago

The issue seems to be the reverse, it’s never 254 so all colors, at least on the RB 250 C, and most notably red, seem to be off visually.

When using hue/sat.

~ sjorge

On 3 Jan 2021, at 20:39, Koen Kanters notifications@github.com wrote:

 Changing https://github.com/Koenkk/zigbee-herdsman-converters/blob/c9ed6bb75a9682cf60b41e4e7647c653480f9a6c/converters/toZigbee.js#L735 to value.saturation = Math.min(255, hsv.s * (2.54)); should fix the error

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

Koenkk commented 3 years ago

I mean it fixes: RangeError [ERR_OUT_OF_RANGE]: The value of "value" is out of range. It must be >= 0 and <= 255. Received 255.99136000000001

sjorge commented 3 years ago

Oh like that, yeah but thats because I was setting it > 100... because I noticed when I use 100 the value was not set to the max range ZCL defined... hence trying 101, 102,... until 108 throws the out of range.

101->107 work... which is because after passing through gammaCorrectHSV... the value is lower.

107 seems to be 254 (which is the max, not 255 btw)... so I'm wondering if we need the util.gammaCorrectHSV when using hue/sat... as without it the colors more close match what they should be.

e.g. hue=0,sat=100 is red, while with gammaCorrectHSV it is more paleish-red

So yeah value.saturation = Math.min(254, hsv.s * (2.54)); would prevent us going out of range. (I'll do a PR for that once I figure out why the gammaCorrectHSV is needed)

sjorge commented 3 years ago

I spend most of the week looking at this and there is a clear trend...

I only tested tint, innr, and hue as those are in active use.

Hue Innr Tint
scene_add (xy) Looks off Looks good Looks good
set (xy) Looks good Looks off (expect red) Looks off

scene_add (xy) image

set color (xy) image

It doesn't show up very well on picture, but in person it's clear in this case the blue LEDs are on.

I did find references to the gamma correct in Hue's developer docs, so perhaps this is only needed for Hue?

I did some more testing:

gamma correction added to scene_add Hue Innr Tint
scene_add (xy) Looks good Looks off Looks off
set (xy) Looks good Looks off Looks off
gamma correction remove everywhere (also applyRedFix) Hue Innr Tint
scene_add (xy) Looks off Looks good Looks good
set (xy) Looks off Looks good Looks good

I'm not sure what the right course of action here is...

  1. keep as is (different behavior) - not work needed, super easy
  2. add gamma correction to scene_add - easy, consistent, but ugly colors for !Hue
  3. add gamma correction to scene_add & only apply it for Hue bulbs in both - harder, consistent, better colors, we can drop applyRedFix, different behavior than before for !Hue
  4. same as 3 & only skip it for !Hue if legacy=false, correct colors, clear path forward
  5. same as 2 & add a device_option to skip it, give users control
sjorge commented 3 years ago

@Koenkk I'd like to tackle this one next, my preference is for either 4 or 5... any objections to either of those?

Koenkk commented 3 years ago

TBH I'm a bit confused by this issue. The issue started with off colours when setting via hue/saturation but in https://github.com/Koenkk/zigbee-herdsman-converters/issues/2017#issuecomment-761226518 you mentioned x/y. Also gamma correction is only applied to hue/saturation not x/y. Note that the red fix does not fix off red colors, but fixes setting {x: 0.701, y: 0.299} doing nothing at all (https://github.com/Koenkk/zigbee-herdsman-converters/blob/4f4679631bbef485ccbb6132365a69837e13d4d0/converters/toZigbee.js#L976).

Could you summarise what the problem is here?

sjorge commented 3 years ago

I’ll try to clarify later, but it stated with me noticing it with hue/sat.

But it ended up being for both hue/sat, xy, hex,... with any way to set it. Then I got confused as sometimes they looked good. Then I did covered it was because scene_add does not have the gamma correction so colors looked correct when using scenes.

It’s one of those goos chases that leads you down one bit rabbit hole. I’ll close this issues when I am on my computer and file a newer one... or well maybe 2 as I think there are two parts to this.

~ sjorge

On 22 Jan 2021, at 16:36, Koen Kanters notifications@github.com wrote:

 TBH I'm a bit confused by this issue. The issue started with off colours when setting via hue/saturation but in #2017 (comment) you mentioned x/y. Also gamma correction is only applied to hue/saturation not x/y. Note that the red fix does not fix off red colors, but fixes setting {x: 0.701, y: 0.299} doing nothing at all (https://github.com/Koenkk/zigbee-herdsman-converters/blob/4f4679631bbef485ccbb6132365a69837e13d4d0/converters/toZigbee.js#L976).

Could you summarise what the problem is here?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

sjorge commented 3 years ago

Perhaps splitting 2128 into two seperate issues would be better even still. I did a bit more retesting and I don't think either solutioned mentioned here are OK so just ignore this entire issue now.