wip gps power fixes #376

.vscode/settings.json

@@ -57,6 +57,7 @@
         "HFSR",
         "Meshtastic",
         "NEMAGPS",
+        "NMEAGPS",
         "RDEF",
         "Ublox",
         "bkpt",

src/PowerFSM.cpp

@@ -91,7 +91,7 @@ static void lsIdle()
 static void lsExit()
 {
     // setGPSPower(true); // restore GPS power
-    gps->startLock();
+    gps->forceWake(true);
 }
 
 static void nbEnter()

src/gps/GPS.cpp

@@ -1,6 +1,7 @@
 
 #include "GPS.h"
 #include "configuration.h"
+#include "sleep.h"
 #include <assert.h>
 #include <time.h>
 
@@ -86,19 +87,99 @@ uint32_t getValidTime()
     return timeSetFromGPS ? getTime() : 0;
 }
 
+bool GPS::setup()
+{
+    notifySleepObserver.observe(&notifySleep);
+
+    return true;
+}
+
 /**
  * Switch the GPS into a mode where we are actively looking for a lock, or alternatively switch GPS into a low power mode
  *
  * calls sleep/wake
  */
-void GPS::setWantLocation(bool on)
+void GPS::setAwake(bool on)
 {
-    if (wantNewLocation != on) {
+    if(!wakeAllowed && on) {
-        wantNewLocation = on;
+        DEBUG_MSG("Inhibiting because !wakeAllowed\n");
+        on = false;
+    }
+
+    if (isAwake != on) {
         DEBUG_MSG("WANT GPS=%d\n", on);
         if (on)
             wake();
         else
             sleep();
+
+        isAwake = on;
     }
 }
+
+void GPS::loop()
+{
+    if (whileIdle()) {
+        // if we have received valid NMEA claim we are connected
+        isConnected = true;
+    }
+
+    // If we are overdue for an update, turn on the GPS and at least publish the current status
+    uint32_t now = millis();
+    bool mustPublishUpdate = false;
+
+    if ((now - lastUpdateMsec) > 30 * 1000 && !isAwake) {
+        // We now want to be awake - so wake up the GPS
+        setAwake(true);
+
+        mustPublishUpdate =
+            true; // Even if we don't have an update this time, we at least want to occasionally publish the current state
+    }
+
+    // While we are awake
+    if (isAwake) {
+        DEBUG_MSG("looking for location\n");
+        bool gotTime = lookForTime();
+        bool gotLoc = lookForLocation();
+
+        if (gotLoc)
+            hasValidLocation = true;
+
+        mustPublishUpdate |= gotLoc;
+
+        // Once we get a location we no longer desperately want an update
+        if (gotLoc) {
+            lastUpdateMsec = now;
+            setAwake(false);
+        }
+    }
+
+    if (mustPublishUpdate) {
+        DEBUG_MSG("publishing GPS lock=%d\n", hasLock());
+
+        // Notify any status instances that are observing us
+        const meshtastic::GPSStatus status =
+            meshtastic::GPSStatus(hasLock(), isConnected, latitude, longitude, altitude, dop, heading, numSatellites);
+        newStatus.notifyObservers(&status);
+    }
+}
+
+void GPS::forceWake(bool on)
+{
+    if (on) {
+        DEBUG_MSG("Looking for GPS lock\n");
+        lastUpdateMsec = 0; // Force an update ASAP
+        wakeAllowed = true;
+    } else {
+        wakeAllowed = false;
+        setAwake(false);
+    }
+}
+
+/// Prepare the GPS for the cpu entering deep or light sleep, expect to be gone for at least 100s of msecs
+int GPS::prepareSleep(void *unused)
+{
+    forceWake(false);
+
+    return 0;
+}

src/gps/GPS.h

@@ -27,11 +27,18 @@ void readFromRTC();
  */
 class GPS
 {
-  protected:
+  private:
+    uint32_t lastUpdateMsec = 0;
+
     bool hasValidLocation = false; // default to false, until we complete our first read
 
-    bool wantNewLocation = false; // true if we want a location right now
+    bool isAwake = false; // true if we want a location right now
 
+    bool wakeAllowed = true; // false if gps must be forced to sleep regardless of what time it is
+
+    CallbackObserver<GPS, void *> notifySleepObserver = CallbackObserver<GPS, void *>(this, &GPS::prepareSleep);
+
+  protected:
   public:
     /** If !NULL we will use this serial port to construct our GPS */
     static HardwareSerial *_serial_gps;
@@ -48,7 +55,7 @@ class GPS
 
     bool isConnected = false; // Do we have a GPS we are talking to
 
-    virtual ~GPS() {}
+    virtual ~GPS() {} // FIXME, we really should unregister our sleep observer
 
     /** We will notify this observable anytime GPS state has changed meaningfully */
     Observable<const meshtastic::GPSStatus *> newStatus;
@@ -56,25 +63,20 @@ class GPS
     /**
      * Returns true if we succeeded
      */
-    virtual bool setup() { return true; }
+    virtual bool setup();
 
-    /// A loop callback for subclasses that need it.  FIXME, instead just block on serial reads
+    virtual void loop();
-    virtual void loop() {}
 
     /// Returns ture if we have acquired GPS lock.
     bool hasLock() const { return hasValidLocation; }
 
-    /**
-     * Switch the GPS into a mode where we are actively looking for a lock, or alternatively switch GPS into a low power mode
-     * 
-     * calls sleep/wake
-     */
-    void setWantLocation(bool on);
-
     /**
      * Restart our lock attempt - try to get and broadcast a GPS reading ASAP
-     * called after the CPU wakes from light-sleep state */
+     * called after the CPU wakes from light-sleep state
-    virtual void startLock() {}
+     *
+     * Or set to false, to disallow any sort of waking
+     * */
+    void forceWake(bool on);
 
   protected:
     /// If possible force the GPS into sleep/low power mode
@@ -82,6 +84,40 @@ protected:
 
     /// wake the GPS into normal operation mode
     virtual void wake() {}
+
+    /** Subclasses should look for serial rx characters here and feed it to their GPS parser
+     *
+     * Return true if we received a valid message from the GPS
+     */
+    virtual bool whileIdle() = 0;
+
+    /**
+     * Perform any processing that should be done only while the GPS is awake and looking for a fix.
+     * Override this method to check for new locations
+     *
+     * @return true if we've acquired a time
+     */
+    virtual bool lookForTime() = 0;
+
+    /**
+     * Perform any processing that should be done only while the GPS is awake and looking for a fix.
+     * Override this method to check for new locations
+     *
+     * @return true if we've acquired a new location
+     */
+    virtual bool lookForLocation() = 0;
+
+  private:
+    /// Prepare the GPS for the cpu entering deep or light sleep, expect to be gone for at least 100s of msecs
+    /// always returns 0 to indicate okay to sleep
+    int prepareSleep(void *unused);
+
+    /**
+     * Switch the GPS into a mode where we are actively looking for a lock, or alternatively switch GPS into a low power mode
+     *
+     * calls sleep/wake
+     */
+    void setAwake(bool on);
 };
 
 extern GPS *gps;

src/gps/NMEAGPS.cpp

@@ -1,7 +1,6 @@
 #include "NMEAGPS.h"
 #include "configuration.h"
 
-
 static int32_t toDegInt(RawDegrees d)
 {
     int32_t degMult = 10000000; // 1e7
@@ -18,35 +17,19 @@ bool NMEAGPS::setup()
     // FIXME - move into shared GPS code
     pinMode(PIN_GPS_PPS, INPUT);
 #endif
+    GPS::setup();
 
     return true;
 }
 
-void NMEAGPS::loop()
+/**
+ * Perform any processing that should be done only while the GPS is awake and looking for a fix.
+ * Override this method to check for new locations
+ *
+ * @return true if we've acquired a new location
+ */
+bool NMEAGPS::lookForTime()
 {
-    // First consume any chars that have piled up at the receiver
-    while (_serial_gps->available() > 0) {
-        int c = _serial_gps->read();
-        // DEBUG_MSG("%c", c);
-        bool isValid = reader.encode(c);
-
-        // if we have received valid NMEA claim we are connected
-        if (isValid)
-            isConnected = true;
-    }
-
-    // If we are overdue for an update, turn on the GPS and at least publish the current status
-    uint32_t now = millis();
-    bool mustPublishUpdate = false;
-    if ((now - lastUpdateMsec) > 30 * 1000 && !wantNewLocation) {
-        // Ugly hack for now - limit update checks to once every 30 secs
-        setWantLocation(true);
-        mustPublishUpdate =
-            true; // Even if we don't have an update this time, we at least want to occasionally publish the current state
-    }
-
-    // Only bother looking at GPS state if we are interested in what it has to say
-    if (wantNewLocation) {
     auto ti = reader.time;
     auto d = reader.date;
     if (ti.isUpdated() && ti.isValid() && d.isValid()) {
@@ -63,14 +46,26 @@ void NMEAGPS::loop()
         t.tm_year = d.year() - 1900;
         t.tm_isdst = false;
         perhapsSetRTC(t);
+
+        return true;
+    } else
+        return false;
 }
 
-        uint8_t fixtype = reader.fixQuality();
+/**
-        hasValidLocation = ((fixtype >= 1) && (fixtype <= 5));
+ * Perform any processing that should be done only while the GPS is awake and looking for a fix.
+ * Override this method to check for new locations
+ *
+ * @return true if we've acquired a new location
+ */
+bool NMEAGPS::lookForLocation()
+{
+    bool foundLocation = false;
+
+    // uint8_t fixtype = reader.fixQuality();
+    // hasValidLocation = ((fixtype >= 1) && (fixtype <= 5));
 
     if (reader.location.isUpdated()) {
-            lastUpdateMsec = now;
-
         if (reader.altitude.isValid())
             altitude = reader.altitude.meters();
 
@@ -78,33 +73,38 @@ void NMEAGPS::loop()
             auto loc = reader.location.value();
             latitude = toDegInt(loc.lat);
             longitude = toDegInt(loc.lng);
-
+            foundLocation = true;
-                // Once we get a location we no longer desperately want an update
-                setWantLocation(false);
         }
+
         // Diminution of precision (an accuracy metric) is reported in 10^2 units, so we need to scale down when we use it
         if (reader.hdop.isValid()) {
             dop = reader.hdop.value();
         }
         if (reader.course.isValid()) {
-                heading =
+            heading = reader.course.value() * 1e3; // Scale the heading (in degrees * 10^-2) to match the expected degrees * 10^-5
-                    reader.course.value() * 1e3; // Scale the heading (in degrees * 10^-2) to match the expected degrees * 10^-5
         }
         if (reader.satellites.isValid()) {
             numSatellites = reader.satellites.value();
         }
 
         // expect gps pos lat=37.520825, lon=-122.309162, alt=158
-            DEBUG_MSG("new NMEA GPS pos lat=%f, lon=%f, alt=%d, hdop=%g, heading=%f\n", latitude * 1e-7, longitude * 1e-7,
+        DEBUG_MSG("new NMEA GPS pos lat=%f, lon=%f, alt=%d, hdop=%g, heading=%f\n", latitude * 1e-7, longitude * 1e-7, altitude,
-                      altitude, dop * 1e-2, heading * 1e-5);
+                  dop * 1e-2, heading * 1e-5);
-            mustPublishUpdate = true;
     }
 
-        if (mustPublishUpdate) {
+    return foundLocation;
-            // Notify any status instances that are observing us
-            const meshtastic::GPSStatus status =
-                meshtastic::GPSStatus(hasLock(), isConnected, latitude, longitude, altitude, dop, heading, numSatellites);
-            newStatus.notifyObservers(&status);
 }
+
+bool NMEAGPS::whileIdle()
+{
+    bool isValid = false;
+
+    // First consume any chars that have piled up at the receiver
+    while (_serial_gps->available() > 0) {
+        int c = _serial_gps->read();
+        // DEBUG_MSG("%c", c);
+        isValid |= reader.encode(c);
     }
+
+    return isValid;
 }

src/gps/NMEAGPS.h

@@ -14,10 +14,29 @@ class NMEAGPS : public GPS
 {
     TinyGPSPlus reader;
 
-    uint32_t lastUpdateMsec = 0;
-
   public:
     virtual bool setup();
 
-    virtual void loop();
+  protected:
+    /** Subclasses should look for serial rx characters here and feed it to their GPS parser
+     * 
+     * Return true if we received a valid message from the GPS
+    */
+    virtual bool whileIdle();
+
+    /**
+     * Perform any processing that should be done only while the GPS is awake and looking for a fix.
+     * Override this method to check for new locations
+     *
+     * @return true if we've acquired a time
+     */
+    virtual bool lookForTime();
+
+    /**
+     * Perform any processing that should be done only while the GPS is awake and looking for a fix.
+     * Override this method to check for new locations
+     *
+     * @return true if we've acquired a new location
+     */
+    virtual bool lookForLocation();
 };

src/gps/UBloxGPS.cpp

@@ -1,11 +1,9 @@
 #include "UBloxGPS.h"
 #include "error.h"
-#include "sleep.h"
 #include <assert.h>
 
-UBloxGPS::UBloxGPS() : concurrency::PeriodicTask()
+UBloxGPS::UBloxGPS() 
 {
-    notifySleepObserver.observe(&notifySleep);
 }
 
 bool UBloxGPS::tryConnect()
@@ -53,13 +51,15 @@ bool UBloxGPS::setup()
     if (isConnected) {
         DEBUG_MSG("Connected to UBLOX GPS successfully\n");
 
+        GPS::setup();
+
         if (!setUBXMode())
             recordCriticalError(UBloxInitFailed); // Don't halt the boot if saving the config fails, but do report the bug
 
-        concurrency::PeriodicTask::setup(); // We don't start our periodic task unless we actually found the device
-
         return true;
     } else {
+        // Note: we do not call superclass setup in this case, because we dont want sleep observer registered
+
         return false;
     }
 }
@@ -120,38 +120,15 @@ bool UBloxGPS::factoryReset()
     return ok;
 }
 
-/// Prepare the GPS for the cpu entering deep or light sleep, expect to be gone for at least 100s of msecs
+/**
-int UBloxGPS::prepareSleep(void *unused)
+ * Perform any processing that should be done only while the GPS is awake and looking for a fix.
+ * Override this method to check for new locations
+ *
+ * @return true if we've acquired a new location
+ */
+bool UBloxGPS::lookForTime()
 {
-    if (isConnected)
+    if (ublox.getT(maxWait())) {
-        ublox.powerOff();
-
-    return 0;
-}
-
-void UBloxGPS::doTask()
-{
-    if (isConnected) {
-        // Consume all characters that have arrived
-
-        uint8_t fixtype = 3; // If we are only using the RX pin, assume we have a 3d fix
-
-        // if using i2c or serial look too see if any chars are ready
-        ublox.checkUblox(); // See if new data is available. Process bytes as they come in.
-
-        // If we don't have a fix (a quick check), don't try waiting for a solution)
-        // Hmmm my fix type reading returns zeros for fix, which doesn't seem correct, because it is still sptting out positions
-        // turn off for now
-        uint16_t maxWait = i2cAddress ? 300 : 0; // If using i2c we must poll with wait
-        fixtype = ublox.getFixType(maxWait);
-        DEBUG_MSG("GPS fix type %d\n", fixtype);
-
-        // DEBUG_MSG("sec %d\n", ublox.getSecond());
-        // DEBUG_MSG("lat %d\n", ublox.getLatitude());
-
-        // any fix that has time
-
-        if (ublox.getT(maxWait)) {
         /* Convert to unix time
         The Unix epoch (or Unix time or POSIX time or Unix timestamp) is the number of seconds that have elapsed since January
         1, 1970 (midnight UTC/GMT), not counting leap seconds (in ISO 8601: 1970-01-01T00:00:00Z).
@@ -165,8 +142,31 @@ void UBloxGPS::doTask()
         t.tm_year = ublox.getYear(0) - 1900;
         t.tm_isdst = false;
         perhapsSetRTC(t);
+        return true;
+    }
+    else
+    {
+        return false;
+    }
 }
 
+/**
+ * Perform any processing that should be done only while the GPS is awake and looking for a fix.
+ * Override this method to check for new locations
+ *
+ * @return true if we've acquired a new location
+ */
+bool UBloxGPS::lookForLocation()
+{
+    bool foundLocation = false;
+
+    // If we don't have a fix (a quick check), don't try waiting for a solution)
+    uint8_t fixtype = ublox.getFixType(maxWait());
+    DEBUG_MSG("GPS fix type %d\n", fixtype);
+
+    // we only notify if position has changed due to a new fix
+    if ((fixtype >= 3 && fixtype <= 4) && ublox.getP(maxWait())) // rd fixes only
+    {
         latitude = ublox.getLatitude(0);
         longitude = ublox.getLongitude(0);
         altitude = ublox.getAltitudeMSL(0) / 1000; // in mm convert to meters
@@ -176,33 +176,24 @@ void UBloxGPS::doTask()
 
         // bogus lat lon is reported as 0 or 0 (can be bogus just for one)
         // Also: apparently when the GPS is initially reporting lock it can output a bogus latitude > 90 deg!
-        hasValidLocation =
+        foundLocation =
             (latitude != 0) && (longitude != 0) && (latitude <= 900000000 && latitude >= -900000000) && (numSatellites > 0);
+    } 
 
-        // we only notify if position has changed due to a new fix
+    return foundLocation;
-        if ((fixtype >= 3 && fixtype <= 4) && ublox.getP(maxWait)) // rd fixes only
+}
+
+bool UBloxGPS::whileIdle()
 {
-            if (hasValidLocation) {
+    // if using i2c or serial look too see if any chars are ready
-                setWantLocation(false);
+    return ublox.checkUblox(); // See if new data is available. Process bytes as they come in. 
-                // ublox.powerOff();
-            }
-        } else // we didn't get a location update, go back to sleep and hope the characters show up
-            setWantLocation(true);
-
-        // Notify any status instances that are observing us
-        const meshtastic::GPSStatus status =
-            meshtastic::GPSStatus(hasLock(), isConnected, latitude, longitude, altitude, dop, heading, numSatellites);
-        newStatus.notifyObservers(&status);
 }
 
-    // Once we have sent a location once we only poll the GPS rarely, otherwise check back every 10s until we have something
+
-    // over the serial
+/// If possible force the GPS into sleep/low power mode
-    setPeriod(hasValidLocation && !wantNewLocation ? 30 * 1000 : 10 * 1000);
+/// Note: ublox doesn't need a wake method, because as soon as we send chars to the GPS it will wake up
+void UBloxGPS::sleep() {
+    if (isConnected)
+        ublox.powerOff();
 }
 
-void UBloxGPS::startLock()
-{
-    DEBUG_MSG("Looking for GPS lock\n");
-    wantNewLocation = true;
-    setPeriod(1);
-}

src/gps/UBloxGPS.h

@@ -1,6 +1,5 @@
 #pragma once
 
-#include "../concurrency/PeriodicTask.h"
 #include "GPS.h"
 #include "Observer.h"
 #include "SparkFun_Ublox_Arduino_Library.h"
@@ -10,12 +9,10 @@
  *
  * When new data is available it will notify observers.
  */
-class UBloxGPS : public GPS, public concurrency::PeriodicTask
+class UBloxGPS : public GPS
 {
     SFE_UBLOX_GPS ublox;
 
-    CallbackObserver<UBloxGPS, void *> notifySleepObserver = CallbackObserver<UBloxGPS, void *>(this, &UBloxGPS::prepareSleep);
-
   public:
     UBloxGPS();
 
@@ -24,13 +21,6 @@ class UBloxGPS : public GPS, public concurrency::PeriodicTask
      */
     virtual bool setup();
 
-    virtual void doTask();
-
-    /**
-     * Restart our lock attempt - try to get and broadcast a GPS reading ASAP
-     * called after the CPU wakes from light-sleep state */
-    virtual void startLock();
-
     /**
      * Reset our GPS back to factory settings
      *
@@ -38,10 +28,33 @@ class UBloxGPS : public GPS, public concurrency::PeriodicTask
      */
     bool factoryReset();
 
+  protected:
+    /** Subclasses should look for serial rx characters here and feed it to their GPS parser
+     *
+     * Return true if we received a valid message from the GPS
+     */
+    virtual bool whileIdle();
+
+    /**
+     * Perform any processing that should be done only while the GPS is awake and looking for a fix.
+     * Override this method to check for new locations
+     *
+     * @return true if we've acquired a time
+     */
+    virtual bool lookForTime();
+
+    /**
+     * Perform any processing that should be done only while the GPS is awake and looking for a fix.
+     * Override this method to check for new locations
+     *
+     * @return true if we've acquired a new location
+     */
+    virtual bool lookForLocation();
+
+    /// If possible force the GPS into sleep/low power mode
+    virtual void sleep();
+
   private:
-    /// Prepare the GPS for the cpu entering deep or light sleep, expect to be gone for at least 100s of msecs
-    /// always returns 0 to indicate okay to sleep
-    int prepareSleep(void *unused);
 
     /// Attempt to connect to our GPS, returns false if no gps is present
     bool tryConnect();
@@ -49,4 +62,6 @@ class UBloxGPS : public GPS, public concurrency::PeriodicTask
     /// Switch to our desired operating mode and save the settings to flash
     /// returns true for success
     bool setUBXMode();
+
+    uint16_t maxWait() const { return i2cAddress ? 300 : 0; /*If using i2c we must poll with wait */ }
 };