Fix GPIO service and cleanup response handling

platformio.ini

@@ -9,7 +9,7 @@
 ; https://docs.platformio.org/page/projectconf.html
 
 [platformio]
-;default_envs = tbeam
+default_envs = tbeam
 ;default_envs = tbeam0.7
 ;default_envs = heltec
 ;default_envs = tlora-v1
@@ -18,7 +18,7 @@
 ;default_envs = lora-relay-v1 # nrf board
 ;default_envs = eink
 ;default_envs = nrf52840dk-geeksville
-default_envs = native # lora-relay-v1 # nrf52840dk-geeksville # linux # or if you'd like to change the default to something like lora-relay-v1 put that here
+;default_envs = native # lora-relay-v1 # nrf52840dk-geeksville # linux # or if you'd like to change the default to something like lora-relay-v1 put that here
 
 [common]
 ; common is not currently used
@@ -397,4 +397,4 @@ lib_deps =
 ;extends = esp32_base
 ;board = genieblocks_lora
 ;build_flags = 
-;  ${esp32_base.build_flags} -D GENIEBLOCKS
+;  ${esp32_base.build_flags} -D GENIEBLOCKS

src/mesh/MeshPlugin.cpp

@@ -86,8 +86,10 @@ void MeshPlugin::callPlugins(const MeshPacket &mp)
         /// We only call plugins that are interested in the packet (and the message is destined to us or we are promiscious)
         bool wantsPacket = (isDecoded || pi.encryptedOk) && (pi.isPromiscuous || toUs) && pi.wantPacket(&mp);
 
+        DEBUG_MSG("Plugin %s wantsPacket=%d\n", pi.name, wantsPacket);
+        assert(!pi.myReply); // If it is !null it means we have a bug, because it should have been sent the previous time
+
         if (wantsPacket) {
-            // DEBUG_MSG("Plugin %s wantsPacket=%d\n", pi.name, wantsPacket);
             pluginFound = true;
 
             /// received channel (or NULL if not decoded)
@@ -124,6 +126,14 @@ void MeshPlugin::callPlugins(const MeshPacket &mp)
                 } else {
                     DEBUG_MSG("Plugin %s considered\n", pi.name);
                 }
+
+                // If the requester didn't ask for a response we might need to discard unused replies to prevent memory leaks
+                if (pi.myReply) {
+                    DEBUG_MSG("Discarding an unneeded response\n");
+                    packetPool.release(pi.myReply);
+                    pi.myReply = NULL;
+                }
+
                 if (handled) {
                     DEBUG_MSG("Plugin %s handled and skipped other processing\n", pi.name);
                     break;
@@ -136,7 +146,7 @@ void MeshPlugin::callPlugins(const MeshPacket &mp)
 
     if (mp.decoded.want_response && toUs) {
         if (currentReply) {
-            DEBUG_MSG("Sending response\n");
+            printPacket("Sending response", currentReply);
             service.sendToMesh(currentReply);
             currentReply = NULL;
         } else {
@@ -154,6 +164,13 @@ void MeshPlugin::callPlugins(const MeshPacket &mp)
         DEBUG_MSG("No plugins interested in portnum=%d\n", mp.decoded.portnum);
 }
 
+MeshPacket *MeshPlugin::allocReply()
+{
+    auto r = myReply;
+    myReply = NULL; // Only use each reply once
+    return r;
+}
+
 /** Messages can be received that have the want_response bit set.  If set, this callback will be invoked
  * so that subclasses can (optionally) send a response back to the original sender.  Implementing this method
  * is optional
@@ -176,7 +193,7 @@ void MeshPlugin::sendResponse(const MeshPacket &req)
 void setReplyTo(MeshPacket *p, const MeshPacket &to)
 {
     assert(p->which_payloadVariant == MeshPacket_decoded_tag); // Should already be set by now
-    p->to = getFrom(&to);
+    p->to = getFrom(&to);    // Make sure that if we are sending to the local node, we use our local node addr, not 0
     p->channel = to.channel; // Use the same channel that the request came in on
 
     // No need for an ack if we are just delivering locally (it just generates an ignored ack)

src/mesh/MeshPlugin.h

@@ -69,6 +69,11 @@ class MeshPlugin
      */
     static const MeshPacket *currentRequest;
 
+    /**
+     * If your handler wants to send a response, simply set currentReply and it will be sent at the end of response handling.
+     */
+    MeshPacket *myReply = NULL;
+
     /**
      * Initialize your plugin.  This setup function is called once after all hardware and mesh protocol layers have
      * been initialized
@@ -87,8 +92,12 @@ class MeshPlugin
     virtual bool handleReceived(const MeshPacket &mp) { return false; }
 
     /** Messages can be received that have the want_response bit set.  If set, this callback will be invoked
-     * so that subclasses can (optionally) send a response back to the original sender.  */
+     * so that subclasses can (optionally) send a response back to the original sender.
-    virtual MeshPacket *allocReply() { return NULL; }
+     *
+     * Note: most implementers don't need to override this, instead: If while handling a request you have a reply, just set
+     * the protected reply field in this instance.
+     * */
+    virtual MeshPacket *allocReply();
 
     /***
      * @return true if you want to be alloced a UI screen frame
@@ -106,6 +115,7 @@ class MeshPlugin
      * the RoutingPlugin to avoid sending redundant acks
      */
     static MeshPacket *currentReply;
+
     friend class ReliableRouter;
 
     /** Messages can be received that have the want_response bit set.  If set, this callback will be invoked

src/plugins/AdminPlugin.cpp

@@ -19,7 +19,7 @@ void AdminPlugin::handleGetChannel(const MeshPacket &req, uint32_t channelIndex)
         AdminMessage r = AdminMessage_init_default;
         r.get_channel_response = channels.getByIndex(channelIndex);
         r.which_variant = AdminMessage_get_channel_response_tag;
-        reply = allocDataProtobuf(r);
+        myReply = allocDataProtobuf(r);
     }
 }
 
@@ -36,7 +36,7 @@ void AdminPlugin::handleGetRadio(const MeshPacket &req)
         r.get_radio_response.preferences.ls_secs = getPref_ls_secs();
 
         r.which_variant = AdminMessage_get_radio_response_tag;
-        reply = allocDataProtobuf(r);
+        myReply = allocDataProtobuf(r);
     }
 }
 
@@ -57,7 +57,7 @@ bool AdminPlugin::handleReceivedProtobuf(const MeshPacket &mp, const AdminMessag
     case AdminMessage_set_channel_tag:
         DEBUG_MSG("Client is setting channel %d\n", r->set_channel.index);
         if (r->set_channel.index < 0 || r->set_channel.index >= (int)MAX_NUM_CHANNELS)
-            reply = allocErrorResponse(Routing_Error_BAD_REQUEST, &mp);
+            myReply = allocErrorResponse(Routing_Error_BAD_REQUEST, &mp);
         else
             handleSetChannel(r->set_channel);
         break;
@@ -66,7 +66,7 @@ bool AdminPlugin::handleReceivedProtobuf(const MeshPacket &mp, const AdminMessag
         uint32_t i = r->get_channel_request - 1;
         DEBUG_MSG("Client is getting channel %u\n", i);
         if (i >= MAX_NUM_CHANNELS)
-            reply = allocErrorResponse(Routing_Error_BAD_REQUEST, &mp);
+            myReply = allocErrorResponse(Routing_Error_BAD_REQUEST, &mp);
         else
             handleGetChannel(mp, i);
         break;
@@ -141,13 +141,6 @@ void AdminPlugin::handleSetRadio(const RadioConfig &r)
     service.reloadConfig();
 }
 
-MeshPacket *AdminPlugin::allocReply()
-{
-    auto r = reply;
-    reply = NULL; // Only use each reply once
-    return r;
-}
-
 AdminPlugin::AdminPlugin() : ProtobufPlugin("Admin", PortNum_ADMIN_APP, AdminMessage_fields)
 {
     // restrict to the admin channel for rx

src/plugins/AdminPlugin.h

@@ -6,8 +6,6 @@
  */
 class AdminPlugin : public ProtobufPlugin<AdminMessage>
 {
-  MeshPacket *reply = NULL;
-  
   public:
     /** Constructor
      * name is for debugging output
@@ -21,10 +19,6 @@ class AdminPlugin : public ProtobufPlugin<AdminMessage>
     */
     virtual bool handleReceivedProtobuf(const MeshPacket &mp, const AdminMessage *p);
 
-    /** Messages can be received that have the want_response bit set.  If set, this callback will be invoked
-     * so that subclasses can (optionally) send a response back to the original sender.  */
-    virtual MeshPacket *allocReply();
-
   private:
     void handleSetOwner(const User &o);
     void handleSetChannel(const Channel &cc);

src/plugins/RemoteHardwarePlugin.cpp

@@ -10,12 +10,13 @@
 
 // Because (FIXME) we currently don't tell API clients status on sent messages
 // we need to throttle our sending, so that if a gpio is bouncing up and down we
-// don't generate more messages than the net can send. So we limit watch messages to 
+// don't generate more messages than the net can send. So we limit watch messages to
 // a max of one change per 30 seconds
 #define WATCH_INTERVAL_MSEC (30 * 1000)
 
 /// Set pin modes for every set bit in a mask
-static void pinModes(uint64_t mask, uint8_t mode) {
+static void pinModes(uint64_t mask, uint8_t mode)
+{
     for (uint8_t i = 0; i < NUM_GPIOS; i++) {
         if (mask & (1 << i)) {
             pinMode(i, mode);
@@ -24,7 +25,8 @@ static void pinModes(uint64_t mask, uint8_t mode) {
 }
 
 /// Read all the pins mentioned in a mask
-static uint64_t digitalReads(uint64_t mask) {
+static uint64_t digitalReads(uint64_t mask)
+{
     uint64_t res = 0;
 
     pinModes(mask, INPUT_PULLUP);
@@ -40,10 +42,9 @@ static uint64_t digitalReads(uint64_t mask) {
     return res;
 }
 
-
 RemoteHardwarePlugin::RemoteHardwarePlugin()
-    : ProtobufPlugin("remotehardware", PortNum_REMOTE_HARDWARE_APP, HardwareMessage_fields),
+    : ProtobufPlugin("remotehardware", PortNum_REMOTE_HARDWARE_APP, HardwareMessage_fields), concurrency::OSThread(
-    concurrency::OSThread("remotehardware")
+                                                                                                 "remotehardware")
 {
 }
 
@@ -69,26 +70,26 @@ bool RemoteHardwarePlugin::handleReceivedProtobuf(const MeshPacket &req, const H
 
     case HardwareMessage_Type_READ_GPIOS: {
         // Print notification to LCD screen
-        if(screen)
+        if (screen)
             screen->print("Read GPIOs\n");
 
         uint64_t res = digitalReads(p.gpio_mask);
 
         // Send the reply
-        HardwareMessage reply = HardwareMessage_init_default;
+        HardwareMessage r = HardwareMessage_init_default;
-        reply.typ = HardwareMessage_Type_READ_GPIOS_REPLY;
+        r.typ = HardwareMessage_Type_READ_GPIOS_REPLY;
-        reply.gpio_value = res;
+        r.gpio_value = res;
-        MeshPacket *p = allocDataProtobuf(reply);
+        MeshPacket *p = allocDataProtobuf(r);
         setReplyTo(p, req);
-        service.sendToMesh(p);
+        myReply = p;
         break;
     }
 
     case HardwareMessage_Type_WATCH_GPIOS: {
         watchGpios = p.gpio_mask;
-        lastWatchMsec = 0; // Force a new publish soon
+        lastWatchMsec = 0;           // Force a new publish soon
         previousWatch = ~watchGpios; // generate a 'previous' value which is guaranteed to not match (to force an initial publish)
-        enabled = true; // Let our thread run at least once
+        enabled = true;              // Let our thread run at least once
         DEBUG_MSG("Now watching GPIOs 0x%llx\n", watchGpios);
         break;
     }
@@ -101,31 +102,31 @@ bool RemoteHardwarePlugin::handleReceivedProtobuf(const MeshPacket &req, const H
         DEBUG_MSG("Hardware operation %d not yet implemented! FIXME\n", p.typ);
         break;
     }
-    
+
-    return true; // handled
+    return false;
 }
 
-int32_t RemoteHardwarePlugin::runOnce() {
+int32_t RemoteHardwarePlugin::runOnce()
-    if(watchGpios) {
+{
+    if (watchGpios) {
         uint32_t now = millis();
 
-        if(now - lastWatchMsec >= WATCH_INTERVAL_MSEC) {
+        if (now - lastWatchMsec >= WATCH_INTERVAL_MSEC) {
             uint64_t curVal = digitalReads(watchGpios);
 
-            if(curVal != previousWatch) {
+            if (curVal != previousWatch) {
                 previousWatch = curVal;
                 DEBUG_MSG("Broadcasting GPIOS 0x%llx changed!\n", curVal);
 
                 // Something changed!  Tell the world with a broadcast message
-                HardwareMessage reply = HardwareMessage_init_default;
+                HardwareMessage r = HardwareMessage_init_default;
-                reply.typ = HardwareMessage_Type_GPIOS_CHANGED;
+                r.typ = HardwareMessage_Type_GPIOS_CHANGED;
-                reply.gpio_value = curVal;
+                r.gpio_value = curVal;
-                MeshPacket *p = allocDataProtobuf(reply);
+                MeshPacket *p = allocDataProtobuf(r);
                 service.sendToMesh(p);
             }
         }
-    }
+    } else {
-    else {
         // No longer watching anything - stop using CPU
         enabled = false;
     }