clean up PeriodicTask so I can eventually use it with a scheduler

src/GPS.cpp

@@ -27,6 +27,8 @@ GPS::GPS() : PeriodicTask() {}
 
 void GPS::setup()
 {
+    PeriodicTask::setup();
+    
     readFromRTC(); // read the main CPU RTC at first
 
 #ifdef GPS_RX_PIN
@@ -114,12 +116,6 @@ void GPS::perhapsSetRTC(const struct timeval *tv)
 
 #include <time.h>
 
-// for the time being we need to rapidly read from the serial port to prevent overruns
-void GPS::loop()
-{
-    PeriodicTask::loop();
-}
-
 uint32_t GPS::getTime()
 {
     return ((millis() - timeStartMsec) / 1000) + zeroOffsetSecs;

src/GPS.h

@@ -29,7 +29,6 @@ class GPS : public PeriodicTask, public Observable<void *>
 
     void setup();
 
-    virtual void loop();
 
     virtual void doTask();
 

src/MeshService.cpp

@@ -48,6 +48,15 @@ MeshService service;
 
 #define NUM_PACKET_ID 255 // 0 is consider invalid
 
+static uint32_t sendOwnerCb()
+{
+    service.sendOurOwner();
+
+    return radioConfig.preferences.send_owner_interval * radioConfig.preferences.position_broadcast_secs * 1000;
+}
+
+static Periodic sendOwnerPeriod(sendOwnerCb);
+
 /// Generate a unique packet id
 // FIXME, move this someplace better
 PacketId generatePacketId()
@@ -65,6 +74,7 @@ MeshService::MeshService() : toPhoneQueue(MAX_RX_TOPHONE)
 
 void MeshService::init()
 {
+    sendOwnerPeriod.setup();
     nodeDB.init();
 
     gpsObserver.observe(&gps);
@@ -184,15 +194,6 @@ int MeshService::handleFromRadio(const MeshPacket *mp)
     return 0;
 }
 
-uint32_t sendOwnerCb()
-{
-    service.sendOurOwner();
-
-    return radioConfig.preferences.send_owner_interval * radioConfig.preferences.position_broadcast_secs * 1000;
-}
-
-Periodic sendOwnerPeriod(sendOwnerCb);
-
 /// Do idle processing (mostly processing messages which have been queued from the radio)
 void MeshService::loop()
 {
@@ -200,9 +201,6 @@ void MeshService::loop()
         fromNumChanged.notifyObservers(fromNum);
         oldFromNum = fromNum;
     }
-
-    // occasionally send our owner info
-    sendOwnerPeriod.loop();
 }
 
 /// The radioConfig object just changed, call this to force the hw to change to the new settings
@@ -216,7 +214,8 @@ void MeshService::reloadConfig()
 
 /**
  *  Given a ToRadio buffer parse it and properly handle it (setup radio, owner or send packet into the mesh)
- * Called by PhoneAPI.handleToRadio.  Note: p is a scratch buffer, this function is allowed to write to it but it can not keep a reference
+ * Called by PhoneAPI.handleToRadio.  Note: p is a scratch buffer, this function is allowed to write to it but it can not keep a
+ * reference
  */
 void MeshService::handleToRadio(MeshPacket &p)
 {

src/PeriodicTask.cpp

@@ -1,21 +1,39 @@
 #include "PeriodicTask.h"
 #include "Periodic.h"
+PeriodicScheduler periodicScheduler;
 
 PeriodicTask::PeriodicTask(uint32_t initialPeriod) : period(initialPeriod) {}
 
-/// call this from loop
+void PeriodicTask::setup()
-void PeriodicTask::loop()
 {
-    {
+    periodicScheduler.schedule(this);
-        meshtastic::LockGuard lg(&lock);
+}
-        uint32_t now = millis();
+
-        if (!period || (now - lastMsec) < period) {
+/// call this from loop
-            return;
+void PeriodicScheduler::loop()
+{
+    meshtastic::LockGuard lg(&lock);
+
+    uint32_t now = millis();
+    for (auto t : tasks) {
+        if (t->period && (now - t->lastMsec) >= t->period) {
+
+            t->doTask();
+            t->lastMsec = now;
         }
-        lastMsec = now;
     }
-    // Release the lock in case the task wants to change the period.
+}
-    doTask();
+
+void PeriodicScheduler::schedule(PeriodicTask *t)
+{
+    meshtastic::LockGuard lg(&lock);
+    tasks.insert(t);
+}
+
+void PeriodicScheduler::unschedule(PeriodicTask *t)
+{
+    meshtastic::LockGuard lg(&lock);
+    tasks.erase(t);
 }
 
 void Periodic::doTask()

src/PeriodicTask.h

@@ -1,8 +1,40 @@
 #pragma once
 
-#include <cstdint>
-
 #include "lock.h"
+#include <cstdint>
+#include <unordered_set>
+
+class PeriodicTask;
+
+/**
+ * Runs all PeriodicTasks in the system.
+ *
+ * Currently called from main loop() but eventually should be its own thread blocked on a freertos timer.
+ */
+class PeriodicScheduler
+{
+    friend class PeriodicTask;
+
+    /**
+     * This really should be some form of heap, and when the period gets changed on a task it should get
+     * rescheduled in that heap.  Currently it is just a dumb array and everytime we run loop() we check
+     * _every_ tasks.  If it was a heap we'd only have to check the first task.
+     */
+    std::unordered_set<PeriodicTask *> tasks;
+
+    // Protects the above variables.
+    meshtastic::Lock lock;
+
+  public:
+    /// Run any next tasks which are due for execution
+    void loop();
+
+  private:
+    void schedule(PeriodicTask *t);
+    void unschedule(PeriodicTask *t);
+};
+
+extern PeriodicScheduler periodicScheduler;
 
 /**
  * A base class for tasks that want their doTask() method invoked periodically
@@ -13,26 +45,33 @@
  */
 class PeriodicTask
 {
+    friend class PeriodicScheduler;
+
     uint32_t lastMsec = 0;
     uint32_t period = 1; // call soon after creation
 
-    // Protects the above variables.
-    meshtastic::Lock lock;
-
   public:
-    virtual ~PeriodicTask() {}
+    virtual ~PeriodicTask() { periodicScheduler.unschedule(this); }
 
+    /**
+     * Constructor (will schedule with the global PeriodicScheduler)
+     */
     PeriodicTask(uint32_t initialPeriod = 1);
 
-    /// call this from loop
+    /** MUST be be called once at startup (but after threading is running - i.e. not from a constructor)
-    virtual void loop();
+     */
+    void setup();
 
-    /// Set a new period in msecs (can be called from doTask or elsewhere and the scheduler will cope)
+    /**
-    void setPeriod(uint32_t p)
+     * Set a new period in msecs (can be called from doTask or elsewhere and the scheduler will cope)
-    {
+     * While zero this task is disabled and will not run
-        meshtastic::LockGuard lg(&lock);
+     */
-        period = p;
+    void setPeriod(uint32_t p) { period = p; }
-    }
+
+    /**
+     * Syntatic sugar for suspending tasks
+     */
+    void disable() { setPeriod(0); }
 
   protected:
     virtual void doTask() = 0;

src/main.cpp

@@ -27,6 +27,7 @@
 #include "NodeDB.h"
 #include "Periodic.h"
 #include "PowerFSM.h"
+#include "Router.h"
 #include "configuration.h"
 #include "error.h"
 #include "power.h"
@@ -225,7 +226,18 @@ const char *getDeviceName()
 
 static MeshRadio *radio = NULL;
 
-#include "Router.h"
+static uint32_t ledBlinker()
+{
+    static bool ledOn;
+    ledOn ^= 1;
+
+    setLed(ledOn);
+
+    // have a very sparse duty cycle of LED being on, unless charging, then blink 0.5Hz square wave rate to indicate that
+    return powerStatus.charging ? 1000 : (ledOn ? 2 : 1000);
+}
+
+Periodic ledPeriodic(ledBlinker);
 
 void setup()
 {
@@ -261,6 +273,8 @@ void setup()
     digitalWrite(LED_PIN, 1 ^ LED_INVERTED); // turn on for now
 #endif
 
+    ledPeriodic.setup();
+
     // Hello
     DEBUG_MSG("Meshtastic swver=%s, hwver=%s\n", xstr(APP_VERSION), xstr(HW_VERSION));
 
@@ -299,19 +313,6 @@ void setup()
     setCPUFast(false); // 80MHz is fine for our slow peripherals
 }
 
-uint32_t ledBlinker()
-{
-    static bool ledOn;
-    ledOn ^= 1;
-
-    setLed(ledOn);
-
-    // have a very sparse duty cycle of LED being on, unless charging, then blink 0.5Hz square wave rate to indicate that
-    return powerStatus.charging ? 1000 : (ledOn ? 2 : 1000);
-}
-
-Periodic ledPeriodic(ledBlinker);
-
 #if 0
 // Turn off for now
 
@@ -330,18 +331,18 @@ uint32_t axpDebugRead()
 }
 
 Periodic axpDebugOutput(axpDebugRead);
+axpDebugOutput.setup();
 #endif
 
 void loop()
 {
     uint32_t msecstosleep = 1000 * 30; // How long can we sleep before we again need to service the main loop?
 
-    gps.loop();
     router.loop();
     powerFSM.run_machine();
     service.loop();
 
-    ledPeriodic.loop();
+    periodicScheduler.loop();
     // axpDebugOutput.loop();
 
 #ifndef NO_ESP32
@@ -419,7 +420,6 @@ void loop()
     screen.debug()->setPowerStatus(powerStatus);
     // TODO(#4): use something based on hdop to show GPS "signal" strength.
     screen.debug()->setGPSStatus(gps.hasLock() ? "ok" : ":(");
-    screen.loop();
 
     // No GPS lock yet, let the OS put the main CPU in low power mode for 100ms (or until another interrupt comes in)
     // i.e. don't just keep spinning in loop as fast as we can.

src/screen.cpp

@@ -384,7 +384,6 @@ void _screen_header()
     if (!disp)
         return;
 
-
     // Message count
     //snprintf(buffer, sizeof(buffer), "#%03d", ttn_get_count() % 1000);
     //display->setTextAlignment(TEXT_ALIGN_LEFT);
@@ -423,6 +422,8 @@ void Screen::handleSetOn(bool on)
 
 void Screen::setup()
 {
+    PeriodicTask::setup();
+
     // We don't set useDisplay until setup() is called, because some boards have a declaration of this object but the device
     // is never found when probing i2c and therefore we don't call setup and never want to do (invalid) accesses to this device.
     useDisplay = true;

src/sleep.cpp

@@ -3,7 +3,6 @@
 #include "MeshRadio.h"
 #include "MeshService.h"
 #include "NodeDB.h"
-#include "Periodic.h"
 #include "configuration.h"
 #include "error.h"