Fixes Xiaomi Phones not Displaying Sensors#141
Conversation
|
|
|
Visit the preview URL for this PR (updated for commit 9b7bd26): https://open-earable-lib-web-example--pr141-fix-xiaomi-ble-s9czsav8.web.app (expires Tue, 04 Aug 2026 12:22:33 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 7f6db4d9d00b851ad49af109fa949061904b3151 |
|
is it confirmed that this does not interfere with the other platforms now? |
There was a problem hiding this comment.
Pull request overview
This PR addresses sensor scheme retrieval reliability on Android (notably Xiaomi devices) by ensuring BLE notification subscriptions are set up before issuing request/response writes, and by adding fallback and retry logic when notifications fail.
Changes:
- Add an explicit
prepareSubscription()step before subscribing/requesting sensor schemes to reduce first-packet drops. - Fall back to a direct characteristic read when a notification-based scheme response times out.
- Improve multi-sensor scheme collection with retry passes and early-stop when no progress is made; add tests for these behaviors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/v2_sensor_scheme_reader_test.dart | Adds tests covering subscription ordering, notification timeout fallback, sensor-id handling, and retry behavior. |
| lib/src/utils/sensor_scheme_parser/v2_sensor_scheme_reader.dart | Ensures subscription setup before listening, adds direct-read fallback on timeout, and introduces retry passes for scheme collection. |
| lib/src/managers/ble_manager.dart | Adds awaitable subscription setup tracking and exposes prepareSubscription() to allow callers to wait for notification enablement. |
| lib/src/managers/ble_gatt_manager.dart | Introduces prepareSubscription() API in the GATT manager interface for subscription preconditioning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final value = await responseFuture; | ||
| logger.d( | ||
| "Received notification for sensor scheme of sensor $sensorId: $value", | ||
| ); | ||
|
|
| /// Ensures notifications are enabled before request/response flows depend on them. | ||
| Future<void> prepareSubscription({ | ||
| required String deviceId, | ||
| required String serviceId, | ||
| required String characteristicId, | ||
| }) async { | ||
| subscribe( | ||
| deviceId: deviceId, | ||
| serviceId: serviceId, | ||
| characteristicId: characteristicId, | ||
| ); | ||
| } |
| try { | ||
| SensorScheme scheme = await getSchemeForSensor(sensorId); | ||
| final scheme = await getSchemeForSensor(sensorId); | ||
| _sensorSchemes[scheme.sensorId] = scheme; | ||
| } catch (e) { |
| final Stream<List<int>> stream = _bleManager.subscribe( | ||
| deviceId: _deviceId, | ||
| serviceId: parseInfoServiceUuid, |
Confirmed to work on a Google Pixel and iPhone |
DennisMoschina
left a comment
DennisMoschina
left a comment
There was a problem hiding this comment.
Overall this PR is a good approach, but it overpromises in the base class. Subscriptions are not necessarily awaited. Falling back to reading can also introduce race conditions that are not checked against. After reading, it has to be made sure, that the actual sensor ID matches the requested ID.
Maybe having a retry logic with a larger timeout would be a better approach.
| "Notification timeout while waiting for sensor scheme $sensorId: $e. " | ||
| "Falling back to direct characteristic read.", | ||
| ); | ||
| final polledValue = await _bleManager.read( |
There was a problem hiding this comment.
This does not check if the id of the sensor scheme matches the requested id
|
The same fixes are implemented by #142 together with some other fixes regarding data races |
No description provided.