Elk M1 integration reports wrong keypad and hence wrong user when using virtual alarm_control_panel

[gjbadros]

The problem

When using the virtual alarm_control_panel card on an Elk M1 system with multiple Areas to arm/disarm, the area that had one of its physical keypads used last gets its changed_by information updated instead of the area that was just virtually armed/disarmed.

Environment

• Home Assistant Core release with the issue: 0.109.4
• Last working Home Assistant Core release (if known): none
• Operating environment (Home Assistant/Supervised/Docker/venv): docker hass.io
• Integration causing this issue: Elk M1
• Link to integration documentation on our website: https://www.home-assistant.io/integrations/elkm1/

Problem-relevant configuration.yaml

Traceback/Error logs

Additional information

@bdraco @gwww

It appears that the elk "IC" message always shows the last physical keypad that was touched in fields 19-20. It is that IC message that triggers updating of the changedby* info by _watch_keypad. The KC message triggers when a code is entered onto a different physical keypad, and that keypad number is what shows up in all future IC messages.

Which all begs the question: when an area is disarmed using alarm_control_panel -- i.e., without touching a physical keypad -- how is the correct area to be ascertained for an "IC" message?

It looks like the right fix may be to make the elkm1_lib/areas.py arm() function save the area number being attempted and then have keypads.py's ichandler grab that area number explicitly in addition to the keypad. Then the hass alarm_control_panel.py updates per-elk-instance fields for all the _changedby* information whenever it gets on IC message (via _watch_keypad which is now no longer area-specific). And then when the arm/disarm action actually takes, the most recent _changed_by information is assumed to apply. But this approach seems to have a race condition between multiple virtual control panel users or a virtual panel user and physical panel attempts.

If there were a way to assign the virtual panel attempted arm/disarm to a faux keypad that would get reported back by Elk, that'd be a better way to do this. Or a way of looking up the code in the user list directly and updating the state off that rather than off the IC message's claimed keypad index (which, again, is wrong when the virtual panel used is for a different area than the last physical keypad used).

Ideas?

[bdraco]

@gjbadros To be clear, you are talking about arming/disarming from the Home Assistant UI and not an external integration?

[gjbadros]

Correct, the alarm_control_panel lovelace card. Or via a service call in hass.

On Wed, May 6, 2020 at 4:42 PM J. Nick Koston notifications@github.com wrote:

@gjbadros https://github.com/gjbadros To be clear, you are talking about arming/disarming from the Home Assistant UI and not an external integration?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/home-assistant/core/issues/35310#issuecomment-624947730, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALOHTMGZIMCJRLK2RHETSTRQHYXFANCNFSM4M23RQTA .

[bdraco]

event.context.user_id is available in the event which is the home assistant user that is associated with the event.

I'm not away of a good way to get a name out of that or a way to get the data into changed_by

If you can figure a way to make it viable, I'm more than happy to review a PR.

[gjbadros]

But that's not what I want either. It should be the name of the elk user who corresponds to the code that was entered in the alarm card panel not the name of the home assistant user who interacted with the alarm card panel.

On Wed, May 6, 2020, 5:00 PM J. Nick Koston notifications@github.com wrote:

event.context.user_id is available in the event which is the home assistant user that is associated with the event.

I'm not away of a good way to get a name out of that or a way to get the data into changed_by

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/home-assistant/core/issues/35310#issuecomment-624952745, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALOHTPUSEEIPAEU4XLVRNDRQH2YRANCNFSM4M23RQTA .

[gwww]

I've been poking at the code. No obvious answer yet. I'll keep looking. Maybe post on the Elk thread on the forum, see if anyone else has seen this and if they have thoughts.

[bdraco]

I dug though the ascii protocol guide and didn't see anything that I think would help. Elk used to have a java web app where you could assign it to a keypad that might be a source of information if I can find it. Also might be useful to see how isy does it

[gjbadros]

Ok, I think I figured it out: there's an LD response from the Elk M1 system -- Log Data. That appears to have all the information we need all in a single bundle!

elk Wed May 06 2020 21:18:03 GMT-0700 (Pacific Daylight Time) any msg: {"commandCode":"LD","raw":"....","length":28,"direction":"from","message":"Log data with index","dataRaw":"117300312118050600042000","data":{"event":1173,"eventName":"AREA ARMED","eventData":3,"area":1,"hour":21,"minute":18,"month":5,"day":6,"logIndex":0,"dayOfWeek":4,"dayOfWeekName":"Wednesday","year":2020,"eventDataText":"User 3"},"time":"2020-05-07T04:18:03.418Z",...}

The "eventData" is the user id. I don't know why it doesn't report eventDataText with the username from the Elk M1 database, but we can look it up based on the map already in the elkm1 hass integration.

[gjbadros]

Looks like LD needs to be decoded in the Elk library (gwww?) and then we hook off of those messages to update the changed_by information. We shouldn't use the IC packets for anything, I think. It looks like they're mostly meant as a way to provide your own user database, but the fact that errant user codes are put in the clear on the wire is just nuts -- presumably they're a short edit distance from an actual user code such that snooping them would be very helpful to a perp.

[bdraco]

https://www.elkproducts.com/_literature_63631/ELK-IP232_Manual

The “System Log Data Update” transmission option, transmits the updated status whenever it changes and is enabled by setting the location TRUE in the M1 Control Global Programming Locations 35 Looks like it has to be turned on

[gjbadros]

Sure, and I think that if it's not on, we probably should not support changed_by attributes because they're subject to awful race conditions that make it risky to do anything with the result. I'm not sure that there's much/any downside to the LD option being turned on.

[gjbadros]

We just document that those attributes are populated if and only if the option is turned on in the Elk M1 globals settings.

[bdraco]

It makes sense to document that changed_by is only reported for changes made on a physical keypad but I don't think we should remove functionality users are currently relying on because it isn't aware of virtual keypads.

[gjbadros]

I don't think it's even reliable for physical keypads due to the race condition -- two people at two separate keypads, one arming an area and one disarming another area at virtually the same time, the order of the two KC messages and two IC messages aren't guaranteed as I understand it. Only the log messages provide the full precise understanding of what the Elk system interpreted.

"Be aware that data is transmitted from the M1 asynchronously so the message received immediately after a command may be unrelated to the response you are expecting." from page 8 of rev 1.84 of the RS-232 docs for M1.

Now you could argue that the IC responses containing the keypad as part of its response solves this problem, but it doesn't since the Elk M1 generates IC responses for service-call (and hence virtual keypad) disarms/arms. That means that you can never trust the keypad # in those IC responses. Even if home assistant were forbidden from making the service calls, some other system could be connected to the M1 and make those calls.

I think we just need to tell users if they want that functionality, they need to turn the logging functionality on, else those attributes don't get populated.

On Wed, May 6, 2020 at 10:06 PM J. Nick Koston notifications@github.com wrote:

It makes sense to document that changed_by is only reported for changes made on a physical keypad but I don't think we should remove functionality users are currently relying on because it isn't aware of virtual keypads.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/home-assistant/core/issues/35310#issuecomment-625031433, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALOHTMACKEMY5OB3MZU6KDRQI6UHANCNFSM4M23RQTA .

[bdraco]

There is a pretty active thread about elkm1 here: https://community.home-assistant.io/t/elk-m1-interface/4461/865

If after implementing LD support, the community has general agreement that removing changed_by if LD is disabled then it would seem ok to remove.

[frenck]

Thank you for reaching out. We use GitHub for tracking issues, not for providing support or tracking feature requests.

I noticed @bdraco tagged this issue as a feature-request, however, that is not allowed and this issue should be closed if it is truly a feature request.

If you want to suggest a feature, you should try our Community Forum: Feature Requests.

If you have additional questions, feel free to join our Discord chat server.

Thanks! 👍

[gjbadros]

@bdraco yea, you're ahead of me in thinking about the transition period. I think it's pretty bad to leave this bug in now that we know about it... it can make it look like a user who has access to only one area actually was the user who gained access to another area (e.g., physical keypad at main house gets used, then guest house only user uses virtual keypad to enter the guest house area, and all of a sudden main house alarm control panel shows the guest house user changed the status of the main house).

However, there is a special case that may make a transition period easier: any setup with only one area can keep using the existing mechanism. So I propose:

1) elkm1 library gets updated to support LD reporting with hook (gwww?) 2) elkm1 hass module keeps working as if if no more than 1 area is config'd 3) elkm1 hass module requires logging be turned on if more than 1 area is config'd and uses only the new LD message support to get these changed_by* attributes correct.

Thoughts?

[bdraco]

However, there is a special case that may make a transition period easier: any setup with only one area can keep using the existing mechanism. So I propose:

1. elkm1 library gets updated to support LD reporting with hook (gwww?)
2. elkm1 hass module keeps working as if if no more than 1 area is config'd
3. elkm1 hass module requires logging be turned on if more than 1 area is config'd and uses only the new LD message support to get these changed_by* attributes correct.

Thoughts?

That seems like a reasonable plan. This isn't the right venue for this discussion though since we are talking about adding new functionality (even if its correcting an underlying design limitation). We should discuss out of band on the forum or in a PR once one is opened.