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

Init statistics sensor upon HASS start #18236

Merged
merged 6 commits into from Nov 14, 2018

Conversation

Projects
None yet
3 participants
@ehendrix23
Contributor

ehendrix23 commented Nov 5, 2018

Description:

Register the state listener and read from the recorder only once HASS has been started.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

@ehendrix23 ehendrix23 requested a review from fabaff as a code owner Nov 5, 2018

@wafflebot wafflebot bot added the in progress label Nov 5, 2018

@ehendrix23 ehendrix23 force-pushed the ehendrix23:Do-initialization-upon-HASS-start branch 2 times, most recently from b3ac2ef to affa6a1 Nov 6, 2018

@ehendrix23 ehendrix23 changed the title from WIP: Perform initialization upon HASS start to Perform initialization upon HASS start Nov 9, 2018

@@ -111,10 +110,22 @@ def async_stats_sensor_state_listener(entity, old_state, new_state):

self._add_state_to_queue(new_state)

hass.async_add_job(self.async_update_ha_state, True)
self.hass.async_add_job(self.async_update_ha_state, True)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 13, 2018

Member

Use hass.async_create_task.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 13, 2018

Member

Btw, since we need to schedule this update we should use self.async_schedule_update_ha_state instead.

This comment has been minimized.

@ehendrix23

ehendrix23 Nov 14, 2018

Contributor

I changed it to self.async_schedule_update_ha_state passing True so that our update method is called first.

async_track_state_change(
self.hass, self._entity_id, async_stats_sensor_state_listener)

if 'recorder' in self._hass.config.components:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 13, 2018

Member

Please remove the instance attribute self._hass and don't pass in hass to the entity.

This comment has been minimized.

@ehendrix23

ehendrix23 Nov 14, 2018

Contributor

Removed. :-)


if 'recorder' in self._hass.config.components:
# only use the database if it's configured
self.hass.async_add_job(self._initialize_from_database)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 13, 2018

Member

We basically should never use hass.async_add_job any longer. Either we use hass.async_create_task or hass.async_add_executor_job. Which one depends one what function we need to schedule.

This comment has been minimized.

@ehendrix23

ehendrix23 Nov 14, 2018

Contributor

Based on my understanding, because we're calling another method that is async (changed the name of this method now to _async_initialize_from_database) it should thus be async_create_task. If it would be a sync method then I would use async_add_executor_job correct?

@@ -275,5 +286,7 @@ def _purge_old(self):
for state in reversed(states):
self._add_state_to_queue(state)

self.hass.async_add_job(self.async_update_ha_state, True)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 13, 2018

Member

See above.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 13, 2018

Member

Why do we need to add this?

This comment has been minimized.

@ehendrix23

ehendrix23 Nov 14, 2018

Contributor

Once HASS is started, we register our listener for state changes on the entity we calculate statistics for.
Then we read from the database (if recorder is one of the components) any previous state data available for that entity.
Once we read it, we call to update our own state and thus do calculations based on what was in the database. This call is to do this.
Another method (I think) would be to use await in our startup and then when we come back to the update state call.

ehendrix23 added some commits Nov 5, 2018

Update query to include maxAge
Updated the query from recorded to include MaxAge if set; reducing the amount of records retrieved that would otherwise be purged anyway for the sensor.
Initialization upon HASS start
Register the state listener and read previous information from recorder once HASS is started.
Updated test_statistics.py for HASS start
Updated test_statistics.py to start HASS and wait it is completed before running test.
Added newline in docstring
Added newline in docstring.
Added start of HASS to test_initialize_from_database_with_maxage
Added start of HASS to new test test_initialize_from_database_with_maxage.

@ehendrix23 ehendrix23 force-pushed the ehendrix23:Do-initialization-upon-HASS-start branch from affa6a1 to 7e8d307 Nov 14, 2018

Updates based on review
Following updates based on review:
-) Removed self._hass and passing hass
-) Changed async_add_job to async_create_task
-) For state update, calling async_schedule_update_ha_state
@MartinHjelmare

Looks good! Can be merged when build passes.

@MartinHjelmare MartinHjelmare changed the title from Perform initialization upon HASS start to Init statistics sensor upon HASS start Nov 14, 2018

@MartinHjelmare MartinHjelmare merged commit d2e102e into home-assistant:dev Nov 14, 2018

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 93.046%
Details

@wafflebot wafflebot bot removed the in progress label Nov 14, 2018

@ahayworth ahayworth referenced this pull request Nov 24, 2018

Merged

Add Awair sensor platform #18570

7 of 7 tasks complete

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment