fdegier / homebridge-jablotron-alarm

Homebridge plugin for connecting Jablotron JA-100 alarm systems to Homekit.
The Unlicense
26 stars 6 forks source link

Alarm trigger #68

Closed fdegier closed 10 months ago

fdegier commented 2 years ago

Support for triggered alarm. I have tested it an come to the following observations. I have setup a spare Jablotron central, it has no modules connected and the lid is off, therefore it is continuous tamper alert. I have added both alarm and tamper as states for a triggered alarm.

The good:

The bad:

My thoughts and ask for help:

@misncz @grzegorz914 can you check the code and chime in on the issues described above?

I can share the Jablotron service so you can play around with it.

fdegier commented 2 years ago

IMG_2346 IMG_2347

fdegier commented 2 years ago

Closes #40

fdegier commented 2 years ago

@grzegorz914 and @misncz can you please check my PR?

misncz commented 2 years ago

There will be conflicts with thermometer changes

fdegier commented 2 years ago

@misncz I have resolved the merge conflicts. Since there is now a third value in the callback which for a triggered alarm is not available, I have just added an empty string. Is this OK? Do you mind checking the merge result, especially in jablotron.js?

misncz commented 2 years ago

Will have a look in the morning. Instead of empty string pass null ( For thermomemters it’s an array of information objects)

misncz commented 2 years ago

jablotron.js changes broke thermometer and PGM (sirene or switch). Need to have a look

misncz commented 2 years ago

Change to parseResponseData method broke getAccessoryStates method that now loop only through first data type (ie sections) and ignores PGM & thermometers - it's something I'm able to fix Also getAccessorState looks suspicious to me as it calls callback with 'triggered' but doesn't check accessory key. Also call to callback needs 2 params callback('triggered', null)

misncz commented 2 years ago

@fdegier I pushed a fix getAccessoryStates and tried to modify getAccessoryState the same way as getAccessoryStates. Now it should be fine. Please test alarm triggering now

fdegier commented 2 years ago

Thank you, good improvements!

I've tested it but now it gives an error that I fixed earlier:

[2/27/2022, 10:58:41 AM] [homebridge-jablotron] This plugin generated a warning from the characteristic 'Security System Target State': characteristic was supplied illegal value: number 4 exceeded maximum of 3. See https://git.io/JtMGR for more info.

This happens because it tries to set the target state to triggered which isn't a target state, only a current state. I'm investigating the exact cause now.

fdegier commented 2 years ago

This part is the cause for the warning:

if (fullReport && state != this.getTriggeredStateMapping()) {
                     this.updateServiceCharacteristic(Characteristic.SecuritySystemTargetState, state);
                 }

So state is probably incorrect.

fdegier commented 2 years ago

I have done some debugging but I couldn't find the culprit. I do notice:

  1. The triggered "state" seems instant now
  2. Notifications no longer work

@misncz shall I give you access to the Jablotron that is always triggered so you can debug the issue?

MartinKristof commented 1 year ago

Could be merged please?

MikeFParkin commented 1 year ago

Does the alarm triggering still not work? ☹️ Will this issue ever be fixed/merged?

fdegier commented 1 year ago

It will, eventually. It was ready to be merged but there is now a conflict that breaks the functionality. Besides that we need to upgrade the API version which has a higher priority and will break more things.

MikeFParkin commented 1 year ago

It will, eventually. It was ready to be merged but there is now a conflict that breaks the functionality. Besides that we need to upgrade the API version which has a higher priority and will break more things.

No problem, thanks for the update - and for all the hard work you, and the team, put into making this plugin a reality!

fdegier commented 10 months ago

Closing this PR as the functionality will be picked up in a new PR that is on par with the new API version.