search

Anamico / node-red-contrib-alarm

Nodes to build your own home alarm system. Designed to work easily with (but does not require) homekit.
MIT License
24 stars 9 forks source link

Bug in Persist State Similar to #19 #20

Closed thk-socal closed 11 months ago

thk-socal commented 3 years ago

Commented out SecuritySystemTargetState setting in panel.js near the "// persist the new state" section causes the same hang up as bug #19. Causes Homekit to hang on state change. I edited the code as below for a quick fix for my situation, but I think there needs to be some checking on my change.

image

thk-socal commented 3 years ago

The more I think through this...I should be able to send a SecuritySystemTargetState command to the Alarm panel at the start of a process, and then send the SecurityStateCurrentState at the end of the process. That way I get the "Arming..." interface in Homekit and if I interrupt the process, like for an open door, the Arming process is cancelled and returned to the previous state without success.

thk-socal commented 3 years ago

Reviewing the code, looks like there needs to be a global variable created for the SecuritySystemTargetState and then logic adjusted around that. The code fixes just assign it to the CurrentState, which is not the intent of the code, but gets around the error for now. This is more than a couple of lines of fixes and still learning all the areas of the code before recommending a change. Might have time to tackle some of it this weekend if you do not get to it before then. Pretty nice node-red module for a custom alarm, just a few bugs to work through to really get it solid in Homekit!

macinspak commented 3 years ago

Thanks. And thanks for looking into this enhancement.

The original intention was pretty simple, just return the new state based on the request. It met the main need to have the alarm arm and disarm instantly.

With some enhancement, then the behavior you suggested sounds like it could be useful. Would just need to make sure we can do it without causing unintended side effects like timeouts or state confusion in fast switching. But what you suggested sounds great to me. If you want to do that in a fork and submit a pull request we can get the library even better.

Regards, Andrew

On Thu, 4 Mar 2021 at 5:27 pm, cscott365 notifications@github.com wrote:

Reviewing the code, looks like there needs to be a global variable created for the SecuritySystemTargetState and then logic adjusted around that. The code fixes just assign it to the CurrentState, which is not the intent of the code, but gets around the error for now. This is more than a couple of lines of fixes and still learning all the areas of the code before recommending a change. Might have time to tackle some of it this weekend if you do not get to it before then. Pretty nice node-red module for a custom alarm, just a few bugs to work through to really get it solid in Homekit!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Anamico/node-red-contrib-alarm/issues/20#issuecomment-790386092, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGHK2LDVIKV2DITYKPFOPSDTB4Y4PANCNFSM4YSSHX7A .

thk-socal commented 3 years ago

Trying to stay with the original intent then, which I completely agree with, I did the work in node-red and just used the modified code above to make sure that this alarm code was sending both states. My node-red flow looks like this now:

image

So after setting the SecuritySystemTargetState setting with the recommended changes above, I was able to get everything to arm and disarm without causing errors. If you just want to look at making that change cool, if you want me to fork and then submit a pull for just that change let me know.

For future, I will start to consider how the panel might have more inputs, but also how not to complicate it and make it too hard for others.