Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
3b8a2a7
Fire events for ISY994 control events
OverloadUT Nov 15, 2017
72bbd5d
Move change event subscription to after entity is added to hass
OverloadUT Nov 26, 2017
f10822f
Overhaul binary sensors in ISY994 to be functional "out of the box"
OverloadUT Nov 23, 2017
6cbe3b7
Parse the binary sensor device class from the ISY's device "type"
OverloadUT Nov 25, 2017
7f9189c
Code review tweaks
OverloadUT Nov 26, 2017
489fbbc
Handle cases where a sensor's state is unknown
OverloadUT Nov 26, 2017
4de13c4
Clean up from code review
OverloadUT Nov 27, 2017
efb000a
Unknown value from PyISY is now -inf rather than -1
OverloadUT Nov 30, 2017
76b9299
Move heartbeat handling to a separate sensor
OverloadUT Dec 5, 2017
624ee06
Add support for Unknown state, which is being added in next PyISY
OverloadUT Dec 6, 2017
24b7af4
Merge branch 'dev' into isy994-control-events
OverloadUT Dec 6, 2017
9cfeaa3
Merge branch 'isy994-control-events' into isy994_sensor_improvements
OverloadUT Dec 6, 2017
b9f8422
Change a couple try blocks to explicit None checks
OverloadUT Dec 7, 2017
e5da5bc
Bump PyISY to 1.1.0, now that it has been published!
OverloadUT Dec 8, 2017
1c59cc5
Merge branch 'isy994-control-events' into isy994_sensor_improvements
OverloadUT Dec 8, 2017
ccf3a9e
Remove -inf checking from base component
OverloadUT Dec 8, 2017
92d8dd4
Restrict negative-node and heartbeat support to known compatible types
OverloadUT Dec 12, 2017
e12ee0f
Use new style string formatting
OverloadUT Dec 12, 2017
fef3515
Add binary sensor detection for pre-5.x firmware
OverloadUT Dec 12, 2017
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
208 changes: 195 additions & 13 deletions homeassistant/components/binary_sensor/isy994.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,28 @@
For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/binary_sensor.isy994/
"""

import asyncio
import logging
from datetime import datetime
from typing import Callable # noqa

from homeassistant.components.binary_sensor import BinarySensorDevice, DOMAIN
import homeassistant.components.isy994 as isy
from homeassistant.const import STATE_ON, STATE_OFF
from homeassistant.const import STATE_ON, STATE_OFF, STATE_UNKNOWN
from homeassistant.helpers.typing import ConfigType

_LOGGER = logging.getLogger(__name__)

VALUE_TO_STATE = {
False: STATE_OFF,
True: STATE_ON,
}

UOM = ['2', '78']
STATES = [STATE_OFF, STATE_ON, 'true', 'false']

ISY_DEVICE_TYPES = {
'moisture': ['16.8', '16.13', '16.14'],
'opening': ['16.9', '16.6', '16.7', '16.2', '16.17', '16.20', '16.21'],
'motion': ['16.1', '16.4', '16.5', '16.3']
}


# pylint: disable=unused-argument
def setup_platform(hass, config: ConfigType,
Expand All @@ -32,10 +36,27 @@ def setup_platform(hass, config: ConfigType,
return False

devices = []
devices_by_nid = {}
child_nodes = []

for node in isy.filter_nodes(isy.SENSOR_NODES, units=UOM,
states=STATES):
devices.append(ISYBinarySensorDevice(node))
if node.parent_node is None:
device = ISYBinarySensorDevice(node)
devices.append(device)
devices_by_nid[node.nid] = device
else:
# We'll process the child nodes last, to ensure all parent nodes
# have been processed
child_nodes.append(node)

for node in child_nodes:
try:
devices_by_nid[node.parent_node.nid].add_child_node(node)
except KeyError:
_LOGGER.error("Node %s has a parent node %s, but no device "
"was created for the parent. Skipping.",
node.nid, node.parent_nid)

for program in isy.PROGRAMS.get(DOMAIN, []):
try:
Expand All @@ -49,22 +70,183 @@ def setup_platform(hass, config: ConfigType,


class ISYBinarySensorDevice(isy.ISYDevice, BinarySensorDevice):
"""Representation of an ISY994 binary sensor device."""
"""Representation of an ISY994 binary sensor device.

Often times, a single device is represented by multiple nodes in the ISY,
allowing for different nuances in how those devices report their on and
off events. This class turns those multiple nodes in to a single Hass
entity and handles both ways that ISY binary sensors can work.
"""

def __init__(self, node) -> None:
"""Initialize the ISY994 binary sensor device."""
isy.ISYDevice.__init__(self, node)
super().__init__(node)
self._negative_node = None
self._heartbeat_node = None
self._heartbeat_timestamp = None
self._device_class_from_type = self._detect_device_type()
# pylint: disable=protected-access
if self._node.status._val == -1:
self._computed_state = None
else:
self._computed_state = bool(self._node.status._val)

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes!


self._node.controlEvents.subscribe(self._positive_node_control_handler)

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

# Heartbeat node doesn't exist
pass

try:
self._heartbeat_node.controlEvents.subscribe(
self._heartbeat_node_control_handler)
except AttributeError:
# Heartbeat node doesn't exist
pass

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When can this AttributeError happen?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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!

return None

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

return None

def add_child_node(self, child):
"""Add a child node to this binary sensor device.

The child node can either be a node that receives the 'off' events, or
a heartbeat node for reporting that this device is still alive
"""
subnode_id = int(child.nid[-1])
if subnode_id == 2:
# "Negative" node that can be used to represent a negative state
# when it reports "On"
self._negative_node = child
elif subnode_id == 4:
# Heartbeat node that just reports "On" every 24 hours
self._heartbeat_timestamp = STATE_UNKNOWN

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

self._heartbeat_node = child

def _negative_node_control_handler(self, event: object) -> None:
"""Handle an "On" control event from the "negative" node."""
if event == 'DON':
_LOGGER.debug("Sensor %s turning Off via the Negative node "
"sending a DON command", self.name)
self._computed_state = False
self.schedule_update_ha_state()

def _positive_node_control_handler(self, event: object) -> None:
"""Handle On and Off control event coming from the primary node.

Depending on device configuration, sometimes only On events
will come to this node, with the negative node representing Off
events
"""
if event == 'DON':
_LOGGER.debug("Sensor %s turning On via the Primary node "
"sending a DON command", self.name)
self._computed_state = True
self.schedule_update_ha_state()
if event == 'DOF':
_LOGGER.debug("Sensor %s turning Off via the Primary node "
"sending a DOF command", self.name)
self._computed_state = False
self.schedule_update_ha_state()

def _heartbeat_node_control_handler(self, event: object) -> None:
"""Update the heartbeat timestamp when an On event is sent."""
if event == 'DON':
self._heartbeat_timestamp = datetime.now().isoformat()
self.schedule_update_ha_state()

# pylint: disable=unused-argument
def on_update(self, event: object) -> None:
"""Ignore primary node status updates.

We listen directly to the Control events on all nodes for this
device.
"""
pass

@property
def value(self) -> object:
"""Get the current value of the device.

Insteon leak sensors set their primary node to On when the state is
DRY, not WET, so we invert the binary state if the user indicates
that it is a moisture sensor.
"""
if self._computed_state is None:
# Do this first so we don't invert None on moisture sensors
return None

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When could this happen? It looks like it will either return None or the cls variable that is a key when iterating ISY_DEVICE_TYPES.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you check the ISY_DEVICE_TYPES const up top, moisture is one of the keys

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What object attribute access could lead to attribute error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed - artifact of a refactor

pass

return self._computed_state

@property
def is_on(self) -> bool:
"""Get whether the ISY994 binary sensor device is on."""
"""Get whether the ISY994 binary sensor device is on.

Note: This method will return false if the current state is UNKNOWN
"""
return bool(self.value)

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

return STATE_ON if self.is_on else STATE_OFF

@property
def device_class(self) -> str:
"""Return the class of this device.

class ISYBinarySensorProgram(ISYBinarySensorDevice):
"""Representation of an ISY994 binary sensor program."""
This was discovered by parsing the device type code during init
"""
return self._device_class_from_type

@property
def device_state_attributes(self):
"""Get the state attributes for the device."""
attr = super().device_state_attributes
if self._heartbeat_timestamp is not None:
attr['last_heartbeat'] = self._heartbeat_timestamp

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Timestamps in state attributes is frowned upon, and usually not allowed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't see the alternative here. Heartbeats are daily pings and that's it. The idea here is that you could write some automatons that sent an alert if the heartbeat timestamp becomes more than 24 hours old.

I'm certainly open to suggestions but this seemed like the most straightforward way to present it to the user. I thought about making it some sort of entity that handled the 24 hour countdown internally, but that would depend on all Insteon devices always being on a 24 hour heartbeat, which is an assumption I don't think we should make...

return attr


class ISYBinarySensorProgram(isy.ISYDevice, BinarySensorDevice):
"""Representation of an ISY994 binary sensor program.

This does not need all of the subnode logic in the device version of binary
sensors.
"""

def __init__(self, name, node) -> None:
"""Initialize the ISY994 binary sensor program."""
ISYBinarySensorDevice.__init__(self, node)
super().__init__(node)
self._name = name

@property
def is_on(self) -> bool:
"""Get whether the ISY994 binary sensor device is on."""
return bool(self.value)
29 changes: 28 additions & 1 deletion homeassistant/components/isy994.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
For configuration details please visit the documentation for this component at
https://home-assistant.io/components/isy994/
"""
import asyncio
from collections import namedtuple
import logging
from urllib.parse import urlparse
Expand Down Expand Up @@ -91,6 +92,16 @@ def filter_nodes(nodes: list, units: list=None, states: list=None) -> list:
return filtered_nodes


def _is_node_a_sensor(node, path: str, sensor_identifier: str) -> bool:
if sensor_identifier in path or sensor_identifier in node.name:
return True

try:
return node.node_def_id == 'BinaryAlarm'
except AttributeError:
return False


def _categorize_nodes(hidden_identifier: str, sensor_identifier: str) -> None:
"""Categorize the ISY994 nodes."""
global SENSOR_NODES
Expand All @@ -106,7 +117,7 @@ def _categorize_nodes(hidden_identifier: str, sensor_identifier: str) -> None:
hidden = hidden_identifier in path or hidden_identifier in node.name
if hidden:
node.name += hidden_identifier
if sensor_identifier in path or sensor_identifier in node.name:
if _is_node_a_sensor(node, path, sensor_identifier):
SENSOR_NODES.append(node)
elif isinstance(node, PYISY.Nodes.Node):
NODES.append(node)
Expand Down Expand Up @@ -227,15 +238,31 @@ class ISYDevice(Entity):
def __init__(self, node) -> None:
"""Initialize the insteon device."""
self._node = node
self._change_handler = None
self._control_handler = None

@asyncio.coroutine
def async_added_to_hass(self) -> None:
"""Subscribe to the node change events."""
self._change_handler = self._node.status.subscribe(

@MartinHjelmare MartinHjelmare Nov 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

'changed', self.on_update)

if hasattr(self._node, 'controlEvents'):
self._control_handler = self._node.controlEvents.subscribe(

@MartinHjelmare MartinHjelmare Nov 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

self.on_control)

# pylint: disable=unused-argument
def on_update(self, event: object) -> None:
"""Handle the update event from the ISY994 Node."""
self.schedule_update_ha_state()

def on_control(self, event: object) -> None:
"""Handle a control event from the ISY994 Node."""
self.hass.bus.fire('isy994_control', {
'entity_id': self.entity_id,
'control': event
})

@property
def domain(self) -> str:
"""Get the domain of the device."""
Expand Down