feat: add timeout for notifications and detect aborted pairing on android
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
timeoutparameter for callers to customize as needed - Maintains backward compatibility by keeping the parameter optional
And 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
startNotificationscall in response - Aligns Android’s behaviour with the existing iOS implementation
Fixes #765
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!
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 startNotification call. Would you be open to an explicit bond workflow?
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 bond method is exposed to the plugin user, that ensures the device is properly bonded before continuing.
The impact to you here would be that you explicitly call await ble.bond(...) and then await.startNotifications(...) after the bond step succeeds.
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?
@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 .startNotifications() on an encrypted characteristic. If the user cancels the pairing prompt, the plugin’s main queue never resumes, and the complete plugin is stuck.
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 createBondStateReceiver() for notifications, but it quickly became a little messy — since we’d need to track different key prefixes and execute different code for both cases on success and error. That’s why I added a separate BroadcastReceiver specifically for the notifications flow.
With a more extensive refactor, we might be able to unify everything under a single BroadcastReceiver. If this is what you suggest, I am open to try that, it will just include some more changes to existing functionality.
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).
Hi @peitschie, just a gentle ping on this — happy to clarify anything or adjust the implementation if needed. Let me know your thoughts when you get a chance. Thanks again!
Thanks again for your patience. I've been trying to get a test rig going for the scenario you are encountering here, but I am struggling to get my BLEvdev boards going again.
I'll give this another shot today
@peitschie what BLEdev boards are you using to test? Maybe I can provide a demo project, where you can reproduce the issue?
@peitschie is there anything we can do to help to get this fixed/merged?
Back again... yep, let's get this landed.
@all-contributors please add @gion-andri for code
@peitschie
@gion-andri already contributed before to code