lora: Improve IRQ handling

* Interrupt handler only triggers a waker:
Do the actual interrupt processing which involves SUBGHZ SPI coms in the task.
* Do not require a static state for the constructor.
* Remove unsafe from construcor.

embassy-lora/src/stm32wl/mod.rs

@@ -1,18 +1,18 @@
 //! A radio driver integration for the radio found on STM32WL family devices.
 use core::future::Future;
-use core::mem::MaybeUninit;
+use core::task::Poll;
 
-use embassy_hal_common::{into_ref, PeripheralRef};
+use embassy_hal_common::{into_ref, Peripheral, PeripheralRef};
 use embassy_stm32::dma::NoDma;
 use embassy_stm32::gpio::{AnyPin, Output};
-use embassy_stm32::interrupt::{InterruptExt, SUBGHZ_RADIO};
+use embassy_stm32::interrupt::{Interrupt, InterruptExt, SUBGHZ_RADIO};
 use embassy_stm32::subghz::{
     CalibrateImage, CfgIrq, CodingRate, Error, HeaderType, Irq, LoRaBandwidth, LoRaModParams, LoRaPacketParams,
     LoRaSyncWord, Ocp, PaConfig, PaSel, PacketType, RampTime, RegMode, RfFreq, SpreadingFactor as SF, StandbyClk,
     Status, SubGhz, TcxoMode, TcxoTrim, Timeout, TxParams,
 };
-use embassy_stm32::Peripheral;
+use embassy_sync::waitqueue::AtomicWaker;
-use embassy_sync::signal::Signal;
+use futures::future::poll_fn;
 use lorawan_device::async_device::radio::{Bandwidth, PhyRxTx, RfConfig, RxQuality, SpreadingFactor, TxConfig};
 use lorawan_device::async_device::Timings;
 
@@ -28,65 +28,43 @@ pub enum State {
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
 pub struct RadioError;
 
-static IRQ: Signal<(Status, u16)> = Signal::new();
+static IRQ_WAKER: AtomicWaker = AtomicWaker::new();
-
-struct StateInner<'d> {
-    radio: SubGhz<'d, NoDma, NoDma>,
-    switch: RadioSwitch<'d>,
-}
-
-/// External state storage for the radio state
-pub struct SubGhzState<'a>(MaybeUninit<StateInner<'a>>);
-impl<'d> SubGhzState<'d> {
-    pub const fn new() -> Self {
-        Self(MaybeUninit::uninit())
-    }
-}
 
 /// The radio peripheral keeping the radio state and owning the radio IRQ.
-pub struct SubGhzRadio<'d> {
+pub struct SubGhzRadio<'d, RS> {
-    state: *mut StateInner<'d>,
+    radio: SubGhz<'d, NoDma, NoDma>,
-    _irq: PeripheralRef<'d, SUBGHZ_RADIO>,
+    switch: RS,
+    irq: PeripheralRef<'d, SUBGHZ_RADIO>,
 }
 
-impl<'d> SubGhzRadio<'d> {
+#[derive(Default)]
+#[non_exhaustive]
+pub struct SubGhzRadioConfig {
+    pub reg_mode: RegMode,
+    pub calibrate_image: CalibrateImage,
+}
+
+impl<'d, RS: RadioSwitch> SubGhzRadio<'d, RS> {
     /// Create a new instance of a SubGhz radio for LoRaWAN.
-    ///
+    pub fn new(
-    /// # Safety
+        mut radio: SubGhz<'d, NoDma, NoDma>,
-    /// Do not leak self or futures
+        switch: RS,
-    pub unsafe fn new(
-        state: &'d mut SubGhzState<'d>,
-        radio: SubGhz<'d, NoDma, NoDma>,
-        switch: RadioSwitch<'d>,
         irq: impl Peripheral<P = SUBGHZ_RADIO> + 'd,
-    ) -> Self {
+        config: SubGhzRadioConfig,
+    ) -> Result<Self, RadioError> {
         into_ref!(irq);
 
-        let mut inner = StateInner { radio, switch };
+        radio.reset();
-        inner.radio.reset();
-
-        let state_ptr = state.0.as_mut_ptr();
-        state_ptr.write(inner);
 
         irq.disable();
-        irq.set_handler(|p| {
+        irq.set_handler(|_| {
-            // This is safe because we only get interrupts when configured for, so
+            IRQ_WAKER.wake();
-            // the radio will be awaiting on the signal at this point. If not, the ISR will
+            unsafe { SUBGHZ_RADIO::steal().disable() };
-            // anyway only adjust the state in the IRQ signal state.
-            let state = &mut *(p as *mut StateInner<'d>);
-            state.on_interrupt();
         });
-        irq.set_handler_context(state_ptr as *mut ());
-        irq.enable();
 
-        Self {
+        Self { radio, switch, irq }
-            state: state_ptr,
-            _irq: irq,
     }
-    }
-}
 
-impl<'d> StateInner<'d> {
     /// Configure radio settings in preparation for TX or RX
     pub(crate) fn configure(&mut self) -> Result<(), RadioError> {
         trace!("Configuring STM32WL SUBGHZ radio");
@@ -151,8 +129,7 @@ impl<'d> StateInner<'d> {
         self.radio.set_tx(Timeout::DISABLED)?;
 
         loop {
-            let (_status, irq_status) = IRQ.wait().await;
+            let (_status, irq_status) = self.irq_wait().await;
-            IRQ.reset();
 
             if irq_status & Irq::TxDone.mask() != 0 {
                 let stats = self.radio.lora_stats()?;
@@ -214,8 +191,8 @@ impl<'d> StateInner<'d> {
         trace!("RX started");
 
         loop {
-            let (status, irq_status) = IRQ.wait().await;
+            let (status, irq_status) = self.irq_wait().await;
-            IRQ.reset();
+
             trace!("RX IRQ {:?}, {:?}", status, irq_status);
             if irq_status & Irq::RxDone.mask() != 0 {
                 let (status, len, ptr) = self.radio.rx_buffer_status()?;
@@ -238,17 +215,24 @@ impl<'d> StateInner<'d> {
         }
     }
 
-    /// Read interrupt status and store in global signal
+    async fn irq_wait(&mut self) -> (Status, u16) {
-    fn on_interrupt(&mut self) {
+        poll_fn(|cx| {
+            self.irq.unpend();
+            self.irq.enable();
+            IRQ_WAKER.register(cx.waker());
+
             let (status, irq_status) = self.radio.irq_status().expect("error getting irq status");
             self.radio
                 .clear_irq_status(irq_status)
                 .expect("error clearing irq status");
-        if irq_status & Irq::PreambleDetected.mask() != 0 {
+            trace!("IRQ status: {=u16:b}", irq_status);
-            trace!("Preamble detected, ignoring");
+            if irq_status == 0 {
+                Poll::Pending
             } else {
-            IRQ.signal((status, irq_status));
+                Poll::Ready((status, irq_status))
             }
+        })
+        .await
     }
 }
 
@@ -257,18 +241,12 @@ impl PhyRxTx for SubGhzRadio<'static> {
 
     type TxFuture<'m> = impl Future<Output = Result<u32, Self::PhyError>> + 'm;
     fn tx<'m>(&'m mut self, config: TxConfig, buf: &'m [u8]) -> Self::TxFuture<'m> {
-        async move {
+        async move { self.do_tx(config, buf).await }
-            let inner = unsafe { &mut *self.state };
-            inner.do_tx(config, buf).await
-        }
     }
 
     type RxFuture<'m> = impl Future<Output = Result<(usize, RxQuality), Self::PhyError>> + 'm;
     fn rx<'m>(&'m mut self, config: RfConfig, buf: &'m mut [u8]) -> Self::RxFuture<'m> {
-        async move {
+        async move { self.do_rx(config, buf).await }
-            let inner = unsafe { &mut *self.state };
-            inner.do_rx(config, buf).await
-        }
     }
 }
 
@@ -278,7 +256,7 @@ impl From<embassy_stm32::spi::Error> for RadioError {
     }
 }
 
-impl<'d> Timings for SubGhzRadio<'d> {
+impl<'d, RS> Timings for SubGhzRadio<'d, RS> {
     fn get_rx_window_offset_ms(&self) -> i32 {
         -200
     }

examples/stm32wl/src/bin/lorawan.rs

@@ -32,10 +32,9 @@ async fn main(_spawner: Spawner) {
     let rfs = RadioSwitch::new(ctrl1, ctrl2, ctrl3);
 
     let radio = SubGhz::new(p.SUBGHZSPI, NoDma, NoDma);
-
     let irq = interrupt::take!(SUBGHZ_RADIO);
-    static mut RADIO_STATE: SubGhzState<'static> = SubGhzState::new();
+
-    let radio = unsafe { SubGhzRadio::new(&mut RADIO_STATE, radio, rfs, irq) };
+    let radio = SubGhzRadio::new(radio, rfs, irq);
 
     let mut region: region::Configuration = region::EU868::default().into();