-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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 sensors for myStrom plugs #97024
Add sensors for myStrom plugs #97024
Conversation
Hi @MadMonkey87 It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
Hey there @fabaff, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
Hi @MadMonkey87 It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
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.
Some initial feedback
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
I worked on some bugs in this release and I noticed that not every device is consistent with the rest, which ones do you have? It might be a good idea to add tests so we don't break this in a regression again, but I can get that it's a big step for a first PR. If you want I can help you with that! |
That would be very helpful, thx! Jep this is the very first time I am working on HA ;P PS: I have some legacy plugs (the ones with power measurement, but no temperature sensor and limited reporting) as well as a newer plug and some of the buttons |
So if you don't have a temperature sensor, we should look if it works, and if it doesn't, how we work with it :). But that's something for tomorrow |
I have just one of the newer ones with temperature sensor, so it works;P |
It would be nice to have some documentation for people to read about this nifty feature you added ;). Doesn't have to be huge, it can be just a sentence like, yea and the plugs have two sensors for temperature and humidity. |
Updated documentation for home-assistant/core#97024
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.
Please fix the CI and make sure it is passing.
Please also implement the requested changes of @gjohansson-ST
@@ -16,7 +16,7 @@ | |||
from .const import DOMAIN | |||
from .models import MyStromData | |||
|
|||
PLATFORMS_SWITCH = [Platform.SWITCH] | |||
PLATFORMS_SWITCH = [Platform.SWITCH, Platform.SENSOR] |
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.
Please do the renaming in this PR, as it will improve the readability of the code. This constant is only used in two places in this file.
Happy Monday everyone. |
This depends on @MadMonkey87, which is required to implement the requested changes. For this reason, the PR is in draft. We can merge the PR only if all is done, including creating a PR for the documentation. |
Thanks for the clarification. @MadMonkey87 Do you have a timeline to do these last bits so we can all benefit from your hard work? I would really love to be able to update my HA install without killing all of my automations. Thank you! |
@MadMonkey87 Ciao! Is there any way you can find the time to complete the docs etc to get these really valuable sensors pushed to be merged? |
Hello, PR documentation will follow in the next few days. Kind regards 😄 |
We don't want a new PR on this as we already have it here. You can always open a PR on @MadMonkey87 's branch to contribute from the back and see if he's still active. |
A PR has been made to update the documentation. You'll find it here |
There is still a mypy error that needs to be addressed. Please don't forget to mark this PR ready for review, once you are done 👍 ../Frenck |
@frenck As explained above, I opened the PR on @MadMonkey87's repo so as not to have duplicate PRs |
Changes addressed
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.
Please address the comments in a new PR. Thanks!
"""Representation of the consumption or temperature of a myStrom switch/plug.""" | ||
|
||
entity_description: MyStromSwitchSensorEntityDescription | ||
device: MyStromSwitch |
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.
We don't need a type annotation and class attribute for device
here as there's already a type annotation of the entity device
parameter.
@@ -16,7 +16,7 @@ | |||
from .const import DOMAIN | |||
from .models import MyStromData | |||
|
|||
PLATFORMS_SWITCH = [Platform.SWITCH] | |||
PLATFORMS_PLUGS = [Platform.SWITCH, Platform.SENSOR] |
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.
Please sort 🔡.
Proposed change
The myStrom power plugs do measure power consumption and temperature too, but the current HA is only representing them as a switch. This PR provides two more sensor entities to bring consumption & temperature into HA too.
Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: