Allow quick exit by setting _attr_code_arm_required to false for DSC systems #142
Conversation
bassdr
force-pushed
the
code_arm_required_false
branch
from
94ea505
to
79cd405
Compare
|
@ufodone, any concern with this? I've been using my fork for a while to workaround this limitation... |
|
I think this would need to be exposed as a configuration option because the ability to quick arm (without a code) is a configured option on the panel itself for DSC. I believe Honeywell panels also require the code to be entered to arm the system. And there may also be cases where people prefer to require a code to arm via HA. |
Ok, I'll familiarize with the code in order to add a config option. I'll try to do this over the weekend. Hints are welcome. |
|
You should just need to add an option either to the Basic or Advanced setup. I tried to keep the Basic option to the minimum needed to setup the system initially so Advanced is probably the place to put it. But then again, the Code is specified in the Basic section so... I guess it could go either way. But definitely make it optional. The configuration is all handles in config_flow.py. If it were to go in the Basic section, take a look at Once the option is added, you should be able to reference it from alarm_control_panel.py to setup the attribute. The default behaviour if the option isn't present should remain as it is. But if the option is found in the config then use that. That should ensure that no changes are required when people update to the new version. |
bassdr
changed the title
bassdr
force-pushed
the
code_arm_required_false
branch
from
2c75711
to
20f90de
Compare
| vol.Required(ATTR_CUSTOM_FUNCTION): cv.string, | ||
| vol.Optional(ATTR_CODE): cv.string, | ||
| } | ||
| ) |
There was a problem hiding this comment.
ATTR_CODE was already defined in homeassistant.const, so I moved it, and I'm pretty sure SERVICE_SCHEMA was not used... These two are unrelated with my changes tho, just cleanups while I was tryting to understand the code.
bassdr
force-pushed
the
code_arm_required_false
branch
from
20f90de
to
5aff221
Compare
bassdr
force-pushed
the
code_arm_required_false
branch
2 times, most recently
from
a6a4476
to
dcef01c
Compare
|
I tried these changes out with a number of different settings and panel types and in the end realizing that HA alarm panels work differently than I'd thought. But that's left me wondering what to do here. On DSC systems, the EVL supports arming in away, stay, and "zero entry delay" (which is mapped to night mode by the integration). None of these require a code to be provided. So it seems that for DSC systems, On Honeywell systems, the arm commands are sent to the EVL using keystroke sequences. From what I can tell from the Honeywell documentation, these all require a code to be provided. So on Honeywell systems, either a code needs to be configured in the settings or The thing that threw me off though was that the alarm panel card seems to display the keypad strictly based on the result of the
My head has started hurting thinking about this but I'm trying to make sure I don't mess up other peoples' setups. What do you think? |
Yeah that complicate things a bit... May I suggest (and maybe relieve headache pain), to do small steps? I don't think providing the option to the user is too bad even if it does nothing for some of them. We can later change the default and hide it later based on the brand.
I can play with |
|
Why I'm now hesitant to add the config option is because, with this new information, I haven't convinced myself it's actually necessary. For DSC, I think you had it right from the get-go: it should never be anything other than For Honeywell, since these panels ALWAYS require a code to arm, So I'm thinking that this attribute should just be set appropriately based on the panel type rather than it needing to be configurable. Sorry if it seems like I'm going around in circles here but I'd like to try and avoid creating knock-on issues and also avoid adding something new only to have to remove it again. |
|
Your last message is much more clear (or I am smarter today :D). I agree with you, if we can avoid a configuration option, I'm all in. What should we do about other types tho? Neither Honeywell nor DSC. Should we keep the old logic for all but DSC? Personally I'd set it |
At least on my DSC system, it is not required to have a code for arming the alarm; only do disarm. It does what I want if I set this attribute to always false. Since I do not want HomeAssistant to be able to disarm my alarm without having to enter a user code, But I want it to be able to arm it, having always false works for me, but maybe we need to have this as an extra configuration option or only for my DSC device? Signed-off-by: David Racine <[email protected]>
bassdr
force-pushed
the
code_arm_required_false
branch
2 times, most recently
from
9aaaeb4
to
a6e090a
Compare
… code_format return None when we do not require a code. Signed-off-by: David Racine <[email protected]>
bassdr
force-pushed
the
code_arm_required_false
branch
from
a6e090a
to
beb685c
Compare
… code_format return None when we do not require a code. Signed-off-by: David Racine <[email protected]>
bassdr
force-pushed
the
code_arm_required_false
branch
from
80fca7d
to
768b920
Compare
|
Tested latest changes. Works fine with my DSC panel, even the keypad is gone when disarmed, this is even better then what I hoped for :) |
|
BTW, I made it that Honeywell has special treatments and keeps the old logic, while !honeywell take the new one that works for me. |
| AlarmControlPanelEntityFeature.ARM_HOME | ||
| | AlarmControlPanelEntityFeature.ARM_AWAY | ||
| | AlarmControlPanelEntityFeature.ARM_NIGHT |
There was a problem hiding this comment.
On DSC systems, the EVL supports arming in away, stay, and "zero entry delay" (which is mapped to night mode by the integration). [...]
I am not able to get the "zero entry delay" on my side, this is why I disabled the night mode completely, it does what I expect: disable the night mode actions, but should I revert that part? Maybe I configured my alarm to refuse it but some people are using it?
There was a problem hiding this comment.
According to my DSC documentation, *9 on a keypad does a "zero entry delay". The description says that it sets the Delay 1 and Delay 2 zones to instant and the Stay/Away zones will remain bypassed. So this does sound like a Night mode to me. But it does require a code to be entered.
I tried this with my own system and it seems to work as expected. So I think we should leave this mode enabled.
There was a problem hiding this comment.
About this option, can bring it back as you are right, it's technically possible to night arm a DSC system even tho it does not have a dedicated button on the hw panel.
bassdr
changed the title
| @@ -153,7 +149,7 @@ def __init__( | |||
| @property | |||
| def code_format(self) -> CodeFormat | None: | |||
| """Regex for code format or None if no code is required.""" | |||
| if self._code: | |||
| if self.state == STATE_ALARM_DISARMED and not self._attr_code_arm_required: | |||
There was a problem hiding this comment.
Ugh. I think this is complicated not by the new info (see comment above) that the DSC night mode requires a code be entered. So some modes of arming on a DSC require a code and others don't. It's probably still safe to set code_arm_required=false for DSC systems but maybe the keypad should always be displayed (unless a code is configured) since you can't know which mode might be armed.
There was a problem hiding this comment.
You are right that *9 does a "no-entry arm" that I never used personally. And it does require a code.
What I don't get tho, is from the ha integration, I don't have the possibility to arm this way (and would disable this either way), so I don't need the keyboard for any reason as I can't do it right now...
Do you mean you'd modify the implementation and give DSC users the ability to "night arm" from the integration by sending *9 + code? If this is what you mean, I'd definitely do this under a configuration option personally... And then we can check this option is set and always display the kbd.
There was a problem hiding this comment.
The integration doesn't explicit use the *9 keystrokes but uses the 032 TPI command which is the equivalent.
Which card are you using for your alarm? The stock alarm control card allows you to select which modes you want to display. I personally only have the away and stay boxes checked but it is possible to enable the night mode as well.
Honeywell requires a code to arm all modes and DSC requires a code to arm Night mode. There is already provision to provide the code in config which can be used to drive the code_arm_required attribute. So I wonder if instead there should be a config option to control whether a code is required to DISARM? That would allow one to specify a code but not have it used automatically for disarming the system.
On DSC systems, if one wanted to arm all modes (including night mode) without a code, they could specify the code in config and set it such that a code is required to disarm (code_disarm_required-true). That would allow a no-code arm but still require a code to disarm.
On Honeywell systems, if one didn't want a code to arm, they set the code in config and set code_disarm_required=true. The current behaviour could be preserved with a smart default if the config doesn't exist - which I think would be code_disarm_required=not code.
I think I need to walk through all the scenarios a little more carefully though to convince myself this isn't yet another possibility with an edge case that doesn't work. 😬
I potential side benefit of this could also be that it might be able to enable the zone bypass switches for DSC systems that require a code a zone since it would be possible to specify a code in the config but not automatically use it for disarming. But I'm probably getting ahead of myself...
There was a problem hiding this comment.
I've almost convinced myself that a code_disarm_required setting could be the solution here. The current behaviour is this:
Panel Type | Is code set? | Keypad on Arm | Keypad on Disarm | Night mode works?
--------------------------------------------------------------------------------
DSC | yes | no | no | yes
DSC | no | yes | yes | yes
HONEYWELL | yes | no | no | yes
HONEYWELL | no | yes | yes | yes
Where "Keypad on Arm" / "Keypad on Disarm" indicate whether the keypad is shown when arming/disarming the system.
And if a code_disarm_required config setting is added and the code_format function updated appropriately, the behaviour could be this:
Panel Type | Is code set? | code_disarm_required | Keypad on Arm | Keypad on Disarm | Night mode works?
------------------------------------------------------------------------------------------------ ------
DSC | yes | yes | no | yes | yes
DSC | yes | no | no | no | yes
DSC | no | yes | no | yes | no
DSC | no | no | no | yes | no
HONEYWELL | yes | yes | no | yes | yes
HONEYWELL | yes | no | no | no | yes
HONEYWELL | no | yes | yes | yes | yes
HONEYWELL | no | no | yes | yes | yes
With the new system, the only variant that would no longer be available would be on a DSC system where the keypad is shown for arming the system. In a practical sense, I think this is only relevant to the Night mode because the Stay and Away modes never require a code so it doesn't matter what someone enters on the keypad in that case.
What do you think?
There was a problem hiding this comment.
Actually, there's one other thing I need to confirm. It looks like the Stay and Away quick arm keys can be enabled or disabled on the panel itself. I'll try and disable them on my system to see what happens. It's possible that's just not a supported setup and arming will start failing via this integration or it's possible that the EVL/panel will response to the arm command with a request for a code. If that latter is the case, then the table above may need to be amended because there would be situations where a DSC setup would require a code for arming.
There was a problem hiding this comment.
Actually, there's one other thing I need to confirm. It looks like the Stay and Away quick arm keys can be enabled or disabled on the panel itself.
I tested it on my DSC PC1832NK, and your guess was right: if you disable section 015 option 3 (aka. the Quick Arming feature will be {en,dis}abled), when HA tries to arm without a code, the System will reject it. It puts mine in a weird state too for many seconds, where my keyboard becomes unavailable with a "System Unavailable" error on screen.
So yeah :/ yet another thing to consider.
What do you think?
THB, I would keep it simple.
- I think the fact the keyboard disappears is a "nice to have". We should do it only if we're 100% sure the user don't need it.
- I think the user expects to see a panel in HA that does the basics: arm and disarm the system.
- Everything else is IMO advanced features, and the integration should make them available thru config options. It's the responsibility of the user to match their setup.
If it's possible to query how the alarm is configured thru the API, well then it opens a lot of possibilities, but it's also a lot more work too. And there might be some cases where it is more misleading than anything. There are cases where a clear error message to the user is better than trying too hard. We can even put in the documentation typical errors and how to configure the panel or the integration to enable it.
So, to summarize, if it was only me I'd have:
- An option for quick arm, disabled by default, with text explaining that not all alarms will accept it
- An option to enable night mode, disabled by default (for both DSC and HW), explaining that these always require a code
- I would set
AlarmControlPanelEntityFeature.ARM_NIGHTbased on this one
- I would set
- An option for code, explaining that the code will be sent to the alarm when required
- I would hide the keyboard only
if (disarmed and quick_arm and not night_mode) or code - Maybe, in advanced, I would add an option "always show keyboard".
There was a problem hiding this comment.
Hey.. just FYI I haven't forgotten about this. I started having another look at it today and stumbled across this which introduced a default code to ALL alarm_control_panel entities which makes the code configuration item redundant and somewhat incompatible. There was also some interesting commentary related to the stock envisalink integration here.
Hopefully you're okay using your version for the time being while I figure out what to do here. I appreciate all your input on this.
There was a problem hiding this comment.
Yeah I'm using a local patch that's fine. And I'm also waiting for a final consensus before trying another version of this PR. Because right now I'm tempted to add the options I proposed earlier, with backward configuration compatibility in mind, but I'll do it only if we agree it's the way to go.
There was a problem hiding this comment.
About your link, this is actually why I am using your HACS, when I saw an issue like this closed with someone clearly stating the regression I had: home-assistant/core#118668 (comment), I knew I should not hold my breath until they fix it :D
There was a problem hiding this comment.
I suspect it won't get fixed there unless someone from the community submits a fix as there isn't anyone, to my knowledge, maintaining the core envisalink integration anymore.
Regarding the changes that they made, it seems the direction they've decided is to lift management of a default code up into the alarm control panel entity itself and exposed through the stock UI. So having the code in this integration (or any alarm integration) seems redundant now. I started experimenting with what to do about this. At the moment I have it such that it will use the default code in the alarm control panel entity if it is set and use the code in envisalink_new's config if it isn't set. And some warnings in the logs for people to set the code in the alarm control panel entity and remove it from this integration. And then maybe over time eventually remove the ability to set the code in this integration.
I don't know whether it's strictly necessary to resolve that before addressing your changes here but it feels like they may intersect.
Signed-off-by: David Racine <[email protected]>
At least on my DSC system, it is not required to have a code for arming the alarm; only to disarm. It does what I want if I always set this attribute to false.
I do not want HomeAssistant to be able to disarm my alarm without having to enter a user code, but I want it to be able to arm it without a code. By setting code_arm_required to false, I now can leave the "code" field in the config empty without breaking my automations that arms the alarm. Only a user entering a code can disarm the alarm, automations can't and this is what I want.
This PR adds a configurable option in the "Basic" page, right after the "Code", so the user can chose the desired behavior.This PR checks the type of panel and chose the right behavior based on if the panel supports quick exit or not.