From 2f03a93f2b82374d12ec5dd2c74ec07d154f55f3 Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Tue, 25 Aug 2026 18:08:36 +0200 Subject: [PATCH 01/16] feat(android): native BluetoothDevice.getName() bridge for the gen5 readiness gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The official gen5 readiness gate reads Android's own getName() after HELLO. flutter_blue_plus's platformName cannot back it: that is an in-memory cache, empty for a device rebuilt with BluetoothDevice.fromId() on a cold process start — exactly the known-device reconnects that skip scanning. One method on a new openstrap/ble_native channel, registered on the long-lived engine so headless syncs can ask too. Needs BLUETOOTH_CONNECT on S+ (already held for every GATT op); permission/adapter failures reach Dart as an error and read as "no name", which is the gate's failing value. --- .../openstrap_edge/NativeChannels.kt | 32 ++++++++++++++++ lib/ble/android_native_name.dart | 37 +++++++++++++++++++ 2 files changed, 69 insertions(+) create mode 100644 lib/ble/android_native_name.dart diff --git a/android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/NativeChannels.kt b/android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/NativeChannels.kt index fa551d0a..7a114cbf 100644 --- a/android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/NativeChannels.kt +++ b/android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/NativeChannels.kt @@ -1,6 +1,7 @@ package wtf.openstrap.openstrap_edge import android.app.ActivityManager +import android.bluetooth.BluetoothManager import android.content.ComponentName import android.content.Context import android.content.Intent @@ -33,6 +34,7 @@ object NativeChannels { private const val EDGE_TRACKING_CHANNEL = "openstrap/edge_tracking" private const val DEVICE_ACTIONS_CHANNEL = "openstrap/device_actions" private const val ANDROID_BG_CHANNEL = "openstrap/android_background" + private const val BLE_NATIVE_CHANNEL = "openstrap/ble_native" const val TASKER_CHANNEL = "openstrap/tasker" const val ACTION_DOUBLE_TAP = "wtf.openstrap.openstrap_edge.DOUBLE_TAP" private const val TASKER_TOKEN_KEY = "tasker_auth_token" @@ -127,6 +129,36 @@ object NativeChannels { } } + // Native Bluetooth reads flutter_blue_plus cannot answer. The one + // method here backs the gen5 readiness gate: it must read the + // platform `BluetoothDevice.getName()` (bond/stack-backed), not the + // plugin's in-memory platformName cache, which is empty for a device + // rebuilt from its id on a cold start. See lib/ble/android_native_name.dart. + MethodChannel(engine.dartExecutor.binaryMessenger, BLE_NATIVE_CHANNEL) + .setMethodCallHandler { call, result -> + when (call.method) { + "remoteDeviceName" -> { + val mac = call.arguments as? String + if (mac.isNullOrEmpty()) { + result.error("bad_args", "expected the remote MAC", null) + return@setMethodCallHandler + } + try { + val mgr = app.getSystemService(Context.BLUETOOTH_SERVICE) + as? BluetoothManager + // getName() needs BLUETOOTH_CONNECT on API 31+ (held — + // every GATT op needs it too); a SecurityException or an + // invalid MAC lands in the catch and reaches Dart as an + // error, which the gate reads as "no name". + result.success(mgr?.adapter?.getRemoteDevice(mac)?.name) + } catch (e: Exception) { + result.error("name_unavailable", e.toString(), null) + } + } + else -> result.notImplemented() + } + } + // OS keep-alive integrations: CompanionDeviceManager association (background // FGS exemption + device-presence relaunch) and the battery-optimization // (Doze) exemption. See CompanionBridge.kt / lib/ble/android_background.dart. diff --git a/lib/ble/android_native_name.dart b/lib/ble/android_native_name.dart new file mode 100644 index 00000000..95e309a7 --- /dev/null +++ b/lib/ble/android_native_name.dart @@ -0,0 +1,37 @@ +// android_native_name.dart — the Android `BluetoothDevice.getName()` read the +// gen5 readiness gate requires, as a platform-channel call. +// +// Why not flutter_blue_plus's `platformName`: that is an in-memory cache the +// plugin fills from scan results and connection events. A device rebuilt with +// `BluetoothDevice.fromId()` on a cold process start has an EMPTY cache, so +// gating readiness on it would fail every known-device reconnect that skipped +// scanning. The official gate reads the native `BluetoothDevice.getName()`, +// which the Android stack backs with its own bond/cache storage — so this +// channel asks the platform directly. +// +// Native handler: NativeChannels.kt (`BLE_NATIVE_CHANNEL`), registered on the +// long-lived engine so it also answers during headless background syncs. +// Requires BLUETOOTH_CONNECT on API 31+ — the same runtime permission every +// flutter_blue_plus GATT operation already needs, so by the time a link is +// connected the permission is held; a denial surfaces here as null. + +import 'package:flutter/foundation.dart'; +import 'package:flutter/services.dart'; + +class AndroidNativeName { + static const MethodChannel _ch = MethodChannel('openstrap/ble_native'); + + /// The native Android name for [remoteId] (the MAC on Android), or null when + /// the platform has none — which is exactly the value the readiness gate + /// compares against. Errors (missing permission, invalid MAC, no adapter) + /// are logged and reported as null: the gate must never pass on a name we + /// could not actually read. + static Future of(String remoteId) async { + try { + return await _ch.invokeMethod('remoteDeviceName', remoteId); + } catch (e) { + debugPrint('[ble_native] remoteDeviceName($remoteId) failed: $e'); + return null; + } + } +} From 95ac27a39abc0f196c520a9c3717147fe14c31bb Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Tue, 25 Aug 2026 18:08:53 +0200 Subject: [PATCH 02/16] fix(ble): the complete official gen5 connection bootstrap, through READY MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gen5 connect now runs the officially recovered order end to end (docs 01/02/07 + the 2026-07-31 hello and 2026-08-18 sync-HCI captures): connect(autoConnect=false, 20 s) -> prefer LE 2M PHY (logged, non-fatal) -> discover + validate fd4b -> request MTU 247 (source intent; band may originate the exchange) -> bond (skip when bonded; refusal is FATAL: no subscriptions, no HELLO, repair guide + clean teardown) -> 600 ms -> serial required registrations (Memfault stays optional) -> 500 ms -> GET_HELLO(145, 01) -> Android native name non-null -> serial/CPU fully alphanumeric -> clock contract -> awaited GET_ADVERTISING_NAME(141, 01) -> READY -> charging-only opcode-151 follow-up. HELLO is mandatory now, not best-effort: write failure, timeout, terminal FAILURE/UNSUPPORTED, or a SUCCESS whose body never parsed all fail the connection — no GET_CLOCK fallback. The consecutive exchange-failure counter survives reconnects, removes the platform bond exactly once at five, and clears ONLY when a bootstrap reaches READY; recovery stays with the existing reconnect owner (no nested reconnect). Identity is enforced (it was logged-only): serial and CPU must each fully match [A-Za-z0-9]+; the all-zero serial still passes and keeps its EEPROM diagnostic. The name gate reads the native bridge, never fbp's cache, and is exactly non-null. Clock: hello's timestamp (subseconds included) is the reading — zero is PRESENT, so the parsed-hello path never sends GET_CLOCK. Below two whole seconds of delta against freshly sampled phone time: no write. At two or more: one awaited SET_CLOCK(10), 8-byte body, no read-back; a null response fails readiness. The phone-suspect deferral no longer returns READY around the contract — it fails the connect until the phone corrects. The scanner accepts a result only for an advertised WHOOP service UUID (the name.contains fallback is gone), and the discovered generation is persisted on the pairing so known-device reconnects — which skip scanning — take the official order from the first pre-discovery step. Without a hint the legacy order runs once and self-heals. Bond position, PHY, discovery, MTU and registration go through one injectable GattBootstrapOps seam (production = flutter_blue_plus) so the order is testable without a radio. gen4 keeps its proven flow unchanged. --- lib/ble/ble_engine.dart | 752 +++++++++++++++++++++++++++++----- lib/ble/ble_state.dart | 53 ++- lib/state/app_state.dart | 30 +- lib/sync/background_sync.dart | 3 +- lib/sync/paired_device.dart | 52 ++- 5 files changed, 764 insertions(+), 126 deletions(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index 4fc74ee8..e76a356d 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -48,6 +48,7 @@ import '../platform/tasker_bridge.dart'; import '../sync/paired_device.dart' show cleanDeviceLabel; import '../sync/sync_policy.dart'; import 'adapters/_registry.dart'; +import 'android_native_name.dart'; import 'ble_state.dart'; // Little-endian u32 reader. The package keeps `u32` private, and the engine only @@ -646,6 +647,140 @@ class _Session { } } +/// How [BleEngine._connectGen5Official] ended. +enum _Gen5ConnectOutcome { + /// Bootstrap completed; the session is listening (READY). + ready, + + /// Setup failed and the session was torn down. + failed, + + /// Discovery contradicted the gen5 hint — the caller falls back to the + /// legacy connect order for whatever the device actually is. + notGen5, +} + +/// The platform seam for the official gen5 connect order: PHY preference, +/// discovery/validation, MTU intent, bond, notification registration. +/// +/// One production implementation ([_FbpGattOps]) wraps flutter_blue_plus; the +/// sequence itself lives in [BleEngine._connectGen5Official], so injecting a +/// recorder here tests the REAL order rather than a parallel one. +abstract class GattBootstrapOps { + /// Whether the explicit bond step applies. Android bonds explicitly; iOS + /// bonds implicitly on the first encrypted operation, as in the legacy flow. + bool get bondingApplies; + + /// Ask for LE 2M PHY. Throws when the request fails — logged, non-fatal. + Future preferLe2mPhy(); + + /// Discover services, pin the session's band and stash the WHOOP + /// characteristics. Returns the discovered band, or null when no WHOOP + /// service — or a required characteristic — is present. + Future discoverAndValidate(); + + /// Request the ATT MTU; returns the negotiated value, throws on failure. + Future requestMtu(int mtu); + + /// Whether an OS bond already exists (skip creating another). + Future isBonded(); + + /// Create the OS bond and wait for it to complete; throws on refusal or + /// failure. + Future createBond(); + + /// Register one required notification ('cmd_from' | 'events' | 'data'); + /// throws when registration fails. + Future subscribe(String role); +} + +/// The flutter_blue_plus implementation of [GattBootstrapOps]. +class _FbpGattOps implements GattBootstrapOps { + final BleEngine _engine; + final BluetoothDevice _device; + final _Session _session; + BluetoothCharacteristic? _cmdFrom, _events, _data; + + _FbpGattOps(this._engine, this._device, this._session); + + @override + bool get bondingApplies => Platform.isAndroid; + + @override + Future preferLe2mPhy() async { + // "when Android supports it" — the request is Android-only + // (flutter_blue_plus throws androidOnly elsewhere); iOS never asks. + if (!Platform.isAndroid) return; + await _device.setPreferredPhy( + txPhy: Phy.le2m.mask, + rxPhy: Phy.le2m.mask, + option: PhyCoding.noPreferred, + ); + } + + @override + Future discoverAndValidate() async { + final services = + await _device.discoverServices().timeout(BleEngine._serviceDiscoveryTimeout); + BluetoothService? svc; + BandProfile band = BandProfile.gen4; + for (final s in services) { + final u = s.uuid.str.toLowerCase(); + if (u.startsWith(GattProfile.gen4.servicePrefix)) { + svc = s; + band = BandProfile.gen4; + break; + } + if (u.startsWith(GattProfile.gen5.servicePrefix)) { + svc = s; + band = BandProfile.gen5; + break; + } + } + if (svc == null) return null; + _session.applyBand(band); + final gatt = band.gatt; + BluetoothCharacteristic? find(String prefix) { + for (final c in svc!.characteristics) { + if (c.uuid.str.toLowerCase().startsWith(prefix)) return c; + } + return null; + } + + _session.cmdTo = find(gatt.cmdTo.substring(0, 8)); + _cmdFrom = find(gatt.cmdFrom.substring(0, 8)); + _events = find(gatt.events.substring(0, 8)); + _data = find(gatt.data.substring(0, 8)); + if (_session.cmdTo == null || + _cmdFrom == null || + _events == null || + _data == null) { + return null; + } + return band; + } + + @override + Future requestMtu(int mtu) => _device.requestMtu(mtu); + + @override + Future isBonded() async => + await _device.bondState.first == BluetoothBondState.bonded; + + @override + Future createBond() => _device.createBond(); + + @override + Future subscribe(String role) { + final c = switch (role) { + 'cmd_from' => _cmdFrom, + 'events' => _events, + _ => _data, + }; + return _engine._subscribe(_session, c!, role); + } +} + class BleEngine { final SampleSink onRecord; final StateSink onState; @@ -1202,6 +1337,33 @@ class BleEngine { return _bootstrapAfterRegistration(session); } + /// Injectable Android native-name getter — the post-HELLO gate reads the + /// platform `BluetoothDevice.getName()`, which only exists behind a radio; + /// tests inject a fake so the gate itself is exercisable anywhere. + /// Production leaves it null: the gate then applies exactly on Android and + /// reads through the [AndroidNativeName] channel. + @visibleForTesting + Future Function(String remoteId)? debugNativeNameReader; + + bool get _nameGateApplies => + debugNativeNameReader != null || Platform.isAndroid; + + Future _nativeName(String remoteId) => + (debugNativeNameReader ?? AndroidNativeName.of)(remoteId); + + /// Drive the REAL official gen5 connect order (PHY → discovery → MTU → + /// bond → 600 ms → registrations → post-registration bootstrap → READY + + /// INIT) over an injected [GattBootstrapOps] on the fake link installed by + /// [debugInstallFakeLink]. This is the production sequence, not a copy — + /// the seam only replaces the radio. + @visibleForTesting + Future debugConnectGen5Official(GattBootstrapOps ops) async { + final session = _session; + if (session == null) return false; + return await _connectGen5Official(session.device, session, ops: ops) == + _Gen5ConnectOutcome.ready; + } + /// Feed one inbound historical frame through the real ingest path (decode → /// plausibility gate → store or archive). /// @@ -1543,11 +1705,16 @@ class BleEngine { /// clock decision. Gen5HelloInfo? _gen5Hello; - /// failures are counted ACROSS reconnect + /// Consecutive HELLO-EXCHANGE failures, counted ACROSS reconnect /// attempts (like `_marginalRadio`/`_postBondLoop`, and deliberately NOT /// reset in the per-connection block in `_doConnect`); at /// [kHelloFailuresBeforeBondReset] the counter resets and the platform bond - /// is removed before starting over. A successful hello clears it. + /// is removed exactly once before starting over — through the existing + /// reconnect owner, never a nested reconnect. Cleared ONLY when a complete + /// bootstrap reaches READY ([_finishConnect]); a hello object arriving is + /// not success. Identity/clock/name failures are connection failures but + /// are deliberately NOT counted here — the evidence scopes this counter to + /// the exchange itself. int _helloFailures = 0; static const int kHelloFailuresBeforeBondReset = 5; @@ -1565,7 +1732,9 @@ class BleEngine { static const int kBatteryPackInfoAttempts = 5; static const Duration kBatteryPackInfoRetryDelay = Duration(seconds: 5); - /// The identity verdict from the last successful hello — observable, never a disconnect. Null until a hello lands. + /// The identity verdict from the last successful hello — ENFORCED by + /// [_gen5PostHelloGates] (a failed verdict fails the connection). Null + /// until a hello lands. HelloIdentity? _helloIdentity; /// The last USABLE `GET_BATTERY_PACK_INFO(151)` reply and when it landed. Diagnostics only — surfaced in @@ -1849,9 +2018,9 @@ class BleEngine { // it drives neither a sync nor the alarm flow. 'last_haptics_termination': _lastHapticsTermination, 'last_haptics_termination_ts': _lastHapticsTerminationTs, - // hello health and the identity gate, both observable rather - // than enforced. `hello_failures` counts ACROSS reconnects and resets - // itself at the bond-reset threshold. + // hello health and the identity verdict (the gate itself is enforced in + // the bootstrap). `hello_failures` counts ACROSS reconnects, resets at + // the bond-reset threshold and clears when a bootstrap reaches READY. 'hello_failures': _helloFailures, 'hello_identity_ok': _helloIdentity?.ok, 'hello_serial_eeprom_failure': _helloIdentity?.eepromFailureSignal, @@ -1931,6 +2100,11 @@ class BleEngine { for (final e in kFramedBands) Guid(e.service), Guid(kWhoopMemberUuid16), ]; + // ACCEPTANCE stays broad (#255) — the match below. The GENERATION HINT is + // narrower: only an advertised 128-bit service names a generation + // ([ScanAcceptPolicy]); a name-only or 16-bit-only match records no hint, + // and the connect path then probes the official gen5 order first and lets + // GATT discovery pin the truth. BluetoothDevice? found; final sub = FlutterBluePlus.onScanResults.listen((results) { for (final r in results) { @@ -1948,6 +2122,12 @@ class BleEngine { s.startsWith('0000fd4b') || kFramedBands.any((e) => s.startsWith(e.servicePrefix))))) { found = r.device; + final adv = ScanAcceptPolicy.accepts( + r.advertisementData.serviceUuids.map((g) => g.str), + ); + if (adv != null) { + _advertisedGeneration[r.device.remoteId.str] = adv; + } unawaited( FlutterBluePlus.stopScan().catchError( (Object e) => _log('stopScan after match failed: $e'), @@ -2040,13 +2220,25 @@ class BleEngine { } /// Reconnect to a previously-paired device by its persisted remote id. - Future connectToRemoteId(String remoteId) => - connect(BluetoothDevice.fromId(remoteId)); + /// + /// [generationHint] is the persisted 'gen4'/'gen5' from the pairing record: + /// a known-device reconnect skips scanning, and the official gen5 connect + /// order differs before discovery, so the generation has to arrive from + /// outside the link. Null runs the legacy order once; discovery then pins + /// the generation and the caller persists it for the next attempt. + Future connectToRemoteId(String remoteId, {String? generationHint}) => + connect(BluetoothDevice.fromId(remoteId), generationHint: generationHint); + + /// What the last accepting scan ADVERTISED per remote id ('gen4'/'gen5'), so + /// a first-ever connect right after a scan takes the generation-correct + /// bootstrap order without waiting for a persisted hint. + final Map _advertisedGeneration = {}; // ── connect ──────────────────────────────────────────────────────────────────── /// Idempotent connect. Serialised through [_opLock] so it can never overlap /// another connect/disconnect. Returns true on a fully-ready link. - Future connect(BluetoothDevice device) => _locked(() async { + Future connect(BluetoothDevice device, {String? generationHint}) => + _locked(() async { // Already connected to this exact peripheral and ready → no-op success. if (_session != null && _session!.connected && @@ -2063,7 +2255,7 @@ class BleEngine { // Any prior session is dead to us now — tear it down before a new one. await _teardownSession(intentional: true); try { - return await _doConnect(device); + return await _doConnect(device, generationHint: generationHint); } catch (e) { // _doConnect guards its own known failure modes, but anything thrown // OUTSIDE those guards (e.g. the connectionState subscription setup, which @@ -2091,7 +2283,7 @@ class BleEngine { _setPhase(BleConnState.idle); } - Future _doConnect(BluetoothDevice device) async { + Future _doConnect(BluetoothDevice device, {String? generationHint}) async { state.address = device.remoteId.str; _setPhase(BleConnState.connecting); final session = _Session(device); @@ -2139,6 +2331,28 @@ class BleEngine { // the setup below (discover/subscribe/SET_CLOCK → bond) is never skipped. session.connected = true; session.sawConnected = true; + + // The official gen5 order differs BEFORE discovery (LE 2M PHY + // preference) and puts the bond after discovery + the MTU intent, so the + // generation must be known ahead of discovery: a scan supplies it from the + // advertisement, a known-device reconnect from the persisted pairing. + // With no hint (the first reconnect after an app update) the legacy order + // below runs once; discovery pins the generation, AppState persists it, + // and every later connect takes the official path. Gen4 always keeps the + // proven legacy flow. + final hint = generationHint ?? _advertisedGeneration[device.remoteId.str]; + if (hint == 'gen5') { + switch (await _connectGen5Official(device, session)) { + case _Gen5ConnectOutcome.ready: + return true; + case _Gen5ConnectOutcome.failed: + return false; + case _Gen5ConnectOutcome.notGen5: + // The hint lied (it names a service the device does not expose) — + // rare enough to pay one extra discovery and take the legacy path. + break; + } + } try { // Bond. On Android we explicitly createBond (the strap gates commands behind // encryption — without a bond the ACK/commands are silently dropped). On iOS @@ -2287,6 +2501,21 @@ class BleEngine { if (data != null) await _subscribe(session, data, 'data'); if (!await _bootstrapAfterRegistration(session)) return false; + return await _finishConnect(session); + } catch (e) { + _log('connect setup failed: $e'); + await _failConnect(); + return false; + } + } + + /// Everything between a completed bootstrap and a live listening link: + /// per-connection policy resets, session timers, the drain controller, the + /// READY transition (with its follow-ups) and INIT. Shared verbatim by the + /// legacy path and the official gen5 path so there is exactly one way a + /// session becomes ready. + Future _finishConnect(_Session session) async { + try { // Fresh clock verification stamp — see kRtcReverifyIntervalSeconds. _lastClockVerifyAt = DateTime.now(); // Per-connection policy reset. Marginal-radio + post-bond-loop are NOT reset @@ -2388,8 +2617,26 @@ class BleEngine { onArchive: onArchiveRecord, log: _log, ); - _setPhase(BleConnState.listening); + // READY ordering: record connection success, clear the + // successful-bootstrap failure state, transition, and only then launch + // the charging follow-up. The hello-failure count clears HERE and + // nowhere earlier — a hello object arriving is not a completed + // bootstrap, and clearing on it would let a link that repeatedly dies + // between hello and READY reset its own counter and never reach the + // five-failure bond reset. _log('Connected + subscribed — listening (history + live).'); + if (_helloFailures > 0) { + _log('[HELLO gen5] bootstrap completed — clearing ' + '$_helloFailures accumulated hello failure(s) at READY.'); + _helloFailures = 0; + } + _setPhase(BleConnState.listening); + // The charging-only battery-pack lookup launches strictly AFTER + // READY, asynchronously; it never blocks or gates anything. + _maybeStartBatteryPackFollowUp(session); + // The INIT drain claim rides the same task-lifecycle rules as every + // other history task ([_startInitDrain]) — quiescence barrier, task + // generation, staleness re-checks, arm/rollback. return await _startInitDrain(session); } catch (e) { _log('connect setup failed: $e'); @@ -2490,6 +2737,145 @@ class BleEngine { return _startInitDrain(session); } + /// The official WHOOP 5 connect order, from an established link through + /// READY: + /// + /// prefer LE 2M PHY → discover + validate the fd4b service → request + /// MTU 247 → establish/await the Android bond → 600 ms → register the + /// required notifications serially → [_bootstrapAfterRegistration] + /// (500 ms → HELLO → name/identity gates → clock → advertising name) → + /// [_finishConnect] (READY + the charging follow-up). + /// + /// The PHY request is a preference, not proof the physical link changed PHY + /// (the official HCI fixture shows no 2M update on a link whose source asked + /// for one) — its failure is logged and non-fatal. MTU 247 is source intent; + /// the band may originate the ATT exchange itself, and a failed request + /// keeps the connection default. A missing service/characteristic, a failed + /// required registration, or a refused bond each prevent READY. + /// + /// All platform work goes through [ops] — one production implementation + /// ([_FbpGattOps]); tests inject a recorder so this exact order is + /// assertable without a radio. + Future<_Gen5ConnectOutcome> _connectGen5Official( + BluetoothDevice device, + _Session session, { + GattBootstrapOps? ops, + }) async { + final gatt = ops ?? _FbpGattOps(this, device, session); + try { + try { + await gatt.preferLe2mPhy(); + _log('[BOOT gen5] LE 2M PHY preference requested (a preference — not ' + 'proof the physical link changed PHY).'); + } catch (e) { + _log('[BOOT gen5] LE 2M PHY preference failed: $e — non-fatal; the ' + 'link stays on its current PHY.'); + } + if (!session.connected || _session != session) { + _log('connect: link dropped before discovery.'); + if (identical(_session, session)) await _failConnect(); + return _Gen5ConnectOutcome.failed; + } + _setPhase(BleConnState.discovering); + final band = await gatt.discoverAndValidate(); + if (band == null) { + _log('[BOOT gen5] required WHOOP service or characteristic missing — ' + 'connection failed.'); + await _failConnect(); + return _Gen5ConnectOutcome.failed; + } + if (!band.isGen5) return _Gen5ConnectOutcome.notGen5; + state.generation = 'gen5'; + _log('Detected WHOOP 5 (gen5) link.'); + try { + final negotiated = await gatt.requestMtu(247); + _log('MTU negotiated: $negotiated (requested 247).'); + } catch (e) { + _log('requestMtu failed: $e — MTU stays at the connection default.'); + } + // Same fast-interval request the legacy path makes for the INIT drain. + _connectSetup = true; + await _applyLinkPriority(); + // Bond — in its official position, after discovery and the MTU intent. + // Already bonded → no second bond. A refused/failed bond is FATAL here: + // the strap gates every command behind encryption, so continuing would + // run subscriptions and HELLO against writes the band silently drops. + if (gatt.bondingApplies) { + try { + if (await gatt.isBonded()) { + _log('[BOOT gen5] already bonded — not creating another bond.'); + } else { + await gatt.createBond(); + _log('Bonded.'); + } + // A clean bond clears the refusal streak + any give-up latch, so a + // later run of refusals can trip the pause again, and un-pauses the + // auto-reconnect loop. + _bondGiveUp.bondSucceeded(); + state.bondRefusals = 0; + state.autoReconnectPaused = false; + } catch (e) { + _log('BOND FAILED: $e — bootstrap stops here (no subscriptions, no ' + 'HELLO, no READY). Remove the bond in system Bluetooth settings ' + 'and re-pair.'); + state.needsRepairGuide = true; + state.bondRefusals++; + // After a run of consecutive refusals, stop the auto-reconnect loop + // (it would otherwise pin the radio + drain the battery on a band + // that will never accept the bond) and surface the re-pair guide. A + // manual user connect still runs the bond, so a successful re-pair + // recovers. + if (_bondGiveUp.bondRefused()) { + state.autoReconnectPaused = true; + _log('[RECONNECT] bond-refusal give-up (${_bondGiveUp.consecutive}) ' + '— pausing auto-reconnect; re-pair required.'); + } + onState(state); + await _failConnect(); + return _Gen5ConnectOutcome.failed; + } + } + if (!session.connected || _session != session) { + _log('connect: link dropped during the bond.'); + if (identical(_session, session)) await _failConnect(); + return _Gen5ConnectOutcome.failed; + } + // 600 ms before notification registration. + if (!await _bootstrapPause( + session, + kGen5PreRegistrationDelay, + 'the pre-registration delay', + )) { + return _Gen5ConnectOutcome.failed; + } + _setPhase(BleConnState.subscribing); + // Serial registration of the REQUIRED notifications; a failed one faults + // setup. Optional Memfault (0007) is deliberately not required, and this + // order is this app's, not a protocol requirement — only one official + // client fixture's registration order was ever captured. + for (final role in const ['cmd_from', 'events', 'data']) { + try { + await gatt.subscribe(role); + } catch (e) { + _log('[BOOT gen5] required notification registration failed ' + '($role): $e — connection failed.'); + await _failConnect(); + return _Gen5ConnectOutcome.failed; + } + } + if (!await _bootstrapAfterRegistration(session)) { + return _Gen5ConnectOutcome.failed; + } + return await _finishConnect(session) + ? _Gen5ConnectOutcome.ready + : _Gen5ConnectOutcome.failed; + } catch (e) { + _log('connect setup failed: $e'); + await _failConnect(); + return _Gen5ConnectOutcome.failed; + } + } + // ── bootstrap ──────────────────────────────────── /// One of the two observed bootstrap delays, with the same stale-session @@ -2515,15 +2901,17 @@ class BleEngine { } /// Everything the phase sequence puts between the last CCC write and - /// READY: the 500 ms post-registration delay, GET_HELLO, the clock decision, - /// the final advertising-name read and the charging follow-up. + /// READY: the 500 ms post-registration delay, the MANDATORY gen5 GET_HELLO, + /// the Android native-name gate, the identity gates, the clock contract and + /// the awaited advertising-name read. [_finishConnect] then owns the READY + /// transition and the charging follow-up. /// /// Lifted out of [_doConnect] because this ORDER is the contract the /// specifies — and as inline statements inside a 400-line connect the only /// way to check it was against a radio. /// - /// Returns false when the link died under one of the steps; the session has - /// already been torn down in that case. + /// Returns false when a step failed or the link died under one; the session + /// has already been torn down in that case. Future _bootstrapAfterRegistration(_Session session) async { // The pause after the last registration, before the higher-level state // machine runs — [BandEntry.postRegistrationDelay], zero on a band with no @@ -2538,34 +2926,49 @@ class BleEngine { return false; } _setPhase(BleConnState.settingUp); - // Set the strap RTC to real wall-clock time. The band ships with an unset - // clock; SET_CLOCK is non-destructive (it is sent routinely on connect - // connect). Records stamped after this carry real unix time. _clockCorrectTries = 0; // fresh retry budget for this connection // Drop the previous session's clock correlation so an alarm armed before - // THIS session's GET_CLOCK reply lands falls back to the raw wall epoch - // (drift 0) instead of the stale strap-RTC frame. The reads below + // THIS session's clock work lands falls back to the raw wall epoch + // (drift 0) instead of the stale strap-RTC frame. The steps below // repopulate it for this connection. _clockRef = null; _gen5Hello = null; - // HELLO FIRST on gen5 — the pinned bootstrap order. Hello - // carries the strap's own timestamp, so it answers the "what time does - // the band think it is" question that the GET_CLOCK below exists to ask, - // and it carries identity/battery/charge/on-body state that everything - // after this wants. The app used to send it late, inside INIT, so none of - // that was available here and gen5 had no serial or battery at connect. - // - // Best effort: a failed or unanswered hello falls through to the ordinary - // clock read, which is the pinned fallback when hello supplies - // no timestamp. Nothing below is gated on it. if (session.band.isGen5) { - await _readGen5Hello(); + // HELLO FIRST on gen5, and MANDATORY. Hello carries the strap's own + // timestamp (the clock decision's input), plus the identity, battery, + // charge and on-body state everything after this wants. A missing or + // failed exchange — write failure, timeout, terminal FAILURE, + // UNSUPPORTED, or a success whose body never parsed — fails the + // CONNECTION: no GET_CLOCK fallback, no identity work, no READY. + // The failure was already counted by _noteHelloFailure (the fifth also + // removes the platform bond); recovery belongs to the reconnect owner, + // never to a nested reconnect from inside this coroutine. + final helloOk = await _readGen5Hello(); if (_session != session || !session.connected) { _log('link dropped during gen5 HELLO — abandoning setup.'); if (identical(_session, session)) await _failConnect(); return false; } + if (!helloOk) { + _log('[HELLO gen5] hello exchange failed — hello is mandatory; ' + 'connection failed.'); + await _failConnect(); + return false; + } + if (!await _gen5PostHelloGates(session)) return false; + if (!await _gen5ClockContract(session)) return false; + // The awaited advertising-name read is the last command before READY. + // Its await completes before READY; its result is not a gate. + await _readAdvertisingNameGen5(session); + if (_session != session || !session.connected) { + _log('link dropped during the advertising-name read — abandoning ' + 'setup.'); + if (identical(_session, session)) await _failConnect(); + return false; + } + return true; } + // ── gen4: the proven legacy clock flow, unchanged ── // READ BEFORE WRITE. This used to be an unconditional SET_CLOCK, which is // precisely the write [ClockPolicy.phoneClockSuspect] says we must never // make: on a phone running >1 day slow it stamps that slow time onto a @@ -2579,22 +2982,12 @@ class BleEngine { // link to drop underneath us, and setClock() absorbs failed writes, so // without these checks setup would carry on past a teardown, rebuild the // drain state and hand back `true` for a dead connection. - // Hello already answered this on gen5, so skip the round trip — the - // pinned flow only falls back to GET_CLOCK when hello carried no - // timestamp. Feed hello's clock through the same handler the GET_CLOCK - // reply uses, so the suspect-phone and unset-RTC verdicts are computed - // from one place regardless of which command supplied the epoch. - // One SET_CLOCK per bootstrap: the reads below run inside the - // window so the absorb handler's own re-correction stands down and - // _bootstrapSetClock is the single writer. + // One SET_CLOCK per bootstrap: the read runs inside the window so the + // absorb handler's own re-correction stands down and _bootstrapSetClock + // is the single writer. _bootstrapClockWrite = true; try { - final helloClock = _gen5Hello?.tsSeconds; - if (helloClock != null && helloClock > 0) { - _absorbClockEpoch(helloClock); - } else { - await _readClock(); - } + await _readClock(); if (_session != session || !session.connected) { _log('link dropped during the clock read — abandoning setup.'); // Tear down ONLY if we are still the live session. `_failConnect` @@ -2610,29 +3003,181 @@ class BleEngine { } if (_session != session || !session.connected) { _log('link dropped during SET_CLOCK — abandoning setup.'); - // Tear down ONLY if we are still the live session. `_failConnect` - // teardown+band-release act on whatever `_session` currently points - // at, so a newer `_doConnect` that already took over would have its - // link killed and its band claim dropped by this stale invocation. + // Tear down ONLY if we are still the live session (see above). if (identical(_session, session)) await _failConnect(); return false; } - // the advertising-name read is the last command before READY, and - // the charging follow-up is launched after it. Neither can fail setup. - await _readAdvertisingNameGen5(session); - _maybeStartBatteryPackFollowUp(session); return true; } - /// The bootstrap SET_CLOCK decision. + /// The post-HELLO readiness gates: the Android native-name + /// requirement and the serial/CPU identity rules. A failed gate is a + /// CONNECTION failure — deliberately not a hello-exchange failure, because + /// the failure counter is scoped to the exchange + /// itself and the evidence never counts these against it. + Future _gen5PostHelloGates(_Session session) async { + final hello = _gen5Hello!; + // Android `BluetoothDevice.getName()` must be non-null. Read from the + // PLATFORM — flutter_blue_plus's `platformName` is an in-memory cache + // that is empty for a device rebuilt with `BluetoothDevice.fromId()` on a + // cold process start, so gating on it would fail every known-device + // reconnect that skipped scanning. The exact gate is non-null (an + // empty-but-present name passes — do not strengthen without evidence). + // Android-only; iOS exposes no equivalent and the source gate is + // Android's. + if (_nameGateApplies) { + final name = await _nativeName(session.device.remoteId.str); + if (_session != session || !session.connected) { + _log('link dropped during the native-name read — abandoning setup.'); + if (identical(_session, session)) await _failConnect(); + return false; + } + if (name == null) { + _log('[BOOT gen5] Android reports no name for this device — ' + 'readiness requires a non-null native name; connection failed.'); + await _failConnect(); + return false; + } + _log('[BOOT gen5] Android native name present ("$name").'); + } + // Family refinement: a recognized discriminator refines the stored type; + // an unrecognized/null mapping is NOT by itself a rejection. + if (hello.isWhoop5) { + _log('[BOOT gen5] family discriminator ${hello.opticalDiscriminator} ' + 'confirms WHOOP 5.0.'); + } else { + _log('[BOOT gen5] family discriminator ${hello.opticalDiscriminator} ' + 'maps to no known family — type stays gen5 (not a rejection).'); + } + // Identity — ENFORCED: serial and CPU must each FULLY match + // [A-Za-z0-9]+; empty and partial matches fail. Evaluated by + // _noteHelloSuccess, judged here. Battery, charging, on-body, firmware, + // hardware, signal-processor, HR-broadcast and error fields are state or + // diagnostics, never gates. + final id = _helloIdentity; + if (id == null || !id.ok) { + _log('[BOOT gen5] identity gate FAILED ($id) — connection failed.'); + await _failConnect(); + return false; + } + if (id.eepromFailureSignal) { + // Passes the alphanumeric gate — a diagnostic, never a rejection. + _log('[HELLO gen5] serial is all zeros — the strap is reporting an ' + 'EEPROM failure. Not a reject; the band stays usable.'); + } + return true; + } + + /// The gen5 bootstrap clock contract: + /// the timestamp hello ALREADY carries (subseconds included) is the strap's + /// clock reading — zero is a present timestamp, not a missing one, so the + /// parsed-hello path never sends GET_CLOCK (opcode 11 exists only as the + /// generic null-timestamp fallback). Compare against a newly sampled phone + /// time: below two whole seconds of absolute delta, succeed with no BLE + /// write; at two or more, send exactly one awaited SET_CLOCK(10). A null + /// SET_CLOCK response fails readiness and disconnects. + Future _gen5ClockContract(_Session session) async { + final hello = _gen5Hello!; + // The absorb handler's own re-correction stands down inside this window + // (_bootstrapClockWrite), leaving this method the single SET_CLOCK writer + // for the bootstrap — including against the clock_epoch retry path. + _bootstrapClockWrite = true; + try { + // Feed the suspect-phone verdict and the strap↔wall correlation the + // same way a GET_CLOCK reply would, so both clock sources share one + // brain. An implausible (unset-RTC) reading is deliberately never + // correlated there; the delta below still forces the correction. + _absorbClockEpoch(hello.tsSeconds); + final helloMs = + hello.tsSeconds * 1000 + (hello.tsSubseconds * 1000) ~/ 32768; + final deltaMs = + (DateTime.now().millisecondsSinceEpoch - helloMs).abs(); + if (!BootstrapClockGate.needsCorrectionMs(deltaMs)) { + _log('[CLOCK] in sync (delta ${deltaMs}ms, tolerance ' + '${BootstrapClockGate.toleranceSeconds}s) — no correction ' + 'needed; no SET_CLOCK written.'); + return true; + } + if (_deferForClock) { + // The PHONE is the suspect party: writing its wall clock onto a + // plausible strap RTC corrupts the RTC and destroys the evidence. + // But READY without a completed clock contract is not allowed + // either — so the connection fails. The reconnect owner retries; + // the moment the phone corrects itself (NTP), the next bootstrap + // completes normally. + _log('[CLOCK] correction needed (delta ${deltaMs}ms) but the PHONE ' + 'clock is the suspect one — refusing to write it onto the strap; ' + 'connection failed (no READY without the clock contract).'); + await _failConnect(); + return false; + } + final ok = await _bootstrapSetClockGen5(); + if (_session != session || !session.connected) { + _log('link dropped during SET_CLOCK — abandoning setup.'); + if (identical(_session, session)) await _failConnect(); + return false; + } + if (!ok) { + _log('[CLOCK] SET_CLOCK failed to write or went unanswered — clock ' + 'synchronization is a readiness requirement; connection failed.'); + await _failConnect(); + return false; + } + return true; + } finally { + _bootstrapClockWrite = false; + } + } + + /// The one bootstrap SET_CLOCK(10): phone timestamp sampled when BUILDING + /// the request, the confirmed 8-byte gen5 body (u32 LE seconds + u32 LE + /// subseconds in 1/32768 s), one correlated await — and deliberately NO + /// GET_CLOCK read-back (the official bootstrap sends none; the periodic + /// re-verify still audits the RTC later). Returns whether a non-null + /// response arrived; per the contract a non-null response object is + /// success regardless of its result byte. + Future _bootstrapSetClockGen5() async { + final ms = DateTime.now().millisecondsSinceEpoch; + final sec = ms ~/ 1000; + final subsec = ((ms % 1000) * 32768) ~/ 1000; // 0..32767, 1/32768 s units + final out = await _sendAwaited(Cmd.setClock, [ + sec & 0xff, + (sec >> 8) & 0xff, + (sec >> 16) & 0xff, + (sec >> 24) & 0xff, + subsec & 0xff, + (subsec >> 8) & 0xff, + 0, + 0, + ]); + if (!out.written) return false; + _log('SET_CLOCK (gen5 bootstrap) → sec=$sec subsec=$subsec — awaiting ' + 'the correlated response.'); + final resp = await out.response; + if (resp != null && resp.success) { + // The strap just took our wall time, so correlate at drift ≈ 0 without + // a read-back — an alarm armed before the next periodic re-verify must + // not be shifted by the drift this write just corrected. + _clockRef = ClockRef( + device: sec, + wall: DateTime.now().millisecondsSinceEpoch ~/ 1000, + ); + } + return resp != null; + } + + /// The LEGACY bootstrap SET_CLOCK decision. The official gen5 path has its + /// own contract ([_gen5ClockContract]) and never comes here; this runs for + /// gen4, and for a gen5 band that reached `_doConnect` because a stored + /// `gen4` hint was wrong — hence the drift gate stays registry-driven + /// ([BandEntry.setClockDriftGated]) rather than being hardcoded to gen4. /// - /// Three rules, in this order: - /// 1. the phone-clock deferral still wins — while THIS phone is the suspect + /// Rules, in order: + /// 1. the phone-clock deferral wins — while THIS phone is the suspect /// party, writing its wall clock onto a possibly-correct strap RTC - /// corrupts the RTC and destroys the evidence (unchanged behaviour); - /// 2. on gen5, below [BootstrapClockGate.toleranceSeconds] of absolute drift - /// the pinned bootstrap makes NO BLE write at all. This app used to send - /// SET_CLOCK unconditionally on every single connect; + /// corrupts the RTC and destroys the evidence; + /// 2. on a drift-gated band, below [BootstrapClockGate.toleranceSeconds] + /// of absolute drift there is NO BLE write at all; /// 3. everything else writes once — including a band with no usable clock /// correlation (unset/implausible RTC), where the drift is null and /// leaving the RTC uncorrected is the one genuinely bad outcome. @@ -2654,15 +3199,14 @@ class BleEngine { } /// `GET_ADVERTISING_NAME(141)` with - /// body `01` and a 5 s timeout is part of the exact bootstrap sequence, sent - /// after the clock step and before READY. + /// body `01` and the correlated 5 s await, sent after the clock step and + /// before READY. (Never the gen4 advertising-name opcode on a gen5 link.) /// - /// "The readiness path does not inspect the returned object or result before - /// transitioning to READY, so this command is part of the exact sequence but - /// is **not** a readiness gate" — so the WRITE is ordered here, and the reply - /// is consumed in the background (same shape as the battery poll): a timeout - /// logs and changes nothing. The name itself lands the way it always has, - /// through the `strap_name` branch of the state absorber. + /// The command must OCCUR — and its await must COMPLETE — before READY, but + /// the returned object/status/content is not a readiness gate: a null, + /// failed or unsupported reply is logged and bootstrap continues. The name + /// itself lands the way it always has, through the `strap_name` branch of + /// the state absorber. Future _readAdvertisingNameGen5(_Session session) async { if (!session.band.isGen5) return; final out = await _sendAwaited( @@ -2674,14 +3218,14 @@ class BleEngine { 'gate; setup continues.'); return; } - // Consumed, never awaited: leaving the pending entry unarmed would hold a - // registry slot for the full timeout with nobody listening. - unawaited(out.response.then((r) { - if (r == null) { - _log('[NAME] GET_ADVERTISING_NAME went unanswered — not a readiness ' - 'gate.'); - } - })); + final r = await out.response; + if (r == null) { + _log('[NAME] GET_ADVERTISING_NAME went unanswered — not a readiness ' + 'gate.'); + } else if (!r.success) { + _log('[NAME] GET_ADVERTISING_NAME status=${r.status} — not a readiness ' + 'gate.'); + } } /// when hello says the band is charging, look @@ -6105,13 +6649,18 @@ class BleEngine { /// read and written by then, so hello's timestamp could never be used and its /// identity fields arrived after everything that wanted them. /// - /// Returns whether a reply landed. A timeout is NOT fatal: the caller falls - /// back to the GET_CLOCK path, which is exactly what the pinned flow does - /// when hello supplies no timestamp. + /// Returns whether a terminal successful, PARSED hello landed. Anything + /// else — write failure, timeout, terminal FAILURE, UNSUPPORTED, a success + /// whose body never parsed — is a failed exchange: counted by + /// [_noteHelloFailure], and the caller fails the CONNECTION (hello is + /// mandatory; there is no GET_CLOCK fallback on the gen5 path). A completed + /// write is NOT success — only the terminal response plus a parsed hello + /// object count. /// Correlated through the [CommandAwaiter]: the reply must echo THIS hello's /// sequence and opcode 145. GET_HELLO is also one of the two commands whose /// `PENDING` is not terminal, so a deferred reply keeps the await /// open for the real result instead of reporting the strap as answered. + /// The timeout is applied exactly once, with no automatic resend. Future _readGen5Hello() async { final out = await _sendAwaited( Cmd.getHello, @@ -6120,14 +6669,13 @@ class BleEngine { frameBuilder: (seq) => gen5ClientHello(seq: seq), ); if (!out.written) { - _log('[HELLO gen5] write failed — falling back to the clock read.'); + _log('[HELLO gen5] write failed.'); await _noteHelloFailure('write failed'); return false; } final resp = await out.response; if (resp == null) { - _log('[HELLO gen5] no reply in ${_helloTimeout.inSeconds}s — falling ' - 'back to GET_CLOCK for the clock decision.'); + _log('[HELLO gen5] no reply in ${_helloTimeout.inSeconds}s.'); await _noteHelloFailure('no reply'); return false; } @@ -6150,24 +6698,19 @@ class BleEngine { /// Matches the standard 5-second command timeout. static const Duration _helloTimeout = Duration(seconds: 5); - /// (identity half) — recorded and logged, never a - /// disconnect. See [HelloIdentity] for why this stays observable. + /// Record the identity verdict of a terminal successful hello. Enforcement + /// happens in [_gen5PostHelloGates] (a failed verdict fails the + /// connection). The accumulated hello-FAILURE count is deliberately NOT + /// cleared here: a hello object arriving is not a completed bootstrap — the + /// counter clears only when the connection reaches READY + /// ([_finishConnect]), so a link that keeps dying between hello and READY + /// still reaches the five-failure bond reset. void _noteHelloSuccess(Gen5HelloInfo h) { - _helloFailures = 0; - final id = HelloIdentity.evaluate( + _helloIdentity = HelloIdentity.evaluate( serial: h.serial, cpuHex: h.cpuHex, eepromFailureSignal: h.serialLooksEepromFailure, ); - _helloIdentity = id; - if (!id.ok) { - _log('[HELLO gen5] identity gate FAILED ($id) — a strict readiness gate ' - 'requires serial and CPU to be alphanumeric; logged, not enforced.'); - } - if (id.eepromFailureSignal) { - _log('[HELLO gen5] serial is all zeros — the strap is reporting an ' - 'EEPROM failure. Not a reject; the band stays usable.'); - } } /// record the failure, and at the fifth @@ -6181,25 +6724,34 @@ class BleEngine { await _removePlatformBond(); } - /// Drop the OS-level bond so the next attempt re-pairs from scratch. + /// Injectable bond remover — production removes the OS bond via + /// flutter_blue_plus; tests inject a counter so the exactly-once semantics + /// of the fifth-failure reset are assertable off-device. + @visibleForTesting + Future Function()? debugBondRemover; + + /// Drop the OS-level bond so the next attempt re-pairs from scratch. The + /// attempt itself belongs to the existing reconnect owner — this NEVER + /// starts a nested reconnect. /// /// Android only: iOS gives no API for removing a pairing, so there the user /// has to forget the device in Settings — say so in the log rather than /// pretending the reset happened. Future _removePlatformBond() async { final device = _session?.device; - if (!Platform.isAndroid) { + final remover = debugBondRemover; + if (remover == null && !Platform.isAndroid) { _log('[HELLO gen5] $kHelloFailuresBeforeBondReset failed hellos — a bond ' 'reset is due, but this platform cannot remove a bond ' 'programmatically; the user must forget the device manually.'); return; } - if (device == null) { + if (remover == null && device == null) { _log('[HELLO gen5] bond reset due but there is no device to unbond.'); return; } try { - await device.removeBond(); + await (remover != null ? remover() : device!.removeBond()); _log('[HELLO gen5] $kHelloFailuresBeforeBondReset failed hellos — ' 'platform bond removed; the next attempt re-pairs.'); } catch (e) { diff --git a/lib/ble/ble_state.dart b/lib/ble/ble_state.dart index 4da50fde..f4207072 100644 --- a/lib/ble/ble_state.dart +++ b/lib/ble/ble_state.dart @@ -1606,18 +1606,17 @@ class CommandAwaiter { void _forget(PendingCommand p) => _pending.remove(p); } -/// The identity half of the bootstrap readiness check, kept as an -/// OBSERVATION rather than a gate. +/// The identity half of the gen5 bootstrap readiness check — ENFORCED. /// -/// A strict readiness gate requires the serial and CPU strings to match -/// `[a-zA-Z0-9]+` before it calls a connection ready. This app records the -/// verdict and logs it rather than dropping the link: a hard disconnect on an -/// identity read we have far less hardware evidence for would turn a cosmetic -/// mismatch into an unreachable band, and the CPU string is lowercase hex by -/// construction so it can only fail if it is empty. +/// After a terminal successful HELLO, readiness requires the serial and CPU +/// strings to each FULLY match `[a-zA-Z0-9]+`. An empty or partially-matching +/// value fails, and the bootstrap treats a failed verdict as a connection +/// failure: no READY, disconnect. The CPU string is lowercase hex by +/// construction, so in practice it can only fail when it is empty — which is +/// precisely a body the parser never filled. /// /// An all-zero serial is an EEPROM-failure signal, NOT a rejection — it passes -/// the alphanumeric gate, and the doc says so explicitly. +/// the alphanumeric gate and is surfaced as a separate diagnostic. class HelloIdentity { static final RegExp alphanumeric = RegExp(r'^[a-zA-Z0-9]+$'); @@ -1680,6 +1679,42 @@ class BootstrapClockGate { /// one outcome worse than a redundant write. static bool needsCorrection(int? driftSec) => driftSec == null || driftSec.abs() >= toleranceSeconds; + + /// The same gate at millisecond resolution, for the gen5 path where hello + /// carries subseconds (32768 units/s) and the comparison is against a newly + /// sampled phone time. "Below two WHOLE seconds of absolute drift, no + /// write" — a delta of 1.999 s has whole-second component 1 and passes; + /// exactly 2.000 s writes. Null keeps the write-on-no-reading rule above. + static bool needsCorrectionMs(int? absDeltaMs) => + absDeltaMs == null || absDeltaMs.abs() >= toleranceSeconds * 1000; +} + +/// Which band a scan result is, by its ADVERTISED service UUIDs — the only +/// thing the scanner may accept on. +/// +/// The scanner accepts a result because its advertisement +/// contains a supported WHOOP service UUID; it does not depend on the display +/// name. This app used to also accept any device whose cached name contained +/// "whoop", which could admit a device advertising no WHOOP service at all — +/// that fallback is gone, and this policy being the single accept decision is +/// what keeps it gone. +class ScanAcceptPolicy { + /// The advertised-service prefixes that identify a WHOOP band: gen4 + /// "Harvard" `61080001-…`, gen5 `fd4b0001-…`. + static const String gen4AdvertisedPrefix = '61080001'; + static const String gen5AdvertisedPrefix = 'fd4b0001'; + + /// The generation the advertisement claims — 'gen4' / 'gen5' — or null when + /// no supported WHOOP service is advertised (not accepted). [serviceUuids] + /// are the advertisement's service UUID strings, any case. + static String? accepts(Iterable serviceUuids) { + for (final raw in serviceUuids) { + final s = raw.toLowerCase(); + if (s.startsWith(gen5AdvertisedPrefix)) return 'gen5'; + if (s.startsWith(gen4AdvertisedPrefix)) return 'gen4'; + } + return null; + } } /// Whether a `GET_BATTERY_PACK_INFO(151)` reply actually identifies a pack. diff --git a/lib/state/app_state.dart b/lib/state/app_state.dart index c8bb3549..2e1f96a3 100644 --- a/lib/state/app_state.dart +++ b/lib/state/app_state.dart @@ -132,7 +132,7 @@ PairedDevice? healedPairing(PairedDevice? current, String? reportedSerial) { final clean = cleanDeviceLabel(reportedSerial); if (clean == null || clean == current.serial) return null; if (current.remoteId.isEmpty) return null; - return PairedDevice(current.remoteId, clean); + return PairedDevice(current.remoteId, clean, generation: current.generation); } class AppState extends ChangeNotifier { @@ -2170,7 +2170,8 @@ class AppState extends ChangeNotifier { _log('===== BACKGROUND SESSION START ====='); try { await _ensureForegroundLease(); - if (await engine.connectToRemoteId(paired!.remoteId)) { + if (await engine.connectToRemoteId(paired!.remoteId, + generationHint: paired!.generation)) { // A process kill followed by an iOS BLE-restore relaunch lands // HERE, not in openSession() — this is the primary case the live // step checkpoint exists for, so recovery has to run on this path @@ -3367,7 +3368,21 @@ class AppState extends ChangeNotifier { final healed = healedPairing(paired, s.serial); if (healed != null) { paired = healed; - unawaited(PairedDevice.save(healed.remoteId, healed.serial)); + unawaited(PairedDevice.save(healed.remoteId, healed.serial, + generation: s.generation)); + } + // Pin the discovered generation onto the pairing record (HEAL ONLY — same + // rule as the serial above: never CREATE a pairing here). A known-device + // reconnect skips scanning, and the official gen5 connect order differs + // before discovery, so the next connect needs this persisted hint. + final p = paired; + if (p != null && + (s.generation == 'gen4' || s.generation == 'gen5') && + s.generation != p.generation) { + paired = + PairedDevice(p.remoteId, p.serial, generation: s.generation); + unawaited( + PairedDevice.save(p.remoteId, p.serial, generation: s.generation)); } // Keep the lock-screen Band Battery widget current — only when it changed. final battPct = roundedPct ?? -1; @@ -4196,7 +4211,8 @@ class AppState extends ChangeNotifier { // link is not up (blocker, bond refusal, repair, quarantine…) and says so // through `engine.bandStatus`, which every surface renders. A second, // staler sentence stored beside it could only disagree with it. - if (!await engine.connectToRemoteId(band.remoteId)) { + if (!await engine.connectToRemoteId(band.remoteId, + generationHint: band.generation)) { _log('Session start: could not reach the band.'); return; } @@ -4319,7 +4335,8 @@ class AppState extends ChangeNotifier { // Mark band ownership before the actual GATT setup so a headless // wake can't fight this reconnect for the peripheral. await _ensureForegroundLease(); - connected = await engine.connectToRemoteId(paired!.remoteId); + connected = await engine.connectToRemoteId(paired!.remoteId, + generationHint: paired!.generation); } else { connected = false; } @@ -4327,7 +4344,8 @@ class AppState extends ChangeNotifier { await Future.delayed(engine.reconnectDelay(attempt)); if (!_keepAlive) break; await _ensureForegroundLease(); - connected = await engine.connectToRemoteId(paired!.remoteId); + connected = await engine.connectToRemoteId(paired!.remoteId, + generationHint: paired!.generation); } if (connected) { // Reclaim the band from the iOS restore central so it stops competing. diff --git a/lib/sync/background_sync.dart b/lib/sync/background_sync.dart index 9c6d8793..0392aeca 100644 --- a/lib/sync/background_sync.dart +++ b/lib/sync/background_sync.dart @@ -90,7 +90,8 @@ Future runHeadlessSync({BandLease? lease}) async { // connect() subscribes → SET_CLOCK → INIT, so the historical offload is already // streaming when this returns. We then await it reaching HISTORY_COMPLETE. - final connected = await engine.connectToRemoteId(paired.remoteId); + final connected = await engine.connectToRemoteId(paired.remoteId, + generationHint: paired.generation); if (!connected) { debugPrint( '[bgsync] strap not reachable this cycle — will catch up next time.', diff --git a/lib/sync/paired_device.dart b/lib/sync/paired_device.dart index 408be6da..8713b622 100644 --- a/lib/sync/paired_device.dart +++ b/lib/sync/paired_device.dart @@ -37,10 +37,26 @@ const String kBandSourceTier = 'wristOptical'; class PairedDevice { static const String _kRemoteId = 'paired_remote_id'; static const String _kSerial = 'paired_serial'; + static const String _kGeneration = 'paired_generation'; final String remoteId; // BLE remote id (iOS: per-install UUID; Android: MAC) final String? serial; - PairedDevice(this.remoteId, this.serial); + + /// 'gen4' / 'gen5', pinned by service discovery on a previous connection, or + /// null before any link has identified itself. A known-device reconnect + /// skips scanning, so this is the only way the connect path can know the + /// generation BEFORE discovery — which the gen5 bootstrap needs, because its + /// PHY preference and bond position differ from gen4's proven flow. + final String? generation; + + PairedDevice(this.remoteId, this.serial, {this.generation}); + + /// The only two values [generation] may hold. `adapter_id` is the whole + /// registry's id space — a notify-only `ble_hrs` / `oura` row names no + /// framed generation — and this value ROUTES the connect order, so anything + /// else reads as unknown rather than steering the bootstrap. + static String? _cleanGeneration(String? g) => + (g == 'gen4' || g == 'gen5') ? g : null; static Future load() async { final row = await LocalDb.deviceRow(); @@ -48,7 +64,11 @@ class PairedDevice { if (id != null && id.isNotEmpty) { // Sanitize on read: drop any garbled value (e.g. "?*" junk persisted by // an older build's HELLO content-scan) so it can never reach the UI. - return PairedDevice(id, cleanDeviceLabel(row?['label'] as String?)); + return PairedDevice( + id, + cleanDeviceLabel(row?['label'] as String?), + generation: _cleanGeneration(row?['adapter_id'] as String?), + ); } // No row: either this install predates the table, or the database was // rebuilt/wiped under a band that is still paired. Same repair either way — @@ -58,27 +78,31 @@ class PairedDevice { final mirrored = prefs.getString(_kRemoteId); if (mirrored == null || mirrored.isEmpty) return null; final serial = cleanDeviceLabel(prefs.getString(_kSerial)); + final generation = _cleanGeneration(prefs.getString(_kGeneration)); await LocalDb.upsertDevice( + adapterId: generation, remoteId: mirrored, label: serial, tier: kBandSourceTier, ); - return PairedDevice(mirrored, serial); + return PairedDevice(mirrored, serial, generation: generation); } - /// Persist the primary band. [adapterId] is the registry's `BandEntry.id` - /// (`gen4` / `gen5`) when the link has said which band this is — omitted, it - /// leaves whatever the row already knows rather than blanking it, and a row - /// that has never been told keeps NULL, which every per-family metric reads - /// as a refusal instead of assuming gen4. + /// Persist the primary band. [generation] is the registry's `BandEntry.id` + /// for a framed band (`gen4` / `gen5`) — the same value the `device` table + /// stores as `adapter_id`, which is why it is passed straight through. + /// Omitted, it leaves whatever the row already knows rather than blanking it + /// (`upsertDevice` COALESCEs), and a row that has never been told keeps + /// NULL, which every per-family metric reads as a refusal instead of + /// assuming gen4. static Future save( String remoteId, String? serial, { - String? adapterId, + String? generation, }) async { final clean = cleanDeviceLabel(serial); await LocalDb.upsertDevice( - adapterId: adapterId, + adapterId: generation, remoteId: remoteId, label: clean, tier: kBandSourceTier, @@ -90,6 +114,13 @@ class PairedDevice { } else { await prefs.remove(_kSerial); // never persist junk } + // Keep the stored generation when a caller doesn't know it — most save + // sites only carry the serial, and a null here must not forget a pinned + // generation (that would demote the next reconnect to the legacy order). + final gen = _cleanGeneration(generation); + if (gen != null) { + await prefs.setString(_kGeneration, gen); + } } /// Forget the primary band. BOTH copies, or the mirror puts it straight back @@ -100,6 +131,7 @@ class PairedDevice { final prefs = await SharedPreferences.getInstance(); await prefs.remove(_kRemoteId); await prefs.remove(_kSerial); + await prefs.remove(_kGeneration); } } From bb19dfba303895e128e239d0919f581ec98f7cce Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Tue, 25 Aug 2026 18:09:03 +0200 Subject: [PATCH 03/16] =?UTF-8?q?test(ble):=20pin=20the=20official=20gen5?= =?UTF-8?q?=20bootstrap=20=E2=80=94=20order,=20gates,=20counter,=20follow-?= =?UTF-8?q?up?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gen5_bootstrap_official_test drives the real _connectGen5Official over a scripted GattBootstrapOps + fake link sharing one trace, and pins: the exact pre-READY order; drift <2 s making no clock write and exactly 2 s making one awaited SET_CLOCK before the advertising name; a zero hello timestamp corrected without GET_CLOCK; PENDING->SUCCESS; every hello failure mode (timeout/FAILURE/UNSUPPORTED/unparsed body/wrong seq/wrong opcode) stopping the sequence; the native-name and identity gates; the all-zero-serial EEPROM diagnostic; the fifth failure removing the bond exactly once with no nested reconnect; failures clearing only at READY; bond/PHY/discovery/registration failure semantics; the awaited-but- non-gating advertising name; supersede safety; and scan acceptance being advertised-service-only. gen5_wiring_test's bootstrap groups move to the new semantics (mandatory hello, awaited 141, the follow-up launching strictly after READY, the suspect-phone deferral failing the connect), and command_correlation_test drops the 'identity is logged, never enforced' framing — the verdict is recorded there and enforced by the bootstrap. --- test/command_correlation_test.dart | 60 +- test/gen5_bootstrap_official_test.dart | 915 +++++++++++++++++++++++++ test/gen5_wiring_test.dart | 254 +++++-- 3 files changed, 1164 insertions(+), 65 deletions(-) create mode 100644 test/gen5_bootstrap_official_test.dart diff --git a/test/command_correlation_test.dart b/test/command_correlation_test.dart index 2cf5f188..2a264098 100644 --- a/test/command_correlation_test.dart +++ b/test/command_correlation_test.dart @@ -64,9 +64,14 @@ class _Link { final written = <({int seq, int opcode})>[]; late final BleEngine engine; + /// Mutable so a test can flip the link's behaviour mid-scenario (e.g. four + /// failed writes, then a working link). + bool writesSucceed; + Decoded? Function(int seq, int opcode)? replyTo; + _Link({ - bool writesSucceed = true, - Decoded? Function(int seq, int opcode)? replyTo, + this.writesSucceed = true, + this.replyTo, }) { engine = BleEngine( onRecord: (_, _) async {}, @@ -490,24 +495,35 @@ void main() { expect(link.engine.helloFailureCount, 1); }); - test('a successful hello clears the accumulated failures', () async { - final failing = _Link(writesSucceed: false); - await failing.engine.debugReadGen5Hello(); - await failing.engine.debugReadGen5Hello(); - expect(failing.engine.helloFailureCount, 2); - - final link = _Link( - replyTo: (seq, opcode) => - opcode == Cmd.getHello ? _helloReply(seq) : null, - ); + test('a successful hello does NOT clear the accumulated failures — only ' + 'READY does', () async { + // A hello object arriving is not a completed bootstrap. Clearing here + // let a link that kept dying between hello and READY reset its own + // counter and never reach the five-failure bond reset; the clear now + // lives at the READY transition (pinned in gen5_bootstrap_official_test). + final link = _Link(writesSucceed: false); await link.engine.debugReadGen5Hello(); - expect(link.engine.helloFailureCount, 0); + await link.engine.debugReadGen5Hello(); + expect(link.engine.helloFailureCount, 2); + + link.writesSucceed = true; + link.replyTo = (seq, opcode) => + opcode == Cmd.getHello ? _helloReply(seq) : null; + expect(await link.engine.debugReadGen5Hello(), isTrue); + expect(link.engine.helloFailureCount, 2, + reason: 'still 2 — the count clears only when the connection ' + 'reaches READY'); }); }); - group('engine wiring — identity is logged, never enforced', () { - test('a non-alphanumeric serial is flagged but the hello still succeeds', - () async { + group('engine wiring — the hello records the identity verdict', () { + // The EXCHANGE succeeding and the IDENTITY passing are different + // questions: _readGen5Hello reports whether a terminal successful, parsed + // hello landed, and records the verdict; the bootstrap + // (_gen5PostHelloGates) then ENFORCES it — a failed verdict fails the + // connection there, which gen5_bootstrap_official_test pins. + test('a non-alphanumeric serial completes the exchange with a failed ' + 'verdict for the bootstrap to enforce', () async { final link = _Link( replyTo: (seq, opcode) => opcode == Cmd.getHello ? _helloReply(seq, serial: 'W5-AB12') @@ -515,13 +531,15 @@ void main() { ); expect(await link.engine.debugReadGen5Hello(), isTrue, - reason: 'a hard disconnect here would brick reconnects'); + reason: 'the exchange itself succeeded — enforcement is the ' + 'bootstrap\'s, and this must NOT count as a hello-exchange ' + 'failure'); + expect(link.engine.helloFailureCount, 0); expect(link.engine.helloIdentity!.ok, isFalse); - expect(link.logs.any((l) => l.contains('identity gate FAILED')), isTrue); expect(link.engine.offloadSnapshot['hello_identity_ok'], isFalse); }); - test('an all-zero serial is reported as an EEPROM failure and passes', + test('an all-zero serial is an EEPROM diagnostic with a PASSING verdict', () async { final link = _Link( replyTo: (seq, opcode) => opcode == Cmd.getHello @@ -530,9 +548,9 @@ void main() { ); expect(await link.engine.debugReadGen5Hello(), isTrue); - expect(link.engine.helloIdentity!.ok, isTrue); + expect(link.engine.helloIdentity!.ok, isTrue, + reason: 'all-zero passes the alphanumeric gate — the official rule'); expect(link.engine.helloIdentity!.eepromFailureSignal, isTrue); - expect(link.logs.any((l) => l.contains('EEPROM')), isTrue); expect( link.engine.offloadSnapshot['hello_serial_eeprom_failure'], isTrue); }); diff --git a/test/gen5_bootstrap_official_test.dart b/test/gen5_bootstrap_official_test.dart new file mode 100644 index 00000000..6d7c9583 --- /dev/null +++ b/test/gen5_bootstrap_official_test.dart @@ -0,0 +1,915 @@ +// The official WHOOP 5 connection bootstrap, end to end. +// +// What this stands in for: the exact readiness sequence recovered from the +// official client — +// +// connect → prefer LE 2M PHY → discover/validate fd4b → MTU 247 → bond → +// 600 ms → register required notifications serially → 500 ms → +// GET_HELLO(145, body 01, 5 s, PENDING non-terminal) → Android native name +// non-null → identity (serial/CPU fully alphanumeric) → clock contract +// (hello's own timestamp incl. subseconds; <2 whole seconds: no write; +// ≥2 s: ONE awaited SET_CLOCK, no read-back) → awaited +// GET_ADVERTISING_NAME(141, body 01) → READY → charging-only opcode-151 +// follow-up — +// +// driven over the REAL production sequence (debugConnectGen5Official runs +// _connectGen5Official itself) with only the radio replaced: a scripted +// GattBootstrapOps records the platform steps and the fake link records every +// command, interleaved with the READY transition in one trace. +// +// The hello-failure counter rules live here too: failures 1–4 record and +// disconnect; the fifth removes the platform bond exactly once and resets the +// counter; NOTHING clears the accumulated count except a complete bootstrap +// reaching READY. + +import 'dart:async'; +import 'dart:typed_data'; + +import 'package:fake_async/fake_async.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:openstrap_edge/ble/ble_engine.dart'; +import 'package:openstrap_edge/ble/ble_state.dart'; +import 'package:openstrap_protocol/openstrap_protocol.dart'; + +int _wallNow() => DateTime.now().millisecondsSinceEpoch ~/ 1000; + +/// A revision-1 gen5 hello body built for the real protocol parser. +Uint8List _helloBody({ + required int tsSeconds, + String serial = 'W5AB12CD34', + bool charging = false, +}) { + final body = Uint8List(Gen5HelloInfo.semanticBodyLen); + final v = ByteData.sublistView(body); + body[0] = 1; // hello revision + v.setUint32(1, 730, Endian.little); + body[5] = charging ? 1 : 0; + v.setUint32(6, tsSeconds, Endian.little); + for (var i = 0; i < serial.length && 14 + i < 25; i++) { + body[14 + i] = serial.codeUnitAt(i); + } + v.setUint32(87, 82, Endian.little); // optical discriminator ⇒ WHOOP 5 + body[91] = 50; + body[92] = 40; + body[93] = 1; + body[102] = 1; + return body; +} + +Decoded _helloReply( + int seq, { + int? tsSeconds, + int status = CommandAwaiter.statusSuccess, + String serial = 'W5AB12CD34', + bool charging = false, + Gen5HelloInfo? hello, +}) => Decoded('cmd_response', { + 'opcode': Cmd.getHello, + 'req_seq': seq, + 'cmd_status': status, + if (status == CommandAwaiter.statusSuccess) + 'gen5_hello': + hello ?? + Gen5HelloInfo.parse( + _helloBody( + tsSeconds: tsSeconds ?? _wallNow(), + serial: serial, + charging: charging, + ), + )!, +}); + +Decoded _clockAck(int seq) => Decoded('cmd_response', { + 'opcode': Cmd.setClock, + 'req_seq': seq, + 'cmd_status': CommandAwaiter.statusSuccess, +}); + +Decoded _nameReply(int seq) => Decoded('cmd_response', { + 'opcode': Cmd.getCustomAdvertisingName, + 'req_seq': seq, + 'cmd_status': CommandAwaiter.statusSuccess, +}); + +/// The fake link + scripted platform ops, sharing ONE trace: +/// 'phy' / 'discover' / 'mtu:247' / 'bond:check' / 'bond:create' / +/// 'sub:{role}' / 'native_name' / 'cmd:{opcode}' / 'ready'. +class _Rig { + final logs = []; + final commands = <({int seq, int opcode, List body})>[]; + final afterSupersede = []; + final trace = []; + late final BleEngine engine; + + Decoded? Function(int seq, int opcode)? replyTo; + + /// What the injected Android native-name reader answers; the reader also + /// records the remoteId it was asked about. + String? nativeName = 'WHOOP 4A0X'; + final nativeNameQueries = []; + + bool _sawReady = false; + bool get ready => _sawReady; + + int bondRemovals = 0; + + _Rig() { + engine = BleEngine( + onRecord: (_, _) async {}, + onState: (s) { + if (!_sawReady && s.connection == 'connected') { + _sawReady = true; + trace.add('ready'); + } + }, + log: logs.add, + ); + engine.debugNativeNameReader = (remoteId) async { + nativeNameQueries.add(remoteId); + trace.add('native_name'); + return nativeName; + }; + engine.debugBondRemover = () async { + bondRemovals++; + }; + engine.debugInstallFakeLink( + band: BandProfile.gen5, + onWrite: (frame) async { + final inner = parseFrame(frame, profile: BandProfile.gen5)!.inner; + commands.add((seq: inner[1], opcode: inner[2], body: inner.sublist(3))); + trace.add('cmd:${inner[2]}'); + final reply = replyTo?.call(inner[1], inner[2]); + if (reply != null) engine.debugAbsorbDecoded(reply); + return true; + }, + ); + } + + List get opcodes => commands.map((c) => c.opcode).toList(); + int count(int opcode) => opcodes.where((o) => o == opcode).length; + bool logged(String needle) => logs.any((l) => l.contains(needle)); + + /// The standard all-answering strap: hello (with [tsSeconds]/[serial]/ + /// [charging]), SET_CLOCK ack, advertising-name reply. + void answerAll({ + int? tsSeconds, + String serial = 'W5AB12CD34', + bool charging = false, + }) { + replyTo = (seq, op) => switch (op) { + Cmd.getHello => _helloReply( + seq, + tsSeconds: tsSeconds ?? _wallNow(), + serial: serial, + charging: charging, + ), + Cmd.setClock => _clockAck(seq), + Cmd.getCustomAdvertisingName => _nameReply(seq), + _ => null, + }; + } + + void supersedeSession() { + engine.debugInstallFakeLink( + band: BandProfile.gen5, + onWrite: (frame) async { + afterSupersede.add( + parseFrame(frame, profile: BandProfile.gen5)!.inner[2], + ); + return true; + }, + ); + } +} + +class _Ops implements GattBootstrapOps { + final _Rig rig; + final bool alreadyBonded; + final bool phyFails; + final bool bondFails; + final bool discoveryFails; + final Set failSubscribe; + + _Ops( + this.rig, { + this.alreadyBonded = false, + this.phyFails = false, + this.bondFails = false, + this.discoveryFails = false, + this.failSubscribe = const {}, + }); + + @override + bool get bondingApplies => true; + + @override + Future preferLe2mPhy() async { + rig.trace.add('phy'); + if (phyFails) throw Exception('PHY_UPDATE unsupported'); + } + + @override + Future discoverAndValidate() async { + rig.trace.add('discover'); + return discoveryFails ? null : BandProfile.gen5; + } + + @override + Future requestMtu(int mtu) async { + rig.trace.add('mtu:$mtu'); + return mtu; + } + + @override + Future isBonded() async { + rig.trace.add('bond:check'); + return alreadyBonded; + } + + @override + Future createBond() async { + rig.trace.add('bond:create'); + if (bondFails) throw Exception('bond refused'); + } + + @override + Future subscribe(String role) async { + rig.trace.add('sub:$role'); + if (failSubscribe.contains(role)) throw Exception('CCC write failed'); + } +} + +/// Run the real official connect under [async]. Null until it settles. +bool? _run( + _Rig rig, + FakeAsync async, { + _Ops? ops, + Duration elapse = const Duration(seconds: 8), +}) { + bool? ok; + rig.engine.debugConnectGen5Official(ops ?? _Ops(rig)).then((v) => ok = v); + async.elapse(elapse); + return ok; +} + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + setUp(BleEngine.resetBandClaimForTest); + tearDown(BleEngine.resetBandClaimForTest); + + group('the successful sequence, in order, through READY', () { + test( + 'drift below two seconds: no GET_CLOCK, no SET_CLOCK, exact order', + () { + fakeAsync((async) { + final rig = _Rig()..answerAll(); + expect(_run(rig, async), isTrue); + + // The exact pre-READY portion of the trace, in order. + final readyAt = rig.trace.indexOf('ready'); + expect(readyAt, isNot(-1)); + expect(rig.trace.sublist(0, readyAt), [ + 'phy', + 'discover', + 'mtu:247', + 'bond:check', + 'bond:create', + 'sub:cmd_from', + 'sub:events', + 'sub:data', + 'cmd:${Cmd.getHello}', + 'native_name', + 'cmd:${Cmd.getCustomAdvertisingName}', + ]); + expect(rig.count(Cmd.getClock), 0); + expect(rig.count(Cmd.setClock), 0); + expect( + rig.commands.first.body, + [0x01], + reason: 'GET_HELLO body is the fixed revision byte 01', + ); + // The gen4 advertising-name opcode must never ride a gen5 link. + expect(rig.opcodes, isNot(contains(Cmd.getAdvertisingNameHarvard))); + }); + }, + ); + + test('drift of exactly two seconds: one awaited SET_CLOCK before the ' + 'advertising name and READY', () { + fakeAsync((async) { + final rig = _Rig()..answerAll(tsSeconds: _wallNow() - 2); + expect(_run(rig, async), isTrue); + + expect( + rig.count(Cmd.setClock), + 1, + reason: + '"at 2 or more, send one SET_CLOCK" — the threshold is ' + 'inclusive', + ); + expect(rig.count(Cmd.getClock), 0, reason: 'and no read-back'); + final t = rig.trace; + expect( + t.indexOf('cmd:${Cmd.setClock}'), + lessThan(t.indexOf('cmd:${Cmd.getCustomAdvertisingName}')), + ); + expect( + t.indexOf('cmd:${Cmd.getCustomAdvertisingName}'), + lessThan(t.indexOf('ready')), + ); + // The 8-byte confirmed gen5 body: u32 LE seconds + u32 LE subseconds + // (the inner packet pads to a 4-byte boundary behind it). The seconds + // are a FRESH phone sample taken when the request was built. + final body = rig.commands + .firstWhere((c) => c.opcode == Cmd.setClock) + .body; + expect(body.length, greaterThanOrEqualTo(8)); + final sec = + body[0] | (body[1] << 8) | (body[2] << 16) | (body[3] << 24); + expect( + (sec - _wallNow()).abs(), + lessThanOrEqualTo(2), + reason: + 'the write carries newly sampled phone time, not the ' + 'hello timestamp', + ); + }); + }); + + test( + 'a zero HELLO timestamp is PRESENT: one SET_CLOCK, never GET_CLOCK', + () { + fakeAsync((async) { + final rig = _Rig()..answerAll(tsSeconds: 0); + expect(_run(rig, async), isTrue); + + expect( + rig.count(Cmd.setClock), + 1, + reason: + 'zero means the RTC needs correcting, not that the ' + 'timestamp is missing', + ); + expect( + rig.count(Cmd.getClock), + 0, + reason: + 'GET_CLOCK is only the generic null-timestamp fallback, ' + 'and a parsed hello always carries the timestamp', + ); + }); + }, + ); + + test('HELLO PENDING → SUCCESS completes the bootstrap', () { + fakeAsync((async) { + final rig = _Rig(); + rig.replyTo = (seq, op) => switch (op) { + Cmd.getHello => () { + // Terminal SUCCESS arrives 800 ms after the PENDING. + Timer(const Duration(milliseconds: 800), () { + rig.engine.debugAbsorbDecoded(_helloReply(seq)); + }); + return _helloReply(seq, status: CommandAwaiter.statusPending); + }(), + Cmd.getCustomAdvertisingName => _nameReply(seq), + _ => null, + }; + expect( + _run(rig, async), + isTrue, + reason: + 'PENDING is non-terminal for GET_HELLO — the await stays ' + 'open for the terminal result', + ); + expect(rig.count(Cmd.getHello), 1, reason: 'no resend on PENDING'); + expect(rig.engine.helloFailureCount, 0); + }); + }); + }); + + group('HELLO is mandatory — every failure mode stops the bootstrap', () { + void expectNothingAfterHello(_Rig rig) { + expect( + rig.trace, + isNot(contains('native_name')), + reason: 'no name gate after a failed hello', + ); + expect(rig.count(Cmd.setClock), 0); + expect( + rig.count(Cmd.getClock), + 0, + reason: 'no GET_CLOCK fallback — hello is mandatory', + ); + expect(rig.count(Cmd.getCustomAdvertisingName), 0); + expect(rig.ready, isFalse); + expect( + rig.engine.isConnected, + isFalse, + reason: 'the failed session is torn down', + ); + } + + test('timeout: no reply within five seconds', () { + fakeAsync((async) { + final rig = _Rig(); // nothing answers + expect(_run(rig, async, elapse: const Duration(seconds: 10)), isFalse); + expect(rig.engine.helloFailureCount, 1); + expectNothingAfterHello(rig); + }); + }); + + test('terminal FAILURE', () { + fakeAsync((async) { + final rig = _Rig(); + rig.replyTo = (seq, op) => op == Cmd.getHello + ? _helloReply(seq, status: CommandAwaiter.statusFailure) + : null; + expect(_run(rig, async), isFalse); + expect(rig.engine.helloFailureCount, 1); + expectNothingAfterHello(rig); + }); + }); + + test('UNSUPPORTED', () { + fakeAsync((async) { + final rig = _Rig(); + rig.replyTo = (seq, op) => op == Cmd.getHello + ? _helloReply(seq, status: CommandAwaiter.statusUnsupported) + : null; + expect(_run(rig, async), isFalse); + expect(rig.engine.helloFailureCount, 1); + expectNothingAfterHello(rig); + }); + }); + + test('a SUCCESS whose body never parsed fails the bootstrap', () { + fakeAsync((async) { + final rig = _Rig(); + rig.replyTo = (seq, op) => op == Cmd.getHello + ? Decoded('cmd_response', { + 'opcode': Cmd.getHello, + 'req_seq': seq, + 'cmd_status': CommandAwaiter.statusSuccess, + // no gen5_hello field — the parser rejected the body + }) + : null; + expect( + _run(rig, async), + isFalse, + reason: + 'a completed write, and even a SUCCESS status, is not a ' + 'hello — only the parsed object counts', + ); + expect(rig.engine.helloFailureCount, 1); + expectNothingAfterHello(rig); + }); + }); + + test('a reply with the WRONG SEQUENCE does not satisfy the hello', () { + fakeAsync((async) { + final rig = _Rig(); + rig.replyTo = (seq, op) => + op == Cmd.getHello ? _helloReply(seq + 1) : null; + expect(_run(rig, async, elapse: const Duration(seconds: 10)), isFalse); + expect(rig.engine.helloFailureCount, 1); + expectNothingAfterHello(rig); + }); + }); + + test('a reply with the WRONG OPCODE does not satisfy the hello', () { + fakeAsync((async) { + final rig = _Rig(); + rig.replyTo = (seq, op) => op == Cmd.getHello + ? Decoded('cmd_response', { + 'opcode': Cmd.getClock, + 'req_seq': seq, + 'cmd_status': CommandAwaiter.statusSuccess, + }) + : null; + expect(_run(rig, async, elapse: const Duration(seconds: 10)), isFalse); + expect(rig.engine.helloFailureCount, 1); + expectNothingAfterHello(rig); + }); + }); + }); + + group('the post-HELLO gates', () { + test('a null Android native name prevents READY', () { + fakeAsync((async) { + final rig = _Rig()..answerAll(); + rig.nativeName = null; + expect(_run(rig, async), isFalse); + + expect(rig.ready, isFalse); + expect( + rig.count(Cmd.setClock), + 0, + reason: 'the sequence stops at the name gate', + ); + expect(rig.count(Cmd.getCustomAdvertisingName), 0); + expect( + rig.engine.helloFailureCount, + 0, + reason: + 'a name-gate failure is a CONNECTION failure, not a ' + 'hello-exchange failure — the evidence never counts it there', + ); + expect(rig.logged('requires a non-null native name'), isTrue); + }); + }); + + test('a cold reconnect reads the NATIVE name for the session\'s remote ' + 'id — never flutter_blue_plus\'s platformName cache', () { + fakeAsync((async) { + // The fake link's device is built the way a cold-start reconnect + // builds it — BluetoothDevice.fromId — so its FBP platformName cache + // is EMPTY by construction. Readiness must come from the injected + // native getter, asked about this exact remote id. + final rig = _Rig()..answerAll(); + expect(_run(rig, async), isTrue); + expect( + rig.nativeNameQueries, + ['AA:BB:CC:DD:EE:FF'], + reason: 'exactly one native read, for the session device', + ); + }); + }); + + test('a partially-alphanumeric serial prevents READY', () { + fakeAsync((async) { + final rig = _Rig()..answerAll(serial: 'W5-AB12'); + expect(_run(rig, async), isFalse); + expect(rig.ready, isFalse); + expect(rig.count(Cmd.setClock), 0); + expect(rig.count(Cmd.getCustomAdvertisingName), 0); + expect(rig.logged('identity gate FAILED'), isTrue); + expect( + rig.engine.helloFailureCount, + 0, + reason: + 'an identity failure is a connection failure, not a ' + 'hello-exchange failure', + ); + }); + }); + + test('an empty serial prevents READY', () { + fakeAsync((async) { + final rig = _Rig()..answerAll(serial: ''); + expect(_run(rig, async), isFalse); + expect(rig.logged('identity gate FAILED'), isTrue); + }); + }); + + test('an empty CPU identity prevents READY', () { + fakeAsync((async) { + // The parser hex-encodes the CPU bytes, so a parsed body can only + // fail this gate when the field is EMPTY — construct that directly. + final parsed = Gen5HelloInfo.parse(_helloBody(tsSeconds: _wallNow()))!; + final noCpu = Gen5HelloInfo( + helloRevision: parsed.helloRevision, + batteryPct: parsed.batteryPct, + charging: parsed.charging, + tsSeconds: parsed.tsSeconds, + tsSubseconds: parsed.tsSubseconds, + serial: parsed.serial, + commitHex: parsed.commitHex, + cpuHex: '', + hardwareFamily: parsed.hardwareFamily, + pcbaRevision: parsed.pcbaRevision, + opticalDiscriminator: parsed.opticalDiscriminator, + fwMajor: parsed.fwMajor, + fwMinor: parsed.fwMinor, + fwBuild: parsed.fwBuild, + fwUnreleased: parsed.fwUnreleased, + sigprocMajor: parsed.sigprocMajor, + sigprocMinor: parsed.sigprocMinor, + sigprocPatch: parsed.sigprocPatch, + hrBroadcast: parsed.hrBroadcast, + wristOn: parsed.wristOn, + errorByte: parsed.errorByte, + ); + final rig = _Rig(); + rig.replyTo = (seq, op) => switch (op) { + Cmd.getHello => _helloReply(seq, hello: noCpu), + Cmd.getCustomAdvertisingName => _nameReply(seq), + _ => null, + }; + expect(_run(rig, async), isFalse); + expect(rig.logged('identity gate FAILED'), isTrue); + }); + }); + + test( + 'an all-zero serial passes the gate and keeps the EEPROM diagnostic', + () { + fakeAsync((async) { + final rig = _Rig()..answerAll(serial: '0000000000'); + expect( + _run(rig, async), + isTrue, + reason: + 'all-zero fully matches [A-Za-z0-9]+ — the official rule ' + 'accepts it', + ); + expect(rig.logged('EEPROM'), isTrue); + expect( + rig.engine.offloadSnapshot['hello_serial_eeprom_failure'], + isTrue, + ); + }); + }, + ); + }); + + group('the hello-failure counter and the bond reset', () { + test('the fifth consecutive failure removes the bond exactly once, ' + 'resets the counter, and starts no nested reconnect', () { + fakeAsync((async) { + final rig = _Rig(); // nothing ever answers the hello + for (var i = 1; i <= 4; i++) { + expect( + _run(rig, async, elapse: const Duration(seconds: 10)), + isFalse, + ); + expect( + rig.engine.helloFailureCount, + i, + reason: 'the count survives reconnect attempts', + ); + expect(rig.bondRemovals, 0); + // The reconnect owner (AppState) would build the next session; the + // engine itself must not. Model the owner's next attempt: + rig.engine.debugInstallFakeLink( + band: BandProfile.gen5, + onWrite: (frame) async { + final inner = parseFrame(frame, profile: BandProfile.gen5)!.inner; + rig.commands.add(( + seq: inner[1], + opcode: inner[2], + body: inner.sublist(3), + )); + rig.trace.add('cmd:${inner[2]}'); + return true; + }, + ); + } + final helloWritesBefore = rig.count(Cmd.getHello); + expect(_run(rig, async, elapse: const Duration(seconds: 10)), isFalse); + expect(rig.bondRemovals, 1, reason: 'exactly one bond removal'); + expect( + rig.engine.helloFailureCount, + 0, + reason: 'the fifth failure resets the counter', + ); + expect( + rig.count(Cmd.getHello), + helloWritesBefore + 1, + reason: + 'no nested reconnect: the failed bootstrap wrote its one ' + 'hello and stopped — the next attempt belongs to the ' + 'existing reconnect owner', + ); + expect(rig.engine.isConnected, isFalse); + }); + }); + + test('a successful complete bootstrap clears earlier failures only at ' + 'READY', () { + fakeAsync((async) { + final rig = _Rig(); + // Three failed exchanges first. + rig.replyTo = (seq, op) => op == Cmd.getHello + ? _helloReply(seq, status: CommandAwaiter.statusFailure) + : null; + for (var i = 0; i < 3; i++) { + rig.engine.debugReadGen5Hello(); + async.flushMicrotasks(); + } + expect(rig.engine.helloFailureCount, 3); + + // Now a working strap — but with the advertising name unanswered, so + // there is a window where hello has landed and READY has not. + rig.replyTo = (seq, op) => switch (op) { + Cmd.getHello => _helloReply(seq), + Cmd.setClock => _clockAck(seq), + _ => null, + }; + bool? ok; + rig.engine.debugConnectGen5Official(_Ops(rig)).then((v) => ok = v); + async.elapse(const Duration(seconds: 3)); + expect(ok, isNull, reason: 'still inside the adv-name await'); + expect( + rig.engine.helloFailureCount, + 3, + reason: + 'the hello object arriving did NOT clear the count — ' + 'only READY does', + ); + async.elapse(const Duration(seconds: 4)); + expect(ok, isTrue); + expect(rig.ready, isTrue); + expect( + rig.engine.helloFailureCount, + 0, + reason: 'cleared at the READY transition', + ); + }); + }); + }); + + group('bond, PHY, discovery and registration', () { + test('a failed bond prevents subscriptions, HELLO and READY', () { + fakeAsync((async) { + final rig = _Rig()..answerAll(); + expect(_run(rig, async, ops: _Ops(rig, bondFails: true)), isFalse); + + expect( + rig.trace.where((t) => t.startsWith('sub:')), + isEmpty, + reason: 'no notification registration after a failed bond', + ); + expect( + rig.commands, + isEmpty, + reason: 'no HELLO — no encrypted command at all', + ); + expect(rig.ready, isFalse); + expect( + rig.engine.state.needsRepairGuide, + isTrue, + reason: 'the existing repair guidance surfaces', + ); + expect( + rig.engine.isConnected, + isFalse, + reason: 'the failed session is torn down cleanly', + ); + }); + }); + + test('an already-bonded device does not create another bond', () { + fakeAsync((async) { + final rig = _Rig()..answerAll(); + expect(_run(rig, async, ops: _Ops(rig, alreadyBonded: true)), isTrue); + expect(rig.trace, contains('bond:check')); + expect(rig.trace, isNot(contains('bond:create'))); + }); + }); + + test('a failed PHY preference is logged, non-fatal, and still ordered ' + 'before discovery', () { + fakeAsync((async) { + final rig = _Rig()..answerAll(); + expect( + _run(rig, async, ops: _Ops(rig, phyFails: true)), + isTrue, + reason: 'LE 2M is a preference; its failure never faults setup', + ); + expect( + rig.trace.indexOf('phy'), + lessThan(rig.trace.indexOf('discover')), + ); + expect(rig.logged('LE 2M PHY preference failed'), isTrue); + }); + }); + + test('a missing required service/characteristic prevents READY', () { + fakeAsync((async) { + final rig = _Rig()..answerAll(); + expect(_run(rig, async, ops: _Ops(rig, discoveryFails: true)), isFalse); + expect(rig.ready, isFalse); + expect(rig.commands, isEmpty); + expect( + rig.logged('required WHOOP service or characteristic missing'), + isTrue, + ); + }); + }); + + test('a failed required notification registration prevents READY', () { + fakeAsync((async) { + final rig = _Rig()..answerAll(); + expect( + _run(rig, async, ops: _Ops(rig, failSubscribe: {'events'})), + isFalse, + ); + expect(rig.ready, isFalse); + expect(rig.commands, isEmpty, reason: 'no HELLO after a failed CCC'); + expect(rig.logged('required notification registration failed'), isTrue); + }); + }); + }); + + group('an advertising-name failure is awaited but never a gate', () { + test('a FAILURE reply is logged and the bootstrap reaches READY', () { + fakeAsync((async) { + final rig = _Rig(); + rig.replyTo = (seq, op) => switch (op) { + Cmd.getHello => _helloReply(seq), + Cmd.getCustomAdvertisingName => Decoded('cmd_response', { + 'opcode': Cmd.getCustomAdvertisingName, + 'req_seq': seq, + 'cmd_status': CommandAwaiter.statusFailure, + }), + _ => null, + }; + expect(_run(rig, async), isTrue); + expect(rig.ready, isTrue); + expect(rig.logged('not a readiness gate'), isTrue); + }); + }); + + test('an unanswered read holds READY for its await, then continues', () { + fakeAsync((async) { + final rig = _Rig(); + rig.replyTo = (seq, op) => switch (op) { + Cmd.getHello => _helloReply(seq), + _ => null, + }; + bool? ok; + rig.engine.debugConnectGen5Official(_Ops(rig)).then((v) => ok = v); + async.elapse(const Duration(seconds: 3)); + expect(rig.ready, isFalse, reason: 'the await completes BEFORE READY'); + async.elapse(const Duration(seconds: 5)); + expect(ok, isTrue); + expect(rig.ready, isTrue); + }); + }); + }); + + group('session replacement mid-bootstrap', () { + test( + 'a stale bootstrap cannot tear down or write into the newer session', + () { + fakeAsync((async) { + final rig = _Rig(); // hello never answered → 5 s await in flight + bool? ok; + rig.engine.debugConnectGen5Official(_Ops(rig)).then((v) => ok = v); + async.elapse(const Duration(seconds: 2)); + expect(rig.count(Cmd.getHello), 1, reason: 'mid-hello await'); + + rig.supersedeSession(); + async.elapse(const Duration(seconds: 10)); + + expect(ok, isFalse); + expect( + rig.afterSupersede, + isEmpty, + reason: 'the stale bootstrap never writes onto the new link', + ); + expect(rig.logged('abandoning setup'), isTrue); + expect(rig.ready, isFalse); + }); + }, + ); + + test('a session replaced during the pre-registration delay stops the ' + 'stale bootstrap before any registration', () { + fakeAsync((async) { + final rig = _Rig()..answerAll(); + final ops = _Ops(rig); + bool? ok; + rig.engine.debugConnectGen5Official(ops).then((v) => ok = v); + // Let it get past the bond into the 600 ms pre-registration sleep. + async.elapse(const Duration(milliseconds: 100)); + expect(rig.trace, contains('bond:create')); + rig.supersedeSession(); + async.elapse(const Duration(seconds: 10)); + + expect(ok, isFalse); + expect( + rig.trace.where((t) => t.startsWith('sub:')), + isEmpty, + reason: 'no registration on a session that is gone', + ); + expect(rig.afterSupersede, isEmpty); + }); + }); + }); + + group('scan acceptance is by advertised WHOOP service only', () { + test('a supported advertised service accepts; a name never does', () { + expect( + ScanAcceptPolicy.accepts(['FD4B0001-CCE1-4033-93CE-002D5875F58A']), + 'gen5', + ); + expect( + ScanAcceptPolicy.accepts(['61080001-8d6d-82b8-614a-1c8cb0f8dcc6']), + 'gen4', + ); + expect( + ScanAcceptPolicy.accepts([]), + isNull, + reason: + 'no advertised WHOOP service, no acceptance — there is no ' + 'name parameter to fall back to, by construction', + ); + expect( + ScanAcceptPolicy.accepts(['0000180f-0000-1000-8000-00805f9b34fb']), + isNull, + ); + }); + }); +} diff --git a/test/gen5_wiring_test.dart b/test/gen5_wiring_test.dart index d1584e44..82873309 100644 --- a/test/gen5_wiring_test.dart +++ b/test/gen5_wiring_test.dart @@ -622,10 +622,15 @@ void _events() { /// A gen4/gen5 link with no radio behind it that records every command written /// and can answer selected opcodes from inside the write itself. +/// +/// [trace] interleaves the commands with the READY transition ('ready') so a +/// test can assert what came strictly before/after READY — the charging +/// follow-up's whole contract is that ordering. class _BootstrapLink { final logs = []; final commands = <({int seq, int opcode, List body})>[]; final afterSupersede = []; + final trace = []; final BandProfile band; /// Answers to inject as the reply to a written command. Injected from INSIDE @@ -634,18 +639,35 @@ class _BootstrapLink { Decoded? Function(int seq, int opcode)? replyTo; late final BleEngine engine; + bool _sawReady = false; + bool get ready => _sawReady; - _BootstrapLink({this.band = BandProfile.gen5}) { + _BootstrapLink({this.band = BandProfile.gen5, String? nativeName = 'WHOOP'}) { engine = BleEngine( onRecord: (_, _) async {}, - onState: (_) {}, + onState: (s) { + if (!_sawReady && s.connection == 'connected') { + _sawReady = true; + trace.add('ready'); + } + }, log: logs.add, ); + // The post-HELLO Android native-name gate, exercisable off-device: the + // injected reader stands in for BluetoothDevice.getName(). Default is a + // present name so unrelated tests pass the gate. + if (band.isGen5) { + engine.debugNativeNameReader = (remoteId) async { + trace.add('native_name'); + return nativeName; + }; + } engine.debugInstallFakeLink( band: band, onWrite: (frame) async { final inner = parseFrame(frame, profile: band)!.inner; commands.add((seq: inner[1], opcode: inner[2], body: inner.sublist(3))); + trace.add('cmd:${inner[2]}'); final reply = replyTo?.call(inner[1], inner[2]); if (reply != null) engine.debugAbsorbDecoded(reply); return true; @@ -671,6 +693,73 @@ class _BootstrapLink { } } +/// A well-behaved scripted [GattBootstrapOps] recording into the link's +/// [trace] — enough for the READY-ordering tests here. The failure-mode +/// variants live in gen5_bootstrap_official_test.dart. +class _FakeOps implements GattBootstrapOps { + final _BootstrapLink link; + _FakeOps(this.link); + + @override + bool get bondingApplies => true; + + @override + Future preferLe2mPhy() async => link.trace.add('phy'); + + @override + Future discoverAndValidate() async { + link.trace.add('discover'); + return link.band; + } + + @override + Future requestMtu(int mtu) async { + link.trace.add('mtu:$mtu'); + return mtu; + } + + @override + Future isBonded() async { + link.trace.add('bond:check'); + return false; + } + + @override + Future createBond() async => link.trace.add('bond:create'); + + @override + Future subscribe(String role) async => link.trace.add('sub:$role'); +} + +/// Success replies for the two awaited non-hello bootstrap commands. +Decoded _nameReply(int seq) => Decoded('cmd_response', { + 'opcode': Cmd.getCustomAdvertisingName, + 'req_seq': seq, + 'cmd_status': CommandAwaiter.statusSuccess, + }); + +Decoded _clockAck(int seq) => Decoded('cmd_response', { + 'opcode': Cmd.setClock, + 'req_seq': seq, + 'cmd_status': CommandAwaiter.statusSuccess, + }); + +/// Run the REAL official gen5 connect order over [ops] to completion under +/// [async]. Returns whether the connection reached READY. +bool _runOfficial( + _BootstrapLink link, + FakeAsync async, { + GattBootstrapOps? ops, + Duration elapse = const Duration(seconds: 8), +}) { + bool? ok; + link.engine + .debugConnectGen5Official(ops ?? _FakeOps(link)) + .then((v) => ok = v); + async.elapse(elapse); + return ok ?? false; +} + /// A revision-1 gen5 hello body, parsed by /// the real protocol decoder so the timestamp and charge bit under test are the /// ones a band would actually produce. @@ -720,13 +809,17 @@ Decoded _packReply(int seq, {required String address, String name = ''}) => /// Run the real post-registration bootstrap to completion under [async]. /// Returns whether it reported success. -bool _runBootstrap(_BootstrapLink link, FakeAsync async) { +bool _runBootstrap( + _BootstrapLink link, + FakeAsync async, { + // Long enough for the 500 ms delay plus the awaited steps when they are + // answered (the 3 s clock read on gen4); an unanswered awaited command + // (5 s timeout) needs a caller-supplied longer window. + Duration elapse = const Duration(seconds: 4), +}) { bool? ok; link.engine.debugBootstrapAfterRegistration().then((v) => ok = v); - // Long enough for the 500 ms delay plus every awaited step's own timeout - // (the 3 s clock read on gen4, the 5 s command timeout on gen5), but short - // of the charging follow-up's first 5 s retry gap. - async.elapse(const Duration(seconds: 4)); + async.elapse(elapse); return ok ?? false; } @@ -801,14 +894,22 @@ void _bootstrap() { expect(BootstrapClockGate.needsCorrection(null), isTrue, reason: 'no correlation at all — an unset band RTC must never be ' 'left uncorrected'); + // The ms form the gen5 bootstrap compares with (hello carries + // subseconds): "below two WHOLE seconds" — 1.999 s passes, 2.000 s + // writes. + expect(BootstrapClockGate.needsCorrectionMs(1999), isFalse); + expect(BootstrapClockGate.needsCorrectionMs(2000), isTrue); + expect(BootstrapClockGate.needsCorrectionMs(null), isTrue); }); test('a band whose clock agrees is not written to at all', () { fakeAsync((async) { final link = _BootstrapLink(); - link.replyTo = (seq, op) => op == Cmd.getHello - ? _helloReply(seq, tsSeconds: _wallNow()) - : null; + link.replyTo = (seq, op) => switch (op) { + Cmd.getHello => _helloReply(seq, tsSeconds: _wallNow()), + Cmd.getCustomAdvertisingName => _nameReply(seq), + _ => null, + }; expect(_runBootstrap(link, async), isTrue); expect(link.count(Cmd.setClock), 0, @@ -819,57 +920,99 @@ void _bootstrap() { }); }); - test('a band 3 s out gets exactly one SET_CLOCK', () { + test('a band 3 s out gets exactly one AWAITED SET_CLOCK', () { fakeAsync((async) { final link = _BootstrapLink(); - link.replyTo = (seq, op) => op == Cmd.getHello - ? _helloReply(seq, tsSeconds: _wallNow() - 3) - : null; + link.replyTo = (seq, op) => switch (op) { + Cmd.getHello => _helloReply(seq, tsSeconds: _wallNow() - 3), + Cmd.setClock => _clockAck(seq), + Cmd.getCustomAdvertisingName => _nameReply(seq), + _ => null, + }; expect(_runBootstrap(link, async), isTrue); expect(link.count(Cmd.setClock), 1, reason: 'at 2 or more, send ONE SET_CLOCK'); + expect(link.count(Cmd.getClock), 0, + reason: 'no read-back — the official bootstrap sends none'); expect(link.opcodes.indexOf(Cmd.setClock), greaterThan(link.opcodes.indexOf(Cmd.getHello)), reason: 'the clock decision comes after hello supplies the time'); + expect(link.opcodes.indexOf(Cmd.setClock), + lessThan(link.opcodes.indexOf(Cmd.getCustomAdvertisingName)), + reason: 'and SET_CLOCK completes before the advertising-name ' + 'read'); }); }); - test('an UNSET RTC gets exactly one SET_CLOCK, not two', () { + test('an unanswered SET_CLOCK fails readiness', () { + fakeAsync((async) { + final link = _BootstrapLink(); + // Drift forces the write; nothing ever answers opcode 10. + link.replyTo = (seq, op) => switch (op) { + Cmd.getHello => _helloReply(seq, tsSeconds: _wallNow() - 30), + Cmd.getCustomAdvertisingName => _nameReply(seq), + _ => null, + }; + expect(_runBootstrap(link, async, elapse: const Duration(seconds: 8)), + isFalse, + reason: 'a null SET_CLOCK response fails readiness and ' + 'disconnects'); + expect(link.count(Cmd.setClock), 1, reason: 'no automatic resend'); + expect(link.count(Cmd.getCustomAdvertisingName), 0, + reason: 'nothing later in the sequence may run'); + expect(link.logged('clock synchronization is a readiness requirement'), + isTrue); + }); + }); + + test('an UNSET RTC gets exactly one SET_CLOCK, not two — and never ' + 'GET_CLOCK', () { fakeAsync((async) { // Factory-epoch hello timestamp: below the plausible floor, so it is - // never correlated (drift == null) and needsCorrection(null) is true. - // Before the bootstrap-window fix, BOTH writers fired — the absorb - // handler's own re-correction on the hello reply AND the bootstrap - // clock step — sending a fresh band two SET_CLOCKs back to back, - // against the one-SET_CLOCK-per-bootstrap rule. + // never correlated — but it is a PRESENT timestamp, so the parsed + // hello path must not fall back to GET_CLOCK; the huge delta forces + // the one correction. Before the bootstrap-window fix, BOTH writers + // fired — the absorb handler's own re-correction on the hello reply + // AND the bootstrap clock step. final link = _BootstrapLink(); - link.replyTo = (seq, op) => op == Cmd.getHello - ? _helloReply(seq, tsSeconds: 1000) - : null; + link.replyTo = (seq, op) => switch (op) { + Cmd.getHello => _helloReply(seq, tsSeconds: 1000), + Cmd.setClock => _clockAck(seq), + Cmd.getCustomAdvertisingName => _nameReply(seq), + _ => null, + }; expect(_runBootstrap(link, async), isTrue); expect(link.count(Cmd.setClock), 1, reason: 'ONE SET_CLOCK per bootstrap — the absorb ' 'handler must stand down inside the bootstrap window'); + expect(link.count(Cmd.getClock), 0, + reason: 'zero/implausible is a present timestamp — GET_CLOCK is ' + 'only the generic NULL-timestamp fallback'); }); }); - test('the phone-clock deferral still beats the drift gate', () { + test('the phone-clock deferral now FAILS the connection instead of ' + 'skipping the clock contract', () { fakeAsync((async) { final link = _BootstrapLink(); - // A plausible strap RTC two days AHEAD of us: the phone is the suspect - // party, and the read is too far out to be correlated — so the drift is - // null and the gate alone would write. The deferral must win. + // A plausible strap RTC two days AHEAD of us: the phone is the + // suspect party. Writing its clock onto the strap would corrupt a + // plausible RTC — but READY without a completed clock contract is not + // allowed either, so the connection fails and the reconnect owner + // retries until the phone corrects itself. link.replyTo = (seq, op) => op == Cmd.getHello ? _helloReply(seq, tsSeconds: _wallNow() + 2 * 86400) : null; - expect(_runBootstrap(link, async), isTrue); + expect(_runBootstrap(link, async), isFalse); - expect(BootstrapClockGate.needsCorrection(null), isTrue, - reason: 'the gate would have written…'); - expect(link.count(Cmd.setClock), 0, reason: '…and must not have'); + expect(link.count(Cmd.setClock), 0, + reason: 'the suspect phone clock is never written to the strap'); + expect(link.count(Cmd.getCustomAdvertisingName), 0, + reason: 'the sequence stops at the failed clock step'); expect(link.engine.historyPausedForClock, isTrue); + expect(link.logged('no READY without the clock contract'), isTrue); }); }); @@ -888,9 +1031,12 @@ void _bootstrap() { test('gen5 sends it last, after the clock step', () { fakeAsync((async) { final link = _BootstrapLink(); - link.replyTo = (seq, op) => op == Cmd.getHello - ? _helloReply(seq, tsSeconds: _wallNow() - 3) - : null; + link.replyTo = (seq, op) => switch (op) { + Cmd.getHello => _helloReply(seq, tsSeconds: _wallNow() - 3), + Cmd.setClock => _clockAck(seq), + Cmd.getCustomAdvertisingName => _nameReply(seq), + _ => null, + }; expect(_runBootstrap(link, async), isTrue); expect(link.opcodes.last, Cmd.getCustomAdvertisingName); @@ -899,20 +1045,25 @@ void _bootstrap() { }); }); - test('an unanswered name read does not fail setup', () { + test('an unanswered name read is AWAITED but does not fail setup', () { fakeAsync((async) { final link = _BootstrapLink(); link.replyTo = (seq, op) => op == Cmd.getHello ? _helloReply(seq, tsSeconds: _wallNow()) : null; // Nothing ever answers opcode 141 here. - expect(_runBootstrap(link, async), isTrue, + bool? ok; + link.engine.debugBootstrapAfterRegistration().then((v) => ok = v); + async.elapse(const Duration(seconds: 4)); + expect(ok, isNull, + reason: 'the sequence COMPLETES the 5 s await before READY — an ' + 'unanswered read holds the bootstrap open until its timeout'); + async.elapse(const Duration(seconds: 3)); + expect(ok, isTrue, reason: 'the response content and result are NOT a ' 'readiness gate'); - async.elapse(const Duration(seconds: 6)); expect(link.logged('GET_ADVERTISING_NAME went unanswered'), isTrue); - expect(link.engine.pendingCommandCount, 0, - reason: 'the unawaited response is still consumed'); + expect(link.engine.pendingCommandCount, 0); }); }); @@ -927,8 +1078,10 @@ void _bootstrap() { }); group('T11 — the charging follow-up, opcode 151', () { - /// Bootstrap a gen5 link whose hello reports [charging], answering - /// GET_BATTERY_PACK_INFO with [packAddress] when one is given. + /// Run the FULL official connect (through READY) for a gen5 link whose + /// hello reports [charging], answering GET_BATTERY_PACK_INFO with + /// [packAddress] when one is given. The follow-up launches at READY, so + /// only the full path can exercise it. _BootstrapLink chargingRig( FakeAsync async, { required bool charging, @@ -940,12 +1093,15 @@ void _bootstrap() { if (op == Cmd.getHello) { return _helloReply(seq, tsSeconds: _wallNow(), charging: charging); } + if (op == Cmd.getCustomAdvertisingName) return _nameReply(seq); if (op == Cmd.getBatteryPackInfo && packAddress != null) { return _packReply(seq, address: packAddress, name: packName); } return null; }; - expect(_runBootstrap(link, async), isTrue, + expect( + _runOfficial(link, async, elapse: const Duration(seconds: 2)), + isTrue, reason: 'the follow-up never blocks READY'); return link; } @@ -981,12 +1137,22 @@ void _bootstrap() { }); }); - test('a charging band is asked five times, five seconds apart', () { + test('a charging band is asked five times, five seconds apart — strictly ' + 'after READY', () { fakeAsync((async) { final link = chargingRig(async, charging: true, packAddress: '00:00:00:00:00:00'); expect(link.count(Cmd.getBatteryPackInfo), 1); - expect(link.commands.last.body.first, revision1, + expect(link.trace.indexOf('ready'), + lessThan(link.trace.indexOf('cmd:${Cmd.getBatteryPackInfo}')), + reason: 'the first possible opcode-151 operation is strictly ' + 'after the READY transition'); + expect( + link.commands + .lastWhere((c) => c.opcode == Cmd.getBatteryPackInfo) + .body + .first, + revision1, reason: 'body 01'); for (var expected = 2; expected <= 5; expected++) { From bd319208e2605875e4a1e2665c1744dba11f84e2 Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Tue, 25 Aug 2026 18:44:41 +0200 Subject: [PATCH 04/16] =?UTF-8?q?fix(ble):=20review=20round=20=E2=80=94=20?= =?UTF-8?q?gen5-first=20routing,=20unconditional=20clock=20contract,=20no?= =?UTF-8?q?=20pre-READY=20priority,=20fixture=20registration=20order?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings against the official-bootstrap commit: Routing: an unknown/null generation no longer takes the legacy bond-before-discovery order (an upgraded pairing hit it once; a headless engine that never persisted the generation hit it forever). connectRouteFor sends everything that is not EXPLICITLY gen4 through the official gen5 sequence first — its pre-discovery steps are safe on any device and its discovery identifies the band; a discovered gen4 falls back to the unchanged legacy flow. runHeadlessSync now pins the discovered generation onto the pairing record like the foreground heal does. Clock: the ≥2 s SET_CLOCK is UNCONDITIONAL per doc 01 — a strap two days ahead gets the same one awaited write, phone-suspect or not. The suspect verdict is a history-safety policy, not a bootstrap rule; an accepted correction clears it (the write made strap and phone agree, so the pre-correction reading is stale by construction) so the initial drain is not deferred against evidence that no longer exists. Priority: the pre-READY requestConnectionPriority is gone from the gen5 path — doc 06 found no such call in the official data path. The post-READY offload transition still raises the interval for the drain, and gen4 keeps its legacy setup request. _applyLinkPriority gained an observability hook so the exact-order test proves the absence instead of assuming it. Registrations: the retained official fixture order — command response → optional Memfault (0007) → data → events. Memfault stays optional in both directions: absent/failing never faults setup, and when present its bytes are collected as diagnostics (counted in the snapshot, never parsed, never a readiness input). New coverage: the routing decision itself, the two-days-ahead contract (drain not suppressed), the Memfault-absent path, and the exact order now including the registration sequence and the priority-request absence. --- lib/ble/ble_engine.dart | 161 +++++++++++++++++++------ lib/ble/ble_state.dart | 20 +++ lib/sync/background_sync.dart | 7 ++ test/gen5_bootstrap_official_test.dart | 89 +++++++++++++- test/gen5_wiring_test.dart | 48 +++++--- 5 files changed, 272 insertions(+), 53 deletions(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index e76a356d..0e97eea6 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -692,6 +692,11 @@ abstract class GattBootstrapOps { /// Register one required notification ('cmd_from' | 'events' | 'data'); /// throws when registration fails. Future subscribe(String role); + + /// Register the OPTIONAL Memfault characteristic (0007) when present. + /// Returns whether it was registered; absence or failure NEVER throws — + /// Memfault is not a required characteristic and must not block READY. + Future subscribeOptionalMemfault(); } /// The flutter_blue_plus implementation of [GattBootstrapOps]. @@ -699,7 +704,7 @@ class _FbpGattOps implements GattBootstrapOps { final BleEngine _engine; final BluetoothDevice _device; final _Session _session; - BluetoothCharacteristic? _cmdFrom, _events, _data; + BluetoothCharacteristic? _cmdFrom, _events, _data, _memfault; _FbpGattOps(this._engine, this._device, this._session); @@ -751,6 +756,8 @@ class _FbpGattOps implements GattBootstrapOps { _cmdFrom = find(gatt.cmdFrom.substring(0, 8)); _events = find(gatt.events.substring(0, 8)); _data = find(gatt.data.substring(0, 8)); + // Optional — its absence must not fail validation. + _memfault = find(gatt.memfault.substring(0, 8)); if (_session.cmdTo == null || _cmdFrom == null || _events == null || @@ -779,6 +786,20 @@ class _FbpGattOps implements GattBootstrapOps { }; return _engine._subscribe(_session, c!, role); } + + @override + Future subscribeOptionalMemfault() async { + final c = _memfault; + if (c == null) return false; + try { + await _engine._subscribeMemfault(_session, c); + return true; + } catch (e) { + // Optional means optional: a CCC write failing on 0007 is logged by + // the caller and never faults setup. + return false; + } + } } class BleEngine { @@ -1337,6 +1358,12 @@ class BleEngine { return _bootstrapAfterRegistration(session); } + /// Observability seam onto [_applyLinkPriority]: called on EVERY entry, + /// before the platform guard, so a test tracing the official gen5 sequence + /// can prove no connection-priority request happens pre-READY. + @visibleForTesting + void Function()? debugOnPriorityRequest; + /// Injectable Android native-name getter — the post-HELLO gate reads the /// platform `BluetoothDevice.getName()`, which only exists behind a radio; /// tests inject a fake so the gate itself is exercisable anywhere. @@ -1450,6 +1477,11 @@ class BleEngine { /// keeps this from spamming the radio — would skip the next legitimate /// step-down, leaving the link fast exactly when it should go quiet. Future _applyLinkPriority() async { + // Fires BEFORE the platform guard so a test can prove where priority + // requests do and do not happen — the official gen5 sequence carries none + // before READY (no requestConnectionPriority was + // found in the official data path). + debugOnPriorityRequest?.call(); if (!Platform.isAndroid) return; // iOS picks its own interval if (_priorityInFlight) { // Someone is mid-request; make them re-evaluate when they land rather @@ -2034,6 +2066,9 @@ class BleEngine { 'battery_pack_type_raw': _batteryPack?.batteryPackTypeRaw, 'battery_pack_status': _batteryPack?.statusRaw, 'battery_pack_ts': _batteryPackTs, + // Optional Memfault (0007) traffic — collected, never parsed or required. + 'memfault_chunks': _memfaultChunks, + 'memfault_bytes': _memfaultBytesTotal, 'pending_commands': _awaiter.pendingKeys, }; @@ -2224,8 +2259,9 @@ class BleEngine { /// [generationHint] is the persisted 'gen4'/'gen5' from the pairing record: /// a known-device reconnect skips scanning, and the official gen5 connect /// order differs before discovery, so the generation has to arrive from - /// outside the link. Null runs the legacy order once; discovery then pins - /// the generation and the caller persists it for the next attempt. + /// outside the link. Anything but an explicit 'gen4' probes the official + /// gen5 sequence first ([connectRouteFor]); a discovered gen4 falls back to + /// the unchanged legacy flow. Future connectToRemoteId(String remoteId, {String? generationHint}) => connect(BluetoothDevice.fromId(remoteId), generationHint: generationHint); @@ -2334,22 +2370,23 @@ class BleEngine { // The official gen5 order differs BEFORE discovery (LE 2M PHY // preference) and puts the bond after discovery + the MTU intent, so the - // generation must be known ahead of discovery: a scan supplies it from the - // advertisement, a known-device reconnect from the persisted pairing. - // With no hint (the first reconnect after an app update) the legacy order - // below runs once; discovery pins the generation, AppState persists it, - // and every later connect takes the official path. Gen4 always keeps the - // proven legacy flow. + // route must be chosen ahead of discovery: a scan supplies the generation + // from the advertisement, a known-device reconnect from the persisted + // pairing. [connectRouteFor] sends everything that is not EXPLICITLY gen4 + // — including an unknown/null hint, e.g. a pairing upgraded from an older + // build or a headless engine that never persisted one — through the + // official gen5 sequence first; its discovery identifies the band, and a + // discovered gen4 falls back to the unchanged legacy flow below. final hint = generationHint ?? _advertisedGeneration[device.remoteId.str]; - if (hint == 'gen5') { + if (connectRouteFor(hint) == ConnectRoute.gen5Official) { switch (await _connectGen5Official(device, session)) { case _Gen5ConnectOutcome.ready: return true; case _Gen5ConnectOutcome.failed: return false; case _Gen5ConnectOutcome.notGen5: - // The hint lied (it names a service the device does not expose) — - // rare enough to pay one extra discovery and take the legacy path. + // Discovery found a gen4 service — take the proven legacy path + // (one extra discovery, paid only until the generation persists). break; } } @@ -2793,9 +2830,11 @@ class BleEngine { } catch (e) { _log('requestMtu failed: $e — MTU stays at the connection default.'); } - // Same fast-interval request the legacy path makes for the INIT drain. - _connectSetup = true; - await _applyLinkPriority(); + // Deliberately NO requestConnectionPriority here: no such call was + // found in the official data path, so the official sequence must not + // carry one before READY. The offload transition + // ([_setOffloadActive], post-READY, separately owned) still raises the + // interval for the drain, and gen4 keeps its legacy setup request. // Bond — in its official position, after discovery and the MTU intent. // Already bonded → no second bond. A refused/failed bond is FATAL here: // the strap gates every command behind encryption, so continuing would @@ -2849,20 +2888,41 @@ class BleEngine { return _Gen5ConnectOutcome.failed; } _setPhase(BleConnState.subscribing); - // Serial registration of the REQUIRED notifications; a failed one faults - // setup. Optional Memfault (0007) is deliberately not required, and this - // order is this app's, not a protocol requirement — only one official - // client fixture's registration order was ever captured. - for (final role in const ['cmd_from', 'events', 'data']) { + // Serial registration in the retained official fixture order: command + // response → optional Memfault → data → events (that order is one + // client fixture, not a protocol requirement — but matching it costs + // nothing). A failed REQUIRED registration faults setup; Memfault + // (0007) stays optional in both directions: absent or failing, setup + // continues, and when present its bytes are collected as diagnostics + // without ever becoming a requirement. + Future requiredRegistration(String role) async { try { await gatt.subscribe(role); + return true; } catch (e) { _log('[BOOT gen5] required notification registration failed ' '($role): $e — connection failed.'); await _failConnect(); - return _Gen5ConnectOutcome.failed; + return false; } } + + if (!await requiredRegistration('cmd_from')) { + return _Gen5ConnectOutcome.failed; + } + if (await gatt.subscribeOptionalMemfault()) { + _log('[BOOT gen5] optional Memfault (0007) registered — collected as ' + 'diagnostics only.'); + } else { + _log('[BOOT gen5] optional Memfault (0007) absent or not registered — ' + 'not required; setup continues.'); + } + if (!await requiredRegistration('data')) { + return _Gen5ConnectOutcome.failed; + } + if (!await requiredRegistration('events')) { + return _Gen5ConnectOutcome.failed; + } if (!await _bootstrapAfterRegistration(session)) { return _Gen5ConnectOutcome.failed; } @@ -3098,19 +3158,13 @@ class BleEngine { 'needed; no SET_CLOCK written.'); return true; } - if (_deferForClock) { - // The PHONE is the suspect party: writing its wall clock onto a - // plausible strap RTC corrupts the RTC and destroys the evidence. - // But READY without a completed clock contract is not allowed - // either — so the connection fails. The reconnect owner retries; - // the moment the phone corrects itself (NTP), the next bootstrap - // completes normally. - _log('[CLOCK] correction needed (delta ${deltaMs}ms) but the PHONE ' - 'clock is the suspect one — refusing to write it onto the strap; ' - 'connection failed (no READY without the clock contract).'); - await _failConnect(); - return false; - } + // The contract is UNCONDITIONAL at ≥2 s: one awaited SET_CLOCK with a + // newly sampled phone time — even for a strap reading days ahead of the + // phone. Edge's phone-suspect policy is a HISTORY-safety rule, not a + // bootstrap rule; it must not turn the official clock step into a + // refusal, and a successful correction clears it below (see + // [_bootstrapSetClockGen5]) so the initial drain is not suppressed by a + // verdict the write just made stale. final ok = await _bootstrapSetClockGen5(); if (_session != session || !session.connected) { _log('link dropped during SET_CLOCK — abandoning setup.'); @@ -3162,6 +3216,18 @@ class BleEngine { device: sec, wall: DateTime.now().millisecondsSinceEpoch ~/ 1000, ); + // And the phone-suspect verdict — computed off the PRE-correction hello + // timestamp — is now stale by construction: strap and phone agree + // because this write made them agree. Left set, it would defer the + // initial history drain (and let record-gate age checks judge against a + // reading that no longer exists). The suspect detector still re-trips + // on the next reading if the phone genuinely is wrong. + if (_phoneClockSuspect) { + _phoneClockSuspect = false; + _phoneClockSuspectSince = null; + _log('[CLOCK] SET_CLOCK accepted — clearing the phone-clock-suspect ' + 'verdict from the pre-correction reading; history may drain.'); + } } return resp != null; } @@ -3731,6 +3797,33 @@ class BleEngine { ); } + /// Collect the OPTIONAL Memfault characteristic's bytes as diagnostics. + /// The official client persists/uploads whatever the strap volunteers here + /// and sends nothing to solicit it; this app only counts + /// what arrived (surfaced in [offloadSnapshot]) — the channel is never + /// parsed, never required, and never a readiness input. + Future _subscribeMemfault( + _Session session, + BluetoothCharacteristic c, + ) async { + await c.setNotifyValue(true).timeout(_notifySetupTimeout); + session.subs.add( + c.onValueReceived.listen((chunk) { + if (_session != session || !session.connected) return; + _memfaultChunks++; + _memfaultBytesTotal += chunk.length; + if (_memfaultChunks == 1) { + _log('[MEMFAULT] strap volunteered its first crash/diagnostic ' + 'chunk (${chunk.length} B) — collected only.'); + } + }), + ); + } + + /// Memfault (0007) traffic counters — diagnostics only. + int _memfaultChunks = 0; + int _memfaultBytesTotal = 0; + // ── link-down handling (drives reconnect via the caller's contract) ───────────── void _onLinkDown(_Session session) { if (LinkDownPolicy.evaluate(sessionIsCurrent: _session == session) == diff --git a/lib/ble/ble_state.dart b/lib/ble/ble_state.dart index f4207072..09322b31 100644 --- a/lib/ble/ble_state.dart +++ b/lib/ble/ble_state.dart @@ -1717,6 +1717,26 @@ class ScanAcceptPolicy { } } +/// Which connect order a link gets, decided BEFORE discovery has run. +enum ConnectRoute { + /// The official gen5 sequence (PHY preference → discovery → MTU → bond …). + gen5Official, + + /// The proven legacy gen4 flow (bond → MTU → discovery …), unchanged. + gen4Legacy, +} + +/// The routing rule: only an EXPLICIT gen4 hint takes the legacy order. +/// Unknown/null — a pairing upgraded from an older build, a garbled stored +/// value — probes gen5-first: the official sequence's pre-discovery steps are +/// safe on any device (the PHY request is non-fatal by contract), its +/// discovery identifies the band, and a discovered gen4 falls back to the +/// unchanged legacy flow. Routing unknown links through the legacy order +/// instead would run a gen5 band's bond in the wrong position on every +/// connect until something persisted the generation. +ConnectRoute connectRouteFor(String? generationHint) => + generationHint == 'gen4' ? ConnectRoute.gen4Legacy : ConnectRoute.gen5Official; + /// Whether a `GET_BATTERY_PACK_INFO(151)` reply actually identifies a pack. /// /// "A response is usable only if its pack address/name field is non-empty and diff --git a/lib/sync/background_sync.dart b/lib/sync/background_sync.dart index 0392aeca..d62e7c2a 100644 --- a/lib/sync/background_sync.dart +++ b/lib/sync/background_sync.dart @@ -99,6 +99,13 @@ Future runHeadlessSync({BandLease? lease}) async { await checkSyncStaleness(); return true; } + // Pin the discovered generation onto the pairing record, exactly like the + // foreground engine-state heal does — a headless-only phone would + // otherwise re-probe the connect route on every wake forever. + final gen = engine.state.generation; + if ((gen == 'gen4' || gen == 'gen5') && gen != paired.generation) { + await PairedDevice.save(paired.remoteId, paired.serial, generation: gen); + } try { final plan = await HighFreqWakeWindow.planNow(); await engine.applyHighFreqWakeWindow( diff --git a/test/gen5_bootstrap_official_test.dart b/test/gen5_bootstrap_official_test.dart index 6d7c9583..58f2e2be 100644 --- a/test/gen5_bootstrap_official_test.dart +++ b/test/gen5_bootstrap_official_test.dart @@ -132,6 +132,11 @@ class _Rig { engine.debugBondRemover = () async { bondRemovals++; }; + // Every entry into the engine's connection-priority path lands in the + // trace, so the exact pre-READY order below PROVES the official sequence + // carries no requestConnectionPriority (doc 06 found none in the official + // data path); the post-READY offload transition still may. + engine.debugOnPriorityRequest = () => trace.add('link_priority'); engine.debugInstallFakeLink( band: BandProfile.gen5, onWrite: (frame) async { @@ -188,6 +193,7 @@ class _Ops implements GattBootstrapOps { final bool phyFails; final bool bondFails; final bool discoveryFails; + final bool memfaultPresent; final Set failSubscribe; _Ops( @@ -196,6 +202,7 @@ class _Ops implements GattBootstrapOps { this.phyFails = false, this.bondFails = false, this.discoveryFails = false, + this.memfaultPresent = true, this.failSubscribe = const {}, }); @@ -237,6 +244,12 @@ class _Ops implements GattBootstrapOps { rig.trace.add('sub:$role'); if (failSubscribe.contains(role)) throw Exception('CCC write failed'); } + + @override + Future subscribeOptionalMemfault() async { + rig.trace.add('sub:memfault(opt)'); + return memfaultPresent; + } } /// Run the real official connect under [async]. Null until it settles. @@ -265,7 +278,12 @@ void main() { final rig = _Rig()..answerAll(); expect(_run(rig, async), isTrue); - // The exact pre-READY portion of the trace, in order. + // The exact pre-READY portion of the trace, in order. The equality + // also PROVES two absences: no requestConnectionPriority (every + // entry into that path would land as 'link_priority' — the official + // data path was found to carry none) and no clock traffic. + // Registrations follow the retained official fixture order: + // command response → optional Memfault → data → events. final readyAt = rig.trace.indexOf('ready'); expect(readyAt, isNot(-1)); expect(rig.trace.sublist(0, readyAt), [ @@ -275,8 +293,9 @@ void main() { 'bond:check', 'bond:create', 'sub:cmd_from', - 'sub:events', + 'sub:memfault(opt)', 'sub:data', + 'sub:events', 'cmd:${Cmd.getHello}', 'native_name', 'cmd:${Cmd.getCustomAdvertisingName}', @@ -361,6 +380,32 @@ void main() { }, ); + test('a strap TWO DAYS ahead: the contract is unconditional, and the ' + 'corrected clock does not suppress the initial history drain', () { + fakeAsync((async) { + // The pre-correction hello reading trips Edge's phone-suspect history + // policy. The official contract still writes exactly one SET_CLOCK + // (doc 01: ≥2 s → one awaited write, non-null response → readiness), + // and the accepted correction must clear the now-stale verdict so + // the INIT drain is not deferred against a reading the write erased. + final rig = _Rig()..answerAll(tsSeconds: _wallNow() + 2 * 86400); + expect(_run(rig, async), isTrue); + + expect(rig.count(Cmd.setClock), 1); + expect(rig.count(Cmd.getClock), 0); + expect(rig.ready, isTrue); + expect(rig.engine.historyPausedForClock, isFalse); + expect( + rig.count(Cmd.sendHistoricalData), + 1, + reason: + 'the initial drain went out — a stale suspect verdict ' + 'would have deferred it', + ); + expect(rig.logs.any((l) => l.contains('DEFERRED')), isFalse); + }); + }); + test('HELLO PENDING → SUCCESS completes the bootstrap', () { fakeAsync((async) { final rig = _Rig(); @@ -800,6 +845,46 @@ void main() { expect(rig.logged('required notification registration failed'), isTrue); }); }); + + test('an absent optional Memfault characteristic never blocks READY', () { + fakeAsync((async) { + final rig = _Rig()..answerAll(); + expect( + _run(rig, async, ops: _Ops(rig, memfaultPresent: false)), + isTrue, + reason: + 'Memfault (0007) is optional in the retained fixture — ' + 'its absence must not fault setup', + ); + expect(rig.ready, isTrue); + expect(rig.logged('not required; setup continues'), isTrue); + }); + }); + }); + + group('the connect route is chosen before discovery', () { + // The production routing decision _doConnect makes — an unknown/null + // generation must NOT bypass the official sequence. A pairing upgraded + // from an older Edge build (no persisted generation) and a headless + // engine both arrive here with null. + test('only an explicit gen4 hint takes the legacy order', () { + expect(connectRouteFor('gen4'), ConnectRoute.gen4Legacy); + expect(connectRouteFor('gen5'), ConnectRoute.gen5Official); + expect( + connectRouteFor(null), + ConnectRoute.gen5Official, + reason: + 'unknown probes gen5-first; discovery identifies the band ' + 'and a discovered gen4 falls back to the unchanged legacy flow', + ); + expect( + connectRouteFor('garbled'), + ConnectRoute.gen5Official, + reason: + 'a corrupted stored value must not demote a gen5 band to ' + 'the wrong bond position', + ); + }); }); group('an advertising-name failure is awaited but never a gate', () { diff --git a/test/gen5_wiring_test.dart b/test/gen5_wiring_test.dart index 82873309..16cd3cad 100644 --- a/test/gen5_wiring_test.dart +++ b/test/gen5_wiring_test.dart @@ -729,6 +729,12 @@ class _FakeOps implements GattBootstrapOps { @override Future subscribe(String role) async => link.trace.add('sub:$role'); + + @override + Future subscribeOptionalMemfault() async { + link.trace.add('sub:memfault(opt)'); + return true; + } } /// Success replies for the two awaited non-hello bootstrap commands. @@ -993,26 +999,34 @@ void _bootstrap() { }); }); - test('the phone-clock deferral now FAILS the connection instead of ' - 'skipping the clock contract', () { + test('a strap TWO DAYS ahead still gets the official contract: one ' + 'awaited SET_CLOCK, and a successful correction clears the ' + 'phone-suspect verdict', () { fakeAsync((async) { final link = _BootstrapLink(); - // A plausible strap RTC two days AHEAD of us: the phone is the - // suspect party. Writing its clock onto the strap would corrupt a - // plausible RTC — but READY without a completed clock contract is not - // allowed either, so the connection fails and the reconnect owner - // retries until the phone corrects itself. - link.replyTo = (seq, op) => op == Cmd.getHello - ? _helloReply(seq, tsSeconds: _wallNow() + 2 * 86400) - : null; - expect(_runBootstrap(link, async), isFalse); + // A plausible strap RTC two days AHEAD of the phone trips Edge's + // phone-suspect history policy — but the official clock contract is + // UNCONDITIONAL at ≥2 s, so the bootstrap still writes exactly one + // SET_CLOCK and reaches the advertising name. The accepted write + // makes strap and phone agree by construction, so the pre-correction + // suspect verdict must not survive to suppress the initial drain. + link.replyTo = (seq, op) => switch (op) { + Cmd.getHello => + _helloReply(seq, tsSeconds: _wallNow() + 2 * 86400), + Cmd.setClock => _clockAck(seq), + Cmd.getCustomAdvertisingName => _nameReply(seq), + _ => null, + }; + expect(_runBootstrap(link, async), isTrue); - expect(link.count(Cmd.setClock), 0, - reason: 'the suspect phone clock is never written to the strap'); - expect(link.count(Cmd.getCustomAdvertisingName), 0, - reason: 'the sequence stops at the failed clock step'); - expect(link.engine.historyPausedForClock, isTrue); - expect(link.logged('no READY without the clock contract'), isTrue); + expect(link.count(Cmd.setClock), 1, + reason: 'the contract is unconditional — one awaited write'); + expect(link.count(Cmd.getClock), 0); + expect(link.engine.historyPausedForClock, isFalse, + reason: 'the accepted correction clears the stale suspect ' + 'verdict; history must not stay deferred'); + expect(link.logged('clearing the phone-clock-suspect verdict'), + isTrue); }); }); From 6f8f9f0ad11181e72a614a110a905f975f48b77e Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Tue, 25 Aug 2026 18:44:50 +0200 Subject: [PATCH 05/16] fix(sync): the pairing generation is device-scoped; guard the native name read's permission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PairedDevice.save kept a stored generation across ANY save that did not carry one — including a save for a DIFFERENT remoteId, so a newly paired band inherited the forgotten band's generation and had its first connect routed by the wrong device's identity. The keep now applies only to same-remoteId saves; a new device starts unknown (and probes gen5-first). Loads sanitize the stored value to gen4/gen5/null so a corrupted pref can never steer the connect route. Direct save/load/clear tests pin all of it. The native-name bridge checks BLUETOOTH_CONNECT explicitly on S+ (a revoked grant answers as a clean error instead of a SecurityException) and carries a targeted MissingPermission suppression for the read lint cannot see past the early return — the permission is the same one every GATT operation already holds by the time a link is connected. --- .../openstrap_edge/NativeChannels.kt | 43 +++++-- lib/sync/paired_device.dart | 11 +- test/paired_device_test.dart | 108 ++++++++++++++++++ 3 files changed, 147 insertions(+), 15 deletions(-) create mode 100644 test/paired_device_test.dart diff --git a/android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/NativeChannels.kt b/android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/NativeChannels.kt index 7a114cbf..b54c1da9 100644 --- a/android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/NativeChannels.kt +++ b/android/app/src/main/kotlin/wtf/openstrap/openstrap_edge/NativeChannels.kt @@ -1,5 +1,7 @@ package wtf.openstrap.openstrap_edge +import android.Manifest +import android.annotation.SuppressLint import android.app.ActivityManager import android.bluetooth.BluetoothManager import android.content.ComponentName @@ -141,18 +143,8 @@ object NativeChannels { val mac = call.arguments as? String if (mac.isNullOrEmpty()) { result.error("bad_args", "expected the remote MAC", null) - return@setMethodCallHandler - } - try { - val mgr = app.getSystemService(Context.BLUETOOTH_SERVICE) - as? BluetoothManager - // getName() needs BLUETOOTH_CONNECT on API 31+ (held — - // every GATT op needs it too); a SecurityException or an - // invalid MAC lands in the catch and reaches Dart as an - // error, which the gate reads as "no name". - result.success(mgr?.adapter?.getRemoteDevice(mac)?.name) - } catch (e: Exception) { - result.error("name_unavailable", e.toString(), null) + } else { + remoteDeviceName(app, mac, result) } } else -> result.notImplemented() @@ -300,6 +292,33 @@ object NativeChannels { return token } + /** + * The platform `BluetoothDevice.getName()` read behind the gen5 readiness + * gate. `getName()` needs BLUETOOTH_CONNECT on API 31+ — the same runtime + * permission every GATT operation already holds by the time a link is + * connected, checked explicitly here so a revoked grant answers as a clean + * error instead of a SecurityException. Lint cannot see that check through + * the early return, hence the targeted suppression; the belt-and-braces + * catch still turns any surprise (invalid MAC, no adapter) into the same + * error, which Dart reads as "no name" — the gate's failing value. + */ + @SuppressLint("MissingPermission") + private fun remoteDeviceName(app: Context, mac: String, result: MethodChannel.Result) { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S && + app.checkSelfPermission(Manifest.permission.BLUETOOTH_CONNECT) != + android.content.pm.PackageManager.PERMISSION_GRANTED + ) { + result.error("name_unavailable", "BLUETOOTH_CONNECT not granted", null) + return + } + try { + val mgr = app.getSystemService(Context.BLUETOOTH_SERVICE) as? BluetoothManager + result.success(mgr?.adapter?.getRemoteDevice(mac)?.name) + } catch (e: Exception) { + result.error("name_unavailable", e.toString(), null) + } + } + private fun perform(ctx: Context, action: String): Boolean { return try { when (action) { diff --git a/lib/sync/paired_device.dart b/lib/sync/paired_device.dart index 8713b622..baea44ae 100644 --- a/lib/sync/paired_device.dart +++ b/lib/sync/paired_device.dart @@ -108,18 +108,23 @@ class PairedDevice { tier: kBandSourceTier, ); final prefs = await SharedPreferences.getInstance(); + // A stored generation belongs to a DEVICE. Keep it only when this save is + // for the same remoteId and merely doesn't know the generation (most save + // sites only carry the serial); pairing a DIFFERENT band must never + // inherit the old band's generation — that would route its first connect + // by the wrong device's identity. + final sameDevice = prefs.getString(_kRemoteId) == remoteId; await prefs.setString(_kRemoteId, remoteId); if (clean != null) { await prefs.setString(_kSerial, clean); } else { await prefs.remove(_kSerial); // never persist junk } - // Keep the stored generation when a caller doesn't know it — most save - // sites only carry the serial, and a null here must not forget a pinned - // generation (that would demote the next reconnect to the legacy order). final gen = _cleanGeneration(generation); if (gen != null) { await prefs.setString(_kGeneration, gen); + } else if (!sameDevice) { + await prefs.remove(_kGeneration); } } diff --git a/test/paired_device_test.dart b/test/paired_device_test.dart new file mode 100644 index 00000000..b70cbf50 --- /dev/null +++ b/test/paired_device_test.dart @@ -0,0 +1,108 @@ +// PairedDevice — the persisted pairing record, including the generation the +// connect route is chosen by. +// +// What this stands in for: the generation is a DEVICE property that steers +// the bond position of every reconnect. Persist it wrong and a gen5 band runs +// its bond in the wrong place (or a new band inherits the forgotten band's +// identity), so the save/load/clear semantics get pinned directly: +// same-device saves without a generation must preserve the stored one, +// a DIFFERENT remoteId must never inherit it, and a corrupted stored value +// must sanitize to null rather than steer the route. + +import 'package:flutter_test/flutter_test.dart'; +import 'package:openstrap_edge/sync/paired_device.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + setUp(() => SharedPreferences.setMockInitialValues({})); + + test('save/load round-trips remoteId, serial and generation', () async { + await PairedDevice.save( + 'AA:BB:CC:DD:EE:FF', + '5AG0000001', + generation: 'gen5', + ); + final p = await PairedDevice.load(); + expect(p!.remoteId, 'AA:BB:CC:DD:EE:FF'); + expect(p.serial, '5AG0000001'); + expect(p.generation, 'gen5'); + }); + + test( + 'a same-device save without a generation preserves the stored one', + () async { + await PairedDevice.save( + 'AA:BB:CC:DD:EE:FF', + '5AG0000001', + generation: 'gen5', + ); + // The serial-heal save site only carries the serial. + await PairedDevice.save('AA:BB:CC:DD:EE:FF', '5AG0000002'); + final p = await PairedDevice.load(); + expect(p!.serial, '5AG0000002'); + expect( + p.generation, + 'gen5', + reason: 'not knowing the generation is not evidence it changed', + ); + }, + ); + + test( + 'pairing a DIFFERENT remoteId without a generation drops the old one', + () async { + await PairedDevice.save( + 'AA:BB:CC:DD:EE:FF', + '5AG0000001', + generation: 'gen4', + ); + await PairedDevice.save('11:22:33:44:55:66', '5AG0000009'); + final p = await PairedDevice.load(); + expect(p!.remoteId, '11:22:33:44:55:66'); + expect( + p.generation, + isNull, + reason: + 'a new band must never inherit the forgotten band\'s ' + 'generation — its first connect probes gen5-first instead', + ); + }, + ); + + test( + 'a garbled generation is refused on save and sanitized on load', + () async { + await PairedDevice.save('AA:BB:CC:DD:EE:FF', null, generation: 'gen6'); + expect((await PairedDevice.load())!.generation, isNull); + + // A corrupted value written by some other path never steers the route. + SharedPreferences.setMockInitialValues({ + 'paired_remote_id': 'AA:BB:CC:DD:EE:FF', + 'paired_generation': 'banana', + }); + expect((await PairedDevice.load())!.generation, isNull); + }, + ); + + test('clear removes the whole record, generation included', () async { + await PairedDevice.save( + 'AA:BB:CC:DD:EE:FF', + '5AG0000001', + generation: 'gen5', + ); + await PairedDevice.clear(); + expect(await PairedDevice.load(), isNull); + // Re-pairing after a clear starts with no generation at all. + await PairedDevice.save('AA:BB:CC:DD:EE:FF', null); + expect((await PairedDevice.load())!.generation, isNull); + }); + + test('junk serials still sanitize to null on load', () async { + SharedPreferences.setMockInitialValues({ + 'paired_remote_id': 'AA:BB:CC:DD:EE:FF', + 'paired_serial': '?*', + }); + expect((await PairedDevice.load())!.serial, isNull); + }); +} From e80554008dcde62848a92ab233c430d3113ee47f Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Tue, 25 Aug 2026 19:10:50 +0200 Subject: [PATCH 06/16] fix(ble): memfault counts as liveness; pin the FAILURE-ack clock semantics; flag the pending #35 repin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-ups on the bootstrap review: A Memfault chunk is real inbound traffic on the link, so it now advances _lastRx like every other notification — a strap volunteering crash data must not look silent to the staleness fuse and get its link bounced. New regression test for the SET_CLOCK result split the contract implies: a FAILURE result inside a non-null response object still satisfies readiness (only a null result fails, one write, no resend) — but the strap did NOT take the write, so the phone-suspect history deferral computed off the pre-correction reading stays live; clearing it is reserved for an accepted correction. The protocol pin stays at 4ce8f02 (#33's tree, whose parser still nulls any non-1 hello revision — under the mandatory hello that means a future revision bump cannot connect). OpenStrap/protocol#35 lifts that gate; the pending repin is now documented at BOTH pin locations — pubspec.yaml's ref and kProtocolPin in derivation_engine.dart — with the rule that every pin location moves together to the main merge commit (the pin-equality test fails a partial repin) and the kAlgoVersion no-bump reasoning to re-verify against the actual merge diff. --- lib/ble/ble_engine.dart | 3 ++ test/gen5_bootstrap_official_test.dart | 47 ++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index 0e97eea6..c6db1522 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -3810,6 +3810,9 @@ class BleEngine { session.subs.add( c.onValueReceived.listen((chunk) { if (_session != session || !session.connected) return; + // Real inbound traffic on this link — it proves liveness the same as + // any other notification, so the staleness/watchdog clock advances. + _lastRx = DateTime.now(); _memfaultChunks++; _memfaultBytesTotal += chunk.length; if (_memfaultChunks == 1) { diff --git a/test/gen5_bootstrap_official_test.dart b/test/gen5_bootstrap_official_test.dart index 58f2e2be..c7b756b5 100644 --- a/test/gen5_bootstrap_official_test.dart +++ b/test/gen5_bootstrap_official_test.dart @@ -406,6 +406,53 @@ void main() { }); }); + test('an unsuccessful but NON-NULL SET_CLOCK response still satisfies ' + 'readiness — without clearing the unconfirmed suspect verdict', () { + fakeAsync((async) { + // The contract judges the clock step on a non-null + // RESPONSE OBJECT, not on its result byte: only a null result fails + // readiness. But a FAILURE result means the strap did NOT take the + // write, so the phone-suspect history deferral computed off the + // pre-correction reading is still live evidence and must survive — + // clearing it is reserved for an ACCEPTED correction. + final rig = _Rig(); + rig.replyTo = (seq, op) => switch (op) { + Cmd.getHello => _helloReply(seq, tsSeconds: _wallNow() + 2 * 86400), + Cmd.setClock => Decoded('cmd_response', { + 'opcode': Cmd.setClock, + 'req_seq': seq, + 'cmd_status': CommandAwaiter.statusFailure, + }), + Cmd.getCustomAdvertisingName => _nameReply(seq), + _ => null, + }; + expect( + _run(rig, async), + isTrue, + reason: + 'a non-null set-clock response object is treated as ' + 'success — only a null result fails readiness', + ); + expect(rig.ready, isTrue); + expect( + rig.count(Cmd.setClock), + 1, + reason: 'one write, no resend on a FAILURE result', + ); + expect( + rig.engine.historyPausedForClock, + isTrue, + reason: + 'the correction went UNCONFIRMED — the history-safety ' + 'deferral keeps the drain parked until a reading agrees', + ); + expect( + rig.logs.any((l) => l.contains('clearing the phone-clock')), + isFalse, + ); + }); + }); + test('HELLO PENDING → SUCCESS completes the bootstrap', () { fakeAsync((async) { final rig = _Rig(); From ea648fb3e29ac02dcc7036e4578c6a9dd0651905 Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Tue, 25 Aug 2026 19:35:40 +0200 Subject: [PATCH 07/16] test(ble): memfault liveness through the one accounting path; pin the FAILURE-ack ordering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Memfault accounting moves into a single _onMemfaultChunk — the real notification listener and the test seam both land there, so the liveness stamp and the byte/chunk counters cannot drift apart. The new regression feeds chunks through it and pins that they advance sinceLastRx (a strap volunteering crash data must not look silent to the staleness fuse) and land in the snapshot counters. The unsuccessful-but-non-null SET_CLOCK regression now also pins the surrounding sequence: zero GET_CLOCK (the FAILURE result changes nothing about the no-read-back rule), SET_CLOCK before the advertising-name read, and the advertising-name read before READY. --- lib/ble/ble_engine.dart | 30 ++++++++++++------ test/gen5_bootstrap_official_test.dart | 43 ++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 9 deletions(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index c6db1522..6b3dc38d 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -3810,19 +3810,31 @@ class BleEngine { session.subs.add( c.onValueReceived.listen((chunk) { if (_session != session || !session.connected) return; - // Real inbound traffic on this link — it proves liveness the same as - // any other notification, so the staleness/watchdog clock advances. - _lastRx = DateTime.now(); - _memfaultChunks++; - _memfaultBytesTotal += chunk.length; - if (_memfaultChunks == 1) { - _log('[MEMFAULT] strap volunteered its first crash/diagnostic ' - 'chunk (${chunk.length} B) — collected only.'); - } + _onMemfaultChunk(chunk); }), ); } + /// The ONE Memfault accounting path — the notification listener above and + /// the test seam both land here, so the liveness stamp and the counters + /// cannot drift apart. A chunk is real inbound traffic on this link: it + /// proves liveness the same as any other notification, so the + /// staleness/watchdog clock advances. + void _onMemfaultChunk(List chunk) { + _lastRx = DateTime.now(); + _memfaultChunks++; + _memfaultBytesTotal += chunk.length; + if (_memfaultChunks == 1) { + _log('[MEMFAULT] strap volunteered its first crash/diagnostic ' + 'chunk (${chunk.length} B) — collected only.'); + } + } + + /// Feed one Memfault chunk through the real accounting path — the liveness + /// stamp and counters live behind a radio otherwise. + @visibleForTesting + void debugIngestMemfaultChunk(List chunk) => _onMemfaultChunk(chunk); + /// Memfault (0007) traffic counters — diagnostics only. int _memfaultChunks = 0; int _memfaultBytesTotal = 0; diff --git a/test/gen5_bootstrap_official_test.dart b/test/gen5_bootstrap_official_test.dart index c7b756b5..4cece57f 100644 --- a/test/gen5_bootstrap_official_test.dart +++ b/test/gen5_bootstrap_official_test.dart @@ -439,6 +439,23 @@ void main() { 1, reason: 'one write, no resend on a FAILURE result', ); + expect( + rig.count(Cmd.getClock), + 0, + reason: + 'no read-back and no fallback — the FAILURE result changes ' + 'nothing about the no-GET_CLOCK rule', + ); + // The sequence holds around the failed-but-answered write: + // SET_CLOCK → advertising name → READY. + expect( + rig.trace.indexOf('cmd:${Cmd.setClock}'), + lessThan(rig.trace.indexOf('cmd:${Cmd.getCustomAdvertisingName}')), + ); + expect( + rig.trace.indexOf('cmd:${Cmd.getCustomAdvertisingName}'), + lessThan(rig.trace.indexOf('ready')), + ); expect( rig.engine.historyPausedForClock, isTrue, @@ -907,6 +924,32 @@ void main() { expect(rig.logged('not required; setup continues'), isTrue); }); }); + + test('a Memfault chunk counts as LIVENESS and lands in the counters', () { + // Through the same _onMemfaultChunk the real notification listener + // uses — a strap volunteering crash data must not look silent to the + // staleness fuse, or a quiet-but-alive link gets bounced. + final rig = _Rig(); + expect( + rig.engine.sinceLastRx.inDays, + greaterThan(365), + reason: 'a fresh engine has never received anything', + ); + + rig.engine.debugIngestMemfaultChunk(const [1, 2, 3]); + expect( + rig.engine.sinceLastRx.inSeconds, + lessThan(5), + reason: 'the chunk advanced the liveness clock', + ); + expect(rig.engine.offloadSnapshot['memfault_chunks'], 1); + expect(rig.engine.offloadSnapshot['memfault_bytes'], 3); + expect(rig.logged('collected only'), isTrue); + + rig.engine.debugIngestMemfaultChunk(const [4, 5]); + expect(rig.engine.offloadSnapshot['memfault_chunks'], 2); + expect(rig.engine.offloadSnapshot['memfault_bytes'], 5); + }); }); group('the connect route is chosen before discovery', () { From 6aaea73d62eee63b9a069d40471792471e98beb2 Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Wed, 26 Aug 2026 15:29:12 +0200 Subject: [PATCH 08/16] =?UTF-8?q?chore(deps):=20repin=20protocol=20to=20th?= =?UTF-8?q?e=20#35=20merge=20=E2=80=94=20the=20hello-revision=20gate=20is?= =?UTF-8?q?=20gone?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three pin locations move together to protocol main @ 6664854, the OpenStrap/protocol#35 merge commit. The old pin's parser returned null for any hello body whose revision byte was not 1; under this branch's mandatory-hello bootstrap that made a future firmware revision bump unable to connect. #35 records the byte instead of gating on it. NO kAlgoVersion bump, verified against the full 4ce8f02..6664854 diff: connection identity/state, not the derivation pipeline); #34, also in the hop, only ADDS files (oura + generic-HRS wire formats nothing here imports); the rest is comment rewording. No decoder for a persisted record moves, so no stored number can. --- lib/compute/derivation_engine.dart | 20 +++++++++++++++++++- pubspec.lock | 4 ++-- pubspec.yaml | 25 ++++++++++++++++++++++--- 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/lib/compute/derivation_engine.dart b/lib/compute/derivation_engine.dart index 05a3fe19..f9cf5c1b 100644 --- a/lib/compute/derivation_engine.dart +++ b/lib/compute/derivation_engine.dart @@ -1520,8 +1520,26 @@ const int kAlgoVersion = 79; // no caller passing it `active`/`basal`/`total` are byte-identical to before // — the walking-cadence NUMBER change already happened at v77 above, via // #283's own a077a4a pin, before this branch's pin moved past it. +// +// REPIN (this branch) @ 6664854 — protocol main at the OpenStrap/protocol#35 +// merge commit ("record HELLO revision instead of gating parsing"). The old +// pin's parser returned null for any hello body whose revision byte was not +// 1; this branch makes HELLO MANDATORY, so under that gate a firmware that +// bumped the revision could not connect at all. #35 records the byte in +// `helloRevision` and reads the fixed revision-1 offsets regardless. +// +// The hop from 19d7291 is ahead 3 / behind 0, and its ONLY lib/ diff is +// #35's `lib/src/control.dart` (+4/-5) — the dropped `if (body[0] != 1)`. +// The other two commits are the #34 merge (2c8448b), whose head 19d7291 this +// pin already WAS, so the oura/generic-HRS wire formats edge#280 imports come +// across unchanged. NO kAlgoVersion bump: hello feeds connection identity and +// state, not the derivation pipeline — no decoder for a persisted record +// moves, so no stored number can. This constant moves together with +// pubspec.yaml's `ref:` and pubspec.lock +// (test/db_serve_version_and_reads_test.dart pins them equal, so a partial +// repin fails the suite). const String kAnalyticsPin = '7105256b37ad61b49453a6b96543a2b80ea74487'; -const String kProtocolPin = '19d72919ecc0cbca518e0fdbbe2f6f9dc7ffe265'; +const String kProtocolPin = '6664854062d6e0e6099eac39b3ee73d96703a49e'; // Fold idempotency, the minimum-nights warm-up, and legacy-payload handling // all live in SleepProfilePolicy (pure, unit-tested) — see diff --git a/pubspec.lock b/pubspec.lock index ec93de94..86530546 100644 --- a/pubspec.lock +++ b/pubspec.lock @@ -933,8 +933,8 @@ packages: dependency: "direct main" description: path: "." - ref: "19d72919ecc0cbca518e0fdbbe2f6f9dc7ffe265" - resolved-ref: "19d72919ecc0cbca518e0fdbbe2f6f9dc7ffe265" + ref: "6664854062d6e0e6099eac39b3ee73d96703a49e" + resolved-ref: "6664854062d6e0e6099eac39b3ee73d96703a49e" url: "https://github.com/OpenStrap/protocol.git" source: git version: "1.0.0" diff --git a/pubspec.yaml b/pubspec.yaml index 265b0339..54e7b15c 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -95,9 +95,28 @@ dependencies: # fit; `ouraCmdAuthenticate` rejects a wrong-size cipher instead of # emitting a malformed frame; `ouraCmdSyncTime` no longer calls # `ByteData.setUint64`, which throws on dart2js. None of the three touch - # a WHOOP code path. #34 is unmerged, so this is a PR-branch-head pin; - # repin to the main merge commit when it lands. - ref: 19d72919ecc0cbca518e0fdbbe2f6f9dc7ffe265 + # a WHOOP code path. + # + # REPIN (this branch): protocol main @ 6664854, the #35 merge commit — + # a MAIN merge commit, reachable regardless of branch deletion, and the + # first pin here that is not a PR-branch head (#34 has since merged at + # 2c8448b, which 6664854 descends from, so the oura/HRS wire formats + # above are carried unchanged). + # + # The hop from 19d7291 is ahead 3 / behind 0 and its ONLY lib/ diff is + # #35's `lib/src/control.dart` (+4/-5): `Gen5HelloInfo.parse` no longer + # returns null for a hello body whose revision byte is not 1 — the byte + # is recorded in `helloRevision` and the fixed revision-1 offsets are + # read regardless. That matters here because this branch makes HELLO + # MANDATORY: under the old gate a firmware that bumped the revision + # could not connect at all. + # + # NO kAlgoVersion bump: hello feeds connection identity and state, not + # the derivation pipeline — no decoder for a persisted record changes, + # so no stored number can move. Reasoning also sits beside kProtocolPin + # in lib/compute/derivation_engine.dart, which moves together with this + # ref and pubspec.lock (the pin-equality test fails a partial repin). + ref: 6664854062d6e0e6099eac39b3ee73d96703a49e openstrap_analytics: git: url: https://github.com/OpenStrap/analytics.git From 26728903fdc43533f7de6f1d9162720ba20b5195 Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Wed, 26 Aug 2026 17:21:51 +0200 Subject: [PATCH 09/16] fix(ble): one service-discovery and characteristic-validation path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (coderabbit + abdulsaheel, #285): `_FbpGattOps.discoverAndValidate` and the legacy block in `_doConnect` were two transcriptions of one decision — find the band's service by prefix, resolve cmd_to/cmd_from/ events/data — and they had already drifted apart in both directions: * only the gen5 copy resolved the optional Memfault characteristic; * only the legacy copy matched on `str128` (`str` returns the SHORTEST form, so a SIG-assigned service reads `180d` and never matches a `0000180d` prefix) and validated against the registry's `BandEntry.requiredCharacteristics` rather than a hardcoded four. Both copies also ran on the same connect: a discovered gen4 falls back to the legacy order, which repeated the whole discovery it had just done. Extract `BleEngine._discoverBand`, returning `_DiscoveredBand` (the registry entry plus the resolved characteristics). Both routes call it; the drifted gen5 copy is gone and the surviving path is the registry- and `str128`-based one. It does not touch the session — pinning the band stays at the caller, because the gen5 route has to decide `notGen5` first. The `GattBootstrapOps` seam now returns `BandEntry?` rather than `BandProfile?`, so the route decision reads the registry id instead of a wire profile, and the gen4 fallback keeps its own discovery unchanged. The characteristics stay NULLABLE in `_DiscoveredBand`: which ones a link must expose is registry data, `_discoverBand` has already refused an entry missing one it declares required, and the legacy route's skip-if-absent subscription for a band that does not require one is preserved rather than turned into a `!` that would crash. --- lib/ble/ble_engine.dart | 256 ++++++++++++++----------- test/gen5_bootstrap_official_test.dart | 7 +- test/gen5_wiring_test.dart | 4 +- 3 files changed, 154 insertions(+), 113 deletions(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index 6b3dc38d..b7300d19 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -660,6 +660,44 @@ enum _Gen5ConnectOutcome { notGen5, } +/// One discovered band link: the registry entry the peripheral turned out to +/// be, plus the characteristics resolved off its service. +/// +/// [BleEngine._discoverBand] returns null rather than a record whose entry is +/// missing a characteristic it declares required, so no caller re-checks that. +class _DiscoveredBand { + final BandEntry entry; + + /// The four command/notify characteristics, resolved by 32-bit prefix. + /// + /// NULLABLE, and deliberately: WHICH of them a link must expose is registry + /// data ([BandEntry.requiredCharacteristics]), and + /// [BleEngine._discoverBand] has already refused any entry missing one it + /// declares. A null here therefore means "this entry does not require it", + /// which is the case the legacy route skips a subscription for — not an + /// unchecked absence. + final BluetoothCharacteristic? cmdTo; + final BluetoothCharacteristic? cmdFrom; + final BluetoothCharacteristic? events; + final BluetoothCharacteristic? data; + + /// Memfault (0007). OPTIONAL in both directions: absent is normal, and it is + /// a diagnostic/liveness input, never a readiness one. It is resolved HERE + /// rather than only on the gen5 route because this is the one validation + /// path — the two copies this replaced had already drifted over exactly + /// this field. + final BluetoothCharacteristic? memfault; + + const _DiscoveredBand({ + required this.entry, + required this.cmdTo, + required this.cmdFrom, + required this.events, + required this.data, + required this.memfault, + }); +} + /// The platform seam for the official gen5 connect order: PHY preference, /// discovery/validation, MTU intent, bond, notification registration. /// @@ -674,10 +712,10 @@ abstract class GattBootstrapOps { /// Ask for LE 2M PHY. Throws when the request fails — logged, non-fatal. Future preferLe2mPhy(); - /// Discover services, pin the session's band and stash the WHOOP - /// characteristics. Returns the discovered band, or null when no WHOOP - /// service — or a required characteristic — is present. - Future discoverAndValidate(); + /// Discover services, pin the session's band and stash the band's + /// characteristics. Returns the discovered registry entry, or null when no + /// known framed service — or a required characteristic — is present. + Future discoverAndValidate(); /// Request the ATT MTU; returns the negotiated value, throws on failure. Future requestMtu(int mtu); @@ -724,47 +762,16 @@ class _FbpGattOps implements GattBootstrapOps { } @override - Future discoverAndValidate() async { - final services = - await _device.discoverServices().timeout(BleEngine._serviceDiscoveryTimeout); - BluetoothService? svc; - BandProfile band = BandProfile.gen4; - for (final s in services) { - final u = s.uuid.str.toLowerCase(); - if (u.startsWith(GattProfile.gen4.servicePrefix)) { - svc = s; - band = BandProfile.gen4; - break; - } - if (u.startsWith(GattProfile.gen5.servicePrefix)) { - svc = s; - band = BandProfile.gen5; - break; - } - } - if (svc == null) return null; - _session.applyBand(band); - final gatt = band.gatt; - BluetoothCharacteristic? find(String prefix) { - for (final c in svc!.characteristics) { - if (c.uuid.str.toLowerCase().startsWith(prefix)) return c; - } - return null; - } - - _session.cmdTo = find(gatt.cmdTo.substring(0, 8)); - _cmdFrom = find(gatt.cmdFrom.substring(0, 8)); - _events = find(gatt.events.substring(0, 8)); - _data = find(gatt.data.substring(0, 8)); - // Optional — its absence must not fail validation. - _memfault = find(gatt.memfault.substring(0, 8)); - if (_session.cmdTo == null || - _cmdFrom == null || - _events == null || - _data == null) { - return null; - } - return band; + Future discoverAndValidate() async { + final found = await _engine._discoverBand(_device); + if (found == null) return null; + _session.applyBand(found.entry); + _session.cmdTo = found.cmdTo; + _cmdFrom = found.cmdFrom; + _events = found.events; + _data = found.data; + _memfault = found.memfault; + return found.entry; } @override @@ -2454,69 +2461,21 @@ class BleEngine { } _setPhase(BleConnState.discovering); - final services = await device - .discoverServices() - .timeout(_serviceDiscoveryTimeout); - // Pin the band from whichever registered service the peripheral exposes. - // This drives the frame header/CRC, command envelope, ACK, and record - // decode for the session. - BluetoothService? svc; - BandEntry? entry; - for (final s in services) { - // `str128`, not `str`: `str` is the SHORTEST form, so a SIG-assigned - // service comes back as `180d` and never starts with a `0000180d` - // prefix. WHOOP's uuids are 128-bit either way — see - // `GattBandLink._find`, which had the live version of this bug. - final u = s.uuid.str128; - // [kFramedBands], for the same reason the scan filters on it: this - // engine speaks a framed envelope and nothing else. - for (final e in kFramedBands) { - if (u.startsWith(e.servicePrefix)) { - svc = s; - entry = e; - break; - } - } - if (svc != null) break; - } - if (svc == null || entry == null) { - _log('No known band service found on device (looked for: ' - '${kFramedBands.map((e) => "${e.servicePrefix}xxxx").join(", ")}).'); + // The SAME discovery+validation the official gen5 route runs — see + // [_discoverBand]; it logs which half failed. + final found = await _discoverBand(device); + if (found == null) { await _failConnect(); return false; } + final entry = found.entry; session.applyBand(entry); state.generation = entry.id; _log('Detected ${entry.label} (${entry.id}) link.'); - // Non-null: `entry` came out of [kFramedBands]. - final gatt = entry.gatt!; - BluetoothCharacteristic? find(String uuid) { - final prefix = uuid.substring(0, 8); - for (final c in svc!.characteristics) { - // `str128` — see the service match above. - if (c.uuid.str128.startsWith(prefix)) return c; - } - return null; - } - - // WHICH characteristics a link must expose is registry data. Demanding - // all four unconditionally is why `hr_sensor.dart` exists as a second - // parallel BLE stack — a generic HRS device has ONE notify - // characteristic and would abort here. - final missing = [ - for (final u in entry.requiredCharacteristics) - if (find(u) == null) u.substring(0, 8), - ]; - if (missing.isNotEmpty) { - _log('${entry.label}: missing required characteristic(s) ' - '${missing.join(", ")}.'); - await _failConnect(); - return false; - } - session.cmdTo = find(gatt.cmdTo); - final cmdFrom = find(gatt.cmdFrom); - final events = find(gatt.events); - final data = find(gatt.data); + session.cmdTo = found.cmdTo; + final cmdFrom = found.cmdFrom; + final events = found.events; + final data = found.data; // The bond is complete by here, so this is the pause that precedes // notification registration — [BandEntry.preRegistrationDelay], zero on @@ -2814,16 +2773,18 @@ class BleEngine { return _Gen5ConnectOutcome.failed; } _setPhase(BleConnState.discovering); - final band = await gatt.discoverAndValidate(); - if (band == null) { - _log('[BOOT gen5] required WHOOP service or characteristic missing — ' + final entry = await gatt.discoverAndValidate(); + if (entry == null) { + _log('[BOOT gen5] required band service or characteristic missing — ' 'connection failed.'); await _failConnect(); return _Gen5ConnectOutcome.failed; } - if (!band.isGen5) return _Gen5ConnectOutcome.notGen5; - state.generation = 'gen5'; - _log('Detected WHOOP 5 (gen5) link.'); + // Discovery is the truth; the generation hint that routed us here was + // only a hint. Anything else goes back to the legacy order unchanged. + if (entry.id != kWhoopGen5.id) return _Gen5ConnectOutcome.notGen5; + state.generation = entry.id; + _log('Detected ${entry.label} (${entry.id}) link.'); try { final negotiated = await gatt.requestMtu(247); _log('MTU negotiated: $negotiated (requested 247).'); @@ -3232,6 +3193,85 @@ class BleEngine { return resp != null; } + /// The ONE service-discovery and characteristic-validation path. + /// + /// BOTH connect routes come here — `_FbpGattOps.discoverAndValidate` on the + /// official gen5 order, and `_doConnect`'s legacy order, which a discovered + /// gen4 falls back to. They used to be two transcriptions of this, and they + /// had already drifted: only the gen5 copy resolved Memfault, and only the + /// legacy copy matched on `str128` and on [BandEntry.requiredCharacteristics]. + /// Which service pins the band and which characteristics are required is one + /// decision, so it is one function. + /// + /// Returns null — having logged which half failed — when the peripheral + /// exposes no known framed service, or is missing a required characteristic. + /// Both callers treat that as a failed connect. It does NOT touch the + /// session: pinning the band stays at the caller, because the gen5 route has + /// to decide `notGen5` before anything is pinned. + Future<_DiscoveredBand?> _discoverBand(BluetoothDevice device) async { + final services = + await device.discoverServices().timeout(_serviceDiscoveryTimeout); + // Pin the band from whichever registered service the peripheral exposes. + // This drives the frame header/CRC, command envelope, ACK, and record + // decode for the session. + BluetoothService? svc; + BandEntry? entry; + for (final s in services) { + // `str128`, not `str`: `str` is the SHORTEST form, so a SIG-assigned + // service comes back as `180d` and never starts with a `0000180d` + // prefix. WHOOP's uuids are 128-bit either way — see + // `GattBandLink._find`, which had the live version of this bug. + final u = s.uuid.str128; + // [kFramedBands], for the same reason the scan filters on it: this + // engine speaks a framed envelope and nothing else. + for (final e in kFramedBands) { + if (u.startsWith(e.servicePrefix)) { + svc = s; + entry = e; + break; + } + } + if (svc != null) break; + } + if (svc == null || entry == null) { + _log('No known band service found on device (looked for: ' + '${kFramedBands.map((e) => "${e.servicePrefix}xxxx").join(", ")}).'); + return null; + } + BluetoothCharacteristic? find(String uuid) { + final prefix = uuid.substring(0, 8); + for (final c in svc!.characteristics) { + // `str128` — see the service match above. + if (c.uuid.str128.startsWith(prefix)) return c; + } + return null; + } + + // WHICH characteristics a link must expose is registry data. Demanding + // all four unconditionally is why `hr_sensor.dart` exists as a second + // parallel BLE stack — a generic HRS device has ONE notify + // characteristic and would abort here. + final missing = [ + for (final u in entry.requiredCharacteristics) + if (find(u) == null) u.substring(0, 8), + ]; + if (missing.isNotEmpty) { + _log('${entry.label}: missing required characteristic(s) ' + '${missing.join(", ")}.'); + return null; + } + // Non-null: `entry` came out of [kFramedBands]. + final gatt = entry.gatt!; + return _DiscoveredBand( + entry: entry, + cmdTo: find(gatt.cmdTo), + cmdFrom: find(gatt.cmdFrom), + events: find(gatt.events), + data: find(gatt.data), + memfault: find(gatt.memfault), + ); + } + /// The LEGACY bootstrap SET_CLOCK decision. The official gen5 path has its /// own contract ([_gen5ClockContract]) and never comes here; this runs for /// gen4, and for a gen5 band that reached `_doConnect` because a stored diff --git a/test/gen5_bootstrap_official_test.dart b/test/gen5_bootstrap_official_test.dart index 4cece57f..38eb7489 100644 --- a/test/gen5_bootstrap_official_test.dart +++ b/test/gen5_bootstrap_official_test.dart @@ -27,6 +27,7 @@ import 'dart:typed_data'; import 'package:fake_async/fake_async.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:openstrap_edge/ble/adapters/_registry.dart'; import 'package:openstrap_edge/ble/ble_engine.dart'; import 'package:openstrap_edge/ble/ble_state.dart'; import 'package:openstrap_protocol/openstrap_protocol.dart'; @@ -216,9 +217,9 @@ class _Ops implements GattBootstrapOps { } @override - Future discoverAndValidate() async { + Future discoverAndValidate() async { rig.trace.add('discover'); - return discoveryFails ? null : BandProfile.gen5; + return discoveryFails ? null : kWhoopGen5; } @override @@ -891,7 +892,7 @@ void main() { expect(rig.ready, isFalse); expect(rig.commands, isEmpty); expect( - rig.logged('required WHOOP service or characteristic missing'), + rig.logged('required band service or characteristic missing'), isTrue, ); }); diff --git a/test/gen5_wiring_test.dart b/test/gen5_wiring_test.dart index 16cd3cad..2819e094 100644 --- a/test/gen5_wiring_test.dart +++ b/test/gen5_wiring_test.dart @@ -707,9 +707,9 @@ class _FakeOps implements GattBootstrapOps { Future preferLe2mPhy() async => link.trace.add('phy'); @override - Future discoverAndValidate() async { + Future discoverAndValidate() async { link.trace.add('discover'); - return link.band; + return bandEntryFor(link.band); } @override From 38b7f92792c1e72d82b08ebe1a655018a2002ece Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Wed, 26 Aug 2026 17:21:58 +0200 Subject: [PATCH 10/16] fix(ble): bound the initial bond-state read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review (coderabbit + abdulsaheel, #285): `_FbpGattOps.isBonded()` awaited `_device.bondState.first` with no timeout, and it was the one platform stream await in this file that had none. `bondState` does emit an initial value, but that first emission awaits the platform's `getBondState` when nothing is cached. If that request never answers, `_connectGen5Official` parks inside bond setup with `_session` non-null and the phase still `discovering` — so `holdsBandLink` keeps the claim live and every later headless drain yields to a connect that will never finish, with no recovery short of a process restart. Five seconds, matching `_serviceDiscoveryTimeout`/`_notifySetupTimeout` in intent and short because this is a cached OS lookup, not a radio round trip. The throw lands in the bootstrap's existing catch, which calls `_failConnect()` and tears the session down. `createBond()` is left alone: the plugin already gives it a 90-second response timeout. --- lib/ble/ble_engine.dart | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index b7300d19..d47366b4 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -779,7 +779,17 @@ class _FbpGattOps implements GattBootstrapOps { @override Future isBonded() async => - await _device.bondState.first == BluetoothBondState.bonded; + // BOUNDED. `bondState` emits an initial value, but that first emission + // awaits the platform's `getBondState` when nothing is cached — and an + // unbounded await here parks `_connectGen5Official` inside bond setup + // with `_session` non-null and the phase still `discovering`, so + // `holdsBandLink` keeps the claim live and every later headless drain + // yields to a connect that will never finish. Throwing instead lands in + // the caller's catch, which fails the connect and tears the session + // down. `createBond()` needs no such bound: the plugin gives it a + // 90-second response timeout of its own. + await _device.bondState.first.timeout(BleEngine._bondStateTimeout) == + BluetoothBondState.bonded; @override Future createBond() => _device.createBond(); @@ -4024,6 +4034,12 @@ class BleEngine { static const Duration _serviceDiscoveryTimeout = Duration(seconds: 15); static const Duration _notifySetupTimeout = Duration(seconds: 15); + /// The initial `bondState` read ([_FbpGattOps.isBonded]) — the same reason + /// as the two above, at the one platform stream await the gen5 bootstrap + /// adds. Short because it is a CACHED OS lookup, not a radio round trip: the + /// bond itself is `createBond()`, which the plugin bounds at 90 s. + static const Duration _bondStateTimeout = Duration(seconds: 5); + /// [owner] pins the write to ONE session. Without it a write queued by a /// long-parked drain (a big commit, then up to ~25 s of ACK retries) lands on /// whatever session happens to be current when the write chain reaches it — From 563c818f6b5865c290c01fcb8315db91f63f4500 Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Wed, 26 Aug 2026 17:22:09 +0200 Subject: [PATCH 11/16] fix(sync): the pairing generation lives in device.adapter_id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The branch was drafted before the `device` table (schema 49) landed, so it persisted the connect route's generation hint into a third SharedPreferences key beside a column that already means exactly that — `adapter_id`, the registry's `BandEntry.id`, which main already writes from `DeviceState.generation`. Two homes for one fact, and `load()` answers from the table, so the prefs copy was the one nobody read. `PairedDevice` now carries `generation` off `adapter_id`, keeps the prefs key only as the mirror that heals a rebuilt database, and sanitizes to gen4/gen5/null on both reads: `adapter_id` is the whole registry's id space (a notify-only `ble_hrs`/`oura` row names no framed generation) and this value routes the connect order. That surfaced the table half of the device-scoping rule this branch already enforced on prefs. Every `upsertDevice` column COALESCEs, and the primary row is reused for whatever band is primary — so pairing a DIFFERENT band with a caller that does not know its family left the FORGOTTEN band's generation on it, and the authoritative read then routed the new band by the wrong device's identity. `upsertDevice` gains `clearAdapterId` for that one case (ignored when `adapterId` is non-null: a caller that knows wins over one that clears), and `save` decides sameness from the table first, falling back to the mirror, so the mirror cannot heal a stale generation back over a corrected one. `paired_device_test` runs against a real database for the same reason — mocking prefs alone would exercise neither the authoritative read nor the COALESCE that preserves a known generation. --- lib/data/db.dart | 21 +++++++++++++++++++-- lib/state/app_state.dart | 2 +- lib/sync/paired_device.dart | 27 +++++++++++++++++++-------- test/paired_device_test.dart | 28 +++++++++++++++++++++++++++- 4 files changed, 66 insertions(+), 12 deletions(-) diff --git a/lib/data/db.dart b/lib/data/db.dart index bd3fa6f0..2a72f6cd 100644 --- a/lib/data/db.dart +++ b/lib/data/db.dart @@ -1636,12 +1636,21 @@ class LocalDb { /// Record that [id] exists and was seen now. Every field except [id] is /// COALESCED, so a caller that only knows the remote id cannot blank out a /// label or an adapter another caller already established. + /// + /// [clearAdapterId] is the one deliberate exception, and it exists because + /// COALESCE is wrong in exactly one case: this row is the PRIMARY band + /// permanently (`id` is `''`), so pairing a DIFFERENT band reuses it, and a + /// null [adapterId] would then leave the FORGOTTEN band's family on the new + /// one. `PairedDevice.save` passes it when the remote id changed and the + /// caller does not know the new band's family. It is ignored when + /// [adapterId] is non-null — a caller that knows wins over one that clears. static Future upsertDevice({ String id = kPrimaryDeviceId, String? adapterId, String? remoteId, String? label, String? tier, + bool clearAdapterId = false, }) async { final db = await instance; final now = DateTime.now().millisecondsSinceEpoch ~/ 1000; @@ -1655,10 +1664,18 @@ class LocalDb { 'last_seen': now, }, conflictAlgorithm: ConflictAlgorithm.ignore); await db.rawUpdate( - 'UPDATE device SET adapter_id = COALESCE(?, adapter_id), ' + 'UPDATE device SET ' + '${adapterId == null && clearAdapterId ? 'adapter_id = NULL, ' : 'adapter_id = COALESCE(?, adapter_id), '}' 'remote_id = COALESCE(?, remote_id), label = COALESCE(?, label), ' 'tier = COALESCE(?, tier), last_seen = ? WHERE id = ?', - [adapterId, remoteId, label, tier, now, id], + [ + if (!(adapterId == null && clearAdapterId)) adapterId, + remoteId, + label, + tier, + now, + id, + ], ); } diff --git a/lib/state/app_state.dart b/lib/state/app_state.dart index 2e1f96a3..26d5cd61 100644 --- a/lib/state/app_state.dart +++ b/lib/state/app_state.dart @@ -3764,7 +3764,7 @@ class AppState extends ChangeNotifier { // `device.adapter_id` blank on every install that pairs once and never // re-pairs. await PairedDevice.save(remoteId, serial ?? device.serial, - adapterId: device.generation); + generation: device.generation); paired = await PairedDevice.load(); // Now that there's a band to alert about, ask for notification permission // (a natural moment; battery/charging alerts depend on it). Best-effort. diff --git a/lib/sync/paired_device.dart b/lib/sync/paired_device.dart index baea44ae..8ef94b84 100644 --- a/lib/sync/paired_device.dart +++ b/lib/sync/paired_device.dart @@ -101,26 +101,37 @@ class PairedDevice { String? generation, }) async { final clean = cleanDeviceLabel(serial); - await LocalDb.upsertDevice( - adapterId: generation, - remoteId: remoteId, - label: clean, - tier: kBandSourceTier, - ); + final gen = _cleanGeneration(generation); final prefs = await SharedPreferences.getInstance(); // A stored generation belongs to a DEVICE. Keep it only when this save is // for the same remoteId and merely doesn't know the generation (most save // sites only carry the serial); pairing a DIFFERENT band must never // inherit the old band's generation — that would route its first connect // by the wrong device's identity. - final sameDevice = prefs.getString(_kRemoteId) == remoteId; + // + // Asked of the TABLE first, because that is the copy `load()` answers + // from. Both copies are then written the same way, so the mirror cannot + // heal a stale generation back over a corrected one. + final row = await LocalDb.deviceRow(); + final knownRemoteId = + (row?['remote_id'] as String?) ?? prefs.getString(_kRemoteId); + final sameDevice = knownRemoteId == remoteId; + await LocalDb.upsertDevice( + adapterId: gen, + remoteId: remoteId, + label: clean, + tier: kBandSourceTier, + // `adapter_id` COALESCEs like every other column, and the primary row is + // reused for whatever band is primary — so a new band needs it said out + // loud that the old family no longer applies. + clearAdapterId: !sameDevice, + ); await prefs.setString(_kRemoteId, remoteId); if (clean != null) { await prefs.setString(_kSerial, clean); } else { await prefs.remove(_kSerial); // never persist junk } - final gen = _cleanGeneration(generation); if (gen != null) { await prefs.setString(_kGeneration, gen); } else if (!sameDevice) { diff --git a/test/paired_device_test.dart b/test/paired_device_test.dart index b70cbf50..3e30d5d5 100644 --- a/test/paired_device_test.dart +++ b/test/paired_device_test.dart @@ -10,12 +10,38 @@ // must sanitize to null rather than steer the route. import 'package:flutter_test/flutter_test.dart'; +import 'package:openstrap_edge/data/db.dart'; import 'package:openstrap_edge/sync/paired_device.dart'; +import 'package:path/path.dart' as p; import 'package:shared_preferences/shared_preferences.dart'; +import 'package:sqflite_common_ffi/sqflite_ffi.dart'; void main() { TestWidgetsFlutterBinding.ensureInitialized(); - setUp(() => SharedPreferences.setMockInitialValues({})); + + // `PairedDevice` is table-first now (the `device` row, schema 49) with the + // prefs pair as the mirror that heals a rebuilt database, so both halves + // have to be real here — mocking prefs alone would exercise neither the + // authoritative read nor the COALESCE that preserves a known generation. + setUpAll(() async { + sqfliteFfiInit(); + databaseFactory = databaseFactoryFfi; + LocalDb.dbName = 'openstrap_paired_device_test.db'; + final dir = await databaseFactory.getDatabasesPath(); + await databaseFactory.deleteDatabase(p.join(dir, LocalDb.dbName)); + }); + + tearDownAll(() async { + await LocalDb.close(); + final dir = await databaseFactory.getDatabasesPath(); + await databaseFactory.deleteDatabase(p.join(dir, LocalDb.dbName)); + }); + + setUp(() async { + SharedPreferences.setMockInitialValues({}); + // Both copies, or one test's pairing steers the next one's. + await LocalDb.deleteDevice(); + }); test('save/load round-trips remoteId, serial and generation', () async { await PairedDevice.save( From 7545f146bd2d60680024bfda0ef67ed4319c84cc Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Wed, 26 Aug 2026 17:50:40 +0200 Subject: [PATCH 12/16] =?UTF-8?q?fix(ble,sync):=20cross-review=20round=20?= =?UTF-8?q?=E2=80=94=20bound=20at=20the=20seam,=20and=20three=20false=20co?= =?UTF-8?q?mments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second-model review pass (Codex, 3 rounds) over the rebase and the two PR-review fixes. Everything it raised was real; nothing was rejected. **The bond-state bound moves to the seam.** It was inside `_FbpGattOps`, where no test can reach it — the fix could have been deleted without turning the suite red. It now wraps `gatt.isBonded()` in `_connectGen5Official`, so it bounds every `GattBootstrapOps` implementation from one place, and `_Ops(bondCheckHangs: true)` — a read that returns a `Completer` nobody completes — pins that the timeout reaches the bond catch and tears the session down. **A forget now beats a save that was already in flight.** The heal sites are `unawaited(PairedDevice.save(...))`, and this branch added a second one, so a save can sit between its awaits while the user's forget lands and then put both copies back — the forgotten band is paired again on the next launch. `clear()` bumps a counter that `save()` samples on entry and re-checks before each copy it writes. A counter rather than a lock: an isolate interleaves only at awaits, so this is decidable, and nothing in the headless isolate unpairs. **Three comments claimed things the code does not do.** Each is the kind that survives to mislead the next reader: - `_bootstrapSetClock` said a gen5 band with a stale `gen4` hint reaches it, which is why the previous commit kept a registry drift gate there. It does not: `_bootstrapAfterRegistration` branches on the DISCOVERED band and the gen5 arm returns after `_gen5ClockContract`, whichever route connected. The gate was unreachable — `setClockDriftGated` is false for every band that gets here — so it goes, and the comment now says where gen5 is really handled and why that gate is better evidence (milliseconds off the hello timestamp, not whole seconds off `_clockRef`). - `ScanAcceptPolicy` still described itself as the single accept decision with the name fallback gone. This branch's rebase deliberately kept main's broader acceptance for the MG scan gap (#255) and demoted this to the generation hint. Documented as a hint, with null meaning "no hint" and never "not a WHOOP", and the test group renamed off the false contract. - `_discoverBand` claimed nothing is pinned before the `notGen5` decision. The fbp seam pins immediately — pre-existing, and harmless because what it pins is what discovery actually found and the legacy fallback re-pins it — but the comment said otherwise. **And two tests that could not fail.** The corrupted-generation test seeded a junk mirror after a save had already created the device row, so `load()` answered from the table and never read it; both read paths are covered separately now, plus a notify-only `adapter_id`. The sibling-pin test read only `pubspec.yaml` while the repin comments promise a partial repin fails the suite — it reads `pubspec.lock`'s `ref` AND `resolved-ref` too, which is the file that decides what a build actually resolves. All three new tests were confirmed to fail with their fix reverted. `upsertDevice`'s clear branch also hoists its condition into one local: the placeholder and its argument were two copies of the same expression, and a `?` count that disagrees with the argument list binds every value one column to the left, which SQLite accepts in silence. --- lib/ble/ble_engine.dart | 93 ++++++++++++----------- lib/ble/ble_state.dart | 25 +++--- lib/data/db.dart | 9 ++- lib/sync/paired_device.dart | 24 ++++++ test/db_serve_version_and_reads_test.dart | 22 ++++++ test/gen5_bootstrap_official_test.dart | 43 ++++++++++- test/paired_device_test.dart | 76 ++++++++++++++++-- 7 files changed, 225 insertions(+), 67 deletions(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index d47366b4..c8e408c7 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -779,17 +779,11 @@ class _FbpGattOps implements GattBootstrapOps { @override Future isBonded() async => - // BOUNDED. `bondState` emits an initial value, but that first emission - // awaits the platform's `getBondState` when nothing is cached — and an - // unbounded await here parks `_connectGen5Official` inside bond setup - // with `_session` non-null and the phase still `discovering`, so - // `holdsBandLink` keeps the claim live and every later headless drain - // yields to a connect that will never finish. Throwing instead lands in - // the caller's catch, which fails the connect and tears the session - // down. `createBond()` needs no such bound: the plugin gives it a - // 90-second response timeout of its own. - await _device.bondState.first.timeout(BleEngine._bondStateTimeout) == - BluetoothBondState.bonded; + // `bondState` emits an initial value, but that first emission awaits the + // platform's `getBondState` when nothing is cached, so this await can + // hang. The BOUND lives at the call site in `_connectGen5Official` — + // the seam, where a test can drive a read that never answers. + await _device.bondState.first == BluetoothBondState.bonded; @override Future createBond() => _device.createBond(); @@ -2812,7 +2806,18 @@ class BleEngine { // run subscriptions and HELLO against writes the band silently drops. if (gatt.bondingApplies) { try { - if (await gatt.isBonded()) { + // BOUNDED. The initial bond-state read is the one platform await in + // this bootstrap that could hang: `bondState`'s first emission falls + // through to the platform's `getBondState` when nothing is cached. + // Unbounded, a request that never answers parks the bootstrap right + // here with `_session` non-null and the phase still `discovering`, + // so `holdsBandLink` keeps the claim live and every later headless + // drain yields to a connect that will never finish — no recovery + // short of a process restart. The TimeoutException lands in the + // catch below, which fails the connect and tears the session down. + // `createBond()` needs no bound of ours: the plugin gives it a + // 90-second response timeout. + if (await gatt.isBonded().timeout(_bondStateTimeout)) { _log('[BOOT gen5] already bonded — not creating another bond.'); } else { await gatt.createBond(); @@ -3215,9 +3220,14 @@ class BleEngine { /// /// Returns null — having logged which half failed — when the peripheral /// exposes no known framed service, or is missing a required characteristic. - /// Both callers treat that as a failed connect. It does NOT touch the - /// session: pinning the band stays at the caller, because the gen5 route has - /// to decide `notGen5` before anything is pinned. + /// Both callers treat that as a failed connect. + /// + /// It does NOT touch the session itself — pinning stays at the caller. Note + /// that `_FbpGattOps.discoverAndValidate` pins IMMEDIATELY, before + /// `_connectGen5Official` reads the outcome, so a `notGen5` device is briefly + /// pinned to what discovery actually found. That is harmless and deliberate: + /// what it pins is the TRUE band, and the legacy route it falls back to + /// re-discovers and re-pins the same entry before using it. Future<_DiscoveredBand?> _discoverBand(BluetoothDevice device) async { final services = await device.discoverServices().timeout(_serviceDiscoveryTimeout); @@ -3282,35 +3292,29 @@ class BleEngine { ); } - /// The LEGACY bootstrap SET_CLOCK decision. The official gen5 path has its - /// own contract ([_gen5ClockContract]) and never comes here; this runs for - /// gen4, and for a gen5 band that reached `_doConnect` because a stored - /// `gen4` hint was wrong — hence the drift gate stays registry-driven - /// ([BandEntry.setClockDriftGated]) rather than being hardcoded to gen4. + /// The gen4 bootstrap SET_CLOCK decision. + /// + /// NO GEN5 BAND REACHES THIS, by either connect route — and that is a + /// property of `_bootstrapAfterRegistration`, not of how the connect was + /// routed: it branches on `session.band.isGen5` and the gen5 arm returns + /// after [_gen5ClockContract]. So a gen5 band that fell into `_doConnect` + /// on a stale `gen4` hint still gets the gen5 contract, because discovery + /// has pinned the true band before the branch is read. /// - /// Rules, in order: - /// 1. the phone-clock deferral wins — while THIS phone is the suspect - /// party, writing its wall clock onto a possibly-correct strap RTC - /// corrupts the RTC and destroys the evidence; - /// 2. on a drift-gated band, below [BootstrapClockGate.toleranceSeconds] - /// of absolute drift there is NO BLE write at all; - /// 3. everything else writes once — including a band with no usable clock - /// correlation (unset/implausible RTC), where the drift is null and - /// leaving the RTC uncorrected is the one genuinely bad outcome. + /// That is also where [BandEntry.setClockDriftGated] is honoured now: + /// [_gen5ClockContract] gates on the hello timestamp at MILLISECOND + /// resolution against a freshly sampled phone time, which is strictly + /// better evidence than the whole-second `_clockRef` drift a flag test here + /// could read. Re-testing the flag on this path would be dead code — it is + /// false for every band that gets here. /// - /// gen4 keeps the unconditional write it has today: its flow is proven, and - /// the WHOOP 5 bootstrap is where the evidence lives. + /// gen4 keeps the unconditional write it has always had: its flow is proven, + /// and the WHOOP 5 bootstrap is where the evidence for gating lives. The + /// phone-clock deferral still wins — while THIS phone is the suspect party, + /// writing its wall clock onto a possibly-correct strap RTC corrupts the RTC + /// and destroys the evidence. Future _bootstrapSetClock(_Session session) async { if (_deferForClock) return; - if (session.entry.setClockDriftGated) { - final drift = _clockRef?.driftSec; - if (!BootstrapClockGate.needsCorrection(drift)) { - _log('[CLOCK] in sync (drift ${drift}s, tolerance ' - '${BootstrapClockGate.toleranceSeconds}s) — no correction ' - 'needed; no SET_CLOCK written.'); - return; - } - } await setClock(); } @@ -4034,10 +4038,11 @@ class BleEngine { static const Duration _serviceDiscoveryTimeout = Duration(seconds: 15); static const Duration _notifySetupTimeout = Duration(seconds: 15); - /// The initial `bondState` read ([_FbpGattOps.isBonded]) — the same reason - /// as the two above, at the one platform stream await the gen5 bootstrap - /// adds. Short because it is a CACHED OS lookup, not a radio round trip: the - /// bond itself is `createBond()`, which the plugin bounds at 90 s. + /// The initial bond-state read, applied at the [GattBootstrapOps] seam in + /// `_connectGen5Official` — the same reason as the two above, at the one + /// platform stream await the gen5 bootstrap adds. Short because it is a + /// CACHED OS lookup, not a radio round trip: the bond itself is + /// `createBond()`, which the plugin bounds at 90 s. static const Duration _bondStateTimeout = Duration(seconds: 5); /// [owner] pins the write to ONE session. Without it a write queued by a diff --git a/lib/ble/ble_state.dart b/lib/ble/ble_state.dart index 09322b31..28ea0889 100644 --- a/lib/ble/ble_state.dart +++ b/lib/ble/ble_state.dart @@ -1689,15 +1689,19 @@ class BootstrapClockGate { absDeltaMs == null || absDeltaMs.abs() >= toleranceSeconds * 1000; } -/// Which band a scan result is, by its ADVERTISED service UUIDs — the only -/// thing the scanner may accept on. +/// Which GENERATION a scan result advertises, by its advertised service UUIDs. /// -/// The scanner accepts a result because its advertisement -/// contains a supported WHOOP service UUID; it does not depend on the display -/// name. This app used to also accept any device whose cached name contained -/// "whoop", which could admit a device advertising no WHOOP service at all — -/// that fallback is gone, and this policy being the single accept decision is -/// what keeps it gone. +/// A HINT, NOT THE ACCEPT DECISION — do not turn it back into one. Acceptance +/// is deliberately broader (`advertisementLooksLikeWhoop` and the scanner's own +/// match): a band whose 128-bit service UUID spills into the scan-response +/// overflow area advertises only the 16-bit member UUID or its name, and +/// refusing those would leave WHOOP MG unpairable (#255). +/// +/// This is the narrower question the connect ROUTE asks: which generation did +/// the advertisement actually name? A name-only or 16-bit-only match names +/// none and returns null, and the connect path then probes the official gen5 +/// order first and lets GATT discovery pin the truth. Returning null here +/// therefore means "no hint", never "not a WHOOP". class ScanAcceptPolicy { /// The advertised-service prefixes that identify a WHOOP band: gen4 /// "Harvard" `61080001-…`, gen5 `fd4b0001-…`. @@ -1705,8 +1709,9 @@ class ScanAcceptPolicy { static const String gen5AdvertisedPrefix = 'fd4b0001'; /// The generation the advertisement claims — 'gen4' / 'gen5' — or null when - /// no supported WHOOP service is advertised (not accepted). [serviceUuids] - /// are the advertisement's service UUID strings, any case. + /// no supported WHOOP service UUID is advertised (no hint; the scanner may + /// still have accepted the result). [serviceUuids] are the advertisement's + /// service UUID strings, any case. static String? accepts(Iterable serviceUuids) { for (final raw in serviceUuids) { final s = raw.toLowerCase(); diff --git a/lib/data/db.dart b/lib/data/db.dart index 2a72f6cd..10c7554d 100644 --- a/lib/data/db.dart +++ b/lib/data/db.dart @@ -1663,13 +1663,18 @@ class LocalDb { 'first_seen': now, 'last_seen': now, }, conflictAlgorithm: ConflictAlgorithm.ignore); + // ONE flag decides both the placeholder and its argument. It is a local + // and not the expression twice because the two must never disagree: a + // statement whose `?` count differs from the argument list binds every + // value one column to the left, which SQLite accepts silently. + final blankAdapter = adapterId == null && clearAdapterId; await db.rawUpdate( 'UPDATE device SET ' - '${adapterId == null && clearAdapterId ? 'adapter_id = NULL, ' : 'adapter_id = COALESCE(?, adapter_id), '}' + '${blankAdapter ? 'adapter_id = NULL, ' : 'adapter_id = COALESCE(?, adapter_id), '}' 'remote_id = COALESCE(?, remote_id), label = COALESCE(?, label), ' 'tier = COALESCE(?, tier), last_seen = ? WHERE id = ?', [ - if (!(adapterId == null && clearAdapterId)) adapterId, + if (!blankAdapter) adapterId, remoteId, label, tier, diff --git a/lib/sync/paired_device.dart b/lib/sync/paired_device.dart index 8ef94b84..cfc39ff2 100644 --- a/lib/sync/paired_device.dart +++ b/lib/sync/paired_device.dart @@ -58,6 +58,20 @@ class PairedDevice { static String? _cleanGeneration(String? g) => (g == 'gen4' || g == 'gen5') ? g : null; + /// Bumped by [clear]. FORGET WINS OVER AN IN-FLIGHT SAVE. + /// + /// The heal call sites are `unawaited(PairedDevice.save(...))` — engine-state + /// pins the serial and the discovered generation fire-and-forget — so one can + /// still be between its awaits when the user's forget lands. Without this, + /// that save recreates both copies after `clear()` deleted them and the band + /// the user just forgot is paired again on the next launch. + /// + /// A counter, not a lock: [save] samples it on entry and refuses to write if + /// it moved, which is decidable because a Dart isolate interleaves only at + /// awaits. It does NOT span isolates — the headless sync isolate has its own + /// copy — and it does not need to: nothing there can unpair. + static int _forgetEpoch = 0; + static Future load() async { final row = await LocalDb.deviceRow(); final id = row?['remote_id'] as String?; @@ -100,6 +114,7 @@ class PairedDevice { String? serial, { String? generation, }) async { + final epoch = _forgetEpoch; final clean = cleanDeviceLabel(serial); final gen = _cleanGeneration(generation); final prefs = await SharedPreferences.getInstance(); @@ -116,6 +131,9 @@ class PairedDevice { final knownRemoteId = (row?['remote_id'] as String?) ?? prefs.getString(_kRemoteId); final sameDevice = knownRemoteId == remoteId; + // A forget that landed while the reads above were in flight wins: this + // save is describing a band the user has just told us to drop. + if (epoch != _forgetEpoch) return; await LocalDb.upsertDevice( adapterId: gen, remoteId: remoteId, @@ -126,6 +144,11 @@ class PairedDevice { // loud that the old family no longer applies. clearAdapterId: !sameDevice, ); + // Re-checked between the two copies as well: `clear()` empties the table + // and the mirror in that order, so a forget landing inside this window + // would otherwise leave the mirror pointing at a band the table no longer + // has — and `load()` heals FROM the mirror. + if (epoch != _forgetEpoch) return; await prefs.setString(_kRemoteId, remoteId); if (clean != null) { await prefs.setString(_kSerial, clean); @@ -143,6 +166,7 @@ class PairedDevice { /// on the next launch — and the measurements it wrote are untouched, which is /// what the forget dialog promises. static Future clear() async { + _forgetEpoch++; await LocalDb.deleteDevice(); final prefs = await SharedPreferences.getInstance(); await prefs.remove(_kRemoteId); diff --git a/test/db_serve_version_and_reads_test.dart b/test/db_serve_version_and_reads_test.dart index ab2584b5..f160691c 100644 --- a/test/db_serve_version_and_reads_test.dart +++ b/test/db_serve_version_and_reads_test.dart @@ -55,6 +55,28 @@ void main() { 'it came from the new analytics'; expect(ref('openstrap_analytics'), kAnalyticsPin, reason: why); expect(ref('openstrap_protocol'), kProtocolPin, reason: why); + + // AND THE LOCK, which is the file that actually decides what a build + // resolves. `pubspec.yaml` and the constants agreeing while the lock + // trails behind is a PARTIAL repin — the exact failure mode the repin + // comments promise this test catches, and it did not until it read this + // file. `resolved-ref` too: `ref` alone can name a moved branch. + final locked = + (loadYaml(File('pubspec.lock').readAsStringSync()) + as Map)['packages'] as Map; + String lockRef(String pkg, String field) => + ((locked[pkg] as Map)['description'] as Map)[field] as String; + + const whyLock = + 'pubspec.lock disagrees with pubspec.yaml and the pin constants — a ' + 'partial repin. Move all three together, or a build resolves a ' + 'sibling nobody reasoned about'; + for (final field in const ['ref', 'resolved-ref']) { + expect(lockRef('openstrap_analytics', field), kAnalyticsPin, + reason: whyLock); + expect(lockRef('openstrap_protocol', field), kProtocolPin, + reason: whyLock); + } }); const name = 'db_serve_version_test.db'; diff --git a/test/gen5_bootstrap_official_test.dart b/test/gen5_bootstrap_official_test.dart index 38eb7489..8134c3ab 100644 --- a/test/gen5_bootstrap_official_test.dart +++ b/test/gen5_bootstrap_official_test.dart @@ -193,6 +193,8 @@ class _Ops implements GattBootstrapOps { final bool alreadyBonded; final bool phyFails; final bool bondFails; + /// The bond-state read never answers — the hang the seam's timeout bounds. + final bool bondCheckHangs; final bool discoveryFails; final bool memfaultPresent; final Set failSubscribe; @@ -202,6 +204,7 @@ class _Ops implements GattBootstrapOps { this.alreadyBonded = false, this.phyFails = false, this.bondFails = false, + this.bondCheckHangs = false, this.discoveryFails = false, this.memfaultPresent = true, this.failSubscribe = const {}, @@ -231,6 +234,8 @@ class _Ops implements GattBootstrapOps { @override Future isBonded() async { rig.trace.add('bond:check'); + // A platform `getBondState` that never comes back. + if (bondCheckHangs) return Completer().future; return alreadyBonded; } @@ -859,6 +864,34 @@ void main() { }); }); + test('a bond-state read that never answers cannot park the bootstrap', () { + fakeAsync((async) { + final rig = _Rig()..answerAll(); + expect(_run(rig, async, ops: _Ops(rig, bondCheckHangs: true)), isFalse); + + expect(rig.trace, contains('bond:check')); + expect( + rig.trace, + isNot(contains('bond:create')), + reason: 'the read never resolved, so nothing decided to bond', + ); + expect( + rig.trace.where((t) => t.startsWith('sub:')), + isEmpty, + reason: 'no registration behind an unfinished bond step', + ); + expect(rig.commands, isEmpty); + expect(rig.ready, isFalse); + expect( + rig.engine.isConnected, + isFalse, + reason: 'the bound expired into the bond catch, which tore the ' + 'session down — without it the claim stays live forever and ' + 'every later headless drain yields to it', + ); + }); + }); + test('an already-bonded device does not create another bond', () { fakeAsync((async) { final rig = _Rig()..answerAll(); @@ -1065,8 +1098,9 @@ void main() { }); }); - group('scan acceptance is by advertised WHOOP service only', () { - test('a supported advertised service accepts; a name never does', () { + group('the advertised generation hint is by service UUID only', () { + test('a supported advertised service names a generation; nothing else ' + 'does', () { expect( ScanAcceptPolicy.accepts(['FD4B0001-CCE1-4033-93CE-002D5875F58A']), 'gen5', @@ -1079,8 +1113,9 @@ void main() { ScanAcceptPolicy.accepts([]), isNull, reason: - 'no advertised WHOOP service, no acceptance — there is no ' - 'name parameter to fall back to, by construction', + 'no advertised WHOOP service, no HINT — by construction there is ' + 'no name parameter to fall back to. Acceptance is a separate, ' + 'deliberately broader decision (#255) and is not this policy.', ); expect( ScanAcceptPolicy.accepts(['0000180f-0000-1000-8000-00805f9b34fb']), diff --git a/test/paired_device_test.dart b/test/paired_device_test.dart index 3e30d5d5..0294cd51 100644 --- a/test/paired_device_test.dart +++ b/test/paired_device_test.dart @@ -101,16 +101,78 @@ void main() { () async { await PairedDevice.save('AA:BB:CC:DD:EE:FF', null, generation: 'gen6'); expect((await PairedDevice.load())!.generation, isNull); - - // A corrupted value written by some other path never steers the route. - SharedPreferences.setMockInitialValues({ - 'paired_remote_id': 'AA:BB:CC:DD:EE:FF', - 'paired_generation': 'banana', - }); - expect((await PairedDevice.load())!.generation, isNull); }, ); + // BOTH read paths, separately: `load()` answers from the table whenever it + // has the row, so a corrupted MIRROR is only ever reached with no row — + // seeding one without deleting the row tests nothing. + test('a corrupted stored generation never steers the route', () async { + // The authoritative copy: a junk `adapter_id` on the device row. + await LocalDb.upsertDevice( + adapterId: 'banana', + remoteId: 'AA:BB:CC:DD:EE:FF', + label: '5AG0000001', + tier: 'wristOptical', + ); + var p = await PairedDevice.load(); + expect(p!.remoteId, 'AA:BB:CC:DD:EE:FF'); + expect( + p.generation, + isNull, + reason: 'adapter_id is the whole registry id space; only a framed ' + 'generation may route the connect', + ); + + // The mirror, with no row for the table branch to answer from. + await LocalDb.deleteDevice(); + SharedPreferences.setMockInitialValues({ + 'paired_remote_id': 'AA:BB:CC:DD:EE:FF', + 'paired_generation': 'banana', + }); + p = await PairedDevice.load(); + expect(p!.remoteId, 'AA:BB:CC:DD:EE:FF'); + expect(p.generation, isNull); + // The heal that just ran must not have written the junk through either. + expect((await LocalDb.deviceRow())?['adapter_id'], isNull); + }); + + test('a notify-only adapter id is not a band generation', () async { + await LocalDb.upsertDevice( + adapterId: 'ble_hrs', + remoteId: 'AA:BB:CC:DD:EE:FF', + tier: 'wristOptical', + ); + expect((await PairedDevice.load())!.generation, isNull); + }); + + // The heal call sites are `unawaited(PairedDevice.save(...))`, so a save can + // be between its awaits when the user's forget lands. If it wins, the band + // the user just forgot is paired again on the next launch. + test('a forget beats a save that was already in flight', () async { + await PairedDevice.save( + 'AA:BB:CC:DD:EE:FF', + '5AG0000001', + generation: 'gen5', + ); + + // Start the save, do not await it, and forget while it is mid-flight. + final inFlight = PairedDevice.save( + 'AA:BB:CC:DD:EE:FF', + '5AG0000002', + generation: 'gen5', + ); + await PairedDevice.clear(); + await inFlight; + + expect( + await PairedDevice.load(), + isNull, + reason: 'the forget stands — neither copy may be written back', + ); + expect((await LocalDb.deviceRow())?['remote_id'], isNull); + }); + test('clear removes the whole record, generation included', () async { await PairedDevice.save( 'AA:BB:CC:DD:EE:FF', From 8b90ef01f504f0d970433128fe224a6158932bc7 Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Thu, 27 Aug 2026 10:42:23 +0200 Subject: [PATCH 13/16] =?UTF-8?q?fix(ble,sync):=20CodeRabbit=20round=20?= =?UTF-8?q?=E2=80=94=20five=20findings=20on=20the=20gen5=20bootstrap?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A stalled bond-state read is not a bond refusal. `isBonded()` moves out of the try that owns the refusal accounting: its TimeoutException is a phone-stack condition, and counted as a refusal it walked the give-up threshold and told the user to remove a bond that was fine. `createBond()` failures still count, which is what the legacy path counted. The pre-registration delay comes off `BandEntry.preRegistrationDelay` like its post-registration twin, not from the constant — same 600 ms today, one source tomorrow. The post-SET_CLOCK correlation takes both halves from the one sample the strap was given. Re-reading the wall clock after the response resolved made `driftSec` the round-trip latency, and `setAlarm` arms at `when - driftSec`, so every alarm before the next re-verify shifted by it. An unknown `helloRevision` no longer takes the "already in sync" shortcut. The parser records the byte instead of gating on it (protocol#35) so a firmware that bumps it can still connect — hello is mandatory here — but the timestamp is still read at revision-1 offsets, so an unknown layout forfeits the shortcut and takes the unconditional SET_CLOCK, which writes freshly sampled phone time and is right under any layout. Not a connection failure: gating on the revision is exactly what the repin removed. `save()` re-checks the forget epoch after the mirror writes. The reported failure — the save re-writing `_kRemoteId` behind a `clear()` — is not reachable: that write is issued in the same synchronous slice as the guard above it, so it is always ordered ahead of the `clear()` that guard did not see, which is why `load()` never healed a forgotten pairing. The serial and generation are issued an await later and could survive, describing a band the record no longer names; the trailing check drops them. Tests: the hang test pins zero refusals and no repair guide, the failed-bond test pins the refusal it does count, an unknown-revision hello reaches READY with exactly one SET_CLOCK, the 2 s test pins the correlation at drift 0, and a forget is walked across every await of an in-flight save. --- lib/ble/ble_engine.dart | 107 +++++++++++++++++-------- lib/sync/paired_device.dart | 20 +++++ test/gen5_bootstrap_official_test.dart | 72 ++++++++++++++++- test/paired_device_test.dart | 42 ++++++++++ 4 files changed, 203 insertions(+), 38 deletions(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index c8e408c7..3afd4801 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -2805,19 +2805,33 @@ class BleEngine { // the strap gates every command behind encryption, so continuing would // run subscriptions and HELLO against writes the band silently drops. if (gatt.bondingApplies) { + // BOUNDED, AND OUTSIDE THE REFUSAL ACCOUNTING. The initial bond-state + // read is the one platform await in this bootstrap that could hang: + // `bondState`'s first emission falls through to the platform's + // `getBondState` when nothing is cached. Unbounded, a request that + // never answers parks the bootstrap right here with `_session` + // non-null and the phase still `discovering`, so `holdsBandLink` keeps + // the claim live and every later headless drain yields to a connect + // that will never finish — no recovery short of a process restart. + // + // A stalled read is a PHONE-STACK condition, not a band that refuses + // to bond, so it fails the connect WITHOUT touching `bondRefusals` / + // `needsRepairGuide`: counted as a refusal it would walk the give-up + // threshold and then tell the user to remove a bond that is fine. The + // legacy path counted `createBond()` failures only, and so does this. + // `createBond()` needs no bound of ours: the plugin gives it a + // 90-second response timeout. + final bool alreadyBonded; try { - // BOUNDED. The initial bond-state read is the one platform await in - // this bootstrap that could hang: `bondState`'s first emission falls - // through to the platform's `getBondState` when nothing is cached. - // Unbounded, a request that never answers parks the bootstrap right - // here with `_session` non-null and the phase still `discovering`, - // so `holdsBandLink` keeps the claim live and every later headless - // drain yields to a connect that will never finish — no recovery - // short of a process restart. The TimeoutException lands in the - // catch below, which fails the connect and tears the session down. - // `createBond()` needs no bound of ours: the plugin gives it a - // 90-second response timeout. - if (await gatt.isBonded().timeout(_bondStateTimeout)) { + alreadyBonded = await gatt.isBonded().timeout(_bondStateTimeout); + } catch (e) { + _log('[BOOT gen5] bond-state read failed or timed out ($e) — ' + 'bootstrap stops here; NOT counted as a bond refusal.'); + await _failConnect(); + return _Gen5ConnectOutcome.failed; + } + try { + if (alreadyBonded) { _log('[BOOT gen5] already bonded — not creating another bond.'); } else { await gatt.createBond(); @@ -2855,12 +2869,18 @@ class BleEngine { if (identical(_session, session)) await _failConnect(); return _Gen5ConnectOutcome.failed; } - // 600 ms before notification registration. - if (!await _bootstrapPause( - session, - kGen5PreRegistrationDelay, - 'the pre-registration delay', - )) { + // The pause before notification registration — + // [BandEntry.preRegistrationDelay] (600 ms on gen5), read off the ENTRY + // exactly like `_bootstrapAfterRegistration` reads its post-registration + // twin. A band-specific duration is data, and a second copy here is the + // one thing the named constant exists to prevent. + final preDelay = session.entry.preRegistrationDelay; + if (preDelay > Duration.zero && + !await _bootstrapPause( + session, + preDelay, + 'the pre-registration delay', + )) { return _Gen5ConnectOutcome.failed; } _setPhase(BleConnState.subscribing); @@ -3123,16 +3143,31 @@ class BleEngine { // same way a GET_CLOCK reply would, so both clock sources share one // brain. An implausible (unset-RTC) reading is deliberately never // correlated there; the delta below still forces the correction. - _absorbClockEpoch(hello.tsSeconds); - final helloMs = - hello.tsSeconds * 1000 + (hello.tsSubseconds * 1000) ~/ 32768; - final deltaMs = - (DateTime.now().millisecondsSinceEpoch - helloMs).abs(); - if (!BootstrapClockGate.needsCorrectionMs(deltaMs)) { - _log('[CLOCK] in sync (delta ${deltaMs}ms, tolerance ' - '${BootstrapClockGate.toleranceSeconds}s) — no correction ' - 'needed; no SET_CLOCK written.'); - return true; + // THE TIMESTAMP IS READ AT REVISION-1 OFFSETS. The pinned parser records + // `helloRevision` and no longer refuses an unknown one (protocol#35 — + // hello is MANDATORY on this path, so a firmware that bumps the byte has + // to still connect), which leaves the timestamp as the one field this + // method acts on that a moved layout could make plausible-but-wrong. An + // unknown revision therefore neither fails the connection nor becomes a + // correlation: it forfeits the "already in sync" shortcut and takes the + // unconditional SET_CLOCK below, which writes a freshly sampled PHONE + // time and is right under any layout. + if (hello.helloRevision == 1) { + _absorbClockEpoch(hello.tsSeconds); + final helloMs = + hello.tsSeconds * 1000 + (hello.tsSubseconds * 1000) ~/ 32768; + final deltaMs = + (DateTime.now().millisecondsSinceEpoch - helloMs).abs(); + if (!BootstrapClockGate.needsCorrectionMs(deltaMs)) { + _log('[CLOCK] in sync (delta ${deltaMs}ms, tolerance ' + '${BootstrapClockGate.toleranceSeconds}s) — no correction ' + 'needed; no SET_CLOCK written.'); + return true; + } + } else { + _log('[CLOCK] hello revision ${hello.helloRevision} is not the ' + 'revision-1 layout these offsets read — its timestamp is neither ' + 'trusted nor correlated; correcting unconditionally.'); } // The contract is UNCONDITIONAL at ≥2 s: one awaited SET_CLOCK with a // newly sampled phone time — even for a strap reading days ahead of the @@ -3185,13 +3220,15 @@ class BleEngine { 'the correlated response.'); final resp = await out.response; if (resp != null && resp.success) { - // The strap just took our wall time, so correlate at drift ≈ 0 without - // a read-back — an alarm armed before the next periodic re-verify must - // not be shifted by the drift this write just corrected. - _clockRef = ClockRef( - device: sec, - wall: DateTime.now().millisecondsSinceEpoch ~/ 1000, - ); + // The strap just took our wall time, so correlate at drift 0 without a + // read-back — an alarm armed before the next periodic re-verify must not + // be shifted by the drift this write just corrected. + // + // BOTH HALVES COME FROM THE ONE SAMPLE THE STRAP WAS GIVEN. Reading the + // wall clock again here samples it after `out.response` resolved — up to + // the awaiter's timeout later — so `driftSec` would be the round-trip + // latency, and `setAlarm` arms at `when - driftSec`. + _clockRef = ClockRef(device: sec, wall: sec); // And the phone-suspect verdict — computed off the PRE-correction hello // timestamp — is now stale by construction: strap and phone agree // because this write made them agree. Left set, it would defer the diff --git a/lib/sync/paired_device.dart b/lib/sync/paired_device.dart index cfc39ff2..478fb3ca 100644 --- a/lib/sync/paired_device.dart +++ b/lib/sync/paired_device.dart @@ -160,6 +160,26 @@ class PairedDevice { } else if (!sameDevice) { await prefs.remove(_kGeneration); } + // AND ONCE MORE AFTER THE MIRROR WRITES. The guards above cover only the + // windows BEFORE each copy is written, and this one covers the writes + // themselves — `clear()` landing inside them empties the mirror between + // two of these `setString`s, and the ones still to come put their keys + // back on a band the user has just forgotten. + // + // `_kRemoteId` specifically cannot come back that way — it is issued in + // the same synchronous slice as the guard above it, so it is always + // ordered ahead of the `remove` in a `clear()` whose epoch bump this guard + // did not see — which is why `load()` (it answers off `_kRemoteId`) never + // healed a forgotten pairing here. What survives is the serial and the + // generation, issued an await later, describing a band the record no + // longer names. Drop them: this is the state the epoch counter exists to + // refuse. The table needs no such re-check — `clear()` deletes it after + // the bump, so its delete is always ordered behind this save's upsert. + if (epoch != _forgetEpoch) { + await prefs.remove(_kRemoteId); + await prefs.remove(_kSerial); + await prefs.remove(_kGeneration); + } } /// Forget the primary band. BOTH copies, or the mirror puts it straight back diff --git a/test/gen5_bootstrap_official_test.dart b/test/gen5_bootstrap_official_test.dart index 8134c3ab..23e34c0f 100644 --- a/test/gen5_bootstrap_official_test.dart +++ b/test/gen5_bootstrap_official_test.dart @@ -358,6 +358,16 @@ void main() { 'the write carries newly sampled phone time, not the ' 'hello timestamp', ); + // BOTH HALVES OF THE CORRELATION COME FROM THAT ONE SAMPLE. Re-reading + // the wall clock after the response resolves would make `driftSec` the + // round trip, and `setAlarm` arms at `when - driftSec`. + expect(rig.engine.clockRef?.device, sec); + expect( + rig.engine.clockRef?.driftSec, + 0, + reason: 'the strap just took this sample — the drift the write ' + 'corrected must not come back as latency', + ); }); }); @@ -386,6 +396,41 @@ void main() { }, ); + test('an UNKNOWN hello revision still reaches READY, and forfeits the ' + '"already in sync" shortcut', () { + fakeAsync((async) { + // The pinned parser records the revision byte instead of gating on it + // (protocol#35) precisely so a firmware that bumps it can still + // connect — HELLO is MANDATORY here. But every field is still read at + // the revision-1 offsets, so the one value this path ACTS on that a + // moved layout could make plausible-but-wrong is the timestamp. + final body = _helloBody(tsSeconds: _wallNow()); + body[0] = 2; // a layout these fixed offsets do not describe + final hello = Gen5HelloInfo.parse(body)!; + final rig = _Rig(); + rig.replyTo = (seq, op) => switch (op) { + Cmd.getHello => _helloReply(seq, hello: hello), + Cmd.setClock => _clockAck(seq), + Cmd.getCustomAdvertisingName => _nameReply(seq), + _ => null, + }; + expect( + _run(rig, async), + isTrue, + reason: 'an unknown revision is NOT a connection failure — that is ' + 'the whole point of the repin', + ); + expect( + rig.count(Cmd.setClock), + 1, + reason: 'a timestamp that reads as in-sync at revision-1 offsets is ' + 'not evidence under an unknown layout, so the unconditional ' + 'write of freshly sampled phone time runs instead', + ); + expect(rig.count(Cmd.getClock), 0); + }); + }); + test('a strap TWO DAYS ahead: the contract is unconditional, and the ' 'corrected clock does not suppress the initial history drain', () { fakeAsync((async) { @@ -856,6 +901,12 @@ void main() { isTrue, reason: 'the existing repair guidance surfaces', ); + expect( + rig.engine.state.bondRefusals, + 1, + reason: 'a refused createBond() IS the thing the refusal counter ' + 'and its give-up threshold are for', + ); expect( rig.engine.isConnected, isFalse, @@ -885,10 +936,25 @@ void main() { expect( rig.engine.isConnected, isFalse, - reason: 'the bound expired into the bond catch, which tore the ' - 'session down — without it the claim stays live forever and ' - 'every later headless drain yields to it', + reason: 'the bound expired, which tore the session down — without ' + 'it the claim stays live forever and every later headless ' + 'drain yields to it', + ); + // AND IT IS NOT A BOND REFUSAL. A stalled `getBondState` is a + // phone-stack condition; counted here it would walk the give-up + // threshold and then tell the user to remove a bond that is fine. + expect( + rig.engine.state.bondRefusals, + 0, + reason: 'the band never refused anything — it was never asked', + ); + expect( + rig.engine.state.needsRepairGuide, + isFalse, + reason: '"remove the bond in system Bluetooth settings" is not the ' + 'remedy for a bond-state read that never answered', ); + expect(rig.engine.state.autoReconnectPaused, isFalse); }); }); diff --git a/test/paired_device_test.dart b/test/paired_device_test.dart index 0294cd51..269a8fee 100644 --- a/test/paired_device_test.dart +++ b/test/paired_device_test.dart @@ -173,6 +173,48 @@ void main() { expect((await LocalDb.deviceRow())?['remote_id'], isNull); }); + // ...and it has to win at EVERY point of that flight, not only at the two + // guards. `save()` writes the table, then the mirror keys one at a time, so + // a forget can land between two writes. Walk it across the save's awaits and + // require BOTH copies gone every time — including the orphan serial and + // generation a mirror write issued after the forget would leave describing + // a band the record no longer names. + test('a forget wins at every point of an in-flight save', () async { + for (var hops = 0; hops < 14; hops++) { + SharedPreferences.setMockInitialValues({}); + await LocalDb.deleteDevice(); + await PairedDevice.save( + 'AA:BB:CC:DD:EE:FF', + '5AG0000001', + generation: 'gen5', + ); + + final inFlight = PairedDevice.save( + 'AA:BB:CC:DD:EE:FF', + '5AG0000002', + generation: 'gen5', + ); + // Let the save advance `hops` turns of the event loop, then forget. + for (var i = 0; i < hops; i++) { + await Future.delayed(Duration.zero); + } + await PairedDevice.clear(); + await inFlight; + + expect( + await PairedDevice.load(), + isNull, + reason: 'the forget lost to a save $hops turns in', + ); + expect( + (await SharedPreferences.getInstance()).getKeys(), + isEmpty, + reason: 'a mirror key left behind describes a forgotten band, ' + '$hops turns in', + ); + } + }); + test('clear removes the whole record, generation included', () async { await PairedDevice.save( 'AA:BB:CC:DD:EE:FF', From 7414dd677321f92e4917cd2f1a39567e68c0e0c5 Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Thu, 27 Aug 2026 11:30:01 +0200 Subject: [PATCH 14/16] =?UTF-8?q?fix(ble,sync):=20cross-model=20review=20r?= =?UTF-8?q?ound=20=E2=80=94=20quarantine=20an=20unknown=20hello=20=20revis?= =?UTF-8?q?ion's=20fields,=20and=20serialize=20the=20pairing=20writers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from an independent review of the previous round, both accepted after checking the control flow. HELLO state publication is DURABLE and runs BEFORE the identity gate. `onState` in the absorb path writes a `band_battery` sample, can fire the battery-low/charging notification, heals the serial onto the pairing record and pushes the lock-screen widget — so an unknown revision read at revision-1 offsets could persist four fabricated values. The earlier round argued this was transient because the identity gate would reject a moved layout; that argument does not hold. The gate runs after the publication, and `cpuHex` is lowercase hex by construction, so it only fails when EMPTY — any printable bytes landing at the serial offset pass. It is a filter, not fail-closed. So the quarantine now covers what the revision-1 map describes, not just the timestamp: at `helloRevision != 1` no serial/battery/charge/wrist state is published and the charging-only opcode-151 follow-up does not start. The connection still succeeds and still takes the unconditional SET_CLOCK — a revision bump must not block connecting, which is what the repin bought. `PairedDevice.save`/`clear` are serialized through one in-isolate queue, and the trailing epoch guard from the previous round is gone. Guarding the windows between the writers' awaits cannot be made correct: a moved epoch says a forget happened, not that the mirror is still this save's to clean up, so every guard that refuses a stale write is also a guard that can delete the pairing the user just made — including via the ownership check, whose own three removals are separated by awaits. Running the two in call order removes the interleaving instead of detecting it. `_forgetEpoch` stays as the inner guard for a save already queued when the forget arrives. Tests: an unknown revision reaches READY publishing no serial/battery/charge/ wrist state and starting no opcode-151 follow-up (both fail without the quarantine). The save/clear ordering test is a contract pin and says so — the test doubles complete writes in issue order on their own, which is precisely why the fix is structural rather than another guard. flutter analyze clean, 3213 tests pass. --- lib/ble/ble_engine.dart | 34 ++++++++++++--- lib/sync/paired_device.dart | 60 +++++++++++++++++--------- test/gen5_bootstrap_official_test.dart | 45 +++++++++++++++++++ test/paired_device_test.dart | 55 ++++++++++++++++++++--- 4 files changed, 161 insertions(+), 33 deletions(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index 3afd4801..2b505a8e 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -3393,6 +3393,10 @@ class BleEngine { /// must **not** move the band out of READY". void _maybeStartBatteryPackFollowUp(_Session session) { if (!session.band.isGen5) return; + // Same quarantine as the state publication: `charging` is body[5] of the + // revision-1 map, so an unknown revision is no evidence the band is on a + // charger and must not start the opcode-151 follow-up. + if (_gen5Hello?.helloRevision != 1) return; if (_gen5Hello?.charging != true) return; if (session.batteryPackFollowUpStarted) return; session.batteryPackFollowUpStarted = true; @@ -5094,11 +5098,31 @@ class BleEngine { if (d.kind == 'cmd_response' && f['gen5_hello'] is Gen5HelloInfo) { final h = f['gen5_hello'] as Gen5HelloInfo; _gen5Hello = h; - state.serial = cleanDeviceLabel(h.serial) ?? state.serial; - if (h.batteryPct != null) state.batteryPct = h.batteryPct!.toDouble(); - state.charging = h.charging; - state.wristOn = h.wristOn; - onState(state); + // PUBLISH ONLY WHAT THE REVISION-1 MAP IS KNOWN TO DESCRIBE. An unknown + // revision still connects — the parser records the byte instead of + // gating on it (protocol#35) and hello is mandatory here — but every + // field below is read at revision-1 offsets, and `onState` is DURABLE, + // not a repaint: it writes a `band_battery` sample, can fire the + // battery-low/charging OS notification, heals this serial onto the + // PAIRING RECORD, and pushes the lock-screen widget. A moved layout + // would make all four fabricated and persistent. + // + // The identity gate below cannot be leaned on to catch that: `cpuHex` is + // lowercase hex by construction, so it only fails when EMPTY, and any + // printable bytes landing at the serial offset pass the alphanumeric + // match. It is a filter, not a fail-closed check — and it runs AFTER + // this publication either way. + if (h.helloRevision == 1) { + state.serial = cleanDeviceLabel(h.serial) ?? state.serial; + if (h.batteryPct != null) state.batteryPct = h.batteryPct!.toDouble(); + state.charging = h.charging; + state.wristOn = h.wristOn; + onState(state); + } else { + _log('[HELLO gen5] revision ${h.helloRevision} is not the revision-1 ' + 'layout these offsets read — serial, battery, charge and wrist ' + 'state are NOT published; the connection continues.'); + } _log('[HELLO gen5] serial=${h.serial} fw=${h.firmwareVersion} ' 'battery=${h.batteryPct}% charging=${h.charging} ' 'wrist=${h.wristOn} whoop5=${h.isWhoop5}'); diff --git a/lib/sync/paired_device.dart b/lib/sync/paired_device.dart index 478fb3ca..9ea819d6 100644 --- a/lib/sync/paired_device.dart +++ b/lib/sync/paired_device.dart @@ -70,8 +70,34 @@ class PairedDevice { /// it moved, which is decidable because a Dart isolate interleaves only at /// awaits. It does NOT span isolates — the headless sync isolate has its own /// copy — and it does not need to: nothing there can unpair. + /// + /// It is now the INNER of two guards: [_serialized] means a save and a clear + /// never overlap at all, so this only ever fires for a save that was already + /// queued when the forget arrived. static int _forgetEpoch = 0; + /// ONE AT A TIME. Both writers touch the same two copies across several + /// awaits each, and guarding the WINDOWS between those awaits does not work: + /// every guard that refuses a stale write is also a guard that can delete a + /// NEWER pairing's keys, because "the epoch moved" says a forget happened, + /// not that the mirror is still this save's to clean up. Running them in call + /// order removes the interleaving instead of trying to detect it — old save → + /// clear → new save, each complete before the next starts. + /// + /// ponytail: an in-isolate queue, so it orders THIS isolate only — the same + /// scope [_forgetEpoch] already had, and the headless sync isolate cannot + /// unpair. A cross-isolate lock would need the database, and nothing has ever + /// needed one. + static Future _queue = Future.value(); + + static Future _serialized(Future Function() op) { + final next = _queue.then((_) => op()); + // Keep the chain alive when an op throws: the queue must order the ones + // behind it either way, and every caller still sees its own error. + _queue = next.catchError((_) {}); + return next; + } + static Future load() async { final row = await LocalDb.deviceRow(); final id = row?['remote_id'] as String?; @@ -113,6 +139,13 @@ class PairedDevice { String remoteId, String? serial, { String? generation, + }) => + _serialized(() => _save(remoteId, serial, generation: generation)); + + static Future _save( + String remoteId, + String? serial, { + String? generation, }) async { final epoch = _forgetEpoch; final clean = cleanDeviceLabel(serial); @@ -160,32 +193,17 @@ class PairedDevice { } else if (!sameDevice) { await prefs.remove(_kGeneration); } - // AND ONCE MORE AFTER THE MIRROR WRITES. The guards above cover only the - // windows BEFORE each copy is written, and this one covers the writes - // themselves — `clear()` landing inside them empties the mirror between - // two of these `setString`s, and the ones still to come put their keys - // back on a band the user has just forgotten. - // - // `_kRemoteId` specifically cannot come back that way — it is issued in - // the same synchronous slice as the guard above it, so it is always - // ordered ahead of the `remove` in a `clear()` whose epoch bump this guard - // did not see — which is why `load()` (it answers off `_kRemoteId`) never - // healed a forgotten pairing here. What survives is the serial and the - // generation, issued an await later, describing a band the record no - // longer names. Drop them: this is the state the epoch counter exists to - // refuse. The table needs no such re-check — `clear()` deletes it after - // the bump, so its delete is always ordered behind this save's upsert. - if (epoch != _forgetEpoch) { - await prefs.remove(_kRemoteId); - await prefs.remove(_kSerial); - await prefs.remove(_kGeneration); - } } /// Forget the primary band. BOTH copies, or the mirror puts it straight back /// on the next launch — and the measurements it wrote are untouched, which is /// what the forget dialog promises. - static Future clear() async { + static Future clear() => _serialized(_clear); + + static Future _clear() async { + // Bumped inside the queued op, so a save queued BEHIND this clear does not + // see the bump and writes normally — which is what "the user re-paired" + // means. Only a save already running ahead of it is refused. _forgetEpoch++; await LocalDb.deleteDevice(); final prefs = await SharedPreferences.getInstance(); diff --git a/test/gen5_bootstrap_official_test.dart b/test/gen5_bootstrap_official_test.dart index 23e34c0f..a6db0ebe 100644 --- a/test/gen5_bootstrap_official_test.dart +++ b/test/gen5_bootstrap_official_test.dart @@ -361,6 +361,14 @@ void main() { // BOTH HALVES OF THE CORRELATION COME FROM THAT ONE SAMPLE. Re-reading // the wall clock after the response resolves would make `driftSec` the // round trip, and `setAlarm` arms at `when - driftSec`. + // + // A CONTRACT PIN, NOT A REGRESSION CATCHER: the engine samples + // `DateTime.now()`, which `fakeAsync` does not drive (it only rebinds + // `package:clock`), so no amount of elapsed fake time separates the two + // reads here and the previous shape passes this too. Making it + // discriminate means moving the engine onto `clock.now()` — a + // production change for a test, and one that would re-base every other + // wall-clock assertion in this file. expect(rig.engine.clockRef?.device, sec); expect( rig.engine.clockRef?.driftSec, @@ -428,6 +436,43 @@ void main() { 'write of freshly sampled phone time runs instead', ); expect(rig.count(Cmd.getClock), 0); + // AND NOTHING REVISION-SPECIFIC IS PUBLISHED. `onState` is durable — + // it writes a band_battery sample, can fire the battery notification, + // heals the serial onto the PAIRING RECORD and pushes the lock-screen + // widget — so a moved layout would persist four fabricated values. + // The identity gate cannot be leaned on to stop that: it runs after + // this publication, and `cpuHex` is hex by construction so it only + // fails when empty. + final st = rig.engine.state; + expect(st.serial, isNull, reason: 'body[14..24] under an unknown map'); + expect(st.batteryPct, isNull, reason: 'body[1..4] — the hello says 73'); + expect(st.charging, isNull, reason: 'body[5] bit0'); + expect(st.wristOn, isNull, reason: 'body[102]'); + }); + }); + + test('an UNKNOWN hello revision does not start the charging follow-up', () { + fakeAsync((async) { + // `charging` is body[5] of the revision-1 map, so an unknown revision + // is no evidence the band is on a charger — and opcode 151 is five + // requests five seconds apart against a band that never said so. + final body = _helloBody(tsSeconds: _wallNow(), charging: true); + body[0] = 2; + final hello = Gen5HelloInfo.parse(body)!; + final rig = _Rig(); + rig.replyTo = (seq, op) => switch (op) { + Cmd.getHello => _helloReply(seq, hello: hello), + Cmd.setClock => _clockAck(seq), + Cmd.getCustomAdvertisingName => _nameReply(seq), + _ => null, + }; + expect(_run(rig, async, elapse: const Duration(seconds: 40)), isTrue); + expect( + rig.count(Cmd.getBatteryPackInfo), + 0, + reason: 'the charge flag was read at offsets that may describe ' + 'something else entirely', + ); }); }); diff --git a/test/paired_device_test.dart b/test/paired_device_test.dart index 269a8fee..3f2e7eee 100644 --- a/test/paired_device_test.dart +++ b/test/paired_device_test.dart @@ -173,13 +173,18 @@ void main() { expect((await LocalDb.deviceRow())?['remote_id'], isNull); }); - // ...and it has to win at EVERY point of that flight, not only at the two - // guards. `save()` writes the table, then the mirror keys one at a time, so - // a forget can land between two writes. Walk it across the save's awaits and - // require BOTH copies gone every time — including the orphan serial and - // generation a mirror write issued after the forget would leave describing - // a band the record no longer names. - test('a forget wins at every point of an in-flight save', () async { + // ...and it keeps winning as the forget slides later into that flight. + // `save()` writes the table, then the mirror keys one at a time. This walks + // the forget across the save's awaits and requires BOTH copies gone at each + // one — orphan serial and generation included, since a mirror key written + // after the forget describes a band the record no longer names. + // + // A SWEEP, NOT A PROOF: the hops are event-loop turns, not a handshake with + // a specific `await`. It pins the invariant broadly; the narrow guarantee it + // cannot express — that the two writers never overlap at all — is the next + // test's. + test('a forget keeps winning as it slides later into an in-flight save', + () async { for (var hops = 0; hops < 14; hops++) { SharedPreferences.setMockInitialValues({}); await LocalDb.deleteDevice(); @@ -215,6 +220,42 @@ void main() { } }); + // THE GUARANTEE UNDERNEATH BOTH: `save()` and `clear()` are serialized, so + // three overlapping calls land in CALL order rather than interleaving across + // each other's awaits. Guarding the windows between those awaits cannot get + // this right — "a forget happened" is not the same claim as "this mirror is + // still mine to clean up", so a guard that refuses a stale write is also a + // guard that can delete the pairing the user just made. + // + // A CONTRACT PIN, NOT A REGRESSION CATCHER, and worth saying plainly: under + // the prefs/sqflite test doubles every write completes in issue order on its + // own, so this stays green with the queue removed. That is the whole reason + // the queue is the fix rather than another guard — the ordering these + // assertions describe should be structural, not a property of how fast the + // store happens to answer. + test('overlapping save/clear/save land in call order', () async { + await PairedDevice.save('AA:BB:CC:DD:EE:FF', 'OLD001', generation: 'gen5'); + + // A fire-and-forget heal for the OLD band, the user's forget, and the + // re-pair — all issued without awaiting the one before it. + final heal = + PairedDevice.save('AA:BB:CC:DD:EE:FF', 'OLD002', generation: 'gen5'); + final forget = PairedDevice.clear(); + final repair = + PairedDevice.save('11:22:33:44:55:66', 'NEW001', generation: 'gen5'); + await Future.wait([heal, forget, repair]); + + final p = await PairedDevice.load(); + expect(p?.remoteId, '11:22:33:44:55:66', reason: 'the last call wins'); + expect(p?.serial, 'NEW001'); + // The mirror is the rebuild-recovery copy, so it has to name the new band + // too — a stale op reaching back to clean up would cost exactly this. + final prefs = await SharedPreferences.getInstance(); + expect(prefs.getString('paired_remote_id'), '11:22:33:44:55:66'); + expect(prefs.getString('paired_serial'), 'NEW001'); + expect(prefs.getString('paired_generation'), 'gen5'); + }); + test('clear removes the whole record, generation included', () async { await PairedDevice.save( 'AA:BB:CC:DD:EE:FF', From a1ad77f865e816d6f7ef7ebe56726d9337f7cc9f Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Thu, 27 Aug 2026 11:59:22 +0200 Subject: [PATCH 15/16] =?UTF-8?q?fix(sync,ble):=20review=20round=203=20?= =?UTF-8?q?=E2=80=94=20load()=20is=20a=20writer,=20and=20an=20unknown=20?= =?UTF-8?q?=20revision=20must=20not=20bank=20fields=20it=20cannot=20read?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PairedDevice.load` goes through the same queue as `save`/`clear`. It reads like an accessor and is not: with no table row it heals one back FROM the mirror, which is a write. Outside the queue, a load that had already read the mirror could let a `clear()` take its one database delete and then upsert the forgotten band back afterwards — into the copy that WINS on the next launch, so the unpair silently did not stick. The heal is the whole point of the mirror and stays; it just happens where a forget cannot land inside it. `_serialized` is generic now so the load's return value passes through. Unlike the last two rounds' races this one reproduces: the new test seeds the exact state the heal exists for (mirror present, no table row), overlaps a forget with the load, and fails against `load()` outside the queue. The detailed HELLO log line moves inside the revision-1 branch. It names six fields read at revision-1 offsets and the foreground logger PERSISTS what it is handed, so under an unknown layout it banked a serial and a battery figure as though they had been read — the same imputation the field quarantine exists to stop. The unknown-revision branch logs the revision and the body length, which are true at any layout. NOT changed, and going to the PR author instead: whether the identity gate should still reject an unknown revision whose revision-1 serial/CPU offsets read as garbage. Round 2 of the review asked for the gate to be kept and round 3 asked for it to be skipped; it is a protocol-posture call about what READY means on a band we cannot identify, not a defect. flutter analyze clean, 3214 tests pass. --- lib/ble/ble_engine.dart | 17 ++++++++++++----- lib/sync/paired_device.dart | 30 ++++++++++++++++++++---------- test/paired_device_test.dart | 27 +++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 15 deletions(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index 2b505a8e..89b9a37a 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -5118,14 +5118,21 @@ class BleEngine { state.charging = h.charging; state.wristOn = h.wristOn; onState(state); + _log('[HELLO gen5] serial=${h.serial} fw=${h.firmwareVersion} ' + 'battery=${h.batteryPct}% charging=${h.charging} ' + 'wrist=${h.wristOn} whoop5=${h.isWhoop5}'); } else { + // REVISION-NEUTRAL, deliberately. The line above names six fields read + // at revision-1 offsets, and the foreground logger PERSISTS what it is + // handed — so under an unknown layout it would bank a serial and a + // battery figure as if they were read, which is the same imputation + // the quarantine above exists to stop. The revision and the body + // length are the two things true at any layout. _log('[HELLO gen5] revision ${h.helloRevision} is not the revision-1 ' - 'layout these offsets read — serial, battery, charge and wrist ' - 'state are NOT published; the connection continues.'); + 'layout these offsets read (body ${h.rawHex.length ~/ 2}B) — ' + 'serial, battery, charge and wrist state are NOT published or ' + 'logged; the connection continues.'); } - _log('[HELLO gen5] serial=${h.serial} fw=${h.firmwareVersion} ' - 'battery=${h.batteryPct}% charging=${h.charging} ' - 'wrist=${h.wristOn} whoop5=${h.isWhoop5}'); } if (d.kind == 'realtime_hr') { final hr = f['hr'] as int; diff --git a/lib/sync/paired_device.dart b/lib/sync/paired_device.dart index 9ea819d6..ce22ef0b 100644 --- a/lib/sync/paired_device.dart +++ b/lib/sync/paired_device.dart @@ -76,13 +76,21 @@ class PairedDevice { /// queued when the forget arrived. static int _forgetEpoch = 0; - /// ONE AT A TIME. Both writers touch the same two copies across several - /// awaits each, and guarding the WINDOWS between those awaits does not work: - /// every guard that refuses a stale write is also a guard that can delete a - /// NEWER pairing's keys, because "the epoch moved" says a forget happened, - /// not that the mirror is still this save's to clean up. Running them in call - /// order removes the interleaving instead of trying to detect it — old save → - /// clear → new save, each complete before the next starts. + /// ONE AT A TIME. Every one of these three touches the same two copies across + /// several awaits, and guarding the WINDOWS between those awaits does not + /// work: every guard that refuses a stale write is also a guard that can + /// delete a NEWER pairing's keys, because "the epoch moved" says a forget + /// happened, not that the mirror is still this save's to clean up. Running + /// them in call order removes the interleaving instead of trying to detect + /// it — old save → clear → new save, each complete before the next starts. + /// + /// [load] IS ONE OF THE THREE. It reads like an accessor and is not: with no + /// table row it heals one back FROM the mirror, which is a write. Left + /// outside, a load that had already read the mirror could let a `clear()` + /// take its one database delete and then upsert the forgotten band back + /// afterwards — into the copy that WINS on the next launch. The heal is the + /// whole point of the mirror, so it cannot be dropped; it just has to happen + /// where a forget cannot land inside it. /// /// ponytail: an in-isolate queue, so it orders THIS isolate only — the same /// scope [_forgetEpoch] already had, and the headless sync isolate cannot @@ -90,15 +98,17 @@ class PairedDevice { /// needed one. static Future _queue = Future.value(); - static Future _serialized(Future Function() op) { + static Future _serialized(Future Function() op) { final next = _queue.then((_) => op()); // Keep the chain alive when an op throws: the queue must order the ones // behind it either way, and every caller still sees its own error. - _queue = next.catchError((_) {}); + _queue = next.then((_) {}).catchError((_) {}); return next; } - static Future load() async { + static Future load() => _serialized(_load); + + static Future _load() async { final row = await LocalDb.deviceRow(); final id = row?['remote_id'] as String?; if (id != null && id.isNotEmpty) { diff --git a/test/paired_device_test.dart b/test/paired_device_test.dart index 3f2e7eee..ebef25ac 100644 --- a/test/paired_device_test.dart +++ b/test/paired_device_test.dart @@ -256,6 +256,33 @@ void main() { expect(prefs.getString('paired_generation'), 'gen5'); }); + // `load()` reads like an accessor and is not: with no table row it heals one + // back FROM the mirror. A forget landing inside that read-then-heal would + // take the one database delete and leave the heal to upsert the forgotten + // band afterwards — into the copy `load()` answers from first. + test('a forget landing inside a heal-from-mirror load still sticks', + () async { + // The state the heal exists for: a rebuilt/wiped database under a band + // that is still paired — mirror present, no table row. + SharedPreferences.setMockInitialValues({ + 'paired_remote_id': 'AA:BB:CC:DD:EE:FF', + 'paired_serial': '5AG0000001', + 'paired_generation': 'gen5', + }); + await LocalDb.deleteDevice(); + + final healing = PairedDevice.load(); + final forget = PairedDevice.clear(); + await Future.wait([healing, forget]); + + expect( + await PairedDevice.load(), + isNull, + reason: 'the heal must not put the forgotten band back', + ); + expect((await LocalDb.deviceRow())?['remote_id'], isNull); + }); + test('clear removes the whole record, generation included', () async { await PairedDevice.save( 'AA:BB:CC:DD:EE:FF', From e4b534e952ec23fac301ec6213bcf583bbd9e8c2 Mon Sep 17 00:00:00 2001 From: Mohammad Abdul Sahil <127765312+abdulsaheel@users.noreply.github.com> Date: Thu, 27 Aug 2026 23:00:27 +0530 Subject: [PATCH 16/16] fix pr285 remaining coderabbit findings: unknown-revision identity gate, session-query day bounds, calorie provenance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ble_engine.dart: _noteHelloSuccess only evaluates HelloIdentity from the revision-1 offsets when hello.helloRevision == 1 (the pinned parser reads those offsets unconditionally, so an unknown revision's bytes aren't identity at all and could spuriously fail the alphanumeric gate). Any other revision passes unverified, matching the clock contract's existing "unknown revision must still connect" stance. - derivation_engine.dart: _derivePreparedDay now queries sessionsInRange over the full local calendar day instead of [daySub.first, daySub.last] — a completed session with zero HR samples at all (PPG fully lost) could fall outside the substrate's own span and never reach the zero-coverage calorie credit. - derivation_engine.dart: the credited-session kcal now carries a calories_session_credit marker into calories_total's inputs_used/note, so the envelope's provenance matches what it actually priced. Left the "reject failed SET_CLOCK response" suggestion alone — an existing test (gen5_bootstrap_official_test.dart) pins the current non-null-response contract deliberately, with documented reasoning; changing it would break an intentional, tested design decision, not fix a bug. --- lib/ble/ble_engine.dart | 24 +++++++++++++++++++----- lib/compute/derivation_engine.dart | 23 +++++++++++++++++++++-- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index 89b9a37a..3f397892 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -6943,11 +6943,25 @@ class BleEngine { /// ([_finishConnect]), so a link that keeps dying between hello and READY /// still reaches the five-failure bond reset. void _noteHelloSuccess(Gen5HelloInfo h) { - _helloIdentity = HelloIdentity.evaluate( - serial: h.serial, - cpuHex: h.cpuHex, - eepromFailureSignal: h.serialLooksEepromFailure, - ); + // The pinned parser reads serial/cpuHex at revision-1 offsets REGARDLESS + // of `helloRevision` (it doesn't gate on the byte). For an unknown + // revision those bytes may not be identity at all, so evaluating them + // would invent a verdict — and a bad one could fail the alphanumeric gate + // in _gen5PostHelloGates and reject a connection the clock contract + // (which already treats an unknown revision as "must still connect") + // would otherwise allow. Only revision 1 gets a real identity verdict; + // any other revision is unverified-but-not-a-rejection. + _helloIdentity = h.helloRevision == 1 + ? HelloIdentity.evaluate( + serial: h.serial, + cpuHex: h.cpuHex, + eepromFailureSignal: h.serialLooksEepromFailure, + ) + : const HelloIdentity( + serialOk: true, + cpuOk: true, + eepromFailureSignal: false, + ); } /// record the failure, and at the fifth diff --git a/lib/compute/derivation_engine.dart b/lib/compute/derivation_engine.dart index d2b38664..f933e539 100644 --- a/lib/compute/derivation_engine.dart +++ b/lib/compute/derivation_engine.dart @@ -3659,7 +3659,16 @@ class DerivationEngine { // `liveSteps`/`stepSpans` were read above, before the pipeline input — // one resolution serves both energy passes. `.strap` is carried // alongside the total so the bundle can name the sensor that counted. - final savedSessions = await LocalDb.sessionsInRange(dayLo, dayHi); + // + // Sessions are queried over the FULL local calendar day, not + // [dayLo, dayHi] — those bounds come from daySub's own first/last + // sample, so a completed session the band's PPG lost contact for + // entirely (zero HR samples at all, e.g. a wrist-gripping lift) can + // fall outside them and never reach the zero-coverage credit below. + final savedSessions = await LocalDb.sessionsInRange( + _localDayLabelToSec(day.date), + localNextMidnightSecForDayLabel(day.date), + ); // Off-wrist / charging spans over the NAP window (which runs past this // day's end), read here because the isolate has no DB handle. These are @@ -5200,6 +5209,11 @@ class DerivationEngine { if (total != null) { wake['calories_total'] = (total as num).toDouble() + credited; } + // Provenance for the envelope built in `_applyWakeDayFeatures` below — + // without this, `calories_total`'s `inputs_used`/note would keep + // claiming an hr_1hz-only figure while the emitted value silently + // includes session-sourced kcal. + wake['calories_session_credit'] = credited; } } _applyWakeDayFeatures(bundle, scalars, wake); @@ -5270,6 +5284,8 @@ class DerivationEngine { // day it contributed nothing to would claim pedometer coverage the day // may not have. final walking = (wake['calories_walking'] as num?)?.toDouble() ?? 0.0; + final sessionCredit = + (wake['calories_session_credit'] as num?)?.toDouble() ?? 0.0; bundle['calories_total'] = { 'value': caloriesTotal.round(), 'active': calories.round(), @@ -5280,11 +5296,14 @@ class DerivationEngine { 'hr_1hz', 'profile', if (walking > 0) 'live_coverage_pedometer', + if (sessionCredit > 0) 'saved_session_calories', ], 'note': 'total daily energy: Mifflin BMR floor over the covered day + ' 'active Keytel surplus over the wake span (HR-flex)' '${walking > 0 ? ' + measured-cadence walking term ' - '(CADENCE-Adults, ${walking.round()} kcal)' : ''}', + '(CADENCE-Adults, ${walking.round()} kcal)' : ''}' + '${sessionCredit > 0 ? ' + workout-gap session calories ' + '(PPG lost the window, ${sessionCredit.round()} kcal)' : ''}', }; } // WHY each of the above is absent, per figure. This recompute is the answer