home-assistant / architecture

Repo to discuss Home Assistant architecture
313 stars 100 forks source link

Add support for custom delay time for manual alarm #458

Closed tomer-w closed 1 year ago

tomer-w commented 3 years ago

Context

Manual alarm has a limitation that the delay time - the time between sensor is activated to alarm triggered cannot be configured differently for different sensors. The limitation is coming from the need to use the alarm_control_panel.alarm_trigger service to trigger the alarm and there is no way to differentiate between the different use cases. For example, door opening should wait for 30 seconds before trigger while window should fire immediately. The delay time should not be only configured by the different alarm states but also by the automation which trigger it and knows if the sensor is one which should be acted immediately or one which should allow some time to disarm it.

Proposal

The change I propose here is to add additional parameter to the alarm_control_panel.alarm_trigger service which can specify the actual delay needed for this invocation. This allow the customer to have different automations use different values based on their scenario instead of picking up the default value stored in the configuration.

Consequences

With this change it will now be possible to configure the windows sensor to trigger the alarm immediately while let the user have 30 seconds to disarm the alarm after getting in from the door.

Pull request

https://github.com/home-assistant/core/pull/41657

tomer-w commented 3 years ago

Any chance to get someone to look at this one? I think it is really minor change which will take the manual alarm integration forward.

elupus commented 3 years ago

Somewhat related to: #477

adampetrovic commented 3 years ago

Can we get an update on this please? It's been sitting in pending for almost 6 months and is a pretty simple semantic change that doesn't impact existing configurations and adds an important feature that is missing from the current manual alarm panel implementation.

frenck commented 3 years ago

Well, this adds a parameter for a verify specific goal to the entity model. We generally only accept these changes, if multiple existing platforms could benefit from them.

Besides, the manual control panel example, which other integrations would support this?

Additional; I don't see how could not be resolved with the existing automation capabilities? (e.g., using a delay and run mode single).

adampetrovic commented 3 years ago

It's not about the fact that I can build a workaround to this problem, it's that a core component is provided by Home Assistant that manages that state machine for me. The more custom automation workflows I have to introduce, the higher the chances I misconfigure the security of my house.

"The manual alarm control panel platform enables you to create an alarm system in Home Assistant."

Almost every alarm system in existence has the notion of setting a zone to be one of the following types:

Is there a solution to this that doesn't violate the architectural concerns you have? I'm all ears.

If it means moving this forward quicker, the last zone type can probably be ignored.

frenck commented 3 years ago

We are not building Home Assistant for all edge cases; thus not all possibilities are accepted or added to our entity components. Hence my previous question on:

Besides, the manual control panel example, which other integrations would support this?

In order to justify the addition of new options to the global entity model (as used by all alarm integrations). This would require research into existing integrations the provide alarm control panels and which ones could benefit/implement the suggested addition/change.

I see your point in your last message, but it is only about the manual alarm panel and did not answer the question I had.

adampetrovic commented 3 years ago

Honestly, if you think that the notion of Instant vs Delay zones is an edge case in the design of any basic alarm system I would question whether you've ever owned an alarm system.

I'll look into ways we can get this to move forward

adampetrovic commented 3 years ago

This would require research into existing integrations the provide alarm control panels and which ones could benefit/implement the suggested addition/change.

Wouldn't this be, by definition, all of them? As this feature is not possible right now assuming they use the alarm panel component

frenck commented 3 years ago

I would question whether you've ever owned an alarm system.

Actually, I do, several, for many years. Even commercially installed a couple of large systems as a side gig.

Currently rewriting the Verisure integration that would not benefit from this change; as it cannot use such a parameter and does not allow remote triggering either.

Nevertheless, there might be others.

adampetrovic commented 3 years ago

All of the provided Alarm integrations have their own workflow for handling "triggered" alarms. They almost all exclusively use the alarm panel state machine to handle the armed and disarmed states and never use the triggered state to denote that an alarm has been triggered (they all opt to send an event instead).

Given this is the case, and the goal of the Manual Alarm Panel is to provide an end-to-end solution to implementing an alarm system, would you agree that it's perfectly reasonable to discount the implementation details of these other alarm systems? I don't think we're comparing a like for like design goal here.

frenck commented 3 years ago

an end-to-end solution to implementing an alarm system, would you agree that it's perfectly reasonable to discount the implementation details of these other alarm systems?

Sure, but the proposal is to add it to the main alarm control panel base entity component, thus effects all integrations. Hence, this discussion is not about just your manual alarm control panel example.

adampetrovic commented 3 years ago

Can you clarify this logic?

As far as I can tell, none of these other alarm integrations make use of the alarm PIN code feature either which is an attribute on the base alarm panel component service schema. Should this be moved to the manual alarm panel component too?

None of them make use of the underlying service calls "arm_custom_bypass" either.

frenck commented 3 years ago

We have lot of history, however, that does kot justify decisions for today.

As for that matter, let's keep focused at the current requested added feature and how it can be used across the available integrations.

That is the main requirement that needs to be answered.

adampetrovic commented 3 years ago

Agreed, let's move on.

To give a generic example of how I imagine this could be useful:

Standard Alarm Integration Scenario As a HASS user, I have an alarm system that has a direct integration with HASS. I would like to use the state of my alarm system at any given time in order to send myself push notifications on important system events (e.g. armed status, zone triggered (delay), siren sounding, tamper detection etc). If someone breaks in through my window, my physical alarm system is configured such that this event would cause the alarm to sound immediately. Given the developer of my alarm integration has no way of differentiating these two states, they have no choice but to call "trigger" on the underlying base alarm panel which then takes the armed state entry delay configuration of "15 seconds" (as an example). Ideally, they would be able to move the state machine from "armed" directly to "triggered" and thus override this global delay. In the case where the alternate configuration is applied (no global delay), they could move the state machine from "armed" to "pending" until the "alarm sounding" event was received in which case the state machine would then move to "triggered". These implementation details are nicely hidden from the end-user. It also means I could implement support for special cases like tamper detection which currently would be delayed by up to whatever the configured entry delay is for that state. I want to know as soon as possible if someone tries to rip down my siren from the roof, not have to wait 15 seconds to receive that event.

I want to implement my own alarm system scenario: Say, I have a bunch of spare alarm parts (motion, door sensors and perhaps an external siren) hooked up to relays / potentiometers that allow me to expose their state to home assistant as binary_sensors and switches. I don't have any physical alarm panel or keypad, but I'd like to use their states to configure my own alarm system (using the manual alarm panel). It would be great if I could specify as part of the configuration all of the zone inputs I'd like the alarm system to use as triggers for the state machine. Some inputs may have no delay, others may. This way, I can build my own automations that react to changes in the state machine. If someone breaks in through my window, I can move the alarm panel directly from "armed" to "triggered". If I push the panic button on my remote, I can move directly from "any" to "triggered" then hook up my siren to sound any time the alarm panel is in the "triggered" state.

Ultimately, what we're asking for here to allow the "trigger" service call to enable the state machine to move to the "triggered" state by overriding the underlying default configuration which only allows delays to be specified depending on the state the alarm is in. There are legitimate scenarios where this isn't ideal.

Let me know what you think.

frenck commented 3 years ago

Honestly, the above scenario can be solved in automations with ease. We have run modes and delays that make this already possible and if one goes fancy, some input helpers can help with more advanced scenarios even.

Nevertheless; as stated a couple of times now: If this is added to the alarm entity component, it needs to be justified that multiple integrations can benefit from this. This question seems to be avoided and is the only relevant question. So please:

Besides, the manual control panel example, which other integrations would support this?

Please provide a list of integration that would support this service call and can actually consume this parameter in their upstream APIs.

MartinHjelmare commented 3 years ago

There are currently only four integrations (out of 30) that supports the trigger alarm service:

frenck commented 3 years ago

That's limited.

Maybe such a service could be specifically implemented for the manual alarm only?

tomer-w commented 3 years ago

I have to say that not approving this change completely block the usage of this integration in any real world scenario. Adam is right here. I had to move to use the excellent https://github.com/nielsfaber/alarmo custom component. If we don't want to approve this change I think the best thing to do is to remove this manual alarm altogether from HA and state that alarmo is the recommended path going forward.

frenck commented 3 years ago

I have to say that not approving this change completely block the usage of this integration in any real world scenario. Adam is right here.

So let me get this clear here. The current PR linked to this architecture discussion changed the base entity model for the alarm control panel. Thus a shared resource/model across all integrations.

As I said in my last response:

Maybe such a service could be specifically implemented for the manual alarm only?

This is merely suggesting on implementing more local for the manual alarm control panel integration specifically instead.

If we don't want to approve this change I think the best thing to do is to remove this manual alarm altogether from HA and state that alarmo is the recommended path going forward.

Not sure how that response matches up or why that is even needed to state, as there is no door closed at all? 🤷

tomer-w commented 3 years ago

@frenck, it needs to be stated as poor people like Adam or me spent lot of time trying to get HA built in alarm working only to later find out that what we currently have in HA is not good enough even to basic use cases. It will better for everyone if we will point people who are looking for such solution to the best approach which is currently available and give better customer experience.

If there are any specific technical changes in the PR which will make the change more specific and remove the issue you have with approving such improvement, than we should do that. I lost interest for now with this is as alarmo is just way better solution but maybe Adam will want to drive that.

frenck commented 3 years ago

If there are any specific technical changes in the PR which will make the change more specific and remove the issue you have with approving such improvement, than we should do that.

Yes, I gave a different solution, and quoted it in my previous message. It can be implemented as a service (and option) specifically for the manual alarm control panel; as that would not modify the entity component.

I lost interest for now with this is as alarmo is just way better solution but maybe Adam will want to drive that.

Fair, so this discussion can't lead to anything at this point? In that case, we should close these PRs and discussions.

dgomes commented 3 years ago

The title of the issue says: Add support for custom delay time for manual alarm

I see no point in this discussion, just implement the service in the manual alarm as @frenck already acknowledged

dgomes commented 3 years ago

Looking at the PR, just create a new service in the manual alarm (instead of the alarm_panel entity)

frenck commented 3 years ago

@dgomes Yeah that would work.

frenck commented 1 year ago

This architecture issue is old, stale, and possibly obsolete. Things changed a lot over the years. Additionally, we have been moving to discussions for these architectural discussions.

For that reason, I'm going to close this issue.

../Frenck