zwave refactor, don't use ozw values directly in properties. #5961
Conversation
|
@turbokongen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @armills to be potential reviewers. |
|
You forget the component :) This function can only be used inside callback (Need docstring or refactory): All stuff with: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/zwave/__init__.py#L260 are wrong. That should be run inside callback and don't be a problem. All propertys need to access a cache instead zwave function. This cache will be update from callback |
| zwave.ZWaveDeviceEntity.__init__(self, value, DOMAIN) | ||
|
|
||
| def update_properties(self): | ||
| """Callback on data changes for node values.""" | ||
| self._state = self.get_value( |
There was a problem hiding this comment.
Why not just self._state = _value.data
You are potentially bringing in data from another binary sensor in the same node.
Also everywhere else.
| @@ -51,12 +51,19 @@ class ZWaveBinarySensor(BinarySensorDevice, zwave.ZWaveDeviceEntity): | |||
| def __init__(self, value, device_class): | |||
| """Initialize the sensor.""" | |||
| self._sensor_type = device_class | |||
| self._state = None | |||
| zwave.ZWaveDeviceEntity.__init__(self, value, DOMAIN) | |||
There was a problem hiding this comment.
Move the base class call to be the first thing in the constructor
| @@ -80,26 +87,28 @@ def __init__(self, value, device_class, hass, re_arm_sec=60): | |||
| self.invalidate_after = dt_util.utcnow() + datetime.timedelta( | |||
| seconds=self.re_arm_sec) | |||
| # If it's active make sure that we set the timeout tracker | |||
| if value.data: | |||
| if self._state: | |||
There was a problem hiding this comment.
self._state is still None at this point
Need another solution for this
There was a problem hiding this comment.
We can remove this whole if-block.
We only need a timeout to reset the value to False again if it was True. Since it's not True here, we should be good.
| track_point_in_time( | ||
| self._hass, self.async_update_ha_state, | ||
| self.invalidate_after) | ||
| self._state = self.get_value( |
There was a problem hiding this comment.
Call base class to update _state
|
Currently discovery is not done in OZW thread, this means we can't touch zwave properties in constructors. |
| zwave.ZWaveDeviceEntity.__init__(self, value, DOMAIN) | ||
| self._sensor_type = device_class | ||
| self._state = None | ||
| self.update_properties() |
There was a problem hiding this comment.
This is not called on OZW thread, so can't call it.
There was a problem hiding this comment.
This is done in all zwave components, and not doing it will result in no data until the device checks in. No problem with mains connected devices, but battery devices will not check in until the wakeup.
Can we accept this behaviour?
|
|
||
| def close_cover(self): | ||
| """Close the garage door.""" | ||
| self._value.data = False | ||
| self.set_value( |
There was a problem hiding this comment.
I think self._value.data = False can stay.
doing it via set_value is not safer
|
b40b6f2 to
54427f4
Compare
|
I think that Andrey makes a good point with that we now don't set any data in the constructors. We could consider to add a call to update properties in the constructor. That way we only violate thread safety on startup but not for the rest of the runtime of HASS. That should at least solve most issues. |
031019d to
de0de9a
Compare
|
I did remove all the |
|
I think it is better to unsafe-call update_properties from constructor (or otherwise pull data if update_properties is doing too much) rather than be left with missing state. We can further improve this in subsequent PR |
|
|
||
| def value_changed(self, value): | ||
|
|
||
| def update_properties(self): |
| self._hass, self.async_update_ha_state, | ||
| self.invalidate_after) | ||
| self._state = self._value.data | ||
| self.schedule_update_ha_state() |
There was a problem hiding this comment.
Is this needed? Component calls this after update_properties()
| @@ -115,11 +117,16 @@ class ZwaveGarageDoor(zwave.ZWaveDeviceEntity, CoverDevice): | |||
| def __init__(self, value): | |||
| """Initialize the zwave garage door.""" | |||
| ZWaveDeviceEntity.__init__(self, value, DOMAIN) | |||
| self._state = None | |||
There was a problem hiding this comment.
Call update_properties() instead?
| @@ -294,11 +290,19 @@ def is_locked(self): | |||
|
|
|||
| def lock(self, **kwargs): | |||
| """Lock the device.""" | |||
| self._value.data = True | |||
| self.set_value( | |||
There was a problem hiding this comment.
Set _value.data directly? Also for unlock.
| @@ -53,6 +53,14 @@ class ZWaveSensor(zwave.ZWaveDeviceEntity): | |||
| def __init__(self, value): | |||
| """Initialize the sensor.""" | |||
| zwave.ZWaveDeviceEntity.__init__(self, value, DOMAIN) | |||
| self._state = None | |||
There was a problem hiding this comment.
You don;t need to set to None - you are calling update_properties to init those values
| @@ -38,16 +38,30 @@ class ZwaveSwitch(zwave.ZWaveDeviceEntity, SwitchDevice): | |||
| def __init__(self, value): | |||
| """Initialize the Z-Wave switch device.""" | |||
| zwave.ZWaveDeviceEntity.__init__(self, value, DOMAIN) | |||
| self._state = None | |||
There was a problem hiding this comment.
You are initing this with update_properties()
|
|
||
| def turn_on(self, **kwargs): | ||
| """Turn the device on.""" | ||
| self._value.node.set_switch(self._value.value_id, True) | ||
| self.set_value( |
There was a problem hiding this comment.
Why change the call? The lib api is more direct and compact.
Also for turn_off
| self.update_properties() | ||
| self.schedule_update_ha_state() | ||
|
|
||
| def update_attributes(self): |
There was a problem hiding this comment.
I think this should be protected, i.e. _update_attributes
Also, probably call this from constructor instead of setting to None-s
| self.node_id = self._value.node.node_id | ||
| self.location = self._value.node.location | ||
| self.battery_level = self._value.node.get_battery_level() | ||
| self.wakeup_interval = _get_wakeup(self._value.node) |
There was a problem hiding this comment.
Since this is no longer called from device_state_attributes inline the function here without the reties.
| attrs[ATTR_BATTERY_LEVEL] = battery_level | ||
|
|
||
| location = self._value.node.location | ||
| if self.battery_level: |
There was a problem hiding this comment.
battery_level and wakeup_interval could be 0, but those would still be legal values.
Use if self.battery_level is not None
07416df to
5cef5a2
Compare
|
|
||
| def unlock(self, **kwargs): | ||
| """Unlock the device.""" | ||
| self._value.data = False | ||
| self._value.date= False |
There was a problem hiding this comment.
missing whitespace around operator
| @@ -294,11 +290,10 @@ def is_locked(self): | |||
|
|
|||
| def lock(self, **kwargs): | |||
| """Lock the device.""" | |||
| self._value.data = True | |||
| self._value.date= True | |||
There was a problem hiding this comment.
missing whitespace around operator
|
👍 🐬 |
|
In the future, no need to wait for me to merge Z-Wave things @turbokongen @armills @andrey-git . I can't test it locally so you are in control 👍 |
Description:
Refactor the use of values.
I think I got them all, but discussion open :)
Related issue (if applicable): fixes #4867
Possibly #5568