CycledLeScanner : a version scanning when screen turns on or off

[herow-io]

This PR permits to create new CycledScanner strategies.

It comes with a new CycledScanner strategy, the CycledScannerScreenState class, that scans for beacons only when screen turns on or turn off.

We split the CycledLeScanner class in two classes:

• LeScanner class that is doing the scanning job (with a LeScannerForJellyBeanMr2 and one LeScannerForLollipop)
• The CycledLeScanner that manages the calls to the LeScanner class

We introduce the notion of "ScanPeriods" for an easiest management of the "scanPeriod" and the "betweenScanPeriod"

We will be glad to get your feedbacks,

Regards,

Olivier

[davidgyoung]

Thanks for the PR, @OPscT. These are interesting ideas, and the addition of extra scanning on screen change is a good one.

Any chance you can provide more details on the test conditions that led you to conclude "we get better scanning result (and faster beacons detection) if an application scans 1s or 6s during 30s than when scanning one time for 30s"? I would be curious to know the device model/OS version used to see this, the beacon transmission rate, and the difference in the detection time when using this technique.

[herow-io]

We observe on all the devices we test from Android 4.3 to Android 7 that when a device scans for a long period (for exemple 30s.) the result of scan is delivered at the end of the scan period when using the AltBeacon library.
(If the device just starts to scan for 30s. when the beacon starts to emit, the device will detect the beacon after ~ 28s, but if the device already scan since 15s, the device will detect the beacon after ~ 14s)
To test we simply set the foregroundScanningPeriod to 30000:

 beaconManager.setForegroundScanPeriod(30000);

---

We did additional test today. We think the problem can come from the AltBeacon library delivery process.
We build a very simple app to detect beacons using directly the scanner.startScan()

ScanSettings settings = new  ScanSettings.Builder().setScanMode(ScanSettings.SCAN_MODE_LOW_LATENCY).build();
    scanner.startScan(null, settings, scanCallback);

With this app, devices detect entry and exit of our beacons immediately when scanning for 30s.
We will do further investigations in the next days.

[davidgyoung]

@OPscT, I believe this is working as designed. The detected beacons are generally delivered at the end of the scan cycle, except when the scan cycle is long and the detection causes a new entry condition, in which case they are delivered immediately to avoid a long detection delay.

[herow-io]

Ok I see and looking deeper to the code I get a better understanding of the delivery process (the staff done in the "onCycleEnd" method from the CycledLeScanCallback class and the "processFromScan" method).

So with these informations, the "ActiveScanPeriods" seems useless and we will remove it. Do you agree?

[davidgyoung]

@OPscT, yes, this may explain your observation that "we notice that we get better scanning result (and faster beacons detection) if an application scans 1s or 6s during 30s than when scanning one time for 30s." This would happen if beacons are already detected recently, so results would not get delivered until the end of the scan cycle.

[davidgyoung]

Very good PR, thank you! I think this is a very good feature that is important to add. Aside from the comments above, my only reluctance to merge right away is out of concern over this creating hard to track down bugs. There are lots of code changes here — over 1000 lines, and this is probably the most complex part of the library. I am not fully confident that after reviewing it that the changes haven’t created little problems. Unfortunately, unit test coverage of this section of code is pretty much non-existent. (Although splitting out the LeScanner from the CycledLeScanner would make adding test coverage easier by being able to have a mock LeScanner for unit tests.)

A few general questions and comments:

1. How much testing have you done with this, and under what configurations?

2. Can you update the PR description to remove the discussion of the active cycle, since this has been removed from the PR?

3. Can you please update the branch to include the latest changes from master?

4. Is there any reason for this not to be the default implementation of CycledLeScanner? The only effective change is for it to give an extra scan period on screen illumination. I think library users would typically want this, it would still be possible for folks who do not want it for some reason, and changing the default implementation does not affect the public API in any way.

[herow-io]

1. I did tests on 4 devices (Samsung S3, S5, S6 and Nexus 5 plus)
   I will redo tests when finishing to merge. Additional tests are welcome.

2. Done

3. In Progress. I have got a question about the "isBluetoothOn()" method in "CycledLeScannerForLollipop" class .
   Is that normal that the "isBluetoothOn()" method is implemented only for "CycledLeScannerForLollipop" and not for "CycledLeScannerForJellyBeanMr2" ?

4. The "CycledLeScannerScreenState" is only scanning when the screen goes on or off.
   If I get your suggestion is to permit to the default implementation to scan in addition of the current scanning period when the screen goes on or off, is it?
   I can code this in a next PR.

[davidgyoung]

@OPscT, to answer your question about isBluetoothOn(), the reason a method is in CycledLeScannerForLollipop and not in the MR2 version is because it exists in order to protect against a crash found with Android 6 ROMs. Because the crash did not happen with Android 4.x ROMs, checking for bluetooth on before starting/stopping scanning was not necessary.

Read here for more info: #414

[herow-io]

Merge done.

I did tests on Samsung S3 - Android 4.4, S4 - Android 5.0.1, S5 - Android 6.0.1, and Nexus 5 plus - Android 7.0

It seems ok for me.

I let you cross check.

[davidgyoung]

Thanks, @OPscT, I'll take a look.

[davidgyoung]

@OPscT, In testing today with the reference app on my Nexus 5X, I am seeing that background scanning continues continually after putting the app to the background. I would expect a 30 second duty cycle on scans. See log below, and note that the scan cycle doesn't stop after putting the app to the background. You always see "Waiting to stop scan cycle for another 1000 milliseconds". I don't think this is right.

Do you see this, too?

12-11 13:17:01.428 17077-17077/org.altbeacon.beaconreference D/CycledLeScanner: Scan started
12-11 13:17:02.083 17077-17077/org.altbeacon.beaconreference D/CycledLeScanner: Set scan periods called with 10000, 300000 Background mode must have changed.
12-11 13:17:02.083 17077-17077/org.altbeacon.beaconreference D/CycledLeScanner: We are in the background.  Setting wakeup alarm
12-11 13:17:02.126 17077-17077/org.altbeacon.beaconreference D/CycledLeScanner: Set a wakeup alarm to go off in 300000 ms: PendingIntent{adb82de: android.os.BinderProxy@70a8cbf}
12-11 13:17:02.461 17077-17077/org.altbeacon.beaconreference D/CycledLeScanner: Waiting to stop scan cycle for another 1000 milliseconds
12-11 13:17:02.474 17077-17077/org.altbeacon.beaconreference D/CycledLeScanner: Set a wakeup alarm to go off in 300000 ms: PendingIntent{adb82de: android.os.BinderProxy@70a8cbf}
12-11 13:17:03.511 17077-17077/org.altbeacon.beaconreference D/CycledLeScanner: Waiting to stop scan cycle for another 1000 milliseconds
12-11 13:17:03.514 17077-17077/org.altbeacon.beaconreference D/CycledLeScanner: Set a wakeup alarm to go off in 300000 ms: PendingIntent{adb82de: android.os.BinderProxy@70a8cbf}
12-11 13:17:04.556 17077-17077/org.altbeacon.beaconreference D/CycledLeScanner: Waiting to stop scan cycle for another 1000 milliseconds
12-11 13:17:04.561 17077-17077/org.altbeacon.beaconreference D/CycledLeScanner: Set a wakeup alarm to go off in 300000 ms: PendingIntent{adb82de: android.os.BinderProxy@70a8cbf}

[herow-io]

I think you did not test with the last version.

This problem was solved in the commit and I re-check this morning: It works correctly from me.

[davidgyoung]

@OPscT, I'm sorry this has been left open for so long. I really like this feature and would like to see it part of the library.

Two issues have come up since this was submitted that prevent this from being merged as-is:

1. Adding the getCycledLeScanner and setCycledLeScanner methods on BeaconManager is a problem, because in cases where the beacon scanner is in a different process per Getting ClassNotFoundException org.altbeacon.beacon.service.StartMRData #306 there will be multiple instances of this class per app, and will cause problems with changes in Add support for running service in a separate process #479. We need a different way of communicating this to the CycledLeScanner, perhaps by making ScreenStateBroadcastReceiver local to the CycledLeScanner.

2. As of Android "O", a long running scan service will not be possible, and an alternate means of scanning will be needed per Android O Background Scanning #484. In this case, the ScreenStateBroadcastReceiver will need to immediately start a scan another way. But if we can come up with an alternative to the first item, I think we can apply to this one.

[herow-io]

Hello David,

I am back,
I take time to look to your PR #484

1. About the getCycledLeScanner/setCycledLeScanner methods, a possibility is to use reflexion.
   The getCycledLeScanner/setCycledLeScanner methods can be replaced by the a getCycledLeScannerClass/setCycledLeScannerClass methods.
   The CycledLeScannerClass will be used to initialize the CycledLeScanner in the beaconService or next in the ScanHelper class, using reflexion.
   What do you think about ?

2. The ideal would be to get a JobService launched on "SCREEN_ON" or "SCREEN_OFF", but it's not a possibility that offers the JobScheduler of Android O.
   So, the only possibility I see is to continue to use the ScreenStateBroadcastReceiver, but registered/unregistered in the ScanJob class.

[davidgyoung]

Hi, @OPscT.

Yes, it would be possible to trigger an immediate ScanJob through a message received by a broadcast receiver. This would be a relatively minor change to the scheduled-job-scanning branch.

As to your other point, "About the getCycledLeScanner/setCycledLeScanner methods, a possibility is to use reflexion...." Is this even necessary? I'd really like to see screen-on scanning be automatic without having to configure a special version of CycledLeScanner. It doesn't seem there is a good reason not to do this.

[herow-io]

Hi David,

I get the point that you would like the "Screen Turns Off / Screen Turns On" scanning feature integrated as default to the CycledLeScanner.

I would like to get the possibility to configure the CycledLeScanner to develop different background scanning strategies.
I think to strategies based on beacon density: if I summarize, the background scan is higher when it's an area with high beacon density with a user is moving than when it's an area with few beacons or no beacons at all.
It's something we start to develop as a hack of the background scanning period. We can integrate it to the library in a next step if you think it's interesting.

About the way to register the "Screen On/ Screen Off" action on Android O: I don't think to launch a JobScheduler when BroadcastReceiver receives a screen action.

The broadcast screen action must be registered and unregistered manually.
And so, I imagine:

• to register the SCREEN actions (Intent.ACTION_SCREEN_OFF and ACTION_SCREEN_ON) when the ScanJob starts and to unregister it when the ScanJob finishes
• to keep the ScreenStateBroadcastReceiver calling the CycledLeScanner
• To change the way the CycledLeScanner instance is get to avoid using the BeaconManager instance.

Whatever, I can start to integrate the Screen Turns Off / On to the default CycledLeScanner to permit a merge.
Once merged, I can update the code into the Android 0 branch.

[davidgyoung]

When the ScanJob is used, I think the Intent.ACTION_SCREEN_OFF and ACTION_SCREEN_ON can be registered at all times and the intents can point at the existing StartupBroadcastReceiver. When the event is received it can simply kick off an extra immediate ScanJob. In the scheduled-job-scanning-android-o branch, this is exactly what happens when an Intent is received based on a background bluetooth detection. So you can simply add a call for that case as well, which should be a pretty simple change. See the line of code here:

https://github.com/AltBeacon/android-beacon-library/blob/scheduled-job-scanning-android-o/src/main/java/org/altbeacon/beacon/startup/StartupBroadcastReceiver.java#L43

The only difference in the screen on/off case is that the scanResults List would be empty.

As to the other point "I think to strategies based on beacon density: if I summarize, the background scan is higher when it's an area with high beacon density with a user is moving than when it's an area with few beacons or no beacons at all." We should probably discuss this in a new Github issue. Although it is related, I would like to keep the change separate if possible. I'd also like to understand more why higher beacon densities would benefit from a different scanning strategy -- it isn't obvious to me immediately why this is true.

[herow-io]

I get your point. I haven't noticed the immediate scan job.

Ok to discuss later on a new issue about the possibility of other scanning strategies.

[paulpv]

Close as fixed by #941 ?