Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overhaul of Blink platform #16942

Merged
merged 23 commits into from Oct 3, 2018

Conversation

Projects
None yet
6 participants
@fronzbot
Copy link
Contributor

commented Sep 28, 2018

Description:

This is a complete overhaul of the Blink component in homeassistant. The previous version relied on old API methods that Blink has since deprecated, causing the component to be mostly non-functional. This component still does not support multiple sync units (since the blinkpy API also doesn't) but both have be restructured in such a way to (hopefully) allow a more seamless add in the future.

I will list off breaking changes of the component, and then go into a bit more detail about what I changed, since this is a pretty big PR.

Breaking Changes

  • No more notification sensor, since the Blink API no longer supports it
    • motion can now be detected either with the blink sensor
      binary_sensor.blink_nameofcamera_motion_detected or through a camera's attributes with the motion_detected key.
  • blink.snap_picture service has been renamed to blink.trigger_camera
  • Camera names now prefaced with blink, ie. camera.blink_nameofcamera

New features

  • Alarm control panel added
  • Sensors and binary sensors can be added within the blink platform using monitored_conditions (se config example)
  • Added ability to change refresh rate via scan_interval
  • Added blink_update service to manually trigger a camera update
  • Added save_video service to save a camera's last recorded video to a local file

Example entry for configuration.yaml (if applicable):

blink:
    username: youremail@example.com
    password: somepassword
    scan_interval: 60               # OPTIONAL
    binary_sensors:
        monitored_conditions:
            - motion_enabled
            - motion_detected
    sensors:
        monitored_conditions:
             - battery
             - temperature
             - status
             - wifi_strength

Related issue (if applicable):
fixes #15665, fixes #17006

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

All platform changes

  • Added alarm_control_panel.blink
  • Do not perform http request upon displaying a camera image, instead rely on the blinkpy library to do that and return the saved data instead
  • Add ability to change refresh rate of blink sensor
    • Excessive polling will drain battery (and possibly result in you account being locked out from accessing the API), so be aware of this.
  • Rename snap_picture service to trigger_camera
  • Add blink_update service to force a refresh of blink information
  • Add save_video service to save the last recorded clip locally
    • Need to add whitelist_external_dirs to homeassistant for this to work properly
  • Notification sensor is now binary_sensor.blink_<camera>_motion_detected
  • New status sensor to report status of individual camera (armed, disarmed, or disabled)
  • New wifi strength sensor to report wifi strength of camera to sync module

Remaining issues/questions

  • I feel like I'm not properly utilizing async but don't know enough. Any advice here is greatly appreciated.
  • When the image is clicked on, I'd like to return the last recorded clip but have not figured out a way to do that. Blink does not have a streaming option that I know of, and also requires some custom headers to be sent with a request (containing an authorization token). Not sure how to approach this.
  • As mentioned before, multi-sync units are not supported, and since I only have one it's difficult for me to test. If someone else wants to contribute to this, I'd greatly appreciate it!

Checklist:

  • The code change is tested and works locally.
  • Documentation added/updated in home-assistant.io
  • New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
  • New dependencies are only imported inside functions that use them ([example][ex-import]).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

EDIT 10/1: Updated text to reflect most recent changes

@fronzbot

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

Ok, how do I get WIP to not set the status of this PR to in progress? It's grabbing info from my first commit , but I can't seems to figure out how to reset that status.

@frenck frenck added the docs-missing label Sep 30, 2018

@MartinHjelmare
Copy link
Member

left a comment

Load the platforms via discovery from component and pass config options via discovery_info to platforms, or store config in hass.data.


from homeassistant.components.alarm_control_panel import AlarmControlPanel
from homeassistant.components.blink import (
DOMAIN, DEFAULT_ATTRIBUTION)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Import the blink domain as another name. This platform belongs to the alarm_control_panel domain.

This comment has been minimized.

Copy link
@fronzbot

fronzbot Sep 30, 2018

Author Contributor

Would something like BLINK_DATA work?

This comment has been minimized.

Copy link
@MartinHjelmare
"""Return the state attributes."""
return {
ATTR_ATTRIBUTION: DEFAULT_ATTRIBUTION,
'device_id': 'not implemented yet'

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Remove this.


def alarm_arm_home(self, code=None):
"""Send arm command."""
self.sync.arm = True

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Don't add this method if it has the same result as arm away.

This comment has been minimized.

Copy link
@fronzbot

fronzbot Sep 30, 2018

Author Contributor

Ok, I wasn't really sure what to do with that. Only reason I added it was because I noticed that the alarm panel on the front end displays a button for both Arm Home and Arm Away, so wasn't sure if I needed to add functionality for both.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Yeah, we don't have supported features yet for the alarm control panel. So we can't do anything about that yet.

Show resolved Hide resolved homeassistant/components/blink.py
cameras = hass.data[DOMAIN].blink.cameras
name = call.data.get(ATTR_FRIENDLY_NAME, '')
from blinkpy import blinkpy
username = config[DOMAIN].get(CONF_USERNAME)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Use dict[key] for required config keys.

'battery':
['Battery', '%', 'mdi:battery-80'],
'motion_detected':
['Motion Detected', '', 'mdi:run-fast'],

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Why shouldn't this be a binary sensor platform?

This comment has been minimized.

Copy link
@fronzbot

fronzbot Sep 30, 2018

Author Contributor

Honestly, I'm just not sure which makes more sense and I really don't have a strong opinion either way. If you think binary_sensormakes more sense than a sensor platform, that's good enough for me.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

If it only has two states, it should be a binary sensor platform 👍.

return

data = hass.data[DOMAIN].blink
data = hass.data[DOMAIN]
devs = list()

This comment has been minimized.

Copy link
@MartinHjelmare

import homeassistant.helpers.config_validation as cv
from homeassistant.components.blink import (
DOMAIN, DEFAULT_BRAND, DEFAULT_ATTRIBUTION)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Import the blink domain as another name.

import requests

from homeassistant.components.blink import DOMAIN
from homeassistant.components.blink import DOMAIN, DEFAULT_BRAND

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Import the blink domain as another name.

"""Return the device state attributes."""
attr = {}
attr[ATTR_ATTRIBUTION] = DEFAULT_ATTRIBUTION
attr['brand'] = DEFAULT_BRAND

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Static info not related to state shouldn't be part of state attributes. We have device info property for that. Although that requires config entry.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 30, 2018

Rebase and update the commit message to remove the WIP tag.

This was referenced Oct 1, 2018

fronzbot added some commits Sep 23, 2018

Using new methods for blink camera
- Refactored blink platform (breaking change)
- Camera needs to be uniquely enabled in config from now on
- Added motion detection enable/disable to camera platform
Fix motion detection
- bumped blinkpy to 0.8.1
- Added wifi strength sensor
Added platform schema to sensor
- Added global variables for brand and attribution to main platform
Wait for valid camera response before returning image
- Motion detection now working!

fronzbot added some commits Sep 28, 2018

@fronzbot fronzbot force-pushed the fronzbot:blink-overhaul branch from a94255b to 35419be Oct 1, 2018

fronzbot added some commits Oct 1, 2018

Changed based on first review
- Pass blink as BLINK_DATA instead of DOMAIN
- Remove alarm_arm_home from alarm_control_panel
- Re-add discovery with schema for sensors/binar_sensors
- Change motion_detected to a binary_sensor
- Added camera_armed binary sensor
- Update camera device_state_attributes rather than state_attributes
@fronzbot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2018

OK, I think I fixed all of the requests. I also moved blink.py into a separate folder and added a services.yaml file to that folder in order to add some descriptions for the available platform services.

@fronzbot fronzbot referenced this pull request Oct 1, 2018

Merged

Update Blink component docs to reflect new changes #6427

2 of 2 tasks complete
"""Call save video service handler."""
result = await async_handle_save_video_service(hass, call)
if not result:
return False

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

Nothing is checking this return value.

SERVICE_SAVE_VIDEO = 'save_video'

BINARY_SENSORS = {
TYPE_CAMERA_ARMED: ('Camera Armed', 'mdi:verified'),

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

Please be consistent and either use tuples or lists in the sensor type dicts. Since they are constants they will not change.

return False
return True

hass.services.register(BLINK_DATA, SERVICE_REFRESH, blink_refresh)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

Services should be registered under a domain. Use the correct variable ie DOMAIN.

devs.append(BlinkSystemSensor(data))
devs = []
for sensor_type in discovery_info[CONF_MONITORED_CONDITIONS]:
for camera in data.sync.cameras:

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

Is this doing I/O? If so, we should optimize and put it in the outer loop.

This comment has been minimized.

Copy link
@fronzbot

fronzbot Oct 2, 2018

Author Contributor

It's not doing I/O, but I think it's more logical to put in the outer loop regardless.

fronzbot added some commits Oct 2, 2018

Register services with DOMAIN
- Change device add for loop order in binary_sensor

@frenck frenck removed the docs-missing label Oct 2, 2018

@frenck

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

Thanks for the docs PR @fronzbot!

SERVICE_TRIGGER,
trigger_camera,
schema=SERVICE_TRIGGER_SCHEMA)
hass.services.async_register(DOMAIN,

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

Use the sync version of the register method, not the async one. It doesn't matter that we're registering an async service handler. We're still in a sync context here.

@MartinHjelmare
Copy link
Member

left a comment

Great!

@MartinHjelmare MartinHjelmare merged commit c78850a into home-assistant:dev Oct 3, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 93.709%
Details

@wafflebot wafflebot bot removed the in progress label Oct 3, 2018

@fronzbot fronzbot deleted the fronzbot:blink-overhaul branch Oct 3, 2018

@balloob balloob referenced this pull request Oct 12, 2018

Merged

0.80.0 #17361

@ambgomes

This comment has been minimized.

Copy link

commented Oct 14, 2018

I have 3 Blinks. I can see entities for each but not able to update them, neither using blink.blink_update. Only able to get image from blink 1. Any help is welcome.

screenshot 2018-10-14 at 12 30 40

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Oct 14, 2018

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 or bug reports. Thanks!

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Oct 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.