Don't restart scanning if device detects duplicate advertisements per scan

[davidgyoung]

Background:

Restarting scanning (stopping and immediately restarting) is necessary on many older Android devices like the Nexus 4 and Moto G because once they detect a distinct BLE packet in a scan, subsequent detections do not get a scan callback. Stopping scanning and restarting clears this out, allowing subsequent detection of identical advertisements.

On most newer devices, however, this is not necessary, and multiple callbacks are given for identical packets detected in a single scan. Because the library has no simple way of knowing whether the device can detect duplicate advertisements per scan, scanning is restarted regardless.

Change

This adds a detection of duplicate identical advertisements in a single scan. If duplicates are ever detected, scans are no longer restarted.

Benefits

The primary benefit of this change is that there will no longer be brief dropouts in detections when scans are stopped and restarted, causing advertisement packets and detections to be missed as described in #473. This change will allow much shorter scan intervals while keeping detections reliable. A secondary benefit may be some power savings with some bluetooth chipsets, although this savings has not been confirmed or characterized.

Typical Impact on New Devices

The flag indicating whether or not a device supports multiple detections per scan is not persisted, so it is effectively cleared on restart. On devices supporting multiple detections per scan, restarting scanning will still happen. For default library settings and beacons transmitting at 10 Hz, this determination will happen within a second or two, causing restarts to quickly cease.

Typical Impact on Old Devices

Older devices not supporting multiple detections per scan will see no change, aside from some extra processing done to track distinct packets in each scan cycle. This extra processing is worth the tradeoff, especially as the number of such devices shrinks.

Text on Android 7.0 with beacon turned on at 04-08 14:43

$ adb logcat | grep stopping

04-08 14:42:55.881 16746 16746 D CycledLeScanner: stopping bluetooth le scan
04-08 14:42:56.999 16746 16746 D CycledLeScanner: Not stopping scan because this is Android N and we keep scanning for a minimum of 6 seconds at a time. We will stop in 4882 millisconds.
04-08 14:42:58.102 16746 16746 D CycledLeScanner: Not stopping scan because this is Android N and we keep scanning for a minimum of 6 seconds at a time. We will stop in 3779 millisconds.
04-08 14:42:59.206 16746 16746 D CycledLeScanner: Not stopping scan because this is Android N and we keep scanning for a minimum of 6 seconds at a time. We will stop in 2675 millisconds.
04-08 14:43:00.313 16746 16746 D CycledLeScanner: Not stopping scan because this is Android N and we keep scanning for a minimum of 6 seconds at a time. We will stop in 1569 millisconds.
04-08 14:43:01.471 16746 16746 D CycledLeScanner: Not stopping scan because this is Android N and we keep scanning for a minimum of 6 seconds at a time. We will stop in 410 millisconds.
04-08 14:43:02.472 16746 16746 D CycledLeScanner: Not stopping scanning.  Device capable of multiple indistinct detections per scan.
04-08 14:43:03.585 16746 16746 D CycledLeScanner: Not stopping scanning.  Device capable of multiple indistinct detections per scan.

[cupakromer]

Can we drop these accessors?

This is an internal flag that's only needed for this class. Exposing it means long term support for this flag and limits the ability of this class to be refactored / modified without breaking API changes.

[cupakromer]

Is there a reason to put this in the service and not the scanner?

[davidgyoung]

This needs to be on the service because the service class is where an
AsyncTask is kicked to do scan processing on another thread. This processing needs to be done there and not on the main thread as it is it is non-trivial. This also causes the accessors to be needed on CycledLeScanner

[cupakromer]

I do like the idea of this feature 👍

Having just recently gone through the cycler/scanner code I found it difficult to reason about the expected behavior given the complexity of the interactions. Currently CycledLeScanner has 6 flags and 5 time trackers. The CycledLeScannerForLollipop subclass adds an additional 2 flags and 2 time trackers. On top of that logic for when scans start/restart and how that happens is spread across both classes as well (for example Android M). Then there is also the CycledLeScannerForJellyBeanMr2 which doesn't add any new flags/timers but also needs to be considered in the logic flow to understand how various Android devices will behave.

I'm concerned adding this 7th flag, which is managed by another external class, will make things more difficult to reason through.

I guess it would help to understand how much of BeaconService, CycledLeScanner, CycledLeScannerForJellyBeanMr2, and CycledLeScannerForLollipop you consider part of the "public" API locked into semver and how much you consider "internal implementation details". I also ask this in light of what further changes #484 and #450 will require.

[davidgyoung]

In general, classes in the service package are not intended to be part of the public API, although this is not explicitly documented anywhere. As a result, I'm not super concerned about maintenance overhead of adding public methods to these classes. There really isn't any useful way to use these public methods when using the library in binary form -- you really have to modify the source code to do this.

There are some folks who have done exactly this by building modified versions of the CycledLEScanner as in #450, but again, these changes require other modifications to non-public methods in source code in order to make use of them.

The number of flags unfortunately is a reflection of the complexity here. This is partly in order to accommodate a number of different Android OS versions and device-specific behaviors. That said, I think this additional flag is worth the additional complexity given the compelling benefit it provides.