[WIP] Add MQTT notifications #102

jbaudoux · 2019-09-01T14:18:26Z

Send MQTT messages. Same events as IFTTT can be subscribed. IFTTT and/or MQTT can be enabled.
Tested MQTT on OSPI using libmosquitto. For arduino-like, code must be extended to use pubsubclient library.
I also rewrite some IFTTT string message generation to use the standard sprintf instead of non-standard itoa. While cleaner for OSPI, I don't know if it was a good idea for arduino-like as it will maybe increase sketch size.

App PR is here: OpenSprinkler/OpenSprinkler-App#80

PeteBa · 2019-09-21T15:54:20Z

@jbaudoux , this looks great. I did something similar to add InfluxDb/Grafana support to OpenSprinkler to enable dashboard logging here.

A few things worth thinking about:

I needed to add support for multiple OS installations so you can differentiate the notifications between say Home and Cabin. I still have the commit and can PR it if interested.
I needed to play around a bit with the sequence of the code in Firmware so that notifications were generated in a logic order. For example, ensuring that a Reboot notification happens before the first Weather notification. Similarly, ensuring that Station Stop happens before the Station Start of the next station in the program.
I adopted a design pattern that assumed the receiving Client was "stateless" and therefore I sent all associated information with a notification i.e. Station Stop notification included start and end time, duration, flow rate, volume, station name ... so that the Client could create meaningful messages for the user
I worried a lot about timeouts when sending notifications as you could end up blocking the main loop if, for example, the destination client was unavailable for some reason.
Putting the above points to one side, I would strongly suggest going for a minimum viable product approach to this as notifications can be quite an intrusive change. I suspect that for even the current PR to be accepted, you would need to demonstrate support for Arduino and that would imply a compatible library and configuring mqtt server/port in the initial PR. Everything else could probably come as a subsequent PR.

Anyway if you want some help on this then happy to work with you. I would be interested in thinking through the message protocol and I have both OSPi and OS dev environments. It would be great to decommission my custom code and use out-of-the-box functionality.

PeteBa

Had a quick pass through the code and added a few comments/questions below. It is quite late here so sorry if I misinterpreted anything.

PeteBa · 2019-09-21T16:07:37Z

OpenSprinkler.cpp

+    mqtt_client = mosquitto_new(NULL, clean_session, NULL);
+    if (!mqtt_client) {
+      DEBUG_PRINTLN("CANNOT CREATE MQTT CLIENT");
+      return 1;


I think you need to continue rather than return so that start_network() can properly configure m_server with the right port settings..

PeteBa · 2019-09-21T16:07:48Z

OpenSprinkler.cpp

+    }
+    if (mosquitto_connect(mqtt_client, DEFAULT_MQTT_HOST, DEFAULT_MQTT_PORT, keepalive)) {
+      DEBUG_PRINTLN("CANNOT CONNECT TO MQTT");
+      return 1;


PeteBa · 2019-09-21T16:21:39Z

build.sh

@@ -15,7 +15,7 @@ if [ "$1" == "demo" ]; then
 elif [ "$1" == "osbo" ]; then
 	g++ -o OpenSprinkler -DOSBO main.cpp OpenSprinkler.cpp program.cpp server.cpp utils.cpp weather.cpp gpio.cpp etherport.cpp -lpthread
 else
-	g++ -o OpenSprinkler -DOSPI main.cpp OpenSprinkler.cpp program.cpp server.cpp utils.cpp weather.cpp gpio.cpp etherport.cpp -lpthread
+	g++ -o OpenSprinkler -DOSPI -DMQTT main.cpp OpenSprinkler.cpp program.cpp server.cpp utils.cpp weather.cpp gpio.cpp etherport.cpp -lpthread -lmosquitto


Suggest once you get past WIP that this becomes a default feature through -DOSPI rather than having a feature specific #define.

PeteBa · 2019-09-21T16:47:59Z

main.cpp

-    itoa(ip[i], str+strlen(str), 10);
-    if(i!=3) strcat(str, ".");
-  }
+  sprintf_P(str+strlen(str), PSTR("%d.%d.%d.%d"), ip[0], ip[1], ip[2], ip[3]);


Worth checking with Ray the backward compatibility requirement. I know in the early days that OS memory was limited and sprintf was left out. That may no longer be the case.

I agree. I did initially this change because my compiler was complaining about itoa and remembered that sprintf is better approach in C. Then I though it was maybe a bad idea for arduino/esp OS. Impact must definitely be tested.

sprintf is fine now: from firmware 2.1.9 we no longer supports legacy OS (any version prior to OS 2.3). Any current OS hardware has plenty of RAM and program space, so we are no longer limited by memory.

PeteBa · 2019-09-21T16:57:13Z

main.cpp

 }

 void push_message(byte type, uint32_t lval, float fval, const char* sval) {
+  // check if this type of event is enabled for push notification
+  byte test_type = (type != NOTIFY_STATION_ON) ? type : NOTIFY_STATION_OFF;
+  if(!os.options[OPTION_IFTTT_ENABLE]&test_type && !os.options[OPTION_MQTT_ENABLE]&test_type) return;


You replicate the bitwise & many times. Might be worth creating two bools for send_mqtt and send_ifft as it would read easier.

PeteBa · 2019-09-21T18:05:47Z

OpenSprinkler.cpp

@@ -489,6 +499,23 @@ extern char ether_buffer[];

 /** Initialize network with the given mac address and http port */
 byte OpenSprinkler::start_network() {
+  #ifdef MQTT
+  if (options[OPTION_MQTT_ENABLE]) {


I think for MQTT it makes more sense for this to be a binary enable/disable flag rather than a bitwise mask. For MQTT , the client determines which messages to subscribe to rather than the server. It is a bit different for IFTTT which is primarily used to generate text messages to the user and therefore OS is where the user does the selection.

PeteBa · 2019-09-21T18:08:01Z

OpenSprinkler.cpp

+
+void OpenSprinkler::mqtt_publish(const char *topic, const char *payload) {
+#if defined(MQTT) && defined(OSPI)
+  int rc = mosquitto_reconnect(mqtt_client);


You should check for mqtt_client != NULL as it may have failed during network_start().

PeteBa · 2019-09-21T18:42:03Z

defines.h

+#define NOTIFY_WEATHER_UPDATE  0x08
+#define NOTIFY_REBOOT          0x10
+#define NOTIFY_STATION_OFF     0x20
+#define NOTIFY_STATION_ON      0x40


Is the list still WIP ? It looks a bit inconsistent with the App PR ?

I agree with your suggestion that we can keep this list only for IFTTT and only have a global enable flag on MQTT

PeteBa · 2019-09-21T18:44:23Z

defines.h

@@ -207,6 +208,8 @@ typedef unsigned long ulong;
 #define DEFAULT_JAVASCRIPT_URL    "https://ui.opensprinkler.com/js"
 #define DEFAULT_WEATHER_URL       "weather.opensprinkler.com"
 #define DEFAULT_IFTTT_URL         "maker.ifttt.com"
+#define DEFAULT_MQTT_HOST         "localhost"


I would suggest HOST and PORT need to be configurable from App as part of minimum viable product.

PeteBa · 2019-09-21T19:19:55Z

main.cpp

+      sprintf_P(payload, PSTR("{\"count\":%d,\"volume\":%.2f}"), lval, (float)volume/100);
+    }
+    if (os.options[OPTION_IFTTT_ENABLE]&test_type) {
+      sprintf_P(postval+strlen(postval), PSTR("Flow count: %d, volume: %.2f"), lval, (float)volume/100);


Are you sure that arduino sprintf supports floats ? I remember that you had jump through hoops when it came to formatting floats and use dtostrf()

You're right https://stackoverflow.com/questions/27651012/arduino-sprintf-float-not-formatting

jbaudoux · 2019-09-23T05:47:52Z

@PeteBa Thanks for the involvement. In fact, I did this to integrate with InfluxDb/Grafana.

If you have a more advanced PR doing all this, we can replace mine with yours. Otherwise, can you make a PR on mine that improves it with those features?

I needed to add support for multiple OS installations so you can differentiate the notifications between say Home and Cabin. I still have the commit and can PR it if interested.
....
Anyway if you want some help on this then happy to work with you. I would be interested in thinking through the message protocol and I have both OSPi and OS dev environments. It would be great to decommission my custom code and use out-of-the-box functionality.

PeteBa · 2019-09-24T06:04:38Z

@jbaudoux my approach has been using direct REST calls to InfluxDb in their write protocol. Unfortunately, that is pretty niche and unlikely to get out-of-the-box support in OS. Whereas MQTT is a far more widespread protocol and is something I know Ray is interested in. Hence, I think you are on the right track with this PR.

To be of some help, I have written up the code for Firmware and App to allow configuring the broker's server and port details. The commits are on my fork and you can take a look at the following links ( Firmware and App ). It is pretty rough-and-ready but works and follows the same design approach as used elsewhere in the code for IFTTT and Weather Options. Feel free to incorporate into this PR.

In terms of multi-OS support, it might make sense to just put a placeholder, e.g. opensprinkler/site/station/..., into the "topic" for now. My guess is that Arduino support would be the most important next addition to prove out that this can work across the product line. I can take a look at that if you dont have an OS to use but I'm out traveling for the next week so may take some time.

jbaudoux · 2019-11-24T20:42:21Z

Replaced by #108

jbaudoux force-pushed the ospi-mqtt branch from 971e9b5 to 83fc2d4 Compare September 1, 2019 14:29

jbaudoux added 3 commits September 1, 2019 19:34

Add MQTT notifications

84ee09a

Add flow in MQTT station off message

655bb89

Add duration in MQTT station off message

21fa6a0

jbaudoux force-pushed the ospi-mqtt branch 2 times, most recently from 09cdad3 to 1cd8638 Compare September 7, 2019 10:51

jbaudoux added 2 commits September 7, 2019 15:25

Split IFTTT and MQTT enabled events

10a828b

Ensure MQTT is connected when publishing message

3a9dbc5

jbaudoux force-pushed the ospi-mqtt branch from 1cd8638 to 3a9dbc5 Compare September 7, 2019 14:27

jbaudoux mentioned this pull request Sep 11, 2019

Feature: MQTT notifications #103

Closed

PeteBa reviewed Sep 21, 2019

View reviewed changes

PeteBa mentioned this pull request Oct 12, 2019

Enhance MQTT notifications #108

Closed

jbaudoux closed this Nov 24, 2019

jbaudoux deleted the ospi-mqtt branch March 22, 2020 17:35

jbaudoux mentioned this pull request Mar 22, 2020

Add MQTT notifications #113

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add MQTT notifications #102

[WIP] Add MQTT notifications #102

jbaudoux commented Sep 1, 2019 •

edited

Loading

PeteBa commented Sep 21, 2019

PeteBa left a comment

PeteBa Sep 21, 2019

PeteBa Sep 21, 2019

PeteBa Sep 21, 2019

PeteBa Sep 21, 2019

jbaudoux Sep 23, 2019

rayshobby Oct 10, 2019

PeteBa Sep 21, 2019

PeteBa Sep 21, 2019

PeteBa Sep 21, 2019

PeteBa Sep 21, 2019

jbaudoux Sep 23, 2019

PeteBa Sep 21, 2019

jbaudoux Sep 23, 2019

jbaudoux Sep 23, 2019

PeteBa Sep 21, 2019

jbaudoux Sep 23, 2019

jbaudoux commented Sep 23, 2019

PeteBa commented Sep 24, 2019

jbaudoux commented Nov 24, 2019

[WIP] Add MQTT notifications #102

[WIP] Add MQTT notifications #102

Conversation

jbaudoux commented Sep 1, 2019 • edited Loading

PeteBa commented Sep 21, 2019

PeteBa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaudoux commented Sep 23, 2019

PeteBa commented Sep 24, 2019

jbaudoux commented Nov 24, 2019

jbaudoux commented Sep 1, 2019 •

edited

Loading