Fix issues in ZHA light (#19368)

* make light report on/off and level

* refactoring and review comments

* refactor

* use boolean for set_state - review comment

* async_schedule_update_ha_state() on level change - review comment

* fix docstring - review comment

homeassistant/components/binary_sensor/zha.py

@@ -12,6 +12,9 @@ from homeassistant.components.zha.const import (
     DATA_ZHA, DATA_ZHA_DISPATCHERS, REPORT_CONFIG_IMMEDIATE, ZHA_DISCOVERY_NEW)
 from homeassistant.components.zha.entities import ZhaEntity
 from homeassistant.const import STATE_ON
+from homeassistant.components.zha.entities.listeners import (
+    OnOffListener, LevelListener
+)
 from homeassistant.helpers.dispatcher import async_dispatcher_connect
 from homeassistant.helpers.restore_state import RestoreEntity
 
@@ -175,78 +178,18 @@ class Remote(RestoreEntity, ZhaEntity, BinarySensorDevice):
 
     _domain = DOMAIN
 
-    class OnOffListener:
-        """Listener for the OnOff Zigbee cluster."""
-
-        def __init__(self, entity):
-            """Initialize OnOffListener."""
-            self._entity = entity
-
-        def cluster_command(self, tsn, command_id, args):
-            """Handle commands received to this cluster."""
-            if command_id in (0x0000, 0x0040):
-                self._entity.set_state(False)
-            elif command_id in (0x0001, 0x0041, 0x0042):
-                self._entity.set_state(True)
-            elif command_id == 0x0002:
-                self._entity.set_state(not self._entity.is_on)
-
-        def attribute_updated(self, attrid, value):
-            """Handle attribute updates on this cluster."""
-            if attrid == 0:
-                self._entity.set_state(value)
-
-        def zdo_command(self, *args, **kwargs):
-            """Handle ZDO commands on this cluster."""
-            pass
-
-        def zha_send_event(self, cluster, command, args):
-            """Relay entity events to hass."""
-            pass  # don't let entities fire events
-
-    class LevelListener:
-        """Listener for the LevelControl Zigbee cluster."""
-
-        def __init__(self, entity):
-            """Initialize LevelListener."""
-            self._entity = entity
-
-        def cluster_command(self, tsn, command_id, args):
-            """Handle commands received to this cluster."""
-            if command_id in (0x0000, 0x0004):  # move_to_level, -with_on_off
-                self._entity.set_level(args[0])
-            elif command_id in (0x0001, 0x0005):  # move, -with_on_off
-                # We should dim slowly -- for now, just step once
-                rate = args[1]
-                if args[0] == 0xff:
-                    rate = 10  # Should read default move rate
-                self._entity.move_level(-rate if args[0] else rate)
-            elif command_id in (0x0002, 0x0006):  # step, -with_on_off
-                # Step (technically may change on/off)
-                self._entity.move_level(-args[1] if args[0] else args[1])
-
-        def attribute_update(self, attrid, value):
-            """Handle attribute updates on this cluster."""
-            if attrid == 0:
-                self._entity.set_level(value)
-
-        def zdo_command(self, *args, **kwargs):
-            """Handle ZDO commands on this cluster."""
-            pass
-
-        def zha_send_event(self, cluster, command, args):
-            """Relay entity events to hass."""
-            pass  # don't let entities fire events
-
     def __init__(self, **kwargs):
         """Initialize Switch."""
         super().__init__(**kwargs)
         self._level = 0
         from zigpy.zcl.clusters import general
         self._out_listeners = {
-            general.OnOff.cluster_id: self.OnOffListener(self),
+            general.OnOff.cluster_id: OnOffListener(
-            general.LevelControl.cluster_id: self.LevelListener(self),
+                self,
+                self._out_clusters[general.OnOff.cluster_id]
+            )
         }
+
         out_clusters = kwargs.get('out_clusters')
         self._zcl_reporting = {}
         for cluster_id in [general.OnOff.cluster_id,
@@ -256,6 +199,14 @@ class Remote(RestoreEntity, ZhaEntity, BinarySensorDevice):
             cluster = out_clusters[cluster_id]
             self._zcl_reporting[cluster] = {0: REPORT_CONFIG_IMMEDIATE}
 
+        if general.LevelControl.cluster_id in out_clusters:
+            self._out_listeners.update({
+                general.LevelControl.cluster_id: LevelListener(
+                    self,
+                    out_clusters[general.LevelControl.cluster_id]
+                )
+            })
+
     @property
     def is_on(self) -> bool:
         """Return true if the binary sensor is on."""

homeassistant/components/light/zha.py

@@ -12,6 +12,9 @@ from homeassistant.components.zha.const import (
     DATA_ZHA, DATA_ZHA_DISPATCHERS, REPORT_CONFIG_ASAP, REPORT_CONFIG_DEFAULT,
     REPORT_CONFIG_IMMEDIATE, ZHA_DISCOVERY_NEW)
 from homeassistant.components.zha.entities import ZhaEntity
+from homeassistant.components.zha.entities.listeners import (
+    OnOffListener, LevelListener
+)
 from homeassistant.helpers.dispatcher import async_dispatcher_connect
 import homeassistant.util.color as color_util
 
@@ -74,6 +77,7 @@ async def _async_setup_entities(hass, config_entry, async_add_entities,
                         UNSUPPORTED_ATTRIBUTE):
                     discovery_info['color_capabilities'] |= \
                         CAPABILITIES_COLOR_TEMP
+
         zha_light = Light(**discovery_info)
         if discovery_info['new_join']:
             await zha_light.async_configure()
@@ -94,12 +98,25 @@ class Light(ZhaEntity, light.Light):
         self._color_temp = None
         self._hs_color = None
         self._brightness = None
+        from zigpy.zcl.clusters.general import OnOff, LevelControl
+        self._in_listeners = {
+            OnOff.cluster_id: OnOffListener(
+                self,
+                self._in_clusters[OnOff.cluster_id]
+            ),
+        }
 
-        import zigpy.zcl.clusters as zcl_clusters
+        if LevelControl.cluster_id in self._in_clusters:
-        if zcl_clusters.general.LevelControl.cluster_id in self._in_clusters:
             self._supported_features |= light.SUPPORT_BRIGHTNESS
             self._supported_features |= light.SUPPORT_TRANSITION
             self._brightness = 0
+            self._in_listeners.update({
+                LevelControl.cluster_id: LevelListener(
+                    self,
+                    self._in_clusters[LevelControl.cluster_id]
+                )
+            })
+        import zigpy.zcl.clusters as zcl_clusters
         if zcl_clusters.lighting.Color.cluster_id in self._in_clusters:
             color_capabilities = kwargs['color_capabilities']
             if color_capabilities & CAPABILITIES_COLOR_TEMP:
@@ -129,6 +146,11 @@ class Light(ZhaEntity, light.Light):
             return False
         return bool(self._state)
 
+    def set_state(self, state):
+        """Set the state."""
+        self._state = state
+        self.async_schedule_update_ha_state()
+
     async def async_turn_on(self, **kwargs):
         """Turn the entity on."""
         from zigpy.exceptions import DeliveryError
@@ -221,6 +243,13 @@ class Light(ZhaEntity, light.Light):
         """Return the brightness of this light between 0..255."""
         return self._brightness
 
+    def set_level(self, value):
+        """Set the brightness of this light between 0..255."""
+        if value < 0 or value > 255:
+            return
+        self._brightness = value
+        self.async_schedule_update_ha_state()
+
     @property
     def hs_color(self):
         """Return the hs color value [int, int]."""

homeassistant/components/zha/entities/listeners.py

@@ -0,0 +1,110 @@
+"""
+Cluster listeners for Zigbee Home Automation.
+
+For more details about this component, please refer to the documentation at
+https://home-assistant.io/components/zha/
+"""
+
+import logging
+
+_LOGGER = logging.getLogger(__name__)
+
+
+def parse_and_log_command(entity_id, cluster, tsn, command_id, args):
+    """Parse and log a zigbee cluster command."""
+    cmd = cluster.server_commands.get(command_id, [command_id])[0]
+    _LOGGER.debug(
+        "%s: received '%s' command with %s args on cluster_id '%s' tsn '%s'",
+        entity_id,
+        cmd,
+        args,
+        cluster.cluster_id,
+        tsn
+    )
+    return cmd
+
+
+class ClusterListener:
+    """Listener for a Zigbee cluster."""
+
+    def __init__(self, entity, cluster):
+        """Initialize ClusterListener."""
+        self._entity = entity
+        self._cluster = cluster
+
+    def cluster_command(self, tsn, command_id, args):
+        """Handle commands received to this cluster."""
+        pass
+
+    def attribute_updated(self, attrid, value):
+        """Handle attribute updates on this cluster."""
+        pass
+
+    def zdo_command(self, *args, **kwargs):
+        """Handle ZDO commands on this cluster."""
+        pass
+
+    def zha_send_event(self, cluster, command, args):
+        """Relay entity events to hass."""
+        pass  # don't let entities fire events
+
+
+class OnOffListener(ClusterListener):
+    """Listener for the OnOff Zigbee cluster."""
+
+    ON_OFF = 0
+
+    def cluster_command(self, tsn, command_id, args):
+        """Handle commands received to this cluster."""
+        cmd = parse_and_log_command(
+            self._entity.entity_id,
+            self._cluster,
+            tsn,
+            command_id,
+            args
+        )
+
+        if cmd in ('off', 'off_with_effect'):
+            self._entity.set_state(False)
+        elif cmd in ('on', 'on_with_recall_global_scene', 'on_with_timed_off'):
+            self._entity.set_state(True)
+        elif cmd == 'toggle':
+            self._entity.set_state(not self._entity.is_on)
+
+    def attribute_updated(self, attrid, value):
+        """Handle attribute updates on this cluster."""
+        if attrid == self.ON_OFF:
+            self._entity.set_state(bool(value))
+
+
+class LevelListener(ClusterListener):
+    """Listener for the LevelControl Zigbee cluster."""
+
+    CURRENT_LEVEL = 0
+
+    def cluster_command(self, tsn, command_id, args):
+        """Handle commands received to this cluster."""
+        cmd = parse_and_log_command(
+            self._entity.entity_id,
+            self._cluster,
+            tsn,
+            command_id,
+            args
+        )
+
+        if cmd in ('move_to_level', 'move_to_level_with_on_off'):
+            self._entity.set_level(args[0])
+        elif cmd in ('move', 'move_with_on_off'):
+            # We should dim slowly -- for now, just step once
+            rate = args[1]
+            if args[0] == 0xff:
+                rate = 10  # Should read default move rate
+            self._entity.move_level(-rate if args[0] else rate)
+        elif cmd in ('step', 'step_with_on_off'):
+            # Step (technically may change on/off)
+            self._entity.move_level(-args[1] if args[0] else args[1])
+
+    def attribute_updated(self, attrid, value):
+        """Handle attribute updates on this cluster."""
+        if attrid == self.CURRENT_LEVEL:
+            self._entity.set_level(value)