From a569f55dc727f1dff397320dc62fedd777296463 Mon Sep 17 00:00:00 2001 From: mikecarper Date: Wed, 23 Sep 2026 13:11:09 -0700 Subject: [PATCH] fix(nrf52): recover idle secured BLE links --- src/helpers/nrf52/SerialBLEInterface.cpp | 81 +++++++---- src/helpers/nrf52/SerialBLEInterface.h | 11 +- test/test_nrf52_ble_reconnect_watchdog.py | 134 ++++++++++++++++++ test/test_nrf52_ble_startup.py | 3 + .../test_security_session_timer.cpp | 9 ++ 5 files changed, 211 insertions(+), 27 deletions(-) create mode 100644 test/test_nrf52_ble_reconnect_watchdog.py diff --git a/src/helpers/nrf52/SerialBLEInterface.cpp b/src/helpers/nrf52/SerialBLEInterface.cpp index 638f19fc..72ea18b7 100644 --- a/src/helpers/nrf52/SerialBLEInterface.cpp +++ b/src/helpers/nrf52/SerialBLEInterface.cpp @@ -12,6 +12,7 @@ #define BLE_HEALTH_CHECK_INTERVAL 10000 // Advertising watchdog check every 10 seconds #define BLE_RETRY_THROTTLE_MS 250 // Throttle retries to 250ms when queue buildup detected #define BLE_BOND_PERSIST_TIMEOUT_MS 15000 // Bound first-pair transition recovery +#define BLE_COMPANION_START_TIMEOUT_MS 15000 // Release a secured link with no app data // Connection parameters (units: interval=1.25ms, timeout=10ms) #define BLE_MIN_CONN_INTERVAL 12 // 15ms @@ -130,6 +131,8 @@ void SerialBLEInterface::onConnect(uint16_t connection_handle) { instance->_conn_handle = connection_handle; instance->_isDeviceConnected = false; instance->_security_timer.start(millis()); + instance->_companion_start_timer.cancel(); + instance->_companionDataSeen.store(false, std::memory_order_release); instance->clearBuffers(); #if COMPANION_FEATURE_BLE_MOTA_SOURCE instance->setMotaStreamActive(false); @@ -145,6 +148,7 @@ void SerialBLEInterface::onDisconnect(uint16_t connection_handle, uint8_t reason instance->_conn_handle = BLE_CONN_HANDLE_INVALID; instance->_isDeviceConnected = false; instance->_security_timer.cancel(); + instance->_companion_start_timer.cancel(); instance->clearBuffers(); #if COMPANION_FEATURE_BLE_MOTA_SOURCE instance->setMotaStreamActive(false); @@ -169,6 +173,9 @@ void SerialBLEInterface::onMotaResponse( // Disable the link so the current transaction times out and the main loop // detaches it instead of consuming a partial or injected frame. instance->setMotaStreamActive(false); + } else { + instance->_companionDataSeen.store(true, std::memory_order_release); + instance->_companion_start_timer.cancel(); } } @@ -213,6 +220,9 @@ void SerialBLEInterface::onSecured(uint16_t connection_handle) { "SerialBLEInterface: secured connection was not bonded"); } instance->_security_timer.cancel(); + if (!instance->_companionDataSeen.load(std::memory_order_acquire)) { + instance->_companion_start_timer.start(millis()); + } // Connection interval units: 1.25ms, supervision timeout units: 10ms // Apple: "The product will not read or use the parameters in the Peripheral Preferred Connection Parameters characteristic." @@ -532,6 +542,8 @@ bool SerialBLEInterface::begin(const char* prefix, const char* name, instance = this; _successfulConnectionPending.store(false, std::memory_order_release); _successfulConnectionStarted.store(0, std::memory_order_relaxed); + _companionDataSeen.store(false, std::memory_order_release); + _companion_start_timer.cancel(); _bondedOnlyRecoveryPending.store(false, std::memory_order_release); _advertisingSuppressed.store(false, std::memory_order_release); _stealth_pair_once = stealth_pair_once; @@ -873,6 +885,7 @@ void SerialBLEInterface::serviceTxRecovery(uint32_t now) { _isDeviceConnected = false; _peer_address_valid = false; _security_timer.cancel(); + _companion_start_timer.cancel(); clearBuffers(); if (advertisingAllowed() && !isAdvertising()) { startAdvertising("advertising failed after TX recovery"); @@ -907,6 +920,7 @@ void SerialBLEInterface::recoverStalledTx(const char* cause) { recv_queue_len = 0; _last_retry_attempt = 0; _tx_stall_watchdog.reset(); + _companion_start_timer.cancel(); bleuart.flush(); #if COMPANION_FEATURE_BLE_MOTA_SOURCE setMotaStreamActive(false); @@ -962,12 +976,51 @@ void SerialBLEInterface::disable() { Bluefruit.Advertising.stop(); disconnect(); _security_timer.cancel(); + _companion_start_timer.cancel(); _last_health_check = 0; #if COMPANION_FEATURE_BLE_MOTA_SOURCE setMotaStreamActive(false); #endif } +void SerialBLEInterface::loop() { + const uint32_t now = (uint32_t)millis(); + serviceBondedOnlyTransition(); + if (_tx_disconnect_recovery.pending()) { + serviceTxRecovery(now); + return; + } + if (!_isEnabled) return; + + if (_conn_handle != BLE_CONN_HANDLE_INVALID) { + if (_security_timer.expired(now)) { + // Keep the two-minute PIN-entry window, but do not let an unfinished + // security exchange occupy the only BLE connection indefinitely. + BLE_DEBUG_PRINTLN("SerialBLEInterface: security setup timed out"); + _security_timer.cancel(); + disconnect(); + } + if (_companionDataSeen.load(std::memory_order_acquire)) { + _companion_start_timer.cancel(); + } else if (_isDeviceConnected && _companion_start_timer.expired( + now, BLE_COMPANION_START_TIMEOUT_MS)) { + // iOS can report a secured BLE link while the app never establishes its + // UART session. Release that occupied link so the app can reconnect. + recoverStalledTx("no Companion data after secured BLE connection"); + } + return; + } + + if (advertisingAllowed() + && now - _last_health_check >= BLE_HEALTH_CHECK_INTERVAL) { + _last_health_check = now; + if (!isAdvertising()) { + BLE_DEBUG_PRINTLN("SerialBLEInterface: advertising watchdog restarting"); + startAdvertising("advertising watchdog restart failed"); + } + } +} + size_t SerialBLEInterface::writeFrame(const uint8_t src[], size_t len) { if (len > MAX_FRAME_SIZE) { BLE_DEBUG_PRINTLN("writeFrame(), frame too big, len=%u", (unsigned)len); @@ -1062,32 +1115,6 @@ size_t SerialBLEInterface::checkRecvFrame(uint8_t dest[]) { return len; } - // Advertising watchdog: periodically check if advertising is running, restart if not - // Only run when truly disconnected (no connection handle), not during connection establishment - unsigned long now = millis(); - if (_isEnabled && _conn_handle != BLE_CONN_HANDLE_INVALID - && _security_timer.expired(now)) { - // A client may open a link and never finish PIN/bond negotiation. That - // otherwise suppresses advertising forever because a connection handle - // remains live. Disconnect only: inactivity is not evidence of a stale - // bond, so do not erase anything here. - BLE_DEBUG_PRINTLN("SerialBLEInterface: security setup timed out after %lu ms", - (unsigned long)BLE_SECURITY_SESSION_TIMEOUT_MS); - _security_timer.cancel(); - disconnect(); - } - if (advertisingAllowed() && !isConnected() - && _conn_handle == BLE_CONN_HANDLE_INVALID) { - if (now - _last_health_check >= BLE_HEALTH_CHECK_INTERVAL) { - _last_health_check = now; - - if (!isAdvertising()) { - BLE_DEBUG_PRINTLN("SerialBLEInterface: advertising watchdog - advertising stopped, restarting"); - startAdvertising("advertising watchdog restart failed"); - } - } - } - return 0; } @@ -1128,6 +1155,8 @@ void SerialBLEInterface::onBleUartRX(uint16_t conn_handle) { instance->recv_queue[instance->recv_queue_len].len = read_len; instance->bleuart.readBytes(instance->recv_queue[instance->recv_queue_len].buf, read_len); instance->recv_queue_len++; + instance->_companionDataSeen.store(true, std::memory_order_release); + instance->_companion_start_timer.cancel(); } } diff --git a/src/helpers/nrf52/SerialBLEInterface.h b/src/helpers/nrf52/SerialBLEInterface.h index e38c18bb..8de096fc 100644 --- a/src/helpers/nrf52/SerialBLEInterface.h +++ b/src/helpers/nrf52/SerialBLEInterface.h @@ -80,9 +80,11 @@ class SerialBLEInterface : public BaseSerialInterface { std::atomic _advertisingSuppressed{false}; std::atomic _bondedOnlyRecoveryPending{false}; std::atomic _pairingRequestPending{false}; + std::atomic _companionDataSeen{false}; std::atomic _successfulConnectionPending{false}; std::atomic _successfulConnectionStarted{0}; SecuritySessionTimer _security_timer; + SecuritySessionTimer _companion_start_timer; mesh::BleTxStallWatchdog _tx_stall_watchdog; mesh::BleDisconnectRecovery _tx_disconnect_recovery; @@ -169,6 +171,7 @@ public: const char* beginFailure() const { return _begin_failure; } void disconnect(); + void loop() override; void enable() override; void disable() override; bool isEnabled() const override { return _isEnabled; } @@ -194,7 +197,13 @@ public: Stream& motaStream() { return _mota_stream; } bool isMotaChannelReady(); bool isMotaStreamActive() const { return _mota_stream.isActive(); } - void setMotaStreamActive(bool active) { _mota_stream.setActive(active); } + void setMotaStreamActive(bool active) { + _mota_stream.setActive(active); + if (active) { + _companionDataSeen.store(true, std::memory_order_release); + _companion_start_timer.cancel(); + } + } #endif }; diff --git a/test/test_nrf52_ble_reconnect_watchdog.py b/test/test_nrf52_ble_reconnect_watchdog.py new file mode 100644 index 00000000..db8dec46 --- /dev/null +++ b/test/test_nrf52_ble_reconnect_watchdog.py @@ -0,0 +1,134 @@ +#!/usr/bin/env python3 +"""Exercise the real nRF52 BLE loop's stalled-link recovery decisions.""" + +from pathlib import Path +import subprocess +import tempfile +import unittest + +from test_t096_full_memory import method + + +ROOT = Path(__file__).resolve().parents[1] +SOURCE = ROOT / "src/helpers/nrf52/SerialBLEInterface.cpp" + +HARNESS = r''' +#include +#include +#include +#include + +#define BLE_HEALTH_CHECK_INTERVAL 10000 +#define BLE_COMPANION_START_TIMEOUT_MS 15000 +#define BLE_CONN_HANDLE_INVALID 0xffff +#define BLE_DEBUG_PRINTLN(...) do {} while (0) + +static uint32_t current_millis; +uint32_t millis() { return current_millis; } + +struct Recovery { + bool active = false; + bool pending() const { return active; } +}; + +struct SerialBLEInterface { + bool _isEnabled = false; + bool _isDeviceConnected = false; + uint16_t _conn_handle = BLE_CONN_HANDLE_INVALID; + uint32_t _last_health_check = 0; + bool allow_advertising = true; + bool advertising = false; + std::atomic _companionDataSeen{false}; + SecuritySessionTimer _security_timer; + SecuritySessionTimer _companion_start_timer; + Recovery _tx_disconnect_recovery; + int disconnects = 0; + int recoveries = 0; + int advertisements = 0; + int recovery_services = 0; + void serviceBondedOnlyTransition() {} + void serviceTxRecovery(uint32_t) { ++recovery_services; } + void disconnect() { ++disconnects; } + void recoverStalledTx(const char*) { + ++recoveries; + _tx_disconnect_recovery.active = true; + } + bool advertisingAllowed() const { return allow_advertising; } + bool isAdvertising() const { return advertising; } + bool startAdvertising(const char*) { ++advertisements; advertising = true; return true; } + void loop(); +}; + +@LOOP@ + +int main() { + SerialBLEInterface pending_security; + pending_security._isEnabled = true; + pending_security._conn_handle = 1; + pending_security._security_timer.start(100); + current_millis = 120099; + pending_security.loop(); + assert(pending_security.disconnects == 0); + current_millis = 120100; + pending_security.loop(); + assert(pending_security.disconnects == 1); + + SerialBLEInterface no_app_data; + no_app_data._isEnabled = true; + no_app_data._isDeviceConnected = true; + no_app_data._conn_handle = 2; + no_app_data._companion_start_timer.start(300); + current_millis = 15299; + no_app_data.loop(); + assert(no_app_data.recoveries == 0); + current_millis = 15300; + no_app_data.loop(); + assert(no_app_data.recoveries == 1); + no_app_data.loop(); + assert(no_app_data.recovery_services == 1); + + SerialBLEInterface app_started; + app_started._isEnabled = true; + app_started._isDeviceConnected = true; + app_started._conn_handle = 3; + app_started._companion_start_timer.start(100); + app_started._companionDataSeen.store(true); + current_millis = 120000; + app_started.loop(); + assert(app_started.recoveries == 0); + assert(!app_started._companion_start_timer.pending()); + + SerialBLEInterface disconnected; + disconnected._isEnabled = true; + current_millis = 9999; + disconnected.loop(); + assert(disconnected.advertisements == 0); + current_millis = 10000; + disconnected.loop(); + assert(disconnected.advertisements == 1); + disconnected.loop(); + assert(disconnected.advertisements == 1); +} +''' + + +class Nrf52BleReconnectWatchdogTest(unittest.TestCase): + def test_stalled_link_releases_and_advertising_restores(self): + loop = method(SOURCE.read_text(), "void SerialBLEInterface::loop()") + source = HARNESS.replace("@LOOP@", loop) + with tempfile.TemporaryDirectory(prefix="meshcore-ble-watchdog-") as temp: + cpp = Path(temp) / "watchdog.cpp" + binary = Path(temp) / "watchdog" + cpp.write_text(source) + built = subprocess.run( + ["c++", "-std=c++17", "-I" + str(ROOT / "src"), + str(cpp), "-o", str(binary)], + capture_output=True, text=True, + ) + self.assertEqual(built.returncode, 0, built.stderr) + result = subprocess.run([str(binary)], capture_output=True, text=True) + self.assertEqual(result.returncode, 0, result.stderr) + + +if __name__ == "__main__": + unittest.main() diff --git a/test/test_nrf52_ble_startup.py b/test/test_nrf52_ble_startup.py index 569044a0..d2a07e6e 100644 --- a/test/test_nrf52_ble_startup.py +++ b/test/test_nrf52_ble_startup.py @@ -29,6 +29,7 @@ HARNESS = r''' #include #include #include +#include #ifndef COMPANION_FEATURE_BLE_MOTA_SOURCE #define COMPANION_FEATURE_BLE_MOTA_SOURCE 1 #endif @@ -147,6 +148,8 @@ struct MotaStream { }; struct SerialBLEInterface { @STARTUP_FIELDS@ + std::atomic _companionDataSeen{false}; + SecuritySessionTimer _companion_start_timer; std::atomic _successfulConnectionPending{false}; std::atomic _successfulConnectionStarted{0}; std::atomic _bondedOnlyRecoveryPending{false}, _advertisingSuppressed{false}; diff --git a/test/test_security_session_timer/test_security_session_timer.cpp b/test/test_security_session_timer/test_security_session_timer.cpp index cb8e3309..2e6bf99c 100644 --- a/test/test_security_session_timer/test_security_session_timer.cpp +++ b/test/test_security_session_timer/test_security_session_timer.cpp @@ -26,6 +26,15 @@ TEST(SecuritySessionTimer, RestartUsesTheLatestConnection) { EXPECT_TRUE(timer.expired(1200, 200)); } +TEST(SecuritySessionTimer, SecuredButIdleCompanionLinkExpiresAtFifteenSeconds) { + SecuritySessionTimer timer; + timer.start(4000); + EXPECT_FALSE(timer.expired(18999, 15000)); + EXPECT_TRUE(timer.expired(19000, 15000)); + timer.cancel(); // First Companion data arrived. + EXPECT_FALSE(timer.expired(100000, 15000)); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS();