Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scan documentation update and stop scan bug fix #513

Merged
merged 5 commits into from
Jun 8, 2017
Merged

Scan documentation update and stop scan bug fix #513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3acdfa0
Add some documentation for cycled scanner handlers
cupakromer May 24, 2017
1be324b
Start documenting thread usage for service objects
cupakromer May 24, 2017
5f98c55
Fix where scanner thread is quit.
cupakromer May 24, 2017
0ab8bee
Future proof threading / scheduled task flows
cupakromer May 24, 2017
0f5e823
Add thread annotations for `CycledLeScanCallback`
cupakromer May 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 35 additions & 14 deletions src/main/java/org/altbeacon/beacon/service/BeaconService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,12 @@
import android.os.Build;
import android.os.Handler;
import android.os.IBinder;
import android.os.Looper;
import android.os.Message;
import android.os.Messenger;
import android.support.annotation.MainThread;
import android.support.annotation.NonNull;
import android.support.annotation.WorkerThread;

import org.altbeacon.beacon.Beacon;
import org.altbeacon.beacon.BeaconManager;
Expand Down Expand Up @@ -145,9 +148,16 @@ static class IncomingHandler extends Handler {
private final WeakReference<BeaconService> mService;

IncomingHandler(BeaconService service) {
/*
* Explicitly state this uses the main thread. Without this we defer to where the
* service instance is initialized/created; which is usually the main thread anyways.
* But by being explicit we document our code design expectations for where things run.
*/
super(Looper.getMainLooper());
mService = new WeakReference<BeaconService>(service);
}

@MainThread
@Override
public void handleMessage(Message msg) {
BeaconService service = mService.get();
Expand Down Expand Up @@ -206,7 +216,7 @@ else if (msg.what == MSG_SYNC_SETTINGS) {
*/
final Messenger mMessenger = new Messenger(new IncomingHandler(this));


@MainThread
@Override
public void onCreate() {
bluetoothCrashResolver = new BluetoothCrashResolver(this);
Expand Down Expand Up @@ -294,6 +304,7 @@ public boolean onUnbind(Intent intent) {
return false;
}

@MainThread
@Override
public void onDestroy() {
LogManager.e(TAG, "onDestroy()");
Expand Down Expand Up @@ -330,6 +341,7 @@ private PendingIntent getRestartIntent() {
/**
* methods for clients
*/
@MainThread
Copy link
Member

Choose a reason for hiding this comment

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

Is this annotation intended to be used this way? The docs say it is supposed to indicate that the method should only be called on the main thread, and I don't see why that would be true in this case. It seems to be intended for methods that manipulate the UI. In this case, I don't think there is anything wrong from calling on a worker thread. Perhaps a comment would be a better way to indicate that it is not necessarily run on a worker thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The scanner classes are not thread safe. They are started / run from the beacon service which runs from the main thread. Additionally, in the scanner class mHandler is defined on the main looper. One of the deferred tasks which are queued on this handler call scanLeDevice. As things are right now, for all intents and purposes these methods need to run on the main thread.

While we could add this to the javadoc, the annotation documents this with the benefit of giving an extra hint to intellisense. Now if you write the following you'll get a lint warning/error depending on the IDE configuration:

mScanHandler.post(new Runnable() {
    @WorkerThread
    @Override
    public void run() {
        scanLeDevice(true);
    }
}

public void startRangingBeaconsInRegion(Region region, Callback callback) {
synchronized (rangedRegionState) {
if (rangedRegionState.containsKey(region)) {
Expand All @@ -342,6 +354,7 @@ public void startRangingBeaconsInRegion(Region region, Callback callback) {
mCycledScanner.start();
}

@MainThread
public void stopRangingBeaconsInRegion(Region region) {
int rangedRegionCount;
synchronized (rangedRegionState) {
Expand All @@ -355,13 +368,15 @@ public void stopRangingBeaconsInRegion(Region region) {
}
}

@MainThread
public void startMonitoringBeaconsInRegion(Region region, Callback callback) {
LogManager.d(TAG, "startMonitoring called");
monitoringStatus.addRegion(region, callback);
LogManager.d(TAG, "Currently monitoring %s regions.", monitoringStatus.regionsCount());
mCycledScanner.start();
}

@MainThread
public void stopMonitoringBeaconsInRegion(Region region) {
LogManager.d(TAG, "stopMonitoring called");
monitoringStatus.removeRegion(region);
Expand All @@ -371,11 +386,13 @@ public void stopMonitoringBeaconsInRegion(Region region) {
}
}

@MainThread
public void setScanPeriods(long scanPeriod, long betweenScanPeriod, boolean backgroundFlag) {
mCycledScanner.setScanPeriods(scanPeriod, betweenScanPeriod, backgroundFlag);
}

protected final CycledLeScanCallback mCycledLeScanCallback = new CycledLeScanCallback() {
@MainThread
@TargetApi(Build.VERSION_CODES.HONEYCOMB)
@Override
public void onLeScan(BluetoothDevice device, int rssi, byte[] scanRecord) {
Expand All @@ -390,6 +407,7 @@ public void onLeScan(BluetoothDevice device, int rssi, byte[] scanRecord) {
}
}

@MainThread
@Override
public void onCycleEnd() {
mDistinctPacketDetector.clearDetections();
Expand All @@ -403,6 +421,9 @@ public void onCycleEnd() {

if (0 != (getApplicationInfo().flags &= ApplicationInfo.FLAG_DEBUGGABLE)) {
for (Beacon beacon : simulatedScanData) {
// This is an expensive call and we do not want to block the main thread.
// But here we are in debug/test mode so we allow it on the main thread.
//noinspection WrongThread
processBeaconFromScan(beacon);
}
} else {
Expand All @@ -415,6 +436,9 @@ public void onCycleEnd() {
if (BeaconManager.getBeaconSimulator().getBeacons() != null) {
if (0 != (getApplicationInfo().flags &= ApplicationInfo.FLAG_DEBUGGABLE)) {
for (Beacon beacon : BeaconManager.getBeaconSimulator().getBeacons()) {
// This is an expensive call and we do not want to block the main thread.
// But here we are in debug/test mode so we allow it on the main thread.
//noinspection WrongThread
processBeaconFromScan(beacon);
}
} else {
Expand All @@ -437,7 +461,15 @@ private void processRangeData() {
}
}

private void processBeaconFromScan(Beacon beacon) {
/**
* Helper for processing BLE beacons. This has been extracted from {@link ScanProcessor} to
* support simulated scan data for test and debug environments.
* <p>
* Processing beacons is a frequent and expensive operation. It should not be run on the main
* thread to avoid UI contention.
*/
@WorkerThread
private void processBeaconFromScan(@NonNull Beacon beacon) {
if (Stats.getInstance().isEnabled()) {
Stats.getInstance().log(beacon);
}
Expand Down Expand Up @@ -505,6 +537,7 @@ public ScanProcessor(NonBeaconLeScanCallback nonBeaconLeScanCallback) {
mNonBeaconLeScanCallback = nonBeaconLeScanCallback;
}

@WorkerThread
@Override
protected Void doInBackground(ScanData... params) {
ScanData scanData = params[0];
Expand Down Expand Up @@ -539,18 +572,6 @@ protected Void doInBackground(ScanData... params) {
}
return null;
}

@Override
protected void onPostExecute(Void result) {
}

@Override
protected void onPreExecute() {
}

@Override
protected void onProgressUpdate(Void... values) {
}
}

private List<Region> matchingRegions(Beacon beacon, Collection<Region> regions) {
Expand Down
11 changes: 9 additions & 2 deletions src/main/java/org/altbeacon/beacon/service/scanner/CycledLeScanCallback.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
package org.altbeacon.beacon.service.scanner;

import android.bluetooth.BluetoothDevice;
import android.support.annotation.MainThread;

/**
* Android API agnostic Bluetooth scan callback wrapper.
* <p>
* Since Android bluetooth scan callbacks occur on the main thread it is expected that these
* callbacks will also occur on the main thread.
*
* Created by dyoung on 10/6/14.
*/
@MainThread
public interface CycledLeScanCallback {
public void onLeScan(BluetoothDevice device, int rssi, byte[] scanRecord);
public void onCycleEnd();
void onLeScan(BluetoothDevice device, int rssi, byte[] scanRecord);
void onCycleEnd();
}
75 changes: 65 additions & 10 deletions src/main/java/org/altbeacon/beacon/service/scanner/CycledLeScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package org.altbeacon.beacon.service.scanner;

import android.Manifest;
import android.annotation.SuppressLint;
import android.annotation.TargetApi;
import android.app.AlarmManager;
import android.app.PendingIntent;
Expand All @@ -15,11 +14,16 @@
import android.os.HandlerThread;
import android.os.Looper;
import android.os.SystemClock;
import android.support.annotation.AnyThread;
import android.support.annotation.MainThread;
import android.support.annotation.NonNull;
import android.support.annotation.WorkerThread;

import org.altbeacon.beacon.BeaconManager;
import org.altbeacon.beacon.logging.LogManager;
import org.altbeacon.beacon.startup.StartupBroadcastReceiver;
import org.altbeacon.bluetooth.BluetoothCrashResolver;

import java.util.Date;

@TargetApi(18)
Expand All @@ -41,8 +45,30 @@ public abstract class CycledLeScanner {

protected long mBetweenScanPeriod;

/**
* Main thread handle for scheduling scan cycle tasks.
* <p>
* Use this to schedule deferred tasks such as the following:
* <ul>
* <li>{@link #scheduleScanCycleStop()}</li>
* <li>{@link #scanLeDevice(Boolean) scanLeDevice(true)} from {@link #deferScanIfNeeded()}</li>
* </ul>
*/
@NonNull
protected final Handler mHandler = new Handler(Looper.getMainLooper());

/**
* Handler to background thread for interacting with the low-level Android BLE scanner.
* <p>
* Use this to queue any potentially long running BLE scanner actions such as starts and stops.
*/
@NonNull
protected final Handler mScanHandler;

/**
* Worker thread hosting the internal scanner message queue.
*/
@NonNull
private final HandlerThread mScanThread;

protected final BluetoothCrashResolver mBluetoothCrashResolver;
Expand All @@ -51,7 +77,22 @@ public abstract class CycledLeScanner {
protected boolean mBackgroundFlag = false;
protected boolean mRestartNeeded = false;

private boolean mDistinctPacketsDetectedPerScan = false;
/**
* Flag indicating device hardware supports detecting multiple identical packets per scan.
* <p>
* 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 device,
* this is not necessary, and multiple callbacks are given for identical packets detected in
* a single scan.
* <p>
* This is declared {@code volatile} because it may be set by a background scan thread while
* we are in a method on the main thread which will end up checking it. Using this modifier
* ensures that when we read the flag we'll always see the most recently written value. This is
* also true for background scan threads which may be running concurrently.
*/
private volatile boolean mDistinctPacketsDetectedPerScan = false;
private static final long ANDROID_N_MIN_SCAN_CYCLE_MILLIS = 6000l;

protected CycledLeScanner(Context context, long scanPeriod, long betweenScanPeriod, boolean backgroundFlag, CycledLeScanCallback cycledLeScanCallback, BluetoothCrashResolver crashResolver) {
Expand Down Expand Up @@ -101,6 +142,7 @@ public static CycledLeScanner createScanner(Context context, long scanPeriod, lo
* between LOW_POWER_MODE vs. LOW_LATENCY_MODE
* @param backgroundFlag
*/
@MainThread
public void setScanPeriods(long scanPeriod, long betweenScanPeriod, boolean backgroundFlag) {
LogManager.d(TAG, "Set scan periods called with %s, %s Background mode must have changed.",
scanPeriod, betweenScanPeriod);
Expand Down Expand Up @@ -141,6 +183,7 @@ public void setScanPeriods(long scanPeriod, long betweenScanPeriod, boolean back
}
}

@MainThread
public void start() {
LogManager.d(TAG, "start called");
mScanningEnabled = true;
Expand All @@ -151,7 +194,7 @@ public void start() {
}
}

@SuppressLint("NewApi")
@MainThread
public void stop() {
LogManager.d(TAG, "stop called");
mScanningEnabled = false;
Expand All @@ -162,26 +205,32 @@ public void stop() {
}
}

@AnyThread
public boolean getDistinctPacketsDetectedPerScan() {
return mDistinctPacketsDetectedPerScan;
}

@AnyThread
public void setDistinctPacketsDetectedPerScan(boolean detected) {
mDistinctPacketsDetectedPerScan = detected;
}

@MainThread
public void destroy() {
LogManager.d(TAG, "Destroying");

// Remove any postDelayed Runnables queued for the next scan cycle
mHandler.removeCallbacksAndMessages(null);

// We cannot quit the thread used by the handler until queued Runnables have been processed,
// because the handler is what stops scanning, and we do not want scanning left on.
// So we stop the thread using the handler, so we make sure it happens after all other
// waiting Runnables are finished.
mHandler.post(new Runnable() {
mScanHandler.post(new Runnable() {
@WorkerThread
@Override
public void run() {
LogManager.d(TAG, "Quitting scan thread");
// Remove any postDelayed Runnables queued for the next scan cycle
mHandler.removeCallbacksAndMessages(null);
mScanThread.quit();
}
});
Expand All @@ -193,14 +242,14 @@ public void run() {

protected abstract void startScan();

@SuppressLint("NewApi")
@MainThread
protected void scanLeDevice(final Boolean enable) {
try {
mScanCyclerStarted = true;
if (getBluetoothAdapter() == null) {
LogManager.e(TAG, "No Bluetooth adapter. beaconService cannot scan.");
}
if (enable) {
if (mScanningEnabled && enable) {
if (deferScanIfNeeded()) {
return;
}
Expand Down Expand Up @@ -253,23 +302,28 @@ protected void scanLeDevice(final Boolean enable) {
mScanCyclerStarted = false;
stopScan();
mLastScanCycleEndTime = SystemClock.elapsedRealtime();
// Clear any queued schedule tasks as we're done scanning
mScanHandler.removeCallbacksAndMessages(null);
finishScanCycle();
}
}
catch (SecurityException e) {
LogManager.w(TAG, "SecurityException working accessing bluetooth.");
}
}

@MainThread
protected void scheduleScanCycleStop() {
// Stops scanning after a pre-defined scan period.
long millisecondsUntilStop = mScanCycleStopTime - SystemClock.elapsedRealtime();
if (millisecondsUntilStop > 0) {
if (mScanningEnabled && millisecondsUntilStop > 0) {
LogManager.d(TAG, "Waiting to stop scan cycle for another %s milliseconds",
millisecondsUntilStop);
if (mBackgroundFlag) {
setWakeUpAlarm();
}
mHandler.postDelayed(new Runnable() {
@MainThread
@Override
public void run() {
scheduleScanCycleStop();
Expand All @@ -282,6 +336,7 @@ public void run() {

protected abstract void finishScan();

@MainThread
private void finishScanCycle() {
LogManager.d(TAG, "Done with scan cycle");
try {
Expand All @@ -298,7 +353,7 @@ private void finishScanCycle() {
// so it is best avoided. If we know the device has detected to distinct
// packets in the same cycle, we will not restart scanning and just keep it
// going.
if (!getDistinctPacketsDetectedPerScan() || mBetweenScanPeriod != 0) {
if (!mDistinctPacketsDetectedPerScan || mBetweenScanPeriod != 0) {
long now = SystemClock.elapsedRealtime();
if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.N &&
mBetweenScanPeriod+mScanPeriod < ANDROID_N_MIN_SCAN_CYCLE_MILLIS &&
Expand Down
Loading