From 368aef591f69f499e4a21071b2e7d8790da7cfa9 Mon Sep 17 00:00:00 2001
From: Todd Herbert <herbert.todd@gmail.com>
Date: Tue, 11 Jun 2024 03:05:05 +1200
Subject: [PATCH] Refactor GPSPowerState enum Identifies a case where the GPS
 hardware is awake, but an update is not yet desired

---
 src/gps/GPS.cpp | 36 +++++++++++++++++++++++++-----------
 src/gps/GPS.h   |  9 +++++----
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/src/gps/GPS.cpp b/src/gps/GPS.cpp
index 17088910a..28af95e8b 100644
--- a/src/gps/GPS.cpp
+++ b/src/gps/GPS.cpp
@@ -28,6 +28,12 @@
 #define GPS_STANDBY_THRESHOLD_MINUTES 15
 #endif
 
+// How many seconds of sleep make it worthwhile for the GPS to use powered-on standby
+// Shorter than this, and we'll just wait instead
+#ifndef GPS_RESTING_THRESHOLD_SECONDS
+#define GPS_RESTING_THRESHOLD_SECONDS 10
+#endif
+
 #if defined(NRF52840_XXAA) || defined(NRF52833_XXAA) || defined(ARCH_ESP32) || defined(ARCH_PORTDUINO)
 HardwareSerial *GPS::_serial_gps = &Serial1;
 #else
@@ -776,14 +782,20 @@ void GPS::setGPSPower(bool on, bool standbyOnly, uint32_t sleepTime)
 {
     // Record the current powerState
     if (on)
-        powerState = GPS_AWAKE;
-    else if (!on && standbyOnly)
+        powerState = GPS_ACTIVE;
+    else if (sleepTime <= GPS_RESTING_THRESHOLD_SECONDS * 1000UL && sleepTime > 0) // Note: sleepTime==0 if from GPS::disable()
+        powerState = GPS_RESTING;
+    else if (standbyOnly)
         powerState = GPS_STANDBY;
     else
         powerState = GPS_OFF;
 
     LOG_DEBUG("GPS::powerState=%d\n", powerState);
 
+    // If the next update is due *really soon*, don't actually power off or enter standby. Just wait it out.
+    if (!on && powerState == GPS_RESTING)
+        return;
+
     if (on) {
         clearBuffer(); // drop any old data waiting in the buffer before re-enabling
         if (en_gpio)
@@ -880,14 +892,14 @@ void GPS::setConnected()
 void GPS::setAwake(bool wantAwake)
 {
 
-    // If user has disabled GPS, make sure it is off, not just in standby
+    // If user has disabled GPS, make sure it is off, not just in standby or "resting"
     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)) {
+    if ((wantAwake && powerState != GPS_ACTIVE) || (!wantAwake && powerState == GPS_ACTIVE)) {
         LOG_DEBUG("WANT GPS=%d\n", wantAwake);
 
         // Calculate how long it takes to get a GPS lock
@@ -910,8 +922,10 @@ void GPS::setAwake(bool wantAwake)
             return;
         }
 
-        // 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
+        // If waking relatively frequently: don't power off. Would use more energy trying to reacquire lock each time
+        // We'll either use a "powered-on" standby, or just wait it out, depending on how soon the next update is due
+        // Will decide which inside setGPSPower method
+        else {
 #ifdef GPS_UC6580
             setGPSPower(wantAwake, false, getSleepTime() - averageLockTime);
 #else
@@ -1033,14 +1047,14 @@ int32_t GPS::runOnce()
     uint32_t timeAsleep = now - lastSleepStartMsec;
 
     auto sleepTime = getSleepTime();
-    if (powerState != GPS_AWAKE && (sleepTime != UINT32_MAX) &&
+    if (powerState != GPS_ACTIVE && (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 (powerState == GPS_AWAKE) {
+    if (powerState == GPS_ACTIVE) {
         // 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);
@@ -1086,7 +1100,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 (powerState == GPS_AWAKE) ? GPS_THREAD_INTERVAL : 5000;
+    return (powerState == GPS_ACTIVE) ? GPS_THREAD_INTERVAL : 5000;
 }
 
 // clear the GPS rx buffer as quickly as possible
@@ -1617,9 +1631,9 @@ bool GPS::whileIdle()
 {
     unsigned int charsInBuf = 0;
     bool isValid = false;
-    if (powerState != GPS_AWAKE) {
+    if (powerState != GPS_ACTIVE) {
         clearBuffer();
-        return (powerState == GPS_AWAKE);
+        return (powerState == GPS_ACTIVE);
     }
 #ifdef SERIAL_BUFFER_SIZE
     if (_serial_gps->available() >= SERIAL_BUFFER_SIZE - 1) {
diff --git a/src/gps/GPS.h b/src/gps/GPS.h
index e9ec111a7..da74e4151 100644
--- a/src/gps/GPS.h
+++ b/src/gps/GPS.h
@@ -39,9 +39,10 @@ typedef enum {
 } GPS_RESPONSE;
 
 enum GPSPowerState : uint8_t {
-    GPS_OFF = 0,
-    GPS_AWAKE = 1,
-    GPS_STANDBY = 2,
+    GPS_OFF = 0,     // Physically powered off
+    GPS_ACTIVE = 1,  // Awake and want a position
+    GPS_STANDBY = 2, // Physically powered on, but soft-sleeping
+    GPS_RESTING = 3, // Awake, but not wanting another position yet
 };
 
 // Generate a string representation of DOP
@@ -93,7 +94,7 @@ 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
+    GPSPowerState powerState = GPS_OFF; // GPS_ACTIVE if we want a location right now
 
     uint8_t numSatellites = 0;