diff --git a/meshterm/context.py b/meshterm/context.py index b26e7d2..40b6ed8 100644 --- a/meshterm/context.py +++ b/meshterm/context.py @@ -264,8 +264,23 @@ class AppContext: # resolution, and no re-scan needed to reconnect to a known address. ble_address, ble_pin = self._resolve_ble_endpoint() if ble_address: + # When this session's scan discovered the device, hand its live BLEDevice to the + # connection so bleak opens it directly instead of re-discovering the address (a + # fresh internal scan that intermittently misses a slow-advertising companion). + # A remembered/explicit address has no scan result and connects by address alone. + selected = self.selected_device + ble_device = ( + selected.ble_device + if selected is not None and selected.is_ble and selected.address == ble_address + else None + ) self._device = make_device( - mock=False, port=None, transport="ble", address=ble_address, pin=ble_pin + mock=False, + port=None, + transport="ble", + address=ble_address, + pin=ble_pin, + ble_device=ble_device, ) self._active_transport = "ble" self._active_address = ble_address diff --git a/meshterm/core/connection.py b/meshterm/core/connection.py index 7757850..e4d704b 100644 --- a/meshterm/core/connection.py +++ b/meshterm/core/connection.py @@ -62,6 +62,29 @@ _MESSAGE_POLL_INTERVAL_S = 3.0 #: drain loop (seconds). _MESSAGE_GET_TIMEOUT_S = 5.0 +#: How long a graceful ``meshcore`` client teardown may take before it is abandoned and the +#: transport is force-closed instead (seconds). A healthy disconnect completes in well under a +#: second; the bound exists because the library's dispatcher shutdown can deadlock — its +#: ``queue.join()`` never returns when two or more events (a routine serial RX burst) are +#: queued at the moment of stop, since the processor task exits after draining only one. Kept +#: comfortably under the interactive session's 5-second exit watchdog so even the forced path +#: finishes as a *clean* exit rather than an ``os._exit`` reap. +_DISCONNECT_TIMEOUT_S = 2.0 + +#: Bound on the forced transport close that follows an abandoned graceful teardown (seconds). +#: ``_DISCONNECT_TIMEOUT_S + _FORCE_DISCONNECT_TIMEOUT_S`` stays under the exit watchdog. +_FORCE_DISCONNECT_TIMEOUT_S = 1.5 + +#: Total attempts at opening the BLE link before its failure is surfaced. Opening a BLE +#: connection on Windows is intermittently flaky (a slow-advertising peripheral is missed by +#: bleak's internal lookup, or the link-layer connect races the just-finished discovery scan); +#: a single retry recovers the common case without meaningfully delaying a genuinely absent +#: device. This retries only the *link open* — never a rejected PIN and never a mesh transmit. +_BLE_CONNECT_ATTEMPTS = 2 + +#: Pause between BLE link-open attempts (seconds), giving the OS radio a beat to settle. +_BLE_CONNECT_RETRY_DELAY_S = 1.0 + TX_POWER_MIN = 1 TX_POWER_MAX = 22 @@ -691,6 +714,7 @@ class MeshCoreDevice(Device): transport: str = "serial", address: Optional[str] = None, pin: Optional[str] = None, + ble_device: Optional[object] = None, ) -> None: """Initialize the device wrapper. @@ -705,6 +729,12 @@ class MeshCoreDevice(Device): transport: ``"serial"`` (default) or ``"ble"``. address: Bluetooth address (e.g. ``AA:BB:CC:DD:EE:FF``); BLE transport only. pin: Optional BLE pairing PIN, when the peripheral requires one (BLE only). + ble_device: The ``bleak.BLEDevice`` the discovery scan already found at + ``address``, when available (BLE only). Passing it lets the connect open the + peripheral directly instead of re-discovering it by address — on Windows a + bare-address connect runs a fresh internal scan that intermittently misses a + slow-advertising companion, which is the main source of flaky BLE startups. + Typed ``object`` so ``bleak`` need not be imported on non-BLE paths. """ self._port = port self._baudrate = baudrate @@ -712,6 +742,7 @@ class MeshCoreDevice(Device): self._transport = transport self._address = address self._pin = pin + self._ble_device = ble_device self._mc = None # type: ignore[var-annotated] # meshcore.MeshCore # Serializes channel reads. The meshcore library's get_channel waits for "the next # CHANNEL_INFO event" with no correlation to the index it asked for, and the dispatcher @@ -807,12 +838,7 @@ class MeshCoreDevice(Device): that was rejected. """ try: - return await mesh_core.create_ble( - address=self._address, - pin=self._pin, - default_timeout=self._connect_timeout, - auto_reconnect=False, - ) + return await self._create_ble_with_retry(mesh_core) except ImportError as exc: raise DeviceCommandError( "Bluetooth support requires the 'bleak' package, which isn't installed. " @@ -836,6 +862,57 @@ class MeshCoreDevice(Device): return await self._open_ble(mesh_core, allow_repair=False) raise DeviceAuthenticationError(self._ble_auth_message()) from exc + async def _create_ble_with_retry(self, mesh_core): # type: ignore[no-untyped-def] + """Call ``create_ble``, retrying the transport-level failures that are transient. + + The meshcore client raises a bare ``ConnectionError`` when the *link itself* could + not be opened — the peripheral wasn't found during bleak's internal lookup, or the + link-layer connect timed out. On Windows both are routinely transient: a companion + advertising on a slow interval is easily missed by a single scan window, and a + connect attempted right after the discovery scan can race the radio. Users learned + to work around it by re-selecting the device, which is nothing but a manual retry — + so retry here, briefly, before surfacing the failure. The already-discovered + ``BLEDevice`` (when the picker's scan produced one) is passed through so the client + connects to it directly instead of re-discovering the address. + + Only ``ConnectionError`` is retried: a PIN/bond rejection or any other GATT failure + propagates unchanged on the first attempt so the auth handling in :meth:`_open_ble` + (and a genuine wrong-PIN) is never looped. + + Args: + mesh_core: The imported ``meshcore.MeshCore`` class. + + Returns: + The connected ``MeshCore`` client, or ``None`` if the transport connected but + the peripheral never answered the identity handshake. + + Raises: + ConnectionError: If every attempt failed to open the link. + """ + last_exc: Optional[ConnectionError] = None + for attempt in range(_BLE_CONNECT_ATTEMPTS): + if attempt: + await asyncio.sleep(_BLE_CONNECT_RETRY_DELAY_S) + try: + return await mesh_core.create_ble( + address=self._address, + device=self._ble_device, + pin=self._pin, + default_timeout=self._connect_timeout, + auto_reconnect=False, + ) + except ConnectionError as exc: + _log.debug( + "BLE link to %s failed to open (attempt %d/%d): %s", + self._address, + attempt + 1, + _BLE_CONNECT_ATTEMPTS, + exc, + ) + last_exc = exc + assert last_exc is not None # the loop always runs; only ConnectionError falls through + raise last_exc + async def _pair_ble_windows(self, *, force: bool) -> bool: """Establish an authenticated BLE bond via the WinRT ProvidePin ceremony (Windows only). @@ -1115,13 +1192,46 @@ class MeshCoreDevice(Device): return True # no port recorded (shouldn't happen once connected) — can't tell return serial_port_present(self._port) - async def disconnect(self) -> None: # noqa: D102 - inherited docstring + async def disconnect(self) -> None: + """Close the connection and release resources. Idempotent, and bounded in time. + + The graceful ``meshcore`` teardown is given :data:`_DISCONNECT_TIMEOUT_S` to finish. + That bound matters: the library's dispatcher stop awaits ``queue.join()``, but its + processor task exits after handling at most one event once stopped — so with two or + more events queued at that instant (a routine advert/RX-log burst on a live mesh) + the join deadlocks and a quit would hang until the exit watchdog force-kills the + process. When the graceful path doesn't return in time it is cancelled and the + teardown is forced instead: the dispatcher task is cancelled synchronously and the + raw transport is closed directly (also bounded), so the port/link is still released. + """ if self._mc is None: return - disconnect = getattr(self._mc, "disconnect", None) - if disconnect is not None: - await disconnect() - self._mc = None + mc, self._mc = self._mc, None + disconnect = getattr(mc, "disconnect", None) + if disconnect is None: + return + try: + await asyncio.wait_for(disconnect(), timeout=_DISCONNECT_TIMEOUT_S) + return + except asyncio.TimeoutError: + _log.debug("graceful disconnect timed out; forcing transport teardown") + except Exception as exc: # noqa: BLE001 - teardown must not raise; fall to forced path + _log.debug("graceful disconnect failed (%s); forcing transport teardown", exc) + # Forced teardown: cancel the (possibly wedged) dispatcher task without awaiting the + # deadlocked join, then close the underlying transport so the serial port / BLE link + # is actually released. Every step is best-effort — nothing here may block the exit. + try: + stop = getattr(mc, "stop", None) + if stop is not None: + stop() + except Exception as exc: # noqa: BLE001 - best-effort force-stop + _log.debug("dispatcher force-stop failed: %s", exc) + try: + raw = getattr(getattr(mc, "connection_manager", None), "connection", None) + if raw is not None: + await asyncio.wait_for(raw.disconnect(), timeout=_FORCE_DISCONNECT_TIMEOUT_S) + except Exception as exc: # noqa: BLE001 - the link may already be gone + _log.debug("forced transport close failed: %s", exc) def _require(self): # type: ignore[no-untyped-def] """Return the live client or raise if not connected.""" @@ -2650,6 +2760,7 @@ def make_device( transport: str = "serial", address: Optional[str] = None, pin: Optional[str] = None, + ble_device: Optional[object] = None, ) -> Device: """Construct the appropriate :class:`Device` for the current invocation. @@ -2662,6 +2773,9 @@ def make_device( transport: ``"serial"`` (default) or ``"ble"``. address: Bluetooth address for the BLE transport. Required when ``transport="ble"``. pin: Optional BLE pairing PIN (BLE only). + ble_device: The scanned ``bleak.BLEDevice`` for ``address``, when this session's + discovery produced one (BLE only) — lets the connect skip re-discovering the + peripheral by address (see :class:`MeshCoreDevice`). Returns: A connected-on-enter :class:`Device` instance. @@ -2677,7 +2791,7 @@ def make_device( "No Bluetooth address configured. Pass --ble, pick a device at startup, " "or use --mock." ) - return MeshCoreDevice(transport="ble", address=address, pin=pin) + return MeshCoreDevice(transport="ble", address=address, pin=pin, ble_device=ble_device) if not port: raise ValueError( "No serial port configured. Pass --port, set a profile, or use --mock." @@ -2729,7 +2843,13 @@ async def probe_device( if device.is_ble: timeout = _PROBE_TIMEOUT_BLE_S probe = MeshCoreDevice( - transport="ble", address=device.address, pin=pin, connect_timeout=timeout + transport="ble", + address=device.address, + pin=pin, + connect_timeout=timeout, + # The scan that discovered the device already holds its BLEDevice; connecting + # through it skips the by-address re-discovery that makes BLE startups flaky. + ble_device=device.ble_device, ) else: timeout = _PROBE_TIMEOUT_SERIAL_S diff --git a/meshterm/core/discovery.py b/meshterm/core/discovery.py index 796929b..aee9b56 100644 --- a/meshterm/core/discovery.py +++ b/meshterm/core/discovery.py @@ -80,6 +80,13 @@ class DiscoveredDevice: transport: ``"serial"`` or ``"ble"`` — which connection layer opens this device. address: Bluetooth address for a BLE device (e.g. ``AA:BB:CC:DD:EE:FF``). name: Advertised BLE local name (e.g. ``"MeshCore-Basestation"``); BLE only. + ble_device: The live ``bleak.BLEDevice`` the scan produced; BLE only, and only for + devices discovered *this session* (a remembered device reloads as a bare + address). Handed to the connection layer so it can open the peripheral + directly — connecting by address alone makes bleak re-discover the device with + a fresh internal scan, which on Windows intermittently misses a + slow-advertising companion. Typed ``object`` so ``bleak`` stays optional; never + persisted. """ port: str = "" @@ -93,6 +100,7 @@ class DiscoveredDevice: transport: str = TRANSPORT_SERIAL address: Optional[str] = None name: Optional[str] = None + ble_device: Optional[object] = None @property def is_ble(self) -> bool: @@ -254,6 +262,7 @@ async def discover_ble_devices(timeout: float = BLE_SCAN_TIMEOUT_S) -> list[Disc name=name, description=name, product=name, + ble_device=dev, ) ) devices.sort(key=lambda d: (d.name or "").casefold()) diff --git a/tests/test_disconnect.py b/tests/test_disconnect.py index 5d74f07..e6e856e 100644 --- a/tests/test_disconnect.py +++ b/tests/test_disconnect.py @@ -214,6 +214,71 @@ async def test_create_ble_gives_up_after_one_repair() -> None: assert calls == [False, True] +async def test_create_ble_retries_a_transient_link_failure(monkeypatch) -> None: + """A transport-level ConnectionError is retried once, and the scanned BLEDevice rides along. + + Models the common Windows flake: the first link open misses the (slow-advertising) + peripheral and the meshcore client raises a bare ``ConnectionError``; users learned to + work around it by re-selecting the device — a manual retry — so the connect retries + itself before surfacing the failure. + """ + scanned_device = object() # the BLEDevice the discovery scan produced + attempts: list[dict] = [] + + class _FakeMeshCore: + @staticmethod + async def create_ble(**kwargs): + attempts.append(kwargs) + if len(attempts) == 1: + raise ConnectionError("Failed to connect to device") + return "connected-client" + + monkeypatch.setattr(connection, "_BLE_CONNECT_RETRY_DELAY_S", 0.0) + dev = connection.MeshCoreDevice( + transport="ble", address="AA:BB:CC:DD:EE:FF", ble_device=scanned_device + ) + await _disable_windows_pairing(dev) + assert await dev._create_ble(_FakeMeshCore) == "connected-client" + assert len(attempts) == 2 # failed once, retried once + # Every attempt connects through the already-discovered BLEDevice, never a bare address. + assert all(kw["device"] is scanned_device for kw in attempts) + + +async def test_create_ble_gives_up_after_the_retry(monkeypatch) -> None: + """A link that never opens surfaces its ConnectionError after the bounded retries.""" + attempts: list[int] = [] + + class _FakeMeshCore: + @staticmethod + async def create_ble(**kwargs): + attempts.append(1) + raise ConnectionError("Failed to connect to device") + + monkeypatch.setattr(connection, "_BLE_CONNECT_RETRY_DELAY_S", 0.0) + dev = connection.MeshCoreDevice(transport="ble", address="AA:BB:CC:DD:EE:FF") + await _disable_windows_pairing(dev) + with pytest.raises(ConnectionError): + await dev._create_ble(_FakeMeshCore) + assert len(attempts) == connection._BLE_CONNECT_ATTEMPTS + + +async def test_create_ble_never_retries_an_auth_rejection() -> None: + """A PIN/bond rejection is translated on the first attempt — never looped by the retry.""" + attempts: list[int] = [] + + class _FakeMeshCore: + @staticmethod + async def create_ble(**kwargs): + attempts.append(1) + raise BleakGATTProtocolError("Insufficient Authentication") + + dev = connection.MeshCoreDevice(transport="ble", address="00:11:22:33:44:55") + await _disable_windows_pairing(dev) + with pytest.raises(connection.DeviceAuthenticationError): + await dev._create_ble(_FakeMeshCore) + assert len(attempts) == 1 + + async def test_pair_ble_windows_noops_without_pin() -> None: """Pairing is skipped (no WinRT touched) when no PIN is set — the fast, hermetic path.""" dev = connection.MeshCoreDevice(transport="ble", address="00:11:22:33:44:55") @@ -710,3 +775,70 @@ async def test_session_loop_resumes_a_fresh_menu_after_reconnect(monkeypatch) -> await asyncio.wait_for(menu._session_loop(object(), session), timeout=2) assert state["handle"] == 1 # one disconnect handled assert state["menu"] == 2 # a fresh menu ran after reconnect + + +class _WedgedMeshCore: + """A fake meshcore client whose graceful ``disconnect()`` never returns. + + Reproduces the library's dispatcher-stop deadlock (``queue.join()`` with events still + queued after the processor task exited), which used to hang MeshTerm's exit until the + watchdog force-killed the process. Records whether the forced path ran. + """ + + def __init__(self) -> None: + self.force_stopped = False + self.connection_manager = SimpleNamespace(connection=self) + self.raw_closed = False + + async def disconnect(self) -> None: + # Called both as the graceful teardown (via the manager-less attribute lookup on + # the client) and as the raw transport close. The graceful call wedges; the raw + # close is distinguished by the force-stop having run first. + if not self.force_stopped: + await asyncio.Event().wait() # the dispatcher deadlock: never returns + self.raw_closed = True + + def stop(self) -> None: + self.force_stopped = True + + +async def test_disconnect_bounds_a_wedged_client_teardown(monkeypatch) -> None: + """A deadlocked graceful teardown is abandoned and the transport force-closed instead.""" + monkeypatch.setattr(connection, "_DISCONNECT_TIMEOUT_S", 0.05) + monkeypatch.setattr(connection, "_FORCE_DISCONNECT_TIMEOUT_S", 0.5) + + dev = connection.MeshCoreDevice(port="COM_TEST") + wedged = _WedgedMeshCore() + dev._mc = wedged + + await asyncio.wait_for(dev.disconnect(), timeout=2.0) # must not hang + + assert wedged.force_stopped # the dispatcher task was cancelled synchronously + assert wedged.raw_closed # the port/link was still released + assert dev._mc is None # idempotent: a second disconnect is a no-op + + +async def test_disconnect_graceful_path_needs_no_force(monkeypatch) -> None: + """A healthy teardown completes gracefully; the forced path is never entered.""" + + class _HealthyMeshCore: + def __init__(self) -> None: + self.disconnected = False + self.force_stopped = False + self.connection_manager = SimpleNamespace(connection=self) + + async def disconnect(self) -> None: + self.disconnected = True + + def stop(self) -> None: + self.force_stopped = True + + dev = connection.MeshCoreDevice(port="COM_TEST") + healthy = _HealthyMeshCore() + dev._mc = healthy + + await dev.disconnect() + + assert healthy.disconnected + assert not healthy.force_stopped + assert dev._mc is None diff --git a/tests/test_discovery.py b/tests/test_discovery.py index 90db7c2..5063365 100644 --- a/tests/test_discovery.py +++ b/tests/test_discovery.py @@ -183,6 +183,10 @@ async def test_discover_ble_filters_to_meshcore(monkeypatch: pytest.MonkeyPatch) devices = await discovery.discover_ble_devices(timeout=0.0) assert [d.name for d in devices] == ["MeshCore-Base"] assert devices[0].is_ble and devices[0].address == "AA:BB:CC:DD:EE:FF" + # The scan's live BLEDevice rides along so the connect can open the peripheral directly + # instead of re-discovering the address (the flaky path on Windows). + assert devices[0].ble_device is not None + assert devices[0].ble_device.address == "AA:BB:CC:DD:EE:FF" async def test_discover_ble_survives_no_adapter(monkeypatch: pytest.MonkeyPatch) -> None: