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 Chime status and control to Alarm Decoder component #11271
Add Chime status and control to Alarm Decoder component #11271
Conversation
from homeassistant.components.alarmdecoder import ( | ||
DATA_AD, SIGNAL_PANEL_MESSAGE) | ||
from homeassistant.const import ( | ||
STATE_ALARM_ARMED_AWAY, STATE_ALARM_ARMED_HOME, STATE_ALARM_DISARMED, | ||
ATTR_CODE, STATE_ALARM_ARMED_AWAY, STATE_ALARM_ARMED_HOME, STATE_ALARM_DISARMED, |
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.
line too long (84 > 79 characters)
Can a reviewer please be assigned to this PR? |
As this PR is based on changes in #11155 we have to wait until that is merged or closed. |
👍 Thanks @goyney |
@MartinHjelmare #11155 has been closed in favor of this one. Ready for review for merge! |
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.
I've added some comments to old code. It would be great if you can fix those too.
|
||
_LOGGER = logging.getLogger(__name__) | ||
|
||
DEPENDENCIES = ['alarmdecoder'] | ||
|
||
SERVICE_ALARM_TOGGLE_CHIME = 'alarm_toggle_chime' |
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.
Prefix the string with the platform name, ie 'alarmdecoder_alarm_toggle_chime'
.
|
||
_LOGGER = logging.getLogger(__name__) | ||
|
||
DEPENDENCIES = ['alarmdecoder'] | ||
|
||
SERVICE_ALARM_TOGGLE_CHIME = 'alarm_toggle_chime' | ||
ALARM_TOGGLE_CHIME_SCHEMA = vol.Schema({ | ||
vol.Optional(ATTR_CODE): cv.string, |
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.
Change to Required
.
device = AlarmDecoderAlarmPanel() | ||
add_devices([device]) | ||
|
||
@callback |
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, as the method called will perform I/O.
|
||
hass.services.register( | ||
alarm.DOMAIN, SERVICE_ALARM_TOGGLE_CHIME, alarm_toggle_chime_handler, | ||
description=None, schema=ALARM_TOGGLE_CHIME_SCHEMA) |
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.
Please add a description.
|
||
hass.services.register( | ||
alarm.DOMAIN, SERVICE_ALARM_TOGGLE_CHIME, alarm_toggle_chime_handler, | ||
description=None, schema=ALARM_TOGGLE_CHIME_SCHEMA) | ||
|
||
return True |
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.
Old code, but please remove. This function shouldn't return anything.
@@ -42,22 +71,63 @@ def async_added_to_hass(self): | |||
SIGNAL_PANEL_MESSAGE, self._message_callback) | |||
|
|||
def _message_callback(self, message): | |||
do_update = False | |||
|
|||
if message.alarm_sounding or message.fire_alarm: | |||
if self._state != STATE_ALARM_TRIGGERED: |
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.
You can remove all these checks of the old state. They don't add any value. Also remove the check if an update should be scheduled. Just schedule the update. There will only be a state update if the state has changed anyway.
@@ -42,22 +71,63 @@ def async_added_to_hass(self): | |||
SIGNAL_PANEL_MESSAGE, self._message_callback) | |||
|
|||
def _message_callback(self, message): | |||
do_update = False |
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, see comment below.
def alarm_toggle_chime(self, code=None): | ||
"""Send toggle chime command.""" | ||
if code: | ||
_LOGGER.debug("alarm_toggle_chime: sending %s9", str(code)) |
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.
If this is the same code that disarms the alarm, we shouldn't log it.
@@ -96,3 +181,9 @@ def alarm_arm_home(self, code=None): | |||
if code: | |||
_LOGGER.debug("alarm_arm_home: sending %s3", str(code)) |
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.
Please remove the code from the log.
Please also link to a docs PR describing the new service and maybe the attributes. |
|
||
hass.services.register( | ||
alarm.DOMAIN, SERVICE_ALARM_TOGGLE_CHIME, alarm_toggle_chime_handler, | ||
descriptions.get(SERVICE_ALARM_TOGGLE_CHIME), schema=ALARM_TOGGLE_CHIME_SCHEMA) |
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.
line too long (87 > 79 characters)
return True | ||
descriptions = yield from hass.async_add_job( | ||
load_yaml_config_file, os.path.join( | ||
os.path.dirname(__file__), 'services.yaml')) |
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.
undefined name 'os'
|
||
return True | ||
descriptions = yield from hass.async_add_job( | ||
load_yaml_config_file, os.path.join( |
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.
undefined name 'os'
@@ -7,23 +7,45 @@ | |||
import asyncio | |||
import logging | |||
|
|||
import voluptuous as vol | |||
|
|||
from homeassistant.core import callback |
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.
'homeassistant.core.callback' imported but unused
@MartinHjelmare All requested changes have been addressed and I have linked to the PR for the documentation update in the original post. Here it is as well: home-assistant/home-assistant.io#4277 |
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.
Great!
Description:
Add the method
alarmdecoder_alarm_toggle_chime
in the Alarm Decoder component only. Doing so allows this service method to be called in the Home Assistant UI Services tab, the REST API, or the Websocket API with an alarm code that sends the command to toggle the alarm's chime function on or off.This PR also includes the changes from #11155 done by @hawk259, which exposes additional attributes on the Alarm Decoder alarm panel. The
chime
andready
attributes in particular are required to round out this feature.Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4277
Example entry for
configuration.yaml
(if applicable):With these changes, you can easily add a template switch to show status and control your chime.
Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices:
tox
run successfully. Your PR cannot be merged unless tests pass