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.

Sign up for GitHub

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

Jump to bottom

Add support for SimpliSafe sensors #41080

Merged
merged 11 commits into from Oct 12, 2020
Merged

Add support for SimpliSafe sensors #41080

merged 11 commits into from Oct 12, 2020

Conversation

nzapponi
Copy link
Contributor

@nzapponi nzapponi commented Oct 2, 2020 •
edited

Proposed change

I took a simpler approach to adding SimpliSafe functionality and only implemented the sensors here (no camera).
Although the refresh rate is still not great (it's set to 30 seconds), the use case here is to show the state of sensors rather than having instant automations running when, e.g., a door opens.

I know it's not an ideal use of contact sensors, but we should at least give SimpliSafe users the option of seeing their sensors in here.

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

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:

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:

@project-bot project-bot bot added this to Needs review in Dev Oct 2, 2020
@nzapponi
Copy link
Contributor Author

nzapponi commented Oct 2, 2020

FYI @bachya

Dev automation moved this from Needs review to Review in progress Oct 2, 2020
Kane610
Kane610 reviewed Oct 2, 2020
View reviewed changes
Copy link
Member

@Kane610 Kane610 left a comment

Choose a reason for hiding this comment

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

Minor comments

homeassistant/components/simplisafe/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/simplisafe/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/simplisafe/config_flow.py Outdated Show resolved Hide resolved
bachya
bachya reviewed Oct 8, 2020
View reviewed changes
homeassistant/components/simplisafe/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/simplisafe/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/simplisafe/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/simplisafe/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/simplisafe/sensor.py Outdated Show resolved Hide resolved
MartinHjelmare
MartinHjelmare approved these changes Oct 12, 2020
View reviewed changes
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.

Code owner should approve before merge.

@bachya
Copy link
Contributor

bachya commented Oct 12, 2020

I ran this PR this morning and noticed that sensors are not updating properly – for instance, I had a door open for far longer than the 30-second update interval and the corresponding binary sensor never changed state. @nzapponi Have you functionally tested this?

@nzapponi
Copy link
Contributor Author

I ran this PR this morning and noticed that sensors are not updating properly – for instance, I had a door open for far longer than the 30-second update interval and the corresponding binary sensor never changed state. @nzapponi Have you functionally tested this?

Hi @bachya , yes I did test it and has been working properly for me. Let me test it for a few more days and see if I run into any other issues you're seeing. Have you had any errors show up in the log?

@nzapponi
Copy link
Contributor Author

Oh wow, can't believe I missed this as I rewrote the earlier PR, so sorry!
The plugin was still using cached sensor data. Should be working now. @bachya would you mind testing it out?

bachya
bachya approved these changes Oct 12, 2020
View reviewed changes
Copy link
Contributor

@bachya bachya left a comment

Choose a reason for hiding this comment

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

The latest is tested and confirmed. Thanks!

Dev automation moved this from Review in progress to Reviewer approved Oct 12, 2020
@bachya bachya merged commit 8a45bc2 into home-assistant:dev Oct 12, 2020
Dev automation moved this from Reviewer approved to Done Oct 12, 2020
@bachya
Copy link
Contributor

bachya commented Oct 12, 2020

@nzapponi Went ahead and merged this. Please open up a documentation PR, as well.

@nzapponi nzapponi mentioned this pull request Oct 12, 2020
Merged
8 tasks
weissm pushed a commit to weissm/core that referenced this pull request Oct 16, 2020
@nzapponi @weissm
Add support for SimpliSafe sensors (home-assistant#41080)
6df48f3 
* Add support for SimpliSafe sensors

* Turn sensor refresh rate to a configurable setting

* Set minimum to scan interval

* Removed dynamic sensor refresh rate

* Refactoring

* Refactoring

* Move battery entities to binary_sensor platform

* Bug fix

* Clean up

* Simplified device info override

* Ignore sensor cache
@ChristianNewton
Copy link

Hi, I see you have merged this. The user needs a way to disable this, because on gen 2 SimpliSafe, it will keep constantly saying your Settings Have been synchronized, and I have tested this in the recent Beta release. Thanks In Advance, Christian

@frenck
Copy link
Member

frenck commented Oct 22, 2020

Hi there @Kickingbird3,

Instead of commenting on closed/merged/handled PRs, please raise an issue instead.

Thanks 👍

@home-assistant home-assistant locked and limited conversation to collaborators Oct 22, 2020
@nzapponi nzapponi deleted the simplisafe-sensors branch October 22, 2020 07:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bdraco bdraco bdraco left review comments

@Kane610 Kane610 Kane610 left review comments

@bachya bachya bachya approved these changes

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

Assignees

@bachya bachya

Labels
cla-signed docs-missing Hacktoberfest integration: simplisafe new-feature new-platform
Projects
Dev
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@nzapponi @bachya @ChristianNewton @frenck @bdraco @MartinHjelmare @Kane610 @homeassistant