-
-
Notifications
You must be signed in to change notification settings - Fork 261
fix(ble): keep pairing PIN dialog open on first connect (#2057) #2113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| // | ||
| // BLEPairingHintTests.swift | ||
| // MeshtasticTests | ||
| // | ||
| // Covers the pure-logic pieces of the issue #2057 fix (custom-PIN BLE pairing sheet | ||
| // auto-dismissing): the persisted paired-peripheral hint used to pick the connect | ||
| // timeout, and the classification of notify-state errors into pairing failures vs. | ||
| // benign per-characteristic errors. | ||
| // | ||
|
|
||
| import Foundation | ||
| import CoreBluetooth | ||
| import Testing | ||
|
|
||
| @testable import Meshtastic | ||
|
|
||
| // Serialized: these tests share global UserDefaults state (`pairedPeripheralIds`), | ||
| // so they must not run in parallel with each other. | ||
| @Suite("Paired peripheral hint", .serialized) | ||
| final class PairedPeripheralHintTests { | ||
|
|
||
| private let idA = UUID(uuidString: "00000000-0000-0000-0000-0000000000AA")! | ||
| private let idB = UUID(uuidString: "00000000-0000-0000-0000-0000000000BB")! | ||
|
|
||
| /// Snapshot of the real persisted value, captured before each test and restored in `deinit` | ||
| /// so this suite leaves no residue for later tests to observe. | ||
| private let originalPairedIds: [String] | ||
|
|
||
| /// Swift Testing creates a fresh instance per test, so `init`/`deinit` act as per-test | ||
| /// setup/teardown: start every test from a clean slate, then restore the original value. | ||
| init() { | ||
| originalPairedIds = UserDefaults.pairedPeripheralIds | ||
| UserDefaults.pairedPeripheralIds = [] | ||
| } | ||
|
|
||
| deinit { | ||
| UserDefaults.pairedPeripheralIds = originalPairedIds | ||
| } | ||
|
|
||
| @Test func rememberMakesPeripheralKnown() { | ||
| #expect(UserDefaults.isPairedPeripheral(idA) == false) | ||
|
|
||
| UserDefaults.rememberPairedPeripheral(idA) | ||
|
|
||
| #expect(UserDefaults.isPairedPeripheral(idA)) | ||
| #expect(UserDefaults.isPairedPeripheral(idB) == false) | ||
| } | ||
|
|
||
| @Test func rememberIsIdempotent() { | ||
| UserDefaults.rememberPairedPeripheral(idA) | ||
| UserDefaults.rememberPairedPeripheral(idA) | ||
|
|
||
| #expect(UserDefaults.pairedPeripheralIds == [idA.uuidString]) | ||
| } | ||
|
|
||
| @Test func forgetRemovesOnlyThatPeripheral() { | ||
| UserDefaults.rememberPairedPeripheral(idA) | ||
| UserDefaults.rememberPairedPeripheral(idB) | ||
|
|
||
| UserDefaults.forgetPairedPeripheral(idA) | ||
|
|
||
| #expect(UserDefaults.isPairedPeripheral(idA) == false) | ||
| #expect(UserDefaults.isPairedPeripheral(idB)) | ||
| } | ||
|
|
||
| @Test func forgetUnknownPeripheralIsNoOp() { | ||
| UserDefaults.rememberPairedPeripheral(idB) | ||
|
|
||
| UserDefaults.forgetPairedPeripheral(idA) | ||
|
|
||
| #expect(UserDefaults.pairedPeripheralIds == [idB.uuidString]) | ||
| } | ||
|
|
||
| @Test func storedIdsAreSorted() { | ||
| UserDefaults.rememberPairedPeripheral(idB) | ||
| UserDefaults.rememberPairedPeripheral(idA) | ||
|
|
||
| #expect(UserDefaults.pairedPeripheralIds == [idA.uuidString, idB.uuidString].sorted()) | ||
| } | ||
| } | ||
|
|
||
| @Suite("BLE pairing failure classification") | ||
| struct BLEPairingFailureTests { | ||
|
|
||
| @Test func authAndEncryptionAttErrorsArePairingFailures() { | ||
| #expect(BLEConnection.isPairingFailure(CBATTError(.insufficientAuthentication))) | ||
| #expect(BLEConnection.isPairingFailure(CBATTError(.insufficientEncryption))) | ||
| #expect(BLEConnection.isPairingFailure(CBATTError(.insufficientAuthorization))) | ||
| } | ||
|
|
||
| @Test func benignAttErrorsAreNotPairingFailures() { | ||
| // e.g. a characteristic that doesn't support notifications — must not fail an | ||
| // otherwise-good connect. | ||
| #expect(BLEConnection.isPairingFailure(CBATTError(.writeNotPermitted)) == false) | ||
| #expect(BLEConnection.isPairingFailure(CBATTError(.requestNotSupported)) == false) | ||
| } | ||
|
|
||
| @Test func encryptionAndRemovedPairingCbErrorsArePairingFailures() { | ||
| #expect(BLEConnection.isPairingFailure(CBError(.encryptionTimedOut))) | ||
| #expect(BLEConnection.isPairingFailure(CBError(.peerRemovedPairingInformation))) | ||
| } | ||
|
|
||
| @Test func unrelatedCbErrorsAreNotPairingFailures() { | ||
| #expect(BLEConnection.isPairingFailure(CBError(.connectionTimeout)) == false) | ||
| #expect(BLEConnection.isPairingFailure(CBError(.peripheralDisconnected)) == false) | ||
| } | ||
|
|
||
| @Test func genericErrorsAreNotPairingFailures() { | ||
| let generic = NSError(domain: "com.example.test", code: 42) | ||
| #expect(BLEConnection.isPairingFailure(generic) == false) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,6 +79,15 @@ During an explicit radio switch from the Connect view, the app uses the same con | |
|
|
||
| This refresh is only enabled for the switch-radio flow. Automatic reconnects and ordinary connects continue using the standard transport handshake without forcing a hardware catalog refresh. | ||
|
|
||
| ### 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. | ||
|
|
||
|
Comment on lines
+82
to
+90
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 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. 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| ## Adding a New Packet Type | ||
|
|
||
| 1. Add the protobuf definition in the `protobufs/` submodule. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.