Radio: Stability Update

SX1262 is now finally stable due to these key changes:
 - Interrupt handling: To circumvent erratum 3.11 level interrupts are utilized.
   Unsure if this was actually an issue as only the DIO1 IRQ is installed,
   but it should be done nonetheless to avoid problems once more GPIO interrupts are used.
 - RadioLib HAL: Use the hooks for beginnig and ending a train of SPI transactions
   to lock and reserve the device, making it more reliable. Also, CS handling is removed
   as RadioLib manages this manually.
 - Board T-Lora Pager: Step down SPI frequency to reasonable rate

App/idf_component.yml

@@ -21,7 +21,7 @@ dependencies:
     rules:
       - if: "target == esp32s3"
   jgromes/radiolib:
-    version: "7.2.1"
+    version: "7.3.0"
     rules:
       - if: "target in [esp32s3, esp32p4]"
 

Boards/LilygoTLoraPager/Source/LilygoTloraPager.cpp

@@ -25,7 +25,7 @@ static DeviceVector createDevices() {
 
     auto sx1262 = std::make_shared<Sx1262>(Sx1262::Configuration{
         .spiHostDevice = SPI2_HOST,
-        .spiFrequency = 10'000'000,
+        .spiFrequency = 4'000'000,
         .csPin = GPIO_NUM_36,
         .resetPin = GPIO_NUM_47,
         .busyPin = GPIO_NUM_48,

Drivers/RadioLibCompat/Source/LevelInterruptHandler.cpp

@@ -0,0 +1,70 @@
+#include "LevelInterruptHandler.h"
+
+#include <esp_attr.h>
+
+void IRAM_ATTR LevelInterruptHandler::handlePositiveEdge(void* isr_arg) {
+    LevelInterruptHandler* self = static_cast<LevelInterruptHandler*>(isr_arg);
+    switch (self->state) {
+        case State::Low:
+            gpio_set_intr_type(self->pin, GPIO_INTR_HIGH_LEVEL);
+            self->state = State::High;
+            break;
+        case State::High:
+            gpio_set_intr_type(self->pin, GPIO_INTR_LOW_LEVEL);
+            self->isr(self->context);
+            self->state = State::Low;
+            break;
+    }
+}
+
+void IRAM_ATTR LevelInterruptHandler::handleNegativeEdge(void* isr_arg) {
+    LevelInterruptHandler* self = static_cast<LevelInterruptHandler*>(isr_arg);
+    switch (self->state) {
+        case State::High:
+            gpio_set_intr_type(self->pin, GPIO_INTR_LOW_LEVEL);
+            self->state = State::Low;
+            break;
+        case State::Low:
+            gpio_set_intr_type(self->pin, GPIO_INTR_HIGH_LEVEL);
+            self->isr(self->context);
+            self->state = State::High;
+                break;
+    }
+}
+
+void IRAM_ATTR LevelInterruptHandler::handleOneshot(void* isr_arg) {
+    LevelInterruptHandler* self = static_cast<LevelInterruptHandler*>(isr_arg);
+    gpio_intr_disable(self->pin);
+    self->isr(self->context);
+}
+
+void LevelInterruptHandler::install() {
+    switch (type) {
+        case Type::HighOneshot:
+            gpio_set_intr_type(pin, GPIO_INTR_HIGH_LEVEL);
+            gpio_isr_handler_add(pin, handleOneshot, this);
+            break;
+        case Type::LowOneshot:
+            gpio_set_intr_type(pin, GPIO_INTR_LOW_LEVEL);
+            gpio_isr_handler_add(pin, handleOneshot, this);
+            break;
+        case Type::PositiveEdge:
+            gpio_set_intr_type(pin, GPIO_INTR_LOW_LEVEL);
+            gpio_isr_handler_add(pin, handlePositiveEdge, this);
+            state = State::Low;
+            break;
+        case Type::NegativeEdge:
+            gpio_set_intr_type(pin, GPIO_INTR_HIGH_LEVEL);
+            gpio_isr_handler_add(pin, handleNegativeEdge, this);
+            state = State::High;
+            break;
+    }
+}
+
+void LevelInterruptHandler::arm() {
+    gpio_intr_enable(pin);
+}
+
+void LevelInterruptHandler::disarm() {
+    gpio_intr_disable(pin);
+}

Drivers/RadioLibCompat/Source/LevelInterruptHandler.h

@@ -0,0 +1,56 @@
+#pragma once
+
+#include <driver/gpio.h>
+
+/**
+ *  An interrupt handler class which can trigger any given IRAM service routine
+ *  for falling/rising edge level changes and oneshot level detection.
+ *  This class is needed if the erratum 3.11 from ESP32 Series SoC Errata Version v2.9 applies.
+ *  In short, once an edge sensitive interrupt is installed, subsequent interrupts may not work.
+ *  This class uses high/low level interrupts to handle both edge detection and oneshot detection of levels
+ *  which circumvents this problem.
+ */
+class LevelInterruptHandler {
+public:
+    enum class Type {
+        HighOneshot,
+        LowOneshot,
+        PositiveEdge,
+        NegativeEdge
+    };
+
+    using ServiceRoutine = void(*)(void* ctx);
+
+private:
+    enum class State {
+        Low,
+        High
+    };
+
+    const gpio_num_t pin;
+    const Type type;
+    const ServiceRoutine isr;
+    void* context;
+    State state;
+
+    static void handlePositiveEdge(void* isr_arg);
+    static void handleNegativeEdge(void* isr_arg);
+    static void handleOneshot(void* isr_arg);
+
+public:
+    LevelInterruptHandler(const gpio_num_t pin, const Type type, ServiceRoutine isr, void* context)
+        : pin(pin)
+        , type(type)
+        , isr(isr)
+        , context(context)
+    {}
+
+    virtual ~LevelInterruptHandler() {
+        disarm();
+    }
+
+
+    void install();
+    void arm();
+    void disarm();
+};

Drivers/RadioLibCompat/Source/RadiolibTactilityHal.cpp

@@ -1,17 +1,17 @@
 #include "RadiolibTactilityHal.h"
 
+#include <Tactility/kernel/Kernel.h>
+
 #include "hal/gpio_hal.h"
 #include "esp_timer.h"
 
 constexpr const char* TAG = "RadiolibTactilityHal";
 
 void RadiolibTactilityHal::init() {
-    // we only need to init the SPI here
     spiBegin();
 }
 
 void RadiolibTactilityHal::term() {
-    // we only need to stop the SPI here
     spiEnd();
 }
 
@@ -24,7 +24,7 @@ void RadiolibTactilityHal::pinMode(uint32_t pin, uint32_t mode) {
     gpiohal.dev = GPIO_LL_GET_HW(GPIO_PORT_0);
 
     gpio_config_t conf = {
-        .pin_bit_mask = (1ULL<<pin),
+        .pin_bit_mask = (1ULL << pin),
         .mode = (gpio_mode_t)mode,
         .pull_up_en = GPIO_PULLUP_DISABLE,
         .pull_down_en = GPIO_PULLDOWN_DISABLE,
@@ -43,61 +43,34 @@ void RadiolibTactilityHal::digitalWrite(uint32_t pin, uint32_t value) {
 
 uint32_t RadiolibTactilityHal::digitalRead(uint32_t pin) {
     if(pin == RADIOLIB_NC) {
-        return(0);
+        return 0;
     }
 
-    return(gpio_get_level((gpio_num_t)pin));
+    return gpio_get_level((gpio_num_t)pin);
 }
 
 void RadiolibTactilityHal::attachInterrupt(uint32_t interruptNum, void (*interruptCb)(void), uint32_t mode) {
-    if(interruptNum == RADIOLIB_NC) {
-        return;
-    }
-
-    if (!isrServiceInitialized) {
-        gpio_install_isr_service((int)ESP_INTR_FLAG_IRAM);
-        isrServiceInitialized = true;
-    }
-    gpio_set_intr_type((gpio_num_t)interruptNum, (gpio_int_type_t)(mode & 0x7));
-
-    // this uses function typecasting, which is not defined when the functions have different signatures
-    // untested and might not work
-    // TODO: I think the wisest course of action is forbidding registration via RadioLib entirely,
-    // as it doesn't suit Tactility with its lack of context passing
-    gpio_isr_handler_add((gpio_num_t)interruptNum, (void (*)(void*))interruptCb, NULL);
+    TT_LOG_E(TAG, "Interrupt registration via RadioLib is not supported!");
 }
 
 void RadiolibTactilityHal::detachInterrupt(uint32_t interruptNum) {
-    if(interruptNum == RADIOLIB_NC) {
-        return;
-    }
-
-    gpio_isr_handler_remove((gpio_num_t)interruptNum);
-    gpio_wakeup_disable((gpio_num_t)interruptNum);
-    gpio_set_intr_type((gpio_num_t)interruptNum, GPIO_INTR_DISABLE);
+    TT_LOG_E(TAG, "Interrupt registration via RadioLib is not supported!");
 }
 
 void RadiolibTactilityHal::delay(unsigned long ms) {
-    vTaskDelay(ms / portTICK_PERIOD_MS);
+    tt::kernel::delayMillis(ms);
 }
 
 void RadiolibTactilityHal::delayMicroseconds(unsigned long us) {
-    uint64_t m = (uint64_t)esp_timer_get_time();
-    if(us) {
-        uint64_t e = (m + us);
-        if(m > e) { // overflow
-            while((uint64_t)esp_timer_get_time() > e);
-        }
-        while((uint64_t)esp_timer_get_time() < e);
-    }
+    tt::kernel::delayMicros(us);
 }
 
 unsigned long RadiolibTactilityHal::millis() {
-    return((unsigned long)(esp_timer_get_time() / 1000ULL));
+    return (unsigned long)(esp_timer_get_time() / 1000ULL);
 }
 
 unsigned long RadiolibTactilityHal::micros() {
-    return((unsigned long)(esp_timer_get_time()));
+    return (unsigned long)(esp_timer_get_time());
 }
 
 long RadiolibTactilityHal::pulseIn(uint32_t pin, uint32_t state, unsigned long timeout) {
@@ -111,52 +84,46 @@ long RadiolibTactilityHal::pulseIn(uint32_t pin, uint32_t state, unsigned long t
 
     while(this->digitalRead(pin) == state) {
         if((this->micros() - curtick) > timeout) {
-            return(0);
+            return 0;
         }
     }
 
-    return(this->micros() - start);
+    return (this->micros() - start);
 }
 
 void RadiolibTactilityHal::spiBegin() {
     if (!spiInitialized) {
-        TT_LOG_I(TAG, "SPI Begin!");
         spi_device_interface_config_t devcfg = {};
         devcfg.clock_speed_hz = spiFrequency;
         devcfg.mode = 0;
-        devcfg.spics_io_num = csPin;
+        // CS is set to unused, as RadioLib sets it manually
+        devcfg.spics_io_num = -1;
         devcfg.queue_size = 1;
         esp_err_t ret = spi_bus_add_device(spiHostDevice, &devcfg, &spiDeviceHandle);
         if (ret != ESP_OK) {
-            TT_LOG_E(TAG, "Failed to add SPI device: %s", esp_err_to_name(ret));
+            TT_LOG_E(TAG, "Failed to add SPI device, error %s", esp_err_to_name(ret));
         }
         spiInitialized = true;
     }
 }
 
 void RadiolibTactilityHal::spiBeginTransaction() {
-    // This function is used to set up the transaction (speed, bit order, mode, ...).
-    // With the ESP-IDF HAL this is automatically done, so no code needed.
+    getLock()->lock();
+    spi_device_acquire_bus(spiDeviceHandle, portMAX_DELAY);
 }
 
 void RadiolibTactilityHal::spiTransfer(uint8_t* out, size_t len, uint8_t* in) {
     spi_transaction_t t;
-
-    auto lock = getLock()->asScopedLock();
-    bool locked = lock.lock(portMAX_DELAY);
-    if (!locked) {
-        TT_LOG_E(TAG, "Failed to aquire SPI lock");
-    }
-
-    memset(&t, 0, sizeof(t));  // Zero out the transaction
-    t.length = len * 8;        // Length is in bits
-    t.tx_buffer = out;         // The data to send
-    t.rx_buffer = in;          // The data to receive
+    memset(&t, 0, sizeof(t));
+    t.length = len * 8;
+    t.tx_buffer = out;
+    t.rx_buffer = in;
     spi_device_polling_transmit(spiDeviceHandle, &t);
 }
 
 void RadiolibTactilityHal::spiEndTransaction() {
-    // nothing needs to be done here
+    spi_device_release_bus(spiDeviceHandle);
+    getLock()->unlock();
 }
 
 void RadiolibTactilityHal::spiEnd() {

Drivers/RadioLibCompat/Source/RadiolibTactilityHal.h

@@ -17,14 +17,12 @@ class RadiolibTactilityHal : public RadioLibHal {
 private:
     spi_host_device_t spiHostDevice;
     int spiFrequency;
-    gpio_num_t csPin;
     spi_device_handle_t spiDeviceHandle;
     std::shared_ptr<tt::Lock> lock;
     bool spiInitialized;
-    bool isrServiceInitialized;
 
 public:
-    explicit RadiolibTactilityHal(spi_host_device_t spiHostDevice, int spiFrequency, gpio_num_t csPin)
+    explicit RadiolibTactilityHal(spi_host_device_t spiHostDevice, int spiFrequency)
     : RadioLibHal(
         GPIO_MODE_INPUT,
         GPIO_MODE_OUTPUT,
@@ -34,10 +32,8 @@ public:
         GPIO_INTR_NEGEDGE)
     , spiHostDevice(spiHostDevice)
     , spiFrequency(spiFrequency)
-    , csPin(csPin)
     , lock(tt::hal::spi::getLock(spiHostDevice))
-    , spiInitialized(false)
-    , isrServiceInitialized(false) {}
+    , spiInitialized(false) {}
 
     void init() override;
     void term() override;

Drivers/SX126x/Source/Sx1262.cpp

@@ -32,8 +32,9 @@ static constexpr Sx1262::ParameterStatus checkValuesAndApply(T &target, const fl
     return Sx1262::ParameterStatus::ValueError;
 }
 
-void IRAM_ATTR dio1handler(void* context) {
-    ((Sx1262*)context)->dio1Event();
+void IRAM_ATTR Sx1262::dio1Isr(void* ctx) {
+    GPIO.out_w1ts = 1 << 9;
+    static_cast<Sx1262*>(ctx)->dio1Event();
 }
 
 Sx1262::ParameterStatus Sx1262::setBaseParameter(const Parameter parameter, const float value) {
@@ -291,35 +292,38 @@ tt::hal::radio::Unit Sx1262::getParameterUnit(const Parameter parameter) const {
 }
 
 void Sx1262::registerDio1Isr() {
-    gpio_hal_context_t gpiohal;
-    gpiohal.dev = GPIO_LL_GET_HW(GPIO_PORT_0);
-
     gpio_config_t conf = {
-        .pin_bit_mask = (1ULL<<configuration.irqPin),
+        .pin_bit_mask = (1ULL << configuration.irqPin),
         .mode = (gpio_mode_t)GPIO_MODE_INPUT,
         .pull_up_en = GPIO_PULLUP_DISABLE,
-        .pull_down_en = GPIO_PULLDOWN_ENABLE,
-        .intr_type = (gpio_int_type_t)gpiohal.dev->pin[configuration.irqPin].int_type,
+        .pull_down_en = GPIO_PULLDOWN_DISABLE,
+        .intr_type = GPIO_INTR_DISABLE,
+    };
+    gpio_config(&conf);
+    // TODO: Debug Code - Remove
+    conf = {
+        .pin_bit_mask = (1ULL<<GPIO_NUM_9),
+        .mode = (gpio_mode_t)GPIO_MODE_OUTPUT,
+        .pull_up_en = GPIO_PULLUP_DISABLE,
+        .pull_down_en = GPIO_PULLDOWN_ENABLE
     };
     gpio_config(&conf);
 
-    // We cannot use the RadioLib API to register this action,
-    // as it does not have the capability to pass an instance pointer via context.
-    // A trampoline has been tried, but is not linkable to be in IRAM_ATTR (dangerous relocation).
-    gpio_install_isr_service((int)ESP_INTR_FLAG_IRAM);
-    gpio_set_intr_type(configuration.irqPin, GPIO_INTR_POSEDGE);
-    gpio_isr_handler_add(configuration.irqPin, dio1handler, this);
+    interrupt.install();
+    interrupt.arm();
 }
 
 void Sx1262::unregisterDio1Isr() {
-    gpio_isr_handler_remove(configuration.irqPin);
-    gpio_wakeup_disable(configuration.irqPin);
-    gpio_set_intr_type(configuration.irqPin, GPIO_INTR_DISABLE);
+    interrupt.disarm();
+}
+
+void Sx1262::armDio1Irq() {
+    gpio_set_level(GPIO_NUM_9, 0);
 }
 
 void IRAM_ATTR Sx1262::dio1Event() {
     static const auto DRAM_ATTR bit = SX1262_DIO1_EVENT_BIT;
-    events.set(bit);
+    auto f = events.set(bit);
 }
 
 void Sx1262::txQueuedSignal() {
@@ -437,21 +441,13 @@ bool Sx1262::doListen() {
     uint16_t rc = RADIOLIB_ERR_NONE;
 
     if (getModulation() != Modulation::LrFhss) {
-        //rc = radio.startReceive(SX1262_RX_TIMEOUT_MILLIS);
-        rc = radio.startReceive();
+        armDio1Irq();
+        rc = radio.startReceiveDutyCycleAuto(preambleLength, 0, SX1262_IRQ_FLAGS);
         if (rc == RADIOLIB_ERR_NONE) {
-            auto flags = events.wait(SX1262_INTERRUPT_BIT | SX1262_DIO1_EVENT_BIT | SX1262_QUEUED_TX_BIT, tt::EventFlag::WaitAny,
-                                     pdMS_TO_TICKS(SX1262_RX_TIMEOUT_MILLIS));
-            if (!(flags & SX1262_DIO1_EVENT_BIT)) {
-                TT_LOG_D(TAG, "SX1262 DIO RX Timeout");
-                rc = radio.standby();
-                if (rc != RADIOLIB_ERR_NONE) {
-                    TT_LOG_W(TAG, "RadioLib returned %hi on RX standby", rc);
-                }
-                return false;
-            } else {
-                return true;
-            }
+            auto flags = events.wait(SX1262_INTERRUPT_BIT | SX1262_DIO1_EVENT_BIT | SX1262_QUEUED_TX_BIT);
+            return (flags & SX1262_DIO1_EVENT_BIT) != 0;
+        } else {
+            TT_LOG_E(TAG, "Error setting dutycycle RX, RadioLib returned %hi", rc);
         }
         return false;
     } else {
@@ -462,12 +458,19 @@ bool Sx1262::doListen() {
 }
 
 void Sx1262::doReceive() {
+    uint16_t rc = RADIOLIB_ERR_NONE;
+
     // LR-FHSS modem only supports TX
     if (getModulation() == Modulation::LrFhss) return;
+    /*
+    rc = radio.standby();
+    if (rc != RADIOLIB_ERR_NONE) {
+        TT_LOG_W(TAG, "RadioLib returned %hi on TX standby", rc);
+    }*/
 
     uint16_t rxSize = radio.getPacketLength(true);
     std::vector<uint8_t> data(rxSize);
-    uint16_t rc = radio.readData(data.data(), rxSize);
+    rc = radio.readData(data.data(), rxSize);
     if (rc != RADIOLIB_ERR_NONE) {
         TT_LOG_E(TAG, "Error receiving data, RadioLib returned %hi", rc);
     } else if(rxSize == 0) {
@@ -488,6 +491,6 @@ void Sx1262::doReceive() {
     }
 
     // A delay before a new command improves reliability
-    vTaskDelay(pdMS_TO_TICKS(SX1262_COOLDOWN_MILLIS));
+    //vTaskDelay(pdMS_TO_TICKS(SX1262_COOLDOWN_MILLIS));
 }
 

Drivers/SX126x/Source/Sx1262.h

@@ -7,6 +7,7 @@
 #include <RadioLib.h>
 #include "RadiolibTactilityHal.h"
 #include "RadiolibThreadedDevice.h"
+#include "LevelInterruptHandler.h"
 
 #include <utility>
 
@@ -32,17 +33,17 @@ private:
     static constexpr auto SX1262_INTERRUPT_BIT = BIT0;
     static constexpr auto SX1262_DIO1_EVENT_BIT = BIT1;
     static constexpr auto SX1262_QUEUED_TX_BIT = BIT2;
+    static constexpr auto SX1262_IRQ_FLAGS = (RADIOLIB_IRQ_RX_DEFAULT_FLAGS);
 
-    std::string name;
     const Configuration configuration;
-    std::shared_ptr<tt::Lock> lock;
+    LevelInterruptHandler interrupt;
+    std::string name;
     tt::EventFlag events;
     RadiolibTactilityHal hal;
     Module radioModule;
     SX1262 radio;
     TxItem currentTx;
 
-    // Shared parameters which have a common lowest value are set here
     int8_t power = -9.0;
     float frequency = 150;
     float bandwidth = 0.0;
@@ -57,6 +58,7 @@ private:
 
     void registerDio1Isr();
     void unregisterDio1Isr();
+    void armDio1Irq();
 
     ParameterStatus setBaseParameter(const Parameter parameter, const float value);
     ParameterStatus setLoraParameter(const Parameter parameter, const float value);
@@ -69,6 +71,8 @@ private:
 
 
 protected:
+    static void dio1Isr(void* ctx);
+
     virtual void txQueuedSignal() override;
     virtual void interruptSignal() override;
 
@@ -82,17 +86,18 @@ public:
 
     explicit Sx1262(const Configuration& configuration, const std::string& name = SX1262_DEFAULT_NAME)
         : RadiolibThreadedDevice(name, 4096)
-        , name(name)
         , configuration(configuration)
-        , hal(configuration.spiHostDevice, configuration.spiFrequency, configuration.csPin)
-        , radioModule(&hal, configuration.csPin, configuration.irqPin, configuration.resetPin, configuration.busyPin)
+        , interrupt(configuration.irqPin, LevelInterruptHandler::Type::PositiveEdge, dio1Isr, this)
+        , name(name)
+        , hal(configuration.spiHostDevice, configuration.spiFrequency)
+        , radioModule(&hal, configuration.csPin, RADIOLIB_NC, configuration.resetPin, configuration.busyPin)
         , radio(&radioModule)
         {}
 
     ~Sx1262() override = default;
 
     std::string getName() const override { return name; }
-    std::string getDescription() const override { return "Semtech SX1262 LoRa, FSK and LR-FHSS capable radio"; }
+    std::string getDescription() const override { return "Semtech SX1262 LoRa and (G)FSK capable radio"; }
 
     ParameterStatus setParameter(const Parameter parameter, const float value) override;
     ParameterStatus getParameter(const Parameter parameter, float &value) const override;