-
Notifications
You must be signed in to change notification settings - Fork 99
feat: add timeout for notifications and detect aborted pairing on android #766
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?
feat: add timeout for notifications and detect aborted pairing on android #766
Conversation
At a first pass, this looks excellent to me @gion-andri I'll test it out locally and report back if I see any quirks 🙂 Thanks for the contribution! |
android/src/main/java/com/capacitorjs/community/plugins/bluetoothle/Device.kt
Outdated
Show resolved
Hide resolved
fc0e1b7
to
d1a624a
Compare
Previously, iOS applied an internal timeout to the `startNotifications` command, while Android did not. This change: - Standardizes timeout behavior across both platforms - Exposes an optional `timeout` parameter for callers to customize as needed - Maintains backward compatibility by keeping the parameter optional a a
d1a624a
to
01105b2
Compare
So... sitting down and looking at this patch, I can definitely see why the functionality is needed. Thanks for the effort putting this patch together. @gion-andri what would be your appetite to make further changes to how the api for the bonding is exposed? I am not that keen on the fact this is an implicit flow hidden behind the I've implemented one like this on a previous plugin (https://github.com/don/cordova-plugin-ble-central/blob/master/src/android/BLECentralPlugin.java#L873). An explicit The impact to you here would be that you explicitly call The main advantage is it means you can more clearly identify a bonding problem in your app and report accordingly. Of course, the down-side is that this method has no iOS equivalent, so you'd need to conditionally call it when on the Android platform (or try/catch if it fails due to not being supported). What are your thoughts here? |
On Android, cancelling the OS pairing prompt for a BLE device did not interrupt the `startNotifications` call, potentially causing hangs. This update: - Detects when the user cancels bonding on Android - Aborts the `startNotifications` call in response - Aligns Android’s behaviour with the existing iOS implementation
01105b2
to
d539bda
Compare
@peitschie Thanks for taking the time to review this. I’m open to solving the issue in a different way—I just want to make sure I understand your suggestion correctly. As it stands, there is already an explicit bonding flow. That flow handles everything I added for the notifications case (including a timeout and listening for bonding state changes) already. The problem I’m trying to address is that Android will automatically initiate bonding when you call Even if we encourage consumers to explicitly bond first, the plugin shouldn’t get stuck if someone relies on the implicit bonding that Android triggers. In regards to the implementation: I initially tried to reuse With a more extensive refactor, we might be able to unify everything under a single Or maybe I misunderstood your comment completely, then please let me know :) (And: while reviewing my changes, I found that I forgot a check on the deviceId, thats my latest change I pushed). |
Previously, iOS applied an internal timeout to the
startNotifications
command, while Android did not. This change:timeout
parameter for callers to customize as neededAnd on Android, cancelling the OS pairing prompt for a BLE device did not interrupt the
startNotifications
call, potentially causing hangs. This update:startNotifications
call in responseFixes #765