Huge ISY994 platform cleanup, fixes support for 5.0.10 firmware #11243

OverloadUT · 2017-12-20T02:37:49Z

Description:

Breaking Change Notes

The hidden_string feature has been removed from the isy994 component. Previously, this allowed entities to be "hidden" in Home Assistant if a configured string was present in an ISY device's name or folder path. This was removed because hiding devices is now done via the customization feature.
- Note however, that this feature was replaced by a new ignore_string config option, which will now cause Home Assistant to completely ignore devices with the matching string so that they will not be imported as a Home Assistant device at all. This can be helpful if you have nodes in the ISY that aren't useful at all in Hass (IR transmitter nodes are a good example.)

Initial Problem Statement

The 5.0.10 update to the ISY994 firmware completely broke Home Assistant's integration. They are deprecating the uom values as useful attributes, and developers are encouraged to use the NodeDefs instead.

Trying to add this support in Hass revealed how much the ISY994 component and platforms were in need of a refactor - many things about how it works would not pass a code review today. During the refactor I found many other issues that I fixed. So this ended up being a pretty large cleanup of all of the isy994 files.

List Of Changes

All of the globals have been removed, and now the data that must be shared across platforms is stored in hass.data
The parent ISY994 component now handles categorizing all of the nodes in to the various domains. Before, this logic was handled partially by the parent component and partially by each individual domain - this required keeping all of the decentralized filtering logic in perfect sync, or risk having a single node turn in to multiple Hass entities. This is what the majority of the new code in components/isy994.py does. I organized it in a way that is hopefully easy to adjust in the future and commented as much as I could about what is going on. It is complex no matter how you shake it though, because we're supporting four entirely different ways of identifying device types, each that work with different families of devices and ISY firmware versions.
I completely removed the hidden_string feature. Accomplishing this behavior can be done via customize, so it didn't make sense to me to have the logic in two different places. I REPLACED the feature with ignore_string, which will now completely ignore a node that contains the string, preventing Hass entities from getting created in the first place. This is very useful to prevent a bunch of pointless nodes from getting Hass entities that just end up hidden.
I found several unused methods and properties in various classes that I removed.
The parsing of the hostname for component setup was doing a bunch of precarious string replaces. I fixed this to use the more convenient properties that were available from urlparse.
Removed broken logic in the fan Program component. It was setting properties that have no setters, which couldn't possibly have worked all this time.
Added the missing SUPPORTED_FEATURES to the fan component to indicate that it can set speed
Added better error handling and a log warning when an ISY994 program entity fails to initialize
Cleaned up a few instances of unnecessarily complicated logic paths, and other cases of unnecessary logic that is already handled by base classes
Added a config option to disable the import of climate nodes as Hass sensors. This allows the user to keep the climate module installed in the ISY firmware, but not have them appear in Hass.
Weather sensors now have spaces instead of underscores, making them much nicer to read out of the box. Not a breaking change, because the entity_id stays the same!

!! This change needs to be tested by other people with other versions of the ISY994 firmware and sets of devices. This is a very big change to the fundamental way that devices are set up! !!

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

Example entry for `configuration.yaml` (if applicable):

isy994:
  host: http://192.168.0.6:80
  username: username
  password: pass
  sensor_string: sensor
  ignore_string: {IGNORE ME}

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

# * No more globals - store on hass.data # * Parent ISY994 component handles categorizing nodes in to Hass components, rather than each individual domain filtering all nodes themselves # * Remove hidden string, replace with ignore string. Hidden should be done via the customize block; ignore fully prevents the node from getting a Hass entity # * Removed a few unused methods in the ISYDevice class # * Cleaned up the hostname parsing # * Removed broken logic in the fan Program component. It was setting properties that have no setters # * Added the missing SUPPORTED_FEATURES to the fan component to indicate that it can set speed # * Added better error handling and a log warning when an ISY994 program entity fails to initialize # * Cleaned up a few instances of unecessarily complicated logic paths, and other cases of unnecessary logic that is already handled by base classes

MartinHjelmare

Very nice clean up!

MartinHjelmare · 2017-12-20T06:11:10Z

homeassistant/components/cover/isy994.py

    """Representation of an ISY994 cover device."""

    def __init__(self, node: object):
        """Initialize the ISY994 cover device."""
-        isy.ISYDevice.__init__(self, node)
+        ISYDevice.__init__(self, node)


super().__init__(node)

MartinHjelmare · 2017-12-20T06:12:31Z

homeassistant/components/fan/isy994.py

    """Representation of an ISY994 fan device."""

    def __init__(self, node) -> None:
        """Initialize the ISY994 fan device."""
-        isy.ISYDevice.__init__(self, node)
+        ISYDevice.__init__(self, node)


Same as above.

MartinHjelmare · 2017-12-20T06:17:44Z

homeassistant/components/fan/isy994.py

@@ -106,23 +102,18 @@ def __init__(self, name: str, node, actions) -> None:
        ISYFanDevice.__init__(self, node)


Same as above.

MartinHjelmare · 2017-12-20T06:25:40Z

homeassistant/components/isy994.py


-SUPPORTED_DOMAINS = ['binary_sensor', 'cover', 'fan', 'light', 'lock',
-                     'sensor', 'switch']
+def _check_for_node_def(node, hass: HomeAssistant,


If hass is an argument, it is usually the first argument.

MartinHjelmare · 2017-12-20T06:28:03Z

homeassistant/components/isy994.py

        try:
-            folder = ISY.programs[KEY_MY_PROGRAMS]['HA.' + component]
+            folder = programs[KEY_MY_PROGRAMS]['HA.' + domain]


Use new style string formatting.

MartinHjelmare · 2017-12-20T06:29:33Z

homeassistant/components/isy994.py

-                                 getattr(ISY.climate, attr + '_units'))
+    climate_attrs = dir(climate)
+    weather_nodes = [WeatherNode(getattr(climate, attr), attr,
+                                 getattr(climate, attr + '_units'))
                     for attr in climate_attrs
                     if attr + '_units' in climate_attrs]


Same as above.

MartinHjelmare · 2017-12-20T06:33:18Z

homeassistant/components/light/isy994.py

    """Representation of an ISY994 light devie."""

    def __init__(self, node: object) -> None:
        """Initialize the ISY994 light device."""
-        isy.ISYDevice.__init__(self, node)
+        ISYDevice.__init__(self, node)


Please update per above.

MartinHjelmare · 2017-12-20T06:34:07Z

homeassistant/components/lock/isy994.py

    """Representation of an ISY994 lock device."""

    def __init__(self, node) -> None:
        """Initialize the ISY994 lock device."""
-        isy.ISYDevice.__init__(self, node)
+        ISYDevice.__init__(self, node)


Same as above.

MartinHjelmare · 2017-12-20T06:35:09Z

homeassistant/components/sensor/isy994.py

@@ -319,8 +313,6 @@ def unit_of_measurement(self) -> str:
 class ISYWeatherDevice(isy.ISYDevice):
    """Representation of an ISY994 weather device."""

-    _domain = 'sensor'
-
    def __init__(self, node) -> None:
        """Initialize the ISY994 weather device."""
        isy.ISYDevice.__init__(self, node)


Same as above.

MartinHjelmare · 2017-12-20T06:35:55Z

homeassistant/components/switch/isy994.py

    """Representation of an ISY994 switch device."""

    def __init__(self, node) -> None:
        """Initialize the ISY994 switch device."""
-        isy.ISYDevice.__init__(self, node)
+        ISYDevice.__init__(self, node)


Same as above.

OverloadUT · 2017-12-20T07:39:39Z

Good additional catches. All implemented!

MartinHjelmare · 2017-12-20T08:46:29Z

homeassistant/components/binary_sensor/isy994.py

-        except (KeyError, AssertionError):
-            pass
+            status = program[KEY_STATUS]
+        except (AttributeError, KeyError, AssertionError):


There doesn't seem to be an assertion check within this try...except.

Would it be possible to move all these try...except blocks for program out of the platforms and into the _categorize_programs function in the component? The aim would be that all programs in ISY994_PROGRAMS should be programs that should be added as entities. That would save a lot of near duplicated code. I see the block here in the binary sensor doesn't access the KEY_ACTIONS key on program. But that should be solvable in _categorize_programs by checking the domain.

Yeah, I agree that's a much better way for this to work. I'll move it over.

Removes the need for a bunch of duplicate exception handling in each individual platform

Sensor platform was crashing when the ISY reported climate nodes. Logic has been fixed. Also added a config option to prevent climate sensors from getting imported from the ISY. Also replace the underscore from climate node names with spaces so they default to friendly names.

OverloadUT · 2017-12-21T01:11:58Z

Alright, just pushed an update that moves all of the program building and validation to the component. Each platform is now so fresh and clean.

I also pushed some more cleanup to the weather modules. Universal Devices was kind enough to activate some paid modules for me so that I could better test some of the stuff here, and weather was indeed broken after my refactor. I also added a config option to prevent importing climate sensors, as I'm not sure why a user would want to rely on the ISY for climate data when Hass has so many of its own ways to get weather. This allows a user to keep the climate modules turned on inside the ISY but not have to hide them all if they don't want them. I also made the weather nodes have spaces instead of underscores. It's awesome that this is not a breaking change because the entity_id ends up the same!

MartinHjelmare · 2017-12-21T05:10:13Z

homeassistant/components/isy994.py

-    HIDDEN_STRING = hidden_identifier
+    ignore_identifier = isy_config.get(CONF_IGNORE_STRING)
+    sensor_identifier = isy_config.get(CONF_SENSOR_STRING)
+    enable_climate = isy_config.get(CONF_ENABLE_CLIMATE)

    if host.scheme == 'http':


These checks should be moved to a validator in the config schema.

You could parse and insert the port in the config in the validator too.

Maybe you can use vol.Url() as the first validator?

Ah, I haven't actually looked in to the config schema framework. If it can handle the URL stuff that would be much better. I'll do some digging.

@MartinHjelmare Do you have an example of what you're looking for? I looked in to this, and the config option is already being passes through cv.url which in turn uses vol.Url(). This code here isn't really doing validation, but just munging the URL in to the format that the PyISY library expects (all of this could/should be done inside PyISY if it just accepted a URL rather than a host, port, and https boolean, but it doesn't.)

I could remove the final else here, because cv.url already verified that we're either http or https, but I'm not sure anything more than that makes sense.

def validate_host(config): host = urlparse(config[CONF_HOST]) if host.scheme == 'http': https = False port = host.port or 80 elif host.scheme == 'https': https = True port = host.port or 443 config[CONF_HOST] = host config[CONF_PORT] = port config[CONF_HTTPS] = https return config

Ah, so this would be just moving it to a method that gets called from within setup, and injects in to the config Dict? It doesn't seem like this is something that happens inside the schema definition itself, right?

My concern with this is that it injects new config values in to the dict that didn't go through Voluptuous's Schema checks. A casual glance at the code (due to the new CONF consts) would also make it look like PORT and HTTPS can be provided as config options, even though they can't.

The validator would be used in the config schema.

CONFIG_SCHEMA = vol.Schema( {DOMAIN: vol.Schema(vol.All({...}, validate_host))}, extra=vol.ALLOW_EXTRA)

MartinHjelmare · 2017-12-21T05:12:50Z

homeassistant/components/isy994.py

        try:
-            folder = ISY.programs[KEY_MY_PROGRAMS]['HA.' + component]
+            folder = programs[KEY_MY_PROGRAMS]['HA.{}'.format(domain)]
        except KeyError:
            pass
        else:
            for dtype, _, node_id in folder.children:
                if dtype is KEY_FOLDER:


String comparison should be done with ==.

pvizeli

--

I'm wrong

pvizeli · 2017-12-24T16:55:28Z

homeassistant/components/isy994.py

+    not have a type.
+    """
+    try:
+        device_type = node.type


instead to make a exception and hard interrupt the flow of your program, just check if type will be a attribute.

pvizeli · 2017-12-24T16:55:56Z

homeassistant/components/isy994.py

    try:
-        if node.node_def_id == 'BinaryAlarm':
-            return True
+        node_uom = set(node.uom)


pvizeli · 2017-12-24T16:56:10Z

homeassistant/components/isy994.py

    try:
-        device_type = node.type
+        node_uom = set(map(str.lower, node.uom))


pvizeli · 2017-12-24T16:57:07Z

homeassistant/components/isy994.py



 def setup(hass: HomeAssistant, config: ConfigType) -> bool:
    """Set up the ISY 994 platform."""
+    if ISY994_NODES not in hass.data:


always true

Doesn't setup get called multiple times if multiple isy994 blocks are specified in the config? I took this paradigm from here and I assumed that's why it was there:

https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/dyson.py#L47-L48

If I've got that wrong I can remove the check

The bootstrap setup a component only one time but the platform it could be call multiple times.

pvizeli · 2017-12-24T16:57:21Z

homeassistant/components/isy994.py

+    if ISY994_NODES not in hass.data:
+        hass.data[ISY994_NODES] = {}
+    for domain in SUPPORTED_DOMAINS:
+        if domain not in hass.data[ISY994_NODES]:


always true

pvizeli · 2017-12-24T16:57:27Z

homeassistant/components/isy994.py

+        if domain not in hass.data[ISY994_NODES]:
+            hass.data[ISY994_NODES][domain] = []
+
+    if ISY994_WEATHER not in hass.data:


always true

pvizeli · 2017-12-24T16:57:33Z

homeassistant/components/isy994.py

+    if ISY994_WEATHER not in hass.data:
+        hass.data[ISY994_WEATHER] = []
+
+    if ISY994_PROGRAMS not in hass.data:


always true

pvizeli · 2017-12-24T16:57:44Z

homeassistant/components/isy994.py

+    if ISY994_PROGRAMS not in hass.data:
+        hass.data[ISY994_PROGRAMS] = {}
+        for domain in SUPPORTED_DOMAINS:
+            if domain not in hass.data[ISY994_PROGRAMS]:


always true

Also removes two stray debug lines

homeassistant added integration: isy994 platform: binary_sensor.isy994 cla-signed labels Dec 20, 2017

OverloadUT added the breaking-change label Dec 20, 2017

MartinHjelmare reviewed Dec 20, 2017

View reviewed changes

OverloadUT added 3 commits December 19, 2017 23:27

Use super() instead of explicit base class calls

df10d7a

Move hass argument to first position

742beb2

Use str.format instead of string addition

eacbf10

homeassistant added the cla-signed label Dec 20, 2017

MartinHjelmare reviewed Dec 20, 2017

View reviewed changes

OverloadUT added 2 commits December 20, 2017 16:38

Move program structure building and validation to component

614b581

Removes the need for a bunch of duplicate exception handling in each individual platform

balloobbot added component platform labels Dec 21, 2017

Space missing in error message

cc000d5

homeassistant added the cla-signed label Dec 21, 2017

MartinHjelmare reviewed Dec 21, 2017

View reviewed changes

Fix string comparison to use ==

b9f1812

homeassistant added the cla-signed label Dec 21, 2017

pvizeli previously requested changes Dec 24, 2017

View reviewed changes

pvizeli reviewed Dec 24, 2017

View reviewed changes

Explicitly check for attributes rather than catch AttributeError

61db17c

Also removes two stray debug lines

homeassistant added the cla-signed label Dec 24, 2017

Remove null checks on hass.data, as they are always null at this point

7185a65

pvizeli approved these changes Dec 26, 2017

View reviewed changes

pvizeli merged commit d687bc0 into home-assistant:dev Dec 26, 2017

OverloadUT mentioned this pull request Dec 28, 2017

Documentation updates for the changes to ISY994 home-assistant/home-assistant.io#4288

Merged

2 tasks

OverloadUT deleted the fix-isy994-newest-firmware branch December 28, 2017 20:12

balloob mentioned this pull request Jan 11, 2018

0.61 #11589

Merged

boojew mentioned this pull request Jan 29, 2018

ISY Component incorrectly detects light as fan #12030

Closed

OverloadUT mentioned this pull request Feb 15, 2018

Fixed 3 small issues in isy994 component #12421

Merged

1 task

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

ghost removed platform: binary_sensor.isy994 labels Mar 21, 2019

		@@ -106,23 +102,18 @@ def __init__(self, name: str, node, actions) -> None:
		ISYFanDevice.__init__(self, node)

Huge ISY994 platform cleanup, fixes support for 5.0.10 firmware #11243

Huge ISY994 platform cleanup, fixes support for 5.0.10 firmware #11243

Conversation

OverloadUT commented Dec 20, 2017 • edited Loading

Description:

Breaking Change Notes

Initial Problem Statement

List Of Changes

Example entry for configuration.yaml (if applicable):

Checklist:

MartinHjelmare left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OverloadUT commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OverloadUT commented Dec 21, 2017

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

MartinHjelmare Dec 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvizeli left a comment • edited Loading

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

OverloadUT Dec 24, 2017 • edited Loading

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

OverloadUT commented Dec 20, 2017 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

MartinHjelmare Dec 22, 2017 •

edited

Loading

pvizeli left a comment •

edited

Loading

OverloadUT Dec 24, 2017 •

edited

Loading