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

[hue] Improve color setting #16436

Merged
merged 10 commits into from Apr 3, 2024
Merged

[hue] Improve color setting #16436

merged 10 commits into from Apr 3, 2024

Conversation

andrewfg
Copy link
Contributor

@andrewfg andrewfg commented Feb 21, 2024

There are three issues..

1) Hue bulb changing color wrongly when brightness is 0

As reported on the forum here if OH sends an HSBType color command, where B is 0, it causes the wrong color value to be set on the lamp.

The reason is that the ColorUtil.hsbToXy() method produces, for the special case where B is 0, an xy color co-ordinate of [0,0] which is outside the color gamut of any physical lamp; and this (wrongly) causes the gamut correction to convert that [0,0] value to the closest xy color co-ordinate within the actual lamp's color gamut i.e. namely it sets the lamp to its gamut:blue color.

2) Use same color Gamut as Hue App

See this #16482

3) RGB gamma correction is inconsistently applied

See core #4102

When the binding sends a color XY command to the bridge, it applies a variable gamma correction depending on the dimming level of the lamp. Whereas when the bridge sends an XY state update to the binding, it applies a fixed reverse gamma correction that does not depend on the dimming level. This means that the round trip HSB => xy => bridge => xy => HSB has inconsistent results particularly at low brightness levels.


This PR fixes all these issues (resolves #16482). (Plus some modernizations of instanceof syntax).

The Jar file in Zip form is here

Signed-off-by: Andrew Fiddian-Green software@whitebear.ch

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg andrewfg added the bug An unexpected problem or unintended behavior of an add-on label Feb 21, 2024
@andrewfg andrewfg requested a review from a team February 21, 2024 18:38
@andrewfg andrewfg self-assigned this Feb 21, 2024
@andrewfg andrewfg added work in progress A PR that is not yet ready to be merged and removed work in progress A PR that is not yet ready to be merged labels Feb 22, 2024
@andrewfg andrewfg requested a review from jlaur February 24, 2024 00:37
@andrewfg
Copy link
Contributor Author

andrewfg commented Mar 2, 2024

See also openhab/openhab-core#4124

@andrewfg andrewfg requested a review from kaikreuzer March 2, 2024 19:29
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/hue-bulb-changing-color-wrongly-when-brightness-is-0/154005/18

@andrewfg andrewfg added the work in progress A PR that is not yet ready to be merged label Mar 8, 2024
@andrewfg
Copy link
Contributor Author

andrewfg commented Mar 8, 2024

Changed to WIP pending #16482

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

@jlaur we seem to have reached a consensus on #16482 so I will update this PR to include those changes, and mark it again as ready for review..

@andrewfg andrewfg removed the work in progress A PR that is not yet ready to be merged label Mar 12, 2024
@andrewfg andrewfg changed the title [hue] Fix setting the wrong color when HSB brightness is 0 [hue] Improved color setting commands Mar 12, 2024
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/hue-bulb-changing-color-wrongly-when-brightness-is-0/154005/26

@andrewfg
Copy link
Contributor Author

andrewfg commented Mar 12, 2024

See this https://community.openhab.org/t/hue-bulb-changing-color-wrongly-when-brightness-is-0/154005/28?u=andrewfg


EDIT: I am wondering if the xy should be constructed from an HSB with a fixed B? reason is that the dimmer data is anyway separately transmitted on a different DTO..


EDIT 2: to answer my own question: it should not use a fixed B. Reason is that the Gamma Correction needs to be applied. However I think there may be another bug in the Hue binding whereby reverse gamma correction is being omitted. I will check it..


EDIT 3: it’s 4:30 AM and I had an epiphany. Marking this now as WIP again..

@andrewfg andrewfg added the work in progress A PR that is not yet ready to be merged label Mar 14, 2024
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

Ok. I have now also fixed the gamma round trip issue. Ready to go again. :)

@andrewfg andrewfg removed the work in progress A PR that is not yet ready to be merged label Mar 15, 2024
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/hue-bulb-changing-color-wrongly-when-brightness-is-0/154005/31

@andrewfg andrewfg added the work in progress A PR that is not yet ready to be merged label Mar 16, 2024
@andrewfg andrewfg added rebuild Triggers Jenkins PR build and removed work in progress A PR that is not yet ready to be merged rebuild Triggers Jenkins PR build labels Mar 17, 2024
@lolodomo lolodomo added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Mar 17, 2024
@andrewfg andrewfg added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Mar 17, 2024
@andrewfg andrewfg requested a review from a team March 21, 2024 15:16
@andrewfg andrewfg changed the title [hue] Improved color setting commands [hue] Improved color setting Mar 21, 2024
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Only minor comment from my side to remove "TODO" code smells.

Is it correct that this PR no longer has any direct relation to openhab/openhab-core#4124 (besides the tests that might be affected), but completely independently fixes issues in the binding that would have to be fixed the same way with or without openhab/openhab-core#4124?

@kaikreuzer - would you like to complete your review after the latest changes and simplifications?

@jlaur jlaur changed the title [hue] Improved color setting [hue] Improve color setting Mar 21, 2024
@andrewfg
Copy link
Contributor Author

andrewfg commented Mar 21, 2024

Is it correct that this PR no longer has any direct relation to openhab/openhab-core#4124 (besides the tests that might be affected), but completely independently fixes issues in the binding that would have to be fixed the same way with or without openhab/openhab-core#4124?

Correct.

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg andrewfg requested a review from jlaur March 22, 2024 11:22
@andrewfg
Copy link
Contributor Author

andrewfg commented Apr 2, 2024

@kaikreuzer I believe that @jlaur is deferring until you complete your own review of this. So in order to make progress, I wonder if there is any further information that you need from my side, in order to help that?

@kaikreuzer
Copy link
Member

@kaikreuzer - would you like to complete your review after the latest changes and simplifications?

@jlaur All fine for me; so feel free to merge if you are fine with it as well.

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM

@jlaur jlaur merged commit 8687fec into openhab:main Apr 3, 2024
3 checks passed
@jlaur jlaur added this to the 4.2 milestone Apr 3, 2024
lo92fr pushed a commit to lo92fr/openhab-addons that referenced this pull request Apr 30, 2024
* [hue] fix xy conversion when B is 0
* [hue] remove gamut correction; let Hue bridge do it instead
* [hue] fix gamma round trips; modernize instanceof syntax

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
adr001db pushed a commit to adr001db/openhab-addons that referenced this pull request May 12, 2024
* [hue] fix xy conversion when B is 0
* [hue] remove gamut correction; let Hue bridge do it instead
* [hue] fix gamma round trips; modernize instanceof syntax

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Alexander Drent <Alex@Drent-ict.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[hue] OH binding should use same color Gamut as Hue App
5 participants