Add zha device entity #14579

damarco · 2018-05-21T18:54:32Z

Description:

This adds a separate entity to expose device level state. The last_seen device attribute is used to derive an online/offline state.

Checklist:

The code change is tested and works locally.

Adminiuga · 2018-05-24T01:19:11Z

What's the reasoning behind having this as a "ZhaEndpointEntity" and not as a "ZhaDeviceEntity"?
It is not like different endpoints on the same physical device are going to have different RSSI/LQI

as another thought, I think it would be great to have 'nwk' address included in the device_state_attributes, so it would be easier to correlate debug messages like:

2018-05-23 19:32:09 DEBUG (MainThread) [zigpy.zcl] [0x402e:1:0x0019] ZCL request 0x0101: [0, 4388, 0, 6, 0]
2018-05-23 19:32:09 DEBUG (MainThread) [zigpy.zcl] [0x402e:1:0x0019] No handler for cluster command 1
2018-05-23 19:32:12 DEBUG (MainThread) [zigpy.zcl] [0x402e:1:0x0019] ZCL request 0x0101: [0, 4388, 0, 6, 0]

damarco · 2018-05-24T19:03:39Z

I initially started with a ZhaDeviceEntity, but on the other hand manufacturer, model and battery levels are cluster attributes bound to an endpoint. So (theoretically) those can differ between different endpoints. I guess having both (device and endpoint) is overkill, so I chose the endpoint entity to have a correct mapping for the mentioned attributes, though on the other hand you might have some redundant information when multiple endpoints are used. In reality most of the devices only have a single endpoint so it should not really make a difference.

Adminiuga · 2018-05-25T01:22:49Z

In reality most of the devices only have a single endpoint so it should not really make a difference

Not quite in my case. I have a lot of GE wall switches and those do have two endpoints. Currently out of 25 devices, 9 of those have two endpoints and currently there's only a single centralite motion sensor. And I have about 7 more switches waiting to go in :)

as about LQI/RSSI, those are "device" attributes, as those are related to "radio" and it's the "radio" which may serve those multiple endpoints, but not vice versa. And zigpy/device.py points the same thing. On the other hand ZDO itself is Enpoint #0, isn't it? So yeah, RSSI/LQI are attributed to the endpoint #0, but are they really attributed to the "application" endpoints?

As for the battery status, we already get battery sensors in HA. We could include battery status in ZhaDeviceEntity, but how much benefit would it provide? I'd rather see there hardware/firmware versions there.

And I still think that it is extremely unlikely that different endpoints are going to be using different batteries or being manufactured by different vendors and my reasoning for this is as follows: all those endpoints belong to the same radio which has an unique IEEE address. I can see why manufacturer may want to expose the "same" battery on different endpoints, but I don't see a practical reason to have two different batteries dedicated for its own endpoint and housed in the same battery powered device.

So for now my opinion is not changed and as my personal preference I'd like to see these as ZhaDeviceEntities. Unless there're other reason which I fail to see for now?

dshokouhi · 2018-05-25T02:00:36Z

As for the battery status, we already get battery sensors in HA. We could include battery status in ZhaDeviceEntity, but how much benefit would it provide? I'd rather see there hardware/firmware versions there.

We actually don't have any battery status for zha devices yet. There was a PR submitted a few months ago but it got closed out. The code may make it back but it was only for Centralite devices. I think any level of battery reporting should be done.

Adminiuga · 2018-05-26T01:28:34Z

We actually don't have any battery status for zha devices yet. There was a PR submitted a few months ago but it got closed out. The code may make it back but it was only for Centralite devices.

Yep, you're right. I forgot that PR didn't get merge. I'm running it in my private branch and so far I get 100% battery reports. Guess need to wait a bit to drain the battery, because it's coded for 2.8V max battery voltage and currently my battery measures 3V

I think any level of battery reporting should be done.

Agreed, but personally I like the battery in a form of a sensor. Although I'm open to idea of having battery indicator as an attribute of a ZhaEndpoint/DeviceEntity. Maybe should add another binary_sensor "battery_low"...

dmulcahey · 2018-05-26T13:04:27Z

@damarco Great work. I think this is going to be very helpful. Question... how come these are hidden by default? The zwave component already does something similar to this and those entities are visible by default. Shouldn't these be visible to stay consistent w/ other components that have similar functionality?

dshokouhi · 2018-05-26T17:35:25Z

I agree with @dmulcahey I actually like making it similar to zwave.

@dmulcahey any plans to add in battery sensors again or do you think they would be better fit in this entity? I remember seeing some mention about where best to place it.

dmulcahey · 2018-05-27T01:25:45Z

*edit - moved comment inline with code.

dmulcahey · 2018-05-27T12:20:02Z

homeassistant/components/zha/__init__.py

@@ -245,6 +248,16 @@ def device_removed(self, device):
                    join,
                )

+            endpoint_key = "zha-{}".format(device_key)


@damarco I’ve been thinking about this some more while also tinkering with it locally. I agree with the comments on the PR... I think ZhaDeviceEntity is a better fit for what this really represents. I completely understand how you ended up where you did. I would have done the same thing.

That being said, many devices have multiple endpoints and leaving this as is will lead to everyone getting duplicate entities for them that really represent the same information.

Would changing the name to ZhaDeviceEntity and only creating these for the ZHA endpoint and therefore only creating 1 per device make sense in your opinion? Something like this:

if endpoint.profile_id == zigpy.profiles.zha.PROFILE_ID: endpoint_key = "zha-{}".format(device_key) endpoint_entity = ZhaEndpointEntity( endpoint, discovered_info['manufacturer'], discovered_info['model'], self, endpoint_key, ) await self._component.async_add_entities([endpoint_entity])

This would avoid duplicate entities and give us a place to put other device specific attributes such as battery level, neighbor tables, route information, etc and it would end up matching what was done for zwave pretty well. It also leaves open the possibility of creating another entity for endpoints if in fact we find a use for that. Just a thought...

We should just use device_key for endpoint_key. No need to add zha to it.

Wouldn't this create a conflict if the same unique_id is used twice? It is already used in the if component: ... block above.

dmulcahey · 2018-05-27T13:12:17Z

There is an interesting side effect in the last_seen / hass state update logic for polled entities. It creates a slightly confusing ui:

The screenshot was taken at 8:25:48 am (11 seconds after the last_update time) because the device is polled for electrical measurements it is updated every ~30 seconds for a new current reading. last_seen updates accordingly but because the state of this entity doesn't "update" the time at the top isn't updated and it's a bit misleading.

I'm not sure what the best way to handle this is. I was going to suggest setting force_update to true on the entity but that breaks the online / offline handling because this entity is polled and it would cause it to update on every poll. I think we want to force update hass every time the last_seen property on the device in zigpy is updated. Does that make sense?

edit

if you think this makes sense, this may work:

add this to the entity class in init:

        self._last_update = None
        self._force_update = False

add this as a property:

    @property
    def force_update(self) -> bool:
        return self._force_update

and modify async_update to be something like this:

    async def async_update(self):
        """Handle polling."""
        self._force_update = False;
        if self._last_update != self._endpoint.device.last_seen:
            self._last_update = self._endpoint.device.last_seen
            self._force_update = True
        if self._endpoint.device.last_seen is None:
            self._state = 'offline'
        else:
            difference = time.time() - self._endpoint.device.last_seen
            if difference > self._keepalive_interval:
                self._state = 'offline'
            else:
                self._state = 'online'
        self.schedule_update_ha_state()

There may be a better way to do this but this seems to work.

damarco · 2018-05-28T08:07:03Z

Thanks for all the comments.

Visibility: I changed this to hidden by default, because I thought the information exposed by this entity will mainly be used for automations and not necessarily directly in the GUI.
Battery levels: I have seen different ways of reporting battery levels from different vendors so I think this is something that should be handled by the quirks framework in zigpy. Also because calculating a relative battery value very much depends on the battery type and brand. The actual values could then be exposed as CustomEndpoint properties and added to this entity.
Endpoint or device entity: I am open to changing this to a ZhaDeviceEntity, but what would be the actual name of the entity? I wanted to use the same naming convention that was used for other entities (manufacturer_model_ieee) so that entities referring to the same device could easily be identified by matching its names. As both manufacturer and model are attributes of an endpoint those might differ for devices with multiple endpoints. I have seen devices with multiple endpoints that only had a cluster zero on one endpoint for example.
Update status: I do not really see this as an issue as long as you look at the "x minutes ago" referring to an actual state change (not a device_state_attribute change). I think for the user the important information is the online/offline status so having an indication when this changed the last time might be useful. If one really wants to know when the last message was received from the device one can refer to the last_update attribute.

dmulcahey · 2018-05-28T13:05:09Z

@damarco

Battery levels: I have seen different ways of reporting battery levels from different vendors so I think this is something that should be handled by the quirks framework in zigpy. Also because calculating a relative battery value very much depends on the battery type and brand. The actual values could then be exposed as CustomEndpoint properties and added to this entity.

I see your point here and I agree.

As both manufacturer and model are attributes of an endpoint those might differ for devices with multiple endpoints. I have seen devices with multiple endpoints that only had a cluster zero on one endpoint for example.

I have yet to see a device where the manufacturer and model differ on endpoints. I think it is very unlikely that there would ever be a single device with endpoints where this information differs.

but what would be the actual name of the entity? I wanted to use the same naming convention that was used for other entities (manufacturer_model_ieee) so that entities referring to the same device could easily be identified by matching its names.

This is why I suggested locking the creation of these to the ZHA endpoint by profile id. This allows everything else to stay the same as it is currently and you only get 1 per device. I have devices from several manufacturers: centralite, sengled, climax technologies, securifi, SmartThings, etc. Most of them have a manufacturer specific endpoint that repeats everything on the ZHA endpoint. because of this multiple entities are created with exactly the same information with this change. I can see this leading to user confusion even with these hidden by default (which I am still not convinced that they should be.) Also, every attribute on this entity is actually pulled from the device. You're using the endpoint to back up to the device to get the data. so why not try to make these represent a device since that's what it seems to be doing anyway...

Update status: I do not really see this as an issue as long as you look at the "x minutes ago" referring to an actual state change (not a device_state_attribute change). I think for the user the important information is the online/offline status so having an indication when this changed the last time might be useful. If one really wants to know when the last message was received from the device one can refer to the last_update attribute.

I agree that if you know what you're looking at and you have some knowledge of the internals that it makes sense. Most users won't have that knowledge and I think that this has the potential to confuse casual users.

Visibility: I changed this to hidden by default, because I thought the information exposed by this entity will mainly be used for automations and not necessarily directly in the GUI.

I understand. My view here is that consistency leads to a better overall user experience. zwave has already created this type of entity and they are visible by default.

Adminiuga · 2018-05-28T17:34:36Z

Battery levels: I have seen different ways of reporting battery levels from different vendors so I think this is something that should be handled by the quirks framework in zigpy. Also because calculating a relative battery value very much depends on the battery type and brand. The actual values could then be exposed as CustomEndpoint properties and added to this entity.

Per ZCL specs, the "Battery Settings Attribute Set" should be writable. If we could allow users to set "BatterySize" (attr id 0x0032) and/or "BatteryRatedVoltage" (attr id 0x0034) then battery sensor could do some approximation based on the above and current voltage.

As both manufacturer and model are attributes of an endpoint those might differ for devices with multiple endpoints. I have seen devices with multiple endpoints that only had a cluster zero on one endpoint for example.

I have yet to see a device where the manufacturer and model differ on endpoints. I think it is very unlikely that there would ever be a single device with endpoints where this information differs.

+1

balloob · 2018-05-29T12:50:29Z

homeassistant/components/zha/__init__.py

+        self._device_state_attributes = {}
+        ieee = endpoint.device.ieee
+        ieeetail = ''.join(['%02x' % (o, ) for o in ieee[-4:]])
+        if manufacturer and model is not None:


Are you trying to check that both manufacturer and model are not none?

balloob · 2018-05-29T12:51:42Z

homeassistant/components/zha/__init__.py

+        if self._endpoint.device.last_seen is not None:
+            time_struct = time.localtime(self._endpoint.device.last_seen)
+            update_time = time.strftime("%Y-%m-%dT%H:%M:%S", time_struct)
+        self._device_state_attributes['last_update'] = update_time


We should not store update time.

Maybe update_time or last_update is not the best name. This is essentially the time when the last message was received from the device. I thought this might be useful for verification or troubleshooting purposes.

balloob · 2018-05-29T12:52:27Z

homeassistant/components/zha/__init__.py

+                self._state = 'offline'
+            else:
+                self._state = 'online'
+        self.schedule_update_ha_state()


Do not call this. This is a polling entity and so this will already be called.

balloob · 2018-05-29T12:53:19Z

homeassistant/components/zha/__init__.py

+        ieee = endpoint.device.ieee
+        ieeetail = ''.join(['%02x' % (o, ) for o in ieee[-4:]])
+        if manufacturer and model is not None:
+            self.entity_id = "{}.{}_{}_{}_{}".format(


There is no need to create such complicated entity ids. Since it has a unique ID, a unique entity ID will be assigned once and used every time. You don't have to assign one at all and it will just work.

I wanted to keep the naming consistent with the other zha entities. Otherwise the user would have no way of identifying which entities belong together.

For me personally, having "manufacturer" and "model" in the entity_id provides me with a better user experience correctly identifying devices, especially if you join more than one device at a time.

balloob · 2018-05-29T12:54:30Z

homeassistant/components/zha/__init__.py

+class ZhaEndpointEntity(entity.Entity):
+    """A base class for ZHA endpoints."""
+
+    _domain = DOMAIN


balloob · 2018-05-29T12:55:14Z

homeassistant/components/zha/__init__.py

+            )
+
+        nwk = endpoint.device.nwk
+        self._device_state_attributes['nwk'] = '0x{0:04x}'.format(nwk)


If you're assigning this all in the constructor, just do it all at once.

self._device_state_attributes = { 'ieee': str(…), # etc }

Adminiuga · 2018-05-29T14:56:03Z

Why not to keep "last_seen" maybe?

…

On Tue, May 29, 2018, 09:59 damarco ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In homeassistant/components/zha/__init__.py <#14579 (comment)> : > + """Return the state of the entity.""" + return self._state + + @Property + def hidden(self) -> bool: + """Hide by default.""" + return True + + @Property + def device_state_attributes(self): + """Return device specific state attributes.""" + update_time = None + if self._endpoint.device.last_seen is not None: + time_struct = time.localtime(self._endpoint.device.last_seen) + update_time = time.strftime("%Y-%m-%dT%H:%M:%S", time_struct) + self._device_state_attributes['last_update'] = update_time Maybe update_time or last_update is not the best name. This is essentially the time when the last message was received from the device. I thought this might be useful for verification or troubleshooting purposes. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#14579 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFjmcKhGH7C-c5iUpNZBqjwG-ES8-aMjks5t3VRAgaJpZM4UHc_O> .

damarco · 2018-06-14T06:00:17Z

I hope all of the comments are addressed now and this is ready to merge.

balloob · 2018-06-20T19:18:22Z

I wonder if we should put this PR on hold until we have official notion of devices added to Home Assistant. Discussion here: home-assistant/architecture#36

dmulcahey · 2018-06-21T12:25:34Z

@balloob any chance we could get this in now and then adapt it as necessary once the official notion of devices is settled?

balloob · 2018-06-26T16:47:18Z

homeassistant/components/zha/__init__.py

+        """Return device specific state attributes."""
+        update_time = None
+        if self._device.last_seen is not None:
+            time_struct = time.localtime(self._device.last_seen)


We should not add a last seen. This will be updated too often, causing a lot of state changes.

This is quite useful bit of information, especially when troubleshooting issues with your zigbee network.
What if we don't include "last_seen" while a device is healthy/online, but have it in the state attributes once a devices becomes unresponsive? at least we could see when there was last communication from the device that way?

That could work, however, I think that it makes more sense if someone would contribute a management panel like we have for Z-Wave.

damarco · 2018-09-16T16:55:26Z

@balloob "last_seen" is now only shown if the device is offline. I would like to add support for the new device registry in a separate PR, though as far as I understood we would need to add support for config entries first.

balloob · 2018-09-17T08:41:39Z

Correct.

homeassistant added cla-signed integration: zha labels May 21, 2018

dmulcahey reviewed May 27, 2018

View reviewed changes

balloob reviewed May 29, 2018

View reviewed changes

Adminiuga mentioned this pull request Jun 2, 2018

WIP - fix light state after manual toggle #14732

Closed

homeassistant added the cla-signed label Jun 9, 2018

ghost added the in progress label Jun 9, 2018

damarco changed the title ~~Add zha endpoint entity~~ Add zha device entity Jun 9, 2018

balloob reviewed Jun 26, 2018

View reviewed changes

damarco added 5 commits September 16, 2018 09:14

Add endpoint entity

a3def4d

Fix lint error

d8971ad

Add nwk address as device state attribute

39c1e7d

Change to ZhaDeviceEntity

a27e2b4

Show last_seen only if offline

a02e325

damarco force-pushed the zha-endpoint-entity branch from cdda9ae to a02e325 Compare September 16, 2018 16:26

homeassistant added the cla-signed label Sep 16, 2018

Remove obsolete _discover_endpoint_info()

1962db5

balloob approved these changes Sep 17, 2018

View reviewed changes

balloob merged commit 1c25100 into home-assistant:dev Sep 17, 2018

ghost removed the in progress label Sep 17, 2018

damarco deleted the zha-endpoint-entity branch September 19, 2018 12:42

balloob mentioned this pull request Sep 28, 2018

0.79.0 #16940

Merged

home-assistant locked and limited conversation to collaborators Feb 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add zha device entity #14579

Add zha device entity #14579

damarco commented May 21, 2018 •

edited

Adminiuga commented May 24, 2018

damarco commented May 24, 2018

Adminiuga commented May 25, 2018

dshokouhi commented May 25, 2018

Adminiuga commented May 26, 2018

dmulcahey commented May 26, 2018

dshokouhi commented May 26, 2018

dmulcahey commented May 27, 2018 •

edited

dmulcahey May 27, 2018 •

edited

balloob May 29, 2018

damarco May 29, 2018

dmulcahey commented May 27, 2018 •

edited

damarco commented May 28, 2018

dmulcahey commented May 28, 2018

Adminiuga commented May 28, 2018

balloob May 29, 2018

balloob May 29, 2018

damarco May 29, 2018

balloob May 29, 2018

balloob May 29, 2018

damarco May 29, 2018

Adminiuga Jun 11, 2018

balloob May 29, 2018

balloob May 29, 2018

Adminiuga commented May 29, 2018 via email

damarco commented Jun 14, 2018

balloob commented Jun 20, 2018

dmulcahey commented Jun 21, 2018

balloob Jun 26, 2018

Adminiuga Jun 26, 2018

balloob Jul 2, 2018

damarco commented Sep 16, 2018

balloob commented Sep 17, 2018

Add zha device entity #14579

Add zha device entity #14579

Conversation

damarco commented May 21, 2018 • edited

Description:

Checklist:

Adminiuga commented May 24, 2018

damarco commented May 24, 2018

Adminiuga commented May 25, 2018

dshokouhi commented May 25, 2018

Adminiuga commented May 26, 2018

dmulcahey commented May 26, 2018

dshokouhi commented May 26, 2018

dmulcahey commented May 27, 2018 • edited

dmulcahey May 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmulcahey commented May 27, 2018 • edited

damarco commented May 28, 2018

dmulcahey commented May 28, 2018

Adminiuga commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adminiuga commented May 29, 2018 via email

damarco commented Jun 14, 2018

balloob commented Jun 20, 2018

dmulcahey commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damarco commented Sep 16, 2018

balloob commented Sep 17, 2018

damarco commented May 21, 2018 •

edited

dmulcahey commented May 27, 2018 •

edited

dmulcahey May 27, 2018 •

edited

dmulcahey commented May 27, 2018 •

edited