Fix #99: move spi ISR operations into helper thread. SPI from ISR is bad!

src/main.cpp

@@ -336,9 +336,9 @@ void loop()
 {
     uint32_t msecstosleep = 1000 * 30; // How long can we sleep before we again need to service the main loop?
 
-    powerFSM.run_machine();
     gps.loop();
     router.loop();
+    powerFSM.run_machine();
     service.loop();
 
     ledPeriodic.loop();

src/rf95/CustomRF95.cpp

@@ -78,13 +78,12 @@ void CustomRF95::handleInterrupt()
 {
     RH_RF95::handleInterrupt();
 
-    BaseType_t higherPriWoken = false;
     if (_mode == RHModeIdle) // We are now done sending or receiving
     {
         if (sendingPacket) // Were we sending?
         {
             // We are done sending that packet, release it
-            packetPool.releaseFromISR(sendingPacket, &higherPriWoken);
+            packetPool.release(sendingPacket);
             sendingPacket = NULL;
             // DEBUG_MSG("Done with send\n");
         }
@@ -123,43 +122,35 @@ void CustomRF95::handleInterrupt()
             }
 
             if (!pb_decode_from_bytes(payload, payloadLen, SubPacket_fields, p)) {
-                packetPool.releaseFromISR(mp, &higherPriWoken);
+                packetPool.release(mp);
             } else {
                 // parsing was successful, queue for our recipient
                 mp->has_payload = true;
 
-                deliverToReceiverISR(mp, &higherPriWoken);
+                deliverToReceiver(mp);
             }
 
             clearRxBuf(); // This message accepted and cleared
         }
 
-        higherPriWoken |= handleIdleISR();
+        handleIdleISR();
     }
-
-    // If we call this _IT WILL NOT RETURN_
-    if (higherPriWoken)
-        portYIELD_FROM_ISR();
 }
 
 /** The ISR doesn't have any good work to do, give a new assignment.
  *
  * Return true if a higher pri task has woken
  */
-bool CustomRF95::handleIdleISR()
+void CustomRF95::handleIdleISR()
 {
-    BaseType_t higherPriWoken = false;
-
     // First send any outgoing packets we have ready
-    MeshPacket *txp = txQueue.dequeuePtrFromISR(0);
+    MeshPacket *txp = txQueue.dequeuePtr(0);
     if (txp)
         startSend(txp);
     else {
         // Nothing to send, let's switch back to receive mode
         setModeRx();
     }
-
-    return higherPriWoken;
 }
 
 /// This routine might be called either from user space or ISR
@@ -197,6 +188,8 @@ void CustomRF95::startSend(MeshPacket *txp)
 
 void CustomRF95::loop()
 {
+    RH_RF95::loop();
+
     // It should never take us more than 30 secs to send a packet, if it does, we have a bug, FIXME, move most of this
     // into CustomRF95
     uint32_t now = millis();

src/rf95/CustomRF95.h

@@ -52,5 +52,5 @@ class CustomRF95 : public RH_RF95, public RadioInterface
     void startSend(MeshPacket *txp);
 
     /// Return true if a higher pri task has woken
-    bool handleIdleISR();
+    void handleIdleISR();
 };

src/rf95/RH_RF95.cpp

@@ -34,16 +34,12 @@ bool RH_RF95::init()
     if (!RHSPIDriver::init())
         return false;
 
-    // Determine the interrupt number that corresponds to the interruptPin
-    int interruptNumber = digitalPinToInterrupt(_interruptPin);
-    if (interruptNumber == NOT_AN_INTERRUPT)
-        return false;
 #ifdef RH_ATTACHINTERRUPT_TAKES_PIN_NUMBER
     interruptNumber = _interruptPin;
 #endif
 
     // Tell the low level SPI interface we will use SPI within this interrupt
-    spiUsingInterrupt(interruptNumber);
+    // spiUsingInterrupt(interruptNumber);
 
     // No way to check the device type :-(
 
@@ -114,6 +110,17 @@ bool RH_RF95::init()
             return false; // Too many devices, not enough interrupt vectors
     }
     _deviceForInterrupt[_myInterruptIndex] = this;
+
+    return enableInterrupt();
+}
+
+bool RH_RF95::enableInterrupt()
+{
+    // Determine the interrupt number that corresponds to the interruptPin
+    int interruptNumber = digitalPinToInterrupt(_interruptPin);
+    if (interruptNumber == NOT_AN_INTERRUPT)
+        return false;
+
     if (_myInterruptIndex == 0)
         attachInterrupt(interruptNumber, isr0, ONHIGH);
     else if (_myInterruptIndex == 1)
@@ -126,6 +133,12 @@ bool RH_RF95::init()
     return true;
 }
 
+void RH_INTERRUPT_ATTR RH_RF95::disableInterrupt()
+{
+    int interruptNumber = digitalPinToInterrupt(_interruptPin);
+    detachInterrupt(interruptNumber);
+}
+
 void RH_RF95::prepareDeepSleep()
 {
     // Determine the interrupt number that corresponds to the interruptPin
@@ -143,6 +156,13 @@ bool RH_RF95::isReceiving()
                                         RH_RF95_MODEM_STATUS_HEADER_INFO_VALID)) != 0;
 }
 
+void RH_INTERRUPT_ATTR RH_RF95::handleInterruptLevel0()
+{
+    disableInterrupt(); // Disable our interrupt until our helper thread can run (because the IRQ will remain asserted until we
+                        // talk to it via SPI)
+    pendingInterrupt = true;
+}
+
 // C++ level interrupt handler for this instance
 // LORA is unusual in that it has several interrupt lines, and not a single, combined one.
 // On MiniWirelessLoRa, only one of the several interrupt lines (DI0) from the RFM95 is usefuly
@@ -222,6 +242,16 @@ void RH_RF95::handleInterrupt()
         _cad = irq_flags & RH_RF95_CAD_DETECTED;
         setModeIdle();
     }
+
+    enableInterrupt(); // Let ISR run again
+}
+
+void RH_RF95::loop()
+{
+    while (pendingInterrupt) {
+        pendingInterrupt = false; // If the flag was set, it is _guaranteed_ the ISR won't be running, because it masked itself
+        handleInterrupt();
+    }
 }
 
 // These are low level functions that call the interrupt handler for the correct
@@ -230,17 +260,17 @@ void RH_RF95::handleInterrupt()
 void RH_INTERRUPT_ATTR RH_RF95::isr0()
 {
     if (_deviceForInterrupt[0])
-        _deviceForInterrupt[0]->handleInterrupt();
+        _deviceForInterrupt[0]->handleInterruptLevel0();
 }
 void RH_INTERRUPT_ATTR RH_RF95::isr1()
 {
     if (_deviceForInterrupt[1])
-        _deviceForInterrupt[1]->handleInterrupt();
+        _deviceForInterrupt[1]->handleInterruptLevel0();
 }
 void RH_INTERRUPT_ATTR RH_RF95::isr2()
 {
     if (_deviceForInterrupt[2])
-        _deviceForInterrupt[2]->handleInterrupt();
+        _deviceForInterrupt[2]->handleInterruptLevel0();
 }
 
 // Check whether the latest received message is complete and uncorrupted

src/rf95/RH_RF95.h

@@ -806,12 +806,17 @@ class RH_RF95 : public RHSPIDriver
     /// Return true if we are currently receiving a packet
     bool isReceiving();
 
+    void loop(); // Perform idle processing
+
   protected:
     /// This is a low level function to handle the interrupts for one instance of RH_RF95.
     /// Called automatically by isr*()
     /// Should not need to be called by user code.
     virtual void handleInterrupt();
 
+    /// This is the only code called in ISR context, it just queues up our helper thread to run handleInterrupt();
+    void RH_INTERRUPT_ATTR handleInterruptLevel0();
+
     /// Examine the revceive buffer to determine whether the message is for this node
     void validateRxBuf();
 
@@ -846,6 +851,11 @@ class RH_RF95 : public RHSPIDriver
     /// Index of next interrupt number to use in _deviceForInterrupt
     static uint8_t _interruptCount;
 
+    bool enableInterrupt();  // enable our IRQ
+    void disableInterrupt(); // disable our IRQ
+
+    volatile bool pendingInterrupt = false;
+    
     /// The configured interrupt pin connected to this instance
     uint8_t _interruptPin;
 

src/rf95/RadioInterface.cpp

@@ -15,8 +15,8 @@ ErrorCode SimRadio::send(MeshPacket *p)
     return ERRNO_OK;
 }
 
-void RadioInterface::deliverToReceiverISR(MeshPacket *p, BaseType_t *higherPriWoken)
+void RadioInterface::deliverToReceiver(MeshPacket *p)
 {
     assert(rxDest);
-    assert(rxDest->enqueueFromISR(p, higherPriWoken)); // NOWAIT - fixme, if queue is full, delete older messages
+    assert(rxDest->enqueue(p, 0)); // NOWAIT - fixme, if queue is full, delete older messages
 }

src/rf95/RadioInterface.h

@@ -24,7 +24,7 @@ class RadioInterface
     /**
      * Enqueue a received packet for the registered receiver
      */
-    void deliverToReceiverISR(MeshPacket *p, BaseType_t *higherPriWoken);
+    void deliverToReceiver(MeshPacket *p);
 
   public:
     /** pool is the pool we will alloc our rx packets from