Create zwave devices on OZW thread and only add them during discovery by andrey-git · Pull Request #6096 · home-assistant/core

andrey-git · 2017-02-19T09:06:20Z

Description:

Create zwave devices on OZW thread and only add them during discovery

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass

…into zwave_thread

mention-bot · 2017-02-19T09:06:21Z

@andrey-git, thanks for your PR! By analyzing the history of the files in this pull request, we identified @turbokongen, @fabaff and @armills to be potential reviewers.

emlove · 2017-02-19T19:08:23Z

Awesome work. This is exactly what we needed for zwave. 👍

I just have two small suggestions:

For the _DEVICES dict, can we use the zwave ValueID as the key, instead of entityID? The ValueID already has a lot of information built-in to ensure uniqueness. Technically, using EntityID shouldn't be a problem, because of the way the zwave component generates them uniqueness should never be a problem. But procedurally, we really shouldn't be using entity IDs as keys for anything until they have been added to the state machine, since that is where HASS authoritatively confirms the entity ID is valid.
in zwave.get_device, can we have it pop from the dict? Each device should only be added and accessed once, so this way we can keep the dictionary size small.

andrey-git · 2017-02-19T19:50:50Z

Will do
Since OZW thread can update the dict it is not safe to update it in loop thread.
I hope it is ok to read it in loop thread.

balloob · 2017-02-19T19:58:56Z

You could have OZW add an async job to hass to add it to the dict.

entity = get_entity(…)

@callback
def add_entity():
    hass.data[DATA_ZWAVE_DICT][entity.entity_id] = entity

hass.add_job(add_entity)

(obv replace entity_id with value id etc)

emlove · 2017-02-19T20:01:38Z

@balloob If we call that before scheduling the platform setup, do we know for sure it will have been added before setup_platform is called?

And actually, is there any reason we can't just pass the entity object directly to platform setup? After the call to platform get_device, nothing is touching that object until setup_platform.

andrey-git · 2017-02-19T20:29:46Z

@armills everything passed to platform setup must be serializable. This was the first thing I tried.

emlove · 2017-02-19T20:35:11Z

OK, I wasn't certain. Actually, what you could do is add a job like @balloob said, and have it first update the dict, then call the discovery method. That way we know for certain it happens in the correct order.

EDIT: Like this from balloob's previous example.

entity = get_entity(…)

@asyncio.coroutine
def add_entity():
    hass.data[DATA_ZWAVE_DICT][entity.entity_id] = entity
    yield from discovery.async_load_platform(hass, component, DOMAIN, {
        ...

hass.add_job(add_entity)

balloob

This looks very good. Just a few minor tweaks.

balloob · 2017-02-19T22:46:05Z

    _LOGGER.debug('name=%s node_config=%s CONF_REFRESH_VALUE=%s'
                  ' CONF_REFRESH_DELAY=%s', name, node_config,
                  refresh, delay)
    if value.command_class != zwave.const.COMMAND_CLASS_SWITCH_MULTILEVEL:


(for a future PR)

Should this filtering happening inside these methods or should get_device only be called with the correct values? (And thus the filtering done before)

Some filtering can be removed as it is already done in component. I'll look into it in another PR

balloob · 2017-02-19T23:09:38Z

    if value.genre != zwave.const.GENRE_USER:
-        return
+        return None
    if node.has_command_class(zwave.const.COMMAND_CLASS_USER_CODE):


Registering these services should stay in setup_platform

They are registered conditionally by accessing node, so must be done on OZW thread.

balloob · 2017-02-19T23:10:17Z

                value.disable_poll()
+            platform = get_platform(component, DOMAIN)
+            device = platform.get_device(
+                node=node, value=value, node_config=node_config, hass=hass)


We can move node_config out of hass.data since we pass it in here.

I am not a big fan of using **kwargs for get_device. I think that we should standardize it.

get_config(node, value, node_config):

There is no hass there because this all about creating the device. Hass will be set on the entity once added to hass.

Climate needs hass to pull the default units.

Also most platforms don't need all 4 - won't it look better if each one would "declare" only what it needs?

balloob · 2017-02-19T23:17:14Z

+    return None
+
+
+def setup_platform(hass, config, add_devices, discovery_info=None):


I think that you can make a generic setup that you just import for each platform. We should make it async for improved speed:

(to be placed in zwave/__init__.py)

@asyncio.coroutine def async_setup_platform(hass, config, async_add_devices, discovery_info=None): """Generic Z-Wave platform setup.""" if discovery_info is None or zwave.NETWORK is None: return False device = zwave.get_device(hass, discovery_info[zwave.const.DISCOVERY_DEVICE]) if device: yield from async_add_devices([device]) return True else: return False

In the case of the lock component, you could then wrap this method and register the services.

from homeassistant.components.zwave import async_setup_platform as zwave_async_setup_platform @asyncio.coroutine def async_setup_platform(hass, config, async_add_devices, discovery_info=None): result = yield from zwave_async_setup_platform(…) if not result or hass.services.has_service('lock', SERVICE_XX): return # Register services

emlove · 2017-02-19T23:21:27Z

+                def discover_device(component, device, dict_id):
+                    """Put device in a dictionary and call discovery on it."""
+                    hass.data[DATA_ZWAVE_DICT][dict_id] = device
+                    discovery.load_platform(hass, component, DOMAIN, {


We should probably use the async equivalent here, since we're already in the event loop at this point. No sense in creating a new job: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/helpers/discovery.py#L136 There's nothing waiting for discover_device to return.

load_platform will blow up when used inside the event loop, so yes. We should.

…into zwave_thread

andrey-git · 2017-02-20T20:47:24Z

Addressed comments, commented on those not addressed.

andrey-git · 2017-02-20T21:00:04Z

Something is broken - the devices are not added. I'll investigate tomorrow.

andrey-git · 2017-02-20T21:28:43Z

+            if device:
+                dict_id = value.value_id
+
+                @callback


When this was a callback it was never executed

andrey-git · 2017-02-20T21:29:08Z

Fixed

…into zwave_thread

balloob · 2017-02-21T06:30:02Z

    return None


+@callback


This is not a callback: there is a yield in this method.

balloob · 2017-02-21T06:31:08Z

+
+
+@asyncio.coroutine
+def async_setup_platform(hass, config, async_add_devices, discovery_info=None):


Why create this method instead of just directly importing it?

from homeassistant.components.zwave import async_setup_platform # noqa

That's all there is to it 👍

(noqa to tell our linter to sssh)

Done with # noqa # pylint: disable=unused-import
noqa only affects flake8

balloob · 2017-02-21T06:33:19Z

-        track_point_in_time(
-            self._hass, self.async_update_ha_state,
-            self.invalidate_after)
+        if self.hass:


Please use a guard clause:

if not self.hass: return

balloob · 2017-02-21T06:35:22Z

+            platform = get_platform(component, DOMAIN)
+            device = platform.get_device(
+                node=node, value=value, node_config=node_config, hass=hass)
+            if device:


Please use a guard clause.

if not device: return

…into zwave_thread

andrey-git · 2017-02-22T21:07:20Z

Lets not merge this into 0.39 but let it soak in dev after the release is cut.

…into zwave_thread

andrey-git · 2017-02-23T19:44:41Z

0.39 has been cut - now ready for merge :)

andrey-git added 2 commits February 19, 2017 11:04

Create zwave devices on OZW thread and only add them during discovery.

9b09960

Merge branch 'dev' of https://github.com/home-assistant/home-assistant …

5a247eb

…into zwave_thread

homeassistant added integration: zwave platform: binary_sensor.zwave cla-signed labels Feb 19, 2017

andrey-git assigned balloob Feb 19, 2017

andrey-git added For Paulus Ready for review labels Feb 19, 2017

Read and write devices dict from loop thread.

d277e9c

homeassistant added the cla-signed label Feb 19, 2017

balloob reviewed Feb 19, 2017

View reviewed changes

emlove reviewed Feb 19, 2017

View reviewed changes

andrey-git added 2 commits February 20, 2017 22:44

More async

4660b8e

Merge branch 'dev' of https://github.com/home-assistant/home-assistant …

61a3c19

…into zwave_thread

replace callback with coroutine

0ea85b8

andrey-git commented Feb 20, 2017

View reviewed changes

Merge branch 'dev' of https://github.com/home-assistant/home-assistant …

b823d08

…into zwave_thread

balloob requested changes Feb 21, 2017

View reviewed changes

balloob removed the Ready for review label Feb 22, 2017

import common function instead of callin git

456eabc

homeassistant added the cla-signed label Feb 22, 2017

Merge branch 'dev' of https://github.com/home-assistant/home-assistant …

2fab8ff

…into zwave_thread

andrey-git added the Ready for review label Feb 22, 2017

Merge branch 'dev' of https://github.com/home-assistant/home-assistant …

cf67e77

…into zwave_thread

homeassistant added the cla-signed label Feb 23, 2017

balloob approved these changes Feb 23, 2017

View reviewed changes

balloob merged commit 1d32bce into home-assistant:dev Feb 23, 2017

andrey-git deleted the zwave_thread branch February 24, 2017 06:10

home-assistant locked and limited conversation to collaborators Jun 2, 2017

ghost removed platform: binary_sensor.zwave labels Mar 21, 2019

		return None


		def setup_platform(hass, config, add_devices, discovery_info=None):



		@asyncio.coroutine
		def async_setup_platform(hass, config, async_add_devices, discovery_info=None):

Uh oh!

Conversation

andrey-git commented Feb 19, 2017

Uh oh!

mention-bot commented Feb 19, 2017

Uh oh!

emlove commented Feb 19, 2017

Uh oh!

andrey-git commented Feb 19, 2017

Uh oh!

balloob commented Feb 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emlove commented Feb 19, 2017

Uh oh!

andrey-git commented Feb 19, 2017

Uh oh!

emlove commented Feb 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emlove Feb 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrey-git commented Feb 20, 2017

Uh oh!

andrey-git commented Feb 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrey-git commented Feb 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrey-git commented Feb 22, 2017

Uh oh!

andrey-git commented Feb 23, 2017

Uh oh!

Reviewers

Assignees

Labels

balloob commented Feb 19, 2017 •

edited

Loading

emlove commented Feb 19, 2017 •

edited

Loading

emlove Feb 19, 2017 •

edited

Loading