Ihc component and platforms by dingusdk · Pull Request #10916 · home-assistant/core

dingusdk · 2017-12-03T15:24:55Z

Description:

Integration of the LK IHC controller (sold under other names like "Elko Living System" in Sweeden and Norway)

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

homeassistant · 2017-12-03T15:24:56Z

Hi @dingusdk,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

houndci-bot · 2017-12-03T15:25:05Z

+            switch.set_name(name)
+            _LOGGER.info("IHC switch set name: " + name + " " + str(ihcid))
+    else:
+        switch = IHCSwitch(ihccontroller, name, ihcid, ihcname, ihcnote, ihcposition)


line too long (85 > 79 characters)

houndci-bot · 2017-12-03T15:25:05Z

+    return add_switch(devices, ihccontroller, ihcid, name, False, ihcname, ihcnote, ihcposition)
+
+def add_switch(devices, ihccontroller, ihcid: int, name: str, overwrite: bool = False,
+               ihcname: str = "", ihcnote: str = "", ihcposition: str = "") -> IHCSwitch:


line too long (89 > 79 characters)

houndci-bot · 2017-12-03T15:25:05Z

+    ihcposition = product.attrib['position']
+    return add_switch(devices, ihccontroller, ihcid, name, False, ihcname, ihcnote, ihcposition)
+
+def add_switch(devices, ihccontroller, ihcid: int, name: str, overwrite: bool = False,


expected 2 blank lines, found 1
line too long (86 > 79 characters)

houndci-bot · 2017-12-03T15:25:05Z

+    ihcname = product.attrib['name']
+    ihcnote = product.attrib['note']
+    ihcposition = product.attrib['position']
+    return add_switch(devices, ihccontroller, ihcid, name, False, ihcname, ihcnote, ihcposition)


line too long (96 > 79 characters)

houndci-bot · 2017-12-03T15:25:06Z

+            pass
+
+
+def add_switch_from_node(devices, ihccontroller, ihcid: int, name: str, product) -> IHCSwitch:


line too long (94 > 79 characters)

houndci-bot · 2017-12-03T15:25:06Z

+
+_IHCSWITCHES = {}
+
+def setup_platform(hass, config, add_devices_callback, discovery_info=None):


expected 2 blank lines, found 1

houndci-bot · 2017-12-03T15:25:06Z

+
+PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
+    vol.Optional(CONF_AUTOSETUP, default='False'): cv.boolean,
+    vol.Optional(CONF_SWITCHES) :


whitespace before ':'

houndci-bot · 2017-12-03T15:25:06Z

+"""
+IHC switch platform that implements a switch.
+"""
+# pylint: disable=too-many-arguments, too-many-instance-attributes, bare-except, unused-argument, missing-docstring


line too long (115 > 79 characters)

houndci-bot · 2017-12-03T15:25:06Z

+
+def add_sensor(devices, ihccontroller, ihcid: int, name: str, sensortype: str,
+               unit: str, overwrite: bool = False,
+               ihcname: str = "", ihcnote: str = "", ihcposition: str = "") -> IHCSensor:


line too long (89 > 79 characters)

houndci-bot · 2017-12-03T15:25:06Z

+        except:
+            pass
+
+def add_sensor(devices, ihccontroller, ihcid: int, name: str, sensortype: str,


expected 2 blank lines, found 1

houndci-bot · 2017-12-03T15:25:07Z

+            switch.set_name(name)
+            _LOGGER.info("IHC switch set name: " + name + " " + str(ihcid))
+    else:
+        switch = IHCSwitch(ihccontroller, name, ihcid, ihcname, ihcnote, ihcposition)


line too long (85 > 79 characters)

houndci-bot · 2017-12-03T15:25:07Z

+    return add_switch(devices, ihccontroller, ihcid, name, False, ihcname, ihcnote, ihcposition)
+
+def add_switch(devices, ihccontroller, ihcid: int, name: str, overwrite: bool = False,
+               ihcname: str = "", ihcnote: str = "", ihcposition: str = "") -> IHCSwitch:


line too long (89 > 79 characters)

houndci-bot · 2017-12-03T15:25:07Z

+    ihcposition = product.attrib['position']
+    return add_switch(devices, ihccontroller, ihcid, name, False, ihcname, ihcnote, ihcposition)
+
+def add_switch(devices, ihccontroller, ihcid: int, name: str, overwrite: bool = False,


expected 2 blank lines, found 1
line too long (86 > 79 characters)

houndci-bot · 2017-12-03T15:25:07Z

+    ihcname = product.attrib['name']
+    ihcnote = product.attrib['note']
+    ihcposition = product.attrib['position']
+    return add_switch(devices, ihccontroller, ihcid, name, False, ihcname, ihcnote, ihcposition)


line too long (96 > 79 characters)

houndci-bot · 2017-12-03T15:25:07Z

+            pass
+
+
+def add_switch_from_node(devices, ihccontroller, ihcid: int, name: str, product) -> IHCSwitch:


line too long (94 > 79 characters)

houndci-bot · 2017-12-03T15:25:08Z

+
+_IHCSWITCHES = {}
+
+def setup_platform(hass, config, add_devices_callback, discovery_info=None):


expected 2 blank lines, found 1

houndci-bot · 2017-12-03T15:25:08Z

+
+PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
+    vol.Optional(CONF_AUTOSETUP, default='False'): cv.boolean,
+    vol.Optional(CONF_SWITCHES) :


whitespace before ':'

houndci-bot · 2017-12-03T15:25:08Z

+"""
+IHC switch platform that implements a switch.
+"""
+# pylint: disable=too-many-arguments, too-many-instance-attributes, bare-except, unused-argument, missing-docstring


line too long (115 > 79 characters)

houndci-bot · 2017-12-03T15:25:08Z

+
+def add_sensor(devices, ihccontroller, ihcid: int, name: str, sensortype: str,
+               unit: str, overwrite: bool = False,
+               ihcname: str = "", ihcnote: str = "", ihcposition: str = "") -> IHCSensor:


line too long (89 > 79 characters)

houndci-bot · 2017-12-03T15:25:08Z

+        except:
+            pass
+
+def add_sensor(devices, ihccontroller, ihcid: int, name: str, sensortype: str,


expected 2 blank lines, found 1

MartinHjelmare

Thanks for the PR! I've commented in depth on the binary sensor platform and on the component.

There are quite many comments, but I've tried to be very specific for these two modules. Hopefully my comments make sense so that you can refactor the other modules in a similar way. But let me know if I should clarify anything.

In general I think you can move many functions from the platforms to the component and be able to reuse them in all the platforms by modifying the functions somewhat and then import them from the component to the platforms as needed.

One thing that you could think about is to change the configuration from individual platform config sections, to have a more streamlined config section only for the ihc component and use the discovery helper to set up the platforms from the component. This might not be possible if you have very specific configuration needs for the platforms. But it's something to consider since it could simplify things.

Another thing that you should think about is moving more of the PRODUCTAUTOSETUP to the ihcsdk library. Is it needed to have that info be defined in home assistant, or could it just be imported?

MartinHjelmare · 2017-12-17T17:08:27Z

@@ -0,0 +1,183 @@
+"""IHC binary sensor platform."""
+# pylint: disable=too-many-instance-attributes


Remove this. It's globally disabled already.

Done. (The reason for having these is because I am developing on Windows, and had problems getting pylint behave the same way as on linux.)

MartinHjelmare · 2017-12-17T17:12:11Z

+"""IHC binary sensor platform."""
+# pylint: disable=too-many-instance-attributes
+import logging
+import voluptuous as vol


Please group and sort imports according to PEP8.
https://www.python.org/dev/peps/pep-0008/#imports

Sort the imports 🔡 within each group standard library, 3rd party and homeassistant.

MartinHjelmare · 2017-12-21T17:44:48Z

+
+PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
+    vol.Optional(CONF_AUTOSETUP, default='False'): cv.boolean,
+    vol.Optional(CONF_BINARY_SENSORS):


Also validate the list with cv.ensure_list, and default to empty list.

vol.Optional(CONF_BINARY_SENSORS, default=[]): vol.All(cv.ensure_list, [...])

MartinHjelmare · 2017-12-21T17:45:55Z

+})
+
+
+PRODUCTAUTOSETUP = [


PRODUCT_AUTO_SETUP

The product auto setup has been completely changed and simplified. The configuration has been moved out of the code to a separate configuration file. This also allow the end user to modify the auto configuration (something I have had as a request from some of the users)

MartinHjelmare · 2017-12-21T17:46:45Z

@@ -0,0 +1,9 @@
+"""IHC platform constants."""
+
+CONF_AUTOSETUP = 'autosetup'


CONF_AUTO_SETUP = 'auto_setup'

MartinHjelmare · 2017-12-21T19:25:10Z

+        """Initialize IHC attributes."""
+        self.ihc = ihccontroller
+        self._name = name
+        self._ihcid = ihcid


self._ihc_id = ihc_id

MartinHjelmare · 2017-12-21T19:25:51Z

+class IHCDevice:
+    """Base class for all ihc devices."""
+
+    def __init__(self, ihccontroller, name, ihcid, ihcname: str,


Please use snake case variable names when the names contain multiple words for legibility. Eg:

def __init__(self, ihc_controller, name, ihc_id, ihc_name: str,

MartinHjelmare · 2017-12-21T19:27:05Z

+        self._name = name
+        self._ihcid = ihcid
+        self.ihcname = ihcname
+        self.ihcnote = ihcnote


Same as above.

MartinHjelmare · 2017-12-21T19:27:14Z

+        self._ihcid = ihcid
+        self.ihcname = ihcname
+        self.ihcnote = ihcnote
+        self.ihcposition = ihcposition


Same as above.

MartinHjelmare · 2017-12-21T19:28:33Z

+_IHCSWITCHES = {}
+
+
+def setup_platform(hass, config, add_devices_callback, discovery_info=None):


…tant into ihc-platform

dingusdk · 2018-01-07T15:11:33Z

Most of the changes requested in the review have been fixed, and I just replied "done" to each of them.

The product auto setup has been simplified and the configuration moved to a separate yaml file, and the user can overwrite the default configuration by creating a yaml file in the home assistant configuration folder. This was primarily because some users requested to be able top do this.
I have not moved the product auto setup to the ihcsdk library as you suggested, because it is a mapping specific to how ihc products should map to home assistant components. If you think it should be moved out I can move it (or maybe to a new ihc home assistant auto setup library).

About moving platform configuration to the component, I don't think it will be more simple because you would have to specify what platform each manual setup of a ihc resource should map to.

The discovery helper is that not for finding component using uPNP, SSDP? - so if the ihc controller did have this, I could use it to find the ihc controller (unfortunately the ihc controller do not have something like this). I don't understand how I could use this for setup of the platforms for the ihc component.

MartinHjelmare

Nice refactor! See comments below.

MartinHjelmare · 2018-01-07T14:20:07Z

+        """Initialize the caching of the project."""
+        self.ihc_controller = ihc_controller
+        self.info = False
+        project = self.ihc_controller.get_project()


Please break this out and do this before instantiating the Ihc wrapper. You don't want to have an instance that is not completely setup. Pass the project to the init method.

MartinHjelmare · 2018-01-07T14:20:50Z

+                                     AUTO_SETUP_YAML)
+        yaml = load_yaml_config_file(yaml_path)
+        try:
+            self.auto_setup_conf = AUTO_SETUP_SCHEMA(yaml)


Same as above.

MartinHjelmare · 2018-01-07T14:22:55Z

+        return False
+
+    ihc = Ihc(hass, ihc_controller)
+    ihc.info = conf.get(CONF_INFO)


Why not pass this to the init method of Ihc?

I have moved the auto setup to the IHC component and the IHC wrapper is no longer needed

MartinHjelmare · 2018-01-07T14:25:14Z

+                                and node.attrib['setting'] == 'yes'):
+                            continue
+                        ihc_id = int(node.attrib['id'].strip('_'), 0)
+                        name = groupname + "_" + str(ihc_id)


Please use new style string formatting, not string concatenation.

I assume "new style" it the string.format (I am new to python so i don't know what is new and old)

MartinHjelmare · 2018-01-07T14:27:27Z

+
+    def set_runtime_value_bool(call):
+        """Set a IHC runtime bool value service function."""
+        ihc_id = int(call.data.get(ATTR_IHC_ID, 0))


Schema validation has already ensured the type is correct. Please remove the int copy.

Schema should have the default value. Don't set that here.

MartinHjelmare · 2018-01-07T17:08:24Z

+    @property
+    def today_energy_kwh(self):
+        """Return the today total energy usage in kWh."""
+        return 0


Same as above.

MartinHjelmare · 2018-01-07T17:08:47Z

+
+    def turn_on(self, **kwargs):
+        """Turn the switch on."""
+        self._state = True


MartinHjelmare · 2018-01-07T17:09:06Z

+        """Turn the switch on."""
+        self._state = True
+        self.ihc.ihc_controller.set_runtime_value_bool(self._ihc_id, True)
+        self.schedule_update_ha_state()


MartinHjelmare · 2018-01-07T17:09:25Z

+
+    def turn_off(self, **kwargs):
+        """Turn the device off."""
+        self._state = False


MartinHjelmare · 2018-01-07T17:11:32Z

+        self.inverting = inverting
+
+    @property
+    def should_poll(self):


You can move this to the main device class.

MartinHjelmare · 2018-01-07T17:20:39Z

+        """Turn the device off."""
+        self._state = False
+        self.ihc.ihc_controller.set_runtime_value_bool(self._ihc_id, False)
+        self.schedule_update_ha_state()


dingusdk · 2018-01-13T12:05:15Z

I have moved the auto setup from each platform to the IHC component - as you also suggested. (I looked at how other components are doing this, and found the dicovery_info to be just a dictionary of whatever data you want to pass toeach platform) This simplifies it even more and the IHC wrapper is also no longer needed, because all the auto setup is within the ihc component. If the user has no manual IHC products to setup, only the IHC component need to be setup - also makes it more simple to the user.

donnib · 2018-01-18T14:51:54Z

@dingusdk i've looked a bit at this and i could not find implementation regarding the Wireless IHC components such as IHC Battery operated switches like https://github.com/home-assistant/home-assistant/pull/10321/files#diff-80c939d2fd205960d0558ad5cfdcf55bR192 or https://www.harald-nyborg.dk/p8943/wireless-batteritryk-4-slutte.

I would love to have support for those, if you decide to do something like that then i strongly suggest you don't create entities for these and just send events on the HASS buss since such switches does not have a state, see here how to send an event https://github.com/home-assistant/home-assistant/pull/10321/files#diff-80c939d2fd205960d0558ad5cfdcf55bR192

Also if you add support for these devices AND the IHC api has information on Battery status then the battery status should be created in HASS as a separete entity for each switch.

MartinHjelmare

Looks very nice! Mostly some minor touch-ups below.

NB, the lazy loading of service descriptions that affect your registration of services.

MartinHjelmare · 2018-01-18T16:41:17Z

+
+
+def setup_platform(hass, config, add_devices, discovery_info=None):
+    """Set up the IHC binary setsor platform."""


MartinHjelmare · 2018-01-18T16:45:10Z

+                vol.Required(CONF_NODE): cv.string,
+                vol.Optional(CONF_TYPE, default='Temperature'): cv.string,
+                vol.Optional(CONF_UNIT_OF_MEASUREMENT,
+                             default='°C'): cv.string,


Import TEMP_CELSIUS from const.py and use that as default unit.

MartinHjelmare · 2018-01-18T16:50:49Z

+    yaml = load_yaml_config_file(yaml_path)
+    try:
+        auto_setup_conf = AUTO_SETUP_SCHEMA(yaml)
+    except VoluptuousError as exception:


except vol.Invalid as exc:

The except VoluptuousError will catch all error from voluptuous (I copied it from somewhere else in home-assistant), but I assume your reason is that vol.Invalid is better because the schema is always valid (build into the code). I have changed it to vol.Invalid

MartinHjelmare · 2018-01-18T16:52:01Z

+        component_setup = auto_setup_conf[component]
+        discovery_info = get_discovery_info(component_setup, groups)
+        if len(discovery_info) > 0:
+            discovery.load_platform(hass, component, DOMAIN, discovery_info)


Add the original config as fifth argument.

I have added the config. It is not used, and I can see other components also not passing the config, but I guess it is something like good practice in case it is need later.

MartinHjelmare · 2018-01-18T16:52:23Z

+    for component in IHC_PLATFORMS:
+        component_setup = auto_setup_conf[component]
+        discovery_info = get_discovery_info(component_setup, groups)
+        if len(discovery_info) > 0:


if discovery_info:

MartinHjelmare · 2018-01-18T16:59:59Z

+    @asyncio.coroutine
+    def async_added_to_hass(self):
+        """Add callback for ihc changes."""
+        self.info = self.hass.data[IHC_DATA][IHC_INFO]


This you should be able to pass in __init__ via setup_platform.

MartinHjelmare · 2018-01-18T17:00:48Z

+    def on_ihc_change(self, ihc_id, value):
+        """Callback when ihc resource changes.
+
+        Derived classes must overwrite this todo device specific stuff.


"this to do device"

MartinHjelmare · 2018-01-18T17:01:30Z

+      description: The boolean value to set
+
+set_runtime_value_int:
+  description: Set a integer runtime value on the ihc controller


"Set an integer"

MartinHjelmare · 2018-01-18T17:06:16Z

+
+    def on_ihc_change(self, ihc_id, value):
+        """Callback from Ihc notifications."""
+        if type(value) is int:


if isinstance(value, int):

if isinstance(value, int):

is not working because if value is bool it will also return true. I have reversed to

if isinstance(value, bool):

Then it is working because - when value is int it will return false. (Now I can remove the #pylint disable)

MartinHjelmare · 2018-01-18T17:08:58Z

+from homeassistant.components.ihc import (
+    validate_name, IHC_DATA, IHC_CONTROLLER)
+from homeassistant.components.ihc.ihcdevice import IHCDevice
+from homeassistant.components.switch import (SwitchDevice, PLATFORM_SCHEMA)


Imported names that fit a single line don't need parenthesis.

dingusdk · 2018-01-20T12:38:58Z

@donnib You are right about the IHC wireless switches, and it would be nice to have push buttons in home assistant. If you want to have the events from a IHC button into home assistant you can associate the IHC button resource id with a binary sensor in home assistant (manually setup). The binary sensor will be on when the button is pressed and off when not pressed - then you can use it from automation to control something.

I don't think IHC expose the battery level anywhere in the soap api - at least I have not been able to find it. It is only in the IHC log - as a warning when battery level is low.

MartinHjelmare

Looks great!

dingusdk added 3 commits December 3, 2017 14:24

Added IHC platform

b81abe9

Updated requirements for IHC platform

4f6abc6

Exclude IHC from test

9afff5a

dingusdk requested a review from andrey-git as a code owner December 3, 2017 15:24

homeassistant added new-platform platform: binary_sensor.ihc integration: ihc cla-needed labels Dec 3, 2017

houndci-bot reviewed Dec 3, 2017

View reviewed changes

dingusdk mentioned this pull request Dec 3, 2017

New IHC platform home-assistant/home-assistant.io#4129

Merged

homeassistant added cla-signed and removed cla-needed labels Dec 3, 2017

Correcting flake8 issues

97986c4

homeassistant added the cla-signed label Dec 3, 2017

balloobbot added platform requirement labels Dec 3, 2017

Fixing more flake8 issues

28e26c7

homeassistant added the cla-signed label Dec 3, 2017

dingusdk added 3 commits December 3, 2017 19:57

Fixed flake8 issues

db2fa5a

Fixing pylint issues

ce17c41

Fixed flake8 issues

1b2a18f

MartinHjelmare requested changes Dec 21, 2017

View reviewed changes

Changes from PR review.

471bbf0

dingusdk added 4 commits January 7, 2018 13:20

Spelling mistake in comment

be1a9aa

Merge branch 'ihc-platform' of https://github.com/dingusdk/home-assis…

f93a617

…tant into ihc-platform

Updated requirements_all.txt with gen_requirements_app.py

12c6dfa

Pylint fix: No space allowed around keyword argument assignment

7bb0bfd

MartinHjelmare requested changes Jan 7, 2018

View reviewed changes

MartinHjelmare reviewed Jan 7, 2018

View reviewed changes

dingusdk added 2 commits January 8, 2018 15:26

PR review changes

1aa6a2a

Moved auto setup from platforms to ihc component

0fda18d

Do no auto setup if there are no IHC products found

ba0ff92

MartinHjelmare requested changes Jan 18, 2018

View reviewed changes

Changes from PR review

5cfee95

homeassistant added the cla-signed label Jan 20, 2018

MartinHjelmare approved these changes Jan 20, 2018

View reviewed changes

MartinHjelmare changed the title ~~Ihc platform~~ Ihc component and platforms Jan 20, 2018

MartinHjelmare merged commit e02d5e7 into home-assistant:dev Jan 20, 2018

balloob mentioned this pull request Jan 26, 2018

0.62 #11921

Merged

dingusdk deleted the ihc-platform branch March 3, 2018 09:52

home-assistant locked and limited conversation to collaborators Jul 26, 2018

ghost added integration: const integration: ihcdevice and removed platform: binary_sensor.ihc labels Mar 21, 2019

		pass


		def add_switch_from_node(devices, ihccontroller, ihcid: int, name: str, product) -> IHCSwitch:


		_IHCSWITCHES = {}

		def setup_platform(hass, config, add_devices_callback, discovery_info=None):

		@@ -0,0 +1,183 @@
		"""IHC binary sensor platform."""
		# pylint: disable=too-many-instance-attributes

		@@ -0,0 +1,9 @@
		"""IHC platform constants."""

		CONF_AUTOSETUP = 'autosetup'



		def setup_platform(hass, config, add_devices, discovery_info=None):
		"""Set up the IHC binary setsor platform."""

		})


		PRODUCTAUTOSETUP = [

Uh oh!

Conversation

dingusdk commented Dec 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Uh oh!

homeassistant commented Dec 3, 2017

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!

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

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!

dingusdk commented Dec 3, 2017 •

edited

Loading

MartinHjelmare left a comment •

edited

Loading