fix(ble): keep pairing PIN dialog open on first connect (#2057)#2113
Open
bruschill wants to merge 2 commits into
Open
fix(ble): keep pairing PIN dialog open on first connect (#2057)#2113bruschill wants to merge 2 commits into
bruschill wants to merge 2 commits into
Conversation
) The connect handshake ran with hard step timeouts and tore down the CoreBluetooth peripheral on expiry. On a first-ever connection iOS presents the pairing PIN sheet during characteristic subscription, but the old flow resolved Step 1 immediately and a 5s timeout could cancelPeripheralConnection while the sheet was up, dismissing it before the user could enter the PIN. - Gate connect completion on the FROMNUM notify subscription confirming (the definitive bonding-complete signal) instead of resolving optimistically. - Give Step 1 a 90s window for a first-time BLE bond; already-bonded radios (and non-BLE transports) keep the fast 5s fail so dead/out-of-range devices still fail quickly. Seeds the hint from preferredPeripheralId to avoid an upgrade slowdown. - Classify notify-state errors: real auth/encryption failures fail the connect cleanly (with the existing check-your-PIN message); benign per-characteristic errors are ignored. - Resume a suspended connect continuation on disconnect so a link drop during pairing (e.g. user cancels the sheet) fails fast instead of stalling. - Persist bonded peripheral UUIDs in UserDefaults as a self-healing hint. Adds unit tests for the paired-peripheral hint helpers and the pairing-failure error classifier.
bruschill
added
the
skip-docs-check
Contributor
📝 WalkthroughWalkthroughBLE connection setup now uses an adaptive first-pairing timeout and waits for CoreBluetooth confirmation that ChangesBLE pairing flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AccessoryManager
participant BLEConnection
participant CoreBluetooth
participant UserDefaults
AccessoryManager->>BLEConnection: Start Step 1 with adaptive timeout
BLEConnection->>CoreBluetooth: Discover characteristics and subscribe to FROMNUM
CoreBluetooth-->>BLEConnection: Confirm notification state
BLEConnection->>UserDefaults: Remember paired peripheral
BLEConnection-->>AccessoryManager: Continue connection process
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Meshtastic/Accessory/Accessory` Manager/AccessoryManager+Connect.swift:
- Around line 51-57: Update the preferred-device fallback used by knownBonded
and isFirstTimeBLEBond so it is consumed only once or invalidated after a
pairing failure. Ensure subsequent reconnects after iOS pairing data is removed
are treated as first-time BLE bonds, allowing the full PIN-entry timeout;
preserve the existing five-second timeout for genuinely bonded devices.
In `@Meshtastic/Accessory/Transports/Bluetooth` Low Energy/BLEConnection.swift:
- Around line 305-308: Update docs/developer/transport.md to document
BLEConnection’s notification-gated handshake, pairing timeout behavior, and
paired-hint lifecycle. Ensure the documentation reflects the current transport
implementation and satisfies the guideline requiring transport Swift changes to
update the developer transport documentation.
In `@MeshtasticTests/BLEPairingHintTests.swift`:
- Around line 25-28: Update the reset helper in BLEPairingHintTests to preserve
the existing UserDefaults.pairedPeripheralIds value and restore it after each
test via defer or equivalent teardown. Keep the initial clean-state reset while
ensuring the suite leaves the original UUID list unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 96f49be5-c3df-487c-b2a5-05157e5f0632
📒 Files selected for processing (4)
Meshtastic/Accessory/Accessory Manager/AccessoryManager+Connect.swiftMeshtastic/Accessory/Transports/Bluetooth Low Energy/BLEConnection.swiftMeshtastic/Extensions/UserDefaults.swiftMeshtasticTests/BLEPairingHintTests.swift
… migration Address CodeRabbit review on meshtastic#2113: - Convert the legacy preferredPeripheralId fallback into a one-time migration guarded by migratedPreferredPeripheralPairing, so a bond removed in iOS Settings self-heals back to the long pairing window instead of being pinned to the fast reconnect timeout. - Restore original pairedPeripheralIds after each test (struct -> final class with init/deinit teardown). - Document the notification-gated handshake, pairing timeout, and paired-hint lifecycle in docs/developer/transport.md.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/developer/transport.md`:
- Around line 82-90: Regenerate the bundled HTML documentation from the updated
transport guide by running scripts/build-docs.sh with Meshtastic/Resources/docs
as the output directory, then include all resulting generated HTML changes in
the PR.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a9279ced-0934-4954-af61-ca88a6ea4c24
📒 Files selected for processing (4)
Meshtastic/Accessory/Accessory Manager/AccessoryManager+Connect.swiftMeshtastic/Extensions/UserDefaults.swiftMeshtasticTests/BLEPairingHintTests.swiftdocs/developer/transport.md
🚧 Files skipped from review as they are similar to previous changes (2)
- Meshtastic/Extensions/UserDefaults.swift
- Meshtastic/Accessory/Accessory Manager/AccessoryManager+Connect.swift
Comment on lines
+82
to
+90
| ### BLE Pairing PIN Handshake | ||
|
|
||
| A first-ever connection to an encrypted radio makes iOS present a 6-digit pairing PIN sheet. `BLEConnection` gates connect-completion on that bond so the sheet is not torn down before the user can respond: | ||
|
|
||
| - **Notification-gated handshake.** After the required characteristics are discovered, `BLEConnection` does *not* resolve the connect step immediately. It subscribes to the `FROMNUM` notify characteristic (always notify-capable and encrypted) and holds the connect continuation open until `didUpdateNotificationState` confirms the subscription. On a first-ever connection that CoreBluetooth callback does not fire until the user dismisses the pairing sheet, so the connection stays alive while the PIN is entered. | ||
| - **Pairing timeout.** `AccessoryManager+Connect` selects the Step 1 timeout based on whether the peripheral is already bonded: a first-time BLE bond gets a long window (90s) so there is time to read and type the PIN, while already-bonded peripherals and non-BLE transports keep the fast reconnect timeout (5s) so a dead/out-of-range radio still fails quickly. | ||
| - **Pairing-failure classification.** A wrong or cancelled PIN surfaces as a `CBATTError` (insufficient authentication/encryption/authorization) or a `CBError` (`encryptionTimedOut`, `peerRemovedPairingInformation`). `BLEConnection.isPairingFailure(_:)` distinguishes these bond failures from benign per-characteristic errors (e.g. "notify not supported") so only real failures fail the connect. Cancelling the sheet often arrives as a plain peripheral disconnect, so `disconnect` also resumes any suspended connect continuation to fail Step 1 fast instead of waiting out the full window. | ||
| - **Paired-hint lifecycle.** The set of bonded peripheral UUIDs is persisted in `UserDefaults.pairedPeripheralIds`. A confirmed subscription calls `rememberPairedPeripheral`; a bond failure or a teardown while still awaiting confirmation calls `forgetPairedPeripheral`, so a bond the user removes in iOS Settings self-heals back to the long pairing window on the next attempt. The legacy `preferredPeripheralId` is migrated into this list exactly once (guarded by `migratedPreferredPeripheralPairing`) so upgrading users skip the long window on their first reconnect without permanently pinning the fast timeout. | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if the bundled HTML was regenerated and included in the recent commits.
if git log -15 --name-only | grep -qi "Meshtastic/Resources/docs/.*\.html"; then
echo "HTML files are included in the PR."
else
echo "Warning: No generated HTML docs found in the recent commits."
fiRepository: meshtastic/Meshtastic-Apple
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== status =="
git status --short
echo
echo "== changed files =="
git diff --name-only HEAD~1..HEAD 2>/dev/null || true
echo
echo "== docs html changes in recent diff =="
git diff --name-only HEAD~1..HEAD 2>/dev/null | grep -E '^Meshtastic/Resources/docs/.*\.html$' || true
echo
echo "== transport markdown change =="
git diff --name-only HEAD~1..HEAD 2>/dev/null | grep -E '^docs/developer/transport\.md$' || trueRepository: meshtastic/Meshtastic-Apple
Length of output: 481
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'HEAD: '; git rev-parse --short HEAD
printf 'Branch: '; git branch --show-current || true
echo "Changed files against merge-base with origin/main (if available):"
base=$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main 2>/dev/null || true)
if [ -n "${base:-}" ]; then
git diff --name-only "$base"..HEAD | sed 's#^`#-` #'
else
echo "(no base found)"
fi
echo
echo "Generated docs output present in diff?"
if [ -n "${base:-}" ]; then
git diff --name-only "$base"..HEAD | grep -E '^Meshtastic/Resources/docs/.*\.html$' || echo "(none)"
fi
echo
echo "Transport markdown present in diff?"
if [ -n "${base:-}" ]; then
git diff --name-only "$base"..HEAD | grep -E '^docs/developer/transport\.md$' || echo "(none)"
fiRepository: meshtastic/Meshtastic-Apple
Length of output: 630
Regenerate the bundled HTML docs. docs/developer/transport.md changed, but Meshtastic/Resources/docs wasn’t updated. Run bash scripts/build-docs.sh --output Meshtastic/Resources/docs and include the generated HTML in this PR.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/developer/transport.md` around lines 82 - 90, Regenerate the bundled
HTML documentation from the updated transport guide by running
scripts/build-docs.sh with Meshtastic/Resources/docs as the output directory,
then include all resulting generated HTML changes in the PR.
Source: Coding guidelines
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
1 participant
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.
What changed?
Fixes #2057 — the custom-PIN BLE pairing dialog opening and then immediately closing before the user can enter the PIN.
BLEConnection.didDiscoverCharacteristicsForno longer resolves the connect step immediately after issuing subscriptions. It now waits for theFROMNUMnotify subscription to confirm (peripheral(_:didUpdateNotificationStateFor:error:)), which is the definitive "bonding succeeded" signal and — on a first-ever connection — does not fire until the user dismisses the pairing sheet.AccessoryManager.connectStep 1 uses a 90s window when the peripheral is not yet known-bonded; already-bonded radios and non-BLE transports keep the fast 5s timeout so a dead/out-of-range radio still fails quickly. The hint is seeded frompreferredPeripheralIdso existing users don't pay the long window after upgrading.BLEConnection.disconnectnow resumes a still-suspended connect continuation, so if the link drops during pairing (iOS commonly delivers a cancelled sheet as a peripheral disconnect rather than a characteristic error) Step 1 fails immediately instead of stalling out the full timeout.UserDefaults(added/removed as bonds succeed or tear down mid-pairing).Why did it change?
The actor-based transport rewrite runs the connect handshake as fixed hard-timeout steps and, on timeout, tears down the CoreBluetooth peripheral via
cancelPeripheralConnection. Because the old flow resolved Step 1 before bonding completed and never waited for the pairing sheet, a 5s (or, later, 30s) timeout could cancel the connection while the system PIN sheet was still displayed — which dismisses the sheet. Pre-rewrite the connection-teardown timer was invalidated atdidConnect, before the pairing-triggering encrypted access, so the sheet survived; this restores that behavior in the new architecture.How is this tested?
MeshtasticTests/BLEPairingHintTests.swift, Swift Testing) — all green:BLEPairingFailureTests:BLEConnection.isPairingFailure(_:)classification (auth/encryptionCBATTErrorandCBError.encryptionTimedOut/peerRemovedPairingInformation→ failure; benign errors → not a failure).PairedPeripheralHintTests(.serialized): theUserDefaultspaired-hint helpers (remember/forget/idempotency/sorted storage).xcodebuild teston iPhone 16 Pro Max sim: 10/10 passing.Screenshots/Videos (when applicable)
N/A — behavior change to the BLE connect handshake; no UI changes.
Checklist
docs/user/ordocs/developer/. NoMeshtastic/Views/**or@Modelchanges, so no doc update is required — adding theskip-docs-checklabel.Summary by CodeRabbit
Bug Fixes
Improvements