[hdpowerview] Add support for enabling/disabling automations

[jlaur]

Fixes #11516

Signed-off-by: Jacob Laursen jacob-github@vindvejr.dk

This pull request adds automation support to the Hunter Douglas PowerView binding. This can be useful for keeping as much logic in the PowerView Hub as possible to reduce dependencies. A typical use-case would be switching between winter and summer automations. In the summer one might want the shades up on weekdays at a fixed time. In the winter when still dark at this time, an offset relative to sunrise might be more appropriate. It will now be possible with rules to switch between such two automations when the sun will start to rise before or after some fixed time.

Channel groups have been introduced for:

• Scenes: Items will need to be relinked to new channel. Backwards compatible channels are created as long as items are linked to them and new channels are unlinked. See also: Naming convention for channels?
• Scene Groups: Introduced in [hdpowerview] Add support for scene groups #11534 so no backwards compatibility needed if PR will be included in 3.2.
• Automations: New.

Visual example:

Additional changes

• Scene and scene group channels are now refreshed automatically, so renaming will be reflected.
• Scene and scene group channels are now sorted by order provided by the API, i.e. same order as in app, except that room is not taken into consideration.
• Scene and scene group channel updates have been optimized for the two most common scenarios: Create all of them initially and skip when nothing has changed.
• Dynamic channels are flushed during initialization (and rebuilt after), so changed system language will be reflected at least when binding is restarted. Previously this required openHAB to be restarted.

[jlaur]

JAR: https://drive.google.com/file/d/15Wgt_cuNb3XUEbHejoAeEj31scXqgBCL/view?usp=sharing

[jlaur]

@andrewfg, @arroyoj - tagging you directly in case you would be interested in this (reviewing and/or testing).

[arroyoj]

@jlaur, this looks like a nice set of new features. I don't use automations, but the improvements to the dynamic channels, like automatic renaming, are things I am looking forward to. I had one thought about how to address the breaking change of moving scene channels into a channel group. I don't fully understand OpenHAB's internals and channel handling, so sorry if this doesn't make sense, but if a user has an item linked to the old (non-grouped) scene channel, will commands from that item still get sent to the hub Thing, but then dropped/ignored? I guess what I'm wondering is rather than making deprecated channels to handle the old item links (which I think you tested but didn't work?), could you still handle the old channels and issue a warning?

The section of the code that I'm thinking about is in handleCommand here: https://github.com/jlaur/openhab-addons/blob/f6b6ec5dff81344f91248b93e6cfdf742f392655/bundles/org.openhab.binding.hdpowerview/src/main/java/org/openhab/binding/hdpowerview/internal/handler/HDPowerViewHubHandler.java#L112-L142, particularly this part:

            HDPowerViewWebTargets webTargets = this.webTargets;
            if (webTargets == null) {
                throw new ProcessingException("Web targets not initialized");
            }
            int id = Integer.parseInt(channelUID.getIdWithoutGroup());
            if (sceneChannelTypeUID.equals(channel.getChannelTypeUID()) && OnOffType.ON.equals(command)) {
                webTargets.activateScene(id);
            } else if (sceneGroupChannelTypeUID.equals(channel.getChannelTypeUID()) && OnOffType.ON.equals(command)) {
                webTargets.activateSceneCollection(id);
            } else if (automationChannelTypeUID.equals(channel.getChannelTypeUID())) {
                webTargets.enableScheduledEvent(id, OnOffType.ON.equals(command));
            }

In handleCommand, it looks like you get the ID associated with the channel, and then send the ID to the appropriate handler based on which channel group it came from. I'm wondering if you can assume that any ID that comes from a channel without a channel group is an old scene channel link, and so have a final else clause that uses that ID to activate the corresponding scene and issue a warning or something that the scene is being activated through a deprecated link. This way, a user's old scene links would still work, assuming those channels were still considered "valid" by OpenHAB and made it to the handleCommand method. I'm not sure what this would do to the UI for links, but maybe when they see the link is missing, they would check the logs and see the message. If this approach worked, though, it would be nice that user's scene items kept working after the update.

[jlaur]

@arroyoj - thanks for your comments. If you are right, the current version of the code would already work. However, handleCommand receives ChannelUID, so I'm not sure the framework will call it for a channel that doesn't exist, even if an item is linked to it. I'm not 100% sure right now, but can test it tomorrow as unfortunately I won't have any time today. You are also welcome to give the JAR a try. :-) It's updated after latest commit and includes all previous changes, including the getter/setter consistency fix.

[jlaur]

@arroyoj - unfortunately it doesn't work, handleCommand will not be called when channel is unknown:

[arroyoj]

@jlaur, thanks for testing it. I tried it out too, and I realized that I was misunderstanding how channels are handled. I was thinking any channel with the right "prefix" would be forwarded to the Thing, but clearly not. It would be great if there was some way to create a "catch-all" channel, but I could find no evidence of that in the core code.

I did have another thought about how you could structure the logic of channel creation to maintain backwards compatibility while avoiding the situation described in Naming convention for channels?. If I understand the problem you described in that post, items linked to channels with a group ID (ie, scenes#123) will also be detected as linked to a similar channel without the group ID (ie, 123). So, if you create an "old"/deprecated no-group channel, you cannot tell if an item linked to it is a deprecated link or a correct new link. In contrast, items previously linked to old channels (ie, 123) will not be detected as linked to new with-group channels (ie, scenes#123). Am I understanding the problem right?

If that is correct, could the following logic (expressed as pseudocode) work for dynamic channel creation while keeping backwards compatibility?

for (scene_id in scene_list_from_hub) {
  channel_id_with_group = new ChannelUID(group id, scene_id)
  deprecated_channel_id = new ChannelUID(scene_id)
  if ( !isLinked(channel_id_with_group) ) {
    // This user does not have an item linked to the new channel, so maybe they have an old link
    if ( isLinked(deprecated_channel_id) ) {
      // Old link detected, need deprecated channel
      createChannel(deprecated_channel_id)
      logger.warn("Deprecated channel being used")
    }
  }
  // Always create the correct new channel
  createChannel(channel_id_with_group)
}

The two if statements could possibly be combined, but I kept them separate to try to show the logic. The second if statement might incorrectly be "true" if the user has new channel links, but the first if statement should guard against that because it will only be true if there are no new channel links.

The assumption here is that if the user has created at least one new channel link for a given scene, they no longer need the deprecated channel. On the other hand, if they have only an old channel link for the scene, a deprecated channel is needed to keep it working, but the new channel should also be available. If someone had two items linked to the same scene, it is possible only one would be updated and the other could stop working, but my guess is most people make a single item for each scene. And, if someone has not linked a scene at all, only the new channel should be created.

Sorry if you have tried this already or if this won't work out as I thought. Unfortunately, I don't have a way to test this myself.

[jlaur]

@arroyoj - thanks, that's a simple and quite reasonable approach - the missing piece in the puzzle, I would say. I've implemented it and this logic works and seems straight-forward. JAR updated.

[andrewfg]

Two comments from my side..

1. I have not checked your code, but I fully support your modifications to ensure that on existing user's systems, the links of pre-existing Items to pre-existing scene Channels will not be broken. (Indeed I would insist that they would not be broken).
2. I note that my just opened PR [hdpowerview] Fix secondary position bug. Add shade database and properties. #11698 is touching some of the same files as this PR does, so it is possible that the one or the other PR might cause merge conflicts; I hope not, but lets see; It probably depends on whose PR gets merged first..

[jlaur]

1. I have not checked your code, but I fully support your modifications to ensure that on existing user's systems, the links of pre-existing Items to pre-existing scene Channels will not be broken. (Indeed I would insist that they would not be broken).

I'm preparing to create an issue for openhab-core for supporting channel migrations. See also:
https://community.openhab.org/t/naming-convention-for-channels/128841/7

Currently it's hard to manage, but with @arroyoj's suggestion I believe the current logic is quite acceptable. See also #11668.

2. I note that my just opened PR [hdpowerview] Fix secondary position bug. Add shade database and properties. #11698 is touching some of the same files as this PR does, so it is possible that the one or the other PR might cause merge conflicts; I hope not, but lets see; It probably depends on whose PR gets merged first..

I did a first quick around of your PR, and as far as I can see we only collide in HDPowerViewBindingConstants which will be easy to resolve. Let's hope it will stay that way and we can have the PR's reviewed and merged sometime soon, so we can be ready for the next rounds. :-)

[jlaur]

I'm preparing to create an issue for openhab-core for supporting channel migrations. See also: https://community.openhab.org/t/naming-convention-for-channels/128841/7

Created openhab/openhab-core#2590

[jlaur]

@andrewfg - do you have more comments, or do you think it's in a state ready for being merged?

[andrewfg]

do you have more comments

I don't have any more comments.

(But -- to be honest -- since I do not actually have automations in my system, I have not fully tested it. Therefore please take my lack of comments as a 'no objection' rather than an 'active approval')..

[jlaur]

I have been using this version in production all the way, and have tested the following:

• Scenes can be activated from old channel not belonging to any channel group (backwards compatibility).
• Scenes can be activated from new channel belonging to channel group.
• Old scene channels not belonging to any channel group and not linked to any items are not created.
• Scene Groups can be activated from new channel belonging to channel group. Scene Groups were not part of 3.1, so no backwards compatibility provided.
• Automations can be enabled/disabled.
• When automations are enabled/disabled from the outside (e.g. PowerView app) this is reflected correctly.
• When scenes, scene groups or automations are changed from the outside, this is reflected (for example if a scene is renamed).
• If locale is changed and binding is restarted, channels are updated.

@lolodomo - would you consider this PR for 3.2? The advantage of including it would be less backwards compatibility struggles as scene groups were not part of 3.1. Otherwise will have to create some additional logic for scene group backwards compatibility.

[arroyoj]

I just tested out the latest JAR from @jlaur on my system. From a user perspective, it all looks good to me. Here is what I tested:

• Backwards compatible scene channels were created and worked. All of my old scene links were maintained, and scene items continued to function properly. The warning messages to upgrade the links showed up in the logs. On the Channels page, I saw both the old scene channels (marked as DEPRECATED) and the new scene channels.
• The new scene channels showed up in a separate "Scenes" channel group.
• Creating a new item and linking it to a new scene channel (ie, scenes#123) worked. The item linked to the new scene channel behaved the same as if it was linked to the deprecated channel.
• After restarting my system with a new scene channel linked (ie, scenes#123), the corresponding deprecated channel (ie, #123) was not created, as expected, and no warning message about that channel was logged, again as expected.
• Renaming a scene in the Powerview App updated the name for the corresponding scene channel! This is great! When I first set up my system, I actually went through and deleted scenes in the Powerview App and then recreated them with new names just because it bothered me that the names in OpenHAB were not the same after editing. Thanks for incorporating this.
• Although I don't use automations, I created an automation in the Powerview App for testing and it promptly showed up in OpenHAB as a new automations channel, within an "Automations" channel group.
• Linking an item to this automation channel allowed me to enable and disable the automation, and the enabled/disabled state set in OpenHAB was reflected in the Powerview App, and vice versa.

I think that covers the features described in the PR, and I didn't see any issues arise. Thanks @jlaur for adding these features.

[jlaur]

Thanks a lot for verifying the functionality, @arroyoj.

[andrewfg]

LGTM

[jlaur]

@lolodomo, @kaikreuzer - any chance of this getting into 3.2?

[andrewfg]

@lolodomo / @kaikreuzer -- it would be super cool if you could squeeze in #11698 as well ;)