Rewrote GpsStatus check and added tests.

src/androidTest/java/de/dennisguse/opentracks/services/handlers/GpsStatusTest.java

@@ -0,0 +1,146 @@
+package de.dennisguse.opentracks.services.handlers;
+
+import static org.junit.Assert.assertEquals;
+import static de.dennisguse.opentracks.services.handlers.GpsStatusValue.GPS_DISABLED;
+import static de.dennisguse.opentracks.services.handlers.GpsStatusValue.GPS_ENABLED;
+import static de.dennisguse.opentracks.services.handlers.GpsStatusValue.GPS_NONE;
+import static de.dennisguse.opentracks.services.handlers.GpsStatusValue.GPS_SIGNAL_BAD;
+import static de.dennisguse.opentracks.services.handlers.GpsStatusValue.GPS_SIGNAL_FIX;
+import static de.dennisguse.opentracks.services.handlers.GpsStatusValue.GPS_SIGNAL_LOST;
+
+import android.content.Context;
+import android.location.Location;
+import android.os.Handler;
+import android.os.HandlerThread;
+import android.os.Looper;
+
+import androidx.test.core.app.ApplicationProvider;
+import androidx.test.ext.junit.runners.AndroidJUnit4;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import java.time.Duration;
+import java.time.Instant;
+import java.util.ArrayList;
+import java.util.List;
+
+import de.dennisguse.opentracks.data.models.Distance;
+import de.dennisguse.opentracks.data.models.TrackPoint;
+
+@RunWith(AndroidJUnit4.class)
+public class GpsStatusTest {
+
+    private final Context context = ApplicationProvider.getApplicationContext();
+
+    @BeforeClass
+    public static void preSetUp() {
+        // Prepare looper for Android's message queue
+        if (Looper.myLooper() == null) Looper.prepare();
+    }
+
+    private final static Location badFix = new Location("gps");
+    private final static Location ok = new Location("gps");
+
+    static {
+        badFix.setAccuracy(50);
+
+        ok.setAccuracy(10);
+    }
+
+    @Test
+    public void testStartDisabledEnabledStop() {
+        ArrayList<GpsStatusValue> statusList = new ArrayList<>();
+
+        // given
+        GpsStatusManager subject = new GpsStatusManager(context, statusList::add, new Handler());
+
+        // when / then
+        subject.start();
+        assertEquals(List.of(GPS_ENABLED), statusList);
+
+        subject.onGpsDisabled();
+        assertEquals(List.of(GPS_ENABLED, GPS_DISABLED), statusList);
+
+        subject.onGpsEnabled();
+        assertEquals(List.of(GPS_ENABLED, GPS_DISABLED, GPS_ENABLED), statusList);
+
+        subject.stop();
+        assertEquals(List.of(GPS_ENABLED, GPS_DISABLED, GPS_ENABLED, GPS_NONE), statusList);
+    }
+
+    @Test
+    public void testStartBadfixOk() {
+        ArrayList<GpsStatusValue> statusList = new ArrayList<>();
+
+        // given
+        GpsStatusManager subject = new GpsStatusManager(context, statusList::add, new Handler());
+        subject.onRecordingDistanceChanged(Distance.of(10));
+
+        // when / then
+        subject.start();
+        subject.onNewTrackPoint(new TrackPoint(badFix, Instant.now()));
+        assertEquals(List.of(GPS_ENABLED, GPS_SIGNAL_BAD), statusList);
+
+        subject.onNewTrackPoint(new TrackPoint(ok, Instant.now()));
+        assertEquals(List.of(GPS_ENABLED, GPS_SIGNAL_BAD, GPS_SIGNAL_FIX), statusList);
+
+        subject.onNewTrackPoint(new TrackPoint(ok, Instant.now()));
+        assertEquals(List.of(GPS_ENABLED, GPS_SIGNAL_BAD, GPS_SIGNAL_FIX), statusList);
+
+        subject.onNewTrackPoint(new TrackPoint(badFix, Instant.now()));
+        assertEquals(List.of(GPS_ENABLED, GPS_SIGNAL_BAD, GPS_SIGNAL_FIX, GPS_SIGNAL_BAD), statusList);
+    }
+
+    @Test
+    public void testStartSignalLost() {
+        ArrayList<GpsStatusValue> statusList = new ArrayList<>();
+
+        // given
+        GpsStatusManager subject = new GpsStatusManager(context, statusList::add, new Handler());
+        subject.onRecordingDistanceChanged(Distance.of(10));
+        subject.onMinRecordingIntervalChanged(GpsStatusManager.SIGNAL_LOST_THRESHOLD.multipliedBy(-1));
+
+        // when / then
+        subject.start();
+        subject.onNewTrackPoint(new TrackPoint(ok, Instant.now().minusMillis(1000)));
+        assertEquals(List.of(GPS_ENABLED, GPS_SIGNAL_FIX), statusList);
+
+        subject.determineGpsStatusByTime(Instant.now());
+        assertEquals(List.of(GPS_ENABLED, GPS_SIGNAL_FIX, GPS_SIGNAL_LOST), statusList);
+
+        subject.onNewTrackPoint(new TrackPoint(ok, Instant.now()));
+        assertEquals(List.of(GPS_ENABLED, GPS_SIGNAL_FIX, GPS_SIGNAL_LOST, GPS_SIGNAL_FIX), statusList);
+    }
+
+    @Test
+    public void testStartSignalLostByTimer() throws InterruptedException {
+        ArrayList<GpsStatusValue> statusList = new ArrayList<>();
+
+        final HandlerThread handlerThread = new HandlerThread("solution!");
+        handlerThread.start();
+
+        // given
+        GpsStatusManager subject = new GpsStatusManager(context, statusList::add, new Handler(handlerThread.getLooper()));
+        subject.onRecordingDistanceChanged(Distance.of(10));
+        subject.onMinRecordingIntervalChanged(GpsStatusManager.SIGNAL_LOST_THRESHOLD.multipliedBy(-1).plus(Duration.ofMillis(10)));
+
+        // when / then
+        subject.start();
+        Thread.sleep(100);
+        assertEquals(List.of(GPS_ENABLED), statusList);
+
+        subject.onNewTrackPoint(new TrackPoint(ok, Instant.now()));
+        assertEquals(List.of(GPS_ENABLED, GPS_SIGNAL_FIX), statusList);
+
+        Thread.sleep(100);
+        assertEquals(List.of(GPS_ENABLED, GPS_SIGNAL_FIX, GPS_SIGNAL_LOST), statusList);
+
+        subject.onNewTrackPoint(new TrackPoint(badFix, Instant.now()));
+        assertEquals(List.of(GPS_ENABLED, GPS_SIGNAL_FIX, GPS_SIGNAL_LOST, GPS_SIGNAL_BAD), statusList);
+
+        Thread.sleep(100);
+        assertEquals(List.of(GPS_ENABLED, GPS_SIGNAL_FIX, GPS_SIGNAL_LOST, GPS_SIGNAL_BAD, GPS_SIGNAL_LOST), statusList);
+    }
+}

src/main/java/de/dennisguse/opentracks/services/handlers/GPSManager.java

@@ -15,7 +15,6 @@ import androidx.core.location.LocationManagerCompat;
 import androidx.core.location.LocationRequestCompat;
 
 import java.time.Duration;
-import java.time.Instant;
 
 import de.dennisguse.opentracks.R;
 import de.dennisguse.opentracks.data.models.Distance;
@@ -26,7 +25,7 @@ import de.dennisguse.opentracks.util.LocationUtils;
 import de.dennisguse.opentracks.util.PermissionRequester;
 
 @VisibleForTesting(otherwise = VisibleForTesting.PACKAGE_PRIVATE)
-public class GPSManager implements SensorConnector, LocationListenerCompat, GpsStatus.GpsStatusListener, SharedPreferences.OnSharedPreferenceChangeListener {
+public class GPSManager implements SensorConnector, LocationListenerCompat, GpsStatusManager.GpsStatusListener, SharedPreferences.OnSharedPreferenceChangeListener {
 
     private final String TAG = GPSManager.class.getSimpleName();
 
@@ -37,7 +36,7 @@ public class GPSManager implements SensorConnector, LocationListenerCompat, GpsS
     private Handler handler;
 
     private LocationManager locationManager;
-    private GpsStatus gpsStatus;
+    private GpsStatusManager gpsStatusManager;
     private Duration gpsInterval;
     private Distance thresholdHorizontalAccuracy;
 
@@ -50,10 +49,10 @@ public class GPSManager implements SensorConnector, LocationListenerCompat, GpsS
         this.handler = handler;
 
         PreferencesUtils.registerOnSharedPreferenceChangeListener(this);
-        gpsStatus = new GpsStatus(context, this);
+        gpsStatusManager = new GpsStatusManager(context, this, handler);
         locationManager = (LocationManager) context.getSystemService(Context.LOCATION_SERVICE);
         registerLocationListener();
-        gpsStatus.start();
+        gpsStatusManager.start();
     }
 
     private boolean isStarted() {
@@ -72,9 +71,9 @@ public class GPSManager implements SensorConnector, LocationListenerCompat, GpsS
             handler = null;
         }
 
-        if (gpsStatus != null) {
+        if (gpsStatusManager != null) {
-            gpsStatus.stop();
+            gpsStatusManager.stop();
-            gpsStatus = null;
+            gpsStatusManager = null;
         }
         PreferencesUtils.unregisterOnSharedPreferenceChangeListener(this);
     }
@@ -88,8 +87,8 @@ public class GPSManager implements SensorConnector, LocationListenerCompat, GpsS
 
             gpsInterval = PreferencesUtils.getMinRecordingInterval();
 
-            if (gpsStatus != null) {
+            if (gpsStatusManager != null) {
-                gpsStatus.onMinRecordingIntervalChanged(gpsInterval);
+                gpsStatusManager.onMinRecordingIntervalChanged(gpsInterval);
             }
         }
         if (PreferencesUtils.isKey(R.string.recording_gps_accuracy_key, key)) {
@@ -98,9 +97,9 @@ public class GPSManager implements SensorConnector, LocationListenerCompat, GpsS
         if (PreferencesUtils.isKey(R.string.recording_distance_interval_key, key)) {
             registerListener = true;
 
-            if (gpsStatus != null) {
+            if (gpsStatusManager != null) {
                 Distance gpsMinDistance = PreferencesUtils.getRecordingDistanceInterval();
-                gpsStatus.onRecordingDistanceChanged(gpsMinDistance);
+                gpsStatusManager.onRecordingDistanceChanged(gpsMinDistance);
             }
         }
 
@@ -121,10 +120,10 @@ public class GPSManager implements SensorConnector, LocationListenerCompat, GpsS
             return;
         }
 
-        if (gpsStatus != null) {
+        if (gpsStatusManager != null) {
             // Send each update to the status; please note that this TrackPoint is not stored.
-            TrackPoint trackPoint = new TrackPoint(location, Instant.ofEpochMilli(location.getTime()));
+            TrackPoint trackPoint = new TrackPoint(location, trackPointCreator.createNow());
-            gpsStatus.onLocationChanged(trackPoint);
+            gpsStatusManager.onNewTrackPoint(trackPoint);
         }
 
         if (!LocationUtils.isValidLocation(location)) {
@@ -146,15 +145,15 @@ public class GPSManager implements SensorConnector, LocationListenerCompat, GpsS
 
     @Override
     public void onProviderEnabled(@NonNull String provider) {
-        if (gpsStatus != null) {
+        if (gpsStatusManager != null) {
-            gpsStatus.onGpsEnabled();
+            gpsStatusManager.onGpsEnabled();
         }
     }
 
     @Override
     public void onProviderDisabled(@NonNull String provider) {
-        if (gpsStatus != null) {
+        if (gpsStatusManager != null) {
-            gpsStatus.onGpsDisabled();
+            gpsStatusManager.onGpsDisabled();
         }
     }
 

src/main/java/de/dennisguse/opentracks/services/handlers/GpsStatusManager.java

@@ -6,6 +6,7 @@ import android.os.Handler;
 
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
+import androidx.annotation.VisibleForTesting;
 
 import java.time.Duration;
 import java.time.Instant;
@@ -17,14 +18,15 @@ import de.dennisguse.opentracks.settings.PreferencesUtils;
 /**
  * This class handle GPS status according to received locations` and some thresholds.
  */
-class GpsStatus {
+class GpsStatusManager {
 
-    private static final String TAG = GpsStatus.class.getSimpleName();
+    private static final String TAG = GpsStatusManager.class.getSimpleName();
 
     // The duration that GpsStatus waits from minimal interval to consider GPS lost.
-    private static final Duration SIGNAL_LOST_THRESHOLD = Duration.ofSeconds(30);
+    @VisibleForTesting
+    public static final Duration SIGNAL_LOST_THRESHOLD = Duration.ofSeconds(30);
 
-    private Distance thresholdHorizontalAccuracy;
+    private Distance horizontalAccuracyThreshold;
     // Threshold for time without points.
     private Duration signalLostThreshold;
 
@@ -35,40 +37,23 @@ class GpsStatus {
     @Nullable
     private TrackPoint lastTrackPoint = null;
 
-    @Nullable
-    // The last valid (not null) location. Null value means that there have not been any location yet.
-    private TrackPoint lastValidTrackPoint = null;
-
     // Flag to prevent GpsStatus checks two or more locations at the same time.
     private boolean checking = false;
 
-    private class GpsStatusRunner implements Runnable {
+    private Handler handler;
-        private boolean stopped = false;
 
-        @Override
+    public final Runnable gpsStatusTimer = () -> {
-        public void run() {
+        determineGpsStatusByTime(Instant.now()); //TODO Get now via TrackPointCreator?
-            if (gpsStatus != null && !stopped) {
+    };
-                onLocationChanged(null);
-                gpsStatusHandler.postDelayed(gpsStatusRunner, getIntervalThreshold().toMillis());
-            }
-        }
 
-        public void stop() {
-            stopped = true;
-        }
-    }
 
-    private final Handler gpsStatusHandler;
+    public GpsStatusManager(Context context, GpsStatusListener client, Handler handler) {
-    private GpsStatusRunner gpsStatusRunner = null;
-
-    public GpsStatus(Context context, GpsStatusListener client) {
         this.client = client;
         this.context = context;
+        this.handler = handler;
 
         onRecordingDistanceChanged(PreferencesUtils.getRecordingDistanceInterval());
         onMinRecordingIntervalChanged(PreferencesUtils.getMinRecordingInterval());
-
-        gpsStatusHandler = new Handler();
     }
 
     public void start() {
@@ -81,10 +66,7 @@ class GpsStatus {
     public void stop() {
         client.onGpsStatusChanged(GpsStatusValue.GPS_NONE);
         client = null;
-        if (gpsStatusRunner != null) {
+        handler = null;
-            gpsStatusRunner.stop();
-            gpsStatusRunner = null;
-        }
     }
 
     /**
@@ -93,11 +75,11 @@ class GpsStatus {
      * @param value New preference value to signalBadThreshold.
      */
     public void onRecordingDistanceChanged(@NonNull Distance value) {
-        thresholdHorizontalAccuracy = value;
+        horizontalAccuracyThreshold = value;
     }
 
     public void onMinRecordingIntervalChanged(Duration value) {
-        signalLostThreshold = SIGNAL_LOST_THRESHOLD.plus(value);
+        signalLostThreshold = SIGNAL_LOST_THRESHOLD.plus(value); //TODO Reschedule gpsStatusTimer?
     }
 
     /**
@@ -105,22 +87,17 @@ class GpsStatus {
      * Receive new trackPoint and calculate the new status if needed.
      * It look for GPS changes in lastLocation if it's not null. If it's null then look for in lastValidLocation if any.
      */
-    public void onLocationChanged(final TrackPoint trackPoint) {
+    //TODO Remove checking; should be synchronized if this is a problem.
+    public void onNewTrackPoint(@NonNull final TrackPoint trackPoint) {
         if (checking) {
             return;
         }
 
         checking = true;
-        if (lastTrackPoint != null) {
-            checkStatusFromLastLocation();
-        } else if (lastValidTrackPoint != null) {
-            checkStatusFromLastValidLocation();
-        }
-
-        if (trackPoint != null) {
-            lastValidTrackPoint = trackPoint;
-        }
         lastTrackPoint = trackPoint;
+
+        determineGpsStatusOnTrackpoint(trackPoint);
+
         checking = false;
     }
 
@@ -130,40 +107,35 @@ class GpsStatus {
      * If there is any change then it does the change.
      * Also, it'll run the runnable if signal is bad or stop it if the signal is lost.
      */
-    private void checkStatusFromLastLocation() {
+    //TODO use MonotonicClock instead of Instant.now()
-        if (Duration.between(lastTrackPoint.getTime(), Instant.now()).compareTo(signalLostThreshold) > 0 && gpsStatus != GpsStatusValue.GPS_SIGNAL_LOST) {
+    @VisibleForTesting
-            // Too much time without receiving signal -> signal lost.
+    void determineGpsStatusOnTrackpoint(@NonNull TrackPoint lastTrackPoint) {
-            setGpsStatus(GpsStatusValue.GPS_SIGNAL_LOST);
+        if (lastTrackPoint.fulfillsAccuracy(horizontalAccuracyThreshold)) {
-            stopStatusRunner();
+            if (gpsStatus != GpsStatusValue.GPS_SIGNAL_FIX) {
-            return;
+                setGpsStatus(GpsStatusValue.GPS_SIGNAL_FIX);
-        }
+                scheduleTimer(); //TODO
-        if (lastTrackPoint.fulfillsAccuracy(thresholdHorizontalAccuracy) && gpsStatus != GpsStatusValue.GPS_SIGNAL_BAD) {
+            }
-            // Too little accuracy -> bad signal.
+        } else {
-            setGpsStatus(GpsStatusValue.GPS_SIGNAL_BAD);
+            // GPS signal is to weak; TODO we might need a time-based threshold here as well (i.e., warn after Duration)
-            startStatusRunner();
+            if (gpsStatus != GpsStatusValue.GPS_SIGNAL_BAD) {
-            return;
+                setGpsStatus(GpsStatusValue.GPS_SIGNAL_BAD);
-        }
+                scheduleTimer();
-        if (lastTrackPoint.fulfillsAccuracy(thresholdHorizontalAccuracy) && gpsStatus != GpsStatusValue.GPS_SIGNAL_FIX) {
+            }
-            // Gps okay.
-            setGpsStatus(GpsStatusValue.GPS_SIGNAL_FIX);
-            startStatusRunner();
-            return;
         }
     }
 
-    /**
+    void determineGpsStatusByTime(Instant now) {
-     * Checks if lastValidLocation has a new GPS status looking up time.
+        if (lastTrackPoint == null) {
-     * It depends on signalLostThreshold.
+            return;
-     * If there is any change then it does the change.
-     */
-    private void checkStatusFromLastValidLocation() {
-        Duration elapsed = Duration.between(lastValidTrackPoint.getTime(), Instant.now());
-        if (signalLostThreshold.minus(elapsed).isNegative()) {
-            // Too much time without locations -> lost signal? (wait signalLostThreshold from last valid location).
-            setGpsStatus(GpsStatusValue.GPS_SIGNAL_LOST);
-            stopStatusRunner();
-            lastValidTrackPoint = null;
         }
+        if (signalLostThreshold.minus(Duration.between(lastTrackPoint.getTime(), now)).isNegative()) {
+            // Too much time without receiving signal -> signal lost.
+            if (gpsStatus != GpsStatusValue.GPS_SIGNAL_LOST) {
+                setGpsStatus(GpsStatusValue.GPS_SIGNAL_LOST);
+            }
+            return;
+        }
+        scheduleTimer();
     }
 
     /**
@@ -177,7 +149,7 @@ class GpsStatus {
         LocationManager locationManager = (LocationManager) context.getSystemService(Context.LOCATION_SERVICE);
         if (locationManager != null && locationManager.isProviderEnabled(LocationManager.GPS_PROVIDER)) {
             setGpsStatus(GpsStatusValue.GPS_ENABLED);
-            startStatusRunner();
+            scheduleTimer();
         } else {
             onGpsDisabled();
         }
@@ -194,33 +166,23 @@ class GpsStatus {
 
         setGpsStatus(GpsStatusValue.GPS_DISABLED);
         lastTrackPoint = null;
-        lastValidTrackPoint = null;
+        stopTimer();
-        stopStatusRunner();
     }
 
     private void setGpsStatus(GpsStatusValue current) {
-        gpsStatus = GpsStatusValue.GPS_DISABLED;
+        gpsStatus = current;
         if (client != null) {
             client.onGpsStatusChanged(current);
         }
     }
 
-    private void startStatusRunner() {
+    private void scheduleTimer() {
-        if (gpsStatusRunner == null) {
+        handler.removeCallbacks(gpsStatusTimer);
-            gpsStatusRunner = new GpsStatusRunner();
+        handler.postDelayed(gpsStatusTimer, signalLostThreshold.toMillis());
-            gpsStatusRunner.run();
-        }
     }
 
-    private void stopStatusRunner() {
+    private void stopTimer() {
-        if (gpsStatusRunner != null) {
+        handler.removeCallbacks(gpsStatusTimer);
-            gpsStatusRunner.stop();
-            gpsStatusRunner = null;
-        }
-    }
-
-    public Duration getIntervalThreshold() {
-        return signalLostThreshold;
     }
 
     public interface GpsStatusListener {