Skip to content
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] Add support for enabling automations (API v2) #16980

Merged
merged 21 commits into from
Aug 19, 2024

Conversation

andrewfg
Copy link
Contributor

@andrewfg andrewfg commented Jul 2, 2024

Add support for enabling automations

Closes #16904

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

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg andrewfg added enhancement An enhancement or new feature for an existing add-on work in progress A PR that is not yet ready to be merged labels Jul 2, 2024
@andrewfg andrewfg requested a review from jlaur July 2, 2024 16:37
@andrewfg andrewfg self-assigned this Jul 2, 2024
@andrewfg andrewfg requested a review from cweitkamp as a code owner July 2, 2024 16:37
@andrewfg
Copy link
Contributor Author

andrewfg commented Jul 2, 2024

@jlaur this is my first contribution; it has not been tested; but you may want to take a look at it (in case you think I am on a wrong track)..

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

andrewfg commented Jul 3, 2024

@jlaur I tested it and it fails. Reasons are as follows:

  • GET https://xxx.xxx.xxx.xxx/clip/v2/resource/behavior_instance returns many more entries than just the obvious automations; it seems to have a behaviour for every switch / button / event type in the system. So we will need to find a way to filter out only those behaviours that are actually automations.

  • Some of the returned behavior_instance resources cause a JsonSyntaxException; and from looking at the actual JSON returned by GET https://xxx.xxx.xxx.xxx/clip/v2/resource/behavior_instance it is evident that the actual resources use a more complex JSON schema than that described in the Hue / Signify API; in other words, we will need to reverse engineer the JSON schema, and figure out where the parse errors occur.

FACIT: this will take much more time and effort than I originally expected!

@jlaur
Copy link
Contributor

jlaur commented Jul 3, 2024

Thanks for giving it a go.

  • GET https://xxx.xxx.xxx.xxx/clip/v2/resource/behavior_instance returns many more entries than just the obvious automations; it seems to have a behaviour for every switch / button / event type in the system. So we will need to find a way to filter out only those behaviours that are actually automations.

Indeed. I just had a quick look in Postman, and it seems like sensors etc. have something like this:

            "state": {
                "model_id": "SML001",
                "source_type": "device"
            },

At least on my system, those without this state are the ones presented as automations in the app.

  • Some of the returned behavior_instance resources cause a JsonSyntaxException; and from looking at the actual JSON returned by GET https://xxx.xxx.xxx.xxx/clip/v2/resource/behavior_instance it is evident that the actual resources use a more complex JSON schema than that described in the Hue / Signify API; in other words, we will need to reverse engineer the JSON schema, and figure out where the parse errors occur.

I'm not sure we need all that since we are only interested in disabling/enabling automations. So we can read this state on first level without defining any nested DTO structures:

"enabled": false,

And likewise we can set this state like described in #16904.

So maybe it wouldn't be that hard?

@jlaur
Copy link
Contributor

jlaur commented Jul 3, 2024

I'm not sure we need all that since we are only interested in disabling/enabling automations.

Of course we might look into if anything else makes sense to be able to control. We can already enable/disable motion sensors (as an example) directly through its Thing. So I guess the API makes it possible to configure everything like behavior of buttons, lux levels for motion sensors, time intervals when light should be dimmed etc.

Currently I don't see a use-case for such fine-grained control - not saying there isn't one though. 😉 If we'd come back to this at some point, Thing Actions would probably be a more appropriate way for controlling this.

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

andrewfg commented Jul 3, 2024

@jlaur as a result of my latest commits, it is now working fully; the only problem is that the Bridge contains (in my case) 22 behaviour_instance's whereas my App shows only 2 automations. The remaining 20 behaviour_instance's (in my case) are associated with various switch and sensor actions. => We need to figure out how to filter only those behaviour_instance's which are displayed by the app.


image

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

andrewfg commented Jul 3, 2024

@jlaur I just added an isStateNull() filter so it should now only show 'real' automations.


EDIT: however the channels do still contain my Tap Dial switch (see below) which is apparently also without a state element. => So we probably need to refine the filter further..

image

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 andrewfg removed the work in progress A PR that is not yet ready to be merged label Jul 3, 2024
@andrewfg
Copy link
Contributor Author

andrewfg commented Jul 4, 2024

Tap Dial switch .. apparently .. without a state element

@jlaur for info: My Tap Dial switch is a device that I purchased for testing purposes. It was actually stored in a box in my shed, out of range of the Hue Bridge, and not programmed to any real room or zone. I think that was the reason why the respective behavior_instance did not have a 'state' element. I have now connected it properly to my live system, and now it does have a 'state' element. So now it is properly excluded from the automations list - see below.

image

Notwithstanding this, I think that perhaps the absence of a 'state' element is perhaps not the proper "official" way to filter automations. Reason is that in the Hue App the Tap Dial switch did never appear -- irrespective of whether it was properly configured or not. I wonder if we should in any case look for another way for filtering automations. => WDYT?

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>
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good! I have given it a test ride, and it works really well.

I'm wondering if it would be possible to update automation channels automatically without having to disable and enable the Thing (i.e. after adding/removing/renaming automations in the app)?

Polling might not be desirable, but perhaps it's possible to trigger a refetch when receiving a message about an unknown automation being updated? That would of course only solve the problem of adding channels, not removing them. And also I don't know if the binding is even receiving such messages.

@andrewfg
Copy link
Contributor Author

andrewfg commented Jul 5, 2024

perhaps it's possible to trigger a refetch when receiving a message about an unknown automation being updated?

Yes I could do that.

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

andrewfg commented Jul 6, 2024

@jlaur I have committed a change that should automatically refresh the automation channels if an event comes in containing an automation resource that is not yet known. I am rather busy, so I have not had the chance to test it, but I made the commit in case you can test it, and want to merge it before 4.2 release.

@jlaur
Copy link
Contributor

jlaur commented Aug 16, 2024

I think it is fixed now

Thanks. I just gave it a spin as well, and I now also see three of my motion sensors added as automations. I don't know why three, because I have more than that, but I'll have a closer look within the next days.

EDIT: A quick look into the returned JSON shows that only three of my motion sensors are part of the behavior instance response. They have no device element within the configuration section. I don't know if it can help, but they have this state (same level as configuration, i.e. first):

            "state": {
                "model_id": "SML003",
                "source_type": "device"
            },

EDIT: For me it seemed to do the trick to combine with your previous proposal:

    public boolean isAutomationResource() {
        Configuration configuration = this.configuration;
        return (Objects.isNull(state) || state.isJsonNull()) && Objects.nonNull(configuration)
                && !configuration.hasDeviceElement() && (ResourceType.BEHAVIOR_INSTANCE == getType());
    }

@jlaur
Copy link
Contributor

jlaur commented Aug 16, 2024

@andrewfg - and now I experienced another quirk. I enabled one automation during my tests, and disabled it again right after. But it seemed to be running, and I now see this in the response:

            "enabled": false,
            "state": {
                "gts_state": "stopped"
            },

and:

            "status": "running",

so with the code proposed above, this automation is now left out.

I will need to get back to this.

@andrewfg
Copy link
Contributor Author

andrewfg commented Aug 17, 2024

It seems to be difficult to find a filter which avoids both false negatives and false positives. I assume that the Hue App is using some kind of filter too..


EDIT: in my case it seems that the presence of a recurrence_days element could be a valid filter criterion for differentiating automations from other behavior instances.


EDIT2: .. or perhaps there is some "magic" in cross referencing the script_id in the behaviour_instances against the id in behaviour_scripts .. ?? .. or something like that ??

EDIT3: to answer my own question, the answer is "yes"!! The behaviour_scripts contain a category element which can have the value automation or accessory -- so the filter is

behaviour_instances => script_id => behaviour_scripts => id => metadata => category

Signed-off-by: AndrewFG <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

@jlaur phew .. hopefully my latest commit has the definitive filter for the subset of behaviour instances whose script id is of the metadata automation category.

@jlaur
Copy link
Contributor

jlaur commented Aug 17, 2024

phew .. hopefully my latest commit has the definitive filter for the subset of behaviour instances whose script id is of the metadata automation category.

Well done, it seems to work very well now:

  • Initial: My existing automations are shown as expected (same as in the app).
  • Create new: The automation is immediately added as channel.
  • Rename: After disabling/enabling the bridge, the channel is updated with new name.
  • Delete: After disabling/enabling the bridge, the channel is deleted.
  • Enable/disable from app: After disabling/enabling the bridge, the channel state is updated.

For the last three cases, I assume that no messages are available that would make it possible to reflect changes immediately, is that correct? I'm wondering if a daily update job would make sense, WDYT?

@andrewfg
Copy link
Contributor Author

For the last three cases, I assume that no messages are available that would make it possible to reflect changes immediately, is that correct? I'm wondering if a daily update job would make sense, WDYT?

  1. Enable/disable from app: => Hmm. That SHOULD work; but it looks like the event processor fails to handle it; -- I fear I mistakenly deleted it in one of my earlier "fix" commits. I will fix it (again).
  2. Delete: => The bridge anyway does an hourly online 'heartbeat' check (against the unlikely case the SSE connection would drop silently without throwing an exception). So I will refresh the channels during that check.
  3. Rename: => A rename cause the automation state to cycle => "initializing" => "running" so it actually generates two events. So this should be fixed by 1. above (or if not then by 2.)

@andrewfg
Copy link
Contributor Author

@jlaur of course the event resources only contain partial data so the script id is missing which means that the new filter was not satisfied so the events were not actually being processed at all.

Signed-off-by: AndrewFG <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

@jlaur today's commit fixes the open issues mentioned below, so I think it is now (finally) ready to go.

  • correct filtering of behavior instances
  • adding, deleting and renaming an automation in the Hue App causes immediate update of the OH channel list
  • enabling and disabling an automation in the Hue App causes immediate update of the OH channel state
  • thread safe access to set and map

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

today's commit fixes the open issues mentioned below, so I think it is now (finally) ready to go.

Cool work! I've also re-run my tests, and everything seems to work really well (and fast) now.

I have added a few comments to the code.

@jlaur jlaur removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Aug 19, 2024
Signed-off-by: AndrewFG <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

andrewfg commented Aug 19, 2024

@jlaur the Maven build now shows some warning messages (see below) .. which (as far as I can tell) are due to #16875 .. however I am not sure if I can fix them here, or if @lsiepel should look at it..

[WARNING] G:\git\andrewfg\openhab-addons\bundles\org.openhab.binding.hue\src\main\java\org\openhab\binding\hue\internal\connection\Clip2Bridge.java:[488,25] Potential null pointer access: This expression of type java.lang.Long may be null but requires auto-unboxing
[WARNING] G:\git\andrewfg\openhab-addons\bundles\org.openhab.binding.hue\src\main\java\org\openhab\binding\hue\internal\handler\Clip2ThingHandler.java:[639,21] Potential null pointer access: This expression of type java.lang.Boolean may be null but requires auto-unboxing
[WARNING] G:\git\andrewfg\openhab-addons\bundles\org.openhab.binding.hue\src\main\java\org\openhab\binding\hue\internal\handler\Clip2ThingHandler.java:[639,57] Potential null pointer access: This expression of type java.lang.Boolean may be null but requires auto-unboxing
[WARNING] G:\git\andrewfg\openhab-addons\bundles\org.openhab.binding.hue\src\main\java\org\openhab\binding\hue\internal\handler\Clip2ThingHandler.java:[643,24] Potential null pointer access: This expression of type java.lang.Boolean may be null but requires auto-unboxing
[WARNING] G:\git\andrewfg\openhab-addons\bundles\org.openhab.binding.hue\src\main\java\org\openhab\binding\hue\internal\handler\Clip2ThingHandler.java:[643,67] Potential null pointer access: This expression of type java.lang.Boolean may be null but requires auto-unboxing

EDIT: applying @SuppressWarnings("null") does "fix" the warnings ;) .. but maybe there is a better way??

@andrewfg andrewfg added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Aug 19, 2024
@jlaur
Copy link
Contributor

jlaur commented Aug 19, 2024

the Maven build now shows some warning messages (see below) .. which (as far as I can tell) are due to #16875 .. however I am not sure if I can fix them here, or if @lsiepel should look at it..

I noticed them as well. I think it would be better to address them in a separate PR to not extend the scope of this PR.

@andrewfg
Copy link
Contributor Author

I think it would be better to address them in a separate PR to not extend the scope of this PR.

Yeah. I already tried "jumping through hoops" to modify the code so it does not create compiler warnings .. however the only thing that I could find that "works" was @SupressWarnings ;) => so I am more than happy to defer to someone more knowledgeable about this issue..

@lsiepel
Copy link
Contributor

lsiepel commented Aug 19, 2024

I think it would be better to address them in a separate PR to not extend the scope of this PR.

Yeah. I already tried "jumping through hoops" to modify the code so it does not create compiler warnings .. however the only thing that I could find that "works" was @SupressWarnings ;) => so I am more than happy to defer to someone more knowledgeable about this issue..

Created a PR here: #17293

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jlaur jlaur merged commit 77561d5 into openhab:main Aug 19, 2024
5 checks passed
@jlaur jlaur added this to the 4.3 milestone Aug 19, 2024
@andrewfg andrewfg deleted the hue-automations branch August 25, 2024 10:56
digitaldan pushed a commit to digitaldan/openhab-addons that referenced this pull request Aug 29, 2024
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@jlaur jlaur changed the title [hue] Add support for enabling automations [hue] Add support for enabling automations (API v2) Sep 14, 2024
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[hue] Managing automations
4 participants