From 882e216f8a80d87c4f84cee8de32836192f9ab66 Mon Sep 17 00:00:00 2001 From: Dominic Clifton Date: Sun, 23 Jan 2022 17:11:35 +0100 Subject: [PATCH 1/3] RX SPI - Fix driver using the NVIC priority assigned to the MPU. --- src/main/drivers/nvic.h | 1 + src/main/drivers/rx/rx_spi.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/drivers/nvic.h b/src/main/drivers/nvic.h index 20c9fd04ee..976a6555d5 100644 --- a/src/main/drivers/nvic.h +++ b/src/main/drivers/nvic.h @@ -30,6 +30,7 @@ #define NVIC_PRIO_TRANSPONDER_DMA NVIC_BUILD_PRIORITY(3, 0) #define NVIC_PRIO_MPU_INT_EXTI NVIC_BUILD_PRIORITY(0x0f, 0x0f) #define NVIC_PRIO_MAG_INT_EXTI NVIC_BUILD_PRIORITY(0x0f, 0x0f) +#define NVIC_PRIO_RX_SPI_INT_EXTI NVIC_BUILD_PRIORITY(0x0f, 0x0f) #define NVIC_PRIO_WS2811_DMA NVIC_BUILD_PRIORITY(1, 2) // TODO - is there some reason to use high priority? (or to use DMA IRQ at all?) #define NVIC_PRIO_SERIALUART_TXDMA NVIC_BUILD_PRIORITY(1, 1) // Highest of all SERIALUARTx_TXDMA #define NVIC_PRIO_SERIALUART1_TXDMA NVIC_BUILD_PRIORITY(1, 1) diff --git a/src/main/drivers/rx/rx_spi.c b/src/main/drivers/rx/rx_spi.c index 58d11953cd..8dfa611ca2 100644 --- a/src/main/drivers/rx/rx_spi.c +++ b/src/main/drivers/rx/rx_spi.c @@ -125,7 +125,7 @@ void rxSpiExtiInit(ioConfig_t rxSpiExtiPinConfig, extiTrigger_t rxSpiExtiPinTrig extiLevel = false; } EXTIHandlerInit(&rxSpiExtiCallbackRec, rxSpiExtiHandler); - EXTIConfig(extiPin, &rxSpiExtiCallbackRec, NVIC_PRIO_MPU_INT_EXTI, rxSpiExtiPinConfig, rxSpiExtiPinTrigger); + EXTIConfig(extiPin, &rxSpiExtiCallbackRec, NVIC_PRIO_RX_SPI_INT_EXTI, rxSpiExtiPinConfig, rxSpiExtiPinTrigger); EXTIEnable(extiPin, true); } } From 3c5a5728b228de81fda161efefdf4377ba99b57a Mon Sep 17 00:00:00 2001 From: Dominic Clifton Date: Sun, 23 Jan 2022 17:46:23 +0100 Subject: [PATCH 2/3] ExpressLRS - Fix an edge-case when a receiver can have both of it's ISR flags set. This has not been observed on the bench but is more of a safeguard so that the task doesn't stall and can decide what to do. --- src/main/drivers/rx/rx_sx127x.c | 4 +++- src/main/drivers/rx/rx_sx1280.c | 5 ++++- src/main/rx/expresslrs.c | 4 +++- src/main/rx/expresslrs_impl.h | 3 ++- 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/main/drivers/rx/rx_sx127x.c b/src/main/drivers/rx/rx_sx127x.c index aa85fd13aa..3bc605e381 100644 --- a/src/main/drivers/rx/rx_sx127x.c +++ b/src/main/drivers/rx/rx_sx127x.c @@ -445,7 +445,9 @@ uint8_t sx127xGetIrqReason(void) { uint8_t irqFlags = sx127xGetIrqFlags(); sx127xClearIrqFlags(); - if ((irqFlags & SX127X_CLEAR_IRQ_FLAG_TX_DONE)) { + if ((irqFlags & SX127X_CLEAR_IRQ_FLAG_TX_DONE) && (irqFlags & SX127X_CLEAR_IRQ_FLAG_RX_DONE)) { + return 3; + } else if ((irqFlags & SX127X_CLEAR_IRQ_FLAG_TX_DONE)) { return 2; } else if ((irqFlags & SX127X_CLEAR_IRQ_FLAG_RX_DONE)) { return 1; diff --git a/src/main/drivers/rx/rx_sx1280.c b/src/main/drivers/rx/rx_sx1280.c index 7d35d88f2e..7d4c141866 100644 --- a/src/main/drivers/rx/rx_sx1280.c +++ b/src/main/drivers/rx/rx_sx1280.c @@ -418,8 +418,11 @@ void sx1280ClearIrqStatus(const uint16_t irqMask) uint8_t sx1280GetIrqReason(void) { uint16_t irqStatus = sx1280GetIrqStatus(); + sx1280ClearIrqStatus(SX1280_IRQ_RADIO_ALL); - if ((irqStatus & SX1280_IRQ_TX_DONE)) { + if ((irqStatus & SX1280_IRQ_TX_DONE) && (irqStatus & SX1280_IRQ_RX_DONE)) { + return 3; + } else if ((irqStatus & SX1280_IRQ_TX_DONE)) { return 2; } else if ((irqStatus & SX1280_IRQ_RX_DONE)) { return 1; diff --git a/src/main/rx/expresslrs.c b/src/main/rx/expresslrs.c index 4991b06e5a..d5d5ddbccc 100644 --- a/src/main/rx/expresslrs.c +++ b/src/main/rx/expresslrs.c @@ -1073,7 +1073,9 @@ rx_spi_received_e expressLrsDataReceived(uint8_t *payload) } uint8_t irqReason = receiver.rxISR(&isrTimeStampUs); - if (irqReason == ELRS_DIO_TX_DONE) { + if (irqReason == ELRS_DIO_RX_AND_TX_DONE) { + startReceiving(); + } else if (irqReason == ELRS_DIO_TX_DONE) { startReceiving(); } else if (irqReason == ELRS_DIO_RX_DONE) { result = processRFPacket(payload, isrTimeStampUs); diff --git a/src/main/rx/expresslrs_impl.h b/src/main/rx/expresslrs_impl.h index 2655cf2c99..fdc6d85484 100644 --- a/src/main/rx/expresslrs_impl.h +++ b/src/main/rx/expresslrs_impl.h @@ -33,7 +33,8 @@ typedef enum { typedef enum { ELRS_DIO_UNKNOWN = 0, ELRS_DIO_RX_DONE = 1, - ELRS_DIO_TX_DONE = 2 + ELRS_DIO_TX_DONE = 2, + ELRS_DIO_RX_AND_TX_DONE = 3, } dioReason_e; typedef enum { From a91166bda0d2844fc7713519de2d6759b627208d Mon Sep 17 00:00:00 2001 From: Dominic Clifton Date: Sun, 23 Jan 2022 17:28:19 +0100 Subject: [PATCH 3/3] ExpressLRS - Fix data-race in ISR handling that was observed that caused RX loss. Likely caused by RX task running *very* late and a new EXTI flag being set, and then being immediately cleared without processing it. More likely to happen on the bench than in the air due to task latency caused by USB activity. --- src/main/drivers/rx/rx_sx127x.c | 25 ++++++++++++++++++------- src/main/drivers/rx/rx_sx1280.c | 26 ++++++++++++++++++-------- 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/src/main/drivers/rx/rx_sx127x.c b/src/main/drivers/rx/rx_sx127x.c index 3bc605e381..99172191f7 100644 --- a/src/main/drivers/rx/rx_sx127x.c +++ b/src/main/drivers/rx/rx_sx127x.c @@ -31,9 +31,12 @@ #ifdef USE_RX_SX127X +#include "build/atomic.h" + #include "drivers/bus_spi.h" #include "drivers/io.h" #include "drivers/io_impl.h" +#include "drivers/nvic.h" #include "drivers/rx/rx_sx127x.h" #include "drivers/rx/rx_spi.h" #include "drivers/time.h" @@ -73,15 +76,23 @@ static bool sx127xDetectChip(void) uint8_t sx127xISR(timeUs_t *timeStamp) { - if (rxSpiPollExti()) { - if (rxSpiGetLastExtiTimeUs()) { - *timeStamp = rxSpiGetLastExtiTimeUs(); + bool extiTriggered = false; + timeUs_t extiTimestamp; + + ATOMIC_BLOCK(NVIC_PRIO_RX_SPI_INT_EXTI) { + // prevent a data-race that can occur if a new EXTI ISR occurs during this block. + extiTriggered = rxSpiPollExti(); + extiTimestamp = rxSpiGetLastExtiTimeUs(); + if (extiTriggered) { + rxSpiResetExti(); } + } - uint8_t irqReason; - irqReason = sx127xGetIrqReason(); - - rxSpiResetExti(); + if (extiTriggered) { + uint8_t irqReason = sx127xGetIrqReason(); + if (extiTimestamp) { + *timeStamp = extiTimestamp; + } return irqReason; } diff --git a/src/main/drivers/rx/rx_sx1280.c b/src/main/drivers/rx/rx_sx1280.c index 7d4c141866..89415760fb 100644 --- a/src/main/drivers/rx/rx_sx1280.c +++ b/src/main/drivers/rx/rx_sx1280.c @@ -32,9 +32,12 @@ #ifdef USE_RX_SX1280 +#include "build/atomic.h" + #include "drivers/bus_spi.h" #include "drivers/io.h" #include "drivers/io_impl.h" +#include "drivers/nvic.h" #include "drivers/rx/rx_sx1280.h" #include "drivers/rx/rx_spi.h" #include "drivers/time.h" @@ -103,16 +106,23 @@ bool sx1280Init(IO_t resetPin, IO_t busyPin) uint8_t sx1280ISR(timeUs_t *timeStamp) { - if (rxSpiPollExti()) { - if (rxSpiGetLastExtiTimeUs()) { - *timeStamp = rxSpiGetLastExtiTimeUs(); + bool extiTriggered = false; + timeUs_t extiTimestamp; + + ATOMIC_BLOCK(NVIC_PRIO_RX_SPI_INT_EXTI) { + // prevent a data-race that can occur if a new EXTI ISR occurs during this block. + extiTriggered = rxSpiPollExti(); + extiTimestamp = rxSpiGetLastExtiTimeUs(); + if (extiTriggered) { + rxSpiResetExti(); } + } - uint8_t irqReason; - irqReason = sx1280GetIrqReason(); - - rxSpiResetExti(); - + if (extiTriggered) { + uint8_t irqReason = sx1280GetIrqReason(); + if (extiTimestamp) { + *timeStamp = extiTimestamp; + } return irqReason; } return 0;