Skip to content
This repository has been archived by the owner on Mar 25, 2024. It is now read-only.

Add support for ble scanning to speed up reconnection #1416

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andyboeh
Copy link
Contributor

@andyboeh andyboeh commented Feb 5, 2019

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.

@andyboeh
Copy link
Contributor Author

andyboeh commented Feb 6, 2019

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:

  • after five minutes of low latency scanning it switches to balanced scanning
  • scanning is successfully stopped if the wanted device has been found

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

Copy link
Contributor

@cpfeiffer cpfeiffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

/**
* Use a BLE scanner for reconnect instead of automatic BLE reconnect
*/
boolean useBleScannerForReconnect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@TargetApi(Build.VERSION_CODES.LOLLIPOP)
private void startBleBackgroundScan(boolean highPowerMode) {
if(mBluetoothScanner == null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use something similarto setDeviceConnectionState(State.WAITING_FOR_RECONNECT) to show the user that a reconnect is in progress.


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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mScanCallback should be this (technically the same, but more understandable, IMHO).

@@ -338,6 +338,13 @@
<action android:name="android.bluetooth.adapter.action.STATE_CHANGED" />
</intent-filter>
</receiver>
<receiver
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list is never cleared, AFAICS. Also, maybe a HashSet would be more appropriate, since the receiver may be listening for a long time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When scanning has stopped, I would say.

@andyboeh
Copy link
Contributor Author

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.

@andyboeh
Copy link
Contributor Author

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
Copy link
Contributor Author

Rebasing was not exactly easy, so I merged everything into one single commit and pushed that. I hope I got everything alright.

@andyboeh
Copy link
Contributor Author

andyboeh commented Apr 2, 2019

@cpfeiffer any updates on this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants