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

[livisismarthome] Add support for rebooting the smart home controller #16969

Merged
merged 15 commits into from
Dec 28, 2024

Conversation

Novanic
Copy link
Contributor

@Novanic Novanic commented Jun 30, 2024

This pull-request adds a switch to restart the smart home controller / bridge. This solves issue #16968

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]>
@jlaur jlaur linked an issue Jun 30, 2024 that may be closed by this pull request
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. I have added a few minor comments and suggestions. Have you considered exposing this functionality as a Thing Action rather than a channel?

@jlaur jlaur added the enhancement An enhancement or new feature for an existing add-on label Jun 30, 2024
@wborn wborn added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jul 1, 2024
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]>
@Novanic
Copy link
Contributor Author

Novanic commented Jul 3, 2024

Thanks. I have added a few minor comments and suggestions. Have you considered exposing this functionality as a Thing Action rather than a channel?

Thank you for your feedback and code review. What do you mean with "Thing Action rather than a channel"?

@Novanic Novanic requested a review from jlaur July 3, 2024 19:15
@jlaur
Copy link
Contributor

jlaur commented Jul 3, 2024

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 Switch channel is quite straight-forward, just wanted to provide options. 🙂

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]>
@Novanic Novanic requested a review from jlaur July 6, 2024 18:43
@Novanic
Copy link
Contributor Author

Novanic commented Jul 6, 2024

Hello @jlaur is it still possible to get this merged for 4.2.0? Thank you

@jlaur
Copy link
Contributor

jlaur commented Jul 6, 2024

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]>
@Novanic
Copy link
Contributor Author

Novanic commented Jul 7, 2024

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.

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...

@jlaur jlaur added the rebuild Triggers Jenkins PR build label Jul 7, 2024
@jlaur jlaur added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jul 7, 2024
@jlaur jlaur added the awaiting feedback Awaiting feedback from the pull request author label Aug 15, 2024
@lsiepel
Copy link
Contributor

lsiepel commented Sep 19, 2024

eased 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 seem

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.

@Novanic
Copy link
Contributor Author

Novanic commented Dec 15, 2024

eased 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 seem

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 jlaur removed their request for review December 16, 2024 12:54
@mkrathome
Copy link

@jlaur
Hi, maybe i can ask you to make Things not that complicated?
The Users are waiting about that feature urgently because of a ram-leak from the Hardware. openHAB was the first middlware, which supports the lokal smart Home from Livisi succeessful. Since some month this feature is usable in other applications...

For the Special unsers of this Binding it dosen't matter how it works but it is important that is able to use 🖖🏻

@jlaur
Copy link
Contributor

jlaur commented Dec 17, 2024

Hi, maybe i can ask you to make Things not that complicated?

Can you be more specific, please?

@jlaur
Copy link
Contributor

jlaur commented Dec 17, 2024

Hello, the review process and the deadlines are just annoying, therefore I lost motivation to update and work on this issue...

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. 🤷‍♂️

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...

Since you already did the change to use the veto policy, I assume your plan is to revert that, or..? There is no comment from me that would require any kind of rewrite or do anything user-unfriendly. The only remaining comment is a tiny one about the README, and all you needed to do was to respond, and it could have been sorted out piecefully and quickly as well.

I have removed myself as reviewer from this PR, so nothing is hindering you from finishing the PR and getting it merged.

@mkrathome
Copy link

Hi, maybe i can ask you to make Things not that complicated?

Can you be more specific, please?

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
The users which Stands with Livisi AND using openHAB are more or less Developers by themself 🤣 all the other guys are allready with HA 🙈🤷🏼‍♀️

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 🖖🏻

@jlaur
Copy link
Contributor

jlaur commented Dec 17, 2024

In the End Sven implemented an easy and simple way which works very well and WE are waiting the merge half a year 🤷🏼‍♀️

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.

@lsiepel
Copy link
Contributor

lsiepel commented Dec 17, 2024

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]>
@Novanic
Copy link
Contributor Author

Novanic commented Dec 27, 2024

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.

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).

@Novanic Novanic requested review from lsiepel and jlaur December 27, 2024 20:25
@lsiepel lsiepel removed the awaiting feedback Awaiting feedback from the pull request author label Dec 28, 2024
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 changed the title [livisismarthome] Support for rebooting the smart home controller [livisismarthome] Add support for rebooting the smart home controller Dec 28, 2024
@jlaur jlaur merged commit 7b986e0 into openhab:main Dec 28, 2024
3 checks passed
@jlaur jlaur added this to the 5.0 milestone Dec 28, 2024
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.

[livisismarthome] Support for rebooting the smart home controller
6 participants