[hue] Fix scene channel updating when scene is changed via Hue App

[andrewfg]

Fixes #16000

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

[openhab-bot]

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

https://community.openhab.org/t/philips-hue-clip-2-api-v2-discussion-thread/142111/411

[andrewfg]

@jlaur for info, I will follow your test test case design from our other PR into this one..

#15999 (review)

[jlaur]

Thanks for the fix. I have added a few comments, most of which will probably be obsoleted by the changed test approach. 🙂

[jlaur]

@andrewfg - looking at this again as well as these methods:

openhab-addons/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/api/dto/clip2/Resource.java

Lines 637 to 665 in 5b98e41

	/**
	* Check if the scene resource contains a 'status.active' element. If such an element is present, returns a Boolean
	* Optional whose value depends on the value of that element, or an empty Optional if it is not.
	*
	* @return true, false, or empty.
	*/
	public Optional<Boolean> getSceneActive() {
	if (ResourceType.SCENE == getType()) {
	JsonElement status = this.status;
	if (Objects.nonNull(status) && status.isJsonObject()) {
	JsonElement active = ((JsonObject) status).get("active");
	if (Objects.nonNull(active) && active.isJsonPrimitive()) {
	return Optional.of(!"inactive".equalsIgnoreCase(active.getAsString()));
	}
	}
	}
	return Optional.empty();
	}
	/**
	* If the getSceneActive() optional result is empty return 'UnDefType.NULL'. Otherwise if the optional result is
	* present and 'true' (i.e. the scene is active) return the scene name. Or finally (the optional result is present
	* and 'false') return 'UnDefType.UNDEF'.
	*
	* @return either 'UnDefType.NULL', a StringType containing the (active) scene name, or 'UnDefType.UNDEF'.
	*/
	public State getSceneState() {
	return getSceneActive().map(a -> a ? new StringType(getName()) : UnDefType.UNDEF).orElse(UnDefType.NULL);
	}

I have a few questions:

• Is it correct that either the the Thing has exactly one active scene, and we need to name of that scene for the scene channel. Or it doesn't have an active scene, in which case the channel should be UNDEF?
• Each resource will be kept in sceneContributorsCache, both the active and the inactive one from the example provided in the linked issue?

If I read the code correctly, the new sequence (after sorting the resources) will now make us end up with the active scene rather than UNDEF. But it will instead change to UNDEF first - a few milliseconds before changing to the new scene?

When both arrive in the same SSE event, I guess we can consider that atomic, and rather the swapping the order, perhaps we should actually discard the inactive scene when there is also an active scene. This, I guess, would result in a direct state update from the previously active scene to the new active scene. Changing to UNDEF should only be needed when there is no longer an active scene.

I'm not enough into scenes to understand how sceneContributorsCache is utilized, but I guess it needs to be kept updated, so probably we can't really "discard" the resource in the bridge handler. Perhaps the thing handler could check for an active scene itself, and simply skip processing any inactive scene in such case. In this case the thing handler would need to receive all resources at once and have its own loop, i.e. introduce onResources in thing handler, and move this loop into that:

openhab-addons/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java

Line 540 in 5b98e41

resources.forEach(resource -> clip2ThingHandler.onResource(resource));

(and make onResource private)

If I understand the behavior correctly, we should avoid an intermediate state update to UNDEF. If not, you can disregard everything. If correct, the actual implementation proposals are just ideas from the top of my head, and you might have a better way to achieve the same result.

[jlaur]

This is unneeded.

[andrewfg]

Ok. I will delete that.

[jlaur]

@andrewfg - can you update the PR title to describe the bug being fixed rather than implementation details?

[andrewfg]

@jlaur to answer your questions..

• Multiple scenes can be associated with a thing
• The scene contributors cache keeps track of these associations
• When selecting a scene from OH the UI drop down options comes from the contributors cache
• If an event comes in with a scene ‘active’ the channel is updated to show the name of that scene
• If an event comes in with scene ‘inactive’ the channel is updated to UNDEF
• When setting scenes manually, then scene ‘active’ resp. ‘inactive’ events occur at different times
• This bug occurs specifically not when setting a scene, but when changing from one scene to another
• In the past the bridge would send a message first with the prior scene inactive, followed by the new one active
• But now it seems that (from time to time) it sends the new one active, followed by the prior one inactive
• I think it is right that the binding should process both active and inactive messages (even if the interval is milliseconds) for purposes of the event log
• However we need to resolve the out of order scene messages
• I have only observed out of order messages within a single SSE packet; strictly they are not actually out of order since the SSE packet has a single time stamp, but the assumption (apparently wrong) is that the data on the left of the string is ‘earlier’ than the data on the right
• I have not observed out of order scenes across multiple SSE packets (if this would occur, then it would be another bug, not solved by this PR

[jlaur]

• I think it is right that the binding should process both active and inactive messages (even if the interval is milliseconds) for purposes of the event log

Why do you think that's right? On the contrary, if the scene is changed from A to B, I would expect to see exactly that in the event log:

2023-12-07 12:10:00.000 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'Scene' changed from A to B

I would NOT expect to see:

2023-12-07 12:10:00.000 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'Scene' changed from A to UNDEF
2023-12-07 12:10:00.001 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'Scene' changed from UNDEF to B

Do you agree that we can/should consider one SSE event as an atomic operation? I believe we should.

You are right that it's very similar to #15905 in the way that we have asymmetry between the Hue API and the openHAB model. In the Hue API the order of scene active/scene inactive doesn't really matter when sent in the same event, since it's considered two different pieces of information. In our model we have only one channel to represent the currently active scene, not a channel for each scene to represent whether that scene is active or inactive.

[andrewfg]

I would NOT expect to see:
consider one SSE event as an atomic operation?

No I think it is not an atomic operation. I reality one scene was actually disabled. And another scene was enabled. As far as the scenes are concerned, and as far as the bridge is concerned there are indeed two separate events. I personally think we should not conflate those two events inside OH.

[maniac103]

I personally think we should not conflate those two events inside OH.

I think this logic is flawed:

• If it's possible to enable more than one scene simultaneously in Hue (and thus the scene activation/deactivation events are completely independent from each other), we should not have a channel pretending there can only be one active scene: instead the channel should publish StringListType, not StringType, containing a list of active scenes.
• If only one scene can be active at a given time, it makes no sense to go through the A -> UNDEF -> B transition, because the activation of B was the reason/root cause of the deactivation of A in the first place.

[andrewfg]

I think this logic is flawed:

Ok. It means a complete rewrite of the PR..

[jlaur]

I personally think we should not conflate those two events inside OH.

I think this logic is flawed:

Thank you @maniac103, this was exactly my point, but you described it in a much clearer way. 👍

[andrewfg]

As I say. This means a complete rewrite of the PR. It needs to check if both the scene deactivation and the scene activation event belong to the same room or zone. The SSE events contain sparse data so they don't have any information about which room or zone they belong to. Therefore it is impossible to do this fix with the current relatively minor changes at bridge level. Instead the room or zone information is only stored in the full resource information in the sceneCache of the respective things. This means that the elimination of the deactivation event that you are asking for must be done at thing level. Which means that the dispatch sequencing of the incoming events must be completely rewired. This is something that you @jlaur struggled with in the early stages of writing #15905 and eventually gave up doing. This is a massive structural change (for a relatively minor bug). Currently I don't have either the time or inclination to do this work. Certainly not before V4.1 release. So I offer you either of the two options..

1. You can have this current PR offering that solves the bug as far as the channel state is concerned (but has the extremely minor side effect of showing a transient state change in the log). Which can make it into v4.1. Or,
2. @jlaur you can write your own alternate PR that meets your own standards of perfection. Most probably this will be too big to be fully tested and reviewed and make it into v4.1

Your choice.

[jlaur]

2. @jlaur you can write your own alternate PR that meets your own standards of perfection. Most probably this will be too big to be fully tested and reviewed and make it into v4.1

Okay, here's a draft of the general approach I had in mind: #16018.

[maniac103]

So I offer you either of the two options..

You can have this current PR offering that solves the bug as far as the channel state is concerned (but has the extremely minor side effect of showing a transient state change in the log). Which can make it into v4.1. Or,
@jlaur you can write your own alternate PR that meets your own standards of perfection. Most probably this will be too big to be fully tested and reviewed and make it into v4.1

This doesn't sound like a binary decision. Why not do 1. now and 2. in the 4.2 timeframe?

[andrewfg]

a draft of the general approach I had in mind

A preferable approach would be to cache the scenes in the bridge handler during the mass download phase.

openhab-addons/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java

Line 733 in d14733f

for (ResourceReference reference : MASS_DOWNLOAD_RESOURCE_REFERENCES) {

it will not work if a resource containing a scene deactivation can also contain completely different fields at the same time

Rather than deleting the whole resource, you would just delete its status field .. similar to what you did with the merge in #15905 ..

[jlaur]

a draft of the general approach I had in mind

A preferable approach would be to cache the scenes in the bridge handler during the mass download phase.

Yes, but I would leave that out the scope of that PR to reduce risk. After spending some time understanding the code, I have some general ideas about cache improvements that I'll share later.

openhab-addons/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java

Line 733 in d14733f

for (ResourceReference reference : MASS_DOWNLOAD_RESOURCE_REFERENCES) {

it will not work if a resource containing a scene deactivation can also contain completely different fields at the same time

Rather than deleting the whole resource, you would just delete its status field .. similar to what you did with the merge in #15905 ..

If you have no hard objections to the approach, perhaps we could discuss details in the proposed PR? The reason I didn't change the status field is because that would (possibly) affect the cache. So to reduce risk, I went with the approach of simply skipping the channel update for undesired scene changes, but fully maintain the cache so that it represents the current state of all scenes.

[andrewfg]

I have some general ideas about cache improvements that I'll share later.

If you are intending to 'improve' the cache, we should know what you intend to do, before discussing your new scene PR.

In any case, it reinforces my feeling that your scene PR cannot make it into v4.1 .. so @maniac103 suggestion might at least alleviate the bug (for non perfectionist users at least).

[jlaur]

I have some general ideas about cache improvements that I'll share later.

If you are intending to 'improve' the cache, we should know what you intend to do, before discussing your new scene PR.

It is not related to the scene PR, and I need to investigate and experiment some more before sharing my ideas/intentions.

In any case, it reinforces my feeling that your scene PR cannot make it into v4.1 .. so @maniac103 suggestion might at least alleviate the bug (for non perfectionist users at least).

I think @maniac103 made this suggestion before seeing that I already provided a better fix? I would therefore instead suggest that you have a look at the proposed fix. I think you are on the wrong track with this one.

[andrewfg]

I think you are on the wrong track with this one.

IMHO I am not on any track.

[maniac103]

I think @maniac103 made this suggestion before seeing that I already provided a better fix?

I made it under the risk (or threat?) of the complete fix not landing in 4.1. I definitely do think it's better to have an incomplete fix than no fix at all, but I can't (and do not want to) asses the risks and merits of your both approaches. I rather wanted to mediate a bit from an outsider perspective ;-)

Personal remark: I do think, though, that your (@andrewfg) tone is slightly passive aggressive to plain confrontative at times. Is this really necessary?

[andrewfg]

slightly passive aggressive

@maniac103 yeah .. I learned that I have to do that with @jlaur .. otherwise he just ignores me .. and being a maintainer he overrules me.

[andrewfg]

Closed in favour of #16018