From 20b41836e255d14ab27aeed7d24ebbddedfa7119 Mon Sep 17 00:00:00 2001 From: geeksville Date: Sat, 18 Apr 2020 09:22:08 -0700 Subject: [PATCH 1/3] clarify log msg --- src/rf95/CustomRF95.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rf95/CustomRF95.cpp b/src/rf95/CustomRF95.cpp index 62c4ff8b..697800e5 100644 --- a/src/rf95/CustomRF95.cpp +++ b/src/rf95/CustomRF95.cpp @@ -20,7 +20,7 @@ bool CustomRF95::canSleep() bool res = (_mode == RHModeInitialising || _mode == RHModeIdle || _mode == RHModeRx) && !isRx && txQueue.isEmpty(); if (!res) // only print debug messages if we are vetoing sleep - DEBUG_MSG("canSleep, mode=%d, isRx=%d, txEmpty=%d, txGood=%d\n", _mode, isRx, txQueue.isEmpty(), _txGood); + DEBUG_MSG("radio wait to sleep, mode=%d, isRx=%d, txEmpty=%d, txGood=%d\n", _mode, isRx, txQueue.isEmpty(), _txGood); return res; } From e5f9a752d8cc5fb67d726d6a9eeade1677b180f2 Mon Sep 17 00:00:00 2001 From: geeksville Date: Sat, 18 Apr 2020 09:22:26 -0700 Subject: [PATCH 2/3] fix comments and cleanup ISR code --- src/rf95/RH_RF95.cpp | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/rf95/RH_RF95.cpp b/src/rf95/RH_RF95.cpp index 3929156f..9810428e 100644 --- a/src/rf95/RH_RF95.cpp +++ b/src/rf95/RH_RF95.cpp @@ -153,15 +153,14 @@ void RH_RF95::handleInterrupt() // Read the interrupt register uint8_t irq_flags = spiRead(RH_RF95_REG_12_IRQ_FLAGS); - // ack all interrupts, note - we did this already in the RX_DONE case above, and we don't want to do it twice + // ack all interrupts + // note from radiohead author wrt old code (with IMO wrong fix) // Sigh: on some processors, for some unknown reason, doing this only once does not actually // clear the radio's interrupt flag. So we do it twice. Why? (kevinh - I think the root cause we want level // triggered interrupts here - not edge. Because edge allows us to miss handling secondard interrupts that occurred // while this ISR was running. Better to instead, configure the interrupts as level triggered and clear pending // at the _beginning_ of the ISR. If any interrupts occur while handling the ISR, the signal will remain asserted and // our ISR will be reinvoked to handle that case) - // kevinh: turn this off until root cause is known, because it can cause missed interrupts! - // spiWrite(RH_RF95_REG_12_IRQ_FLAGS, 0xff); // Clear all IRQ flags spiWrite(RH_RF95_REG_12_IRQ_FLAGS, 0xff); // Clear all IRQ flags // Note: there can be substantial latency between ISR assertion and this function being run, therefore @@ -169,14 +168,10 @@ void RH_RF95::handleInterrupt() // Note: we are running the chip in continuous receive mode (currently, so RX_TIMEOUT shouldn't ever occur) bool haveRxError = irq_flags & (RH_RF95_RX_TIMEOUT | RH_RF95_PAYLOAD_CRC_ERROR); - if (haveRxError) - // if (_mode == RHModeRx && irq_flags & (RH_RF95_RX_TIMEOUT | RH_RF95_PAYLOAD_CRC_ERROR)) - { + if (haveRxError) { _rxBad++; clearRxBuf(); - } - - if ((irq_flags & RH_RF95_RX_DONE) && !haveRxError) { + } else if (irq_flags & RH_RF95_RX_DONE) { // Read the RegHopChannel register to check if CRC presence is signalled // in the header. If not it might be a stray (noise) packet.* uint8_t crc_present = spiRead(RH_RF95_REG_1C_HOP_CHANNEL) & RH_RF95_RX_PAYLOAD_CRC_IS_ON; From db766f18edb4fd9e4b1b5f7da81010f324e5d387 Mon Sep 17 00:00:00 2001 From: geeksville Date: Sat, 18 Apr 2020 14:22:24 -0700 Subject: [PATCH 3/3] Fix #99: move spi ISR operations into helper thread. SPI from ISR is bad! --- src/main.cpp | 2 +- src/rf95/CustomRF95.cpp | 23 +++++++------------ src/rf95/CustomRF95.h | 2 +- src/rf95/RH_RF95.cpp | 46 ++++++++++++++++++++++++++++++------- src/rf95/RH_RF95.h | 10 ++++++++ src/rf95/RadioInterface.cpp | 4 ++-- src/rf95/RadioInterface.h | 2 +- 7 files changed, 61 insertions(+), 28 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 0c95aaf5..b39e61fc 100644 --- a/src/main.cpp +++ b/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(); diff --git a/src/rf95/CustomRF95.cpp b/src/rf95/CustomRF95.cpp index 697800e5..7005a9ce 100644 --- a/src/rf95/CustomRF95.cpp +++ b/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(); diff --git a/src/rf95/CustomRF95.h b/src/rf95/CustomRF95.h index a424f249..ad799648 100644 --- a/src/rf95/CustomRF95.h +++ b/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(); }; \ No newline at end of file diff --git a/src/rf95/RH_RF95.cpp b/src/rf95/RH_RF95.cpp index 9810428e..14a2f182 100644 --- a/src/rf95/RH_RF95.cpp +++ b/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 diff --git a/src/rf95/RH_RF95.h b/src/rf95/RH_RF95.h index 7a4a34a6..3e077603 100644 --- a/src/rf95/RH_RF95.h +++ b/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; diff --git a/src/rf95/RadioInterface.cpp b/src/rf95/RadioInterface.cpp index bd8bb131..d43922ff 100644 --- a/src/rf95/RadioInterface.cpp +++ b/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 } \ No newline at end of file diff --git a/src/rf95/RadioInterface.h b/src/rf95/RadioInterface.h index 1172745f..ec7dfce4 100644 --- a/src/rf95/RadioInterface.h +++ b/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