Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Notifier - Extending integration options and flow related notifications #42

Closed
wants to merge 6 commits into from

Conversation

PeteBa
Copy link
Contributor

@PeteBa PeteBa commented Sep 25, 2017

This PR (and the associated PR for Firmware) addresses three objectives with OpenSprinkler:

  • Extend notification integration to get real-time OS event/usage into InfluxDb
  • Get water usage at the Station and Program level rather than across a combined run
  • Provide monitoring of flow rate to allow triggering of alarms if too high (i.e. leak/cut pipe)

During the process of implementing the above, the following have also been included:

  • Specifying endpoints for IFTTT triggers so that you can send some events to, say, a spreadsheet log, and other to a phone notification
  • Adding richer data to the notification messages i.e. Station End event also contains start/finish times, water level used, water volume usage and average flow rate
  • Adding a generic WebHook interface for notifications that other services could hook onto. This is only “outbound” notifications and not an api.
  • Allowing a “name” to be assigned to the unit that is sent with the notification to aid identification in multi-OS environments

In order to implement the above, the PRs have made a number of significant changes to both Firmware and App that require review:

  • Generalized the IFTTT notification handler and UI to support multiple destinations (IFTTT, InfluxDb, WebHook)
  • Moved much of the scheduling code from main.cpp to program.cpp to allow pre- and post- process of station and program scheduling
  • Added flow volume to be apportioned across each of the running stations (done in a uniform distribution)

In the commits below, I have provided more detail/questions on the specific changes. Appreciate your feedback and happy to take any modifications on-board.

@PeteBa
Copy link
Contributor Author

PeteBa commented Sep 25, 2017

Samer, there are a few questions related to these changes that I wanted to confirm with you:

  • EscapeJSON - I changed the json filter to allow nested objects for more complex settings. I want to confirm this is appropriate and doesn't have unforeseen implications
  • Option Processing - I have added new options into submitOptions() routine but could do with a sanity-check that I've done it correctly (gets a bit tricky with the different version of setting names i.e. oXX vs "tag" variants)
  • Import/Export - I haven't had to do anything additional to support import/export as it all appears "auto-magically"! Is that a fair assumption?
  • Site Names - Do you think we could align Unit Name with Site Name ? I don't know enough about the Site Name use case to know if it is worthwhile but it seems possible looking at the code. Thoughts?

@salbahra
Copy link
Member

Thanks for submitting the PR and accompanying information! I looked over it and it all looks good. To answer your questions:

  • The escapeJSON changes seem appropriate and I do not foresee any issues with that change
  • The options are super confusing and I have discussed this with Ray. It's hard to do much about this at the moment since it could break prior compatiblity. I think you have everything in there correctly. Another thing I badly need to do is refactor this code for the era of webpack so it's small logical files instead of a giant main.js. One day...
  • The import/export should be auto detecting everything before using it for compatibility with the target firmware and also converting any options it knows how. However, in order for it to know exists it has an object named keyIndex in the importConfig function on line 9406 which maps the options. For data stored in the settings object, I believe you will need to add a line to copy it over in the import function (with some sort of feature detection) such as this example: https://github.com/OpenSprinkler/OpenSprinkler-App/blob/master/www/js/main.js#L9147-L9150
  • For the unit name, I think using the already present site name would be better since it likely is the same name being used and already present in the app. Is there a reason to have a separate name?

I am going to await your feedback on the unit/site name and about importing a config with InfluxDB data before I merge this in but otherwise looks great, thanks again!

@PeteBa
Copy link
Contributor Author

PeteBa commented Oct 2, 2017

Samer, thanks for the feedback on all of this.

I have introduced the necessary import/export routines to cover the new settings options. I have used a minor version bump of Firmware to identify whether the importing Controller can support the new options.

I have also aligned on Site Name as the identifier used in notifications. The code in App that manages this setting assumes that the "master" reference is held in browser storage rather than sourced from Firmware which is a bit different to all the other settings. I have continued with this design pattern and look to ensure Firmware is kept in sync with any changes. I trigger Firmware sync (i.e. sendToOS call) in the newLoad() routine once App has confirmed Controller is connected and we can determine if the Site Name needs updating. Appreciate your thoughts on this.

I have yet to hear back from Ray so appreciate if we hold off any merge till I get a steer on the Firmware side of things. Thanks again for the support.

@salbahra
Copy link
Member

salbahra commented Oct 3, 2017

Thank you for your changes! I will wait until I hear from you regarding the firmware changes to merge this PR however it all looks good!

Thank you again!

@salbahra salbahra force-pushed the master branch 2 times, most recently from 98192e8 to 23b7659 Compare December 3, 2017 15:00
@PeteBa
Copy link
Contributor Author

PeteBa commented Jan 31, 2018

Closing as Firmware PR is stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants