Add support for ble scanning to speed up reconnection #1416
Conversation
|
Testing has shown that a bug that prevented the scan from being stopped leads to excessive battery usage. This should be fixed now in two ways:
Unfortunately, setting a ScanFilter with the device address of the previously connected device does not work, results are only received if another app scans at the same time. Thus, the filtering is performed in the BluetoothScanCallbackReceiver |
There was a problem hiding this comment.
Thanks a lot for this huge contribution! I tried my best to review it thoroughly, since the BLE handling with all its statefulness is quite intricate already, so please don't feel offended for the many questions and remarks!
Do I get it right that while disconnected Gadgetbridge will continue to scan forever in balanced mode? How does the balanced scan affect battery usage?
.../java/nodomain/freeyourgadget/gadgetbridge/externalevents/BluetoothScanCallbackReceiver.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Use a BLE scanner for reconnect instead of automatic BLE reconnect | ||
| */ | ||
| boolean useBleScannerForReconnect(); |
There was a problem hiding this comment.
I think this belongs a little lower in the hierarchy AbstractBTLEDeviceSupport, since the upper classes usually do not use BLE at all.
| @@ -142,6 +142,11 @@ public Context getContext() { | |||
| return context; | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean useBleScannerForReconnect() { | |||
| return false; | |||
There was a problem hiding this comment.
If this is moved to AbstractBTLEDeviceSupport, then we might even have this user configurable instead of hardcoded. Then users which suffer from bad reconnect can enable this.
There was a problem hiding this comment.
You mean similar to "Enable automatic BLE reconnect", globally for all devices? I was thinking on a per-device configuration, since the Casio devices require this anyway.
There was a problem hiding this comment.
The default implementation could check the global configuration and the casio implemention could override to forefully enable it.
I agree a pretty device-specific thing. I think until we have better device specific configurabilities, we could make it a global option.
app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/btle/BtLEQueue.java
Outdated
Show resolved
Hide resolved
|
|
||
| @TargetApi(Build.VERSION_CODES.LOLLIPOP) | ||
| private void startBleBackgroundScan(boolean highPowerMode) { | ||
| if(mBluetoothScanner == null) |
There was a problem hiding this comment.
I think we should use something similarto setDeviceConnectionState(State.WAITING_FOR_RECONNECT) to show the user that a reconnect is in progress.
app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/btle/BtLEQueue.java
Outdated
Show resolved
Hide resolved
|
|
||
| LOG.info("Scanner: Found: " + deviceName + " " + deviceAddress); | ||
| // The filter already filtered for our specific device, so it is enough to connect to it | ||
| mBluetoothScanner.stopScan(mScanCallback); |
There was a problem hiding this comment.
mScanCallback should be this (technically the same, but more understandable, IMHO).
app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/btle/BtLEQueue.java
Show resolved
Hide resolved
| @@ -338,6 +338,13 @@ | |||
| <action android:name="android.bluetooth.adapter.action.STATE_CHANGED" /> | |||
| </intent-filter> | |||
| </receiver> | |||
| <receiver | |||
There was a problem hiding this comment.
What's the lifecycle of this receiver? Is it running all the time, basically as a singleton? Is that intended? What if there were two devices, waiting for reconnection?
There was a problem hiding this comment.
Does GB support multiple devices in a waiting for reconnection state already?
As far as I have understood the Android-concept here, the receiver is always active and acting like a singleton. Since the address to match and a uuid are given as extras to the intent when starting a scan, it shouldn't be a problem to have several devices in a waiting for reconnect state.
There was a problem hiding this comment.
No, not yet, I'm just checking for the future. There's even a WIP PR for multi device PR, but we haven't gotten around at making it ready for prime time. :-(
I think I understand the intention now, maybe deserves a javadoc explanation :-)
| for(ScanResult result: scanResults) { | ||
| BluetoothDevice device = result.getDevice(); | ||
| if(device.getAddress().equals(wantedAddress) && !mSeenScanCallbackUUIDs.contains(uuid)) { | ||
| mSeenScanCallbackUUIDs.add(uuid); |
There was a problem hiding this comment.
This list is never cleared, AFAICS. Also, maybe a HashSet would be more appropriate, since the receiver may be listening for a long time.
There was a problem hiding this comment.
True, I haven't yet figured out the best point in time / the best way to clear this list. Without it, I end up with several connect requests since the device is found multiple times.
There was a problem hiding this comment.
When scanning has stopped, I would say.
|
Thank you very much for the thorough review! TBH, I fixed a few of your comments already locally, but haven't had the time to push it, yet. I will look through your comments during the next days and see what needs to be done. |
|
I think I got all the comments fixed now - regarding the UUID in the ScanCallbackReceiver: The purpose is to know when a device has been found for a given scan. The receiver is called several times for the same device and it might get called even after stopping the scan. IMHO it is sufficient to remember the last UUID that was scanned, so I removed the List and replaced it by a simple String. For multi-device-support, this probably needs fixing anyway, since there is no callback when scanning has really stopped. |
andyboeh
force-pushed
the
ble_reconnect_scan
branch
from
18643fd
to
b0fca75
Compare
andyboeh
force-pushed
the
ble_reconnect_scan
branch
from
16d7597
to
caf1e98
Compare
|
Rebasing was not exactly easy, so I merged everything into one single commit and pushed that. I hope I got everything alright. |
|
@cpfeiffer any updates on this? |
andyboeh
force-pushed
the
ble_reconnect_scan
branch
from
caf1e98
to
cfe5e52
Compare
andyboeh
force-pushed
the
ble_reconnect_scan
branch
from
cfe5e52
to
340bbec
Compare
As discussed in #1399 this adds support for BLE background scanning to speed up automatic BLE reconnect. Since this uses more power than the automatic reconnect, it needs to be configured on a per-device basis.
This work is based on improved BtLEQueue with Gatt server handling features, so #1404 should be merged first. I'm posting it early to get feedback.