Align manual_mqtt alarm control panel with manual #82368
Conversation
bonzini
force-pushed
the
manual-mqtt-align
branch
from
e7893a0
to
c724683
Compare
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
bonzini
force-pushed
the
manual-mqtt-align
branch
7 times, most recently
from
da4963e
to
d83df9b
Compare
|
Added more changes. At this point the easiest way to review this is to diff the two components and the corresponding tests, since the only changes basically are related to MQTT. |
bonzini
force-pushed
the
manual-mqtt-align
branch
from
d9d8c61
to
27f02de
Compare
|
@MartinHjelmare a few years ago you reviewed my |
|
Given little attention on this PR, I have opened #82990 with the |
bonzini
force-pushed
the
manual-mqtt-align
branch
from
27f02de
to
177bddb
Compare
bonzini
changed the title
frenck
changed the title
There was a problem hiding this comment.
If you want to add new functionality, please register an issue in the UI to notify users and make sure it does not break the config of existing users.
If you want to fix anything, please apply a non breaking fix in a separate PR.
If you want to improve code quality to non related code, do this in a separate PR.
| @@ -9,6 +9,9 @@ | |||
| ATTR_CHANGED_BY: Final = "changed_by" | |||
| ATTR_CODE_ARM_REQUIRED: Final = "code_arm_required" | |||
|
|
|||
| ATTR_PREVIOUS_STATE: Final = "previous_state" | |||
There was a problem hiding this comment.
Let us not attributes here.
| } | ||
| if self.state in (STATE_ALARM_PENDING, STATE_ALARM_ARMING): | ||
| return { | ||
| ATTR_PREVIOUS_STATE: self._previous_state, |
There was a problem hiding this comment.
Is it really necessary to change these attributes and break.
There was a problem hiding this comment.
Unfortunately yes, it is. The real motivation for me to do this PR is that I would like to have alarm control panel groups (reviving my old work from #11323), and the group needs some consistency between the devices that they include. While the group code I have written can deal with a control panel that doesn't have previous/next state attributes, the behavior is inferior.
This also justifies moving them to const, because they become standardized for all alarm control panels that want to provide full functionality for groups.
I have created a single large pull request because, more than new functionality I saw this work as fixing the mistakes that led "manual" and "manual MQTT" to become unsynchronized (#10697, #17270, #32950 and #55340). It is really easier to review the combined changes than any split PR, but of course I can split it if you prefer.
To be honest, I did wonder if the "manual MQTT" integration should exist at all. It seems just as easy to do it using a "normal" manual control panel plus a template alarm control panel. But it seems to have a couple hundred users according to the telemetry, so I didn't propose the deprecation route.
|
I will open separate pull requests with (to be applied in this order): [x] #84264: new states and test parametrization (largest and with the worst conflicts) I will keep this pull request as "perennial draft" to track the missing changes. |
bonzini
force-pushed
the
manual-mqtt-align
branch
from
306d66f
to
5cb44fb
Compare
The manual alarm control panel supports ARMED_CUSTOM_BYPASS and ARMED_VACATION. Bring them over to the MQTT version.
…rm_control_panel.const
As part of home-assistant#32950, the manual control panel\s state attributes were renamed from pre_pending_state and post_pending_state to previous_state and next_state. Unfortunately, this change was not applied to manual_mqtt. I would like to add alarm control panel groups, and the group code needs some consistency between the devices that they include. So, bring over the change.
This will be useful to properly restore timers on restart.
Commit 7bfc1d2 made a major change to the manual alarm panel state machine, but it wasn't brought over to manual MQTT. Do it now.
bonzini
force-pushed
the
manual-mqtt-align
branch
from
5cb44fb
to
c5ba73d
Compare
Try to deprecate first to avoid breaking using |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Breaking change
Manual MQTT 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
This makes the manual MQTT alarm control panel integration consistent with the manual alarm control panel.
Proposed change
The manual_mqtt and manual alarm control panel integrations have diverged. The intention of the two is to be as similar as possible, in fact manual_mqtt documentation leaves most of the detail to manual.
This includes:
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: