add assertIsSetup() and use it from OSThread constructor

fixes nasty bug @mc-hamster discovered with plugin order of operations

docs/software/plugin-api.md

@@ -46,6 +46,7 @@ The easiest way to get started is:
 
 * [Build and install](build-instructions.md) the standard codebase from github.
 * Copy [src/plugins/ReplyPlugin.*](/src/plugins/ReplyPlugin.cpp) into src/plugins/YourPlugin.*.  Then change the port number from REPLY_APP to PRIVATE_APP.
+* Edit plugins/Plugins.cpp:setupPlugins() to add a call to create an instance of your plugin (see comment at head of that function)
 * Rebuild with your new messaging goodness and install on the device
 * Use the [meshtastic commandline tool](https://github.com/meshtastic/Meshtastic-python) to send a packet to your board "meshtastic --dest 1234 --ping"
 

src/concurrency/OSThread.cpp

@@ -28,6 +28,8 @@ void OSThread::setup()
 OSThread::OSThread(const char *_name, uint32_t period, ThreadController *_controller)
     : Thread(NULL, period), controller(_controller)
 {
+    assertIsSetup();
+
     ThreadName = _name;
 
     if (controller)
@@ -81,4 +83,36 @@ void OSThread::run()
     currentThread = NULL;
 }
 
+/**
+ * This flag is set **only** when setup() starts, to provide a way for us to check for sloppy static constructor calls.
+ * Call assertIsSetup() to force a crash if someone tries to create an instance too early.
+ *
+ * it is super important to never allocate those object statically.  instead, you should explicitly
+ *  new them at a point where you are guaranteed that other objects that this instance
+ * depends on have already been created.
+ *
+ * in particular, for OSThread that means "all instances must be declared via new() in setup() or later" -
+ * this makes it guaranteed that the global mainController is fully constructed first.
+ */
+bool hasBeenSetup;
+
+void assertIsSetup()
+{
+
+    /**
+     * Dear developer comrade - If this assert fails() that means you need to fix the following:
+     *
+     * This flag is set **only** when setup() starts, to provide a way for us to check for sloppy static constructor calls.
+     * Call assertIsSetup() to force a crash if someone tries to create an instance too early.
+     *
+     * it is super important to never allocate those object statically.  instead, you should explicitly
+     *  new them at a point where you are guaranteed that other objects that this instance
+     * depends on have already been created.
+     *
+     * in particular, for OSThread that means "all instances must be declared via new() in setup() or later" -
+     * this makes it guaranteed that the global mainController is fully constructed first.
+     */
+    assert(hasBeenSetup);
+}
+
 } // namespace concurrency

src/concurrency/OSThread.h

@@ -17,14 +17,14 @@ extern InterruptableDelay mainDelay;
 
 /**
  * @brief Base threading
- * 
+ *
  * This is a pseudo threading layer that is super easy to port, well suited to our slow network and very ram & power efficient.
  *
  * TODO FIXME @geeksville
  *
  * move more things into OSThreads
  * remove lock/lockguard
- * 
+ *
  * move typedQueue into concurrency
  * remove freertos from typedqueue
  */
@@ -42,7 +42,6 @@ class OSThread : public Thread
     static bool showWaiting;
 
   public:
-
     /// For debug printing only (might be null)
     static const OSThread *currentThread;
 
@@ -71,4 +70,19 @@ class OSThread : public Thread
     virtual void run();
 };
 
+/**
+ * This flag is set **only** when setup() starts, to provide a way for us to check for sloppy static constructor calls.
+ * Call assertIsSetup() to force a crash if someone tries to create an instance too early.
+ *
+ * it is super important to never allocate those object statically.  instead, you should explicitly
+ *  new them at a point where you are guaranteed that other objects that this instance
+ * depends on have already been created.
+ *
+ * in particular, for OSThread that means "all instances must be declared via new() in setup() or later" -
+ * this makes it guaranteed that the global mainController is fully constructed first.
+ */
+extern bool hasBeenSetup;
+
+void assertIsSetup();
+
 } // namespace concurrency

src/graphics/Screen.cpp

@@ -746,7 +746,7 @@ void Screen::setup()
     powerStatusObserver.observe(&powerStatus->onNewStatus);
     gpsStatusObserver.observe(&gpsStatus->onNewStatus);
     nodeStatusObserver.observe(&nodeStatus->onNewStatus);
-    textMessageObserver.observe(&textMessagePlugin);
+    textMessageObserver.observe(textMessagePlugin);
 }
 
 void Screen::forceDisplay()

src/main.cpp

@@ -22,6 +22,7 @@
 #include "meshwifi/meshhttp.h"
 #include "meshwifi/meshwifi.h"
 #include "sleep.h"
+#include "plugins/Plugins.h"
 #include "target_specific.h"
 #include <OneButton.h>
 #include <Wire.h>
@@ -275,6 +276,8 @@ RadioInterface *rIf = NULL;
 
 void setup()
 {
+    concurrency::hasBeenSetup = true;
+
 #ifdef SEGGER_STDOUT_CH
     SEGGER_RTT_ConfigUpBuffer(SEGGER_STDOUT_CH, NULL, NULL, 1024, SEGGER_RTT_MODE_NO_BLOCK_TRIM);
 #endif
@@ -390,15 +393,15 @@ void setup()
     readFromRTC(); // read the main CPU RTC at first (in case we can't get GPS time)
 
 #ifdef GENIEBLOCKS
-    //gps setup
-    pinMode (GPS_RESET_N, OUTPUT);
+    // gps setup
+    pinMode(GPS_RESET_N, OUTPUT);
     pinMode(GPS_EXTINT, OUTPUT);
     digitalWrite(GPS_RESET_N, HIGH);
     digitalWrite(GPS_EXTINT, LOW);
-    //battery setup
+    // battery setup
     // If we want to read battery level, we need to set BATTERY_EN_PIN pin to low.
     // ToDo: For low power consumption after read battery level, set that pin to high.
-    pinMode (BATTERY_EN_PIN, OUTPUT);
+    pinMode(BATTERY_EN_PIN, OUTPUT);
     digitalWrite(BATTERY_EN_PIN, LOW);
 #endif
 
@@ -439,14 +442,17 @@ void setup()
 
     service.init();
 
+    // Now that the mesh service is created, create any plugins
+    setupPlugins();
+
     // Do this after service.init (because that clears error_code)
 #ifdef AXP192_SLAVE_ADDRESS
-    if(!axp192_found)
+    if (!axp192_found)
         recordCriticalError(CriticalErrorCode_NoAXP192); // Record a hardware fault for missing hardware
-#endif    
+#endif
 
-    // Don't call screen setup until after nodedb is setup (because we need
-    // the current region name)
+        // Don't call screen setup until after nodedb is setup (because we need
+        // the current region name)
 #if defined(ST7735_CS) || defined(HAS_EINK)
     screen->setup();
 #else

src/mesh/MeshService.cpp

@@ -60,7 +60,8 @@ static int32_t sendOwnerCb()
     currentGeneration = radioGeneration;
 
     DEBUG_MSG("Sending our nodeinfo to mesh (wantReplies=%d)\n", requestReplies);
-    nodeInfoPlugin.sendOurNodeInfo(NODENUM_BROADCAST, requestReplies); // Send our info (don't request replies)
+    assert(nodeInfoPlugin);
+    nodeInfoPlugin->sendOurNodeInfo(NODENUM_BROADCAST, requestReplies); // Send our info (don't request replies)
 
     return getPref_send_owner_interval() * getPref_position_broadcast_secs() * 1000;
 }
@@ -134,7 +135,8 @@ bool MeshService::reloadConfig()
 /// The owner User record just got updated, update our node DB and broadcast the info into the mesh
 void MeshService::reloadOwner()
 {
-    nodeInfoPlugin.sendOurNodeInfo();
+    assert(nodeInfoPlugin);
+    nodeInfoPlugin->sendOurNodeInfo();
     nodeDB.saveToDisk();
 }
 
@@ -193,10 +195,11 @@ void MeshService::sendNetworkPing(NodeNum dest, bool wantReplies)
     assert(node);
 
     DEBUG_MSG("Sending network ping to 0x%x, with position=%d, wantReplies=%d\n", dest, node->has_position, wantReplies);
+    assert(positionPlugin && nodeInfoPlugin);
     if (node->has_position)
-        positionPlugin.sendOurPosition(dest, wantReplies);
+        positionPlugin->sendOurPosition(dest, wantReplies);
     else
-        nodeInfoPlugin.sendOurNodeInfo(dest, wantReplies);
+        nodeInfoPlugin->sendOurNodeInfo(dest, wantReplies);
 }
 
 
@@ -264,7 +267,8 @@ int MeshService::onGPSChanged(const meshtastic::GPSStatus *unused)
         currentGeneration = radioGeneration;
 
         DEBUG_MSG("Sending position to mesh (wantReplies=%d)\n", requestReplies);
-        positionPlugin.sendOurPosition(NODENUM_BROADCAST, requestReplies);
+        assert(positionPlugin);
+        positionPlugin->sendOurPosition(NODENUM_BROADCAST, requestReplies);
     }
 
     return 0;

src/plugins/NodeInfoPlugin.cpp

@@ -6,7 +6,7 @@
 #include "configuration.h"
 #include "main.h"
 
-NodeInfoPlugin nodeInfoPlugin;
+NodeInfoPlugin *nodeInfoPlugin;
 
 bool NodeInfoPlugin::handleReceivedProtobuf(const MeshPacket &mp, const User &p)
 {

src/plugins/NodeInfoPlugin.h

@@ -29,4 +29,4 @@ class NodeInfoPlugin : public ProtobufPlugin<User>
     virtual MeshPacket *allocReply();
 };
 
-extern NodeInfoPlugin nodeInfoPlugin;
+extern NodeInfoPlugin *nodeInfoPlugin;

src/plugins/Plugins.cpp

@@ -0,0 +1,20 @@
+#include "plugins/NodeInfoPlugin.h"
+#include "plugins/PositionPlugin.h"
+#include "plugins/ReplyPlugin.h"
+#include "plugins/RemoteHardwarePlugin.h"
+#include "plugins/TextMessagePlugin.h"
+
+/**
+ * Create plugin instances here.  If you are adding a new plugin, you must 'new' it here (or somewhere else)
+ */
+void setupPlugins() {
+    nodeInfoPlugin = new NodeInfoPlugin();
+    positionPlugin = new PositionPlugin();
+    textMessagePlugin = new TextMessagePlugin();
+
+    // Note: if the rest of meshtastic doesn't need to explicitly use your plugin, you do not need to assign the instance
+    // to a global variable.
+
+    new RemoteHardwarePlugin();
+    new ReplyPlugin();
+}

src/plugins/Plugins.h

@@ -0,0 +1,6 @@
+#pragma once
+
+/**
+ * Create plugin instances here.  If you are adding a new plugin, you must 'new' it here (or somewhere else)
+ */
+void setupPlugins();

src/plugins/PositionPlugin.cpp

@@ -5,7 +5,7 @@
 #include "Router.h"
 #include "configuration.h"
 
-PositionPlugin positionPlugin;
+PositionPlugin *positionPlugin;
 
 bool PositionPlugin::handleReceivedProtobuf(const MeshPacket &mp, const Position &p)
 {

src/plugins/PositionPlugin.h

@@ -30,4 +30,4 @@ class PositionPlugin : public ProtobufPlugin<Position>
     virtual MeshPacket *allocReply();
 };
 
-extern PositionPlugin positionPlugin;
+extern PositionPlugin *positionPlugin;

src/plugins/RemoteHardwarePlugin.cpp

@@ -6,8 +6,6 @@
 #include "configuration.h"
 #include "main.h"
 
-RemoteHardwarePlugin remoteHardwarePlugin;
-
 #define NUM_GPIOS 64
 
 // Because (FIXME) we currently don't tell API clients status on sent messages

src/plugins/ReplyPlugin.cpp

@@ -5,9 +5,6 @@
 
 #include <assert.h>
 
-// Create an a static instance of our plugin - this registers with the plugin system
-ReplyPlugin replyPlugin;
-
 MeshPacket *ReplyPlugin::allocReply()
 {
     assert(currentRequest); // should always be !NULL

src/plugins/TextMessagePlugin.cpp

@@ -3,7 +3,7 @@
 #include "NodeDB.h"
 #include "PowerFSM.h"
 
-TextMessagePlugin textMessagePlugin;
+TextMessagePlugin *textMessagePlugin;
 
 bool TextMessagePlugin::handleReceived(const MeshPacket &mp)
 {

src/plugins/TextMessagePlugin.h

@@ -22,4 +22,4 @@ class TextMessagePlugin : public SinglePortPlugin, public Observable<const MeshP
     virtual bool handleReceived(const MeshPacket &mp);
 };
 
-extern TextMessagePlugin textMessagePlugin;
+extern TextMessagePlugin *textMessagePlugin;