[hue] Improve support for third party lights - #21062
Conversation
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
andrewfg
changed the title
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
andrewfg
changed the title
andrewfg
changed the title
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
- harmonize contract between handler and resource - log stack trace if critical field is missing - fix brightness and on/off handling - fix test cases for brightness and on/off and missing critical field - apply full dto's for min dim level, mirek schema, gamut - change color transforms to use actual gamut if available Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
AI re-review
AI re-reviewed the current head 5bf1fffe387802a8dcd7e36964545db8c906d744 after the latest delta-command revert. The previous INCREASE / DECREASE findings have been addressed and are no longer applicable.
Changes are still requested for the following regression risks:
- The optimistic loopback is currently triggered for every successful response containing an
errorsentry, rather than only for the known benignattribute_may_have_no_effectcase. This can publish a requested state even when an attribute was actually rejected. - The recursive room/zone workaround detection adds per-child HTTP GETs during initialization and can repeatedly fetch the same devices through different room/zone/home hierarchies.
- The existing unresolved transition-duration concern still applies: #21062 (comment). The new room/zone propagation makes its scope broader because a single affected child can cause grouped OFF commands to include
dynamics.duration = 0, changing the transition behavior of unaffected lights in the same group.
The updateLightCacheRequiredFields() documentation and corresponding PR description are also stale after the recent refactoring. The remaining delta DTO/classes and setters appear unused after the revert and could be removed as non-blocking cleanup.
The LegacyLightState implementation looks internally consistent, but targeted hardware regression testing is still advisable for CT → color, color → CT, repeated XY notifications, restart/power-cycle behavior, and commands while soft-off.
A human maintainer review is still needed after the issues above are addressed.
jlaur
left a comment
jlaur
left a comment
There was a problem hiding this comment.
A few very minor comments/questions in addition to the AI comments.
I have verified that #21057 is fixed and that the LK Wiser work-around still works.
I have not tested anything related to color temperature, i.e. that the other linked issues are fixed. Hopefully @kaikreuzer can confirm that the scenarios he experienced are now as expected.
jlaur
changed the title
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
AI re-review
AI re-reviewed the current head 388f53b63aa602fe86aebf9d3eeb912cf2ac8599.
The latest changes address the previous repeated per-child HTTP lookup concern and substantially improve the 207 error handling. The earlier INCREASE / DECREASE findings also remain addressed after those changes were reverted.
Changes are still requested for the three issues noted inline: mixed 207 errors can still trigger an optimistic loopback, fixed-width Mirek schemas can cause a zero-denominator percentage calculation, and the off-transition work-around ID set is concurrently rebuilt and read without safe publication.
The existing unresolved TRÅDFRI transition-duration concern still applies, and point 8 of the PR description should still be updated because updateLightCacheRequiredFields() now fills the Mirek schema and gamut, but not a minimum dimming level.
A human maintainer review is still needed after these issues are addressed.
I have no idea what it is talking about. So ignoring it. |
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Now you are ignoring @jlaur 😉 |
Well that is AFAIK resolved, so the AI was wrong. |
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
AI re-review
The latest changes address the previously raised 207 handling, startup lookup, and workaround-set concurrency concerns.
One remaining correctness issue was found in the refresh of the OFF-transition workaround IDs: an unsuccessful refresh can still publish a partial result and discard the previous known-good state.
There is also one narrower question about the TRÅDFRI 1055l workaround. The device testing indicates that dynamics is required for combined on:false + dimming requests, while a plain on:false succeeds without it. The current implementation also adds dynamics to plain OFF requests. This may be intentional for grouped resources, so this is raised as a question rather than as a confirmed regression.
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
|
Great! It's functionally correct now. 👍 |
jlaur
left a comment
jlaur
left a comment
There was a problem hiding this comment.
LGTM. I would still like to hear if this resolves @kaikreuzer's color temperature issues, as these fixes accounts for most of the changes made here.
| : "%s .. %s".formatted(toKelvin(mirekMinimum), toKelvin(mirekMaximum)); | ||
| } | ||
|
|
||
| public static int toMirek(double kelvinValue) { |
There was a problem hiding this comment.
Minor note: This appears to be unused. Left-over or on purpose?
Indeed. Please don't merge until @kaikreuzer has tested. |
jlaur
added
the
additional testing preferred
|
I'll try to test asap! |
|
@andrewfg I've just tested the latest version - unfortunately with no improvements over the last time. 😢 Let me know if you need anything else for debugging - thanks for your efforts! |
@kaikreuzer I lost track of what particular error applied to what particular light, so "no improvements" does not help me much. => could you please say precisely what is not working? In the meantime, I can see from the logs the following:
What is the exact model of this device? it may help if I can buy one on eBay and test it myself |
|
Sorry @andrewfg for not providing the details, I am aware that you are addressing a lot of different issues with this PR. My issue that I am still looking to be solved is the one I described here and the rule is the one I provided there: #21018 (comment) The problem is still that after changing CT while the light is (soft-)off, it is not considered when it is turned on. |
Many thanks in advance. Please try with say 10 seconds interval in your script rather than 1 second. |
This PR resolves many issues with "exotic" light models. In this context "exotic" mostly means lights made by third parties other than Philips / Signify ("Friends of Hue") which are therefore not certified for full compliance with the Hue API v2. Albeit it covers a special case with the Philips / Signify "Signe" model as well.
For testing purposes the JAR is here:
Specifically this PR does the following:
In the prior version of the binding code I had wrongly assumed that the min .. max range of the dimming channels were "minimum dimming level .. 100%). This PR refactors the code so the min .. max range of the dimming channels are now "0% .. 100%" regardless of the actual minimum dimming level.
Therefore, this PR changes the threshold for a light being deemed ON from
brightness >= minimum dimming leveltobrightness > 0.0.Some non certified lights fail to provide a minimum dimming level anyway - see [hue] IKEA Trådfri bulb turns off when setting low brightness #21057 So with 2. above, this PR resolves this issue.
Some certified lights do provide a minimum dimming level of 0.0 (e.g. Philips Signe) and currently therefore sending 0.0 to the bulb fails to turn it off. So with 2. above (changing
b >= 0.0check tobri > 0.0) this PR ensures that a precise 0.0 command will turn the lamp off.Some non certified lights fail to provide minimum and maximum supported Mirek values. So analogous to the case with dimming level above, this PR defaults to the min .. max range of 153Mk .. 500Mk.
Previously the binding did not use the Gamut provided by certified lights. It always used the default Gamut instead. As a general rule Philips / Signify lights do provide a Gamut, whereas non certified products generally do not. With this PR now we use the Gamut if provided, and only use the default if none is provided.
Some non certified lights provide invalid minimum and maximum supported Mirek values. So this PR checks those values and if necessary defaults to the min .. max range of 153Mk .. 500Mk.
As a consequence of the above changes, the thing handler calls a new extra added method that ensures the light state cache contains all the required parameters for mirek schema, and gamut -- be they provided by the device itself in its initial GET response, or otherwise using the system defaults.
Often non certified lights will respond to a PUT request with an HTTP 207 error rather than HTTP 200. In the past, when the bridge returned an HTTP 207 response "soft off" the binding would produce an
INFOlog output. This PR reduces that toDEBUGlevel.On some non certified lights, when the bridge returned an HTTP 207 response, the devices would not send an SSE event update with the new state. This PR adds a loopback whereby the JSON of the PUT request that caused the 207 response is looped back to the binding as a dummy event. This prevents the states in the UI from "flapping".
As a consequence of my misunderstanding about the min .. max range of the dimming channels, the binding was incorrectly calculating the threshold to be used when determining the changeover point between sending a hard on or a hard off command. This applied in particular to non certified products that do not provide their own minimum dimming level. This PR adds an extra
Settersmethod that fixes the threshold and implements the hard on/off logic.The prior version of the binding code would wrongly produce Channel State values of
UNDEFin some cases when certain DTO fields were missing. Again in particular it occurred for non certified products. In this PR someUNDEFcases are eliminated, and instead an exception may be thrown at runtime (see next point)... therefore, in this PR now when a device delivers a DTO with incomplete data, rather than producing the opaque
UNDEFstate, the binding now logs the detailed error with a full stack trace instead. The extra transparency should help fixing future non certified light models.In order to test the above changes I purchased both a Lidl Livarno TS0502A E27 CT bulb that should be similar to the TS0505A of @kaikreuzer and also an Ikea Tradfri 1055 lumen E27 bulb that should be similar to the Tradfri lamp of @jlaur, for testing the code. And as far as I can tell the errors reported in the below mentioned issues have now been resolved.
It seems that the (also non certified) "Ikea Tradfri" bulbs suffer the same bug as the "LK Wiser Dimmer". So this PR extends the same fix to those Tradfri bulbs.
Furthermore, if a room or zone contains such a Tradfri or LK Wiser light, then the bug also occurs on the respective room or zone. So this PR applies the same fix to such things. It also applies the fix to the "all lights zone".
Some older (legacy) lights operate in a dual mode manner. Either they are in 'CT' mode, or 'XY' mode. By contrast the API v2 is designed to operate in a mode-less manner. In such mode-less operation the bridge and respective lights provide notifications containing both colour temperature and color xy values which are simultaneously valid. Whereas older legacy lights provide notifications where EITHER the colour temperature OR the color xy value is valid. And such notifications may be interleaved, thus causing flapping between channel states. So this PR introduces a new
LegacyLightStateclass that implements a state machine to model the the operating mode of such lights, and apply a work-around to processes the notifications to only present values that belong to the correct legacy mode.The Junit tests have been adapted to the above. And a new test class has been introduced to test the
LegacyLightStatestate machine class.Resolves #21057
Resolves #21044
Resolves #21045
Resolves #21043
Resolves #21011
Signed-off-by: Andrew Fiddian-Green software@whitebear.ch