Add deCONZ component #10321

Kane610 · 2017-11-03T21:40:33Z

Description:

Component supporting Deconz (lights, sensors) rest API and web sockets for push notification.

The Implementation is more or less code complete.

The Component supports all devices (on a theoretical level) supported by deCONZ. There are lists of verified devices in the documentation.

Pull request in home-assistant.github.io with documentation: home-assistant/home-assistant.io#3967
Pull request in home-assistant-polymer with new logo for configurator: home-assistant/frontend#689

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

deconz:
  host: 192.168.0.90
  api_key: 1234567890

If you don't have a API key and want to generate one use this config and then follow the steps in the GUI:

deconz:
  host: 192.168.0.90

The component will automatically create an API key and put it into deconz.conf.

If you prefer you can use auto discovery component instead and follow instructions in GUI.

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).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

houndci-bot · 2017-11-10T18:28:43Z

homeassistant/components/light/deconz.py

+
+from homeassistant.components.light import (Light,
+    ATTR_BRIGHTNESS, ATTR_COLOR_TEMP, ATTR_RGB_COLOR, ATTR_XY_COLOR,
+    SUPPORT_BRIGHTNESS, SUPPORT_COLOR_TEMP, SUPPORT_RGB_COLOR, SUPPORT_XY_COLOR)


continuation line under-indented for visual indent
line too long (80 > 79 characters)

houndci-bot · 2017-11-10T18:28:43Z

homeassistant/components/light/deconz.py

+import logging
+
+from homeassistant.components.light import (Light,
+    ATTR_BRIGHTNESS, ATTR_COLOR_TEMP, ATTR_RGB_COLOR, ATTR_XY_COLOR,


continuation line under-indented for visual indent

houndci-bot · 2017-11-10T18:28:43Z

homeassistant/components/light/deconz.py

+import asyncio
+import logging
+
+from homeassistant.components.light import (Light,


'homeassistant.components.light.ATTR_XY_COLOR' imported but unused

houndci-bot · 2017-11-12T00:27:10Z

homeassistant/components/sensor/deconz.py

+from homeassistant.helpers.dispatcher import (
+    async_dispatcher_send, async_dispatcher_connect)
+from homeassistant.helpers.entity import Entity
+from homeassistant.util import slugify


'homeassistant.util.slugify' imported but unused

tinloaf · 2017-11-15T22:31:52Z

homeassistant/components/deconz.py

+DOMAIN = 'deconz'
+
+DECONZ_DATA = 'deconz_data'
+CONFIG_FILE = 'deconz.conf'


I think the path to the config file should be configurable.

This is just the name, the read_json, load_json gets the full path from HASS. From my understanding this is standard behaviour.

As, I see what that file is for.

If it's not meant for users to manually read/write it, prefix the name with a .

tinloaf · 2017-11-15T22:34:00Z

homeassistant/components/deconz.py

+        vol.Required(CONF_HOST): cv.string,
+        vol.Optional(CONF_API_KEY): cv.string,
+        vol.Optional(CONF_PORT, default=80): cv.port,
+        vol.Optional(TYPE_AS_EVENT, default=['ZHASwitch']):


What is this supposed to do? It looks strange, and it's not documented in the accompanying documentation PR.

Normal behaviour for momentary sensors are to be events, I'd like to have an opening in getting it as a sensor entity instead by disabling it. I'm just keeping it configurable.

tinloaf · 2017-11-15T22:34:51Z

homeassistant/components/deconz.py

+DECONZ_DATA = 'deconz_data'
+CONFIG_FILE = 'deconz.conf'
+
+TYPE_AS_EVENT = 'type_as_event'


For naming reasons, this should probably also start with CONF_.

Sure, good catch!

tinloaf · 2017-11-15T22:38:16Z

homeassistant/components/deconz.py

+def async_setup(hass, config):
+    """Setup services for Deconz."""
+    deconz_config = config[DOMAIN]
+    config_file = load_json(hass.config.path(CONFIG_FILE))


Not sure, but: Since this is a coroutine, it runs in the event loop, and load_json probably blocks. Is there an asynchroneous way of reading the JSON?

There is no similar async version but from what I could see in Discord, Balloob didn't think this was an issue.

Okay, great! 👍

uuuh what. This is a really big issue. NEVER do I/O inside the event loop.

It is not a big issue that no async version exists, because one does never do I/O inside the event loop. Instead, do yield from hass.async_add_job(load_json, hass.config.path(CONFIG_FILE)). Although, if you are not familiar with how asyncio works, it is preferred that you don't write async components. Write sync ones instead.

tinloaf · 2017-11-15T22:40:37Z

homeassistant/components/deconz.py

+        Store API key in deconz.conf.
+        """
+        from pydeconz.utils import get_api_key
+        deconz_config[CONF_USERNAME] = call.data.get(CONF_USERNAME, 'delight')


I don't think that the default values for username / password should be hard coded here, but rather added as default value to the config schema. Nobody's going to look for the default value here. ;)

I guess I could create a second config schema for the service? Since the username and password is only relevant for the service to generate the API key. Not sure what is best :)

Oh, I just now realize that this is a service! Why do you make this a service? The component cannot work before that service was called / the API key was generated, can it? So how about this workflow:

On startup, check if an API key was generated

If so, initialize the component

If not, retrieve username / password from the config, generate a new API key

If that worked, initialize the component

If it didn't, fail the initialization

I had it like that initially, but then I thought that I don't want the user to need to remove the username and password afterwords. Originally I created a notification in the GUI for the user to write down and add to the configuration.yaml. This is the opposite in that after initially specifying the component you are not required to add any additional configuration for the component.

But in the end I guess it is a matter of taste.

I'm gonna think a bit more about this, you have some valid points!

I changed it to more as you suggested

tinloaf · 2017-11-15T22:41:23Z

homeassistant/components/deconz.py

+        deconz_config[CONF_USERNAME] = call.data.get(CONF_USERNAME, 'delight')
+        deconz_config[CONF_PASSWORD] = call.data.get(CONF_PASSWORD, 'delight')
+        api_key = yield from get_api_key(hass.loop, **deconz_config)
+        if not save_json(


Same as above - is there an async way of doing this?

See previous comment.

tinloaf · 2017-11-15T22:43:49Z

homeassistant/components/deconz.py

+    else:
+        hass.services.async_register(
+            DOMAIN, 'generate_api_key', generate_api_key)
+        _LOGGER.warning("Deconz needs API key to set up session. See docs.")


If the component can't do anything without an API key, this should probably be an error, not just a warning (i.e., return False here)

An error is a bit extreme since you still intentionally have to enable the component. Which means that if you don't specify the API key you have the intent of generating one using the service which will only load when the API key is missing.

Yes, I agree that a failure would be too hard if you have to call the generate_api_key service manually. But with the workflow from above, that should not be necessary, should it?

Absolutely. See the other post for my reasoning.

tinloaf · 2017-11-15T22:44:49Z

homeassistant/components/deconz.py

+                                                     DOMAIN,
+                                                     deconz_config,
+                                                     config))
+    deconz.start()


Does this block?

No, it is a asyncio transport class.

tinloaf · 2017-11-15T22:45:35Z

homeassistant/components/deconz.py

+
+
+class DeconzEvent(object):
+    """When you want signals instead of entities."""


I'm afraid I don't understand what that means. :(

I understand this since you only had time to spend on this file ;).

It is a event class that will take a device sensor and generate events in hass instead of creating a sensor entity.

I will improve the pydocstring.

amelchio · 2017-11-15T23:10:07Z

homeassistant/components/light/deconz.py

+        data = {'on': True}
+
+        if ATTR_BRIGHTNESS in kwargs:
+            data['bri'] = kwargs[ATTR_BRIGHTNESS]


You might want to move this down below RGB so one can specify both and have the RGB brightness overridden.

I hear you.

tinloaf · 2017-11-15T23:41:02Z

Regarding the event / entity thing: I think I got it now. I wonder though what the advantage of the "event mode" is over having an entity. If you have an entity for every sensor, you still get an event every time the sensor's state changes (a STATE_CHANGED event), so "entity" mode basically has all features of the "event" mode … and then some.

Also, from what I know it's pretty uncommon to not represent physical devices by entities, but by a stream of events instead. I think if my assumption above is correct (that the event mode doesn't give you any "extra features") I would be in favor of just using the "entity mode". Reduces code complexity and makes this component behave more like all other components.

Kane610 · 2017-11-15T23:42:59Z

This specific thing I've had a long talk with another author of a Deconz component as well as Balloob to get a proper decision. I can agree that some people would prefer entities for all devices, that's why I left the 'undocumented' configuration possibility to alter this from the default behaviour.

tinloaf · 2017-11-15T23:46:55Z

homeassistant/components/sensor/deconz.py

+    @property
+    def unit_of_measurement(self):
+        """Unit of measurement of this entity."""
+        return self._sensor.sensor_unit


I think you should translate here into home assistant - standardized units such as TEMP_CELSIUS (which is defined as 'C', while pydeconz would use 'Celsius'). The same probably holds for other units?

Good catch! Too quick on that one.

tinloaf · 2017-11-15T23:47:36Z

homeassistant/components/binary_sensor/deconz.py

+        return self._sensor.sensor_icon
+
+    @property
+    def unit_of_measurement(self):


See the other sensor file - maybe translation would be benefical here, although I am not sure what unit a binary sensor should have.

No that is true, copy paste issue...

tinloaf · 2017-11-15T23:48:10Z

homeassistant/components/binary_sensor/deconz.py

+    def device_state_attributes(self):
+        """Return the state attributes of the sensor."""
+        attr = {
+            'battery_level': self._sensor.battery,


ATTR_BATTERY_LEVEL from const.py

Good point!

tinloaf · 2017-11-15T23:48:51Z

homeassistant/components/binary_sensor/deconz.py

+            'manufacturer': self._sensor.manufacturer,
+            'model_number': self._sensor.modelid,
+            'reachable': self._sensor.reachable,
+            'uniqueid': self._sensor.uniqueid,


ATTR_ID from const.py ?

Good point!

tinloaf · 2017-11-15T23:49:47Z

homeassistant/components/binary_sensor/deconz.py

+    @property
+    def device_state_attributes(self):
+        """Return the state attributes of the sensor."""
+        attr = {


For the attribute names that cannot be imported from const.py, I think it would be cleaner to define your own ATTR_* constants in the component, so that you have one central place where they are defined and could be changed.

tinloaf · 2017-11-15T23:53:27Z

homeassistant/components/light/deconz.py

+    @property
+    def device_state_attributes(self):
+        """Return the state attributes of the sensor."""
+        if self._light.type != 'LightGroup':


See binary sensor.

tinloaf · 2017-11-15T23:59:31Z

Edit: If you're in a hurry

What follows is a long thread of me not understanding that the "sensors" sometimes represent clickable buttons instead of real sensors, and having the (on the first glance somewhat weird-looking) "event instead of entity" approach is probably a good idea. If you're in a hurry you can probably fast-forward to #10321 (comment)

Original comment

My preference for "entity only" is less because of what the user might want, but because of (in my eyes unnecessarily) added code complexity. I mean, you could remove two out of five classes (the event class and the battery class), you would not have the (somewhat strange, I think) "battery" entities, …

Also, breaking the "an phyiscal device is an entity" rule makes your component unusable with some other components, for example the recorder, mqtt statestream, anything that wants to do templating, anything that records historical data, …

But if @balloob is on okay with this, it's probably okay. 😉

Kane610 · 2017-11-16T00:01:20Z

Valid points

tinloaf

I just had another quick discussion with @balloob , and we think that physical sensors should always be represented by an entity, instead of "hiding" them, generating custom events plus a battery sensor.

We have to try to keep a certain consistency throughout the home assistant code base to ensure maintainability. Since we're not able to see the advantages, we think the added code complexity and different approach from all other sensor platforms is not worth it.

Kane610 · 2017-11-16T21:45:15Z

Sure, as long as there isn't a third change back to events later on ;)

tinloaf · 2017-11-16T21:49:24Z

Just to make sure: We're really talking about sensors here, not some buttons that are internally called "sensors", are we?

MartinHjelmare

Nice!

Kane610 · 2017-12-31T15:30:28Z

@MartinHjelmare what's left for this PR to be merged?

MartinHjelmare · 2017-12-31T15:34:41Z

I'm happy but our CLA bot seems to have troubles processing this PR. My guess is the PR simply has too many commits. Try squashing the older commits into one commit if you are comfortable with git. Otherwise you can make a new branch and copy your changes manually and open a new PR and link to this PR.

Kane610 · 2017-12-31T15:47:02Z

I will look into how to squash commits tomorrow

Kane610 · 2017-12-31T18:33:47Z

@balloob do you have any knowledge about the issue mentioned by MartinHjelmare?

…e line

Bump dependency to v23 Use only HASS aiohttp session Change when to use 'deconz' or domain or deconz data Clean up unused logger defines Remove unnecessary return values Fix faulty references to component documentation Move callback registration to after entity has been initialized by HASS Less inception style on pydocs ;) Simplify loading platforms by using a for loop Added voluptous schema for service Yaml file is for deconz only, no need to have the domain present Remove domain constraint when creating event title

Kane610 · 2018-01-01T13:53:27Z

@MartinHjelmare I squashed a few commits, less than a 100 commits seems to be the sweet spot. But there seems to be a bigger issue with travis atm

MartinHjelmare · 2018-01-01T15:32:18Z

@Kane610 I've restarted the build. Let's see what happens.

Kane610 · 2018-01-01T15:56:20Z

Great @MartinHjelmare ! It is ready for merging 🍾

MartinHjelmare

Great!

ScuttleSE · 2018-01-04T09:19:49Z

So...I'm not sure I'm doing this correctly, but I have a bug report for this component :)

I have merged this pull request with my current installation of 0.60 and have gotten the deCONZ integration running just fine.

Except for one type of sensor I have; the Aqara Temperature/Humidity sensor.

When this is added to HA from deCONZ it is added as three sensors. One for temperature, one for humidity and one for airpressure.

They are added as sensor.[name in deconz], sensor.[name in deconz]_2 and sensor.[name in deconz]_3

The problem is that this order is seeminly random. After restarting HA, the sensor that previously was temperature might be the humidity or airpressure sensor.

Attaching two screenshots to illustrate. The first image is before restarting HA, the second one is after restarting.

Kane610 · 2018-01-04T09:22:14Z

@ScuttleSE this is known. It is expected that the user name their devices uniquely.

ScuttleSE · 2018-01-04T09:26:54Z

How though? This is one device in deCONZ named temp_1_balcony which in HA gets named sensor.temp_1_balcony, sensor.temp_1_balcony_2 and sensor.temp_1_balcony_3

Kane610 · 2018-01-04T09:30:00Z

@ScuttleSE either through the API, which you can use the deconz service in hass for, I think you can also use the new web frontend phoscon to do it. I'm also planning on looking in to doing frontend support later on for deconz. Lets take further discussions on the forums, this PR is closed.

ScuttleSE · 2018-01-04T09:33:49Z

@Kane610 But...maybe I am misunderstanding here... :)

I have named the device temp_1_balcony using the phoscon webthing, that's where I added the sensor too, but there is only an option to name the whole device there, not the three "sub-sensors".

When the device gets sucked into HA through the API, it gets added as three separate devices, and HA seems to add the extensions _2 and _3 to two of the sensors at random to be able to differentiate between them

Kane610 · 2018-01-04T09:35:46Z

As I said, lets take further discussions in the forum https://community.home-assistant.io/t/conbee-zigbee-stick-and-ha-will-this-work-together/

ScuttleSE · 2018-01-04T09:38:18Z

~~I would if the site weren't in read-only mode :)~~

Never mind, it just disappeared

Kane610 · 2018-01-04T09:39:47Z

I don't understand how that could be, have you posted earlier in the forums?

MartinHjelmare · 2018-01-04T19:04:11Z

Please open an issue if you suspect a bug. If you need help please use our help channels:
https://home-assistant.io/help/#communication-channels

Merged PRs should not be used for support, bug reports or to discuss the implementation. Thanks!

homeassistant added new-platform platform: binary_sensor.deconz integration: deconz cla-signed labels Nov 3, 2017

balloobbot added component platform labels Nov 3, 2017

Kane610 requested a review from andrey-git as a code owner November 5, 2017 23:35

homeassistant added the cla-signed label Nov 5, 2017

balloobbot added the requirement label Nov 5, 2017

homeassistant added the cla-signed label Nov 6, 2017

houndci-bot reviewed Nov 10, 2017

View reviewed changes

Kane610 force-pushed the deconz-component branch from eecc13b to 4d4d2e4 Compare November 12, 2017 00:27

homeassistant added the cla-signed label Nov 12, 2017

houndci-bot reviewed Nov 12, 2017

View reviewed changes

This was referenced Nov 12, 2017

Add deCONZ component documentation home-assistant/home-assistant.io#3967

Merged

deconz not auto-discovered any more in Home Assistant dresden-elektronik/deconz-rest-plugin#257

Closed

tinloaf reviewed Nov 15, 2017

View reviewed changes

amelchio reviewed Nov 15, 2017

View reviewed changes

tinloaf reviewed Nov 15, 2017

View reviewed changes

homeassistant added the cla-signed label Nov 16, 2017

tinloaf previously requested changes Nov 16, 2017

View reviewed changes

homeassistant added the cla-signed label Dec 31, 2017

MartinHjelmare approved these changes Dec 31, 2017

View reviewed changes

Kane610 added 2 commits January 1, 2018 11:55

Fix flake8 Multi-line docstring closing quotes should be on a separat…

3f82d98

…e line

Kane610 force-pushed the deconz-component branch from b83d816 to b0582b1 Compare January 1, 2018 10:58

homeassistant added the cla-signed label Jan 1, 2018

MartinHjelmare approved these changes Jan 1, 2018

View reviewed changes

MartinHjelmare changed the title ~~deCONZ component~~ Add deCONZ component Jan 1, 2018

MartinHjelmare merged commit b9c8523 into home-assistant:dev Jan 1, 2018

home-assistant locked and limited conversation to collaborators Jan 4, 2018

Kane610 deleted the deconz-component branch January 31, 2018 21:24

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



		class DeconzEvent(object):
		"""When you want signals instead of entities."""

Add deCONZ component #10321

Add deCONZ component #10321

Conversation

Kane610 commented Nov 3, 2017 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinloaf commented Nov 15, 2017

Kane610 commented Nov 15, 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

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

tinloaf commented Nov 15, 2017 • edited Loading

Edit: If you're in a hurry

Original comment

Kane610 commented Nov 16, 2017

tinloaf left a comment

Choose a reason for hiding this comment

Kane610 commented Nov 16, 2017

tinloaf commented Nov 16, 2017

MartinHjelmare left a comment

Choose a reason for hiding this comment

Kane610 commented Dec 31, 2017

MartinHjelmare commented Dec 31, 2017 • edited Loading

Kane610 commented Dec 31, 2017

Kane610 commented Dec 31, 2017

Kane610 commented Jan 1, 2018 • edited Loading

MartinHjelmare commented Jan 1, 2018

Kane610 commented Jan 1, 2018

MartinHjelmare left a comment

Choose a reason for hiding this comment

ScuttleSE commented Jan 4, 2018 • edited Loading

Kane610 commented Jan 4, 2018

ScuttleSE commented Jan 4, 2018

Kane610 commented Jan 4, 2018 • edited Loading

ScuttleSE commented Jan 4, 2018 • edited Loading

Kane610 commented Nov 3, 2017 •

edited

Loading

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

Kane610 commented Nov 15, 2017 •

edited

Loading

tinloaf commented Nov 15, 2017 •

edited

Loading

MartinHjelmare commented Dec 31, 2017 •

edited

Loading

Kane610 commented Jan 1, 2018 •

edited

Loading

ScuttleSE commented Jan 4, 2018 •

edited

Loading

Kane610 commented Jan 4, 2018 •

edited

Loading

ScuttleSE commented Jan 4, 2018 •

edited

Loading

ScuttleSE commented Jan 4, 2018 •

edited

Loading