Use "arming" state during transition in manual alarm panel #32950

starkillerOG · 2020-03-18T20:34:17Z

Breaking change

Manual Alarm Control Panel
When going from state "disarmed" to any other (armed) state such as "armed_away", the state will be "arming" instead of "pending" during the transition time as set in the configuration.
When going from an armed state (such as "armed_away") to the "triggered" state the state will still be "pending" during the transition time as set in the configuration (as it was before).

State attribute "pre_pending_state" changed to "previous_state"
State attribute "post_pending_state" changed to "next_state"

config option "pending_time" is renamed to "arming_time", functionality is the same

The time the alarm stays at "pending" when triggered has changed from delay_time of the previous state + arming_time(previously known as pending_time) of the triggered state to only the delay_time of the previous state

Proposed change

Manual Alarm Control Panel
When going from state "disarmed" to any other (armed) state such as "armed_away", the state will be "arming" instead of "pending" during the transition time as set in the configuration.
When going from an armed state (such as "armed_away") to the "triggered" state the state will still be "pending" during the transition time as set in the configuration (as it was before).

This can be usefull if you want to activate a warning before an alarm goes off, for instance:
Make an automation that triggers on the "pending" status and sends a text to speach message anounching that "You have 10 seconds to disable the alarm", and then activat the actual alarm on the "triggered" state.
Of course you do not want this tts anaounchment when arming the device, therefore when arming the device a diffrent state needs to be used which will be "arming".
Besides since HomeAssistant supports the "arming" state it is probably better for uniformity to actualy use the "arming" state when arming the alarm instead of the "pending" state which is used when an alarm is pending (about to go to triggerd).

Docs PR:

home-assistant/home-assistant.io#12531

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

Example entry for `configuration.yaml`:

# Example configuration.yaml

Additional information

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

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

starkillerOG · 2020-03-18T20:39:54Z

I think the failed check has nothing to do with this PR, can someone confirm?

MartinHjelmare · 2020-03-19T12:51:05Z

Please describe the background for this change, not only what the change is.

starkillerOG · 2020-03-20T08:05:44Z

@MartinHjelmare I just updated the discription to tell a bit more about the background

MartinHjelmare · 2020-03-20T09:11:44Z

The pending time config option is used for both the pending time and the arming time. Is that ok?

homeassistant/components/manual/alarm_control_panel.py

starkillerOG · 2020-03-20T11:11:10Z

The pending time config option is used for both the pending time and the arming time. Is that ok?

I think that is okay since you can control the transition times between each mode individually anyway (see docs). For example this config will ensure "arming" state for 60 seconds and "pending" state for 10 seconds, but in principle all these timings can be set for each state (also "armed_home" "armed_away" etc.):

    pending_time: 60   #default arming time 60 seconds
    triggered:
      pending_time: 10  #trigger after 10 seconds

MatthewFlamm · 2020-03-20T11:14:06Z

The configuration already treats these two things as different, so I think it is good to support different states. It is much more intuitive to the user. However, in configuring this according to the documentation, pending_time will now refer to the state of arming while delay_time will refer to the state of pending. If true, we'd also have to change the configuration. But in this case pending_time will switch meaning from the 'pre pending state' to the 'post pending state'. I think this will cause much confusion during the breaking change.

MatthewFlamm · 2020-03-20T11:21:20Z

 pending_time: 60   #default arming time 60 seconds
 triggered:
   pending_time: 10  #trigger after 10 seconds

Currently this would be configured:

 pending_time: 60   #default arming time 60 seconds
 delay_time: 10  #trigger after 10 seconds (note this would have state pending)

I would propose that delay_time is kept but pending_time would be renamed to arming_time.

starkillerOG · 2020-03-20T11:26:21Z

@MatthewFlamm I agree, I will start changing it

starkillerOG · 2020-03-20T11:33:15Z

@MatthewFlamm I cannot change the configuration variable names since they are imported from HomeAssistant const.py, Other platforms might be using those same variable names and changing those would probably require discussion about the Home Assistant architecture.

Therefore I propose changing the configuration varibale names (delay_time is kept but pending_time would be renamed to arming_time), is left for a seprate PR.

MatthewFlamm · 2020-03-20T12:07:53Z

I cannot change the configuration variable names since they are imported from HomeAssistant const.py,

You can create a new variable. I don't think there should be two breaking changes however this is implemented.

manual_mqtt operates the same way and would presumably also need to be changed to match.

homeassistant/components/manual/alarm_control_panel.py

starkillerOG · 2020-03-20T12:58:19Z

I cannot change the configuration variable names since they are imported from HomeAssistant const.py,

You can create a new variable. I don't think there should be two breaking changes however this is implemented.

manual_mqtt operates the same way and would presumably also need to be changed to match.

I will work on it tonight

starkillerOG · 2020-03-22T20:39:01Z

@MatthewFlamm @MartinHjelmare I think this is now ready to merge.
Any more comments?

starkillerOG · 2020-04-17T19:41:00Z

@MartinHjelmare I commited the changes that you requested.
If you agree with these changes, can you mark you "changes reqeusted" as done?

homeassistant/components/manual/alarm_control_panel.py

MartinHjelmare · 2020-04-17T23:35:15Z

It would be good to change to async api for the whole platform, as it doesn't do any I/O anyway, but that's for another PR.

Co-Authored-By: Martin Hjelmare <[email protected]>

starkillerOG · 2020-04-18T09:46:59Z

@MartinHjelmare any more sugestions, or can the "changes requested" be marked as done?

MartinHjelmare

Looks good!

Since this a quite big breaking change, and it's for alarm devices, I'd like a second opinion before merge.

balloob · 2020-04-21T06:08:28Z

This change looks good.

We should also update the device triggers which still use pending to accept both pending and arming.

starkillerOG · 2020-04-21T06:49:20Z

@balloob what do you mean with "device triggers", is that something in the manual alarm panel or in which files should that be changed?

balloob · 2020-04-21T06:50:31Z

alarm_control_panel/device_triggers.py

homeassistant/components/alarm_control_panel/device_trigger.py

starkillerOG · 2020-04-21T11:16:48Z

@balloob I added the "arming" state to the device triggers.
Could you check if this is alright?

I also made a review with some questions about the device triggers

starkillerOG · 2020-04-23T05:56:11Z

@MartinHjelmare could you take a look at why the python tests stopped?
I do not see any errors, I think it just exceeded its max run time.

Can this be merged now?

MartinHjelmare · 2020-04-23T19:17:12Z

I restarted the tests.

starkillerOG · 2020-04-23T20:50:10Z

@MartinHjelmare yes now it passed, thanks for the restart!

Can this now be merged?

MartinHjelmare · 2020-04-23T22:05:33Z

Good!

Manual Alarm Control Panel: use proper "Arming" state

3630aab

probot-home-assistant bot added integration: manual small-pr PRs with less than 30 lines. labels Mar 18, 2020

MartinHjelmare changed the title ~~Manual Alarm Control Panel: use proper "arming" state~~ Change manual alarm_control_panel to use "arming" state during transition Mar 19, 2020

MartinHjelmare reviewed Mar 20, 2020

View reviewed changes

homeassistant/components/manual/alarm_control_panel.py Outdated Show resolved Hide resolved

Update previous and next attributes

3567396

homeassistant added the cla-signed label Mar 20, 2020

MatthewFlamm reviewed Mar 20, 2020

View reviewed changes

homeassistant/components/manual/alarm_control_panel.py Show resolved Hide resolved

starkillerOG added 7 commits March 22, 2020 18:37

add CONF_ARMING_TIME

cec4907

Split up arming and pending time, pending_time --> arming_time

daa4984

update tests

ff28352

fix issort

e19d965

fix issort

6f80716

fix demo platform

48d30f6

fix alarm test

af82553

remove arming_time from the triggered state

ed2c8ad

starkillerOG mentioned this pull request Mar 28, 2020

change pending_time to arming_time home-assistant/home-assistant.io#12530

Closed

7 tasks

starkillerOG added 2 commits April 17, 2020 20:09

async_update_ha_state -> async_write_ha_state

536fc9b

black formatting

8f41e76

MartinHjelmare reviewed Apr 17, 2020

View reviewed changes

homeassistant/components/manual/alarm_control_panel.py Outdated Show resolved Hide resolved

starkillerOG and others added 2 commits April 18, 2020 10:48

add Callback

471ea54

Co-Authored-By: Martin Hjelmare <[email protected]>

import callback

3aafcf8

MartinHjelmare approved these changes Apr 18, 2020

View reviewed changes

balloob removed the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Apr 21, 2020

update device triggers alarm_control_panel

3675069

starkillerOG commented Apr 21, 2020

View reviewed changes

homeassistant/components/alarm_control_panel/device_trigger.py Show resolved Hide resolved

homeassistant/components/alarm_control_panel/device_trigger.py Outdated Show resolved Hide resolved

homeassistant/components/alarm_control_panel/device_trigger.py Show resolved Hide resolved

Update test_device_trigger.py

ffe5616

Update device_trigger.py

85a12ad

MartinHjelmare changed the title ~~Change manual alarm_control_panel to use "arming" state during transition~~ Use "arming" state during transition in manual alarm_control_panel Apr 23, 2020

MartinHjelmare changed the title ~~Use "arming" state during transition in manual alarm_control_panel~~ Use "arming" state during transition in manual alarm panel Apr 23, 2020

MartinHjelmare merged commit 7bfc1d2 into home-assistant:dev Apr 23, 2020

lock bot locked and limited conversation to collaborators Apr 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use "arming" state during transition in manual alarm panel #32950

Use "arming" state during transition in manual alarm panel #32950

starkillerOG commented Mar 18, 2020 •

edited by MartinHjelmare

Loading

starkillerOG commented Mar 18, 2020

MartinHjelmare commented Mar 19, 2020

starkillerOG commented Mar 20, 2020

MartinHjelmare commented Mar 20, 2020

starkillerOG commented Mar 20, 2020

MatthewFlamm commented Mar 20, 2020

MatthewFlamm commented Mar 20, 2020

starkillerOG commented Mar 20, 2020

starkillerOG commented Mar 20, 2020

MatthewFlamm commented Mar 20, 2020

starkillerOG commented Mar 20, 2020

starkillerOG commented Mar 22, 2020

starkillerOG commented Apr 17, 2020

MartinHjelmare commented Apr 17, 2020

starkillerOG commented Apr 18, 2020

MartinHjelmare left a comment

balloob commented Apr 21, 2020

starkillerOG commented Apr 21, 2020

balloob commented Apr 21, 2020

starkillerOG commented Apr 21, 2020

starkillerOG commented Apr 23, 2020

MartinHjelmare commented Apr 23, 2020

starkillerOG commented Apr 23, 2020

MartinHjelmare commented Apr 23, 2020

Use "arming" state during transition in manual alarm panel #32950

Use "arming" state during transition in manual alarm panel #32950

Conversation

starkillerOG commented Mar 18, 2020 • edited by MartinHjelmare Loading

Breaking change

Proposed change

Docs PR:

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

starkillerOG commented Mar 18, 2020

MartinHjelmare commented Mar 19, 2020

starkillerOG commented Mar 20, 2020

MartinHjelmare commented Mar 20, 2020

starkillerOG commented Mar 20, 2020

MatthewFlamm commented Mar 20, 2020

MatthewFlamm commented Mar 20, 2020

starkillerOG commented Mar 20, 2020

starkillerOG commented Mar 20, 2020

MatthewFlamm commented Mar 20, 2020

starkillerOG commented Mar 20, 2020

starkillerOG commented Mar 22, 2020

starkillerOG commented Apr 17, 2020

MartinHjelmare commented Apr 17, 2020

starkillerOG commented Apr 18, 2020

MartinHjelmare left a comment

Choose a reason for hiding this comment

balloob commented Apr 21, 2020

starkillerOG commented Apr 21, 2020

balloob commented Apr 21, 2020

starkillerOG commented Apr 21, 2020

starkillerOG commented Apr 23, 2020

MartinHjelmare commented Apr 23, 2020

starkillerOG commented Apr 23, 2020

MartinHjelmare commented Apr 23, 2020

starkillerOG commented Mar 18, 2020 •

edited by MartinHjelmare

Loading

Example entry for `configuration.yaml`: