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 security systems controlled by IFTTT #12975

Merged
merged 15 commits into from
Mar 18, 2018
Merged

Support for security systems controlled by IFTTT #12975

merged 15 commits into from
Mar 18, 2018

Conversation

maxclaey
Copy link
Contributor

@maxclaey maxclaey commented Mar 7, 2018

Description:

Added support for security systems that have no open API but can be controlled via IFTTT (e.g. piper, ismartalarm, ...). To set the security system to a specific state, a webhook call is made to change the device state. Furthermore, applets have to be made to call home assistant's REST API when the device's state has changed.

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

Example entry for configuration.yaml (if applicable):

ifttt:
  key: !secret webhook_key

alarm_control_panel:
  - platform: ifttt
    name: "IFTTT Alarm"
    code: !secret alarm_code

Checklist:

  • The code change is tested and works locally.

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][ex-requir]).
  • New dependencies are only imported inside functions that use them ([example][ex-import]).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

def alarm_arm_night(self, code=None):
"""Send arm night command."""
if not self._check_code(code)
return

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unexpected indentation

def alarm_arm_home(self, code=None):
"""Send arm home command."""
if not self._check_code(code)
return

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unexpected indentation

def alarm_arm_away(self, code=None):
"""Send arm away command."""
if not self._check_code(code)
return

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unexpected indentation

def alarm_disarm(self, code=None):
"""Send disarm command."""
if not self._check_code(code)
return

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unexpected indentation


def alarm_disarm(self, code=None):
"""Send disarm command."""
if not self._check_code(code)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SyntaxError: invalid syntax

@maxclaey
Copy link
Contributor Author

maxclaey commented Mar 8, 2018

I will add a documentation page on home-assistant.github.io to explain the required ifttt webhook setup when the pull request gets approved. Thanks in advance for reviewing!

@c727
Copy link
Contributor

c727 commented Mar 8, 2018

If this gets merged there should be a warning in the docs that this shows an assumed state and not the real device state

@maxclaey
Copy link
Contributor Author

maxclaey commented Mar 8, 2018

Yes, for sure. But that's unavoidable if IFTTT is the only way to interact with the device off course :)

@arsaboo
Copy link
Contributor

arsaboo commented Mar 8, 2018

Super excited about this one. For a future PR, we should also think about including other platforms. For example, we could use binary sensors for door/window status.

CONF_NAME, CONF_CODE)
import homeassistant.helpers.config_validation as cv

REQUIREMENTS = ['requests==2.18.4', 'pyfttt==0.3']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not add requests. It's already a core dependency of Home Assistant. Also move the requests import to the top of the file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, thinking of it. This platform should depend on the ifttt component instead of asking for the key itself. The ifttt component should be added via DEPENDENCIES and should share the key via hass.data[DOMAIN]

Copy link
Contributor

@arsaboo arsaboo Mar 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ifttt component already requires the key. You can use DEPENDENCIES = ['ifttt'] to use the IFTTT component as a dependency and not require the user to enter the key again here. There are several components that do that, e.g., Arlo, Abode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestions! It made me realize I don't even need the key. When I have the ifttt component as a dependency, I can just call the component's trigger service

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly...that is the point 👍


try:
# Translate the state to a service/event name
event = STATE_TO_SERVICE[state]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either call it event or service, please don't mix 'n match.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, stick with event since the method to sent it to IFTTT is called send_event

DEFAULT_NAME = "Home"

STATE_TO_SERVICE = {
STATE_ALARM_ARMED_AWAY: SERVICE_ALARM_ARM_AWAY,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not use the service names from Home Assistant. There is no guarantee that they will remain the same. Instead I suggest you create your own constants (named could be the same)

CONF_WEBHOOK_KEY = "webhook_key"
DEFAULT_NAME = "Home"

STATE_TO_SERVICE = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have this constant instead of the methods like alarm_arm_night just pass in the right constant directly?

@balloob
Copy link
Member

balloob commented Mar 10, 2018

Please add the assumed_state property and have it return True

code = config.get(CONF_CODE)

alarmpanel = IFTTTAlarmPanel(name, webhook_key, code)
add_devices([alarmpanel], True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no update method so no need to add True. Please remove

@balloob
Copy link
Member

balloob commented Mar 10, 2018

Make sure to write extensive docs as to how to configure IFTTT.

@arsaboo
Copy link
Contributor

arsaboo commented Mar 11, 2018

Ok...so, I have been testing this for 24 hours and there is some issue. The alarm_control_panel goes right back to disarmed after changing the state. Basically, the alarm_control_panel does not reflect the current state. See the logbook entries:

image

(btw...I am not sure why the case of the entity changes in the logbook).

Also, not sure if it matters, but I have the regular Abode integration as well, which is entirely separate from the IFTTT Abode.

@maxclaey
Copy link
Contributor Author

Apparently, when you call the REST api to set the state of the entity, it is not reflected to the state property. Could someone tell me how to make sure that some kind of setter is called on the entity when the API is used to set the state?

@balloob
Copy link
Member

balloob commented Mar 11, 2018

That won't work. It's the opposite direction. States represent entities, not the other way around. Add a new rest endpoint if you want to interact with your entity

Interfaces with alarm control panels that have to be controlled through IFTTT.

For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/alarm_control_panel.ifttt_securitysystem/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The url should end with the platform name after the period, in this case that's ifttt.

https://home-assistant.io/components/alarm_control_panel.ifttt_securitysystem/
"""
import logging
import voluptuous as vol
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a blank line between standard library and 3rd party imports.

class IFTTTAlarmPanel(alarm.AlarmControlPanel):
"""Representation of an alarm control panel controlled throught IFTTT."""

def __init__(self, hass, name, code):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't pass in hass. It will be set on the entity when it has been added to home assistant.

self._name = name
self._code = code
# Set default state to disarmed
self._state = STATE_ALARM_DISARMED
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An unknown state should be initialized with None.

return
self.set_alarm_state(EVENT_ALARM_ARM_NIGHT, code)

def set_alarm_state(self, event, code):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code isn't used in the method.

"""Call the IFTTT trigger service to change the alarm state."""
data = {ATTR_EVENT: event}

self._hass.services.call(IFTTT_DOMAIN, SERVICE_TRIGGER, data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.hass.services.call(...)

@maxclaey
Copy link
Contributor Author

Thanks for the clarification about the state updates. I will try another approach tonight or tomorrow

@arsaboo
Copy link
Contributor

arsaboo commented Mar 14, 2018

Have been testing this for the last 24 hours and it is working perfectly 👍


def setup_platform(hass, config, add_devices, discovery_info=None):
"""Set up a control panel managed through IFTTT."""
if DATA_IFTT_ALARM not in hass.data:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You miss a T in IFTTT

schema=PUSH_ALARM_STATE_SERVICE_SCHEMA)


class IFTTTAlarmData:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for an extra class, you can just store the list directly in hass.data

vol.Optional(CONF_CODE): cv.string,
})

SERVICE_PUSH_ALARM_STATE = "push_alarm_state"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefix platform specific services with the platform name. So this should be ifttt_push_alarm_state

hass.data[DATA_IFTT_ALARM].devices.append(alarmpanel)
add_devices([alarmpanel])

def push_state_update(service):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this async by changing:

async def push_state_update(service):
    …
    for device in devices:
        …
        device.async_schedule_update_ha_state()

@@ -69,3 +69,13 @@ alarmdecoder_alarm_toggle_chime:
code:
description: A required code to toggle the alarm control panel chime with.
example: 1234

push_alarm_state:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be changed to ifttt_push_alarm_state

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, forgot about it when renaming the service. Updating it

@balloob
Copy link
Member

balloob commented Mar 15, 2018

🎉 great work! 🐬

@maxclaey
Copy link
Contributor Author

I think all changes requested by @MartinHjelmare are fixed as well

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@MartinHjelmare
Copy link
Member

Please update the config example in the description in this PR to reflect the latest changes. Then we can merge.

@balloob balloob merged commit 022d8fb into home-assistant:dev Mar 18, 2018
@maxclaey maxclaey deleted the ifttt_security_systems branch March 20, 2018 16:57
@balloob balloob mentioned this pull request Mar 30, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants