Update to DoorBirdPy 2.0.2 by Klikini · Pull Request #14800 · home-assistant/core

Klikini · 2018-06-03T20:40:18Z

Description:

Update to DoorBirdBy 2.0.2, which uses the new version (0.21) of the DoorBird LAN API.

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

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 or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

Klikini · 2018-06-03T21:17:43Z

I have tested all of the code locally except the event bus because I can't open the logbook or settings tabs in the frontend. Every tab except home is just a blank page and Chrome logs GET http://127.0.0.1:8123/undefined 404 (Not Found) to the console. Is this a known issue with the dev branch or did I screw something up with my test instance?

MartinHjelmare · 2018-06-05T08:07:32Z

    device_ip = config[DOMAIN].get(CONF_HOST)
    username = config[DOMAIN].get(CONF_USERNAME)
    password = config[DOMAIN].get(CONF_PASSWORD)
+    doorbell_num = config[DOMAIN].get(CONF_DOORBELL_NUM, 1)


Move the default to the config schema.

MartinHjelmare · 2018-06-05T08:13:04Z

+
+        # Register doorbell push schedule if enabled
+        if config[DOMAIN].get(CONF_DOORBELL_EVENTS):
+            doorbell_num = config[DOMAIN].get(CONF_DOORBELL_NUM, 1)


This is already assigned.

MartinHjelmare · 2018-06-05T08:15:36Z

+    hass_url = hass.config.api.base_url
+
+    # Override it if another is specified in the component configuration
+    if config[DOMAIN].get(CONF_CUSTOM_URL):


Cache the config for the domain in conf, since it's accessed so frequently.

MartinHjelmare · 2018-06-05T08:18:57Z

-        url = '{}{}/{}'.format(hass_url, API_URL, SENSOR_DOORBELL)
-        device.reset_notifications()
-        device.subscribe_notification(SENSOR_DOORBELL, url)
+    # Deregister doorbell pushes if not enabled


Shouldn't this be done at each home assistant stop instead?

It's deregistering the notifications that aren't in the configuration (but were on a previous start), not the ones that it registered at startup.

Yes, but shouldn't we instead deregister everything at stop? Otherwise we need to start again to deregister.

Where are we registering the notifications?

When HA starts, it will register (with the DoorBird device itself) that it wants doorbell and/or motion notifications, depending on which are enabled in the HA config. It will also check to see if it there are old records (used to be in config but then got removed/commented out) for HA that it doesn't want anymore and remove them, otherwise putting a notification in your HA config is permanent even if you remove it later.

We could deregister at stop, but right now I just leave them there so they don't need to be re-registered next time HA restarts. That way there aren't any EEPROM writes on the device every time HA restarts, only when the config changes.

Sounds good!

MartinHjelmare · 2018-06-05T08:21:40Z

+    if 'http' not in favs:
+        return False
+
+    for fav in list(favs['http'].values()):


Just iterate the dict values directly. It's not needed to make a list of the values.

for fav in favs['http'].values():

MartinHjelmare · 2018-06-05T08:24:29Z

+
+    The favorite must exist or there will be problems.
+    """
+    for fav_id in list(favs['http'].keys()):


for fav in favs['http']:

MartinHjelmare · 2018-06-05T08:26:40Z

-        _LOGGER.debug("Adding DoorBird switch %s", SWITCHES[switch]["name"])
-        switches.append(DoorBirdSwitch(device, switch))
+    relays = device.info()['RELAYS']
+    relays.append("")  # IR lights


Why not call it something readable like 'ir_light'.

I used __ir_light__ because a device is more likely to actually be named ir_light if another one is present.

houndci-bot · 2018-06-05T14:01:39Z

-    username = config[DOMAIN].get(CONF_USERNAME)
-    password = config[DOMAIN].get(CONF_PASSWORD)
+    from doorbirdpy import DoorBird, DoorBirdScheduleEntrySchedule
+


blank line contains whitespace

syssi · 2018-06-09T06:07:42Z

@Klikini Thanks for your contribution! Are you aware of #13994? Could you support @oblogic7? No matter which PR gets merged first one of you has to port the changes unfortunately. :-(

Klikini · 2018-06-09T13:45:10Z

I think their changes are more involved, so their PR should be merged first and then I will manually reapply my changes in a new PR to the new code handling each device.

oblogic7 · 2018-06-10T15:13:36Z

I deliberately did not make any updates to API calls as part of my PR. All of my devices are up-to-date on firmware and I did verify that the notification endpoint is still available. The documentation states that notifications are automatically converted to the new schedule format so the existing approach still works well. The API calls should be updated in case the notifications endpoint is deprecated in a future release.

Klikini · 2018-06-10T15:16:56Z

@oblogic7 Yes, which is why your PR should merge first. I can reapply my changes to your new code easily once it merges because most of my changes were in the DoorBirdPy library itself.

syssi · 2018-06-10T17:09:42Z

Please rebase and re-apply your changes! :-)

Klikini · 2018-06-11T21:38:58Z

Moved to #14933

homeassistant added cla-signed integration: doorbird platform: switch.doorbird labels Jun 3, 2018

Klikini mentioned this pull request Jun 3, 2018

Update for new DoorBird API home-assistant/home-assistant.io#5487

Closed

2 tasks

homeassistant added the cla-signed label Jun 3, 2018

MartinHjelmare reviewed Jun 5, 2018

View reviewed changes

homeassistant added the cla-signed label Jun 5, 2018

houndci-bot reviewed Jun 5, 2018

View reviewed changes

homeassistant added the cla-signed label Jun 5, 2018

Klikini closed this Jun 11, 2018

homeassistant added the cla-signed label Jun 11, 2018

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

ghost removed the platform: switch.doorbird label Mar 21, 2019

Uh oh!

Conversation

Klikini commented Jun 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

Klikini commented Jun 3, 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!

MartinHjelmare Jun 5, 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!

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 Jun 9, 2018

Uh oh!

Klikini commented Jun 9, 2018

Uh oh!

oblogic7 commented Jun 10, 2018

Uh oh!

Klikini commented Jun 10, 2018

Uh oh!

syssi commented Jun 10, 2018

Uh oh!

Klikini commented Jun 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Klikini commented Jun 3, 2018 •

edited

Loading

MartinHjelmare Jun 5, 2018 •

edited

Loading