Add sensor and camera support to SimpliSafe #40129

nzapponi · 2020-09-16T06:25:09Z

Proposed change

This PR adds support to SimpliSafe sensors, cameras and doorbells. Cameras appear not to work with HomeKit bridge, whereas everything else looks good!

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

I also need some support in writing out some tests. I noticed that the SimpliSafe component generally does not have many tests except for config_flow, so there are quite a few that need to be written out.

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

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

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

nzapponi · 2020-09-16T06:26:05Z

@bachya could you please take a look at the above?
Also, I also need some support in writing out some tests. I noticed that the SimpliSafe component generally does not have many tests except for config_flow, so there are quite a few that need to be written out... Could we work together on these?

probot-home-assistant · 2020-09-16T06:30:19Z

Hey there @bachya, mind taking a look at this pull request as its been labeled with an integration (simplisafe) you are listed as a codeowner for? Thanks!
_{^{(message by CodeOwnersMention)}}

bachya

Hi @nzapponi – thank you for your contribution.

I've left you some specific code review comments, but in general, this PR won't work in its current form. Multiple things going on:

First, because of the popularity of the request, a previous attempt to integrate SimpliSafe sensors was made in #30926. The primary comment to notice:

After running this for a bit, I’m not happy with the results. The extra hop introduced here - in which HASS talks to the SimpliSafe cloud, which talks to the base station - can be very laggy, especially during HASS startup. I don’t think that’s a particular good user experience, so I’m going to close this. Should SimpliSafe ever update their architecture to be more responsive to what we’d like to do, I will re-open.

Your PR uses the same polling mechanisms attempted there; since nothing has fundamentally changed, I'm not comfortable with the approach.

Second, this PR is much too large - you're attempting to add multiple platforms (sensors, cameras, etc.), all while modifying code that is already there. In general, PRs should focus on a single piece of functionality/change.

Lastly, you mention tests – in general, Home Assistant allows for ignoring device/service-specific code from test coverage (via .coveragerc); in the case of this integration, I focus my non-HASS testing efforts in simplisafe-python.

bachya · 2020-09-16T14:28:37Z

homeassistant/components/simplisafe/binary_sensor.py

+    doorbells = []
+    for system in simplisafe.systems.values():
+        if (
+            "cameras" in system._location_info["system"]


We should not be accessing a protected attribute here – this should be abstracted out in simplisafe-python.

bachya · 2020-09-16T14:30:41Z

homeassistant/components/simplisafe/binary_sensor.py

+    @callback
+    def async_update_from_rest_api(self):
+        """Update the entity with the provided REST API data."""
+        self._is_on = self._sensor.triggered


This is only going to show as triggered every 30 seconds (i.e., each time the REST API polls).

bachya · 2020-09-16T14:31:55Z

homeassistant/components/simplisafe/binary_sensor.py

+    def async_update_from_websocket_event(self, event):
+        """Update the entity with the provided websocket event data."""
+        if (
+            event.event_type == EVENT_MOTION_DETECTED


I understand that other integrations surface events like this as entities, but we already have a way to detect (and run automation on) these events: https://www.home-assistant.io/integrations/simplisafe/#events – what does your implementation provide beyond the event architecture?

This is to surface this as a visible binary sensor in the UI, like other motion sensors. I've taken the implementation from similar sensors already implemented in HA.

But why is that UI needed (especially since we're manually flipping the sensor 10 seconds later)? The events provide the same mechanism, but in a more flexible way (which can create notifications, etc.).

bachya · 2020-09-16T14:34:05Z

homeassistant/components/simplisafe/camera.py

+    @property
+    def _video_url(self):
+        """Provide the video URL."""
+        return '{} -i "https://media.simplisafe.com/v1/{}/flv?x=1280&audioEncoding=AAC"'.format(


This shouldn't be so explicit and should be abstracted away in simplisafe-python. In addition, please see this ongoing simplisafe-python issue re: camera support; the URL you use here isn't long-lived and has several other issues.

The URL is long lived, the auth token isn't. It works for the odd check of the camera. Yes, if you want to continuously stream for hours, then this is not a good implementation, but for the odd check for a few minutes it's absolutely fine.

That's good to know. Nevertheless, HASS shouldn't be aware of these implementation details; they belong in simplisafe-python.

bachya · 2020-09-16T14:36:00Z

homeassistant/components/simplisafe/lock.py

@@ -38,6 +38,11 @@ def __init__(self, simplisafe, system, lock):
        for event_type in (EVENT_LOCK_LOCKED, EVENT_LOCK_UNLOCKED):
            self.websocket_events_to_listen_for.append(event_type)

+    @property


Why? The SimpliSafeEntity class from which the SimpliSafeLock class inherits already has a unique ID defined.

Fair enough

bachya · 2020-09-16T14:37:26Z

homeassistant/components/simplisafe/__init__.py

@@ -517,7 +529,7 @@ async def async_update(self):

        async def update_system(system):
            """Update a system."""
-            await system.update()
+            await system.update(cached=False)


We can't do this – their API isn't structured to properly support that. See this attempt for more details: #30926

Why not? See my comment below - it runs fine on SimpliSafe 3.

I appreciate the thinking, but again, I don't see the value for door/window sensors that poll for state every 30 seconds; if someone opens/closes a door within that timeframe, HASS will never hear about it.

The use case isn't just triggering something when a door/window sensor opens, but it is also to check the current state of the sensor, e.g. check if there are any open windows before leaving the house. There's still value in this use case, and it doesn't require real time sensor data.
Also, we can poll every 15 seconds without any issues.

I hear you and can see the validity. Overall, I'm concerned that if tens/hundreds/more of HASS users start pinging SimpliSafe's cloud every 15 seconds in an un-cached way, we're going to get blocked off for good. That's definitely my own fear and not substantiated in any way. 😆

I'm definitely open to continuing the conversation, but at a minimum, I think the simplisafe-python work mentioned throughout needs to be looked at before this PR can proceed. Feel free to start looking around and let me know of any questions.

Alright, let me rework some of these changes into simplisafe-python. I'm aware they're meant to be in there but I wanted to start by getting something functional up and running first.

I'll close this PR and will re-open a few smaller ones once simplisafe-python is updated.

bachya · 2020-09-16T14:38:46Z

homeassistant/components/simplisafe/binary_sensor.py

+        ):
+            for cam in system._location_info["system"]["cameras"]:
+                cameras.append(cam)
+                if cam["model"] == "SS002":


What is this? Seems brittle to have it hardcoded here.

This is how doorbells are differentiated vs standard cameras.

HASS shouldn't be aware of such details. This is better suited for simplisafe-python.

nzapponi · 2020-09-16T16:32:38Z

Hi @bachya ,
I have created this same integration for Homebridge (https://github.com/nzapponi/homebridge-simplisafe3) and, aside from the odd hiccup once a month or so, it actually works really well. I've been running it for over a year with very little issues.
Yes, there is a slight delay in getting updates from sensors, but nothing different than other cloud polling integrations...

As a SimpliSafe customer, I'd rather get something in Home Assistant, even if not perfect, than nothing at all and be forced to put two sensors on each door.

We can caveat all this in user documentation, but honestly I don't see why we shouldn't be providing some basic functionality here.

I agree with you re cameras - we should be moving some of that logic into simplisafe-python.

nzapponi added 4 commits September 10, 2020 10:32

Add support for binary sensors

e0fbbac

Add support for freeze sensor

79a5cea

Add support for camera motion sensor and doorbell ring

a28c821

Add camera support

7bca377

homeassistant added the cla-signed label Sep 16, 2020

probot-home-assistant bot added integration: simplisafe new-feature new-platform labels Sep 16, 2020

probot-home-assistant bot assigned bachya Sep 16, 2020

bachya reviewed Sep 16, 2020

View reviewed changes

bachya removed their assignment Sep 27, 2020

nzapponi closed this Sep 28, 2020

This was referenced Sep 30, 2020

Added support to expose camera and doorbell data bachya/simplisafe-python#182

Merged

Add support for SimpliSafe sensors #41080

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sensor and camera support to SimpliSafe #40129

Add sensor and camera support to SimpliSafe #40129

nzapponi commented Sep 16, 2020

nzapponi commented Sep 16, 2020

probot-home-assistant bot commented Sep 16, 2020

bachya left a comment •

edited

Loading

bachya Sep 16, 2020 •

edited

Loading

bachya Sep 16, 2020

bachya Sep 16, 2020

nzapponi Sep 16, 2020

bachya Sep 16, 2020

bachya Sep 16, 2020 •

edited

Loading

nzapponi Sep 16, 2020

bachya Sep 16, 2020

bachya Sep 16, 2020

nzapponi Sep 16, 2020

bachya Sep 16, 2020

nzapponi Sep 16, 2020

bachya Sep 16, 2020

nzapponi Sep 17, 2020

bachya Sep 17, 2020

nzapponi Sep 18, 2020

bachya Sep 16, 2020

nzapponi Sep 16, 2020

bachya Sep 16, 2020

nzapponi commented Sep 16, 2020

Add sensor and camera support to SimpliSafe #40129

Add sensor and camera support to SimpliSafe #40129

Conversation

nzapponi commented Sep 16, 2020

Proposed change

Type of change

Additional information

Checklist

nzapponi commented Sep 16, 2020

probot-home-assistant bot commented Sep 16, 2020

bachya left a comment • edited Loading

Choose a reason for hiding this comment

bachya Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachya Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzapponi commented Sep 16, 2020

bachya left a comment •

edited

Loading

bachya Sep 16, 2020 •

edited

Loading

bachya Sep 16, 2020 •

edited

Loading