Add thread annotations for CycledLeScanCallback

Through testing it's been confirmed that the bluetooth scan callbacks are always run on the main thread. This chases that through the scanners and our callbacks so that this is properly documented / annotated. During this process the unused `AsyncTask` methods were removed as we don not use them. Additionally, the `mDistinctPacketsDetectedPerScan` flag has been declared `volatile` to ensure reads always see the most recently written value without having to rely on a heavy weight `synchronized` block. As this is a flag we only perform simple reads / writes. While we do perform a multi-step operation around reading/writing this value the work performed is too heavy weight and long to wrap it in a `synchronized` block. Using `volatile` gives multipel concurrent background threads a better chance of aborting this logic if another has changed the value. However, the downside of not having complete synchronization is that a background thread will try to check a few more packets and then re-set the flag to `true`.
AltBeacon · May 26, 2017 · 0f5e823 · 0f5e823
1 parent 0ab8bee
commit 0f5e823
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 18 deletions.
diff --git a/src/main/java/org/altbeacon/beacon/service/BeaconService.java b/src/main/java/org/altbeacon/beacon/service/BeaconService.java
@@ -42,6 +42,7 @@
 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;
@@ -391,6 +392,7 @@ public void setScanPeriods(long scanPeriod, long betweenScanPeriod, boolean back
     }
 
     protected final CycledLeScanCallback mCycledLeScanCallback = new CycledLeScanCallback() {
+        @MainThread
         @TargetApi(Build.VERSION_CODES.HONEYCOMB)
         @Override
         public void onLeScan(BluetoothDevice device, int rssi, byte[] scanRecord) {
@@ -405,6 +407,7 @@ public void onLeScan(BluetoothDevice device, int rssi, byte[] scanRecord) {
             }
         }
 
+        @MainThread
         @Override
         public void onCycleEnd() {
             mDistinctPacketDetector.clearDetections();
@@ -418,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 {
@@ -430,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 {
@@ -452,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);
         }
@@ -520,6 +537,7 @@ public ScanProcessor(NonBeaconLeScanCallback nonBeaconLeScanCallback) {
             mNonBeaconLeScanCallback = nonBeaconLeScanCallback;
         }
 
+        @WorkerThread
         @Override
         protected Void doInBackground(ScanData... params) {
             ScanData scanData = params[0];
@@ -554,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) {

diff --git a/src/main/java/org/altbeacon/beacon/service/scanner/CycledLeScanCallback.java b/src/main/java/org/altbeacon/beacon/service/scanner/CycledLeScanCallback.java
@@ -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();
 }
diff --git a/src/main/java/org/altbeacon/beacon/service/scanner/CycledLeScanner.java b/src/main/java/org/altbeacon/beacon/service/scanner/CycledLeScanner.java
@@ -14,6 +14,7 @@
 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;
@@ -22,6 +23,7 @@
 import org.altbeacon.beacon.logging.LogManager;
 import org.altbeacon.beacon.startup.StartupBroadcastReceiver;
 import org.altbeacon.bluetooth.BluetoothCrashResolver;
+
 import java.util.Date;
 
 @TargetApi(18)
@@ -75,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) {
@@ -188,10 +205,12 @@ public void stop() {
         }
     }
 
+    @AnyThread
     public boolean getDistinctPacketsDetectedPerScan() {
         return mDistinctPacketsDetectedPerScan;
     }
 
+    @AnyThread
     public void setDistinctPacketsDetectedPerScan(boolean detected) {
         mDistinctPacketsDetectedPerScan = detected;
     }
@@ -334,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 &&

diff --git a/src/main/java/org/altbeacon/beacon/service/scanner/CycledLeScannerForLollipop.java b/src/main/java/org/altbeacon/beacon/service/scanner/CycledLeScannerForLollipop.java
@@ -280,7 +280,7 @@ private BluetoothLeScanner getScanner() {
     private ScanCallback getNewLeScanCallback() {
         if (leScanCallback == null) {
             leScanCallback = new ScanCallback() {
-
+                @MainThread
                 @Override
                 public void onScanResult(int callbackType, ScanResult scanResult) {
                     if (LogManager.isVerboseLoggingEnabled()) {
@@ -299,6 +299,7 @@ public void onScanResult(int callbackType, ScanResult scanResult) {
                     }
                 }
 
+                @MainThread
                 @Override
                 public void onBatchScanResults(List<ScanResult> results) {
                     LogManager.d(TAG, "got batch records");
@@ -311,6 +312,7 @@ public void onBatchScanResults(List<ScanResult> results) {
                     }
                 }
 
+                @MainThread
                 @Override
                 public void onScanFailed(int errorCode) {
                     switch (errorCode) {

diff --git a/src/main/java/org/altbeacon/beacon/service/scanner/NonBeaconLeScanCallback.java b/src/main/java/org/altbeacon/beacon/service/scanner/NonBeaconLeScanCallback.java
@@ -1,6 +1,7 @@
 package org.altbeacon.beacon.service.scanner;
 
 import android.bluetooth.BluetoothDevice;
+import android.support.annotation.WorkerThread;
 
 /**
  * Allows an implementation to see non-Beacon BLE devices as they are scanned.
@@ -23,6 +24,7 @@
  *  }
  * </code></pre>
  */
+@WorkerThread
 public interface NonBeaconLeScanCallback {
     /**
      * NOTE: This method is NOT called on the main UI thread.