Repository navigation
ble: read the rest of the Device Information Service - #1653
Merged
Merged
Conversation
Manufacturer (0x2A29), model number (0x2A24) and software revision (0x2A28) sit in the SAME service NOOP already reads the serial, hardware revision and firmware from, and have simply never been asked for. All read-only, all standard, and reachable on an unbonded link - a standard 0x2A19 read succeeded on one in the field - so they cost one round-trip each and can name a strap that will not pair. Chained rather than fired together: Android runs ONE GATT operation at a time, so issuing four reads at once means three return false and are silently lost. Each is kicked by the previous landing, the same shape the serial -> hardware-revision -> firmware chain already uses. A characteristic the strap does not implement is skipped rather than stalling the chain - a strap need not implement all of DIS, and the ones it does should still be read. Log-only. Nothing gates on these; they are identity strings for a strap that may never pair, and the point is to have them in a capture at all. Caught while wiring: noteReadFailure tested three DIS characteristics BY NAME and would have gone silent on the three added beside them - re-opening the exact "a refused read is indistinguishable from one never issued" gap that reporter was added to close. The set now lives in one place so a seventh cannot be added without it. 4472 tests green; doc lint and i18n audit clean.
Re-review. The extras chain is kicked when the firmware read LANDS, and readDisFirmwareRevision returns silently in three cases - no DIS service, no 0x2A26, or 0x2A26 not readable. In all three the chain never started, so a strap publishing a manufacturer string but no firmware revision yielded nothing at all. "Not implemented" is per-characteristic, not per-service. I had already written exactly that skip-and-carry-on rule into readNextDisExtra for the middle of the chain and then left its head without it. The failure path is deliberately NOT chained past a refusal, and that asymmetry is intended: an absent characteristic says nothing about the next one, whereas a refused read latches "this strap declines DIS" per device (#1647), so carrying on would just produce more refusals and more log lines for a verdict already reached. 4472 tests green; doc lint and i18n audit clean.
ryanbr
added a commit
that referenced
this pull request
Aug 27, 2026
Android has stopped the 5/MG reconnect loop since #1642. iOS never got it, so a 5/MG here still wrote the CLIENT_HELLO every connect, lost the link ~3s later and reconnected forever - the behaviour v10.6.0 shipped on both platforms. An HCI capture settled the cause: the phone sends an SMP Pairing Request and the strap answers Pairing Not Supported (0x05), so the encrypted bond the hello waits behind can never arrive on a 5/MG. Suppressing it after the give-up latches trades an unreachable capability (puffin commands, history offload) for one that demonstrably works - continuous live HR, the "Live HR, not fully paired" state the app already models (#69). Ported, mirroring the Kotlin twins: shouldSendClientHello, giveUpSuppressesHello, countsAsBondRefusal and helloSuppressionPrefKey in a new pure Strand/BLE/HelloSuppression.swift; helloSuppressedEpitaph, helloSuppressedHint and pausedHintHandshakeUnanswered on BondRefusalGiveUp; and recordWhoop5BondRefusal, shared by both causes so the split cannot drift. #1640 had to come with it. An unanswered hello produces no write error at all - the link just drops - so it never reached this platform's give-up, which only ever saw INSUFFICIENT_AUTHENTICATION. Without that half the suppression could never trigger. didBond stays permanently false once suppressed, and CLAUDE.md names that the trap. The audit: iOS has no bond watchdog and no #982 detector, so neither can undo it; countsAsBondRefusal gates on family, so a 4.0 can never latch; an auth refusal cannot be double-counted or mislabelled, because the write-error branch clears clientHelloWriteAt before the count; and only user-driven paths reach connect(), so the one-shot retry cannot leak into an automatic reconnect. Three re-review passes found three defects, all of that same family: - keepAliveFire ALSO read didBond, so the timer - and with it the liveness watchdog - would have been switched off for exactly the link the suppression keeps up for hours, leaving a stalled stream with nothing to bounce it. The first correction was dead code: both existing startKeepAlive() sites are post-bond, so on a suppressed link the timer never started at all. - the latch outlived the strap. releaseStrap/forgetDevice reset the give-up under the invariant that no state outlives the strap it belonged to, but the latch is persisted and survived a removal. ANDROID HAD THIS TOO, since #1642; the Kotlin half is in this PR, per the parity contract. - the keep-alive started in the wrong place. Android starts it on the live-HR bonded transition, which also covers an unbonded 5/MG that has not latched yet; iOS would have had no watchdog during the give-up's first five cycles. SettingsView carried a comment left for whoever did this port: its status line falls through to "Finishing the secure pairing handshake" and would describe a handshake no longer being attempted. It now carries the same "streaming but not fully paired" arm as SettingsLogic.strapStatusLine, in all ten locales, placed after the hint so the actionable text still wins. NOT ported: the #1669 opt-in override, which is experimental, default-off, and produced 57 reconnect cycles in an hour on hardware before #1670 bounded it. KNOWN REMAINING GAP: Android reads the Device Information Service on an unbonded link (#1647/#1652/#1653); iOS reads it post-bond only, so a suppressed iOS 5/MG never learns its firmware or whether it is an MG, leaving MG-only capabilities locked. Deliberately left as a follow-up - it is a separate feature that was never ported here at all. Verification: the three hint strings are proven byte-identical to the Kotlin source by extracting and comparing both, not by eye. 17 tests over the pure rules, incl. the family gate across every DeviceFamily case, the keep-alive gate either side, and the latch's case-insensitivity; the pure file also compiled standalone with swiftc and every rule exercised. Android 4552 tests green. app-build green on Build Strand, Test Strand and Build NOOPiOS. NOT HARDWARE-VALIDATED. BLE behaviour cannot be CI-tested and no 5/MG was available on iOS. The falsifiable prediction is the one Android's change carried: after the latch, the loop stops and live HR streams continuously. If a link still drops on a cycle with no CLIENT_HELLO line, the port is wrong.
ryanbr
added a commit
that referenced
this pull request
Aug 27, 2026
…OS) (#1673) The gap #1672 left open. iOS issued the DIS identity read only inside the post-bond handshake, so a 5/MG that never bonds never read DIS at all - and since the #1635 suppression landed, "never bonded" is a PERMANENT state on these straps rather than a transient one. The Devices screen showed a WHOOP 4.0 with its firmware beside a 5/MG with none, and MG-only capabilities stayed locked because the strap never got to attest what it is. Android has had this since #1647/#1649/#1653. Ported: shouldReadDisUnbonded, disRefusedPrefKey, disReadFailureLine and shouldPublishDisFirmware as a pure Strand/BLE/DisUnbondedRead.swift twinning DisUnbondedRead.kt; the unbonded attempt itself, scheduled from the suppressed path and nowhere else, because that is the only 5/MG state known to be stable; and the four DIS characteristics this platform never read - firmware (0x2A26), manufacturer, model number, software revision. Firmware is the point of it. DIS 0x2A26 is the ONLY source an unbonded strap has, since the puffin report needs the bond that never happens - and #1634 records that the 5/MG hello frequently decodes no firmware even when it DOES bond, so this fills a real void rather than racing a value that is coming. Published as a FALLBACK, never an override. Self-limiting, deliberately. A read is the same class of operation as the write that provokes the teardown, so this could cost the stable link the suppression just bought: once per link, and never again on a strap that has refused once. A refusal is a RESULT - it settles #490, which no capture has ever done - so it is reported with its status rather than swallowed by the generic notify-failure line. Five review passes found five defects, and the last two are the ones worth reading: - the ECG PROBE was blocked only by accident. Its gate checks opt-in, attested MG and connected - but not the bond. On a suppressed strap the MG check blocked it incidentally, because the variant could only resolve from a post-bond read. Reading DIS unbonded removes that accident, and the gate would then have let the probe write puffin frames to fd4b0002 - the very characteristic whose write tears the link down, spending the stable link the suppression exists to buy. send() is no backstop: it guards connected, not bonded. The gate now requires the encrypted bond, as the ECG GATE-WRITE path always did (#269). - the per-device firmware key had readers and NO writer. I first claimed iOS had no FirmwareAttribution twin; that was wrong, and wrong because I grepped Kotlin's names against Swift. It exists, has tests, and is read by DevicesView and the debug export - but the decoded path cannot key on identity, which its own comment concedes, so perDevice always resolved nil and both readers fell back to a global value that is wrong the moment a second strap exists. The DIS site has the peripheral, so it is the one place that can attribute a firmware honestly. That fills a hole predating this change. Also from the passes: the delayed read now checks `connected`, because `peripheral` outlives a disconnect on this platform and the line would otherwise claim an attempt that never happened; three comments still describing the read as post-bond-only were corrected; and the delay is pinned to the twin's actual number (1500 x 2 = 3.0s, not the 4.0 I first guessed). Deliberate divergences, all documented in code: the extras are issued together rather than chained, because CoreBluetooth already serialises reads; the refusal latches only for an UNBONDED attempt, since a transient failure on a bonded link would poison a permanent latch; there is no explicit-bond-deferred call site, that experiment being Android-only; and the post-bond path stays silent about a missing serial, because it is re-entered from the keep-alive tick and would log every cycle. Verification: disReadFailureLine and both firmware log lines proven byte-identical to the Kotlin source by extracting and comparing - which caught a hyphen where Kotlin has an em-dash. All six DIS UUIDs verified against the twin. 14 tests over the pure rules incl. every arm of the attempt gate, the fallback-not-override rule, and that the DIS latch key cannot collide with the hello-suppression one; the pure file also compiled standalone with swiftc and every rule exercised. doc_comment_lint and the i18n gate clean. NOT hardware-validated, and this one is genuinely unproven rather than merely untested: whether a 5/MG answers a standard read on an unencrypted link is the open question #490 asks, and nothing here can settle it without a strap. Either outcome is informative, and a refusal costs one read per strap, once.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Manufacturer (
0x2A29), model number (0x2A24) and software revision (0x2A28) sit in the same service NOOP already reads the serial, hardware revision and firmware from — and have never been asked for.They matter for the 5/MG specifically: all read-only, all standard, and reachable on an unbonded link (a standard
0x2A19read succeeded on one in the field). So they cost one round-trip each and can name a strap that will not pair — unlike every puffin probe, which needs the bond.Chained, not fired together. Android runs one GATT operation at a time, so issuing four reads at once means three return
falseand are silently lost. Each is kicked by the previous landing — the same shape the existing serial → hardware-revision → firmware chain uses. A characteristic the strap does not implement is skipped rather than stalling the chain.Log-only; nothing gates on these.
Caught while wiring
noteReadFailuretested three DIS characteristics by name, and would have gone silent on the three added beside them — re-opening the exact "a refused read is indistinguishable from one never issued" gap that reporter was added to close. The set now lives in one place, so a seventh cannot be added without it.4472 tests green; doc lint and i18n audit clean. Android only — the DIS chain is Android-side.