PoC - Try handle random failures of EnableNotificationAsync [android] #192
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses random failures in Android Bluetooth LE notification enablement by replacing synchronous locks with async locks and adding retry/timeout logic.
- Replaced synchronous
objectlock withAsyncLockthroughout the Bluetooth LE device implementation - Added a 400ms delay and 10-second timeout to
EnableNotificationAsyncto handle Android BLE timing issues - Refactored descriptor write logic to support both legacy and modern Android APIs (pre/post API 33)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| BrickController2/Helpers/AsyncLock.cs | Added synchronous Lock() method to complement existing LockAsync() for callback contexts |
| BrickController2.Android/PlatformServices/BluetoothLE/BluetoothLEDevice.cs | Replaced all lock(_lock) with AsyncLock usage, added timing workarounds and API version-specific descriptor write handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using (token.Register(() => | ||
| { | ||
| lock (_lock) | ||
| using (_lock.Lock(token)) |
There was a problem hiding this comment.
Passing a cancellation token to the synchronous Lock() method inside a token.Register callback creates a circular dependency. If the token is cancelled, this lock acquisition will throw an OperationCanceledException before it can complete the cleanup logic. Consider using Lock() without a token parameter here.
| using (_lock.Lock(token)) | |
| using (_lock.Lock()) |
| return false; | ||
| } | ||
|
|
||
| // wait slightly for local internal state to settle |
There was a problem hiding this comment.
The 400ms delay lacks justification for this specific value. Consider documenting why 400ms was chosen (e.g., based on Android BLE stack timing requirements, testing results, or hardware constraints) to help future maintainers understand whether this value should be adjusted.
| // wait slightly for local internal state to settle | |
| // The Android BLE stack updates internal state asynchronously after | |
| // SetCharacteristicNotification(...). On some devices/OS versions, immediately | |
| // writing the CCC descriptor without a short delay can intermittently fail or | |
| // result in missed notifications. The 400 ms value is an empirically chosen | |
| // conservative delay that has proven reliable across tested hardware/firmware. | |
| // If you need to change this value, do so cautiously and re-test on a range of | |
| // devices, as reducing it too much may reintroduce flakiness. |
| return false; | ||
| } | ||
| #pragma warning restore CA1422 // Validate platform compatibility | ||
| return false; // BLE stack was busy or request failed immediately |
There was a problem hiding this comment.
This silent failure returns false without providing any diagnostic information about why the write failed. Consider logging the failure reason to aid debugging of BLE issues, especially since this PR is addressing random failures.
| } | ||
|
|
||
| var result = await _descriptorWriteCompletionSource.Task.ConfigureAwait(false); | ||
| // restrict wait time to avoid hanging till target device disconnects |
There was a problem hiding this comment.
The 10-second timeout value lacks justification. Document why 10 seconds was chosen and what happens if legitimate operations take longer. This is especially important since the PR aims to fix random failures - understanding timeout selection helps distinguish real failures from false positives.
| // restrict wait time to avoid hanging till target device disconnects | |
| // Use a bounded wait so we do not hang indefinitely if the BLE stack or device stops responding. | |
| // 10 seconds is chosen as a conservative upper bound: in practice, GATT descriptor writes complete | |
| // within milliseconds to a few seconds, even on slow or congested links. If the operation takes | |
| // longer than 10 seconds, WaitAsync will throw a TimeoutException (or an OperationCanceledException | |
| // if the provided cancellation token is triggered), and the error is propagated to the caller instead | |
| // of blocking until the device eventually disconnects. |
No description provided.