Add Konnected component with support for discovery, binary sensor, and switch by heythisisnate · Pull Request #13670 · home-assistant/core

heythisisnate · 2018-04-04T03:49:43Z

Description:

Adds a component for Konnected devices. Konnected is an open source application that runs on a NodeMCU ESP8266. Konnected develops hardware designed for connecting traditional wired alarm system sensors and sirens to home automation.

Pull request in home-assistant.github.io with documentation: home-assistant/home-assistant.io#5102

Example entry for `configuration.yaml`:

konnected:
  access_token: REPLACE_ME_WITH_A_RANDOM_STRING
  devices:
    - id: 8bcd53
      binary_sensors:
        - zone: 1
          type: door
          name: 'Front Door'
        - zone: 3
          type: motion
          name: 'Test Motion'
      switches:
        - zone: out
          name: siren
    - id: 438a38
      binary_sensors:
        - pin: 1
          type: motion
          name: 'Office Motion'
        - pin: 2
          type: door
          name: 'Office Door'
      switches:
        - pin: 5
          name: 'Garage Door'
          activation: low

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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:

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.

homeassistant · 2018-04-04T03:49:44Z

Hi @heythisisnate,

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 · 2018-04-04T03:49:54Z

line too long (101 > 79 characters)

houndci-bot · 2018-04-04T03:49:54Z

line too long (101 > 79 characters)

houndci-bot · 2018-04-04T03:49:54Z

line too long (111 > 79 characters)

houndci-bot · 2018-04-04T03:49:54Z

line too long (82 > 79 characters)

houndci-bot · 2018-04-04T03:49:54Z

line too long (90 > 79 characters)

houndci-bot · 2018-04-04T03:49:55Z

line too long (94 > 79 characters)

houndci-bot · 2018-04-04T03:49:55Z

line too long (102 > 79 characters)

houndci-bot · 2018-04-04T03:49:56Z

line too long (89 > 79 characters)

houndci-bot · 2018-04-04T03:49:56Z

line too long (93 > 79 characters)

houndci-bot · 2018-04-04T03:49:56Z

line too long (123 > 79 characters)

houndci-bot · 2018-04-04T17:13:25Z

continuation line over-indented for visual indent

houndci-bot · 2018-04-04T17:13:25Z

missing whitespace after ','

houndci-bot · 2018-04-04T17:13:25Z

expected 2 blank lines, found 1

houndci-bot · 2018-04-04T17:13:25Z

expected 2 blank lines, found 1

houndci-bot · 2018-04-04T17:13:25Z

'aiohttp' imported but unused

houndci-bot · 2018-04-04T17:13:26Z

blank line contains whitespace

houndci-bot · 2018-04-04T17:13:26Z

expected 2 blank lines, found 1

heythisisnate · 2018-04-06T20:56:30Z

This is my first Home Assistant PR. This code was written in collaboration with @emosenkis (squashed and cleaned up commits for easy reading). Can anyone help us out with a review?

fabaff · 2018-04-06T21:23:04Z

Looks like that this is required. We have a validation helper for strings in homeassistant.helpers.config_validation.

fabaff · 2018-04-06T21:30:44Z

The DOMAIN contains a schema as well.

fabaff · 2018-04-06T21:31:23Z

Please check const.py if there is already a defined constant.

fabaff · 2018-04-06T21:33:09Z

Is 'supersecret' the default? If yes, then pass it in the validation.

I opted to remove the default and make it required for the user to set.

houndci-bot · 2018-04-07T15:51:46Z

continuation line over-indented for visual indent

houndci-bot · 2018-04-07T15:51:46Z

continuation line over-indented for visual indent

houndci-bot · 2018-04-07T15:51:46Z

line too long (83 > 79 characters)

houndci-bot · 2018-04-07T15:51:46Z

line too long (80 > 79 characters)

houndci-bot · 2018-04-07T15:51:46Z

line too long (80 > 79 characters)

heythisisnate · 2018-04-07T19:36:16Z

Thank you for the feedback @fabaff. I think I've addressed all of the concerns mentioned.

heythisisnate · 2018-04-14T22:05:42Z

I just rebased on the latest dev branch and fixed a merge conflict. Is there anything else that I need to here @fabaff?

pfiggins · 2018-04-19T16:36:46Z

It would be great to get this accepted and merged in ASAP. Konnected is the whole reason I'm beginning to migrate from SmartThings!

dejanzelic · 2018-04-21T19:57:39Z

I'm excited for this module! I tried it out and it works flawlessly!

OttoWinter · 2018-04-21T20:19:44Z

The default seems to be set by the configuration schema:

vol.Required(CONF_TYPE, default='motion'): DEVICE_CLASSES_SCHEMA,

OttoWinter · 2018-04-21T20:20:24Z

This seems overly verbose for an info log message... I would consider it a debug message if at all.

OttoWinter · 2018-04-21T20:22:47Z

The binary sensor doesn't seem to have an update method, so passing True here has no effect and should be removed.

OttoWinter · 2018-04-21T20:23:42Z

This debug message also seems quite over-verbose. The async_schedule_update_ha_state() will already make a state changed debug message for you.

OttoWinter · 2018-04-21T20:27:33Z

If the user doesn't provide pin nor zone, this config validation will still pass even though it's clearly invalid. See has_at_least_one_key for potentially fixing this.

Nice. thanks for that tip.

OttoWinter · 2018-04-21T20:34:46Z

Suggestion: Please use the new async/await syntax if you can. We have Python 3.5 now and I've seen a few PRs getting comments like this one. See http://stackabuse.com/python-async-await-tutorial/ for a simple tutorial.

Ok, I will read up on it. I'm pretty new to Python so I'm sure that I just modeled this after some other code that maybe wasn't up date.

OttoWinter · 2018-04-21T20:38:09Z

Just a style suggestion: it's weird to read code where sometimes 'devices' is used and sometimes CONF_DEVICES. I would recommend using one of them and sticking to it IMO.

OttoWinter · 2018-04-21T20:39:13Z

This also seems like a non-info log message. Especially with info messages, I don't expect to see raw python dictionaries in the logs.

OttoWinter · 2018-04-21T20:41:33Z

These log messages should IMO not be info. Home Assistant's default is to print every log message above and including info. If someone needs these log messages they can still manually set the log level for this component using the logger component.

You're right, these were really meant for debugging discovery. I will change them to debug.

OttoWinter · 2018-04-21T20:43:57Z

This is not safe to timing attacks. See hmac.compare_digest for safe auth token checks.

OttoWinter · 2018-04-21T20:59:43Z

If this integration requires discovery: to be enabled as the docs state, it might be good to have it in DEPENDENCIES too I think.

bradleyscott · 2018-05-15T07:07:33Z

Assuming the constants, Would that mean it could be released as part of 0.70 @syssi?

syssi · 2018-05-15T07:12:15Z

I hope so. :-P I believe the merge window closes on friday.

pfiggins · 2018-05-15T11:30:18Z

Awesome! Nate has been great about turning the fixes around quickly. So it sounds like we'll be getting native integration in 0.70. Thank you @syssi!

heythisisnate · 2018-05-15T14:43:57Z

@syssi Thank you SO much for the review! 🙌
I've added the constants as suggested. Please have a second look.

Konnected users who are beta testing this: I've changed the auth_token configuration variable to access_token to be consistent with other similar HASS configs and to reuse an existing constant string. I'm updating the documentation accordingly.

syssi · 2018-05-15T15:14:25Z

@heythisisnate Sorry, but you've to move the const to the top of your component. The const.py is just used for prominent consts. ;-)

heythisisnate · 2018-05-15T15:19:25Z

@syssi Oh, gotcha. I misunderstood. Give me 5 minutes.

heythisisnate · 2018-05-15T15:33:12Z

@syssi updated

syssi · 2018-05-15T16:00:28Z

Do you like to add your component and name to the CODEOWNERS file? It basically means you'll automatically get notified for all changes in your source files.

heythisisnate · 2018-05-15T16:06:04Z

@syssi Nice. Added.

syssi

LGTM. There are some more consts which should be introduced on the long run IMO.

Kane610 · 2018-05-15T17:03:26Z

+    def save_data(self):
+        """Save the device configuration to `hass.data`.
+
+        TODO: This can probably be refactored and tidied up.


Since this is a new component we should make sure to have it as ready as possible. At the least remove todo comment, at best follow the todo 👍

I've removed the TODO comment. There will certainly be more work done on this component in the near future and opportunities to refactor later. This code works and has been beta tested for more than a month, so I don't think it's prudent to change right this moment or hold up an initial release.

heythisisnate · 2018-05-15T17:15:16Z

Thank you for the approval @syssi. So what happens next? Do I have to merge this or does somebody from the core team do the merge into dev?

heythisisnate · 2018-05-15T18:04:33Z

Woohoo! 🎉 Thanks for the merge.

@syssi is it possible for you to un-block the documentation PR for merge?:
home-assistant/home-assistant.io#5102

EDIT: Thanks!!

MartinHjelmare

Please open a new PR, to at least address the I/O being done inside coroutines.

MartinHjelmare · 2018-05-15T19:02:41Z

+import logging
+
+from homeassistant.components.binary_sensor import BinarySensorDevice
+from homeassistant.components.konnected import (DOMAIN, PIN_TO_ZONE)


We're not allowed to overwrite the binary sensor domain with the konnected component domain. Instead import it like so:

from homeassistant.components.konnected import (DOMAIN as KONNECTED_DOMAIN, ...)

MartinHjelmare · 2018-05-15T19:05:46Z

+        """Return the device class."""
+        return self._device_class
+
+    @asyncio.coroutine


We're now using Python 3.5 async syntax, ie async def.

MartinHjelmare · 2018-05-15T19:19:18Z

+
+        import konnected
+        self.client = konnected.Client(host, str(port))
+        self.status = self.client.get_status()


This call is making a request, ie I/O, and we're inside a coroutine here. That's not allowed. Probably change async_device_discovered to be non async as well as KonnectedDevice. If the interface library is not async, there's usually no point in making the home assistant component async.

Hmm, I think this was all originally synchronous but at the suggestion of another reviewer I made these into coroutines: #13670 (comment)
So now I'm thoroughly confused as to the best way to resolve. Seems like either I make everything synchronous or change the konnected library to use asyncio http. Which is preferred? Pros/cons of each?

Async is preferred, but I suggest you wait with that. It's probably easier to just make the home assistant component use the sync api for this release. Then you have time later to convert to async.

MartinHjelmare · 2018-05-15T19:21:04Z

+        if (desired_sensor_configuration != current_sensor_configuration) or \
+                (current_actuator_config != desired_actuator_config):
+            _LOGGER.debug('pushing settings to device %s', self.device_id)
+            self.client.put_settings(


This is also making a request inside a coroutine.

MartinHjelmare · 2018-05-15T19:25:03Z

+    def sensor_configuration(self):
+        """Return the configuration map for syncing sensors."""
+        return [{'pin': p} for p in
+                self.stored_configuration[CONF_BINARY_SENSORS].keys()]


.keys() is not needed when iterating a dict.

MartinHjelmare · 2018-05-15T19:34:42Z

+            return self.json_message('uninitialized sensor/actuator',
+                                     status_code=HTTP_INTERNAL_SERVER_ERROR)
+
+        await entity.async_set_state(state)


Entities should usually not be accessed directly from outside their platform. That's against the design philosophy of home assistant. Instead, we have the dispatch helper for this purpose, to send an update from the component to platform entities.

It's ok to store a key that references a specific device or entity, eg the entity_id in a dict to be able to know if the entity needs an update and send the update to the correct target with the dispatcher. But we shouldn't store the entities themselves so that they are accessible from outside the platform.

How do I get the entity_id to store in a dict? It's unclear to me where this is set or how to retrieve it. self.entity_id returns None from inside the entity class. I must be missing something here.

You might not need the entity_id. You don't need it specifically to use the dispatcher. I just mentioned it as an example identifier of an entity.

There's two parts to the dispatcher, a connect function and a send function. First you connect a target function to a target key. Then you send an update to the target key which will call all connected target functions of the target key.

The target key can be anything that is specific enough for what you want to target. If you want to target a specific entity, the entity_id is probably good. You could also use the builtin id function on the entity instance to get a unique id of the entity instance, if you don't need the key to be persistent between restarts.

You want to connect the entity target method from the entity coroutine method async_added_to_hass. At this point, the entity_id will be available on the entity, ie it will be something other than None.

I understand conceptually.
Could you point me to a component that uses this pattern? This would help me tremendously.

MartinHjelmare · 2018-05-15T19:38:54Z

+    def _set_state(self, state):
+        self._state = state
+        self._data[ATTR_STATE] = state
+        self.schedule_update_ha_state()


Is there no feedback from the device when it changes state? If there is no feedback, the entity should be marked to use assumed_state, ie set that entity property to True.

If there is feedback after a state change, we shouldn't update state from the turn_on/turn_off methods, but let the feedback handle that, if it's done within a couple of seconds.

homeassistant added cla-needed integration: discovery new-platform platform: binary_sensor.konnected integration: konnected labels Apr 4, 2018

houndci-bot reviewed Apr 4, 2018

View reviewed changes

homeassistant added cla-signed and removed cla-needed labels Apr 4, 2018

heythisisnate mentioned this pull request Apr 4, 2018

Add Konnected component home-assistant/home-assistant.io#5102

Merged

2 tasks

houndci-bot reviewed Apr 4, 2018

View reviewed changes

heythisisnate force-pushed the konnected branch from 42a1efd to d28105f Compare April 4, 2018 21:17

homeassistant added the cla-signed label Apr 4, 2018

fabaff reviewed Apr 6, 2018

View reviewed changes

homeassistant added the cla-signed label Apr 7, 2018

houndci-bot reviewed Apr 7, 2018

View reviewed changes

heythisisnate force-pushed the konnected branch from f080de3 to b051cca Compare April 7, 2018 15:57

homeassistant added the cla-signed label Apr 7, 2018

heythisisnate force-pushed the konnected branch from b051cca to 0fc832f Compare April 14, 2018 22:03

homeassistant added the cla-signed label Apr 14, 2018

homeassistant added the cla-signed label Apr 15, 2018

OttoWinter reviewed Apr 21, 2018

View reviewed changes

heythisisnate force-pushed the konnected branch from e9e7129 to 6c4e399 Compare April 22, 2018 04:43

heythisisnate requested a review from a team as a code owner May 15, 2018 14:36

homeassistant added the cla-signed label May 15, 2018

add constants suggested by @syssi

6049128

heythisisnate force-pushed the konnected branch from faa2d4c to 6049128 Compare May 15, 2018 15:30

homeassistant added the cla-signed label May 15, 2018

add to CODEOWNERS

f322164

homeassistant added the cla-signed label May 15, 2018

syssi approved these changes May 15, 2018

View reviewed changes

Kane610 reviewed May 15, 2018

View reviewed changes

remove TODO comment

a9316f4

homeassistant added the cla-signed label May 15, 2018

syssi merged commit de50d5d into home-assistant:dev May 15, 2018

MartinHjelmare reviewed May 15, 2018

View reviewed changes

heythisisnate mentioned this pull request May 16, 2018

Konnected component follow up #14491

Merged

2 tasks

balloob mentioned this pull request May 28, 2018

0.70.0 #14657

Merged

home-assistant locked and limited conversation to collaborators Sep 5, 2018

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

Uh oh!

Conversation

heythisisnate commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml:

Checklist:

Uh oh!

homeassistant commented Apr 4, 2018

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!

heythisisnate commented Apr 6, 2018

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!

heythisisnate commented Apr 7, 2018

Uh oh!

heythisisnate commented Apr 14, 2018

Uh oh!

pfiggins commented Apr 19, 2018

Uh oh!

dejanzelic commented Apr 21, 2018

Uh oh!

OttoWinter Apr 21, 2018 • 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!

heythisisnate commented Apr 4, 2018 •

edited

Loading

Example entry for `configuration.yaml`:

OttoWinter Apr 21, 2018 •

edited

Loading

OttoWinter Apr 21, 2018 •

edited

Loading

OttoWinter Apr 21, 2018 •

edited

Loading