From 78470ed3f59f5c84fbd1325bcff1fd95b2b20183 Mon Sep 17 00:00:00 2001 From: geeksville Date: Sat, 18 Apr 2020 08:48:03 -0700 Subject: [PATCH] fix #97, we need the RF95 IRQ to be level triggered, or we have slim chance of missing events --- src/rf95/RH_RF95.cpp | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/rf95/RH_RF95.cpp b/src/rf95/RH_RF95.cpp index c29929637..3929156f5 100644 --- a/src/rf95/RH_RF95.cpp +++ b/src/rf95/RH_RF95.cpp @@ -115,11 +115,11 @@ bool RH_RF95::init() } _deviceForInterrupt[_myInterruptIndex] = this; if (_myInterruptIndex == 0) - attachInterrupt(interruptNumber, isr0, RISING); + attachInterrupt(interruptNumber, isr0, ONHIGH); else if (_myInterruptIndex == 1) - attachInterrupt(interruptNumber, isr1, RISING); + attachInterrupt(interruptNumber, isr1, ONHIGH); else if (_myInterruptIndex == 2) - attachInterrupt(interruptNumber, isr2, RISING); + attachInterrupt(interruptNumber, isr2, ONHIGH); else return false; // Too many devices, not enough interrupt vectors @@ -153,6 +153,17 @@ 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 + // 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 // multiple flags might be set. Handle them all @@ -170,8 +181,6 @@ void RH_RF95::handleInterrupt() // 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; - spiWrite(RH_RF95_REG_12_IRQ_FLAGS, 0xff); // Clear all IRQ flags, required before reading fifo (according to datasheet) - if (!crc_present) { _rxBad++; clearRxBuf(); @@ -218,15 +227,6 @@ void RH_RF95::handleInterrupt() _cad = irq_flags & RH_RF95_CAD_DETECTED; setModeIdle(); } - - // ack all interrupts, note - we did this already in the RX_DONE case above, and we don't want to do it twice - if (!(irq_flags & RH_RF95_RX_DONE)) { - // 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: 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 - } } // These are low level functions that call the interrupt handler for the correct