-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: ble write handling #3471
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?
fix: ble write handling #3471
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a sequential write queue with retry logic to handle BLE write limitations on Android. The implementation correctly serializes write operations and adds robustness with timeouts and exponential backoff for retries. My review includes one suggestion to improve the robustness of the error-checking logic for retries, to avoid potential false positives from a broad string match.
| bool _isRetryable(Object e) { | ||
| if (e is FlutterBluePlusException && e.code == 201) return true; | ||
| final msg = e.toString().toLowerCase(); | ||
| return msg.contains("busy") || msg.contains("201"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string check msg.contains("201") is a bit fragile and could lead to incorrectly retrying on errors that are not retryable. For example, it could match parts of other numbers in an error message (e.g., a different error code like 1201, or part of a memory address). To make this more robust, consider using a more specific pattern, for example by checking for word boundaries.
| return msg.contains("busy") || msg.contains("201"); | |
| return msg.contains("busy") || RegExp(r'\b201\b').hasMatch(msg); |
|
where are the tests sir ? |
|
@beastoin pls review, thanks! |
|
1/ don't update the state on the writing; whatever the error is, it's not the write func's responsibility. 2/ why do we need to put the writewithoutresponse here https://github.com/BasedHardware/omi/pull/3471/files#diff-9642bdfe5ce0f1472db61c4f7cb153a7b340385781f777a1d674a649eb824333R203-R204? |
removed
It will choose the fastest supported write mode for the characteristic, when the device allows it for better performance and falling back to normal writes when required. |


Android processes only one ble write at once, that’s why it was crashing
fix: added sequential queue, timeout, and busy error handling
closes #3440