search

aficustree / homebridge-alarmdecoder-platform

Homebridge plugin for the AlarmDecoder interface for Honeywell and DSC Alarm Systems. Exposes the security system and all zones (via Contact Sensors and Motion Sensors) to Apple's Homekit. Control your alarm through Siri.
Apache License 2.0
18 stars 11 forks source link

A few improvements and quite a bit of cleanup #7

Closed jondthompson closed 4 years ago

jondthompson commented 4 years ago

It appears that this revision has a few logical tweaks, as well as removes some extraneous whitespace, and would be beneficial to merge into the official version that is on nom.

aficustree commented 4 years ago

a couple questions @jondthompson @codekitchen

  1. The call to initPlatform does have a race condition, we should use the async pattern though to stay aligned to the rest of the codebase vs. the .then() semantics
  2. Why do you want to poll? Both the Honeywell and DSC will emit the full status message anytime a sensor is tripped, any state is changed or on-boot and the full status message is parsed to realign state on the chance that it's gone out of sync. Polling will just consume CPU cycles
  3. I didn't even really understand the advantages/disadvantages of dynamic platform. You seem to want to turn dynamic off. Any reason?
codekitchen commented 4 years ago

Oh some of this was to fix quirks in my specific setup, which is why I didn't pursue getting it merged back in. @jondthompson must have done some digging to find it :) But I'll try to answer your questions...

  1. Yeah that'd make sense.
  2. I originally implemented polling because of an annoying problem with my home automation setup, where I couldn't have a two-way network config dependency between my homebridge container and my alarmdecoder container. I've since fixed that so I don't use the polling anymore. I agree it doesn't seem generally useful.
  3. I made this change a while ago but if I recall, I couldn't even get homebridge-alarmdecoder-platform to boot up and initialize properly when it was a dynamic platform, on whatever version of homebridge that I was using at a time. It seemed like the dynamic platform API had changed and making it not dynamic was the simplest solution for me at the time. I've upgraded homebridge a few times since then so I really don't know if I'd be able to reproduce at this point. I wouldn't suggest merging that change in if nobody else is seeing that issue.
aficustree commented 4 years ago

removed the race condition on initPlatform, left the platform dynamic and tidied up logging, pushed to dev and master