[hue] Fix multiple state updates (API v2)

[jlaur]

The overall goal of this pull request is to address issues with unneeded state updates that can happen under certain circumstances.

This can cause:

• Flickering in UI's.
• Misbehavior from rules (see chapter Context in [hue] Wrong additional channel update after sending command (API v2) #15700).
• A bit of a mess in persisted values.

Three root causes have been identified:

• Multiple messages wrapped in one event are processed sequentially (one by one), causing intermediate state updates.
• State sent as multiple events where dimming information is missing in first event. This has been extracted to [hue] Brightness handling is inconsistent for some devices #15995.
• Auto update policy is default, so commands will trigger state update immediately rather than awaiting the final state from the bridge. This fix for this has been extracted to [hue] Use auto update policy 'veto' for light channels (API v2) #15984.

Given an initial brightness of 0 and this command:

openhab> openhab:send Brightness 100
Command has been sent successfully.

Yielding this payload:

[
	{
		"creationtime": "2023-10-05T19:32:40Z",
		"data": [
			{
				"id": "00000000-0000-0000-0000-000000000001",
				"id_v1": "/lights/1",
				"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/1",
				"owner": {
					"rid": "00000000-0000-0000-0000-000000000000",
					"rtype": "device"
				},
				"type": "light"
			}
		],
		"id": "00000000-0000-0000-0000-000000000002",
		"type": "update"
	}
]

will result in the following events:

2023-11-26 00:14:12.970 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'Brightness' received command 100
2023-11-26 00:14:12.971 [INFO ] [penhab.event.ItemStatePredictedEvent] - Item 'Brightness' predicted to become 100
2023-11-26 00:14:12.972 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'Brightness' changed from 0 to 100
2023-11-26 00:14:13.085 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'Brightness' changed from 100 to 0
2023-11-26 00:14:13.086 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'Brightness' changed from 0 to 100
2023-11-26 00:14:13.086 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'Switch' changed from OFF to ON

The reason for the state update back to 0 is that this part of the message is fully processed with channel update:

				"on": {
					"on": true
				},

before processing the next message:

				"dimming": {
					"brightness": 100.0
				},

This means that the already cached brightness will be applied rather than the brightness received in the separate message. The final state will be correct, but an additional update is made.

Testing

In addition to the tests documented above, some manual regression tests performed:

• Motion sensor channels are working.
• Brightness can be changed for Thing with Zigbee connectivity issue.
• Thing is updated according to Zigbee connectivity status.

Fixes #15700
Related to #15983

[andrewfg]

@jlaur I only just noticed this PR today. At first glance I would say that this is NOT a solution. Because I think there is actually NOT a problem that could or should be 'solved' within OH. As mentioned elsewhere the Hue system handles different DTOs orthogonally, and IMHO the binding must also handle them orthogonally. And attempting to munge one or more orthogonal DTOs into a common state change is not correct.

[andrewfg]

^
Specifically I can think of several hard examples where DTO munging would be most dangerous..

• Push buttons risk to lose the individual button down, button hold, button up events
• Security and tamper contacts risk to lose intermediate alarm states
• Motion detectors ditto.
• Rotary dials (where each event represents number of clicks offset from the prior event) risk to lose intermediary clicks.

Also as said before there are three ways that the bridge can send multiple events .. namely

1. Combine two DTOs in one event
2. Pack two DTOs into two sequential packets in the same event stream
3. Send two DTOs one after another in two sequential event streams

Your 'solution' only tries to address 2. above.
Push buttons, security devices, and rotary dials use sometimes 2. sometimes 3. and sometimes even both..

[andrewfg]

^
We also can't rule out crossover issues with scenes and effects when one scene/effect is deactivated by another being activated.

[jlaur]

Hi @andrewfg, thanks for your feedback. Let me first make it clear that the provided draft is only in PoC/experimental state. I shared it to show that I'm working on this, and to possibly get early feedback - it worked. 😉 All your comments will be considered and addressed.

Your good points about different types of events are quite important. It's clear to me now that this fix should be limited in scope to specifically address problems related to color, brightness and on/off, i.e. values that are separated on the API side and combined into the HSBType in our openHAB model.

1. Combine two DTOs in one event

This is already working correctly.

2. Pack two DTOs into two sequential packets in the same event stream

This is what I aim to fix with this PR.

3. Send two DTOs one after another in two sequential event streams

I don't think there is anything to fix here, i.e. this is also working correctly already. When we receive completely separate events, we'll naturally (fully) process them one by one as we receive them.

Going back to the issue... when we receive this payload (payload A):

[
	{
		"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"
	}
]

the bridge is telling us:

1. Light 40 is on.
2. Light 40 brightness is 100%.

These are separate attributes, but we need to merge them into one HSBType on our side. We do that already taking consideration of the already cached values. Example:

1. Initial state: OFF, blue, brightness 50%.
2. HSBType: blue, brightness 0% (or black, not sure).
3. Cached: OFF, blue, brightness 50%.
4. Event: ON
5. State update HSBType: ON (received), blue (from cache), brightness 50% (from cache)
6. Cached: ON

Now, if we would instead receive this payload (payload B) in step 4:

[
	{
		"creationtime": "2023-10-05T19:32:40Z",
		"data": [
			{
				"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"
			}
		],
		"id": "00000000-0000-0000-0000-000000000002",
		"type": "update"
	}
]

we could replay the example like this:

1. Initial state: OFF, blue, brightness 50%.
2. HSBType: blue, brightness 0% (or black, not sure).
3. Cached: OFF, blue, brightness 50%.
4. Event: ON, brightness 100%
5. State update HSBType: ON (received), blue (from cache), brightness 100% (received)
6. Cached: ON, brightness 100%

Therefore, I would clearly consider it a bug that payload A is currently handled like this when receiving a single event:

1. Initial state: OFF, blue, brightness 50%.
2. HSBType: blue, brightness 0% (or black, not sure).
3. Cached: OFF, blue, brightness 50%.
4. Event: ON
5. State update HSBType: ON (received), blue (from cache), brightness 50% (from cache)
6. Cached: ON
7. Event: Brightness 100%
8. State update HSBType: ON (from cache), blue (from cache), brightness 100% (received).
9. Cached: Brightness 100%

This causes flickering in the UI (because of two state updates), and can also cause issues in rules, like the one I have described in the linked issue.

And it can be mitigated by merging the attributes before merging with the cache, i.e. in some sort of pre-processor logic. This way we'll reach the same final state, but without the intermediate "temporary" state causing the mentioned issues. As mentioned in my introduction, it should only apply to color, brightness and on/off.

[andrewfg]

in some sort of pre-processor logic.
it should only apply to color, brightness and on/off.

Ok. That makes sense. But I would ask that the abcOnly channels should retain the original (non pre-processed) logic.

Maybe just to point out one thing (in case you overlooked it): in your examples above you imply that the 'cache' is the channel HSBType value, which is not strictly true .. the cache actually comprises an XY pair, a Dimming, and an on-off attribute .. and as you imply (particularly when 'B' is close to zero, or when 'XY' is close to the edge of the Gamut) there will be round trip conversion issues between 'XY' and 'HS' .. and perhaps also between 'D', 'B', and on-off (particularly when 'D' is close to the minimum dimming level) .. so I advise that the pre-processing should cache and merge the 'XYDO' values (rather than the HSBType value) i.e. you should only make a one way final conversion 'XYDO' => 'HSB' when you actually set the HSBType on the channel.

[jlaur]

@andrewfg - are you still providing backports to 3.4.x?

[andrewfg]

are you still providing backports to 3.4.x?

No.

[jlaur]

@wborn - does the integration test failure mean anything to you?

EDIT: Sorry, I found the issue a few minutes after asking. That's how it works. 😉

[andrewfg]

@jlaur a few things..

1. what other PR are you waiting for?
2. apart from that, is this PR now ready for me to review?
3. it seems to be a lot bigger, than the original scope of the PR => do you want to change the title?

[jlaur]

• what other PR are you waiting for?

See last line of description. I found a bug causing auto update policy to be ignored when defined at channel level. This directly impacts this PR, unfortunately.

• apart from that, is this PR now ready for me to review?

Yes, finally. 🙂

• it seems to be a lot bigger, than the original scope of the PR => do you want to change the title?

The scope should be what the title currently says. The only thing is that the issue turned out to have two different root causes. I tried to explain the thoroughly in the description, but please let me know if you need anything else. The linked issue only mentions one of those root causes, but they both have similar consequences. Therefore I decided to extend the original scope.

It was a long way for me to get it to the current state, with a lot of testing and head banging, and also with a lot of changes now reverted. One thing that took me a while to understand is that all thing handlers receive all resources and maintain their own caches. Each has its own Map, but the contained Resource objects are actually shared. This lead to some problems until I realized that the pre-processing had to be done by the bridge before iterating through the handlers and sharing the modified list of resources. I was finally able to get everything working yesterday and reverted a lot of copy constructors etc. that are now outside the scope of this PR.

[jlaur]

Since I am not able to progress with openhab/openhab-core#3888, I have extracted the auto update policy part into #15984.

[andrewfg]

@jlaur I am a bit confused. You seem to have deleted some of your comments and my responses. (Probably a good thing). But can you please just update me on where you stand with the scope of this PR? I assume that you want to just handle the merge of multiple SSE events in this PR? And handle the other topic elsewhere? Is that correct? In which case I will do a final review of the PR on that basis. Is it all ready for my final review.

And concerning the other topic, how do you want to proceed? As before I am willing to propose some code if you like.

[jlaur]

@jlaur I am a bit confused. You seem to have deleted some of your comments and my responses. (Probably a good thing).

Not intentionally at least, but I resolved the comment we used for discussing today, since that is now a separate issue.

But can you please just update me on where you stand with the scope of this PR? I assume that you want to just handle the merge of multiple SSE events in this PR?

Yes.

And handle the other topic elsewhere? Is that correct? In which case I will do a final review of the PR on that basis. Is it all ready for my final review.

Yes, see #15905 (comment).

And concerning the other topic, how do you want to proceed? As before I am willing to propose some code if you like.

You are welcome to propose.

[andrewfg]

LGTM

[andrewfg]

You are welcome to propose.

Yes I prefer it that way. I mean no criticism, but the process seems more fruitful when I write the code, and you review for compliance, and test case resolution. It also means the merge process can be faster.

[andrewfg]

@jlaur I will create the PR tomorrow..

[jlaur]

@lolodomo - we have three different Hue bugfixes at the moment, all concerning some quite related symptoms with undesired state updates. #15984 is blocked by a dependency towards a bugfix in core. #15999 needs test and review, so also not ready. However, this one is ready. Any chance of a quick review? 😉 That would help testing and managing the combined fix which is really needed, and therefore would be much appreciated. Most of the lines are test coverage.

[kaikreuzer]

Thanks for reviewing @andrewfg!
I just did a quick sanity check and guess we can merge. 👍

[jlaur]

Thanks for the sanity check and merge, @kaikreuzer. 🙂