-
-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
Add Yale Smart Alarm component #16377
Add Yale Smart Alarm component #16377
Conversation
Hi @domwillcode, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
Yale Smart Alarm client for interacting with the Yale Smart Alarm System API. | ||
|
||
For more details about this platform, please refer to the documentation at | ||
https://github.com/domwillcode/yale-smart-alarm-client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace with a link to our docs. See other platforms for examples.
|
||
import asyncio | ||
import logging | ||
import voluptuous as vol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a blank line between standard library and 3rd party imports.
import logging | ||
import voluptuous as vol | ||
|
||
import homeassistant.components.alarm_control_panel as alarm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import AlarmControlPanel
instead and group the import with the line below.
from homeassistant.components.alarm_control_panel import PLATFORM_SCHEMA | ||
from homeassistant.const import ( | ||
CONF_PASSWORD, | ||
CONF_USERNAME, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't break line after each name.
|
||
CONF_AREA_ID = 'area_id' | ||
|
||
YALE_SMART_ALARM_DOMAIN = 'yale_smart_alarm' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this to YALE_SMART_ALARM_DATA
.
|
||
from yalesmartalarmclient.client import YaleSmartAlarmClient | ||
|
||
self._client = YaleSmartAlarmClient( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This client isn't async safe. Replace used coroutines with normal functions and use the home assistant sync api instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should login and check credentials before creating the entity. If credentials are bad, we should log an error in setup_platform
and return from that.
So move this out of the init method and pass the client in to the entity after login is successful instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did check/fail before creating the entity, but you're right, it is horrendously unclear. Do you have a preferred way of handling authentication failure? Client exception -> catch, log and return?
password, | ||
area_id) | ||
|
||
_LOGGER.debug("Yale Smart Alarm client created and authenticated") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No logging in init method.
elif status == YALE_STATE_ARM_FULL: | ||
state = STATE_ALARM_ARMED_AWAY | ||
else: | ||
state = STATE_UNKNOWN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use None
.
|
||
status = self._client.get_armed_status() | ||
|
||
if status == YALE_STATE_DISARM: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this if/else you can create a dictionary that maps the Yale states to the home assistant states and store that as an instance attribute in init. Then look up the home assistant state here.
|
||
self._state = state | ||
|
||
@asyncio.coroutine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use async api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some quick comments.
Also, domwillcode/yale-smart-alarm-client#1 needs to be addressed.
self._name = name | ||
self._username = username | ||
self._password = password | ||
self._state = STATE_UNKNOWN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use None
and let Home Assistant handle the unknown state.
config.get(CONF_NAME), | ||
config.get(CONF_USERNAME), | ||
config.get(CONF_PASSWORD), | ||
config.get(CONF_AREA_ID)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YaleSmartAlarmClient()
should be created during the setup of the platform and passed as an object to YaleAlarmDevice()
. If the credentials are wrong then the platform setup should fail. Otherwise the users end up with a non-working platform in their instance.
def async_setup_platform(hass, config, async_add_devices, discovery_info=None): | ||
"""Set up the alarm platform.""" | ||
if YALE_SMART_ALARM_DOMAIN not in hass.data: | ||
hass.data[YALE_SMART_ALARM_DOMAIN] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that needed?
Thanks for the feedback both, I'm new to Python/HA (and cobbled this together from other alarm components - copy paste is the devil) so your critique is appreciated. I'll make the changes shortly. |
…en tcreation and improved authentication failure error. Added state map to replace nasty if blocks.
…en tcreation and improved authentication failure error. Added state map to replace nasty if blocks.
Alright, PR updated with changes. I've added timeouts to the client package (thanks for that!), https://github.com/domwillcode/yale-smart-alarm-client/tree/v0.1.4. |
For more details about this platform, please refer to the documentation at | ||
https://www.home-assistant.io/components/alarm_control_panel.yale_smart_alarm | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this blank line.
def setup_platform(hass, config, add_devices, discovery_info=None): | ||
"""Set up the alarm platform.""" | ||
name = config.get(CONF_NAME) | ||
username = config.get(CONF_USERNAME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use dict.get
for required config keys, use dict[key]
.
}) | ||
|
||
|
||
def setup_platform(hass, config, add_devices, discovery_info=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename add_devices
to add_entities
.
"""Return the state of the device.""" | ||
armed_status = self._client.get_armed_status() | ||
|
||
self._state = self._state_map.get(armed_status, None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None
is already the default value returned if the key is missing in the dict.
"""Send arm away command.""" | ||
self._client.arm_full() | ||
|
||
def alarm_arm_night(self, code=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't implement this method if it has the same result as alarm_arm_home
.
try: | ||
client = YaleSmartAlarmClient(username, password, area_id) | ||
except AuthenticationError: | ||
_LOGGER.error("Authentication failed. Check credentials.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't end log messages with period.
Updated :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Are you happy too @fabaff?
@domwillcode is there a risk this API will be removed as it appears to be for a discontinued app..? |
It seems to be just the UI 'app' and the API remains functional, it's been that way for months. I don't know of an alternative API (I'll see if Yale will tell me any secrets), but I wouldn't be surprised if their Android and iOS apps use the same API. I'll see what I can dig up and monitor the situation/update the client if anything changes. |
Description:
Add a Yale Smart Alarm Component to control Yale Smart Alarm Hub through their API (currently only available for purchase in the UK).
Uses yalesmartalarmclient PyPi package (https://github.com/domwillcode/yale-smart-alarm-client) and Yale API to support 'arm_home', 'arm_away' and 'disarm'.
Documentation PR: home-assistant/home-assistant.io#6176
Example entry for
configuration.yaml
(if applicable):Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices: