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

Essent sensor #23513

Merged
merged 20 commits into from
Apr 30, 2019
Merged

Essent sensor #23513

merged 20 commits into from
Apr 30, 2019

Conversation

TheLastProject
Copy link
Contributor

@TheLastProject TheLastProject commented Apr 28, 2019

Description:

Shows you the daily energy and gas usage per tariff.

image

Pull request in home-assistant.io: home-assistant/home-assistant.io#9351

Example entry for configuration.yaml (if applicable):

sensors:
  - platform: essent
    username: xxxxxx
    password: xxxxxx

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly (example).
  • New dependencies have been added to requirements in the manifest (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@TheLastProject
Copy link
Contributor Author

I will add documentation in home-assistant.io once this passes tests :)

Copy link
Contributor

@dgomes dgomes left a 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

homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dgomes dgomes left a 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.

homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
@TheLastProject TheLastProject changed the title WIP: Essent sensor Essent sensor Apr 28, 2019
homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
Copy link
Contributor

@amelchio amelchio left a 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.

homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/essent/sensor.py Show resolved Hide resolved
@TheLastProject
Copy link
Contributor Author

Right now it crashes for non-existing tariffs:

019-04-29 19:26:47 ERROR (MainThread) [homeassistant.components.sensor] essent: Error on device update!                                                                   
Traceback (most recent call last):                                                                                                                                         
  File "/home/sylvia/Projects/Forks/home-assistant/homeassistant/helpers/entity_platform.py", line 261, in _async_add_entity                                               
    await entity.async_device_update(warning=False)                                                                                                                        
  File "/home/sylvia/Projects/Forks/home-assistant/homeassistant/helpers/entity.py", line 377, in async_device_update                                                      
    await self.hass.async_add_executor_job(self.update)                                                                                                                    
  File "/usr/lib64/python3.7/concurrent/futures/thread.py", line 57, in run                                                                                                
    result = self.fn(*self.args, **self.kwargs)                                                                                                                            
  File "/home/sylvia/Projects/Forks/home-assistant/homeassistant/components/essent/sensor.py", line 89, in update                                                          
    self._meter_unit = data['values']['LVR'][self._tariff]['unit']                                                                                                         
KeyError: 'H'                                                                                                                                                              

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?

@TheLastProject
Copy link
Contributor Author

Well, this is working well! 9f343a7 may be a bit ugly though. Any suggestions for a cleaner way?

@TheLastProject
Copy link
Contributor Author

TheLastProject commented Apr 29, 2019

Ignore my rambling, I got it pretty tidy now I think. Please tell me if there's anything left you are unhappy with :)

Copy link
Contributor

@dgomes dgomes left a comment

Choose a reason for hiding this comment

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

LGTM :)

Copy link
Contributor

@dgomes dgomes left a 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.

@TheLastProject
Copy link
Contributor Author

EssentMeter uses SCAN_INTERVAL to only update every hour, why is EssenBase.update() Throttle to 30min ?

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.

@dgomes
Copy link
Contributor

dgomes commented Apr 29, 2019

EssentMeter.update() is only called by the update, I see no need to Throttle it (maybe I'm missing something)

@TheLastProject
Copy link
Contributor Author

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.

Copy link
Contributor

@amelchio amelchio left a 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 ...

homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/essent/sensor.py Outdated Show resolved Hide resolved
@TheLastProject
Copy link
Contributor Author

So, I think this is it. Code works fine :)

Copy link
Contributor

@amelchio amelchio left a 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.

@TheLastProject
Copy link
Contributor Author

Yeah, it's a lot better than the initial commit. Thank you for your patience and guidance, @amelchio and @dgomes :)

@amelchio amelchio merged commit 603e2cd into home-assistant:dev Apr 30, 2019
@balloob balloob mentioned this pull request May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants