serious bug: connection to phones not being properly tracked

src/SerialConsole.cpp

@@ -32,6 +32,13 @@ SerialConsole::SerialConsole() : StreamAPI(&Port), RedirectablePrint(&Port)
     emitRebooted();
 }
 
+// For the serial port we can't really detect if any client is on the other side, so instead just look for recent messages
+bool SerialConsole::checkIsConnected()
+{
+    uint32_t now = millis();
+    return (now - lastContactMsec) < getPref_phone_timeout_secs() * 1000UL;
+}
+
 /**
  * we override this to notice when we've received a protobuf over the serial
  * stream.  Then we shunt off debug serial output.
@@ -46,14 +53,3 @@ bool SerialConsole::handleToRadio(const uint8_t *buf, size_t len)
     return StreamAPI::handleToRadio(buf, len);
 }
 
-/// Hookable to find out when connection changes
-void SerialConsole::onConnectionChanged(bool connected)
-{
-    if (connected) { // To prevent user confusion, turn off bluetooth while using the serial port api
-        powerFSM.trigger(EVENT_SERIAL_CONNECTED);
-    } else {
-        // FIXME, we get no notice of serial going away, we should instead automatically generate this event if we haven't
-        // received a packet in a while
-        powerFSM.trigger(EVENT_SERIAL_DISCONNECTED);
-    }
-}

src/SerialConsole.h

@@ -25,8 +25,9 @@ class SerialConsole : public StreamAPI, public RedirectablePrint
     }
 
   protected:
-    /// Hookable to find out when connection changes
+
-    virtual void onConnectionChanged(bool connected);
+    /// Check the current underlying physical link to see if the client is currently connected
+    virtual bool checkIsConnected();    
 };
 
 // A simple wrapper to allow non class aware code write to the console

src/mesh/MeshPlugin.cpp

@@ -86,10 +86,11 @@ void MeshPlugin::callPlugins(const MeshPacket &mp)
         /// We only call plugins that are interested in the packet (and the message is destined to us or we are promiscious)
         bool wantsPacket = (isDecoded || pi.encryptedOk) && (pi.isPromiscuous || toUs) && pi.wantPacket(&mp);
 
-        DEBUG_MSG("Plugin %s wantsPacket=%d\n", pi.name, wantsPacket);
         assert(!pi.myReply); // If it is !null it means we have a bug, because it should have been sent the previous time
 
         if (wantsPacket) {
+            DEBUG_MSG("Plugin %s wantsPacket=%d\n", pi.name, wantsPacket);
+
             pluginFound = true;
 
             /// received channel (or NULL if not decoded)
@@ -97,7 +98,11 @@ void MeshPlugin::callPlugins(const MeshPacket &mp)
 
             /// Is the channel this packet arrived on acceptable? (security check)
             /// Note: we can't know channel names for encrypted packets, so those are NEVER sent to boundChannel plugins
-            bool rxChannelOk = !pi.boundChannel || (ch && ((mp.from == 0) || (strcmp(ch->settings.name, pi.boundChannel) == 0)));
+
+            /// Also: if a packet comes in on the local PC interface, we don't check for bound channels, because it is TRUSTED and it needs to
+            /// to be able to fetch the initial admin packets without yet knowing any channels.
+
+            bool rxChannelOk = !pi.boundChannel || (mp.from == 0) || (ch && (strcmp(ch->settings.name, pi.boundChannel) == 0));
 
             if (!rxChannelOk) {
                 // no one should have already replied!

src/mesh/PhoneAPI.cpp

@@ -27,11 +27,6 @@ PhoneAPI::~PhoneAPI()
 
 void PhoneAPI::handleStartConfig()
 {
-    if (!isConnected()) {
-        onConnectionChanged(true);
-        observe(&service.fromNumChanged);
-    }
-
     // even if we were already connected - restart our state machine
     state = STATE_SEND_MY_INFO;
 
@@ -39,6 +34,11 @@ void PhoneAPI::handleStartConfig()
     nodeInfoForPhone = NULL;   // Don't keep returning old nodeinfos
     nodeDB.resetReadPointer(); // FIXME, this read pointer should be moved out of nodeDB and into this class - because
                                // this will break once we have multiple instances of PhoneAPI running independently
+
+    if (!isConnected()) {
+        onConnectionChanged(true);
+        observe(&service.fromNumChanged);
+    }
 }
 
 void PhoneAPI::close()
@@ -56,10 +56,9 @@ void PhoneAPI::close()
 void PhoneAPI::checkConnectionTimeout()
 {
     if (isConnected()) {
-        uint32_t now = millis();
+        bool newContact = checkIsConnected();
-        bool newContact = (now - lastContactMsec) < getPref_phone_timeout_secs() * 1000UL;
         if (!newContact) {
-            DEBUG_MSG("Timed out on phone contact, dropping phone connection\n");
+            DEBUG_MSG("Lost phone connection\n");
             close();
         }
     }
@@ -93,7 +92,8 @@ bool PhoneAPI::handleToRadio(const uint8_t *buf, size_t bufLength)
             break;
 
         default:
-            DEBUG_MSG("Error: unexpected ToRadio variant\n");
+            // Ignore nop messages
+            // DEBUG_MSG("Error: unexpected ToRadio variant\n");
             break;
         }
     } else {

src/mesh/PhoneAPI.h

@@ -50,9 +50,6 @@ class PhoneAPI
     /// Use to ensure that clients don't get confused about old messages from the radio
     uint32_t config_nonce = 0;
 
-    /** the last msec we heard from the client on the other side of this link */
-    uint32_t lastContactMsec = 0;
-
   public:
     PhoneAPI();
 
@@ -88,12 +85,18 @@ class PhoneAPI
     /// Our fromradio packet while it is being assembled
     FromRadio fromRadioScratch;
 
+    /** the last msec we heard from the client on the other side of this link */
+    uint32_t lastContactMsec = 0;
+    
     /// Hookable to find out when connection changes
     virtual void onConnectionChanged(bool connected) {}
 
     /// If we haven't heard from the other side in a while then say not connected
     void checkConnectionTimeout();
 
+    /// Check the current underlying physical link to see if the client is currently connected
+    virtual bool checkIsConnected() = 0;
+
     /**
      * Subclasses can use this as a hook to provide custom notifications for their transport (i.e. bluetooth notifies)
      */

src/mesh/ReliableRouter.cpp

@@ -50,7 +50,7 @@ bool ReliableRouter::shouldFilterReceived(const MeshPacket *p)
             stopRetransmission(key);
         }
         else {
-            DEBUG_MSG("Possible bug? didn't find pending packet");
+            DEBUG_MSG("didn't find pending packet\n");
         }
     }
 

src/mesh/StreamAPI.cpp

@@ -1,4 +1,5 @@
 #include "StreamAPI.h"
+#include "PowerFSM.h"
 #include "configuration.h"
 
 #define START1 0x94
@@ -28,37 +29,42 @@ int32_t StreamAPI::readStream()
             uint8_t c = stream->read();
 
             // Use the read pointer for a little state machine, first look for framing, then length bytes, then payload
-            size_t ptr = rxPtr++; // assume we will probably advance the rxPtr
+            size_t ptr = rxPtr;
 
+            rxPtr++; // assume we will probably advance the rxPtr
             rxBuf[ptr] = c; // store all bytes (including framing)
 
+            // console->printf("rxPtr %d ptr=%d c=0x%x\n", rxPtr, ptr, c);
+
             if (ptr == 0) { // looking for START1
                 if (c != START1)
                     rxPtr = 0;     // failed to find framing
             } else if (ptr == 1) { // looking for START2
                 if (c != START2)
                     rxPtr = 0;                             // failed to find framing
-            } else if (ptr >= HEADER_LEN) {                // we have at least read our 4 byte framing
+            } else if (ptr >= HEADER_LEN - 1) {            // we have at least read our 4 byte framing
                 uint32_t len = (rxBuf[2] << 8) + rxBuf[3]; // big endian 16 bit length follows framing
 
-                if (ptr == HEADER_LEN) {
+                console->printf("len %d\n", len);
+
+                if (ptr == HEADER_LEN - 1) {
                     // we _just_ finished our 4 byte header, validate length now (note: a length of zero is a valid
                     // protobuf also)
                     if (len > MAX_TO_FROM_RADIO_SIZE)
                         rxPtr = 0; // length is bogus, restart search for framing
                 }
 
-                if (rxPtr != 0 && ptr + 1 == len + HEADER_LEN) {
+                if (rxPtr != 0) // Is packet still considered 'good'?
+                    if (ptr + 1 >= len + HEADER_LEN) { // have we received all of the payload?
                         rxPtr = 0; // start over again on the next packet
 
                         // If we didn't just fail the packet and we now have the right # of bytes, parse it
-                    if (handleToRadio(rxBuf + HEADER_LEN, len))
+                        handleToRadio(rxBuf + HEADER_LEN, len);
-                        return 0; // we want to be called again ASAP because we still have more work to do
                     }
             }
         }
 
-        // we had packets available this time, so assume we might have them next time also
+        // we had bytes available this time, so assume we might have them next time also
         lastRxMsec = now;
         return 0;
     }
@@ -110,3 +116,17 @@ void StreamAPI::emitRebooted()
     // DEBUG_MSG("Emitting reboot packet for serial shell\n");
     emitTxBuffer(pb_encode_to_bytes(txBuf + HEADER_LEN, FromRadio_size, FromRadio_fields, &fromRadioScratch));
 }
+
+/// Hookable to find out when connection changes
+void StreamAPI::onConnectionChanged(bool connected)
+{
+    // FIXME do reference counting instead
+
+    if (connected) { // To prevent user confusion, turn off bluetooth while using the serial port api
+        powerFSM.trigger(EVENT_SERIAL_CONNECTED);
+    } else {
+        // FIXME, we get no notice of serial going away, we should instead automatically generate this event if we haven't
+        // received a packet in a while
+        powerFSM.trigger(EVENT_SERIAL_DISCONNECTED);
+    }
+}

src/mesh/StreamAPI.h

@@ -67,6 +67,11 @@ class StreamAPI : public PhoneAPI, protected concurrency::OSThread
      */
     void emitRebooted();
 
+    virtual void onConnectionChanged(bool connected);
+    
+    /// Check the current underlying physical link to see if the client is currently connected
+    virtual bool checkIsConnected() = 0;
+
     /**
      * Send the current txBuffer over our stream
      */

src/mesh/http/ContentHandler.h

@@ -40,7 +40,9 @@ class HttpAPI : public PhoneAPI
     // Nothing here yet
 
   protected:
-    // Nothing here yet
+  
+    /// Check the current underlying physical link to see if the client is currently connected
+    virtual bool checkIsConnected() { return true; } // FIXME, be smarter about this
 };
 
 

src/mesh/wifi/WiFiServerAPI.cpp

@@ -1,5 +1,4 @@
 #include "WiFiServerAPI.h"
-#include "PowerFSM.h"
 #include "configuration.h"
 #include <Arduino.h>
 
@@ -26,18 +25,7 @@ WiFiServerAPI::~WiFiServerAPI()
     // FIXME - delete this if the client dropps the connection!
 }
 
-/// Hookable to find out when connection changes
-void WiFiServerAPI::onConnectionChanged(bool connected)
-{
-    // FIXME - we really should be doing global reference counting to see if anyone is currently using serial or wifi and if so,
-    // block sleep
 
-    if (connected) { // To prevent user confusion, turn off bluetooth while using the serial port api
-        powerFSM.trigger(EVENT_SERIAL_CONNECTED);
-    } else {
-        powerFSM.trigger(EVENT_SERIAL_DISCONNECTED);
-    }
-}
 
 /// override close to also shutdown the TCP link
 void WiFiServerAPI::close()
@@ -46,6 +34,12 @@ void WiFiServerAPI::close()
     StreamAPI::close();
 }
 
+/// Check the current underlying physical link to see if the client is currently connected
+bool WiFiServerAPI::checkIsConnected()
+{
+    return client.connected();
+}
+
 int32_t WiFiServerAPI::runOnce()
 {
     if (client.connected()) {

src/mesh/wifi/WiFiServerAPI.h

@@ -21,10 +21,11 @@ class WiFiServerAPI : public StreamAPI
     virtual void close();
 
   protected:
-    /// Hookable to find out when connection changes
-    virtual void onConnectionChanged(bool connected);
 
     virtual int32_t runOnce(); // Check for dropped client connections
+
+    /// Check the current underlying physical link to see if the client is currently connected
+    virtual bool checkIsConnected();
 };
 
 /**

src/nimble/NimbleBluetoothAPI.cpp

@@ -31,6 +31,10 @@ void BluetoothPhoneAPI::onNowHasData(uint32_t fromRadioNum)
     }
 }
 
+bool BluetoothPhoneAPI::checkIsConnected() {
+    return curConnectionHandle != -1;
+}
+
 int toradio_callback(uint16_t conn_handle, uint16_t attr_handle, struct ble_gatt_access_ctxt *ctxt, void *arg)
 {
     auto om = ctxt->om;

src/nimble/NimbleBluetoothAPI.h

@@ -6,10 +6,14 @@ extern uint16_t fromNumValHandle;
 
 class BluetoothPhoneAPI : public PhoneAPI
 {
+protected: 
     /**
      * Subclasses can use this as a hook to provide custom notifications for their transport (i.e. bluetooth notifies)
      */
     virtual void onNowHasData(uint32_t fromRadioNum);
+
+    /// Check the current underlying physical link to see if the client is currently connected
+    virtual bool checkIsConnected();
 };
 
 extern PhoneAPI *bluetoothPhoneAPI;

src/plugins/AdminPlugin.cpp

@@ -30,10 +30,11 @@ void AdminPlugin::handleGetRadio(const MeshPacket &req)
         AdminMessage r = AdminMessage_init_default;
         r.get_radio_response = radioConfig;
 
-        // NOTE: The phone app needs to know the ls_secs value so it can properly expect sleep behavior.
+        // NOTE: The phone app needs to know the ls_secs & phone_timeout value so it can properly expect sleep behavior.
         // So even if we internally use 0 to represent 'use default' we still need to send the value we are
         // using to the app (so that even old phone apps work with new device loads).
         r.get_radio_response.preferences.ls_secs = getPref_ls_secs();
+        r.get_radio_response.preferences.phone_timeout_secs = getPref_phone_timeout_secs();
 
         r.which_variant = AdminMessage_get_radio_response_tag;
         myReply = allocDataProtobuf(r);