Refactor zwave discovery to entity schema #6445< 8000 /span>

emlove · 2017-03-06T02:45:32Z

Description:

This PR is a big overhaul to the zwave discovery system. Currently, the discovery code is focused around matching to a single zwave value, and then creates an entity object. If this entity requires access to multiple zwave values on a node, the current code searches all node values for the matching value each time it needs to be accessed.

This change creates a discovery schema that allows each entity to describe all of the values it will need to access ahead of time. As values are added to the network, they are checked against this schema and associated with entities as necessary. Then, when the entity needs access, it only needs to check if the value has been discovered or not. It is also possible to require multiple associated values to be discovered before creating the entity, although this is not yet in use.

The core of this functionality comes from the new class ZWaveDeviceEntityValues. This class aggregates all values that should be associated to one entity, and provides easy access to these values by schema name.

I've tested with the devices I have, but I'll need help making sure that everything works with this change.

Testing Checklist:

mention-bot · 2017-03-06T02:45:33Z

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

MartinHjelmare · 2017-03-06T03:51:01Z

homeassistant/components/zwave/util.py

-    """Get the values for a given command_class with arguments.
+def check_node_schema(node, schema):
+    """Check if node matches the passed node schema."""
+    if (const.DISC_NODE_ID in schema and


Would it be possible to make the schemas voluptuous schemas and just validate here instead?

It's something I considered, and I could go either way on it. It would mean schema definitions would be a little more verbose, since almost everything would include vol.In( wrapping the requirements. We would then gain some more flexibility though if we wanted to do more advanced selection later.

voluptuous is good for validating user input.
For code I think it is better to make it more compact and validate it with tests.

I think voluptuous seems a perfect fit for what's needed here. The validators can be written modularly. We would also not need this long trail of if and return statement, which I don't think is nice python.

Validating is not about if the code works in this case, it's to choose logic path.

In my opinion, I'd rather have the longer schema checking functions, but then more concise schema definitions, than sacrifice schema readability to make the checking functions more readable. The schemas are already a pretty big word soup as it is. The util functions will pretty much be ignored after this PR, but we'll be working in the schema definitions a lot.

I also think that voluptuous looks perfect on the surface, but isn't exactly what we need. Since values are coming in one at a time, we have to extract the individual value schemas from the definition and test against those. We might end up fighting the voluptuous normal usage patterns to get it working.

andrey-git

Thanks for you awesome PR!

One change in behavior I see here is that everything (except primary value) used to be optional. Now entity won't be created until all non-optional dependencies are satisfied.

I think this should be merged after 0.40 is cut to let this soak in dev as much as possible.

andrey-git · 2017-03-06T15:52:47Z

homeassistant/components/zwave/util.py

-    """Get the values for a given command_class with arguments.
+def check_node_schema(node, schema):
+    """Check if node matches the passed node schema."""
+    if (const.DISC_NODE_ID in schema and


Python if doesn't require parentheses (also elsewhere)h

They're used here as an alternative to backslash line continuation.

andrey-git · 2017-03-06T16:03:11Z

homeassistant/components/climate/zwave.py

-        self.set_value(
-            class_id=zwave.const.COMMAND_CLASS_THERMOSTAT_SETPOINT,
-            index=self._index, data=temperature)
+        self.values.primary.data = temperature
        self.schedule_update_ha_state()


Maybe better left for a separate PR - but I think this call is not needed.

Probably right. It should get updated on the value changed callback. I'd rather leave that separate though, since I don't have a climate device to test with.

andrey-git · 2017-03-06T16:05:59Z

homeassistant/components/cover/zwave.py

+            zwave.const.COMMAND_CLASS_SWITCH_MULTILEVEL
+            and values.primary.index == 0):
+        return ZwaveRollershutter(values)
+    elif (values.primary.command_class ==


elif values.primary.command_class in [ zwave.const.COMMAND_CLASS_SWITCH_BINARY, zwave.const.COMMAND_CLASS_BARRIER_OPERATOR]:

andrey-git · 2017-03-06T16:17:21Z

homeassistant/components/lock/zwave.py

-        alarm_level = self.get_value(class_id=zwave.const
-                                     .COMMAND_CLASS_ALARM,
-                                     label=['Alarm Level'], member='data')
+        alarm_level = self.values.alarm_level and self.values.alarm_level.data


Won't this make alarm_level boolean?

If the first expression evaluates to True, it will return the second expression, although I agree it should be rewritten to be less confusing.

andrey-git · 2017-03-06T16:38:09Z

homeassistant/components/zwave/__init__.py

-                component = workaround_component
-
-            name = "{}.{}".format(component, object_id(value))
+            name = "{}.{}".format(


The name here is pre-workaround (which can change component and thus name)

Would it be possible to use post-workaround name?

Great catch! I think what will be best is just passing a reference to device_config to ZWaveDeviceEntityValues, then recheck the node config in the equivalent location when creating the new entity.

emlove · 2017-03-06T18:26:36Z

I definitely agree about saving this for 0.41.

Right now in the schema I've defined all values other than the primary as optional, to mimic the old behavior. Once we've worked out the kinks we can come back and review each platform for what makes sense.

andrey-git · 2017-03-06T19:10:50Z

homeassistant/components/zwave/__init__.py

-                class_id=[const.COMMAND_CLASS_SENSOR_MULTILEVEL,
-                          const.COMMAND_CLASS_METER],
-                label=['Power'], member='value_id',
-                instance=self._value.instance)


Power consumption should be read from a value with same instance as the primary

Right now I have ZWaveDeviceEntityValues require all associated values to have the same instance. This is a little more aggressive on instance checking than previously for some of the other associations, but it should be the right thing to do. https://github.com/home-assistant/home-assistant/pull/6445/files#diff-34736f9c4da86b6334cf5d201370707fR833

Oh, I missed that.
It would remove battery and wakeup from "subdevices" as there is only a single instance of those.

I guess that's OK.

Good point. I suppose I'll have to make it a little more advanced. Any preference on how "same instance as primary" should look in the schema? Maybe we have separate sections for node values and instance values?

Actually I have plans to make per-Node zwave entities with more data and functionality. Then wakeup and battery would move there anyway.

So let's keep the "always same instance" rule you had in the first version.

Oh, I see you already solved this. Great!

Yeah, we can always refine this later, but this at least gives us a jumping off point.

Thanks again for the great feedback!

balloob · 2017-03-07T04:21:33Z

Just going to link #6437 here.

balloob · 2017-03-07T04:22:34Z

homeassistant/components/zwave/__init__.py

+}
+
+
+DISCOVERY_SCHEMAS = [


We should consider moving this to it's own file.

balloob · 2017-03-07T04:38:17Z

homeassistant/components/zwave/__init__.py

@@ -752,24 +823,166 @@ def start_zwave(_service_or_event):
    return True


+class ZWaveDeviceEntityValues():


Let's move this to a new file to help with dealing with values.

So I started this, but I think it will create more of a mess than it will solve. The logic to build the entity uses a lot of module-level information from zwave that would need to be moved out of place even further to avoid circular dependencies. For example, we use DOMAIN, CONF_IGNORED, CONF_POLLING_INTENSITY, DATA_ZWAVE_DICT, and the object_id helper.

emlove · 2017-03-07T11:25:40Z

I agree about the new tests. I'll try to get some new tests written in a separate PR, since they should pass the same with or without this one. Plan is still to wait until after 0.40 for this PR. @andrey-git Let's move test planning to #6437.

houndci-bot · 2017-03-07T14:49:09Z

homeassistant/components/zwave/discovery_schemas.py

+             const.DISC_GENRE: const.GENRE_USER,
+         }})},
+]
+


blank line at end of file

andrey-git · 2017-03-08T18:05:02Z

homeassistant/components/cover/zwave.py

+                self._open_id = self.values.close.value_id
+                self._close_id = self.values.open.value_id
+                self._workaround = None
+            else:


This will remove the workaround on 2nd update.

The main if should check the _open_id & _clode_id are not set yet.

Thanks again! This one seems like a good candidate for extra required values. If open/close are both required, we could just check the workaround in init, and make this code much simpler. But like I said, for this PR I want to minimize any extra side-effects until this change has more time to be tested.

andrey-git · 2017-03-08T18:12:03Z

Except for the reverse open-close workaround I commented on - binary_sensor, sensor, switch, light & Rollershutter seem to work fine.

I don't have lock & climate.

andrey-git · 2017-03-12T16:29:47Z

I think we should merge this now and add tests / other improvements from here.
@balloob WDYT?

balloob · 2017-03-13T06:13:31Z

Sounds good 👍

This reverts commit 56abc7f.

…6564)" This reverts commit 58826b2.

balloob · 2017-03-13T06:35:34Z

Bah. The PR was outdated and crashed all the tests. I have reverted it.

emlove · 2017-03-13T17:22:25Z

Ah, well silver lining the tests work, right? I'll get this updated tonight.

* Revert "Revert "Refactor zwave discovery to entity schema (#6445)" (#6564)" This reverts commit 58826b2. * Update zwave tests for enitity schema * Fix merge error * Switch dict_id to id(self)

Refactor zwave discovery to entity schema

ab3df2a

homeassistant added the cla-signed label Mar 6, 2017

MartinHjelmare reviewed Mar 6, 2017

View reviewed changes

andrey-git self-requested a review March 6, 2017 14:08

andrey-git reviewed Mar 6, 2017

View reviewed changes

Address PR concerns

d9f38e0

homeassistant added the cla-signed label Mar 7, 2017

balloob reviewed Mar 7, 2017

View reviewed changes

homeassistant added the cla-signed label Mar 7, 2017

houndci-bot reviewed Mar 7, 2017

View reviewed changes

homeassistant/components/zwave/discovery_schemas.py Outdated

const.DISC_GENRE: const.GENRE_USER,

}})},

]

Copy link

houndci-bot Mar 7, 2017

Choose a reason for hiding this comment

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

blank line at end of file

Split DISCOVERY_SCHEMAS into separate file

071ae3f

homeassistant added the cla-signed label Mar 7, 2017

andrey-git reviewed Mar 8, 2017

View reviewed changes

Only check cover reverse workaround once

cfefffb

andrey-git added the For Paulus label Mar 12, 2017

balloob merged commit 56abc7f into home-assistant:dev Mar 13, 2017

balloob added a commit that referenced this pull request Mar 13, 2017

Revert "Refactor zwave discovery to entity schema (#6445)"

d8f998e

This reverts commit 56abc7f.

balloob mentioned this pull request Mar 13, 2017

Revert "Refactor zwave discovery to entity schema" #6564

Merged

balloob added a commit that referenced this pull request Mar 13, 2017

Revert "Refactor zwave discovery to entity schema (#6445)" (#6564)

58826b2

This reverts commit 56abc7f.

balloob added a commit that referenced this pull request Mar 13, 2017

Revert "Revert "Refactor zwave discovery to entity schema (#6445)" (#…

677153f

…6564)" This reverts commit 58826b2.

balloob mentioned this pull request Mar 13, 2017

Add "Refactor zwave discovery to entity schema" #6565

Merged

emlove deleted the zwave-entity-schema branch March 14, 2017 01:32

balloob mentioned this pull request Mar 24, 2017

0.41 #6756

Merged

emlove mentioned this pull request Apr 16, 2017

Fix for zwave RGB light setting #7137

Merged

home-assistant locked and limited conversation to collaborators Jul 17, 2017

		@@ -752,24 +823,166 @@ def start_zwave(_service_or_event):
		return True


		class ZWaveDeviceEntityValues():

Uh oh!

Refactor zwave discovery to entity schema #6445< 8000 /span>

Refactor zwave discovery to entity schema #6445

Uh oh!

Conversation

Uh oh!

Description:

Testing Checklist:

Uh oh!

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!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!