DoorBird by Klikini · Pull Request #8871 · home-assistant/core

Klikini · 2017-08-06T18:29:31Z

Description:

Adds components for the DoorBird video doorbell.

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

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

doorbird:
  host: 192.168.2.16
  username: ghaigb0001
  password: **********

binary_sensor:
  - platform: doorbird
    monitored_conditions:
      - doorbell

camera:
  - platform: doorbird
    last_visitor: true

switch:
  - platform: doorbird
    switches:
      - light_on
      - open_door

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.

About the older commits: I have tried everything and cannot seem to get rid of them. Help is welcome.

…kini/home-assistant/commit/39c6484d89337240918ffe60c35705b5b3405705)

…oorbird

mention-bot · 2017-08-06T18:29:34Z

@Klikini, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @happyleavesaoc to be potential reviewers.

fabaff

Please remove the commit for homeassistant/components/frontend/www_static/home-assistant-polymer.

fabaff · 2017-08-09T10:21:14Z

+}
+
+PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
+    vol.Required(CONF_MONITORED_CONDITIONS, default=[]):


If only doorbell is available then this should be the default and the configuration by the user omitted. According to the DoorBird docs there is an additional connector available. That one could be the second type.

I saw that in the docs, but when I query my device, it will only return the doorbell status. I assume the docs are outdated. I'll just make it the default here.

fabaff · 2017-08-09T10:29:50Z

+        return False
+    else:
+        _LOGGER.error("Could not connect to DoorBird at %s as %s: Error %s",
+                      ip, username, str(status[1]))


I would like to suggest to not send the username to the log file as for sharing the log file it would require to be sanitized and the chances are high that it won't happen.

fabaff · 2017-08-09T10:29:52Z

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


Please use hass.data[DATA_YOUR_DOMAIN] instead hass.data[DOMAIN].

I'm not sure what this means. Do I define a new constant called DATA_DOORBIRD and use that here? But what would its value be? I'm using the Ring component as an example, and this is what is used there.

fabaff · 2017-08-09T10:34:40Z

+}
+
+PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
+    vol.Required(CONF_SWITCHES, default=[]):


Why not make switches optional with open_door as default? This seems to be one of the core features of DoorBird. Or is this just one of the two built-in relays?

If one was to be the default, I'd say light_on would be a better choice. It turns on the IR array for night vision, which all DoorBird devices include. The open_door switch is only useful if you have an electronic door strike installed.

fabaff · 2017-08-09T10:39:55Z

+from homeassistant.components.doorbird import DOMAIN
+import homeassistant.helpers.config_validation as cv
+from homeassistant.const import CONF_MONITORED_CONDITIONS, STATE_UNKNOWN
+from homeassistant.util import Throttle


Please order the import.

fabaff · 2017-08-09T10:44:02Z

+    """A binary sensor of a DoorBird device."""
+    def __init__(self, device, sensor_type):
+        """Initialize a binary sensor on a DoorBird device."""
+        if sensor_type not in SENSOR_TYPES:


Already covered by the configuration validation.

fabaff · 2017-08-09T10:48:30Z

+
+def setup_platform(hass, config, add_devices, discovery_info=None):
+    """Set up the DoorBird binary sensor component."""
+    device = hass.data.get(DOMAIN)


See comment on the component about this.

fabaff · 2017-08-09T10:50:54Z

+
+    @property
+    def name(self):
+        """:returns: The name of the sensor."""


Please follow PEP257 for docstrings.

fabaff · 2017-08-09T10:51:22Z

+    def icon(self):
+        """:returns: An icon to display."""
+        icon = SENSOR_TYPES[self._sensor_type]["icon"][self._state]
+        return "mdi:" + str(icon)


Please use string formatting.

fabaff · 2017-08-09T10:55:07Z

+CONF_SHOW_LAST_VISITOR = 'last_visitor'
+
+PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
+    vol.Required(CONF_SHOW_LAST_VISITOR, default=False): cv.boolean


Should be vol.Optional if you want to set a default. Is "optional" according to your docs.

houndci-bot · 2017-08-09T14:22:09Z

+
+from homeassistant.components.binary_sensor import BinarySensorDevice
+from homeassistant.components.doorbird import DOMAIN
+from homeassistant.const import  STATE_UNKNOWN


multiple spaces after keyword

pvizeli · 2017-08-18T13:44:14Z

We do you not use our lock component?

Klikini · 2017-08-18T13:49:23Z

DoorBird is a doorbell, not a door lock.

…

On Aug 18, 2017 8:44 AM, "Pascal Vizeli" ***@***.***> wrote: We do you not use our lock component? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8871 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABnZCIibtmuogVzRoFbN3lbCcf_jTmCHks5sZZU4gaJpZM4Ouzd1> .

cveenman · 2017-08-30T08:19:47Z

Andy, What's holding up merge of your pull request?

Klikini · 2017-08-30T12:37:39Z

@cveenman: I don't know. @fabaff

pvizeli · 2017-08-30T13:21:07Z

You need cleanup your PR and remove changes on home-assistant-polymer

Klikini · 2017-09-02T23:45:34Z

@pvizeli I'm not sure how to fix the submodule issue. I didn't make any changes to it, so I'm not sure why it always gets pushed. I tried checking out 1ea7137c78b3df577f5df72a6c9bbe91bb5f7cd0 (the diff of the submodule between this branch and home-assistant/dev is home-assistant/frontend@1ea7137...1ad4259 and it is currently checkout out as 1ad42592134c290119879e8f8505ef5736a3071e) but it creates a new hash instead.

As for cleaning up the RachioPy commits, I said I had tried and failed to do that.

Am I missing something in the docs, or does Git just hate me lately?

pvizeli · 2017-09-03T13:00:19Z

Make a new PR and copy you files into this new PR. That is the fast way to fix that.

Klikini · 2017-09-03T15:31:27Z

#9281

Andy Castille and others added 12 commits July 2, 2017 00:16

use upstream RachioPy, fix manual run switches

39c6484

Update requirements for PyPi version of rachiopy

1d8575b

Use upstream RachioPy 0.1.2 (partial revert of https://github.com/Kli…

d7979ef

…kini/home-assistant/commit/39c6484d89337240918ffe60c35705b5b3405705)

Revert rachiopy downgrade

206904d

DoorBird base component

96313d2

DoorBird cameras

44b1969

DoorBird binary sensors

a7e70f1

Sanity check

648927e

DoorBird switches

4c0baf7

Add DoorBird to .coveragerc

b141ab1

Add doorbirdpy to requirements_all.txt

14b7fa0

Merge remote-tracking branch 'refs/remotes/home-assistant/dev' into d…

a34a9b2

…oorbird

homeassistant added new-platform platform: binary_sensor.doorbird integration: doorbird cla-signed labels Aug 6, 2017

Klikini mentioned this pull request Aug 6, 2017

DoorBird home-assistant/home-assistant.io#3147

Merged

Klikini changed the title ~~Doorbird~~ DoorBird Aug 6, 2017

homeassistant added the cla-signed label Aug 6, 2017

balloobbot added component platform requirement labels Aug 6, 2017

Add more docstrings

54483fb

fabaff requested changes Aug 9, 2017

View reviewed changes

Andy Castille added 2 commits August 9, 2017 09:21

Only the doorbell sensor works

3bafed3

Make last_visitor optional

a70ac13

homeassistant added the cla-signed label Aug 9, 2017

houndci-bot reviewed Aug 9, 2017

View reviewed changes

Andy Castille added 3 commits August 9, 2017 09:22

Use PEP257 for docstrings

5d381a3

Don't log the username

e952b15

remove extra space after import

98e1fdb

Klikini closed this Sep 3, 2017

home-assistant locked and limited conversation to collaborators Dec 11, 2017

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

Uh oh!

Conversation

Klikini commented Aug 6, 2017 • 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!

mention-bot commented Aug 6, 2017

Uh oh!

fabaff left a comment

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!

pvizeli commented Aug 18, 2017

Uh oh!

Klikini commented Aug 18, 2017 via email

Uh oh!

cveenman commented Aug 30, 2017

Uh oh!

Klikini commented Aug 30, 2017

Uh oh!

pvizeli commented Aug 30, 2017

Uh oh!

Klikini commented Sep 2, 2017

Uh oh!

pvizeli commented Sep 3, 2017

Uh oh!

Klikini commented Sep 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Klikini commented Aug 6, 2017 •

edited

Loading

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