Add support for multiple Doorbird stations by oblogic7 · Pull Request #13994 · home-assistant/core

oblogic7 · 2018-04-19T01:40:12Z

Description:

Adds support for configuring multiple Doorbird stations.

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

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

doorbird:
  devices:
    - host: <host_ip>
      username: <username>
      password: <password>
      name: Side Entry
    - host: <host_ip>
      username: <username>
      password: <password>
      name: Front Door
      monitored_conditions:
         - doorbell
         - motion

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

houndci-bot · 2018-04-19T01:40:25Z


    return True

+class ConfiguredDoorbird():


expected 2 blank lines, found 1

houndci-bot · 2018-04-19T01:40:25Z

+                hass_url = doorstation_config.get(CONF_CUSTOM_URL)
+
+            # This will make HA the only service that gets doorbell events
+            url = '{}{}/{}/{}'.format(hass_url, API_URL, index + 1, SENSOR_DOORBELL)


line too long (84 > 79 characters)

houndci-bot · 2018-04-19T01:40:25Z

+            # Get the URL of this server
+            hass_url = hass.config.api.base_url
+
+            # Override it if another is specified in the component configuration


line too long (80 > 79 characters)

houndci-bot · 2018-04-19T01:40:25Z

+            _LOGGER.info("Connected to DoorBird at %s as %s", device_ip, username)
+            doorstations.append(ConfiguredDoorbird(device, name))
+        elif status[1] == 401:
+            _LOGGER.error("Authorization rejected by DoorBird at %s", device_ip)


line too long (80 > 79 characters)

houndci-bot · 2018-04-19T01:40:25Z

+        status = device.ready()
+
+        if status[0]:
+            _LOGGER.info("Connected to DoorBird at %s as %s", device_ip, username)


line too long (82 > 79 characters)

houndci-bot · 2018-04-19T01:40:25Z

+        device_ip = doorstation_config.get(CONF_HOST)
+        username = doorstation_config.get(CONF_USERNAME)
+        password = doorstation_config.get(CONF_PASSWORD)
+        name = doorstation_config.get(CONF_NAME) or 'DoorBird {}'.format(index + 1)


line too long (83 > 79 characters)

houndci-bot · 2018-04-19T01:40:26Z

 import voluptuous as vol

-from homeassistant.const import CONF_HOST, CONF_USERNAME, CONF_PASSWORD
+from homeassistant.const import CONF_HOST, CONF_USERNAME, CONF_PASSWORD, \


'homeassistant.const.CONF_DEVICES' imported but unused

houndci-bot · 2018-04-19T01:40:26Z

+                device.history_image_url(1, 'doorbell'), _CAMERA_LAST_VISITOR.format(doorstation.name),
+                _LAST_VISITOR_INTERVAL),
+            DoorBirdCamera(
+                device.history_image_url(1, 'motionsensor'), _CAMERA_LAST_MOTION.format(doorstation.name),


line too long (106 > 79 characters)

houndci-bot · 2018-04-19T01:40:26Z

+        async_add_devices([
+            DoorBirdCamera(device.live_image_url, _CAMERA_LIVE.format(doorstation.name), _LIVE_INTERVAL),
+            DoorBirdCamera(
+                device.history_image_url(1, 'doorbell'), _CAMERA_LAST_VISITOR.format(doorstation.name),


line too long (103 > 79 characters)

houndci-bot · 2018-04-19T01:40:26Z

+    for doorstation in hass.data.get(DOORBIRD_DOMAIN):
+        device = doorstation.device
+        async_add_devices([
+            DoorBirdCamera(device.live_image_url, _CAMERA_LIVE.format(doorstation.name), _LIVE_INTERVAL),


line too long (105 > 79 characters)

houndci-bot · 2018-04-19T01:45:42Z

+        username = doorstation_config.get(CONF_USERNAME)
+        password = doorstation_config.get(CONF_PASSWORD)
+        name = doorstation_config.get(CONF_NAME) \
+               or 'DoorBird {}'.format(index + 1)


continuation line over-indented for visual indent

syssi · 2018-04-19T04:57:29Z

Did you see the lint errors at the travis builds?

lint runtests: commands[1] | flake8
./homeassistant/components/doorbird.py:58:12: E127 continuation line over-indented for visual indent
./homeassistant/components/doorbird.py:102:1: D101 Missing docstring in public class
./homeassistant/components/doorbird.py:103:1: D102 Missing docstring in public method
./homeassistant/components/doorbird.py:108:1: D102 Missing docstring in public method
./homeassistant/components/doorbird.py:112:1: D102 Missing docstring in public method
./homeassistant/components/doorbird.py:115:1: E302 expected 2 blank lines, found 1
ERROR: InvocationError for command '/home/travis/build/home-assistant/home-assistant/.tox/lint/bin/flake8' (exited with code 1)

syssi · 2018-04-19T05:00:35Z

+
 CONFIG_SCHEMA = vol.Schema({
-    DOMAIN: vol.Schema({
-        vol.Required(CONF_HOST): cv.string,


I would like to suggest this schema:

doorbird: gateways: - host: <host_ip> username: <username> password: <password> doorbell_events: 1 name: Side Entry - host: <host_ip> username: <username> password: <password> doorbell_events: 1 name: Front Door

In this case the schema can be extended in future with parameters applied to all gateways without another beaking change of the configuration.

cp. #13517

This schema is better (extensible)
Although these are not really gateways I think. So gateways should be devices probably.

syssi · 2018-04-19T13:58:03Z

 CONF_DOORBELL_EVENTS = 'doorbell_events'
 CONF_CUSTOM_URL = 'hass_url_override'

+DEVICES_SCHEMA = vol.All(cv.ensure_list,


Call it DEVICE_SCHEMA and move the vol.All(cv.ensure_list, [DEVICE_SCHEMA]) part to the CONFIG_SCHEMA.

pvizeli

Why you wrap it into a devices?

doorbird:
  - xy: fdsf
     kds: dfsd
  - ...

That allow a more clean struct if you have only one device.

syssi · 2018-04-21T09:02:20Z

I've suggested this schema to be future proof: #13994 (comment)

syssi · 2018-04-25T06:14:01Z

@pvizeli Do you agree with the devices key?

pvizeli · 2018-04-27T22:51:28Z

It was only a comment. With 2 devices it look okay but with only one device (and maybe the biggest case) it look a bit wired. Anyway it's only a comment :)

syssi · 2018-04-28T11:32:32Z

Do you like to improve the schema of the doorbell events? You could use the entity_id instead of enumerate the devices:

# cp. https://www.home-assistant.io/components/binary_sensor.xiaomi_aqara/#xiaomi-wireless-button
- alias: Doorbird ring
  trigger:
    platform: event
    event_type: doorbird
    event_data:
      entity_id: doorbird.front_door
      doorbird_type: doorbell
  action:
     service: script.turn_on
       entity_id: script.doorbell

oblogic7 · 2018-04-29T00:51:59Z

I think that is a good idea. I'll work on making those changes.

Use dispatcher for doorbird pushed events.

houndci-bot · 2018-05-05T03:43:36Z

+class ConfiguredDoorbird():
+    """Attach additional information to pass along with configured device."""
+
+    def __init__(self, device, name, custom_url = None):


unexpected spaces around keyword / parameter equals

houndci-bot · 2018-05-05T03:43:37Z

+        doorstation.device.subscribe_notification(self._event_type, url)
+
+
+    @asyncio.coroutine


too many blank lines (2)

houndci-bot · 2018-05-05T03:43:37Z

+def setup_platform(hass, config, add_devices, discovery_info=None):
+    """Set up the Doorbird doorbell sensor platform."""
+
+    doorbird = hass.data.get(DOORBIRD_DOMAIN)


local variable 'doorbird' is assigned to but never used

houndci-bot · 2018-05-05T03:43:37Z

+import logging
+import datetime
+import voluptuous as vol
+from homeassistant.core import callback


'homeassistant.core.callback' imported but unused

houndci-bot · 2018-05-20T19:54:19Z

+            # Get the URL of this server
+            hass_url = hass.config.api.base_url
+
+            # Override url if another is specified in the doorstation configuration


line too long (83 > 79 characters)

houndci-bot · 2018-05-20T19:54:19Z

+        for sensor_type in doorstation.monitored_events:
+            name = '{} {}'.format(doorstation.name,
+                                  SENSOR_TYPES[sensor_type][0])
+            device_class = SENSOR_TYPES[sensor_type][1]


local variable 'device_class' is assigned to but never used

houndci-bot · 2018-05-20T19:54:19Z

+            # Get the URL of this server
+            hass_url = hass.config.api.base_url
+
+            # Override url if another is specified in the doorstation configuration


line too long (83 > 79 characters)

houndci-bot · 2018-05-20T19:54:19Z

+        for sensor_type in doorstation.monitored_events:
+            name = '{} {}'.format(doorstation.name,
+                                  SENSOR_TYPES[sensor_type][0])
+            device_class = SENSOR_TYPES[sensor_type][1]


local variable 'device_class' is assigned to but never used

MartinHjelmare · 2018-05-21T10:00:35Z

-import asyncio
 import logging
-
+import asyncio


Please sort imports 🔤 within groups standard library, 3rd party and homeassistant. There should be a blank line between the three groups.

MartinHjelmare · 2018-05-21T10:01:19Z

 """Support for powering relays in a DoorBird video doorbell."""
 import datetime
 import logging
-


Don't remove blank line between standard library and 3rd party imports.

MartinHjelmare · 2018-05-21T10:03:21Z

+
+        device = doorstation.device
+
+        switches = []


Move this out of the loop. It's accessed outside the loop.

MartinHjelmare · 2018-05-21T10:06:44Z

+        return self._monitored_events
+
+
+class DoorbirdEventSubscriber():


Why is this a class? Looks like you just need a function.

houndci-bot · 2018-05-21T13:33:47Z

+
+    return True
+
+def subscribe_events(hass, doorstation):


expected 2 blank lines, found 1

oblogic7 · 2018-05-21T13:36:04Z

@MartinHjelmare Thank you for the diligent code review. It is very helpful since I'm new to Python and HA development. Hopefully these latest changes are all that will be required.

syssi · 2018-05-23T20:42:55Z

I'm fine with the state of this PR. Nevertheless I would like to point you to this comment again: #13994 (comment)

It's important here/now because this is already a breaking change. The component shouldn't break again in the near future.

oblogic7 · 2018-05-23T20:48:24Z

@syssi How would I get the entity_id from within the subscribe_events method? This is before any entities (camera or switch) have been added via add_devices.

oblogic7 · 2018-05-29T17:28:43Z

I'm not familiar enough with HA core to know when/how an entity_id is assigned. This is ready to merge unless somebody can provide direction on @syssi's suggestion.

syssi · 2018-06-08T06:38:19Z

The subscribe_events call could be moved the setup_platform of the sensor/switch. The entity_id is available there and the code belongs somehow to the entity IMO. Please feel free to improve your PR. I will care about a soonish merge.

syssi · 2018-06-10T16:02:51Z

 CONF_CUSTOM_URL = 'hass_url_override'

+DOORBELL_EVENT = 'doorbell'
+MOTION_EVENT = 'motionsensor'


Could you call the event motion? I will approved and merge the PR afterwards.

That is the event name required by the device API for event subscriptions.

https://www.doorbird.com/downloads/api_lan.pdf?rev=0.20

Got it. Sounds a bit weird.

@oblogic7 That's only for monitor.cgi. The new schedule.cgi uses 'motion'. No need to update this PR, I'm fixing it in mine 🙂

oblogic7 added 2 commits April 18, 2018 15:45

Support for multiple DoorBird devices

3a85104

Remove accidental commit of partial component

2d9fb0a

homeassistant added cla-signed integration: doorbird platform: camera.doorbird labels Apr 19, 2018

houndci-bot reviewed Apr 19, 2018

View reviewed changes

Formatting

332e6d1

homeassistant added the cla-signed label Apr 19, 2018

houndci-bot reviewed Apr 19, 2018

View reviewed changes

Correct indentation

c3907b0

oblogic7 mentioned this pull request Apr 19, 2018

Add support for multiple Doorbird stations home-assistant/home-assistant.io#5205

Merged

2 tasks

syssi reviewed Apr 19, 2018

View reviewed changes

oblogic7 added 2 commits April 19, 2018 08:30

May this offering please the linting gods.

21c5c38

Requested change to config schema

0dfade4

syssi reviewed Apr 19, 2018

View reviewed changes

oblogic7 added 2 commits April 19, 2018 09:00

Requested schema update.

a37e86e

Punctuation

98e7339

pvizeli reviewed Apr 21, 2018

View reviewed changes

syssi approved these changes Apr 25, 2018

View reviewed changes

Move doorbell events to proper binary sensor.

645bfac

Use dispatcher for doorbird pushed events.

homeassistant added the cla-signed label May 5, 2018

houndci-bot reviewed May 5, 2018

View reviewed changes

Fix linting issues.

ccac2c4

Undo binary sensor changes. Fire events for motion and doorbell actions.

e55556f

houndci-bot reviewed May 20, 2018

View reviewed changes

oblogic7 added 4 commits May 20, 2018 14:56

Hound wanted a treat

e485192

Missed one.

8966636

Lint fixes

3e29719

More lint fixes

1b19410

MartinHjelmare reviewed May 21, 2018

View reviewed changes

Suggested changes

1ef8fff

houndci-bot reviewed May 21, 2018

View reviewed changes

Add blank line

334a3f0

homeassistant added the cla-signed label May 21, 2018

Missed a requested change

d0e511e

syssi mentioned this pull request May 22, 2018

Adding motion sensor #14585

Closed

8 tasks

alandtse mentioned this pull request May 22, 2018

Adds documentation for subscribing to motion events home-assistant/home-assistant.io#5414

Closed

2 tasks

Exclude notification reset if not specified on config

26a93bd

syssi mentioned this pull request Jun 9, 2018

Update to DoorBirdPy 2.0.2 #14800

Closed

6 tasks

syssi reviewed Jun 10, 2018

View reviewed changes

syssi changed the title ~~Doorbird component enhancements~~ Add support for multiple Doorbird stations Jun 10, 2018

syssi merged commit 1c561ea into home-assistant:dev Jun 10, 2018

balloob mentioned this pull request Jun 22, 2018

0.72 #15088

Merged

girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018

Add support for multiple Doorbird stations (home-assistant#13994)

c75b0da

home-assistant locked and limited conversation to collaborators Dec 10, 2018

ghost removed platform: camera.doorbird labels Mar 21, 2019

		doorstation.device.subscribe_notification(self._event_type, url)


		@asyncio.coroutine

		return self._monitored_events


		class DoorbirdEventSubscriber():

Uh oh!

Conversation

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

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

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!

syssi commented Apr 19, 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!

pvizeli left a comment

Choose a reason for hiding this comment

Uh oh!

syssi commented Apr 21, 2018

Uh oh!

syssi commented Apr 25, 2018

Uh oh!

pvizeli commented Apr 27, 2018

Uh oh!

syssi commented Apr 28, 2018

Uh oh!

oblogic7 commented Apr 29, 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!

oblogic7 commented May 21, 2018

Uh oh!

syssi commented May 23, 2018

oblogic7 commented Apr 19, 2018 •

edited

Loading

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