-
-
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
[livisismarthome] Add support for rebooting the smart home controller #16969
Conversation
Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Sven Strohschein <[email protected]>
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. I have added a few minor comments and suggestions. Have you considered exposing this functionality as a Thing Action rather than a channel?
.../org/openhab/binding/livisismarthome/internal/client/api/entity/action/RestartActionDTO.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/openhab/binding/livisismarthome/internal/handler/LivisiBridgeHandler.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.livisismarthome/src/main/resources/OH-INF/thing/channels.xml
Show resolved
Hide resolved
bundles/org.openhab.binding.livisismarthome/src/main/resources/OH-INF/thing/channels.xml
Outdated
Show resolved
Hide resolved
.../src/main/java/org/openhab/binding/livisismarthome/internal/handler/LivisiBridgeHandler.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/openhab/binding/livisismarthome/internal/handler/LivisiBridgeHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Sven Strohschein <[email protected]>
Thank you for your feedback and code review. What do you mean with "Thing Action rather than a channel"? |
It's another way to interact with a binding. Example usage in a rule: val actions = getActions("livisismarthome", "livisismarthome:WDS:mybridge")
actions.restart() Whether this approach is preferable or not depends on the specific use-case. I guess for a simple use-case like this, an advanced stateless |
...marthome/src/main/java/org/openhab/binding/livisismarthome/internal/client/LivisiClient.java
Outdated
Show resolved
Hide resolved
.../org/openhab/binding/livisismarthome/internal/client/api/entity/action/RestartActionDTO.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/openhab/binding/livisismarthome/internal/handler/LivisiBridgeHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Sven Strohschein <[email protected]>
Hello @jlaur is it still possible to get this merged for 4.2.0? Thank you |
Unfortunately not, see https://community.openhab.org/t/openhab-4-2-milestone-builds/154315/5. 4.2 is planned for today. |
Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Sven Strohschein <[email protected]>
Only RC2 was released today, but probably only critical fixes are allowed anymore. And now everything is broken caused by the version switch, I love it... Update: It is still broken after merging main into the branch and executing spotless:apply... And it is still broken when I revert the change. Something seems to be broken with spotless (the spotless rules) now. I will wait... |
Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Sven Strohschein <[email protected]>
Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Sven Strohschein <[email protected]>
Builds seem fine now. Have not looked into this PR in detail, but it seems only this #16969 (comment) is not yet resolved? Would be nice to get this in an upcoming 4.3.0 milestone. |
Hello, the review process and the deadlines are just annoying, therefore I lost motivation to update and work on this issue... And the mentioned comment means a rewrite of the feature with an rare, user-unfriendly approach which didn't work directly. Probably I will give it a try sometime after 4.3 is released... |
@jlaur For the Special unsers of this Binding it dosen't matter how it works but it is important that is able to use 🖖🏻 |
Can you be more specific, please? |
I'm sorry to hear you feel this way. However, there is nothing I or anyone can do about the deadlines. I suppose you agree that we should not postpone a full openHAB release because one feature didn't meet the deadline? We are transparent about the deadlines, feature freeze and code freeze. When you missed the deadline in July we already had code freeze, which means no new enhancements are merged, because it can compromise the release as a whole. Regarding the review process: If you are annoyed that we have a review process, well, again I can not do anything about that. See also my previous comment about that. If you mean that you are annoyed with the way I have carried out the review of the PR at hand and/or the 16k lines I reviewed in #12440, you could have spoken up much earlier. When you just suddenly go silent, nothing will improve, and we both lost our time. 🤷♂️
Since you already did the change to use the I have removed myself as reviewer from this PR, so nothing is hindering you from finishing the PR and getting it merged. |
All we need is a simple trigger to Restart the controller this don't need to be implemented in the best or perfect way also the README can contain some Developers stuf because every day the Community wich ist using that Binding is getting less In the End Sven implemented an easy and simple way which works very well and WE are waiting the merge half a year 🤷🏼♀️ Im verry thankfull for all of You, but Sometimes "Done ist better then Perfect (undone)" Thanks 4 all 🖖🏻 |
Just to be clear, it's not me you have been waiting for half a year. I don't think there is anything more for me to do here. |
As @jlaur said, the only remaining is about a rather small adjustment of the README. I hope you understand that the documentation is an important part of every binding. Would be nice if you can fix that so we can merge this and move forward, as i think this is an important fix for this binding. |
- Readme updated to remove the auto-update veto hint Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Sven Strohschein <[email protected]>
I have now removed the auto-update veto hint from the README. I don't like the veto concept and I don't know how a user should see that this switch channel has another behaviour compared with other switch channels, but I just want to get this pull request resolved... I don't have documented an example rule for this channel, because I would rather have to document that "changed" and "received update" isn't working for this special channel (and the most users wouldn't write a rule for that channel, therefore the documentation would be confusing for the most users). |
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
This pull-request adds a switch to restart the smart home controller / bridge. This solves issue #16968