Skip to content

zha: Clean up listener registration and button state #14197

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.

Sign up for GitHub

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

Merged
balloob merged 1 commit into from
May 1, 2018
Merged

zha: Clean up listener registration and button state #14197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions homeassistant/components/binary_sensor/zha.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,6 @@ def attribute_updated(self, attrid, value):
"""Handle attribute updates on this cluster."""
if attrid == 0:
self._entity.set_state(value)
self._entity.schedule_update_ha_state()

def zdo_command(self, *args, **kwargs):
"""Handle ZDO commands on this cluster."""
Expand Down Expand Up @@ -202,14 +201,14 @@ def zdo_command(self, *args, **kwargs):

def __init__(self, **kwargs):
"""Initialize Switch."""
super().__init__(**kwargs)
self._state = True
self._level = 255
from zigpy.zcl.clusters import general
self._out_listeners = {
general.OnOff.cluster_id: self.OnOffListener(self),
general.LevelControl.cluster_id: self.LevelListener(self),
}
super().__init__(**kwargs)

@property
def is_on(self) -> bool:
Expand All @@ -227,20 +226,20 @@ def move_level(self, change):
self._level = 0
self._level = min(255, max(0, self._level + change))
self._state = bool(self._level)
self.schedule_update_ha_state()
self.async_schedule_update_ha_state()
Copy link
Copy Markdown
Member

@balloob balloob Apr 30, 2018

Choose a reason for hiding this comment

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

This is called from an async context?

Copy link
Copy Markdown
Contributor Author

@rcloran rcloran Apr 30, 2018

Choose a reason for hiding this comment

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

@MartinHjelmare's comments on #12528 suggested I should change this ... but, reading the code, it looks like I shouldn't be changing this. These are run from callbacks. Since async_... is a coro, I'd need to ensure it runs, and that's exactly what the schedule_... does.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Apr 30, 2018

Choose a reason for hiding this comment

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

We need to know what is calling the callback, a thread or the event loop. That was my question.

Copy link
Copy Markdown
Contributor Author

@rcloran rcloran May 1, 2018

Choose a reason for hiding this comment

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

The event loop

Copy link
Copy Markdown
Member

@balloob balloob May 1, 2018

Choose a reason for hiding this comment

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

Ok, in that case async_schedule_update_ha_state() is ok

Copy link
Copy Markdown
Contributor Author

@rcloran rcloran May 1, 2018

Choose a reason for hiding this comment

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

Since that is a coro, doesn’t it need to be scheduled anyways?

Copy link
Copy Markdown
Contributor Author

@rcloran rcloran May 1, 2018

Choose a reason for hiding this comment

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

Doh, I was reading async_update_ha_state, not async_schedule_update_ha_state. Alright, this all makes much more sense now.


def set_level(self, level):
"""Set the level, setting state if appropriate."""
self._level = level
self._state = bool(self._level)
self.schedule_update_ha_state()
self.async_schedule_update_ha_state()

def set_state(self, state):
"""Set the state."""
self._state = state
if self._level == 0:
self._level = 255
self.schedule_update_ha_state()
self.async_schedule_update_ha_state()

async def async_update(self):
"""Retrieve latest state."""
Expand Down
2 changes: 1 addition & 1 deletion homeassistant/components/sensor/zha.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ def attribute_updated(self, attribute, value):
_LOGGER.debug("Attribute updated: %s %s %s", self, attribute, value)
if attribute == self.value_attribute:
self._state = value
self.schedule_update_ha_state()
self.async_schedule_update_ha_state()


class TemperatureSensor(Sensor):
Expand Down
26 changes: 16 additions & 10 deletions homeassistant/components/zha/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,11 +288,6 @@ class Entity(entity.Entity):
"""A base class for ZHA entities."""

_domain = None # Must be overridden by subclasses
# Normally the entity itself is the listener. Base classes may set this to
# a dict of cluster ID -> listener to receive messages for specific
# clusters separately
_in_listeners = {}
_out_listeners = {}

def __init__(self, endpoint, in_clusters, out_clusters, manufacturer,
model, application_listener, unique_id, **kwargs):
Expand Down Expand Up @@ -321,19 +316,30 @@ def __init__(self, endpoint, in_clusters, out_clusters, manufacturer,
kwargs.get('entity_suffix', ''),
)

for cluster_id, cluster in in_clusters.items():
cluster.add_listener(self._in_listeners.get(cluster_id, self))
for cluster_id, cluster in out_clusters.items():
cluster.add_listener(self._out_listeners.get(cluster_id, self))

self._endpoint = endpoint
self._in_clusters = in_clusters
self._out_clusters = out_clusters
self._state = ha_const.STATE_UNKNOWN
self._unique_id = unique_id

# Normally the entity itself is the listener. Sub-classes may set this
# to a dict of cluster ID -> listener to receive messages for specific
# clusters separately
self._in_listeners = {}
self._out_listeners = {}
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Apr 30, 2018

Choose a reason for hiding this comment

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

This will reset the listeners added in the child. You can init the parent before the setting the rest of the child's attributes.


application_listener.register_entity(ieee, self)

async def async_added_to_hass(self):
Copy link
Copy Markdown
Member

@balloob balloob Apr 30, 2018

Choose a reason for hiding this comment

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

For a future PR: add async_will_remove_from_hass and you're almost able to load/unload ZHA while Home Assistant is running 👍

"""Callback once the entity is added to hass.

It is now safe to update the entity state
"""
for cluster_id, cluster in self._in_clusters.items():
cluster.add_listener(self._in_listeners.get(cluster_id, self))
for cluster_id, cluster in self._out_clusters.items():
cluster.add_listener(self._out_listeners.get(cluster_id, self))

@property
def unique_id(self) -> str:
"""Return a unique ID."""
Expand Down