"Alarm group" platform #11323

bonzini · 2017-12-27T13:24:31Z

Description:

This pull request introduces a "group" alarm control panel platform. The platform distributes the commands to multiple sub-panels, and reports a unified state from all the sub-panels. The main use case is to apply different delay times to different rooms, and to disable some rooms in the "armed_home" or "armed_night" states.

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

Example entry for `configuration.yaml` (if applicable):

alarm:
  - name: ALARM_NAME
    platform: group
    code_format: '\d+'
    panels:
    - panel: alarm_control_panel.garage
    - panel: alarm_control_panel.living_room
    - panel: alarm_control_panel.bedroom
    - panel: alarm_control_panel.tampering

Checklist:

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

Documentation added/updated in home-assistant.github.io

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New files were added to .coveragerc.
Tests have been added to verify that the new code works.

bonzini · 2018-01-10T09:16:12Z

Any news?

MartinHjelmare · 2018-01-10T09:30:23Z

I've had a brief look and I think I like it. I haven't had time to go through it in detail.

I would like to have a second opinion though too, since this is a structural change and also core change.

bonzini · 2018-01-20T09:30:13Z

Another gentle ping... Sorry :-)

bonzini · 2018-02-01T12:41:34Z

And now it's been a couple weeks more.

MartinHjelmare · 2018-02-01T19:57:39Z

Yeah, sorry about this. I'll try to get some time this weekend, if nobody else beats me to it.

MartinHjelmare

The most important question before we go ahead is, what is the difference between this new group platform and a regular home assistant group of alarm control panels, in terms of features.

I haven't looked at the test for the new platform.

I like this if a normal group doesn't work, but I want at least a second opinion from another member, before we go ahead and merge. Grouping stuff in home assistant is controversial sometimes. 😄

There's also a merge conflict currently.

See comments below.

MartinHjelmare · 2018-02-05T02:35:02Z

homeassistant/components/alarm_control_panel/group.py

+from collections import Counter
+from copy import deepcopy
+import logging
+import voluptuous as vol


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

MartinHjelmare · 2018-02-05T02:37:47Z

homeassistant/components/alarm_control_panel/group.py

+
+DEFAULT_ALARM_NAME = 'Group Alarm'
+
+PLATFORM_SCHEMA = vol.Schema(vol.All({


This should extend the base alarm_control_panel component PLATFORM_SCHEMA.

MartinHjelmare · 2018-02-05T02:44:04Z

homeassistant/components/alarm_control_panel/group.py

+    vol.Optional(CONF_NAME, default=DEFAULT_ALARM_NAME): cv.string,
+    vol.Optional(CONF_CODE_FORMAT, default=''): cv.string,
+    vol.Required(CONF_PANELS): vol.All(cv.ensure_list, [{
+        vol.Required(CONF_PANEL): cv.entity_id,


Why should this be a list of dictionaries? Can it not just be a list of entity_ids? The CONF_PANEL key seems to not add anything.

Do we want to allow the same entity_id multiple times?

The CONF_PANEL key is mostly for future extensibility, in case we want to add more panel-specific configuration.

Having the same entity_id multiple times would probably be a bug in the configuration. However, I haven't seen uses of vol.Unique in hass, which is why I left it out.

I think something like the scene config makes more sense. But right now there are no values for the entity keys:
https://home-assistant.io/components/scene/.

MartinHjelmare · 2018-02-05T02:44:37Z

homeassistant/components/alarm_control_panel/group.py

+
+
+def setup_platform(hass, config, add_devices, discovery_info=None):
+    """Set up the manual alarm platform."""


Stale docstring.

MartinHjelmare · 2018-02-05T02:45:44Z

homeassistant/components/alarm_control_panel/group.py

+        hass,
+        config[CONF_NAME],
+        config[CONF_CODE_FORMAT],
+        config.get(CONF_PANELS)


Don't use dict.get if accessing a required key in the config.

MartinHjelmare · 2018-02-05T03:12:08Z

homeassistant/components/alarm_control_panel/group.py

+        """Return the state of the device."""
+        # _update_state always behaves as if a transition was in progress.
+        # But if the pre- and post-state match, we're not "pending"!
+        self._update_state()


I would move all this logic to _update_state which should be renamed to async_update. Just return self._state here.

MartinHjelmare · 2018-02-05T03:13:39Z

homeassistant/components/alarm_control_panel/group.py

+        def _common_state(states):
+            # This can happen if the sub-panels have not loaded yet.
+            if len(states) == 0:
+                return STATE_ALARM_DISARMED


Return None if state is unknown.

MartinHjelmare · 2018-02-05T03:15:10Z

homeassistant/components/alarm_control_panel/group.py

+            if states[STATE_ALARM_PENDING]:
+                return STATE_ALARM_PENDING
+
+            # If all entities have the same state, great.  Otherwise,


There's two spaces after the period, before "Otherwise". Please remove one space.

MartinHjelmare · 2018-02-05T03:17:17Z

homeassistant/components/alarm_control_panel/group.py

+                return STATE_ALARM_PENDING
+
+            # If all entities have the same state, great.  Otherwise,
+            # we can only summarize the state as "custom bypass".


Why is this? I'm not following here, sorry.

Suppose we follow the example in the group documentation, and add a group alarm that is composed of two subpanels (one per floor). The group alarm is then placed under the "home" tab, while the subpanels would go under the "upstairs" and "downstairs" tabs respectively.

You can go to the "upstairs" tab and only arm the "upstairs" control panel. The result is that you have a custom setup for the whole group, where some environments have had their sensors bypassed. The same holds if you arm the entire home via the group alarm, and then disarm the "upstairs" control panel manually. This is expressed with return STATE_ALARM_ARMED_CUSTOM_BYPASS here.

Thanks, makes sense.

MartinHjelmare · 2018-02-05T03:21:37Z

homeassistant/components/alarm_control_panel/group.py

+        post_states = Counter()
+        for entity_id, state in self._states:
+            if state.state == STATE_ALARM_PENDING and \
+                    (ATTR_PRE_PENDING_STATE in state.attributes):


I think it's a bit weird that we depend on the state attributes to define state. Maybe we can store the previous pre/post states in two new instance attibutes, instead? Let me know if I'm missing the point here.

The code is looking "inside" the subpanels' state attributes to understand more clearly what the subpanels are doing, and possible synthesize those same state attributes for the group (ATTR_PRE_PENDING_STATE goes to self._pre_state, ATTR_POST_PENDING_STATE goes to self._post_state.

It would certainly be possible to make before/after states into instance attributes that are available even in non-PENDING states; and in fact it may possibly make some sense for the "before" state, because that is effectively the "current operating state" of the panel (I think it makes less sense for the "after" state). However, considering that most alarm platforms do not provide PRE_PENDING_STATE, I think it's cleaner to leave it as a "nice to have" state attribute.

bonzini · 2018-02-05T11:04:23Z

The most important question before we go ahead is, what is the difference between this new group platform and a regular home assistant group of alarm control panels, in terms of features.

The group alarm platform and the control groups are for different use cases. If you have multiple alarm panels, placing them in a group only has a "visual" effect, each control panel is still completely independent and is armed/disarmed separately. The alarm group platform instead lets you use a single control to arm/disarm multiple control panels, e.g. "clicking" a single control (the group alarm) to arm/disarm alarms in the entire home.

The two can also be used together. Following the example in the group documentation, you could have a group alarm that is composed of two subpanels (one per floor). The group alarm is then placed under the "home" tab, while the subpanels would go under the "upstairs" and "downstairs" tabs respectively.

houndci-bot · 2018-02-10T09:35:51Z

homeassistant/components/alarm_control_panel/group.py

+
+
+class GroupAlarm(alarm.AlarmControlPanel):
+    """Implement services and state computation for the group alarm platform."""


line too long (80 > 79 characters)

houndci-bot · 2018-02-10T09:35:51Z

homeassistant/components/alarm_control_panel/group.py

+    DOMAIN, PLATFORM_SCHEMA, SERVICE_ALARM_DISARM, SERVICE_ALARM_ARM_HOME,
+    SERVICE_ALARM_ARM_AWAY, SERVICE_ALARM_ARM_NIGHT, SERVICE_ALARM_TRIGGER,
+    SERVICE_ALARM_ARM_CUSTOM_BYPASS)
+from homeassistant.const import (


'homeassistant.const.STATE_ALARM_DISARMED' imported but unused
'homeassistant.const.CONF_PLATFORM' imported but unused

houndci-bot · 2018-02-10T09:35:51Z

homeassistant/components/alarm_control_panel/group.py

+
+
+class GroupAlarm(alarm.AlarmControlPanel):
+    """Implement services and state computation for the group alarm platform."""


line too long (80 > 79 characters)

houndci-bot · 2018-02-10T09:35:51Z

homeassistant/components/alarm_control_panel/group.py

+    DOMAIN, PLATFORM_SCHEMA, SERVICE_ALARM_DISARM, SERVICE_ALARM_ARM_HOME,
+    SERVICE_ALARM_ARM_AWAY, SERVICE_ALARM_ARM_NIGHT, SERVICE_ALARM_TRIGGER,
+    SERVICE_ALARM_ARM_CUSTOM_BYPASS)
+from homeassistant.const import (


'homeassistant.const.STATE_ALARM_DISARMED' imported but unused
'homeassistant.const.CONF_PLATFORM' imported but unused

MartinHjelmare · 2018-02-12T15:51:25Z

There's been a similar discussion in #12229, regarding grouping of lights. As mentioned there an alternative is to update the behavior of grouped entities, in our case alarms, via change of the frontend. What would be the limitations if we go that route?

bonzini · 2018-02-12T17:09:23Z

This is interesting! I think the main one is that the group alarm mediates between sensors and switches in the automation rules: you attach sensors to the individual room alarms, and switches (e.g. siren) to the group. So the state update algorithm has a more "active" role than the light group in #12229. Sure it's not hard to add an OR to the "trigger siren" and a NOR to the opposite, but it's a bit simpler and comes out very natural with the group alarm.

In my case, I am also adding GPIO-based arm/disarm (could be done with a keypad, with RFID keys, or similar) which goes beyond what the frontend can provide. And another difference is that there is no equivalent to TemplateLight for alarms.

balloob · 2018-02-13T06:18:43Z

@MartinHjelmare one argument against having groups be aware if it is grouping lights/switches/alarms etc is that all of a sudden group becomes tightly coupled with other implementations which is something we should try to avoid.

balloob · 2018-02-13T06:19:51Z

As for implementation of a group alarm platform, I guess I'm not against. I think that if we want to make this more common, we should make sure that they are all called group. So alarm_control_panel.group, light.group etc to make it clear that they have a similar "mission".

Included the comments from home-assistant#11323

* Add grouped_light platform * 📝 Fix Lint issues * 🎨 Reformat code with yapf * A Few changes * ✨ Python 3.5 magic * Improvements Included the comments from #11323 * Fixes * Updates * Fixes & Tests * Fix bad-whitespace * Domain Config Validation ... by rebasing onto #12592 * Style changes & Improvements * Lint * Changes according to Review Comments * Use blocking light.async_turn_* * Revert "Use blocking light.async_turn_*" This reverts commit 9e83198. * Update service calls and state reporting * Add group service call tests * Remove unused constant.

They will be used by the group alarm platform too. While having to adjust the test code for over-80 characters lines, switch to assertEqual too.

The group alarm platform provides a mechanism to automatically arm sub-panels and compute the state of the overall alarm system. For example, a siren switch can be easily attached to the group entering or exiting the triggered state. A full example is included in the documentation pull request.

OttoWinter · 2018-03-18T12:25:42Z

homeassistant/components/alarm_control_panel/group.py

+
+CONF_CODE_FORMAT = 'code_format'
+
+DEFAULT_ALARM_NAME = 'Group Alarm'


Please also change the default name to Alarm Group for consistency among group platforms.

OttoWinter · 2018-03-18T12:26:55Z

homeassistant/components/alarm_control_panel/group.py

+PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
+    vol.Optional(CONF_NAME, default=DEFAULT_ALARM_NAME): cv.string,
+    vol.Optional(CONF_CODE_FORMAT, default=''): cv.string,
+    vol.Required(CONF_ENTITIES): vol.All(cv.ensure_list, [cv.entity_id]),


Use cv.entities_domain instead:

vol.Required(CONF_ENTITIES): cv.entities_domain(alarm.DOMAIN)

OttoWinter · 2018-03-18T12:28:50Z

homeassistant/components/alarm_control_panel/group.py

+
+            self.async_schedule_update_ha_state(True)
+
+        self.hass.bus.async_listen_once(


Maybe have a look at how light groups do async_added_to_hass:

https://github.com/home-assistant/home-assistant/blob/ca5f4709564773c8cebd6ee40aa7cc1095b19105/homeassistant/components/light/group.py#L72-L82

Your current code would (I think) break with dynamically loading/unloading your entity.

OttoWinter · 2018-03-18T12:29:57Z

homeassistant/components/alarm_control_panel/group.py

+            EVENT_HOMEASSISTANT_START, group_alarm_startup)
+
+    @asyncio.coroutine
+    def _async_send_message(self, service, **kwargs):


If the only kwarg is code, why make this accept **kwargs?

OttoWinter · 2018-03-18T12:31:02Z

homeassistant/components/alarm_control_panel/group.py

+            sending_payload = deepcopy(payload.copy())
+            sending_payload[ATTR_ENTITY_ID] = entity_id
+            tasks.append(self.hass.services.async_call(
+                DOMAIN, service, sending_payload))


You need to specify blocking=True. Also the entities will automatically be expanded for you, so no need for a for-loop I think.

https://github.com/home-assistant/home-assistant/blob/ca5f4709564773c8cebd6ee40aa7cc1095b19105/homeassistant/components/light/group.py#L188-L189

pvizeli · 2018-04-17T11:07:06Z

What is the state of this? @bonzini can you address the open comments so that we can merge it?

bonzini · 2018-04-17T14:15:43Z

I should be able to look at it again over the weekend.

balloob · 2018-05-02T18:26:39Z

This PR seems to have gone stale. Closing it. You can reopen it when you're ready to finish it.

bonzini requested a review from a team as a code owner December 27, 2017 13:24

homeassistant added cla-signed new-platform platform: alarm_control_panel.manual labels Dec 27, 2017

balloobbot added core platform labels Dec 27, 2017

bonzini mentioned this pull request Dec 27, 2017

Add documentation for the group alarm platform home-assistant/home-assistant.io#4283

Closed

2 tasks

bonzini force-pushed the alarm-group branch from 74199fb to d5b253b Compare December 27, 2017 13:45

homeassistant added the cla-signed label Dec 27, 2017

bonzini force-pushed the alarm-group branch from d5b253b to d25bbc9 Compare December 27, 2017 13:47

homeassistant added the cla-signed label Dec 27, 2017

MartinHjelmare requested changes Feb 5, 2018

View reviewed changes

bonzini force-pushed the alarm-group branch from d25bbc9 to 20b3e44 Compare February 10, 2018 09:35

homeassistant added the cla-signed label Feb 10, 2018

houndci-bot reviewed Feb 10, 2018

View reviewed changes

bonzini force-pushed the alarm-group branch from 20b3e44 to e6e32ec Compare February 10, 2018 09:41

homeassistant added the cla-signed label Feb 10, 2018

bonzini force-pushed the alarm-group branch from e6e32ec to e639704 Compare February 25, 2018 10:32

homeassistant added the cla-signed label Feb 25, 2018

OttoWinter added a commit to OttoWinter/home-assistant that referenced this pull request Feb 27, 2018

Improvements

dd32be2

Included the comments from home-assistant#11323

bonzini force-pushed the alarm-group branch from e639704 to 9789adc Compare March 10, 2018 09:05

bonzini requested review from andrey-git and cdce8p as code owners March 10, 2018 09:05

homeassistant added the cla-signed label Mar 10, 2018

bonzini added 6 commits March 12, 2018 15:13

alarm: move pre/post_pending_state to const.py

b245472

They will be used by the group alarm platform too. While having to adjust the test code for over-80 characters lines, switch to assertEqual too.

updates for review

2dc51c4

return None if state is unknown

1670da7

replace CONF_PANELS and CONF_PANEL with CONF_ENTITIES

f020ada

fix for pylint

03869ca

bonzini force-pushed the alarm-group branch from 9789adc to 03869ca Compare March 12, 2018 14:13

homeassistant added the cla-signed label Mar 12, 2018

OttoWinter reviewed Mar 18, 2018

View reviewed changes

cdce8p removed their request for review March 21, 2018 18:12

balloob closed this May 2, 2018

home-assistant locked and limited conversation to collaborators Sep 5, 2018

ghost added integration: group integration: manual integration: manual_mqtt and removed platform: alarm_control_panel.group labels Mar 21, 2019


		DEFAULT_ALARM_NAME = 'Group Alarm'

		PLATFORM_SCHEMA = vol.Schema(vol.All({



		def setup_platform(hass, config, add_devices, discovery_info=None):
		"""Set up the manual alarm platform."""



		class GroupAlarm(alarm.AlarmControlPanel):
		"""Implement services and state computation for the group alarm platform."""


		CONF_CODE_FORMAT = 'code_format'

		DEFAULT_ALARM_NAME = 'Group Alarm'


		self.async_schedule_update_ha_state(True)

		self.hass.bus.async_listen_once(

"Alarm group" platform #11323

"Alarm group" platform #11323

Conversation

bonzini commented Dec 27, 2017 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

bonzini commented Jan 10, 2018

MartinHjelmare commented Jan 10, 2018

bonzini commented Jan 20, 2018

bonzini commented Feb 1, 2018

MartinHjelmare commented Feb 1, 2018

MartinHjelmare left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonzini commented Feb 5, 2018

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

MartinHjelmare commented Feb 12, 2018

bonzini commented Feb 12, 2018

balloob commented Feb 13, 2018

balloob commented Feb 13, 2018

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

pvizeli commented Apr 17, 2018

bonzini commented Apr 17, 2018

balloob commented May 2, 2018

bonzini commented Dec 27, 2017 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

MartinHjelmare left a comment •

edited

Loading