search

SignalK / signalk-zones

Signal K Node server plugin to handle zones: value ranges per Signal K key
Apache License 2.0
2 stars 5 forks source link

Multiple changes #5

Closed sbender9 closed 8 years ago

sbender9 commented 8 years ago

Too lazy and tired to split these up into multiple pull requests.

Added [visual, sound] method to notifications Added ability to silence alarms Changes lower check to be <=, otherwise there will be values that don't fall in a zone

sbender9 commented 8 years ago

Typo, lower check is >=

              return value => value < zone.upper && value >= zone.lower
tkurki commented 8 years ago

(1) The alarm method is already configurable (I commited it accidentaly when fixing the firing): https://github.com/SignalK/signalk-zones/blob/8236b985a72b58e3a0683df0960a3c1f145e0b1d/index.js#L70-L76 and this change would override that totally. If the method is hard coded to the state then it doesn't really play any role and should be removed from the schema. I suggest we forget this. How about a default value for it in the UI (plugin and WilhelmSK)?

(2) I do not think that silencing the notification should be part of this plugin's API, but we need to define the way to do it using the normal Signal K http api. You should be able to POST a new value to the method in the full model via HTTP and that should send a delta out to notify all clients of the change.

In fact this goes for the whole zones information - now this plugin and I assume WilhelmSK as well is using the plugin's API to modify the zones. This is not in any way part of the Signal K API. The way to do it would be to POST the zones json to the relevant path in the full model.

(3) I'll fix the lower condition separately.

OK?

sbender9 commented 8 years ago

All sounds good. We just need to make the plugin set the method value based on what's in the zone info, right?

tkurki commented 8 years ago

Silencing is really just changing the notification method, right?

So when you silence the audible alarm (but leave visual on) you would POST ['visual'] to /signalk/v1/api/vessels/self/notifications/navigation/speedOverGround/method.

Is this what you meant with your last comment?

sbender9 commented 8 years ago

No, but my comment was addressed with commit 2993ce7