From d179bda7280f765b3b7cc9ff7dc824b426abb154 Mon Sep 17 00:00:00 2001 From: Kevin Hester Date: Mon, 3 May 2021 14:46:30 +0800 Subject: [PATCH] serious bug: connection to phones not being properly tracked --- src/SerialConsole.cpp | 18 ++++++-------- src/SerialConsole.h | 5 ++-- src/mesh/MeshPlugin.cpp | 9 +++++-- src/mesh/PhoneAPI.cpp | 18 +++++++------- src/mesh/PhoneAPI.h | 9 ++++--- src/mesh/ReliableRouter.cpp | 2 +- src/mesh/StreamAPI.cpp | 40 +++++++++++++++++++++++-------- src/mesh/StreamAPI.h | 5 ++++ src/mesh/http/ContentHandler.h | 4 +++- src/mesh/wifi/WiFiServerAPI.cpp | 18 +++++--------- src/mesh/wifi/WiFiServerAPI.h | 5 ++-- src/nimble/NimbleBluetoothAPI.cpp | 4 ++++ src/nimble/NimbleBluetoothAPI.h | 4 ++++ src/plugins/AdminPlugin.cpp | 3 ++- 14 files changed, 90 insertions(+), 54 deletions(-) diff --git a/src/SerialConsole.cpp b/src/SerialConsole.cpp index 137fbcf0d..7435981ea 100644 --- a/src/SerialConsole.cpp +++ b/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); - } -} \ No newline at end of file diff --git a/src/SerialConsole.h b/src/SerialConsole.h index b057c1931..cc3ec4ec3 100644 --- a/src/SerialConsole.h +++ b/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 diff --git a/src/mesh/MeshPlugin.cpp b/src/mesh/MeshPlugin.cpp index 0291ac941..a31a7b499 100644 --- a/src/mesh/MeshPlugin.cpp +++ b/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! diff --git a/src/mesh/PhoneAPI.cpp b/src/mesh/PhoneAPI.cpp index 018ab6d3c..a14e3a7c9 100644 --- a/src/mesh/PhoneAPI.cpp +++ b/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 = (now - lastContactMsec) < getPref_phone_timeout_secs() * 1000UL; + bool newContact = checkIsConnected(); 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 { diff --git a/src/mesh/PhoneAPI.h b/src/mesh/PhoneAPI.h index 02165aae3..90a8c4d18 100644 --- a/src/mesh/PhoneAPI.h +++ b/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) */ diff --git a/src/mesh/ReliableRouter.cpp b/src/mesh/ReliableRouter.cpp index ca9f8c131..7570ab654 100644 --- a/src/mesh/ReliableRouter.cpp +++ b/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"); } } diff --git a/src/mesh/StreamAPI.cpp b/src/mesh/StreamAPI.cpp index 2bc82b084..3785e5028 100644 --- a/src/mesh/StreamAPI.cpp +++ b/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) { - rxPtr = 0; // start over again on the next packet + 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)) - return 0; // we want to be called again ASAP because we still have more work to do - } + // If we didn't just fail the packet and we now have the right # of bytes, parse it + handleToRadio(rxBuf + HEADER_LEN, len); + } } } - // 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; } @@ -109,4 +115,18 @@ 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); + } } \ No newline at end of file diff --git a/src/mesh/StreamAPI.h b/src/mesh/StreamAPI.h index 6180a95d8..b33e73eac 100644 --- a/src/mesh/StreamAPI.h +++ b/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 */ diff --git a/src/mesh/http/ContentHandler.h b/src/mesh/http/ContentHandler.h index 8e22faefc..fc091eff9 100644 --- a/src/mesh/http/ContentHandler.h +++ b/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 }; diff --git a/src/mesh/wifi/WiFiServerAPI.cpp b/src/mesh/wifi/WiFiServerAPI.cpp index c0be8bc4a..79c749af5 100644 --- a/src/mesh/wifi/WiFiServerAPI.cpp +++ b/src/mesh/wifi/WiFiServerAPI.cpp @@ -1,5 +1,4 @@ #include "WiFiServerAPI.h" -#include "PowerFSM.h" #include "configuration.h" #include @@ -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()) { diff --git a/src/mesh/wifi/WiFiServerAPI.h b/src/mesh/wifi/WiFiServerAPI.h index 96cfb2bb0..96ff62999 100644 --- a/src/mesh/wifi/WiFiServerAPI.h +++ b/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(); }; /** diff --git a/src/nimble/NimbleBluetoothAPI.cpp b/src/nimble/NimbleBluetoothAPI.cpp index dde9171a3..b47a75603 100644 --- a/src/nimble/NimbleBluetoothAPI.cpp +++ b/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; diff --git a/src/nimble/NimbleBluetoothAPI.h b/src/nimble/NimbleBluetoothAPI.h index 5fa66cc24..e2260ba4d 100644 --- a/src/nimble/NimbleBluetoothAPI.h +++ b/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; \ No newline at end of file diff --git a/src/plugins/AdminPlugin.cpp b/src/plugins/AdminPlugin.cpp index 9ec305ec7..9c560fdb1 100644 --- a/src/plugins/AdminPlugin.cpp +++ b/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);