From 7aacfd66ef7e924dbc83f00be5ba952be4207f98 Mon Sep 17 00:00:00 2001 From: Kevin Hester Date: Fri, 8 Jan 2021 13:15:49 +0800 Subject: [PATCH] add assertIsSetup() and use it from OSThread constructor fixes nasty bug @mc-hamster discovered with plugin order of operations --- docs/software/plugin-api.md | 1 + src/concurrency/OSThread.cpp | 34 ++++++++++++++++++++++++++++ src/concurrency/OSThread.h | 20 +++++++++++++--- src/graphics/Screen.cpp | 2 +- src/main.cpp | 22 +++++++++++------- src/mesh/MeshService.cpp | 14 ++++++++---- src/plugins/NodeInfoPlugin.cpp | 2 +- src/plugins/NodeInfoPlugin.h | 2 +- src/plugins/Plugins.cpp | 20 ++++++++++++++++ src/plugins/Plugins.h | 6 +++++ src/plugins/PositionPlugin.cpp | 2 +- src/plugins/PositionPlugin.h | 2 +- src/plugins/RemoteHardwarePlugin.cpp | 2 -- src/plugins/ReplyPlugin.cpp | 3 --- src/plugins/TextMessagePlugin.cpp | 2 +- src/plugins/TextMessagePlugin.h | 2 +- 16 files changed, 108 insertions(+), 28 deletions(-) create mode 100644 src/plugins/Plugins.cpp create mode 100644 src/plugins/Plugins.h diff --git a/docs/software/plugin-api.md b/docs/software/plugin-api.md index e0699a6bc..360acb51b 100644 --- a/docs/software/plugin-api.md +++ b/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" diff --git a/src/concurrency/OSThread.cpp b/src/concurrency/OSThread.cpp index 58a0d59ca..5e20debf9 100644 --- a/src/concurrency/OSThread.cpp +++ b/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 diff --git a/src/concurrency/OSThread.h b/src/concurrency/OSThread.h index 3be149d5b..7a86498b9 100644 --- a/src/concurrency/OSThread.h +++ b/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 diff --git a/src/graphics/Screen.cpp b/src/graphics/Screen.cpp index 92e38337e..34ab53ac5 100644 --- a/src/graphics/Screen.cpp +++ b/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() diff --git a/src/main.cpp b/src/main.cpp index 029f753ea..dffbfc9b2 100644 --- a/src/main.cpp +++ b/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 #include @@ -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 diff --git a/src/mesh/MeshService.cpp b/src/mesh/MeshService.cpp index 075c0007e..073c48144 100644 --- a/src/mesh/MeshService.cpp +++ b/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; diff --git a/src/plugins/NodeInfoPlugin.cpp b/src/plugins/NodeInfoPlugin.cpp index 876f5ea02..54aea4cc7 100644 --- a/src/plugins/NodeInfoPlugin.cpp +++ b/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) { diff --git a/src/plugins/NodeInfoPlugin.h b/src/plugins/NodeInfoPlugin.h index 968d5f8b7..aabeb0059 100644 --- a/src/plugins/NodeInfoPlugin.h +++ b/src/plugins/NodeInfoPlugin.h @@ -29,4 +29,4 @@ class NodeInfoPlugin : public ProtobufPlugin virtual MeshPacket *allocReply(); }; -extern NodeInfoPlugin nodeInfoPlugin; \ No newline at end of file +extern NodeInfoPlugin *nodeInfoPlugin; \ No newline at end of file diff --git a/src/plugins/Plugins.cpp b/src/plugins/Plugins.cpp new file mode 100644 index 000000000..7a43c45ff --- /dev/null +++ b/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(); +} \ No newline at end of file diff --git a/src/plugins/Plugins.h b/src/plugins/Plugins.h new file mode 100644 index 000000000..7d2a20cf2 --- /dev/null +++ b/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(); \ No newline at end of file diff --git a/src/plugins/PositionPlugin.cpp b/src/plugins/PositionPlugin.cpp index fe4f57735..f9f86bf68 100644 --- a/src/plugins/PositionPlugin.cpp +++ b/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) { diff --git a/src/plugins/PositionPlugin.h b/src/plugins/PositionPlugin.h index 9153a2288..6a1eb05f0 100644 --- a/src/plugins/PositionPlugin.h +++ b/src/plugins/PositionPlugin.h @@ -30,4 +30,4 @@ class PositionPlugin : public ProtobufPlugin virtual MeshPacket *allocReply(); }; -extern PositionPlugin positionPlugin; \ No newline at end of file +extern PositionPlugin *positionPlugin; \ No newline at end of file diff --git a/src/plugins/RemoteHardwarePlugin.cpp b/src/plugins/RemoteHardwarePlugin.cpp index e63e05cff..26b78c800 100644 --- a/src/plugins/RemoteHardwarePlugin.cpp +++ b/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 diff --git a/src/plugins/ReplyPlugin.cpp b/src/plugins/ReplyPlugin.cpp index 788edc55e..a8c607436 100644 --- a/src/plugins/ReplyPlugin.cpp +++ b/src/plugins/ReplyPlugin.cpp @@ -5,9 +5,6 @@ #include -// 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 diff --git a/src/plugins/TextMessagePlugin.cpp b/src/plugins/TextMessagePlugin.cpp index 1e742454b..4ad080702 100644 --- a/src/plugins/TextMessagePlugin.cpp +++ b/src/plugins/TextMessagePlugin.cpp @@ -3,7 +3,7 @@ #include "NodeDB.h" #include "PowerFSM.h" -TextMessagePlugin textMessagePlugin; +TextMessagePlugin *textMessagePlugin; bool TextMessagePlugin::handleReceived(const MeshPacket &mp) { diff --git a/src/plugins/TextMessagePlugin.h b/src/plugins/TextMessagePlugin.h index 1a6a45076..f7eba65f6 100644 --- a/src/plugins/TextMessagePlugin.h +++ b/src/plugins/TextMessagePlugin.h @@ -22,4 +22,4 @@ class TextMessagePlugin : public SinglePortPlugin, public Observable