From 117244a7ef1b8c12604989cf3453f99b4de3ff43 Mon Sep 17 00:00:00 2001 From: "Jonathan Pallant (Ferrous Systems)" Date: Thu, 25 Jan 2024 13:20:19 +0000 Subject: [PATCH] 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 | 1 + nrf52-code/boards/dongle/src/lib.rs | 41 +++++++ nrf52-code/loopback-fw/Cargo.lock | 3 +- nrf52-code/loopback-fw/Cargo.toml | 2 +- nrf52-code/loopback-fw/src/main.rs | 164 ++++++++++++++-------------- nrf52-code/puzzle-fw/Cargo.lock | 2 + nrf52-code/puzzle-fw/Cargo.toml | 1 + nrf52-code/puzzle-fw/src/main.rs | 164 +++++++++++++++++----------- 8 files changed, 231 insertions(+), 147 deletions(-) diff --git a/nrf52-code/boards/dongle/Cargo.toml b/nrf52-code/boards/dongle/Cargo.toml index b1de4840..05694d37 100644 --- a/nrf52-code/boards/dongle/Cargo.toml +++ b/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"] } diff --git a/nrf52-code/boards/dongle/src/lib.rs b/nrf52-code/boards/dongle/src/lib.rs index e5b7f235..480f3812 100644 --- a/nrf52-code/boards/dongle/src/lib.rs +++ b/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>; + 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, +} + +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 { + 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 { diff --git a/nrf52-code/loopback-fw/Cargo.lock b/nrf52-code/loopback-fw/Cargo.lock index aa3bbca5..23cdad2d 100644 --- a/nrf52-code/loopback-fw/Cargo.lock +++ b/nrf52-code/loopback-fw/Cargo.lock @@ -163,6 +163,7 @@ dependencies = [ "defmt", "defmt-rtt", "embedded-hal", + "heapless", "nrf52840-hal", "panic-probe", ] @@ -263,9 +264,9 @@ dependencies = [ "consts", "cortex-m", "cortex-m-rt", + "critical-section", "dongle", "embedded-hal", - "heapless", "rand", "usb-device", "usbd-hid", diff --git a/nrf52-code/loopback-fw/Cargo.toml b/nrf52-code/loopback-fw/Cargo.toml index e122340d..0acc7ecf 100644 --- a/nrf52-code/loopback-fw/Cargo.toml +++ b/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" diff --git a/nrf52-code/loopback-fw/src/main.rs b/nrf52-code/loopback-fw/src/main.rs index 1d571a0a..cba73573 100644 --- a/nrf52-code/loopback-fw/src/main.rs +++ b/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>; +/// 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> = None; - -/// The USB Bus Driver (owned by the USBD interrupt). -static mut USB_BUS: Option> = None; +static USB_DEVICE: Mutex>>> = Mutex::new(RefCell::new(None)); /// The USB Serial Device Driver (owned by the USBD interrupt). -static mut USB_SERIAL: Option> = None; +static USB_SERIAL: Mutex>>> = Mutex::new(RefCell::new(None)); /// The USB Human Interface Device Driver (owned by the USBD interrupt). -static mut USB_HID: Option> = None; +static USB_HID: Mutex>>> = 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, - 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> = 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> = None; + static mut LOCAL_USB_SERIAL: Option> = None; + static mut LOCAL_USB_HID: Option> = None; + static mut IS_PENDING: Option = 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(); } diff --git a/nrf52-code/puzzle-fw/Cargo.lock b/nrf52-code/puzzle-fw/Cargo.lock index 534ad960..5b593381 100644 --- a/nrf52-code/puzzle-fw/Cargo.lock +++ b/nrf52-code/puzzle-fw/Cargo.lock @@ -163,6 +163,7 @@ dependencies = [ "defmt", "defmt-rtt", "embedded-hal", + "heapless", "nrf52840-hal", "panic-probe", ] @@ -381,6 +382,7 @@ dependencies = [ "consts", "cortex-m", "cortex-m-rt", + "critical-section", "dongle", "embedded-hal", "heapless", diff --git a/nrf52-code/puzzle-fw/Cargo.toml b/nrf52-code/puzzle-fw/Cargo.toml index fe8de4e2..31de2856 100644 --- a/nrf52-code/puzzle-fw/Cargo.toml +++ b/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" diff --git a/nrf52-code/puzzle-fw/src/main.rs b/nrf52-code/puzzle-fw/src/main.rs index 86600a88..06445c79 100644 --- a/nrf52-code/puzzle-fw/src/main.rs +++ b/nrf52-code/puzzle-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,6 +20,7 @@ use dongle::peripheral::interrupt; use dongle::{ hal::usbd, ieee802154::{Channel, Packet}, + UsbBus, }; /// The secret message, but encoded. @@ -31,25 +34,17 @@ static PLAIN_LETTERS: &[u8] = include_bytes!(concat!(env!("OUT_DIR"), "/PLAIN_LE /// The ciphertext side of the map static CIPHER_LETTERS: &[u8] = include_bytes!(concat!(env!("OUT_DIR"), "/CIPHER_LETTERS.txt")); -/// A 64-byte USB Serial buffer -static RING_BUFFER: Ringbuffer = Ringbuffer { - buffer: heapless::mpmc::Q64::new(), -}; - -/// A short-hand for the nRF52 USB types -type UsbBus<'a> = usbd::Usbd>; +/// 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> = None; - -/// The USB Bus Driver (owned by the USBD interrupt). -static mut USB_BUS: Option> = None; +static USB_DEVICE: Mutex>>> = Mutex::new(RefCell::new(None)); /// The USB Serial Device Driver (owned by the USBD interrupt). -static mut USB_SERIAL: Option> = None; +static USB_SERIAL: Mutex>>> = Mutex::new(RefCell::new(None)); /// The USB Human Interface Device Driver (owned by the USBD interrupt). -static mut USB_HID: Option> = None; +static USB_HID: Mutex>>> = Mutex::new(RefCell::new(None)); /// Track how many CRC successes we had receiving radio packets static RX_COUNT: AtomicU32 = AtomicU32::new(0); @@ -67,42 +62,33 @@ static NEW_CHANNEL: AtomicU32 = AtomicU32::new(u32::MAX); /// We print some info in response. static WANT_INFO: AtomicBool = AtomicBool::new(false); -struct Ringbuffer { - buffer: heapless::mpmc::Q64, -} - -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(()) - } -} - #[entry] fn main() -> ! { + // The USB Bus, statically allocated + static mut USB_BUS: Option> = 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 @@ -130,9 +116,9 @@ 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) @@ -141,34 +127,43 @@ fn main() -> ! { .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 dict: heapless::LinearMap = heapless::LinearMap::new(); - for (&plain, &cipher) in PLAIN_LETTERS.iter().zip(CIPHER_LETTERS.iter()) { - let _ = dict.insert(plain, cipher); - } - // Turn on USB interrupts... unsafe { cortex_m::peripheral::NVIC::unmask(dongle::peripheral::Interrupt::USBD); }; + let _ = writeln!( + &RING_BUFFER, + "deviceid={:08x}{:08x} channel={} TxPower=+8dBm app=puzzle-fw", + dongle::deviceid1(), + dongle::deviceid0(), + current_ch_id + ); + + board.leds.ld1.on(); + board.leds.ld2_green.on(); + + let mut dict: heapless::LinearMap = heapless::LinearMap::new(); + for (&plain, &cipher) in PLAIN_LETTERS.iter().zip(CIPHER_LETTERS.iter()) { + let _ = dict.insert(plain, cipher); + } + let mut pkt = Packet::new(); loop { - board.leds.ld1.on(); // Wait up to 1 second for a radio packet match board .radio .recv_timeout(&mut pkt, &mut board.timer, 1_000_000) { Ok(crc) => { - board.leds.ld1.off(); + board.leds.ld1.toggle(); let _ = writeln!( &RING_BUFFER, "\nRX CRC {:04x}, LQI {}, LEN {}", @@ -206,7 +201,10 @@ fn main() -> ! { board.leds.ld2_red.on(); } } - // now send it + // 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); } @@ -243,10 +241,10 @@ fn main() -> ! { _ => None, } { board.radio.set_channel(channel); - let _ = writeln!(&RING_BUFFER, "\nChannel {} set", ch_id); + let _ = writeln!(&RING_BUFFER, "now listening on channel {}", ch_id); current_ch_id = ch_id; } else { - let _ = writeln!(&RING_BUFFER, "\nChannel {} invalid", ch_id); + let _ = writeln!(&RING_BUFFER, "Channel {} invalid", ch_id); } } @@ -303,10 +301,33 @@ fn handle_packet(packet: &mut Packet, dict: &heapless::LinearMap) - /// 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> = None; + static mut LOCAL_USB_SERIAL: Option> = None; + static mut LOCAL_USB_HID: Option> = None; + static mut IS_PENDING: Option = 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]; @@ -348,17 +369,32 @@ 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]); + + cortex_m::asm::sev(); } #[panic_handler]