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

Add support for TPLinkSmartPlug plugin #1256

Merged

Conversation

xunleii
Copy link
Contributor

@xunleii xunleii commented Dec 18, 2020

Add the support for TP Link as alternative of PSU Controler plugin (and fix the issue #1244).

It implements 2 custom actions ([!TPLINKOFF] and [!TPLINKON]) and an option to power on the smartplug when the standby mode exits. I tested these changes on my RPi 3B (and on my laptop) and it seems working without any issues (this is barely a copy/paste of the PSU plugin, without the toggle feature).

Unfortunately, we need a keyboard to setup the IP address in the settings menu (or edit the config.json file).

Closes #1244

@UnchartedBull
Copy link
Owner

Thank you for your contribution! This looks really promising. I'm kind of busy right now, so it probably will take me a day or two to review this. But from a first glance everything looked alright!

Copy link
Owner

@UnchartedBull UnchartedBull left a comment

Choose a reason for hiding this comment

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

Really solid PR for a highly requested feature. Thank you so much!

I just have one really minor issue with this, with your changes the turnOnPowerWhenExitingSleep will exist twice which might be a bit confusing to some users. I'd prefer if we keep the already existing attribute and then just check whether TPLink SmartPlug Plugin is enabled (then use the TPLink SmartPlug) or not (then use PSUControl). I proposed some changes (altough not tested), but they should work I think. Let me know if you have any objections on removing turnOnPowerWhenExitingSleep from the TPLink Plugin :)

edit: Might be a good idea to rename the toggle to turn on power when exiting sleep (on the UI side should be more than enough) to make it less confusing.

src/app/config/config.default.ts Outdated Show resolved Hide resolved
src/app/config/config.model.ts Outdated Show resolved Hide resolved
src/app/config/config.schema.ts Outdated Show resolved Hide resolved
src/app/config/config.schema.ts Outdated Show resolved Hide resolved
src/app/config/config.service.ts Outdated Show resolved Hide resolved
src/app/settings/settings.component.html Outdated Show resolved Hide resolved
src/app/settings/settings.component.html Outdated Show resolved Hide resolved
src/app/standby/standby.component.ts Outdated Show resolved Hide resolved
src/app/settings/settings.component.html Outdated Show resolved Hide resolved
@xunleii
Copy link
Contributor Author

xunleii commented Dec 21, 2020

Really solid PR for a highly requested feature. Thank you so much!

I just have one really minor issue with this, with your changes the turnOnPowerWhenExitingSleep will exist twice which might be a bit confusing to some users. I'd prefer if we keep the already existing attribute and then just check whether TPLink SmartPlug Plugin is enabled (then use the TPLink SmartPlug) or not (then use PSUControl). I proposed some changes (altough not tested), but they should work I think. Let me know if you have any objections on removing turnOnPowerWhenExitingSleep from the TPLink Plugin :)

edit: Might be a good idea to rename the toggle to turn on power when exiting sleep (on the UI side should be more than enough) to make it less confusing.

Yep, it looks better to have only one button to do that. Moreover, it allows us to extends OctoDash with another PSU plugin more easier in the future ^^. I will apply these changes and test it today.

Also, I have an idea for this option; What about moving the option turnOnPSUWhenExitingSleep directly in the plugin settings (not affiliated to a specific plugin like PSUControl) ? I think it will be less confusing because, when we use the TPLink plugin, we need to toggle an option on the PSUControl settings.

Thanks a lot for your review.

@xunleii
Copy link
Contributor Author

xunleii commented Dec 21, 2020

After testing with suggestion, moving the turnOnPSUWhenExitingSleep option as affiliated options make more sense; currently, we need to enable PSUControl plugin to toggle turnOnPSUWhenExitingSleep option.

EDIT: I push a working draft of this idea. It also avoid breaking changes with the current PSUControl settings

@xunleii
Copy link
Contributor Author

xunleii commented Dec 21, 2020

I have another issue ... when I upgrade from the last release to the next with my PR, the settings menu is broken. The only way I found to fix it is to add manually the TPLink plugin settings section inside the config.json file.

EDIT: I found why (I forgot a requirement in the config.schema.ts) ... but now, I have the error message Your config is invalid! because the plugin settings doesn't exists inside the config file. Is it a way to autofix this ?
Fixed using the autofix mecanism (a bit modified to "autofix" only invalid parameters)

xunleii and others added 3 commits December 21, 2020 18:22
Signed-off-by: Alexandre Nicolaie <[email protected]>
Apply suggestion of Timon G. about this option, from Github

Co-authored-by: Timon G. <[email protected]>
Copy link
Owner

@UnchartedBull UnchartedBull left a comment

Choose a reason for hiding this comment

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

That migrate function is awesome! That is something that I've been wanting to do from the beginning but didn't really know how to tackle. Thanks for cleaning that up and making future config changes a lot easier!

I feel bad to request some more changes, but I think the migratePSUControl is better fitted in your new awesome migrate function! After that this is ready to be merged and will then be included in the next release :)

src/app/app.service.ts Show resolved Hide resolved
src/app/app.component.ts Outdated Show resolved Hide resolved
src/app/app.service.ts Outdated Show resolved Hide resolved
src/app/app.service.ts Outdated Show resolved Hide resolved
src/app/config/config.model.ts Outdated Show resolved Hide resolved
@xunleii
Copy link
Contributor Author

xunleii commented Dec 22, 2020

All is fixed. I also use the optional chaining in getErrors to avoid null exception when no error are found (and update the valid field when the configuration is updated, because it is only set during the init ... which is false before autofixing then true if all errors are fixed).

Thanks for your review and your suggestions.

Copy link
Owner

@UnchartedBull UnchartedBull left a comment

Choose a reason for hiding this comment

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

🚢

@UnchartedBull UnchartedBull merged commit fd3a719 into UnchartedBull:master Dec 23, 2020
kantlivelong pushed a commit to kantlivelong/OctoDash that referenced this pull request May 5, 2021
* Add support for TPLinkSmartPlug plugin

Signed-off-by: Alexandre Nicolaie <[email protected]>

* Remove `turnOnPowerWhenExitingSleep` option from TPLink settings

Apply suggestion of Timon G. about this option, from Github

Co-authored-by: Timon G. <[email protected]>

* Move turnOnPSUWhenExitingSleep to Octodash settings

Signed-off-by: Alexandre Nicolaie <[email protected]>

* Remove `migratePSUControlOption` function & clean code

Co-authored-by: Timon G. <[email protected]>

Co-authored-by: Timon G. <[email protected]>
@xunleii xunleii deleted the features/SmartTPLink-plugin branch May 20, 2021 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for OctoPrint-TPLinkSmartplug as a plugin on OctoDash
2 participants