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

Add Risco integration #36930

Merged
merged 11 commits into from
Aug 22, 2020
Merged

Add Risco integration #36930

merged 11 commits into from
Aug 22, 2020

Conversation

OnFreund
Copy link
Contributor

@OnFreund OnFreund commented Jun 19, 2020

Proposed change

A new integration for Risco alarm systems, through the Risco Cloud platform.
This is the first step, with a config flow and an alarm control panel for every partition.
Future PRs will add:

  • Group arming support, and user-configurable mapping between groups and the various arm states.
  • Optionally require pin code to arm/disarm
  • Fetching data through DataUpdateCoordinator, in preparation for:
  • Binary sensors for the zones

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

@skank01
Copy link

skank01 commented Jun 21, 2020

Ohhh i would love to see such an integration.
Risco group user here

@pergolafabio
Copy link

Nice indeed, allthough official api is coming soon, really like this one

@pergolafabio
Copy link

Btw , is your based on riscocloud or are you retrieving based on the risco app? Risco app is better

@OnFreund
Copy link
Contributor Author

OnFreund commented Jun 22, 2020

@pergolafabio this is based on the app API. As for official API coming soon, I wouldn't count on it, but if it ever gets released, I can update the underlying library to communicate through it, and hopefully leave the user facing parts unchanged.

@pergolafabio
Copy link

Cool , looking forward to it, and also the binary sensor ... I am now running the docker from mancioshell, his version is working great, also using the App API

@daxda
Copy link

daxda commented Jun 22, 2020

thanks, i will love this integration. it was just what I needed

@OnFreund
Copy link
Contributor Author

@pergolafabio working great is mostly a function of polling frequency (which I intend to make configurable. The mancioshell version is also configurable and defaults to polling every 5 seconds), and handling logouts. The advantages of using the app api are different (mainly that it's more future proof).
As for binary sensors, that's part of the roadmap, but HA guidelines are for the initial PR for a new integration to only have one platform.
Other items on the roadmap, including stuff missing from the mancioshell version, are the ability to arm groups and map them to states (so, for example, you can map group A to "Arm Home"), etc...

@pergolafabio
Copy link

Great! Gonna like it, and yes, good idea to make it a config option, I think I have mine setup to 3000 if if remember correctly, going lower was not good...
Using the sensors to turn lights

@pergolafabio
Copy link

Is it also possible to use multiple pin , or even use the pin coded from the existing users?

@OnFreund
Copy link
Contributor Author

I haven't implemented any pin code functionality yet, and haven't thought it through. The first implementation will most likely include a single front end pin code, which you can set to be the same as one of the control panel users.

homeassistant/components/risco/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/risco/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/risco/alarm_control_panel.py Outdated Show resolved Hide resolved
@skank01
Copy link

skank01 commented Jun 24, 2020

Is this staging for the next HA release? Or way too early? I have absolutely no idea :)

@OnFreund
Copy link
Contributor Author

@skank01 I wish I could tell you. This still needs to pass review, which might take a few back-and-forths, and I'll have to write the documentation for it.

@pergolafabio
Copy link

can we already test this ? or not possible since its config flow

@OnFreund
Copy link
Contributor Author

@pergolafabio the best way I know is to setup a development environment and checkout my branch. The developer docs can guide you, but keep in mind it's not trivial.

@OnFreund OnFreund requested a review from frenck June 24, 2020 16:24
@pergolafabio
Copy link

Hmm, I don't have a dev env, guess I need to wait for release... Not possible to load as custom and yaml method?

@OnFreund
Copy link
Contributor Author

You can try as a custom component, but I haven't tested it, and don't know how it works. There's no YAML support, but maybe you can create a config entry in core.config_entries.

@pergolafabio
Copy link

ok, gonna wait for, will test soon when it comes out
and will also run the mancio version for the binary sensors
probably gonna make a high polling interval for this one then, otherwise i have 2 polling, and will be locked out :)

@OnFreund
Copy link
Contributor Author

The first version doesn't have a configurable interval just yet. This will be added in a future PR along with the DataUpdateCoordinator.
I know this seems slow and can be frustrating, but the core team is swamped with a lot of open PRs, so reviews are slow, and hopefully by splitting into smaller PRs we'll be able to get things out faster (small PRs are tagged as such, so reviewers can tackle them quickly. The initial PR for an integration is never small, I believe, but the future PRs in this series should be).

@pergolafabio
Copy link

no problem, take your time, i am already happy you are working on it,... then i can ditch mqtt and portainer for this, i have setup those 2 especially for risco from mancio

but isnt it possible to make also yaml? so we can sideload your test/new features as a custom component, so we dont have to wait untill it get merged everytime

@OnFreund
Copy link
Contributor Author

Possible, yes :), but adds significantly more work for me, and given my limited time, I'd rather focus on tackling the next items in the roadmap (group arming, pin codes, data updater, binary sensors).

@pergolafabio
Copy link

Ok, gonna check then how I can load this as custom with that core file.... Gonna ask the community, I have no idea

@pergolafabio
Copy link

hmm, i cant find how to load as custom, seems not so easy when running HassOS
wil this be part of 113 ? maybe its easier to load the extra stuff later as custom ?

@OnFreund OnFreund deleted the risco_integration branch August 22, 2020 04:51
@skank01
Copy link

skank01 commented Aug 22, 2020

Nice!!! Thank you!

coordinator = hass.data[DOMAIN][config_entry.entry_id][DATA_COORDINATOR]
entities = [
RiscoAlarm(hass, coordinator, partition_id)
for partition_id in coordinator.data.partitions.keys()
Copy link
Member

Choose a reason for hiding this comment

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

Keys is the default iterator of a dict. We can remove .keys().

class RiscoAlarm(AlarmControlPanelEntity):
"""Representation of a Risco partition."""

def __init__(self, hass, coordinator, partition_id):
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.


def __init__(self, hass, coordinator, partition_id):
"""Init the partition."""
self._hass = hass
Copy link
Member

Choose a reason for hiding this comment

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

Remove this.

if self._partition.disarmed:
return STATE_ALARM_DISARMED

return STATE_UNKNOWN
Copy link
Member

Choose a reason for hiding this comment

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

Use None to represent unknown state.


async def async_alarm_arm_night(self, code=None):
"""Send arm night command."""
await self._call_alarm_method("partial_arm")
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't have more than one service partially arm.

try:
info = await validate_input(self.hass, user_input)

return self.async_create_entry(title=info["title"], data=user_input)
Copy link
Member

Choose a reason for hiding this comment

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

Please move the entry creation to an else: block.

Copy link
Member

Choose a reason for hiding this comment

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

We should use the username as unique_id and guard from creating more than one entry with the same username.

new_callable=PropertyMock(return_value=partition_mocks),
), patch(
"homeassistant.components.risco.RiscoAPI.get_state",
AsyncMock(return_value=alarm_mock),
Copy link
Member

Choose a reason for hiding this comment

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

Coroutines are patched with AsyncMock by default.

), patch(
"homeassistant.components.risco.RiscoAPI.close", AsyncMock()
):
config_entry.add_to_hass(hass)
Copy link
Member

Choose a reason for hiding this comment

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

Move this out of the context manager.

@OnFreund
Copy link
Contributor Author

Thanks @MartinHjelmare. I addressed those in #39143

BTW, how do I get the integration scored?

@MartinHjelmare
Copy link
Member

In the PR, list the requirements bullets for the levels you want to pass and make a comment for each.

@majkers
Copy link

majkers commented Aug 23, 2020

@OnFreund is it possible to make sensors or get info from each detector?

@OnFreund
Copy link
Contributor Author

@majkers yes, that was added in #39137

@majkers
Copy link

majkers commented Aug 24, 2020

But this pr is about zones and I ask about movement detectors...

@pergolafabio
Copy link

Isn't a movement detector the same as a pir sensor? :)

@OnFreund
Copy link
Contributor Author

A zone is a collection of one or more detectors (not necessarily just movement, BTW) that work as an atomic unit. Risco doesn't really communicate in detectors, just zones. For example, when the alarm is triggered, you see the Zone that triggered it, not the individual detector.

@majkers
Copy link

majkers commented Aug 24, 2020

Yeah you are right but from time time I see in risco app one on my detectors that is actually trigerred by movement without trigerring alarm (zone is disarmed) changes its colour to red from green so they do communicate.

@pergolafabio
Copy link

That will be the binary sensor ;)

@OnFreund
Copy link
Contributor Author

What you're seeing in the app are zones, not individual detectors, just like in the integration (it even uses the same api).

@majkers
Copy link

majkers commented Aug 24, 2020

No no. I see individual detectors with menu to bypass and unbypass them and completely different thing is zones view with option to arm, disarm etc.

@majkers
Copy link

majkers commented Aug 24, 2020

More or less like here:

image

@OnFreund
Copy link
Contributor Author

Once again - what you're seeing in the app are zones, not detectors. (look at their names, they're even called "Zone 34", Zone 36aaaaaaaa").
Based on your description of "Zones", you mean either Partitions (each one of those will be a separate Alarm Control Panel in HA), or Groups (A/B/C/D - I'm working on support for those).

@OnFreund
Copy link
Contributor Author

"Yaron Office" and "Amir Office" in your screenshot are both partitions.

@majkers
Copy link

majkers commented Aug 24, 2020

Echh we don't understand eachother. They are called zone in this screen. In my house ther are called eg. kitchen, garage etc. and are grouped in partitions. In this screen partitions are called Yaron office etc. And as far as I understand You create one sensor for one partition right? This guy https://github.com/lucacalcaterra/risco-mqtt-bridge added ability to bypass and unbypass detectors. This is what I am talking about

@OnFreund
Copy link
Contributor Author

This isn't really the right place for this discussion, maybe we can move to discord?

In any case, Yaron office, etc... are partitions. I don't know what "kitchen, garage, etc.." are, but bypassing in Risco works on Zones, not detectors (once again, Risco doesn't really have an externally visible concept of detectors, and the "Detectors" screen in the app shows Zones).
The integration doesn't yet support bypassing, but every Zone has an attribute to show if it's bypassed.

@majkers
Copy link

majkers commented Aug 24, 2020

OK. Now I think I get it. Detector = zone 👍 It doesn't matter. Look. I think that You made a great thing with that. Thank and looking forward to 0.115 to test it

@OnFreund
Copy link
Contributor Author

Still curios what "kitchen, garage, etc..." are in your setup. Do you have a screenshot with them?

@majkers
Copy link

majkers commented Aug 24, 2020

Kitchen, garage are just the names my installator gave them. By "them" I think about detectors just like "Detectors" screen is called in app but You call them zones. They have the same icon as eg. "21: PIR Detector" from above and are groupped under Partition name of my installation. In the scrren above it is "Yaron Office" and in my place it is eg. "Downstairs".
What I am courius about is that from time to time they turn to red (not green like in the screen)...

@majkers
Copy link

majkers commented Aug 24, 2020

The ideal world would be to use those detectors/zones to detect movement and eg. turn on/off the lights :)

@OnFreund
Copy link
Contributor Author

So yes, they are zones, and that's also the name Risco uses anywhere by the app (in the physical panel, for example).
They are red when they are triggered.
You can use them to detect movement, but they'll have a delay, so might not be best for things like turning on the lights (but could be ok for turning off).

@majkers
Copy link

majkers commented Aug 24, 2020

OK thanks

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Aug 24, 2020
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.