ISY994 sensor improvements by OverloadUT · Pull Request #10805 · home-assistant/core

OverloadUT · 2017-11-26T00:11:11Z

Description:

Binary Sensor Changes

Today, using Insteon sensors via the ISY994 platform is a pretty big pain. Issues:

The user has to name the sensors very specifically inside ISY994 based on the Hass config option.
The user has to specify the device_class manually using customize:
If the hardware sensor is configured to use unique nodes for on and off (which allows more powerful scene configuration in the Insteon ecosystem) then the sensor entities will not function whatsoever inside Hass, requiring the user to set up a "virtual entity" using the Programs feature in the ISY994 platform. While functional, these virtual entities update slower than native devices and require a lot of user configuration inside the ISY994 software.
More recent Insteon hardware report a daily heartbeat for the purpose of determining when a battery is dead, and there is no way to see this inside Hass due to it being an entity that never changes status.

This overhaul eliminates all of those pain points, and make all Insteon ISY994 sensors work out of the box with the following improvements:

The multiple ISY994 "nodes" used by a sensor no longer each get unique entities in Hass, but instead all attach to a single binary_sensor.
The binary_sensor logic is now smart enough to handle BOTH ways that hardware can report changes: Either by the primary node changing status, OR by the primary and negative nodes reporting ON events.
All Insteon ISY994 sensor classes are now automatically determined by looking at the ISY994 "type" attribute, which indicates the exact hardware in use.
The timestamp of the last heartbeat is now stored as a sensor attribute, allowing for automations to trigger on a missed heartbeat. On boot-up, the attribute will be set to STATE_UNKNOWN if and only if a heartbeat node is actually present for the device. This means that older sensors without the heartbeat feature will not get erroneously identified as missing heartbeats (if the automation is written correctly - I will provide an example in the updated docs)!

Event Support

This PR also adds ISY994 Control event support: all Insteon "control" events will now emit as Hass events. This allows automations based on button presses, double-presses, long-holds, and others!

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4207

Breaking Change Info

ISY994 sensor devices previously showed up as switches by default, and now they will show up as binary_sensors. Any automations set up to trigger based on those old switch versions should be updated to the binary_sensor versions. Note: For now, this only applies to people using the 5.x beta versions of the ISY firmware.

Checklist:

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
New dependencies are only imported inside functions that use them ([example][ex-import]).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

This allows hass to react directly to Insteon button presses (on switches and remotes), including presses, double-presses, and long holds

MartinHjelmare · 2017-11-26T00:20:10Z

super().__init__(node)

MartinHjelmare · 2017-11-26T00:27:49Z

Remove this, since you return None anyway.

Whoops; artifacts from refactoring this in to its own method.

MartinHjelmare · 2017-11-26T00:31:05Z

Remove this.

MartinHjelmare · 2017-11-26T00:31:11Z

Remove this.

MartinHjelmare · 2017-11-26T00:36:59Z

I don't think you should use STATE_UNKNOWN for this. None is probably best.

It's important for there to be a difference between the attribute not existing (None) and the attribute currently being "Unknown" because we haven't seen a heartbeat since the last boot. If it was simply None until the first heartbeat, an automation can't tell if this device ever SHOULD give a heartbeat.

MartinHjelmare · 2017-11-26T00:47:29Z

attr = super().device_state_attributes

MartinHjelmare · 2017-11-26T00:48:33Z

super().__init__(node)

MartinHjelmare · 2017-11-26T00:56:39Z

You should probably move this subscription to the coroutine async_added_to_hass since the callback on_update requires hass to be set on the entity. This will not be happen until the entity is added to home assistant.

Done! Good catch to eliminate a race condition. This issue was present before as well!

MartinHjelmare · 2017-11-26T00:57:07Z

You should probably move this subscription to the coroutine async_added_to_hass since the callback on_control requires the entity_id. This will not be created until the entity is added to home assistant.

You know what, it didn't occur to me that this PR is actually branched off of PR #10664, which is where this code comes from. That being said, I'll make the change to the base PR and rebase this one to that.

(This is now done even though GitHub is not recognizing this comment as outdated)

MartinHjelmare · 2017-11-26T01:00:15Z

Where is this used?

Hm, I had intended to store the child nodes in this array, but then I suppose I never actually ended up needing to. It feels weird not storing them, but since it would not be used in the current implementation I'll just remove this!

Edit: Oh right, I do store the subnodes - they are just stored as explicit properties since each subnode needs to be handled uniquely

The event handler method requires `self.hass` to exist, which doesn't have a value until the async_added_to_hass method is called. Should eliminate a race condition.

We now smash all of the subnodes from the ISY994 in to one Hass binary_sensor, and automatically support both paradigms of state reporting that Insteon sensors can do. Sometimes a single node's state represents the sensor's state, other times two nodes are used and only "ON" events are sent from each. The logic between the two forunately do not conflict so we can support both without knowing which mode the device is in. This also allows us to handle the heartbeat functionality that certain sensors have - we simply store the timestamp of the heartbeat as an attribute on the sensor device. It defaults to Unknown on bootup if and only if the device supports heartbeats, due to the presence of subnode 4.

Now we automatically know which sensors are moisture, motion, and openings! (We also reverse the moisture sensor state, because Insteon reports ON for dry on the primary node.)

The one material change here is that the event subscribers were moved to the `async_added_to_hass` method, as the handlers depend on things that only exist after the entity has been added.

When the ISY first boots up, if a battery-powered sensor has not reported in yet (due to heartbeat or a change in state), the state is unknown until it does.

MartinHjelmare

I think you should ask @balloob about the timestamp in the state attributes, and possible alternatives.

MartinHjelmare · 2017-11-26T11:06:19Z

+    @asyncio.coroutine
+    def async_added_to_hass(self) -> None:
+        """Subscribe to the node and subnode event emitters."""
+        super().async_added_to_hass()


This is a coroutine, so you should yield from it to schedule it.

Thanks for the help here - I am a little unclear on when to yield in a coroutine. Are you saying that I should yield from the super() call, but the other calls in this method are fine?

MartinHjelmare · 2017-11-26T11:09:38Z

+    def state(self):
+        """Return the state of the binary sensor."""
+        if self._computed_state is None:
+            return STATE_UNKNOWN


Return None. The base entity class will handle unknown states.

Hm, I see that the async_update_ha_state will indeed do this, but I feel like if we know the state is unknown, this method return STATE_UNKNOWN to be explicit. That's what the Entity version of this method does, so the established paradigm is to fall back on STATE_UNKNOWN.

No, we should let the entity base class handle this.

MartinHjelmare · 2017-11-26T11:15:01Z

+        try:
+            if self.device_class == 'moisture':
+                return not self._computed_state
+        except AttributeError:


What object attribute access could lead to attribute error?

Fixed - artifact of a refactor

Fix coroutine await, remove unnecessary exception check, and return None when state is unknown

OverloadUT · 2017-11-28T06:45:28Z

To summarize the question that needs @balloob's input:

Some battery-powered Insteon devices send a heartbeat signal every 24 hours as a mechanism to tell if a battery has died. Currently I have this implemented as:

A heartbeat attribute that gets updated to the current time (datetime.now().isoformat()) whenever the heartbeat signal is received.
If we know the device is heartbeat-capable but haven't seen one yet (on a Hass reboot) then the attribute gets set to STATE_UNKNOWN (allowing an automation to trigger if it stays UNKNOWN for >24 hours, for example)
If we know the device is not heartbeat-capable, the attribute does not exist in the first place

@MartinHjelmare pointed out that timestamps in attributes is frowned-upon, but I'm not immediately seeing another approach here that keeps the edge case handling. What do you think? Is there any heartbeat precedent in other components?

Edit: The answer is that we're going to split the heartbeat functionality in to a separate binary_sensor that represents a healthy battery. If we miss a heartbeat, it'll flip to false. I'll work on that change!

pvizeli · 2017-11-28T09:42:44Z

I set it to WIP. You can remove it after you implement that binary and remove the timestamp from attributes 👍

Now all heartbeat-compatible sensors will have a separate `binary_sensor` device that represents the battery state (on = dead)

MartinHjelmare

Overall looks good. Some minor questions/comments below.

MartinHjelmare · 2017-12-05T10:51:58Z

+    def _detect_device_type(self) -> str:
+        try:
+            device_type = self._node.type
+        except AttributeError:


When can this AttributeError happen?

This is to protect against the self._node.type not being set. It comes from a third party API that itself can be populated by fourth party code, so just being defensive here!

MartinHjelmare · 2017-12-05T11:03:22Z

+        try:
+            self._negative_node.controlEvents.subscribe(
+                self._negative_node_control_handler)
+        except AttributeError:


Why not check if self._negative_node is not None instead?

I went with the Python pattern of "ask for forgiveness" for no other reason than it seems like the Pythonic way. Happy to change it, and I'd love to hear if you have thoughts about when except should be used vs checking for None.

If there's a simple check that is safe and always works, I go with an if. If there are multiple checks needed or if those checks don't always work, or if cases are unpredictable, I go with try... except. But it all depends... 😄
Here the case looks contained to me since we initialize the attribute to None.

Got it! Since we control that property in a closed environment, we should just check for values that we know it can be. I just pushed the change for these comments

MartinHjelmare · 2017-12-05T11:05:29Z

+        """Send a heartbeat to our heartbeat device, if we have one."""
+        try:
+            self._heartbeat_device.heartbeat()
+        except AttributeError:


Why not check if self._heartbeat_device is not None instead?

(Same answer as the line 115 comment)

PyISY will report unknown states as the number "-inf". This is implemented in the base ISY994 component, but subcomponents that override the `state` method needed some extra logic to handle it as well.

OverloadUT · 2017-12-06T22:50:11Z

I merged in some changes from PR #10664 which this is branched off of

MartinHjelmare · 2017-12-08T08:36:01Z

+        if self.is_unknown():
+            return None
+        else:
+            return super().state


When would this be used? All isy device classes except binary sensor program seem to override the state property anyway. If this call would go to the Entity class, it would return STATE_UNKNOWN, which would lead to the same outcome as the above if self.is_unknown() case.

The fan and light isy devices don't implement their own state property, so this base-class code would apply to them. I could move the logic in to those last types, but I like that having it in the base class also serves as a signal to future implementers that the UNKNOWN state needs to be handled if you override it.

This actually doesn't end up going to the Entity class! I had to test it a bunch because super()'s behavior is new to me, but super will actually call the implementation in the other base class of the child object.

To use Fan as an example, it is defined as ISYFanDevice(isy.ISYDevice, FanEntity). So state will first get handled by ISYDevice.state, and when it calls super() it kicks it on over to FanEntity.state which handles the fan-specific version of state.

OverloadUT · 2017-12-08T18:55:45Z

@MartinHjelmare when you are done reviewing this PR, I'd love if you could toss the same message on to #10664, which this PR is branched off of (so all of the code in that PR is in the code you've been reviewing here)

MartinHjelmare · 2017-12-08T21:46:38Z

If that's true what's the point of #10664?

OverloadUT · 2017-12-08T22:40:53Z

Originally, #10664 was meant to be a small PR adding an important feature that things like this PR and others would need. But it took quite a while to get the new PyISY version all done and published, so in the meantime I worked on this PR as well.

I was trying to stick to the "smaller PRs" request, but this one depended on that other functionality so there was no choice but to branch from it. In a perfect world that other one would have been merged weeks ago :)

I suppose since you've been reviewing all of the code here, I could just close that other PR and roll both new features in to this one. I'll go with whatever is preferable on that front.

MartinHjelmare · 2017-12-09T07:35:28Z

Yes, please close the other PR.

OverloadUT · 2017-12-09T21:06:34Z

Done. Let me know when we're looking good here and I'll get the documentation written up! I wanted to wait until the implementation was locked-in :)

OverloadUT · 2017-12-11T20:07:01Z

Re-adding WIP tag. I have discovered some issues with new devices I purchased for testing this change, and possibly with the latest ISY firmware 😭

Not all Insteon sensors use the same subnode IDs for the same things, so we need to use different logic depending on device type. Negative node and heartbeat support is now only used for leak sensors and open/close sensors.

OverloadUT · 2017-12-12T07:42:31Z

Okay, should be ready for a final review again. I bought a motion sensor to validate these changes, and it turns out they work totally differently. I added a simple check to only apply the new logic in this PR to leak and door sensors.

I also improved initial state support so that doors with an unknown state on bootup will report as unknown until the sensor reports for the first time.

MartinHjelmare · 2017-12-12T08:39:40Z

+
+    split_type = device_type.split('.')
+    for device_class, ids in ISY_DEVICE_TYPES.items():
+        if split_type[0] + '.' + split_type[1] in ids:


Use new style string formatting.

'{}.{}'.format(split_type[0], split_type[1])

MartinHjelmare

Looks good!

OverloadUT · 2017-12-12T19:49:53Z

Excellent; I'll write up the documentation now!

MartinHjelmare · 2017-12-12T19:50:03Z

Add a link to a docs PR and I think we can merge this.

Meant to do this originally; writing documentation revealed that this requirement was missed!

OverloadUT · 2017-12-12T20:11:21Z

Sorry for the one additional commit! In writing the documentation I realized that I never added the small bit of logic necessary for pre-5.x firmware to work, which was always my intention.

OverloadUT · 2017-12-12T22:53:25Z

Description updated with a link to the documentation PR, and also added event support description here, which became part of this PR.

…into dev * 'dev' of https://github.com/home-assistant/home-assistant: Disable html5 notify dependency (home-assistant#11135) ISY994 sensor improvements (home-assistant#10805) Allow using more than one keyboard remote (home-assistant#11061) set default utc offset to 0 (home-assistant#11114) Add problem device class (home-assistant#11130) Always consume the no_throttle keyword argument. (home-assistant#11126) Skip HASS emulated Hue bridges from detection. (home-assistant#11128) update pyripple (home-assistant#11122) Add media position properties (home-assistant#10076) Fixed typo in automation.py (home-assistant#11116)

Fire events for ISY994 control events

3b8a2a7

This allows hass to react directly to Insteon button presses (on switches and remotes), including presses, double-presses, and long holds

OverloadUT added the breaking-change label Nov 26, 2017

OverloadUT requested a review from rmkraus November 26, 2017 00:11

homeassistant added integration: isy994 platform: binary_sensor.isy994 cla-signed labels Nov 26, 2017

balloobbot added component platform labels Nov 26, 2017

MartinHjelmare reviewed Nov 26, 2017

View reviewed changes

OverloadUT changed the title ~~Isy994 sensor improvements~~ ISY994 sensor improvements Nov 26, 2017

OverloadUT added 4 commits November 25, 2017 21:51

Move change event subscription to after entity is added to hass

72bbd5d

The event handler method requires `self.hass` to exist, which doesn't have a value until the async_added_to_hass method is called. Should eliminate a race condition.

Parse the binary sensor device class from the ISY's device "type"

6cbe3b7

Now we automatically know which sensors are moisture, motion, and openings! (We also reverse the moisture sensor state, because Insteon reports ON for dry on the primary node.)

Code review tweaks

7f9189c

The one material change here is that the event subscribers were moved to the `async_added_to_hass` method, as the handlers depend on things that only exist after the entity has been added.

OverloadUT force-pushed the isy994_sensor_improvements branch from 731b7ff to 7f9189c Compare November 26, 2017 05:53

homeassistant added the cla-signed label Nov 26, 2017

Handle cases where a sensor's state is unknown

489fbbc

When the ISY first boots up, if a battery-powered sensor has not reported in yet (due to heartbeat or a change in state), the state is unknown until it does.

MartinHjelmare reviewed Nov 26, 2017

View reviewed changes

Clean up from code review

4de13c4

Fix coroutine await, remove unnecessary exception check, and return None when state is unknown

pvizeli changed the title ~~ISY994 sensor improvements~~ WIP: ISY994 sensor improvements Nov 28, 2017

OverloadUT added 2 commits November 30, 2017 13:23

Unknown value from PyISY is now -inf rather than -1

efb000a

Move heartbeat handling to a separate sensor

76b9299

Now all heartbeat-compatible sensors will have a separate `binary_sensor` device that represents the battery state (on = dead)

homeassistant added the cla-signed label Dec 5, 2017

MartinHjelmare reviewed Dec 5, 2017

View reviewed changes

OverloadUT added 3 commits December 6, 2017 14:22

Add support for Unknown state, which is being added in next PyISY

624ee06

PyISY will report unknown states as the number "-inf". This is implemented in the base ISY994 component, but subcomponents that override the `state` method needed some extra logic to handle it as well.

Merge branch 'dev' into isy994-control-events

24b7af4

Merge branch 'isy994-control-events' into isy994_sensor_improvements

9cfeaa3

MartinHjelmare reviewed Dec 8, 2017

View reviewed changes

OverloadUT changed the title ~~WIP: ISY994 sensor improvements~~ ISY994 sensor improvements Dec 8, 2017

OverloadUT mentioned this pull request Dec 9, 2017

Fire events for ISY994 control events #10664

Closed

4 tasks

OverloadUT changed the title ~~ISY994 sensor improvements~~ WIP ISY994 sensor improvements Dec 11, 2017

homeassistant added the cla-signed label Dec 12, 2017

OverloadUT changed the title ~~WIP ISY994 sensor improvements~~ ISY994 sensor improvements Dec 12, 2017

MartinHjelmare reviewed Dec 12, 2017

View reviewed changes

Use new style string formatting

e12ee0f

homeassistant added the cla-signed label Dec 12, 2017

MartinHjelmare approved these changes Dec 12, 2017

View reviewed changes

Add binary sensor detection for pre-5.x firmware

fef3515

Meant to do this originally; writing documentation revealed that this requirement was missed!

OverloadUT mentioned this pull request Dec 12, 2017

Documentation for ISY994 changes to sensors and events home-assistant/home-assistant.io#4207

Merged

2 tasks

balloob merged commit 1c8b583 into home-assistant:dev Dec 14, 2017

fabaff mentioned this pull request Dec 16, 2017

0.60 #11153

Merged

OverloadUT deleted the isy994_sensor_improvements branch December 22, 2017 19:57

home-assistant locked and limited conversation to collaborators Mar 30, 2018

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

Uh oh!

Conversation

OverloadUT commented Nov 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Binary Sensor Changes

Event Support

Breaking Change Info

Checklist:

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!

MartinHjelmare Nov 26, 2017 • 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!

MartinHjelmare Nov 26, 2017 • 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!

OverloadUT Nov 26, 2017 • 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!

MartinHjelmare left a comment

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!

OverloadUT commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pvizeli commented Nov 28, 2017

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

OverloadUT commented Nov 26, 2017 •

edited

Loading

MartinHjelmare Nov 26, 2017 •

edited

Loading

MartinHjelmare Nov 26, 2017 •

edited

Loading

OverloadUT Nov 26, 2017 •

edited

Loading

OverloadUT commented Nov 28, 2017 •

edited

Loading

OverloadUT Dec 6, 2017 •

edited

Loading