Conversation
There was a problem hiding this comment.
Pull request overview
This PR standardizes BLE device IDs to a single canonical form in the Dart layer by lower-casing them on ingestion and ensuring all emitted IDs from streams/callbacks are lower-case, while converting back to the native-required case at platform boundaries (Pigeon channel + Linux BlueZ lookup). This is a breaking change intended for the next major release and is documented in the changelog.
Changes:
- Canonicalize device IDs to lower-case in
UniversalBlePlatformupdate handlers and simplify stream matching to a single lower-case compare. - Upper-case device IDs at the Dart→native boundary for Pigeon platform operations, and normalize Linux device lookup to BlueZ’s expected case.
- Add tests asserting lower-case emission from update handlers; update changelog and example lockfile.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/device_id_case_insensitivity_test.dart | Adds tests asserting lower-case emission from update* handlers. |
| lib/src/universal_ble_pigeon/universal_ble_pigeon_channel.dart | Introduces _nativeId() and applies it to Pigeon native calls; keeps emitted IDs lower-case. |
| lib/src/universal_ble_linux/universal_ble_linux.dart | Normalizes Linux device lookup by upper-casing IDs before BlueZ resolution. |
| lib/src/interfaces/universal_ble_platform_interface.dart | Canonicalizes IDs to lower-case on ingestion and simplifies stream filters accordingly. |
| example/pubspec.lock | Bumps the path-dependency version recorded for the example app. |
| CHANGELOG.md | Documents the breaking change for “next major”. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Follow-up to the case-insensitive matching in Navideck#269 (2.1.1): make the emitted case consistent too. Device ids are now canonicalised to lower-case throughout the Dart layer — every scan result, callback and stream carries the lower-case form regardless of the case the platform reports (Android upper-cased MACs, Windows/WinRT lower-cased them). Native BLE calls still need the platform's case (Android's getRemoteDevice REQUIRES upper-case; Apple's peripheral cache, Windows' address parse and Linux's BlueZ address are upper-case too), so the platform implementations convert back at their boundary — a single `_nativeId` helper in the pigeon channel, and one line in the Linux instance's device lookup. No Kotlin / Swift / C++ changes. This also lets the Navideck#269 stream matching collapse from a dual-case compare to a single lower-case one. BREAKING: callers that stored/compared an emitted id by exact case must now lower-case it (or compare case-insensitively). For the next major release. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
b7bf158 to
33e19b4
Compare
Copilot's review pointed out that peripheral-mode Pigeon calls were left out of the id canonicalisation. Peripheral round-trips still worked (emitted ids were native-case and callers passed them back verbatim), but it broke the promise of lower-case ids everywhere and Android's getMaximumNotifyLength lookup is case-sensitive, so a caller normalising ids to the documented lower-case form would get null there. - Canonicalise emitted central ids to lower-case in the peripheral base class streams (connection state, MTU, characteristic subscription) and in the pigeon read/write/descriptor request handlers and getSubscribedClients results. - Convert back at the pigeon boundary (getMaximumNotifyLength, updateCharacteristicValue) via a shared nativeDeviceId helper. - Correct the boundary comment: Windows formats MACs lower-case and parses them case-insensitively; it's Android/Apple/Linux that want upper-case (also flagged by Copilot). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011rAEwavZL9tHSe28EVhU2E
Updated breaking changes section to clarify that device IDs are now emitted in lower-case across all platforms, affecting various callbacks and streams. This change follows the case-insensitive matching introduced in version 2.1.1.
|
@postmaxin instead of parsing id's, it would be better to use class DeviceId {
final String _id;
const DeviceId(this._id);
String get native => id.toLowerCase();
// Code conversion will remain in this class only
@override
String toString() => native;
} |
Web Bluetooth device ids are opaque, case-sensitive browser tokens (Chromium emits Base64 of a random value), not the case-insensitive addresses every other platform reports. Lower-casing one corrupted the id the caller sees and broke the lookup it is passed back to: startScan stores the device under `BluetoothDevice.id`, so an emitted (folded) id missed `_bluetoothDeviceList` and connect/read/write reported deviceNotFound. Make canonicalisation a platform-overridable operation instead of an unconditional toLowerCase(): `UniversalBlePlatform.canonicalDeviceId()` defaults to lower-case, and both emission (the update* handlers) and matching (the stream filters, `_connectionEventCompleter`) go through it, so an override cannot desync the two halves. UniversalBleWeb overrides it to identity. Mirrored on UniversalBlePeripheralPlatform, with the pigeon sites calling the hook rather than open-coding the fold; `nativeDeviceId()` (upper-case at the native boundary) is unchanged. Reported by @fotiDim in review of Navideck#270. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkNeiMyH9mAWmK4opt4o8Y
|
Happy to go that way if you'd like it in this PR — one thing to check first: in the sketch, Also worth noting the canonical form can't be a constant The cost is scope: every public |
|
@postmaxin public apis should still remain same, except the case difference, DeviceId should be use internally only |
Per @rohitsangwan01's review: keep the conversion in a single class instead of spreading toLowerCase()/toUpperCase() through the layer. Public APIs still take and return ids as plain Strings — DeviceId is internal (not exported from the barrel). DeviceId carries both forms an id needs and is the only place either conversion happens: `canonical`, what the Dart layer emits, matches and keys per-device state by, and `native`, what channel calls take (Android's getRemoteDevice REQUIRES upper case, Apple's peripheral cache is keyed by the upper-case uuidString, BlueZ addresses are upper-case). `DeviceId.address` covers case-insensitive addresses; `DeviceId.opaque` carries Web Bluetooth's opaque, case-sensitive tokens through untouched. Platforms now declare only which kind they report, via `UniversalBlePlatform.hasAddressDeviceIds` (Web overrides it to false), replacing the canonicalDeviceId hook; the interface derives both forms from it, so emission and matching cannot disagree. nativeDeviceId() and native_device_id.dart are folded into DeviceId, as are the Linux device lookup and the service-cache key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkNeiMyH9mAWmK4opt4o8Y
|
Done in a3c7c27 — It carries both forms an id needs and is the only place either conversion happens: DeviceId.address('AA:BB:CC:DD:EE:FF') // canonical 'aa:bb:...', native 'AA:BB:...'
DeviceId.opaque('mHZbW+PZqBpUlZlVQrPzOQ==') // both forms verbatim (Web)
130 tests pass ( |
There was a problem hiding this comment.
🟡 Changes recommended
Cache keying currently case-folds opaque Web Bluetooth IDs (risking collisions/incorrect per-device cached state) and the example lockfile changes appear unrelated and should be clarified or reverted.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/14 changed files
- Comments generated: 1
- Review effort level: Lite
CacheHandler folded every key to lower case, which is right for an address but not for Web's opaque, case-sensitive ids: two distinct Web ids differing only in case would share one cache entry and hand back another device's services/subscriptions. Canonicalise where the platform's id kind is known instead — `UniversalBle.canonicalDeviceId` (@internal), applied at the points a caller-supplied id enters: isSubscribed, getSubscribedCharacteristics, updateSubscription and BleDeviceExtension's service-cache reads/writes. CacheHandler now stores exactly the key it is given, so ids that are not case-insensitive are never folded together, and an address still lands on one entry whichever case the caller uses. Also restore example/pubspec.lock to main's pins — an unrelated downgrade (matcher, meta, test_api) picked up from an older SDK. Both reported by Copilot in review of Navideck#270. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkNeiMyH9mAWmK4opt4o8Y
Follow-up to #269 (merged in 2.1.1): make the emitted device-id case consistent too — the fully-universal, breaking version discussed there for the next major.
What changes
Device ids are now canonicalised to lower-case throughout the Dart layer and emitted lower-case on every platform (scan results + connection / value / pairing / connection-parameter callbacks and streams). Previously each platform reported its native case — Android upper-cased MACs, Windows/WinRT lower-cased them — so a caller holding the id in the "wrong" case could split state or (before #269) miss events.
How — Dart-only, no Kotlin / Swift / C++ changes
update*handlers lower-case on ingestion, so every event, callback and per-device map key is lower-case. This also lets Match device ids case-insensitively across event streams #269's dual-case stream matching collapse to a single lower-case compare.getRemoteDevicerequires upper-case and throws otherwise; Apple's peripheral cache is keyed by the upper-caseuuidString, Windows parses the address either way, Linux's BlueZ address is upper-case), so the platform implementations convert back at their boundary: a single_nativeId()helper at the pigeon-channel native-op sites, and one line in the Linux instance's device lookup (the single point every Linux op funnels through).Note the native side receives the same upper-case id it always has — it's just reconstructed at the Dart boundary now instead of being supplied by the caller — so native behaviour is unchanged.
Breaking
Callers that stored or compared an emitted id by exact case (e.g. an Android upper-case MAC) must now lower-case it, or compare case-insensitively.
CHANGELOGupdated under "next major".Testing
flutter test) andflutter analyzeis clean — including 5 new tests asserting lower-case emission from everyupdate*handler, with the existing Match device ids case-insensitively across event streams #269 case-insensitive-matching / dedup / cache tests still green.connect()) at this branch and confirmed the whole path: scan →connect()→ auth → characteristic streaming → and the reconnect/self-heal path. That's exactly the round-trip this change relies on — Android'sgetRemoteDevicethrows on a lower-case MAC, so a successful connect proves the boundary conversion is doing its job.Developed with AI assistance (noted via the commit's
Co-Authored-Bytrailer).