-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[hdpowerview] Add support for Generation 3 #13355
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, @andrewfg, this looks really good and clean to me - good job! I already added a few initial comments, but will need more time for going into the nitty gritty details.
How progressed is it, did you have a chance to test it with actual Gen 3 equipment through VPN?
Regarding the structure:
- I believe you can remove the "3" suffix from all DTO classes as there are no namespace conflicts.
- I'm in doubt whether I like all Gen 3 classes being in a completely separate tree as opposed to existing alongside the old classes. See below.
With the new hierarchy a lot of classes seems "hidden". So I would consider instead of:
- api/responses/shade
- discovery/HDPowerViewDeviceDiscoveryService
- handler/HDPowerViewHubHandler
-
- gen3/discovery/HDPowerViewDeviceDiscoveryService3
-
- gen3/handler/HDPowerViewHubHandler3
to have it flattened like this:
- dto/responses/Scenes
- dto/responses/Shade
-
- dto/gen3/Shade
-
- dto/gen3/Scene
- discovery/HDPowerViewDeviceDiscoveryService
- discovery/HDPowerViewDeviceDiscoveryService3
- handler/HDPowerViewHubHandler
- handler/HDPowerViewHubHandler3
...hdpowerview/src/main/java/org/openhab/binding/hdpowerview/internal/gen3/dto/SceneEvent3.java
Outdated
Show resolved
Hide resolved
...hdpowerview/src/main/java/org/openhab/binding/hdpowerview/internal/gen3/dto/ShadeEvent3.java
Outdated
Show resolved
Hide resolved
...hdpowerview/src/main/java/org/openhab/binding/hdpowerview/internal/gen3/dto/ShadeEvent3.java
Outdated
Show resolved
Hide resolved
...owerview/src/main/java/org/openhab/binding/hdpowerview/internal/gen3/dto/ShadePosition3.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.hdpowerview/src/main/resources/OH-INF/thing/bridge.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.hdpowerview/src/main/resources/OH-INF/thing/bridge.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.hdpowerview/src/main/resources/OH-INF/thing/bridge.xml
Outdated
Show resolved
Hide resolved
...a/org/openhab/binding/hdpowerview/internal/discovery/HDPowerViewHubDiscoveryParticipant.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/openhab/binding/hdpowerview/internal/database/ShadeCapabilitiesDatabase.java
Outdated
Show resolved
Hide resolved
.../openhab/binding/hdpowerview/internal/gen3/discovery/HDPowerViewDeviceDiscoveryService3.java
Outdated
Show resolved
Hide resolved
No. Unfortunately @vves seems to have other priorities at the moment. => ??? |
@andrewfg if it helps, I have a new gen3 install that I'm willing to alpha test with. 😃 |
@jlaur for me my next step will be to confirm that Gen1/2 is not broken. |
That's good to know, thanks! However, I'm afraid that the API is not finalized/published yet, so I don't think it will be possible to test with your firmware just yet. @andrewfg - for transparency, perhaps the PR should be marked as draft for now? |
@jlaur before I rename any classes, and/or move them to other folders, I would like to make 100% sure we are in agreement on the approach. So can you please confirm the following from your side?
|
In general I don't think a total segregation with two complete separate hierarchies will be useful. That would be almost like having two bindings in one, but actually I do think there will be some overlap and dependencies. The exception would be the DTO classes, which I think should be kept separate. Two reasons for that:
In general, yes.
Not for this case. First, I don't think any Gen3 DTO class should include other DTO classes from Gen 1/2 and vice versa. This would break the separation. And also, in specific scenarios where we need to reference both, this shouldn't be a problem: We can just reference them by fully qualified namespaces. As an example, this might be needed in the console class being introduced with #13615.
Yes, that seems reasonable. In a way we are lucky that Hub is now Gateway, as it resolves naming conflicts for us. :-)
Yes. I suggested to Wes to add possibility in the API for enabling/disabling automations, but don't know the status. This is very useful for automating the automations. :-) See #11637. I would certainly miss this. Probably it would appear in some new form with new JSON structures, so we'll see. |
@jlaur many thanks for the advice; I have renamed and moved the classes as you suggest; and for the DTO's it was indeed possible to use the same classes in gen 3 as in gen 1/2 by means of fully qualified references as you proposed. In this last commit, I also extended your new console class, and did some extra work on the ReadMe.. |
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>
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>
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>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
…ewfg/openhab-addons into hdpowerview-generation3-pr2
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>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/hunter-douglas-luxaflex-powerview-generation-3-binding/146796/1 |
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/hunter-douglas-luxaflex-powerview-gen-3-3-4-0-0-4-0-0-0/146805/1 |
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. The code looks good, I have added only a few comments.
...dpowerview/src/main/java/org/openhab/binding/hdpowerview/internal/HDPowerViewWebTargets.java
Outdated
Show resolved
Hide resolved
.../main/java/org/openhab/binding/hdpowerview/internal/console/HDPowerViewCommandExtension.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.hdpowerview/src/main/resources/OH-INF/thing/channels.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.hdpowerview/src/main/resources/OH-INF/thing/bridge.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.hdpowerview/src/main/resources/OH-INF/config/config.xml
Outdated
Show resolved
Hide resolved
...iew/src/main/java/org/openhab/binding/hdpowerview/internal/handler/GatewayBridgeHandler.java
Show resolved
Hide resolved
...erview/src/main/java/org/openhab/binding/hdpowerview/internal/handler/ShadeThingHandler.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.hdpowerview/src/main/resources/OH-INF/thing/channels.xml
Show resolved
Hide resolved
bundles/org.openhab.binding.hdpowerview/src/main/resources/OH-INF/thing/channels.xml
Show resolved
Hide resolved
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
...ain/java/org/openhab/binding/hdpowerview/internal/discovery/GatewayDiscoveryParticipant.java
Outdated
Show resolved
Hide resolved
...ng.hdpowerview/src/main/java/org/openhab/binding/hdpowerview/internal/GatewayWebTargets.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
...ng.hdpowerview/src/main/java/org/openhab/binding/hdpowerview/internal/GatewayWebTargets.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch> Signed-off-by: Thomas Burri <thomas.burri@alstomgroup.com>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch> Signed-off-by: Matt Myers <mmyers75@icloud.com>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch> Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Resolves #12678
Background
This PR adds support for Hunter Douglas Generation 3 hubs into the binding (which currently only supports Generation 1 & 2 hubs).
Note: the binding code in this PR automatically defaults to Generation 1/2 functionality unless a Generation 3 hub is pro-actively detected. Therefore the JAR resulting from this PR should run perfectly on Generation 1/2 hubs.
Specifics of Changes
The following classes have been added or modified for Generation 3 support..
New DTO Classes
Note that some of these DTO classes have the same class names as already existing Gen 1/2 DTO classes.
New Handler Classes
New Discovery Classes
Other New Classes
Changes to Existing Classes
JAR Files for Testing
You can download the current binding JAR files HERE
Signed-off-by: Andrew Fiddian-Green software@whitebear.ch