search

michbeck100 / pimatic-alarm

Alarm system plugin for pimatic
GNU General Public License v3.0
0 stars 2 forks source link

feature request: multiple include lists #3

Closed bstrebel closed 7 years ago

bstrebel commented 7 years ago

@michbeck100 thanks for the awesome plugin. Even for me, completely new to nodejs and coffescript, it looks like a very neat but powerful piece of code. Tested it at the weekend without any problems.

To completely replace my home-brewed rule based alarm system I would require a mechanism to configure multiple independent device lists. I have implemented a multi level alarm system (external PIRs switching on alarm lights) and internal contacts/PIRs to trigger the alert with siren and notifications. Furthermore, a check of the contact devices and appropriate warnings ("window xyz is open") when enabling the alarm system would be helpful.

I need a lot (really a lot!) of rules and some external python scripts to enable/disable rules which make the system hard to maintain. It would be great to have a slim and robust solution based on your plugin.

bstrebel commented 7 years ago

I' have studied your code thoroughly and tried to extended the pimatic-alarm plugin by myself. It's now possible to define multiple alarm systems with different include sets of devices (which is essential to replace my own alarm system). Code and sample configuration can be found at https://github.com/bstrebel/pimatic-alarm/tree/groups.

It would be great if you can have a look on it. I don't know, who else is using the plugin. Maybe some code to convert the old configuration to the new layout on plugin loading will be helpful.

michbeck100 commented 7 years ago

I did have a look and I think it would be better to move the config from the plugin to the AlarmSystem device instead. This way you have multiple devices of type AlarmSystem, with a list of devices to include for each of them. What do you think? I think this makes the config easier to understand.

bstrebel commented 7 years ago

I'm on the road for some hours and will reply in the evening ...

bstrebel commented 7 years ago

I also thought about something like an AlarmSystem controller device to keep the include lists and other configuration options separated at a device level. But I like the way how you initialize the system through addDevice events during the loading of the plugin. Im not very experienced in pimatic development but I think, it's difficult to wait for the sensor and actuator devices when you do the job along with the creation of the AlarmSystem device. But I'm not sure ... Furthermore I like your simple but powerfull plugin an tried to limit the code changes to a minimum ;-)

bstrebel commented 7 years ago

Added some code to convert "includes" to the alarm group "default" for legacy installations. Also modified the log messages to display the alarm group name. This way the new plugin should be fully compatible with old configurations but allows additional alarm system configuration. I would suggest to release this updated revision. Should I merge the groups branch and create a pull request?

We can then talk about the new AlarmSystem device to make the configuration more intuitive and add more functionality like, e.g. check device state (all windows closed?) when activating an alarm system ...

michbeck100 commented 7 years ago

Of course you can create a pull request. . But i would prefer to release a version where the config is already moved from the plugin to the alarm system device. I hope i have the time to look into your changes next week.

bstrebel commented 7 years ago

Developed pimatic-alert to test the device based configuration. Pls. have a look on it and let's decide how to proceed.

michbeck100 commented 7 years ago

Thats what i had in mind for your pull request. As i said, i won't have time to look into your changes before next week. But i think i will use some of your changes from the PR and rework the plugin to support a list of devices per AlarmSystem device. But you can still keep your plugin alive, thats up to you. Both plugins will do the same, though.

bstrebel commented 7 years ago

I think it doesn't make sense to have two plugins doing the same. Let's talk next week after you have reviewed the pull request (plugin based config) and the code from the test module pimatic-alert (device based config). I'm not going to publish the new plugin so far.

michbeck100 commented 7 years ago

With version 0.2.0 which includes https://github.com/michbeck100/pimatic-alarm/commit/d2e04af80c0412e7b950498fdcbdcd4bfb6ea92e i think this issue is resolved.

@bstrebel do you have any comments?

bstrebel commented 7 years ago

Again a pleasure to look at your compact code. Thanks. You may have noticed, that in the meantime I've finished my own pimatic-alert plugin that provides some additional features that I wanted to have in the plugin instead of struggling with a bunch of stupid rules.

michbeck100 commented 7 years ago

Are you planning to release this plugin to npm?

bstrebel commented 7 years ago

Yes, it's already published. Is that a problem?

michbeck100 commented 7 years ago

No not a problem. Good luck. As you already released your plugin i close this issue.