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

Change nest to cloud push #14656

Merged
merged 18 commits into from Jun 1, 2018

Conversation

@awarecan
Contributor

awarecan commented May 27, 2018

Description:

This is the follow up of python-nest upgrade #14638. After python-nest change to stream API, we can change nest component to Cloud Push mode.

Related issue (if applicable): fixes #10442

Pull request in home-assistant.github.io with documentation (if applicable):
home-assistant/home-assistant.io#5442

Example entry for configuration.yaml (if applicable):

No configuration changes

Checklist:

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

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

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

  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

awarecan added some commits May 27, 2018

Change nest component to Cloud Push
Change sensors.nest, binary_sensors.nest and climate.nest to push mode
nest camera still need poll to update snapshot image
Also change nest component to async

awarecan added some commits May 27, 2018

@@ -104,6 +104,10 @@ def setup_platform(hass, config, add_devices, discovery_info=None):
activity_zone)]
add_devices(sensors, True)
for sensor in sensors:
hass.bus.listen(EVENT_NEST_UPDATE,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member

Use our dispatch helper instead, to communicate between component and its platforms.

This comment has been minimized.

@awarecan

awarecan May 28, 2018

Contributor

Done

discovery.load_platform(hass, 'climate', DOMAIN, {}, config)
discovery.load_platform(hass, 'camera', DOMAIN, {}, config)
_LOGGER.debug("proceeding with discovery -- climate")
await discovery.async_load_platform(hass, 'climate', DOMAIN, {}, config)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member

Use async_add_job to schedule the discovery to avoid a deadlock where the component waits for the platform to be setup which waits for the component to be setup.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member

Also use a for loop that loads all the platforms. Iterate over component and platform config discovery info.

This comment has been minimized.

@awarecan

awarecan May 28, 2018

Contributor

Done

async def async_nest_update_event_handler(self, event):
"""Update device state."""
await self.async_device_update()

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member
self.async_schedule_update_ha_state(True)

This comment has been minimized.

@awarecan

awarecan May 28, 2018

Contributor

After change to use dispatcher, can I use awit self.async_update_ha_state(True) here?
As my understanding, dispatcher will create a job for the callback, if I use async_schedule_update_ha_state, it will add another job, seems unnecessary

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member

Yes, that should also work. Then keep this method a coroutine.

This comment has been minimized.

@awarecan

awarecan May 28, 2018

Contributor

Done

async def async_nest_update_event_handler(self, event):
"""Update device state."""
await self.async_device_update()
await self.async_update_ha_state()

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member

Remove this.

"""Do not need poll thanks using Nest streaming API."""
return False
async def async_nest_update_event_handler(self, event):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member

This should be a callback and not a coroutine. Decorate it with @callback imported from core.py.

add_devices(all_devices, True)
for device in all_devices:
hass.bus.listen(EVENT_NEST_UPDATE,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member

See above.

@@ -8,9 +8,9 @@
import logging
from homeassistant.components.binary_sensor import (BinarySensorDevice)
from homeassistant.components.nest import DATA_NEST, EVENT_NEST_UPDATE
from homeassistant.components.sensor.nest import NestSensor

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member

It's not allowed to depend on another platform from a platform. You can define a common base nest entity class instead in the nest component and import that into the nest platforms.

This comment has been minimized.

@awarecan

awarecan May 28, 2018

Contributor

That is old code. Do you want to me fix that in this PR?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member

It can be done separately. I mentioned it since the event handler that is defined in the sensor nest base class is now used in this binary sensor platform.

This comment has been minimized.

@awarecan

awarecan May 28, 2018

Contributor

Will do that later

so specific an executor to save default thread pool.
"""
_LOGGER.debug("listening nest.update_event")
with ThreadPoolExecutor(max_workers=1) as executor:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member

I don't think we should do this. Can't python-nest allow a callback to be registered? The callback should be called by python-nest when a state update is needed.

Otherwise waiting for the event should be done by a coroutine asynchronously. But the current event is a threading.Event so I don't think that will work as it is now.

This comment has been minimized.

@awarecan

awarecan May 28, 2018

Contributor

My code works. run_in_executor(event.wait) is the way to glue threading.Event and asyncio, and async_nest_update_event_broker is a coroutine.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member

Yes, but it's blocking a thread, which isn't good.

This comment has been minimized.

@awarecan

awarecan May 28, 2018

Contributor

That is why I am creating a new executor, so that I just block my own thread, won't affect others.

python-nest's author is not responsive like you guys, the stream API changes have to wait months to get review and merged. I think we better to use whatever we have now.

BTW, I really dig myself into a hole. That update_event was introduce by myself to the python-nest, I should allow caller pass in an optional event object

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member

The new executor is what I don't like.

Maybe @balloob has a better idea than me how to do this?

This comment has been minimized.

@balloob

balloob May 28, 2018

Member

The dev of python-nest seems active right now, if it's a small change should probably be able to get in quickly?

This comment has been minimized.

@awarecan

awarecan May 28, 2018

Contributor

What is the prefer way? An simple callback function enough?

This comment has been minimized.

@awarecan

awarecan May 28, 2018

Contributor

@balloob I am rethinking that under the influence of caffeine, I feel like current implement is not bad from python-nest perspective. As a service provider, it doesn't know what its client will put in the callback, it might be better just use thread event to loosen the coulping.

My next change for python-nest will be allow only monitor part of the data, so I may need multiple update event, and I may try to async-ized whole package, at that time, I will provide an better interface for coroutine call back.

This comment has been minimized.

@balloob

balloob May 29, 2018

Member

Ok, that sounds good. If you go async the whole package, consider allowing passing in an aiohttp client session, that way it is best suited for HASS 😉

awarecan added some commits May 28, 2018

Refactoring load_platform
Move service registration into async_setup_nest(),
 resolve an issue that before the first time configuration done,
 set_mode service should not be registered
add_devices(all_devices, True)
for device in all_devices:
async_dispatcher_connect(hass, SIGNAL_NEST_UPDATE,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member

This should be done inside the entity coroutine method async_added_to_hass. Otherwise there might be an error if the entity state update methods are called before the entity has been added to home assistant.

async def async_nest_update_callback(self):
"""Update device state."""
await self.async_update_ha_state(True)

This comment has been minimized.

@balloob

balloob May 28, 2018

Member

We should never await this. Just call self.self.async_schedule_update_ha_state(True) (no need to await)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 28, 2018

Member

@balloob It's the coroutine async_update_ha_state not the callback async_schedule_update_ha_state.

This comment has been minimized.

@awarecan

awarecan May 28, 2018

Contributor

I will remove callback from function name to avoid confusion

from homeassistant.components.sensor.nest import NestSensor
from homeassistant.const import CONF_MONITORED_CONDITIONS
from homeassistant.components.nest import DATA_NEST
from homeassistant.helpers.dispatcher import async_dispatcher_connect

This comment has been minimized.

@houndci-bot

houndci-bot May 28, 2018

'homeassistant.helpers.dispatcher.async_dispatcher_connect' imported but unused

@@ -8,9 +8,10 @@
import logging
from homeassistant.components.binary_sensor import (BinarySensorDevice)
from homeassistant.components.nest import DATA_NEST, SIGNAL_NEST_UPDATE

This comment has been minimized.

@houndci-bot

houndci-bot May 28, 2018

'homeassistant.components.nest.SIGNAL_NEST_UPDATE' imported but unused

_LOGGER.debug("listening nest.update_event")
with ThreadPoolExecutor(max_workers=1) as executor:
while True:
await hass.loop.run_in_executor(executor, nest.update_event.wait)

This comment has been minimized.

@balloob

balloob May 29, 2018

Member

The problem with this is that now Home Assistant is unable to shut down until an event comes in from Nest.

This comment has been minimized.

@awarecan

awarecan May 29, 2018

Contributor

Already think about it, next line will check if hass.is_running, and at line 172 shut_down method, I set the event to allow this statement continue

This comment has been minimized.

@balloob

balloob May 29, 2018

Member

Yes, but if we don't get any data, we will never return and then never check is_running.

Once the HOME_ASSISTANT_STOP event fires, we need to quit asap.

This comment has been minimized.

@awarecan

awarecan May 29, 2018

Contributor

Here is line 172, I am listening HOME_ASSISTANT_STOP and set the event

    def shut_down(event):
        """Stop Nest update event listener."""
        if nest:
            nest.update_event.set()

    hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, shut_down)
@awarecan

This comment has been minimized.

Contributor

awarecan commented May 30, 2018

Caught a bug in python-nest, jkoelker/python-nest#136. We better wait this fix got merged.

This error most likely no to affect current poll implementation, since all non-coroutine update will execute sequentially, the thread re-entry won't be triggered

Upgrade to python-nest 4.0.1
Fix a thread race condition issue
@awarecan

This comment has been minimized.

Contributor

awarecan commented May 31, 2018

@balloob @MartinHjelmare upstream package has updated, this PR is good for merge now.

@balloob balloob added the new-feature label Jun 1, 2018

_LOGGER.error("An error occurred while setting temperature: %s",
api_error)
# restore target temperature
self.hass.add_job(self.async_update_state)

This comment has been minimized.

@balloob

balloob Jun 1, 2018

Member

I don't really see why we would have to restore target temperature? Where was it overridden?

Also, just call hass.schedule_update_ha_state(True)

"""Do not need poll thanks using Nest streaming API."""
return False
async def async_update_state(self):

This comment has been minimized.

@balloob

balloob Jun 1, 2018

Member

Can you define this function inside async_added_to_hass, I wouldn't want to have it accidentally clash with a future function on entity with a similiar name.

@balloob

balloob approved these changes Jun 1, 2018

@balloob balloob merged commit cba8333 into home-assistant:dev Jun 1, 2018

4 of 5 checks passed

coverage/coveralls Coverage decreased (-0.5%) to 93.48%
Details
Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@awarecan awarecan deleted the awarecan:change-nest-to-cloud-push branch Jun 1, 2018

@rossdargan

This comment has been minimized.

rossdargan commented Jun 4, 2018

The docs still say this component used cloud polling - Is that still correct?

@awarecan

This comment has been minimized.

Contributor

awarecan commented Jun 4, 2018

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Jun 4, 2018

Done.

@balloob balloob referenced this pull request Jun 8, 2018

Merged

0.71.0 #14876

cyberjacob pushed a commit to cyberjacob/home-assistant that referenced this pull request Sep 4, 2018

Change nest to cloud push (home-assistant#14656)
* Change nest component to Cloud Push

Change sensors.nest, binary_sensors.nest and climate.nest to push mode
nest camera still need poll to update snapshot image
Also change nest component to async

* Flake8 lint

* Fix async_notify_errors, it is not a coroutine

* Fix pylint

* Fix pylint, function name should shall shorter than 32

* Use dispatcher helper instead event bus

* Use async_update_ha_state(True)

* Refactoring load_platform

Move service registration into async_setup_nest(),
 resolve an issue that before the first time configuration done,
 set_mode service should not be registered

* Fix an issue that authorization failure may leave a blocked thread

* Pylinting

* async_nest_update_callback => async_update_state to avoid confusion

* Move signal handler register to async_added_to_hass

* Better handle nest api error

* Remove unnecessary register for binary_sensor

* Remove unused import

* Upgrade to python-nest 4.0.1

Fix a thread race condition issue

* Address my own comments

* Address my own comment

@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018

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