Triple-press not disabling GPS (#4041)

* Replace (bool) isAwake with an enum, to track standby states

* Tidy-up, extra logging

* Rename enum values

* Reorder GPSPowerState enum
Possibly more intuitive when reading logs

* Avoid lego comments
de22c57298 (r1627334779)

src/gps/GPS.cpp

@@ -21,6 +21,13 @@
 #define GPS_RESET_MODE HIGH
 #endif
 
+// How many minutes of sleep make it worthwhile to power-off the GPS
+// Shorter than this, and GPS will only enter standby
+// Affected by lock-time, and config.position.gps_update_interval
+#ifndef GPS_STANDBY_THRESHOLD_MINUTES
+#define GPS_STANDBY_THRESHOLD_MINUTES 15
+#endif
+
 #if defined(NRF52840_XXAA) || defined(NRF52833_XXAA) || defined(ARCH_ESP32) || defined(ARCH_PORTDUINO)
 HardwareSerial *GPS::_serial_gps = &Serial1;
 #else
@@ -767,7 +774,16 @@ GPS::~GPS()
 
 void GPS::setGPSPower(bool on, bool standbyOnly, uint32_t sleepTime)
 {
-    LOG_INFO("Setting GPS power=%d\n", on);
+    // Record the current powerState
+    if (on)
+        powerState = GPS_AWAKE;
+    else if (!on && standbyOnly)
+        powerState = GPS_STANDBY;
+    else
+        powerState = GPS_OFF;
+
+    LOG_DEBUG("GPS::powerState=%d\n", powerState);
+
     if (on) {
         clearBuffer(); // drop any old data waiting in the buffer before re-enabling
         if (en_gpio)
@@ -861,17 +877,21 @@ void GPS::setConnected()
  *
  * calls sleep/wake
  */
-void GPS::setAwake(bool on)
+void GPS::setAwake(bool wantAwake)
 {
-    if (isAwake != on) {
-        LOG_DEBUG("WANT GPS=%d\n", on);
-        isAwake = on;
-        if (!enabled) { // short circuit if the user has disabled GPS
-            setGPSPower(false, false, 0);
-            return;
-        }
 
-        if (on) {
+    // If user has disabled GPS, make sure it is off, not just in standby
+    if (!wantAwake && !enabled && powerState != GPS_OFF) {
+        setGPSPower(false, false, 0);
+        return;
+    }
+
+    // If GPS power state needs to change
+    if ((wantAwake && powerState != GPS_AWAKE) || (!wantAwake && powerState == GPS_AWAKE)) {
+        LOG_DEBUG("WANT GPS=%d\n", wantAwake);
+
+        // Calculate how long it takes to get a GPS lock
+        if (wantAwake) {
             lastWakeStartMsec = millis();
         } else {
             lastSleepStartMsec = millis();
@@ -883,23 +903,31 @@ void GPS::setAwake(bool on)
             GPSCycles++;
             LOG_DEBUG("GPS Lock took %d, average %d\n", (lastSleepStartMsec - lastWakeStartMsec) / 1000, averageLockTime / 1000);
         }
-        if ((int32_t)getSleepTime() - averageLockTime >
-            15 * 60 * 1000) { // 15 minutes is probably long enough to make a complete poweroff worth it.
-            setGPSPower(on, false, getSleepTime() - averageLockTime);
+
+        // If long interval between updates: power off between updates
+        if ((int32_t)getSleepTime() - averageLockTime > GPS_STANDBY_THRESHOLD_MINUTES * MS_IN_MINUTE) {
+            setGPSPower(wantAwake, false, getSleepTime() - averageLockTime);
             return;
-        } else if ((int32_t)getSleepTime() - averageLockTime > 10000) { // 10 seconds is enough for standby
+        }
+
+        // If waking frequently: standby only. Would use more power trying to reacquire lock each time
+        else if ((int32_t)getSleepTime() - averageLockTime > 10000) { // 10 seconds is enough for standby
 #ifdef GPS_UC6580
-            setGPSPower(on, false, getSleepTime() - averageLockTime);
+            setGPSPower(wantAwake, false, getSleepTime() - averageLockTime);
 #else
-            setGPSPower(on, true, getSleepTime() - averageLockTime);
+            setGPSPower(wantAwake, true, getSleepTime() - averageLockTime);
 #endif
             return;
         }
+
+        // Gradually recover from an abnormally long "time to get lock"
         if (averageLockTime > 20000) {
             averageLockTime -= 1000; // eventually want to sleep again.
         }
-        if (on)
-            setGPSPower(true, true, 0); // make sure we don't have a fallthrough where GPS is stuck off
+
+        // Make sure we don't have a fallthrough where GPS is stuck off
+        if (wantAwake)
+            setGPSPower(true, true, 0);
     }
 }
 
@@ -1005,14 +1033,14 @@ int32_t GPS::runOnce()
     uint32_t timeAsleep = now - lastSleepStartMsec;
 
     auto sleepTime = getSleepTime();
-    if (!isAwake && (sleepTime != UINT32_MAX) &&
+    if (powerState != GPS_AWAKE && (sleepTime != UINT32_MAX) &&
         ((timeAsleep > sleepTime) || (isInPowersave && timeAsleep > (sleepTime - averageLockTime)))) {
         // We now want to be awake - so wake up the GPS
         setAwake(true);
     }
 
     // While we are awake
-    if (isAwake) {
+    if (powerState == GPS_AWAKE) {
         // LOG_DEBUG("looking for location\n");
         // If we've already set time from the GPS, no need to ask the GPS
         bool gotTime = (getRTCQuality() >= RTCQualityGPS);
@@ -1058,7 +1086,7 @@ int32_t GPS::runOnce()
 
     // 9600bps is approx 1 byte per msec, so considering our buffer size we never need to wake more often than 200ms
     // if not awake we can run super infrquently (once every 5 secs?) to see if we need to wake.
-    return isAwake ? GPS_THREAD_INTERVAL : 5000;
+    return (powerState == GPS_AWAKE) ? GPS_THREAD_INTERVAL : 5000;
 }
 
 // clear the GPS rx buffer as quickly as possible
@@ -1589,9 +1617,9 @@ bool GPS::whileIdle()
 {
     unsigned int charsInBuf = 0;
     bool isValid = false;
-    if (!isAwake) {
+    if (powerState != GPS_AWAKE) {
         clearBuffer();
-        return isAwake;
+        return (powerState == GPS_AWAKE);
     }
 #ifdef SERIAL_BUFFER_SIZE
     if (_serial_gps->available() >= SERIAL_BUFFER_SIZE - 1) {

src/gps/GPS.h

@@ -38,6 +38,12 @@ typedef enum {
     GNSS_RESPONSE_OK,
 } GPS_RESPONSE;
 
+enum GPSPowerState : uint8_t {
+    GPS_OFF = 0,
+    GPS_AWAKE = 1,
+    GPS_STANDBY = 2,
+};
+
 // Generate a string representation of DOP
 const char *getDOPString(uint32_t dop);
 
@@ -78,8 +84,6 @@ class GPS : private concurrency::OSThread
      */
     bool hasValidLocation = false; // default to false, until we complete our first read
 
-    bool isAwake = false; // true if we want a location right now
-
     bool isInPowersave = false;
 
     bool shouldPublish = false; // If we've changed GPS state, this will force a publish the next loop()
@@ -89,6 +93,8 @@ class GPS : private concurrency::OSThread
     bool GPSInitFinished = false; // Init thread finished?
     bool GPSInitStarted = false;  // Init thread finished?
 
+    GPSPowerState powerState = GPS_OFF; // GPS_AWAKE if we want a location right now
+
     uint8_t numSatellites = 0;
 
     CallbackObserver<GPS, void *> notifyDeepSleepObserver = CallbackObserver<GPS, void *>(this, &GPS::prepareDeepSleep);