Merge pull request #900 from adafruit/tyeth/issue899

fix(ping): Avoid Device Connection Flipping, KeepAlive Fixup

Author: tyeth

src/Wippersnapper.cpp

@@ -1735,27 +1735,40 @@ void cbThrottleTopic(char *throttleData, uint16_t len) {
   (void)len; // marking unused parameter to avoid compiler warning
   WS_DEBUG_PRINT("IO Throttle Error: ");
   WS_DEBUG_PRINTLNVAR(throttleData);
-  char *throttleMessage;
-  // Parse out # of seconds from message buffer
-  throttleMessage = strtok(throttleData, ",");
-  throttleMessage = strtok(NULL, " ");
-  // Convert from seconds to to millis
-  int throttleDuration = atoi(throttleMessage) * 1000;
-
+  uint32_t throttleDuration = 60000UL; // duration of throttle in ms
+  bool parsingSuccessful = false;
+  if (throttleData != NULL) {
+    char *throttleMessage;
+    // Parse out # of seconds from message buffer
+    throttleMessage = strtok(throttleData, ",");
+    if (throttleMessage != NULL) {
+      throttleMessage = strtok(NULL, " ");
+      if (throttleMessage != NULL) {
+        // Convert from seconds to to millis
+        throttleDuration = (uint32_t)atoi(throttleMessage) * 1000UL;
+        parsingSuccessful = true;
+      }
+    }
+  }
+  if (!parsingSuccessful) {
+    WS_DEBUG_PRINTLN("ERROR: Unable to parse throttle duration from message, "
+                     "please report this! Defaulting to 60s.");
+  }
   WS_DEBUG_PRINT("Device is throttled for ");
   WS_DEBUG_PRINTVAR(throttleDuration);
   WS_DEBUG_PRINTLN("ms and blocking command execution.");
 
   // If throttle duration is less than the keepalive interval, delay for the
   // full keepalive interval
-  if (throttleDuration < WS_KEEPALIVE_INTERVAL_MS) {
-    delay(WS_KEEPALIVE_INTERVAL_MS);
+  if (throttleDuration < WS_DEVICE_PING_MS) {
+    delay(WS_DEVICE_PING_MS);
   } else {
-    // round to nearest millis to prevent delaying for less time than req'd.
-    float throttleLoops = ceil(throttleDuration / WS_KEEPALIVE_INTERVAL_MS);
+    // Round up so throttling never ends earlier than requested.
+    uint32_t throttleLoops =
+        (throttleDuration + WS_DEVICE_PING_MS - 1) / WS_DEVICE_PING_MS;
     // block the run() loop
     while (throttleLoops > 0) {
-      delay(WS_KEEPALIVE_INTERVAL_MS);
+      delay(WS_DEVICE_PING_MS);
       WS.feedWDT();
       WS._mqtt->ping();
       throttleLoops--;
@@ -2474,7 +2487,7 @@ void Wippersnapper::runNetFSM() {
       fsmNetwork = FSM_NET_CHECK_NETWORK;
       break;
     case FSM_NET_ESTABLISH_MQTT:
-      WS._mqtt->setKeepAliveInterval(WS_KEEPALIVE_INTERVAL_MS / 1000);
+      WS._mqtt->setKeepAliveInterval(_brokerKeepAliveIntervalSeconds);
       // Attempt to connect
       maxAttempts = 5;
       while (maxAttempts > 0) {
@@ -2599,9 +2612,8 @@ ws_board_status_t Wippersnapper::getBoardStatus() { return WS._boardStatus; }
 */
 /**************************************************************************/
 void Wippersnapper::pingBroker() {
-  // ping within keepalive-10% to keep connection open
-  if (millis() > (_prv_ping + (WS_KEEPALIVE_INTERVAL_MS -
-                               (WS_KEEPALIVE_INTERVAL_MS * 0.10)))) {
+  // if it's past time to send the next ping
+  if (millis() > (_prv_ping + WS_DEVICE_PING_MS)) {
     WS_DEBUG_PRINT("Sending MQTT PING: ");
     if (WS._mqtt->ping()) {
       WS_DEBUG_PRINTLN("SUCCESS!");
@@ -2795,6 +2807,8 @@ void Wippersnapper::connect() {
   // Dump device info to the serial monitor
   printDeviceInfo();
 
+  _brokerKeepAliveIntervalSeconds = WS_BROKER_KEEPALIVE_MS / 1000;
+
   // Generate device identifier
   if (!generateDeviceUID()) {
     haltError("Unable to generate Device UID");

src/Wippersnapper.h

@@ -248,8 +248,10 @@ typedef enum {
 
 #define WS_MAX_ALT_WIFI_NETWORKS 3 ///< Maximum number of alternative networks
 /* MQTT Configuration */
-#define WS_KEEPALIVE_INTERVAL_MS                                               \
-  5000 ///< Session keepalive interval time, in milliseconds
+#define WS_BROKER_KEEPALIVE_MS                                                 \
+  11000 ///< Maximum time without a ping before broker disconnects (ms)
+#define WS_DEVICE_PING_MS                                                      \
+  5000 ///< Interval at which device sends ping to broker, in milliseconds
 
 #define WS_MQTT_MAX_PAYLOAD_SIZE                                               \
   512 ///< MAXIMUM expected payload size, in bytes
@@ -476,6 +478,8 @@ class Wippersnapper {
   ws_status_t _status = WS_IDLE;   /*!< Adafruit IO connection status */
   uint32_t _last_mqtt_connect = 0; /*!< Previous time when client connected to
                                           Adafruit IO, in milliseconds. */
+  uint16_t _brokerKeepAliveIntervalSeconds =
+      0; /*!< Cached MQTT broker keepalive interval, in seconds. */
   uint32_t _prv_ping = 0;    /*!< Previous time when client pinged Adafruit IO's
                                 MQTT broker, in milliseconds. */
   uint32_t _prvKATBlink = 0; /*!< Previous time when client pinged Adafruit IO's