[hue] Fix scene channel updates (API v2) #16018

jlaur · 2023-12-07T18:52:48Z

This is a proposal for replacing the approach in #16001.

When receiving this payload:

[
	{
		"creationtime": "2023-12-07T21:39:38Z",
		"data": [
			{
				"id": "00000000-0000-0000-0000-000000000001",
				"id_v1": "/scenes/0000000000000001",
				"status": {
					"active": "static"
				},
				"type": "scene"
			},
			{
				"id": "00000000-0000-0000-0000-000000000002",
				"id_v1": "/scenes/0000000000000002",
				"status": {
					"active": "inactive"
				},
				"type": "scene"
			}
		],
		"id": "00000000-0000-0000-0000-000000000000",
		"type": "update"
	}
]

The correct state change is triggered:

2023-12-07 22:39:37.368 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'Kontor_Scene' changed from Klar to Slap af

Also tested transition from UNDEF -> scene and scene -> UNDEF.

Since there is only one supported channel for resource type scene, it should be safe to skip updateChannels() entirely for the resource:

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

Lines 933 to 935 in 3814f37

    
           case SCENE: 
        
               updateState(CHANNEL_2_SCENE, resource.getSceneState(), fullUpdate); 
        
               break;

Fixes #16000

andrewfg · 2023-12-08T17:13:02Z

...ab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2ThingHandler.java

+    public void onResources(Collection<Resource> resources) {
+        boolean sceneActivated = false;
+        for (Resource resource : resources) {
+            boolean skipUpdate = sceneActivated && ResourceType.SCENE == resource.getType()


OK, if you really want to handle this perfectly then you need to process SMART_SCENE transitions as well. A room or a zone can have either a scene active, a smart scene active, or an inactive state. Therefore you need to handle transitions between all of those states.

.. and yes, before you say so, I do admit that my prior PR was not handling smart scenes either ..

OK, if you really want to handle this perfectly then you need to process SMART_SCENE transitions as well. A room or a zone can have either a scene active, a smart scene active, or an inactive state. Therefore you need to handle transitions between all of those states.

I think I only want to handle that if I can reproduce that, or receive payloads for verifying such issue. Using the app I found one scene which is actually a "smart scene": natural light. But it seems it will actually just transition through some "normal" scenes, so this already works. Do you know a way to create a smart scene that will appear in the payload as active/inactive?

Do you know a way to create a smart scene that will appear in the payload as active/inactive?

No.

I think I only want to handle that if I can reproduce that

Perhaps you can understand why @maniac103 thinks that I respond too strongly. If any suggestion is your idea, it is always perfect, no matter how much work is involved for the other party. But only if it is your idea. Whereas if I have an opinion you always say it is too difficult for you to implement, and always rule it out. Perhaps a little more flexibility, please.

In response to:

If any suggestion is your idea, it is always perfect, no matter how much work is involved for the other party. But only if it is your idea. Whereas if I have an opinion you always say it is too difficult for you to implement, and always rule it out. Perhaps a little more flexibility, please.

and: #16001 (comment)

Sincerely, thanks for giving me this feedback, which will allow me to reflect and self-improve. My communication skills are definitely lacking.

This is no excuse, but just to give you some explanation. Besides the mandatory review feedback about openHAB compliance, conventions, guidelines etc. I try to give individualized reviews based on multiple factors. As an example, when reviewing a high quality contribution, I might suggest improvements I probably would not have suggested otherwise. Contributor history is also a parameter - the better I know a contributor, the better I can help by knowing how much details I should go into. Some appreciate even small comments that will give a slightly better end result, others prefer a faster merge and can get frustrated by too many comments.

I have gotten the impression that you are very detail-oriented, for example based on your review of my contributions and our discussions in general. I might therefore have made some false conclusions about your preferences. I'm sorry for that, and I'll try to recalibrate.

That said, I will also give you some personal feedback. For a long time I have felt hostility from you, and at times just plain rudeness. I'm sensitive to that, so it takes energy and motivation from me. If you have felt ignored, that might have been a consequence of me simply needing to take pauses from that, i.e. reduce interactions or "throttle down" if you will. I have also already tried to put much more consideration into how I phrase things to you, to not upset you. Sometimes it slips, especially if you have really pissed me off, to be frank. It should not, but I am human. Sorry.

Going forward, besides the recalibration I will make, on short term I can try my best to stay away as contributor and reviewer of the bindings for which you are code maintainer. That should reduce tensions, and I also need that for my mental health. I can't promise for how long, since I also enjoy working on improving these bindings. After all, this was my original motivation for becoming a contributor. So it will be a balance act.

@jlaur many thanks for this apology; it seems that we may both be 'too detail oriented', and/or have mutually 'p**d each other off'; I therefore offer you also my apology (albeit not as eloquent as above).

My question now is how to proceed? Shall we continue with this PR? Or shall I adapt mine? I think the solution will ultimately be a combination of your code and mine. I have offered some code code below, so either you can take it over; or alternatively I can take over your code into my PR?

andrewfg · 2023-12-08T17:17:29Z

...ab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2ThingHandler.java

+        boolean sceneActivated = false;
+        for (Resource resource : resources) {
+            boolean skipUpdate = sceneActivated && ResourceType.SCENE == resource.getType()
+                    && resource.getSceneActive().isPresent();


I think you need to check for !resource.getSceneActive().orElse(true)

And BTW likewise for SMART_SCENE

And actually getSceneActive() resp. getSmartSceneActive() implicitly check the ResourceType internally so the ResourceType.SCENE == resource.getType() resp. ResourceType.SMART_SCENE == resource.getType() checks are not required.

Just to help a bit more, getSmartSceneActive() and getSceneActive() return a tri-state result..

Optional.ofEmpty() the resource is NOT a (smart-) scene

Optional.of(false) the resource is a (smart-) scene and its state is inactive

Optional.of(true) the resource is a (smart-) scene and its state is active (actually !inactive to be precise)

I think you need to check for !resource.getSceneActive().orElse(true)

Why?

Because your current ifPresent() code returns true whether the next processed scene is active or inactive, whereas you only want to skip the scene if it is specifically inactive.

That is intentional. There can't be more than one active scene (right?), so after reaching the first one, it doesn't make sense to process anything else.

Please see my proposed code below.

There can't be more than one active scene (right?)

There might be two scenes activated within one SSE package.

so after reaching the first one

We are supposed to be ignoring any IN-active messages in an SSE package, that also contains one (or more) active messages. So the suppression of the channel updates should only apply to the specifically IN-active messages.

In addition to the above, your code steps through the resources, so you may not detect a scene active until after your have received an inactive one; and thus generating the unwanted transient inactive channel state.

...ab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2ThingHandler.java

andrewfg

And I assume you will add Junit tests for this?

andrewfg · 2023-12-09T14:37:47Z

@jlaur your onResources() method should IMHO be as follows.

    public void onResources(Collection<Resource> resources) {
        boolean sceneActivated = resources.stream().anyMatch(r -> r.getSceneActive().orElse(false) || r.getSmartSceneActive().orElse(false));
        for (Resource resource : resources) {
            boolean skipUpdate = sceneActivated && (!resource.getSceneActive().orElse(true) || !resource.getSmartSceneActive().orElse(true));
            onResource(resource, !skipUpdate);
        }
    }

EDIT: and here is some test code..

    private static final JsonElement STATIC = JsonParser.parseString("{\"active\":\"static\"}");
    private static final JsonElement INACTIVE = JsonParser.parseString("{\"active\":\"inactive\"}");

    @Test
    void testTransitionA() {
        List<Resource> list = new ArrayList<>();
        list.add(new Resource(ResourceType.LIGHT).setId("0"));
        list.add(new Resource(ResourceType.SCENE).setId("1").setStatus(STATIC));
        list.add(new Resource(ResourceType.LIGHT).setId("2"));
        list.add(new Resource(ResourceType.SCENE).setId("3").setStatus(INACTIVE));
        list.add(new Resource(ResourceType.LIGHT).setId("4"));
        System.out.println("testTransitionA");
        onResources(list);
        System.out.println();
    }

    @Test
    void testTransitionB() {
        List<Resource> list = new ArrayList<>();
        list.add(new Resource(ResourceType.LIGHT).setId("0"));
        list.add(new Resource(ResourceType.SCENE).setId("1").setStatus(INACTIVE));
        list.add(new Resource(ResourceType.LIGHT).setId("2"));
        list.add(new Resource(ResourceType.SCENE).setId("3").setStatus(STATIC));
        list.add(new Resource(ResourceType.LIGHT).setId("4"));
        System.out.println("testTransitionB");
        onResources(list);
        System.out.println();
    }

    @Test
    void testTransitionC() {
        List<Resource> list = new ArrayList<>();
        list.add(new Resource(ResourceType.LIGHT).setId("0"));
        list.add(new Resource(ResourceType.SCENE).setId("1").setStatus(STATIC));
        list.add(new Resource(ResourceType.LIGHT).setId("2"));
        list.add(new Resource(ResourceType.SMART_SCENE).setId("3").setState("inactive"));
        list.add(new Resource(ResourceType.LIGHT).setId("4"));
        System.out.println("testTransitionC");
        onResources(list);
        System.out.println();
    }

    @Test
    void testTransitionD() {
        List<Resource> list = new ArrayList<>();
        list.add(new Resource(ResourceType.LIGHT).setId("0"));
        list.add(new Resource(ResourceType.SMART_SCENE).setId("1").setState("active"));
        list.add(new Resource(ResourceType.LIGHT).setId("2"));
        list.add(new Resource(ResourceType.SCENE).setId("3").setStatus(INACTIVE));
        list.add(new Resource(ResourceType.LIGHT).setId("4"));
        System.out.println("testTransitionD");
        onResources(list);
        System.out.println();
    }

    @Test
    void testTransitionE() {
        List<Resource> list = new ArrayList<>();
        list.add(new Resource(ResourceType.LIGHT).setId("0"));
        list.add(new Resource(ResourceType.SMART_SCENE).setId("1").setState("active"));
        list.add(new Resource(ResourceType.LIGHT).setId("2"));
        list.add(new Resource(ResourceType.SMART_SCENE).setId("3").setState("inactive"));
        list.add(new Resource(ResourceType.LIGHT).setId("4"));
        System.out.println("testTransitionE");
        onResources(list);
        System.out.println();
    }

    @Test
    void testTransitionF() {
        List<Resource> list = new ArrayList<>();
        list.add(new Resource(ResourceType.LIGHT).setId("0"));
        list.add(new Resource(ResourceType.SCENE).setId("1").setStatus(STATIC));
        list.add(new Resource(ResourceType.LIGHT).setId("2"));
        list.add(new Resource(ResourceType.SCENE).setId("3").setStatus(STATIC));
        list.add(new Resource(ResourceType.LIGHT).setId("4"));
        System.out.println("testTransitionF");
        onResources(list);
        System.out.println();
    }

    @Test
    void testTransitionG() {
        List<Resource> list = new ArrayList<>();
        list.add(new Resource(ResourceType.LIGHT).setId("0"));
        list.add(new Resource(ResourceType.SMART_SCENE).setId("1").setState("active"));
        list.add(new Resource(ResourceType.LIGHT).setId("2"));
        list.add(new Resource(ResourceType.SMART_SCENE).setId("3").setState("active"));
        list.add(new Resource(ResourceType.LIGHT).setId("4"));
        System.out.println("testTransitionG");
        onResources(list);
        System.out.println();
    }

    public void onResources(Collection<Resource> resources) {
        boolean sceneActivated = resources.stream()
                .anyMatch(r -> r.getSceneActive().orElse(false) || r.getSmartSceneActive().orElse(false));
        for (Resource resource : resources) {
            boolean skipUpdate = sceneActivated
                    && (!resource.getSceneActive().orElse(true) || !resource.getSmartSceneActive().orElse(true));
            System.out.println(resource.getId() + ":" + skipUpdate);
        }
    }

jlaur · 2023-12-09T19:51:22Z

your onResources() method should IMHO be as follows.

How would you like to proceed?

You challenged/invited me to propose this alternative solution I have provided here. I can go along and add smart scene support to this PR assuming that smart scenes can be inactive exactly the same way as scenes, and that scenes and smart scenes are mutually exclusive. (It just bothers me that I can't test smart scene functionality, because I can't seem to create one) - and you can review.

Or you can take over, if you think I'm too far from getting it right, and you can leave it for another maintainer if you prefer?

Regarding unit test, I don't think I'll find the time for that since there is currently no coverage for the thing handler at all (I think), so it would need to be created from scratch. I don't quite understand the test code you have provided - it uses println() and duplicates the code from the thing handler?

andrewfg · 2023-12-09T20:01:08Z

I don't think I'll find the time for that since there is currently no coverage for the thing handler at all
I don't quite understand the test code you have provided
Or you can take over

Yes the code I posted is not Junit capable. However I do have an idea how to create a Junit test around that. Therefore let me update my prior PR as proposed. As mentioned, I will take over your top level code from this PR, so it will be a combined effort.

andrewfg · 2023-12-10T00:33:40Z

@jlaur I realised just now that the approach you propose in this PR is still wrong. Its aim is that if an SSE packet contains both a scene activate and a scene deactivate resource then the latter should be ignored. However this would only work if both scene resources belong to the same room or zone. By contrast if the two resources belong to different rooms or zones, then the deactivate resource should not be ignored. The more I think about this, the more I realise that this is a massive challenge, and the more I am convinced that the ‘perfect solution’ can’t make it into v4.1. My imperfect PR #16001 whilst not solving your basic requirements, would nevertheless at least allow the UI to show the correct status. So my suggestion as @maniac103 says would be to merge that one for v4.1 and get back to the perfect solution later. => WDYT?

...ab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2ThingHandler.java

Fixes openhab#16000 Also-by: Andrew Fiddian-Green <software@whitebear.ch> Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

andrewfg

LGTM (finally)

gcatellani · 2023-12-11T20:25:26Z

Thanks for all the effort and for overcoming your differences to provide us with a quick fix and a more thorough one as well as the binding in general! It‘s very much appreciated.

lolodomo

LGTM, thank you

Fixes openhab#16000 Also-by: Andrew Fiddian-Green <software@whitebear.ch> Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk> Signed-off-by: Jørgen Austvik <jaustvik@acm.org>

Fixes openhab#16000 Also-by: Andrew Fiddian-Green <software@whitebear.ch> Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

jlaur added the bug An unexpected problem or unintended behavior of an add-on label Dec 7, 2023

jlaur changed the title ~~[hue] Fix scene channel updates~~ [hue] Fix scene channel updates (API v2) Dec 7, 2023

jlaur mentioned this pull request Dec 7, 2023

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

Closed

jlaur force-pushed the 16000-hue-fix-scene-channel-updates branch from beb9c25 to efc42c6 Compare December 7, 2023 21:36

jlaur marked this pull request as ready for review December 8, 2023 16:18

jlaur requested review from andrewfg and cweitkamp as code owners December 8, 2023 16:18

andrewfg requested changes Dec 8, 2023

View reviewed changes

jlaur force-pushed the 16000-hue-fix-scene-channel-updates branch from efc42c6 to e696209 Compare December 8, 2023 17:36

andrewfg requested changes Dec 8, 2023

View reviewed changes

jlaur force-pushed the 16000-hue-fix-scene-channel-updates branch 2 times, most recently from 4fe6320 to 868d245 Compare December 8, 2023 22:47

jlaur force-pushed the 16000-hue-fix-scene-channel-updates branch from 868d245 to 7d1c235 Compare December 10, 2023 09:32

jlaur marked this pull request as draft December 10, 2023 10:05

jlaur force-pushed the 16000-hue-fix-scene-channel-updates branch from 7d1c235 to efbaed7 Compare December 10, 2023 10:52

andrewfg requested changes Dec 10, 2023

View reviewed changes

...ab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2ThingHandler.java Show resolved Hide resolved

jlaur marked this pull request as ready for review December 10, 2023 11:16

jlaur force-pushed the 16000-hue-fix-scene-channel-updates branch from efbaed7 to e365ddc Compare December 10, 2023 11:58

Fix scene channel updates

b6bb3a4

Fixes openhab#16000 Also-by: Andrew Fiddian-Green <software@whitebear.ch> Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

jlaur force-pushed the 16000-hue-fix-scene-channel-updates branch from e365ddc to b6bb3a4 Compare December 10, 2023 19:12

andrewfg approved these changes Dec 11, 2023

View reviewed changes

jlaur requested a review from a team December 11, 2023 18:22

lolodomo approved these changes Dec 12, 2023

View reviewed changes

lolodomo merged commit f456300 into openhab:main Dec 12, 2023

lolodomo added this to the 4.1 milestone Dec 12, 2023

jlaur deleted the 16000-hue-fix-scene-channel-updates branch December 21, 2023 14:00

joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024

Fix scene channel updates (openhab#16018)

8de1f55

Fixes openhab#16000 Also-by: Andrew Fiddian-Green <software@whitebear.ch> Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

	case SCENE:
	updateState(CHANNEL_2_SCENE, resource.getSceneState(), fullUpdate);
	break;

Uh oh!

[hue] Fix scene channel updates (API v2) #16018

[hue] Fix scene channel updates (API v2) #16018

Uh oh!

Conversation

jlaur commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewfg Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewfg left a comment

Choose a reason for hiding this comment

Uh oh!

andrewfg commented Dec 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jlaur commented Dec 9, 2023

Uh oh!

andrewfg commented Dec 9, 2023

Uh oh!

andrewfg commented Dec 10, 2023

Uh oh!

Uh oh!

andrewfg left a comment

Choose a reason for hiding this comment

Uh oh!

gcatellani commented Dec 11, 2023

Uh oh!

lolodomo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jlaur commented Dec 7, 2023 •

edited

Loading

andrewfg Dec 8, 2023 •

edited

Loading

andrewfg commented Dec 9, 2023 •

edited

Loading