WIP - convert zha remotes from binary sensors to events by dmulcahey · Pull Request #14902 · home-assistant/core

dmulcahey · 2018-06-10T01:05:56Z

Description:

This PR removes zha binary_sensor entities and converts them to events based on feedback from this PR: #14795

houndci-bot · 2018-06-10T01:06:02Z

+                                EventOrigin.remote)
+        self._level = level
+        self._hass.bus.fire('zha.level_change', { 'device': self._id,
+                    'level': self._level }, EventOrigin.remote)


continuation line under-indented for visual indent
whitespace before '}'

houndci-bot · 2018-06-10T01:06:02Z

+            self._hass.bus.fire('zha.on', { 'device': self._id },
+                                EventOrigin.remote)
+        self._level = level
+        self._hass.bus.fire('zha.level_change', { 'device': self._id,


whitespace after '{'

houndci-bot · 2018-06-10T01:06:02Z

+            self._hass.bus.fire('zha.off', { 'device': self._id },
+                                EventOrigin.remote)
+        elif level > 0 and self._level == 0:
+            self._hass.bus.fire('zha.on', { 'device': self._id },


whitespace after '{'
whitespace before '}'

houndci-bot · 2018-06-10T01:06:02Z

+    def set_level(self, level):
+        """Set the level."""
+        if level == 0 and self._level > 0:
+            self._hass.bus.fire('zha.off', { 'device': self._id },


whitespace after '{'
whitespace before '}'

houndci-bot · 2018-06-10T01:06:02Z

+    def __init__(self, hass, cluster, discovery_info):
+      """Initialize Switch."""
+      super().__init__(hass, cluster, discovery_info)
+      self._level = 0


indentation is not a multiple of four

houndci-bot · 2018-06-10T01:06:03Z

+
+    def __init__(self, hass, cluster, discovery_info):
+      """Initialize Switch."""
+      super().__init__(hass, cluster, discovery_info)


indentation is not a multiple of four

houndci-bot · 2018-06-10T01:06:03Z

+    """Switch / remote event for zha"""
+
+    def __init__(self, hass, cluster, discovery_info):
+      """Initialize Switch."""


indentation is not a multiple of four

houndci-bot · 2018-06-10T01:06:03Z

+        self._ieee = discovery_info['endpoint'].device.ieee
+        self._ieeetail = ''.join(['%02x' % (o, ) for o in self._ieee[-4:]])
+        if discovery_info['manufacturer'] and discovery_info['model'] is not \
+                                None:


continuation line over-indented for visual indent

houndci-bot · 2018-06-10T01:06:03Z

+                await safe(cluster.bind())
+                await safe(cluster.configure_reporting(0, 1, 600, 1))
+            self._hass.data[DATA_ZHA_EVENT].append(ZHALevelEvent(self._hass,
+                                    cluster, discovery_info))


continuation line under-indented for visual indent

houndci-bot · 2018-06-10T01:06:03Z

+                await safe(cluster.bind())
+                await safe(cluster.configure_reporting(0, 0, 600, 1))
+            self._hass.data[DATA_ZHA_EVENT].append(ZHASwitchEvent(self._hass,
+                                    cluster, discovery_info))


continuation line under-indented for visual indent

houndci-bot · 2018-06-10T01:26:29Z

+                                EventOrigin.remote)
+        self._level = level
+        self._hass.bus.fire('zha.level_change', {'device': self._id,
+                                                 'level': self._level}, EventOrigin.remote)


line too long (91 > 79 characters)

houndci-bot · 2018-06-10T01:26:29Z

+                await safe(cluster.bind())
+                await safe(cluster.configure_reporting(0, 1, 600, 1))
+            self._hass.data[DATA_ZHA_EVENT].append(ZHALevelEvent(self._hass,
+                                                                 cluster, discovery_info))


line too long (90 > 79 characters)

houndci-bot · 2018-06-10T01:26:29Z

+                await safe(cluster.bind())
+                await safe(cluster.configure_reporting(0, 0, 600, 1))
+            self._hass.data[DATA_ZHA_EVENT].append(ZHASwitchEvent(self._hass,
+                                                                  cluster, discovery_info))


line too long (91 > 79 characters)

balloob · 2018-06-10T14:01:18Z

+        self._ieeetail = ''.join(['%02x' % (o, ) for o in self._ieee[-4:]])
+        if discovery_info['manufacturer'] and discovery_info['model'] is not \
+                None:
+            self._id = "{}.{}_{}_{}_{}{}".format(


Why is the ID so complicated ?

I kept it what it was before to try to minimize the impact of the breaking change. Zigbee devices consist of endpoints that expose functionality on clusters as attributes. device -> endpoints -> clusters -> attributes and commands. 1 physical device may expose several functions. so what has been done up to this point is:

domain.manufacturer_model_short_address_endpoint_id_cluster_id

here is an example of a single device that exposes motion and temperature sensors:
binary_sensor.centralite_3326l_0e35a368_1_1280
sensor.centralite_3326l_0e35a368_1_1026

does this make sense?

balloob · 2018-06-10T14:01:45Z

+            )
+        else:
+            self._id = "{}.zha_{}_{}{}".format(
+                'zha',


So eh, now it will be zha.zha_ ? Anything that's not dynamic you can just put in the string.

I'll take a look at this case. Pretty sure that is supposed to be domain and I messed something up there.

balloob · 2018-06-10T14:02:16Z

+    def cluster_command(self, tsn, command_id, args):
+        """Handle commands received to this cluster."""
+        if command_id in (0x0000, 0x0040):
+            self._hass.bus.fire('zha.off', {'device': self._id},


Please extract the event names as EVENT_XXX constants at the top

balloob · 2018-06-10T14:53:05Z

+    @callback
+    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


I would love to see all things like 0x0000 and 0x0004 be extracted into constants with names like MOVE_TO_LEVEL etc. These constants should at least live at the top of the file, but preferably they are moved to ZHA.

agreed. I can try to tackle that this week in separate PR's to both repos.

Can you start with putting them in constants defined at the top of this file

something like?

cmd = zigpy.zcl.clusters.general.LevelControl.server_commands.get(command_id, ('unknown'))[0] if cmd in ('move_to_level', 'move_to_level_with_on_off'): pass

houndci-bot · 2018-06-10T15:03:33Z

+                                EventOrigin.remote)
+        self._level = level
+        self._hass.bus.fire(LEVEL_CHANGE_EVENT_KEY, {DEVICE: self._id,
+                                LEVEL: self._level}, EventOrigin.remote)


continuation line under-indented for visual indent

houndci-bot · 2018-06-10T15:03:33Z

+        self._cluster.add_listener(self)
+        self._ieee = discovery_info['endpoint'].device.ieee
+        self._ieeetail = ''.join(['%02x' % (o, ) for o in self._ieee[-4:]])
+        self._domain = domain;


statement ends with a semicolon

houndci-bot · 2018-06-10T15:03:33Z

+                await safe(cluster.bind())
+                await safe(cluster.configure_reporting(0, 1, 600, 1))
+            self._hass.data[DATA_ZHA_EVENT].append(ZHALevelEvent(self._hass,
+                                                    cluster, discovery_info))


continuation line under-indented for visual indent

balloob · 2018-06-10T15:44:53Z

+        """Increment the level."""
+        self.set_level(min(255, max(0, self._level + change)))
+
+    def set_level(self, level):


This is called from a method annotated with @callback. This means that it is run inside the event loop. Please also annotate this method with callback to indicate that. Preferably you prefix all methods likeasync_set_level, to indicate it's inside the event loop.

cluster_command is a function called by zigpy. I may have added @callback incorrectly. Is there a way to determine if this is the case?

I think that zigpy runs inside the event loop, check the internals of the module or look at how the other code that interacts with zigpy does it.

balloob · 2018-06-10T15:45:09Z

+    def set_level(self, level):
+        """Set the level."""
+        if level == 0 and self._level > 0:
+            self._hass.bus.fire(


Inside the event loop, you need to call bus.async_fire

balloob · 2018-06-10T15:46:21Z

+        self._cluster.add_listener(self)
+        self._ieee = discovery_info['endpoint'].device.ieee
+        self._ieeetail = ''.join(['%02x' % (o, ) for o in self._ieee[-4:]])
+        self._domain = domain


This instance variable is not used.

balloob · 2018-06-10T15:46:38Z

+        self._hass = hass
+        self._cluster = cluster
+        self._cluster.add_listener(self)
+        self._ieee = discovery_info['endpoint'].device.ieee


This instance variable is not used.

balloob · 2018-06-10T15:47:40Z

+            )
+        self._level = level
+        self._hass.bus.fire(
+            LEVEL_CHANGE_EVENT_KEY,


Wouldn't something that has a level be a sensor ?

I think that's why this was originally added as a binary sensor. the binary sensor was trying to track on vs. off and level was an attribute.

I'm not sure how to feel about "level". On one hand I'd expect event to be stateless, so no actual level value, just "level_up" or "level_down" events, but on the other hand it is nice to have everything wired together out of the box.

Adminiuga · 2018-06-11T16:54:56Z

+            if args[0] == 0xff:
+                rate = 10  # Should read default move rate
+            self.move_level(-rate if args[0] else rate)
+        elif command_id == 0x0002:  # step


there's 'step' and 'step_with_on_off' == '0x06. We should handle both of those. Check #14756

Will do, ty

Adminiuga · 2018-06-11T17:07:56Z

+            self._hass.bus.fire(
+                OFF_EVENT_KEY,
+                {DEVICE: self._id},
+                EventOrigin.remote


What about either having an attribute indicating how many times the key was pressed in the last second or two (clicks_per_second), or maybe a separate event indicating double/triple clicks? eg for the 1st "off" command received in the last 2s time window we fire OFF_EVENT_KEY, for the 2nd command received in the last 2s windows, we fire OFF_DOUBLE_EVEN_KEY or anything like this?
I could get some fancy automation with events like this, like controlling group of lights with a single remote.

Maybe in a subsequent PR. For now I just want to focus on the functionality that was already here.

Adminiuga · 2018-06-11T17:29:41Z

+            cluster = out_clusters[LevelControl.cluster_id]
+            if discovery_info['new_join']:
+                await safe(cluster.bind())
+                await safe(cluster.configure_reporting(0, 1, 600, 1))


I'm not sure having "min_report_interval" at such low value is a good idea. Two reasons:

remotes are unlikely to send attribute reports anyway, as those usually just send "cluster commands"

if for some reason it would send attribute reports (which I still have to see), you are risking to put too much traffic onto network. I've tried it with the lights and when dimming more then one light at the same time, those reports were overloading the network to the point it was causing Delivery Timeout exceptions.

All of this came from the binary_sensor component of zha. It is all pre-existing logic. I’ll look into it as well though.

Adminiuga · 2018-06-11T17:37:47Z

-        )
+        from zigpy.zcl.clusters.general import OnOff
+        if component == 'binary_sensor' and OnOff.cluster_id == \
+                cluster.cluster_id:


Should we better add something like SINGLE_OUTPUT_CLUSTER_EVENT_CLASS to zha/const.py and handle those in a separate pass from handling SINGLE_INPUT/OUTPUT_DEVICE_CLASSes??? in current implementation, a "hypothetical" IAS device with an OnOff input cluster would cause the event handler to be instantiated, even if there's no OnOff output cluster, wouldn't it?

Yes, was speaking to @damarco about this as well. I am going to fix this. I ordered some Xiaomi sensors to test this case specifically because apparently they work this way. I have to add a sensor class back into binary_sensor to fix this as well.

balloob · 2018-09-21T13:19:37Z

What's the status of this, merge conflicts and it is still marked as WIP

dmulcahey · 2018-09-21T13:28:10Z

I’ll try to pick this back up this weekend

Adminiuga · 2018-09-21T15:09:08Z

            self._config,
        )

+    async def _async_setup_remote(self, discovery_info):


should we move this into a separate file, just to keep __init__.py less cluttered?

Adminiuga · 2018-09-21T15:09:54Z

        pass


+class ZHAEvent(object):


same as above

dmulcahey · 2018-10-13T13:46:38Z

closing this and starting fresh off of current dev. will reopen and reference this.

homeassistant added integration: zha platform: binary_sensor.zha cla-signed labels Jun 10, 2018

ghost added the in progress label Jun 10, 2018

houndci-bot reviewed Jun 10, 2018

View reviewed changes

dmulcahey mentioned this pull request Jun 10, 2018

Force update of zha remotes to make automation easier #14795

Closed

homeassistant added the cla-signed label Jun 10, 2018

houndci-bot reviewed Jun 10, 2018

View reviewed changes

balloob reviewed Jun 10, 2018

View reviewed changes

houndci-bot reviewed Jun 10, 2018

View reviewed changes

balloob added the breaking-change label Jun 10, 2018

balloob reviewed Jun 10, 2018

View reviewed changes

homeassistant added the cla-signed label Jun 10, 2018

Adminiuga mentioned this pull request Jun 11, 2018

Restore state of ZHA binary_sensor if available #14755

Closed

2 tasks

Adminiuga reviewed Jun 11, 2018

View reviewed changes

dmulcahey requested review from Danielhiversen, amelchio, bachya, cdce8p and dgomes as code owners July 2, 2018 11:05

dmulcahey requested review from a team and syssi as code owners July 2, 2018 11:05

dmulcahey requested a review from a team July 2, 2018 11:05

dmulcahey requested a review from a team as a code owner July 2, 2018 11:05

homeassistant added the cla-signed label Jul 2, 2018

cdce8p removed their request for review July 2, 2018 12:34

homeassistant added the cla-signed label Jul 2, 2018

dmulcahey added 8 commits July 2, 2018 14:22

remove remotes from binary_sensor in zha

572218a

add events to zha to support remote like devices

a108234

hound

429eb20

hound

0f9d8c6

constants and event id fix

02235d7

hound

27cf615

formatting

9064cdb

remove instance variables

e6d4350

dmulcahey force-pushed the dm/zha-remote-events branch from eb8bc8c to e6d4350 Compare July 2, 2018 18:24

homeassistant added the cla-signed label Jul 2, 2018

fix on off binary sensor and better event registration

3ee6dde

Adminiuga reviewed Sep 21, 2018

View reviewed changes

Comment thread homeassistant/components/zha/__init__.py

pass

class ZHAEvent(object):

Copy link
Copy Markdown

Contributor

Adminiuga Sep 21, 2018

Choose a reason for hiding this comment

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

same as above

balloob mentioned this pull request Oct 1, 2018

Use only_cache parameter in zha remotes #16711

Merged

2 tasks

dmulcahey closed this Oct 13, 2018

ghost removed the in progress label Oct 13, 2018

dmulcahey mentioned this pull request Nov 15, 2018

ZHA - add events for remote like devices #18493

Closed

dmulcahey deleted the dm/zha-remote-events branch December 26, 2018 12:11

ghost removed the platform: binary_sensor.zha label Mar 21, 2019

janiversen mentioned this pull request Apr 25, 2020

Add retry on empty modbus messages for serial protocol #34678

Merged

14 tasks

Uh oh!

Conversation

dmulcahey commented Jun 10, 2018

Description:

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!

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!

Adminiuga Jun 11, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmulcahey Jun 10, 2018 • 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!

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

Adminiuga Jun 11, 2018 •

edited

Loading

dmulcahey Jun 10, 2018 •

edited

Loading

Adminiuga Jun 11, 2018 •

edited

Loading

Adminiuga Jun 11, 2018 •

edited

Loading