-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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] Wrong additional channel update after sending command (API v2) #15700
Comments
@jlaur is this issue just limited to LK Wiser products? (I don't have any such products..) |
Actually that didn't strike my mind. I provided only that example because my "work-around rule" magnifies the problem, so this is how I fully discovered it. Before then I just had this strange impression that something was weirdly flickering when using sitemaps dimmers. I have now made a first test with another device, a Hue White and color ambiance GU10, EAN 8719514339880. I linked the
It's slightly different because it changed back to 0.4 rather than 0.0, but otherwise it seems to be the same issue. It's worth noting that there is some randomness involved - it only happened on second attempt. Usually it's quite reproduceable when trying a few times. |
It sounds that you have a UI page open when you make the change? I have seen previously cases where the SSE event updates the UI page with a rounding error, and the UI page then sends another command to correct the rounding error. So I suggest you try what happens also when no UI page is open, and the command is issued by a rule. |
I closed my browser to be sure, and repeated the test with the same result. |
@andrewfg - have you tried also? I would be a little surprised if you are not able to reproduce this at all. If you cannot, then that bit of information might also give a clue, i.e. how to isolate it. |
I will try tomorrow. |
On a closer look, this seems to be related to the handling of this payload: [
{
"creationtime": "2023-10-05T19:32:40Z",
"data": [
{
"id": "00000000-0000-0000-0000-000000000001",
"id_v1": "/lights/40",
"on": {
"on": true
},
"owner": {
"rid": "00000000-0000-0000-0000-000000000000",
"rtype": "device"
},
"type": "light"
},
{
"dimming": {
"brightness": 100.0
},
"id": "00000000-0000-0000-0000-000000000001",
"id_v1": "/lights/40",
"owner": {
"rid": "00000000-0000-0000-0000-000000000000",
"rtype": "device"
},
"type": "light"
}
],
"id": "00000000-0000-0000-0000-000000000002",
"type": "update"
}
] Both resources are of type light, but only the last one contains dimming. Perhaps the first one is incorrectly handled as well with initial values, i.e. brightness considered as 0? Based on the logs and payload, I think the flow goes like this... the two resources above are received here: Lines 530 to 540 in 45760bf
We end on this line: Line 661 in 45760bf
Then here: Lines 861 to 881 in 45760bf
Then I'm a bit unsure what is happening, because I would have assumed that Lines 164 to 183 in 45760bf
|
A couple of general comments..
|
@andrewfg - ah, thanks, now I finally get it. I noticed Lines 658 to 661 in 59c3135
This obviously triggers the issue I'm seeing when receiving the two mentioned objects. The first one only containing: "on": {
"on": true
}, will be merged with the current brightness which is still 0 because the next object hasn't been processed yet, and the command is not taken into consideration for this particular logic (it's not included in the cache). Regarding type of light: LK Wiser Dimmer as well as the Hue bulb mentioned here: #15700 (comment) However, while it may influence reproduction, the payload posted is delivered by the bridge, so we have to be able to process it correctly no matter the lights or firmware version. And it seems quite doable. The only concern I have is how generic we can fix it. In this specific case, I believe it would almost have worked if we didn't merge anything from the cache before calling In general, without considering implementation details yet, in which cases is it important to merge with cached data, and in which cases would it be sufficient to process only received data and update only channels for the provided sparse data? In this case it might even make sense to merge those two {
"id": "00000000-0000-0000-0000-000000000001",
"id_v1": "/lights/40",
"on": {
"on": true
},
"owner": {
"rid": "00000000-0000-0000-0000-000000000000",
"rtype": "device"
},
"type": "light"
},
{
"dimming": {
"brightness": 100.0
},
"id": "00000000-0000-0000-0000-000000000001",
"id_v1": "/lights/40",
"owner": {
"rid": "00000000-0000-0000-0000-000000000000",
"rtype": "device"
},
"type": "light"
} merged into: {
"id": "00000000-0000-0000-0000-000000000001",
"id_v1": "/lights/40",
"on": {
"on": true
},
"dimming": {
"brightness": 100.0
},
"owner": {
"rid": "00000000-0000-0000-0000-000000000000",
"rtype": "device"
},
"type": "light"
} and then merged with the cache would probably produce the correct result. However, it would be only a partial fix, as it would still not work if we received those two data packets separately with a few milliseconds between them. |
Yeah. The two event 'slices' could arrive in any of three ways..
FWIW I have only Philips lamps (of various varieties) and as a general rule case 1. is by far the most common. |
@jlaur thinking about this further, I am not even sure that I would class this behaviour as a bug. Rather I would class it as a simple fact. As discussed quite frequently and heatedly during the development process, the various state attributes on-off, dimming, color-xy and color-temperature are orthogonal .. that is to say, principally, that each such attribute can change state without influencing the other. (The exception is color-xy and color-temperature where there IS a cross linkage due to color temperatures being a limited arc of values on the color XY gamut). As I stressed before, the real problem is within OH when one tries to map these orthogonal state attributes into a single HSB entity. And as I said before, this is the reason why I fought so strongly to maintain the (orthogonal) "abc-only" channels. Facit: beware of trying to "fix" a problem that is OH self created -- you may end up just making the self created problem worse! |
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
There is one more issue. If the events are received apart: [
{
"creationtime": "2023-11-30T22:41:08Z",
"data": [
{
"id": "00000000-0000-0000-0000-000000000001",
"id_v1": "/lights/40",
"on": {
"on": true
},
"owner": {
"rid": "00000000-0000-0000-0000-00000000000",
"rtype": "device"
},
"type": "light"
}
],
"id": "00000000-0000-0000-0000-000000000009",
"type": "update"
}
] and: [
{
"creationtime": "2023-11-30T22:41:25Z",
"data": [
{
"dimming": {
"brightness": 100.0
},
"id": "00000000-0000-0000-0000-000000000001",
"id_v1": "/lights/40",
"owner": {
"rid": "00000000-0000-0000-0000-000000000000",
"rtype": "device"
},
"type": "light"
}
],
"id": "00000000-0000-0000-0000-000000000009",
"type": "update"
}
] We will have the same issue when the current dimming is 0. Example:
So what can we do about this? When we receive on=on, we should assume brightness > 0, since otherwise HSB will be off, and we have an inconsistent state (it can't be both on and 0). |
By pure physics, a lamp cannot be both on and have brightness = 0. If it has transitioned from off and B=0 to on, (with B not yet defined), then probably B = the minimum dimming level for the particular lamp. |
If you are fine with fixing this issue, we could include it in #15905 - WDYT? The only thing is which value to use. Brightness is actually missing - it's undefined, so we don't know the actual value, we only know that it must be > 0. We could use minimum dimming as you suggest, but we could also use 100%. In real life I have only observed the latter. When I turn on the lamp either from an openHAB command or from a physical button, it turns on with 100% brightness, and the messages received are pasted above. This happens sometimes. Other times, I get both events in the same packet (i.e. fixed by the merge). I can also turn it on to a specific dimming level. In this case it wouldn't be 100%, but it wouldn't be minimum dimming level either. So it's not possible to correctly predict the brightness, we can only guess. |
I have no problem if you assume B = 100. The actual B value would come via SSE anyway I suppose. And yes, let us add this to the PR merge function |
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes #15700 Signed-off-by: Jacob Laursen <[email protected]>
Fixes openhab#15700 Signed-off-by: Jacob Laursen <[email protected]> Signed-off-by: Jørgen Austvik <[email protected]>
For this thing:
and this item:
when performing this command:
the following scenario plays out:
Summary to highlight the issue:
Although it ends with the correct state, this additional update is a problem. For example, it will cause unneeded/confusing persisted values. It also causes some strange flickering when using a slider in a sitemap to control the light.
Expected Behavior
I would expect to see only the relevant update setting
brightness
to 100.Current Behavior
brightness
is updated to 0 after being commanded to 100, then set back to 100.Possible Solution
I have not looked into the code yet, but might update this issue unless someone will find the issue before me. 😉
Steps to Reproduce
See description.
Context
I have this rule to trick the Wiser dimmer into turning off when the Hue app turns it off without transition time (more details here):
This will be triggered by the excess update, and as a result, this rule will not work:
Your Environment
The text was updated successfully, but these errors were encountered: