Clean up USB Serial code.

* dongle BSP provides RingBuffer and UsbBus types.
* use critical-section mutexes to handle Usb types, not static mut
* Add a 5ms delay before replying - makes it much more reliable

nrf52-code/boards/dongle/Cargo.toml

@@ -13,4 +13,5 @@ defmt = "0.3.5"
 defmt-rtt = "0.4"
 embedded-hal = "0.2.7"
 hal = { package = "nrf52840-hal", version = "0.16.0" }
+heapless = "0.8.0"
 panic-probe = { version = "0.3.0", features = ["print-defmt"] }

nrf52-code/boards/dongle/src/lib.rs

@@ -35,6 +35,9 @@ pub mod peripheral {
     pub use hal::pac::{interrupt, Interrupt, POWER, USBD};
 }
 
+/// A short-hand for the nRF52 USB types
+pub type UsbBus = hal::usbd::Usbd<hal::usbd::UsbPeripheral<'static>>;
+
 use peripheral::interrupt;
 
 /// Components on the board
@@ -190,6 +193,44 @@ impl ops::DerefMut for Timer {
     }
 }
 
+/// A byte-based ring-buffer that you can writeln! into, and drain under
+/// interrupt.
+///
+/// Used for buffering serial port output.
+///
+/// Stores 128 bytes, maximum.
+pub struct Ringbuffer {
+    buffer: heapless::mpmc::MpMcQueue<u8, 128>,
+}
+
+impl Ringbuffer {
+    /// Construct a new Ringbuffer
+    pub const fn new() -> Ringbuffer {
+        Ringbuffer {
+            buffer: heapless::mpmc::MpMcQueue::new(),
+        }
+    }
+
+    /// Take an item from the buffer
+    pub fn read(&self) -> Option<u8> {
+        self.buffer.dequeue()
+    }
+
+    /// Add an item to the queue
+    pub fn write(&self, value: u8) -> Result<(), u8> {
+        self.buffer.enqueue(value)
+    }
+}
+
+impl core::fmt::Write for &Ringbuffer {
+    fn write_str(&mut self, s: &str) -> core::fmt::Result {
+        for b in s.bytes() {
+            let _ = self.buffer.enqueue(b);
+        }
+        Ok(())
+    }
+}
+
 /// The ways that initialisation can fail
 #[derive(Debug, Copy, Clone, defmt::Format)]
 pub enum Error {

nrf52-code/loopback-fw/Cargo.toml

@@ -9,9 +9,9 @@ version = "0.0.0"
 consts = { path = "../consts" }
 cortex-m = {version = "0.7.6", features = ["critical-section-single-core"]}
 cortex-m-rt = "0.7"
+critical-section = "1.1.2"
 dongle = { path = "../boards/dongle" }
 embedded-hal = "0.2.7"
-heapless = "0.8"
 usb-device = "0.2.7"
 usbd-hid = "0.6"
 usbd-serial = "0.1.0"

nrf52-code/loopback-fw/src/main.rs

@@ -5,10 +5,12 @@
 #![no_std]
 #![no_main]
 
+use core::cell::RefCell;
 use core::fmt::Write;
 use core::sync::atomic::{AtomicBool, AtomicU32, Ordering};
 
 use cortex_m_rt::entry;
+use critical_section::Mutex;
 use usb_device::class_prelude::UsbBusAllocator;
 use usb_device::device::{UsbDevice, UsbDeviceBuilder, UsbVidPid};
 use usbd_hid::hid_class::HIDClass;
@@ -18,28 +20,20 @@ use dongle::peripheral::interrupt;
 use dongle::{
     hal::usbd,
     ieee802154::{Channel, Packet},
+    UsbBus,
 };
 
-/// A 64-byte USB Serial buffer
-static RING_BUFFER: Ringbuffer = Ringbuffer {
-    buffer: heapless::mpmc::Q64::new(),
-    force_buffer: AtomicBool::new(true),
-};
-
-/// A short-hand for the nRF52 USB types
-type UsbBus<'a> = usbd::Usbd<usbd::UsbPeripheral<'a>>;
+/// A buffer for holding bytes we want to send to the USB Serial port
+static RING_BUFFER: dongle::Ringbuffer = dongle::Ringbuffer::new();
 
 /// The USB Device Driver (owned by the USBD interrupt).
-static mut USB_DEVICE: Option<UsbDevice<UsbBus>> = None;
-
-/// The USB Bus Driver (owned by the USBD interrupt).
-static mut USB_BUS: Option<UsbBusAllocator<UsbBus>> = None;
+static USB_DEVICE: Mutex<RefCell<Option<UsbDevice<UsbBus>>>> = Mutex::new(RefCell::new(None));
 
 /// The USB Serial Device Driver (owned by the USBD interrupt).
-static mut USB_SERIAL: Option<SerialPort<UsbBus>> = None;
+static USB_SERIAL: Mutex<RefCell<Option<SerialPort<UsbBus>>>> = Mutex::new(RefCell::new(None));
 
 /// The USB Human Interface Device Driver (owned by the USBD interrupt).
-static mut USB_HID: Option<HIDClass<UsbBus>> = None;
+static USB_HID: Mutex<RefCell<Option<HIDClass<UsbBus>>>> = Mutex::new(RefCell::new(None));
 
 /// Track how many CRC successes we had receiving radio packets
 static RX_COUNT: AtomicU32 = AtomicU32::new(0);
@@ -57,53 +51,33 @@ static NEW_CHANNEL: AtomicU32 = AtomicU32::new(u32::MAX);
 /// We print some info in response.
 static WANT_INFO: AtomicBool = AtomicBool::new(false);
 
-/// Is the UART connected?
-static IS_CONNECTED: AtomicBool = AtomicBool::new(false);
-
-struct Ringbuffer {
-    buffer: heapless::mpmc::Q64<u8>,
-    force_buffer: AtomicBool,
-}
-
-impl core::fmt::Write for &Ringbuffer {
-    fn write_str(&mut self, s: &str) -> core::fmt::Result {
-        for b in s.bytes() {
-            if IS_CONNECTED.load(Ordering::Relaxed) || self.force_buffer.load(Ordering::Relaxed) {
-                // spin until space in the UART
-                while let Err(_) = self.buffer.enqueue(b) {
-                    cortex_m::asm::wfi();
-                }
-            } else {
-                // drop the data - we're not connected
-            }
-        }
-        Ok(())
-    }
-}
-
 #[entry]
 fn main() -> ! {
+    // The USB Bus, statically allocated
+    static mut USB_BUS: Option<UsbBusAllocator<UsbBus>> = None;
+
     let mut board = dongle::init().unwrap();
+
     board.usbd.inten.modify(|_r, w| {
         w.sof().set_bit();
         w
     });
+
     let usb_bus = UsbBusAllocator::new(usbd::Usbd::new(usbd::UsbPeripheral::new(
         board.usbd,
         board.clocks,
     )));
-    unsafe {
-        // Note (safety): This is safe as interrupts haven't been started yet
-        USB_BUS = Some(usb_bus);
-    }
+    *USB_BUS = Some(usb_bus);
+
+    // This reference has static lifetime
+    let bus_ref = USB_BUS.as_ref().unwrap();
+
     // Grab a reference to the USB Bus allocator. We are promising to the
     // compiler not to take mutable access to this global variable whilst this
     // reference exists!
-    let bus_ref = unsafe { USB_BUS.as_ref().unwrap() };
-    let serial = SerialPort::new(bus_ref);
-    unsafe {
-        USB_SERIAL = Some(serial);
-    }
+    critical_section::with(|cs| {
+        *USB_SERIAL.borrow(cs).borrow_mut() = Some(SerialPort::new(bus_ref));
+    });
 
     let desc = &[
         0x06, 0x00, 0xFF, // Item(Global): Usage Page, data= [ 0x00 0xff ] 65280
@@ -131,24 +105,23 @@ fn main() -> ! {
         0xC0, // Item(Main  ): End Collection, data=none
     ];
     let hid = HIDClass::new(bus_ref, desc, 100);
-    unsafe {
-        USB_HID = Some(hid);
-    }
+    critical_section::with(|cs| {
+        *USB_HID.borrow(cs).borrow_mut() = Some(hid);
+    });
 
     let vid_pid = UsbVidPid(consts::USB_VID_DEMO, consts::USB_PID_DONGLE_PUZZLE);
     let usb_dev = UsbDeviceBuilder::new(bus_ref, vid_pid)
         .manufacturer("Ferrous Systems")
-        .product("Dongle Puzzle")
+        .product("Dongle Loopback")
         .device_class(USB_CLASS_CDC)
         .max_packet_size_0(64) // (makes control transfers 8x faster)
         .build();
-    unsafe {
-        // Note (safety): This is safe as interrupts haven't been started yet
-        USB_DEVICE = Some(usb_dev);
-    }
+    critical_section::with(|cs| {
+        *USB_DEVICE.borrow(cs).borrow_mut() = Some(usb_dev);
+    });
 
-    let mut current_ch_id = 25;
-    board.radio.set_channel(dongle::ieee802154::Channel::_25);
+    let mut current_ch_id = 20;
+    board.radio.set_channel(dongle::ieee802154::Channel::_20);
 
     // Turn on USB interrupts...
     unsafe {
@@ -163,9 +136,6 @@ fn main() -> ! {
         current_ch_id
     );
 
-    // Now the sign-up message is done, turn off force buffering
-    RING_BUFFER.force_buffer.store(false, Ordering::Relaxed);
-
     board.leds.ld1.on();
     board.leds.ld2_blue.on();
     let mut pkt = Packet::new();
@@ -184,7 +154,10 @@ fn main() -> ! {
                     crc,
                     pkt.lqi()
                 );
-                // now send it back
+                // send packet after 5ms (we know the client waits for 10ms and
+                // we want to ensure they are definitely in receive mode by the
+                // time we send this reply)
+                board.timer.delay(5000);
                 board.radio.send(&mut pkt);
                 RX_COUNT.fetch_add(1, Ordering::Relaxed);
             }
@@ -227,13 +200,6 @@ fn main() -> ! {
             }
         }
 
-        let connected = IS_CONNECTED.load(Ordering::Relaxed);
-        if connected {
-            board.leds.ld2_red.on();
-        } else {
-            board.leds.ld2_red.off();
-        }
-
         // Print help text when ? is pressed
         if WANT_INFO.load(Ordering::Relaxed) {
             WANT_INFO.store(false, Ordering::Relaxed);
@@ -254,10 +220,33 @@ fn main() -> ! {
 /// USB UART.
 #[interrupt]
 fn USBD() {
-    // Grab the global objects. This is OK as we only access them under interrupt.
-    let usb_dev = unsafe { USB_DEVICE.as_mut().unwrap() };
-    let serial = unsafe { USB_SERIAL.as_mut().unwrap() };
-    let hid = unsafe { USB_HID.as_mut().unwrap() };
+    static mut LOCAL_USB_DEVICE: Option<UsbDevice<UsbBus>> = None;
+    static mut LOCAL_USB_SERIAL: Option<SerialPort<UsbBus>> = None;
+    static mut LOCAL_USB_HID: Option<HIDClass<UsbBus>> = None;
+    static mut IS_PENDING: Option<u8> = None;
+
+    // Grab a reference to our local vars, moving the object out of the global as required...
+
+    let usb_dev = LOCAL_USB_DEVICE.get_or_insert_with(|| {
+        critical_section::with(|cs| {
+            // Move USB device here, leaving a None in its place
+            USB_DEVICE.borrow(cs).replace(None).unwrap()
+        })
+    });
+
+    let serial = LOCAL_USB_SERIAL.get_or_insert_with(|| {
+        critical_section::with(|cs| {
+            // Move USB device here, leaving a None in its place
+            USB_SERIAL.borrow(cs).replace(None).unwrap()
+        })
+    });
+
+    let hid = LOCAL_USB_HID.get_or_insert_with(|| {
+        critical_section::with(|cs| {
+            // Move USB device here, leaving a None in its place
+            USB_HID.borrow(cs).replace(None).unwrap()
+        })
+    });
 
     let mut buf = [0u8; 64];
 
@@ -299,18 +288,31 @@ fn USBD() {
         }
     }
 
-    // Copy from ring-buffer to USB UART
-    let mut count = 0;
-    while count < buf.len() {
-        if let Some(item) = RING_BUFFER.buffer.dequeue() {
-            buf[count] = item;
-            count += 1;
-        } else {
+    // Is there a pending byte from last time?
+    if let Some(n) = IS_PENDING {
+        match serial.write(core::slice::from_ref(n)) {
+            Ok(_) => {
+                // it took our pending byte
+                *IS_PENDING = None;
+            }
+            Err(_) => {
+                // serial buffer is full
+                return;
+            }
+        }
+    }
+
+    // Copy some more from the ring-buffer to the USB Serial interface,
+    // until the serial interface is full.
+    while let Some(item) = RING_BUFFER.read() {
+        let s = &[item];
+        if serial.write(s).is_err() {
+            // the USB UART can't take this byte right now
+            *IS_PENDING = Some(item);
             break;
         }
     }
-    let _ = serial.write(&buf[0..count]);
-    IS_CONNECTED.store(serial.dtr(), Ordering::Relaxed);
+
     cortex_m::asm::sev();
 }
 

nrf52-code/puzzle-fw/Cargo.toml

@@ -9,6 +9,7 @@ version = "0.0.0"
 consts = { path = "../consts" }
 cortex-m = {version = "0.7.6", features = ["critical-section-single-core"]}
 cortex-m-rt = "0.7"
+critical-section = "1.1.2"
 dongle = { path = "../boards/dongle" }
 embedded-hal = "0.2.7"
 heapless = "0.8"