-
-
Notifications
You must be signed in to change notification settings - Fork 32k
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
Essent sensor #23513
Essent sensor #23513
Conversation
I will add documentation in home-assistant.io once this passes tests :) |
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.
Most of the code should move into it's own supporting library
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.
Thank you for pushing PyEssent, but more things could live there.
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.
This generally looks good 👍. Just a bunch of small comments. Let me know if I should elaborate some of them.
Right now it crashes for non-existing tariffs:
Is there a cleaner way to "uninitialize" devices like this for tariffs the user doesn't have? Or should I just catch this and not set the values, leaving the unused device "empty" instead? |
Well, this is working well! 9f343a7 may be a bit ugly though. Any suggestions for a cleaner way? |
Ignore my rambling, I got it pretty tidy now I think. Please tell me if there's anything left you are unhappy with :) |
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 :)
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.
EssentMeter
uses SCAN_INTERVAL to only update every hour, why is EssenBase.update()
Throttle
to 30min ?
I must now agree with @amelchio initial comment that EssenBase
looks redundant, you could get away with just create_meters.
I figured that putting them both on one hour could, depending on when the timers would run, cause it to skip it every now and then? Like, race condition stuff? I could use the same time for both though if you'd prefer. |
|
Well, multiple meters that will all call it? Because the EssentBase update updates all the meter data together, it could cause several calls to the same API endpoint in quick succession. |
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! Just nitpicking left ...
So, I think this is it. Code works fine :) |
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.
Yes, this looks great now. Amazing to see you pick up so fast 🎉. I hope you also like the result.
Description:
Shows you the daily energy and gas usage per tariff.
Pull request in home-assistant.io: home-assistant/home-assistant.io#9351
Example entry for
configuration.yaml
(if applicable):Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
requirements
in the manifest (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices: