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

Support for August doorbell #11124

Merged
merged 10 commits into from Feb 18, 2018

Conversation

Projects
None yet
5 participants
@snjoetw
Copy link
Contributor

commented Dec 13, 2017

Description:

Added support for August Smart Lock and Doorbell. This PR adds support to lock/unlock August Smart Lock as well as be able to see the lock status, batter firmware and serial number of the lock.

For doorbell, this PR adds 2 3 binary sensors, ding, motion and online. Ding sensor indicates if someone ringed the doorbell, motion indicates if there's motion detected by the doorbell and online sensor indicates if the doorbell is connected to the internet or not. There's also a camera component added that shows the recent image from the doorbell that's triggered by motion.

Initial authentication will involve code verification that's sent to phone or email depending on the login_method. Verification code is sent the first time configurator is run and user will need to enter the code in the configuration panel to verify.

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

Example entry for configuration.yaml (if applicable):

august:                                                                                                                                                                                                                                                                                                                  
  login_method: phone
  username: "+YOUR_PHONE_NUMBER"
  password: YOUR_PASSWORD

Checklist:

If user exposed functionality or configuration variables are added/changed:

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.
vol.Required(CONF_USERNAME): cv.string,
vol.Required(CONF_PASSWORD): cv.string,
vol.Optional(CONF_INSTALL_ID): cv.string,
vol.Optional(CONF_TIMEOUT, default=DEFAULT_TIMEOUT): cv.positive_int,

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 1, 2018

Member

I don't think this should be configurable.

NOTIFICATION_ID = "august_notification"
NOTIFICATION_TITLE = "August Setup"

AUGUST_CONFIG_FILE = "august.conf"

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 1, 2018

Member

If the user is not supposed to edit the file it should be hidden, ie ".august.conf".

ACTIVITY_FETCH_LIMIT = 10
ACTIVITY_INITIAL_FETCH_LIMIT = 20

CONF_LOGIN_METHOD = "login_method"

This comment has been minimized.

Copy link
@MartinHjelmare

CONFIG_SCHEMA = vol.Schema({
DOMAIN: vol.Schema({
vol.Required(CONF_LOGIN_METHOD): cv.string,

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 1, 2018

Member

Is it not known what these login methods might be? If it is known, you should validate that one of them is used.

configurator.notify_errors(_CONFIGURING[DOMAIN],
"Invalid verification code")
elif result == ValidationResult.VALIDATED:
return setup_august(hass, config, api, authenticator)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 1, 2018

Member

Nothing is checking the return value of the configuration callback, so just call setup_august here.


CONFIG_SCHEMA = vol.Schema({
DOMAIN: vol.Schema({
vol.Optional(CONF_SCAN_INTERVAL, default=SCAN_INTERVAL): cv.time_period

This comment has been minimized.

Copy link
@MartinHjelmare
def device_state_attributes(self):
"""Return the device specific state attributes."""
return {
ATTR_BATTERY_LEVEL: "{}%".format(self._lock_detail.battery_level),

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 1, 2018

Member

Don't include the percent unit with the value. The battery level attribute should always be reported as a percent 0-100.

"""Return the device specific state attributes."""
return {
ATTR_BATTERY_LEVEL: "{}%".format(self._lock_detail.battery_level),
ATTR_SERIAL_NUMBER: self._lock_detail.serial_number,

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 1, 2018

Member

Remove this. It's not allowed to be part of state attributes.

return {
ATTR_BATTERY_LEVEL: "{}%".format(self._lock_detail.battery_level),
ATTR_SERIAL_NUMBER: self._lock_detail.serial_number,
ATTR_FIRMWARE_VERSION: self._lock_detail.firmware_version,

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 1, 2018

Member

Remove this. It's not allowed to be part of state attributes.

ATTR_BATTERY_LEVEL: "{}%".format(self._lock_detail.battery_level),
ATTR_SERIAL_NUMBER: self._lock_detail.serial_number,
ATTR_FIRMWARE_VERSION: self._lock_detail.firmware_version,
ATTR_CHANGED_BY: self._changed_by

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 1, 2018

Member

Hmm, the base lock entity class seems to do a bad thing in the state_attributes property. It's only allowing state attributes to be reported if the code_format property returns something. That's not good. I think we should fix this. It should return all attributes that are not None.

PROP_TO_ATTR = {
    'changed_by': ATTR_CHANGED_BY,
    'code_format': ATTR_CODE_FORMAT,
}
...
@property
def state_attributes(self):
    """Return the state attributes."""
    data = {}
    for prop, attr in PROP_TO_ATTR.items():
        value = getattr(self, prop)
        if value is not None:
            data[attr] = value
    return data

After that fix, you don't need to report ATTR_CHANGED_BY again here.

Can you open a separate PR with that fix? After that is merged you can rebase on latest dev.

This comment has been minimized.

Copy link
@snjoetw

snjoetw Jan 30, 2018

Author Contributor

Here's the PR for that: #12049

@MartinHjelmare
Copy link
Member

left a comment

Either add tests or add the modules to .coveragerc.

@balloob

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

@snjoetw, plans on addressing the comments?

@snjoetw

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2018

Yep but I'll need more time :)

@snjoetw

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2018

@MartinHjelmare @balloob I no longer own an August locks, I only have doorbell now. But this PR has both lock and doorbell support. I'm wondering if I should close this PR and submit another one just for the doorbell? or keep the lock support? The problem with keeping lock support is that I won't be able to fix any bugs if there's any problem as I don't have actual lock to test.

@balloob

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

Let's keep it in. It's easier for someone to stop by and fix bugs than add their own.

from datetime import timedelta

from homeassistant.components.august import DATA_AUGUST
from homeassistant.components.lock import LockDevice, ATTR_CHANGED_BY

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 30, 2018

'homeassistant.components.lock.ATTR_CHANGED_BY' imported but unused

snjoetw added some commits Jan 30, 2018

@MartinHjelmare
Copy link
Member

left a comment

Looks great! Just a small docstring update that's needed.



class AugustLock(LockDevice):
"""Representation of an Abode lock."""

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 5, 2018

Member

Stale docstring.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

Oh yeah, I forgot about coverage. Please update .coveragerc with the modules.

@snjoetw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2018

Just updated .coveragerc, I'll try to add unit tests later

@snjoetw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2018

I'll start working on the doc

@MartinHjelmare
Copy link
Member

left a comment

Great!

Can be merged as soon as a docs PR is linked.

@balloob

This comment has been minimized.

Copy link
Member

commented Feb 13, 2018

@snjoetw can you open a PR to add docs?

@snjoetw snjoetw referenced this pull request Feb 18, 2018

Merged

Added documentation for august component #4696

0 of 2 tasks complete
@snjoetw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2018

@MartinHjelmare @balloob sorry for the delay, docs added

@MartinHjelmare MartinHjelmare merged commit a8444b2 into home-assistant:dev Feb 18, 2018

5 checks passed

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 increased (+0.1%) to 93.964%
Details
hound No violations found. Woof!
@balloob

This comment has been minimized.

Copy link
Member

commented Feb 18, 2018

Awesome. Great work 🐬🐳

dmulcahey added a commit to dmulcahey/home-assistant that referenced this pull request Feb 19, 2018

Support for August doorbell (home-assistant#11124)
* Add support for August doorbell

* Address PR comment for August platform

* Address PR comment for August binary sensor

* Address PR comment for August camera

* Addressed PR comment for August lock

* - Fixed houndci-bot error

* - Updated configurator description

* - Fixed stale docstring

* Added august module to .coveragerc

@balloob balloob referenced this pull request Feb 22, 2018

Merged

0.64.0 #12609

@home-assistant home-assistant locked and limited conversation to collaborators May 29, 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.