rust-embedded-community / usbd-serial

Work-in progress minimal CDC-ACM (USB serial port) class for usb-device
MIT License
115 stars 35 forks source link

How can we handle read buffer overflow? #35

Open hacknus opened 1 year ago

hacknus commented 1 year ago

I have an embedded project on an STM32F405 with USB serial configured and running with FreeRTOS (rust wrapper) as follows:


// Make USB serial device globally available
pub static G_USB_SERIAL: Mutex<RefCell<Option<SerialPort<UsbBus<USB>>>>> =
    Mutex::new(RefCell::new(None));

// Make USB device globally available
pub static G_USB_DEVICE: Mutex<RefCell<Option<UsbDevice<UsbBus<USB>>>>> =
    Mutex::new(RefCell::new(None));

pub unsafe fn usb_init(usb: USB) {
    static mut EP_MEMORY: [u32; 1024] = [0; 1024];
    static mut USB_BUS: Option<UsbBusAllocator<stm32f4xx_hal::otg_fs::UsbBusType>> = None;
    USB_BUS = Some(stm32f4xx_hal::otg_fs::UsbBusType::new(usb, &mut EP_MEMORY));
    let usb_bus = USB_BUS.as_ref().unwrap();
    let serial_port = SerialPort::new(&usb_bus);
    let usb_dev = UsbDeviceBuilder::new(&usb_bus, UsbVidPid(0x17c0, 0x28dd))
        .manufacturer("University of Bern")
        .product("Thermometry")
        .serial_number("IceLab814")
        .device_class(usbd_serial::USB_CLASS_CDC)
        .build();
    cortex_m::interrupt::free(|cs| {
        *G_USB_SERIAL.borrow(cs).borrow_mut() = Some(serial_port);
        *G_USB_DEVICE.borrow(cs).borrow_mut() = Some(usb_dev);
    });
}

pub fn usb_read(message: &mut [u8; 1024]) -> bool {
    cortex_m::interrupt::free(|cs| {
        *message = [0; 1024];
        return match G_USB_SERIAL.borrow(cs).borrow_mut().as_mut() {
            None => false,
            Some(serial) => match serial.read(message) {
                Ok(a) => if a < 1024 {
                    true
                } else {
                    false
                },
                Err(_) => false,
            },
        };
    })
}

#[interrupt]
#[allow(non_snake_case)]
fn OTG_FS() {
    cortex_m::interrupt::free(|cs| {
        match G_USB_DEVICE.borrow(cs).borrow_mut().as_mut() {
            None => {}
            Some(usb_dev) => {
                match G_USB_SERIAL.borrow(cs).borrow_mut().as_mut() {
                    None => {}
                    Some(serial) => {
                        // do this regularly to keep connection to USB host
                        usb_dev.poll(&mut [serial]);
                    }
                }
            }
        }
    });
}

initialized in the main function like this:

let mut dp = pac::Peripherals::take().unwrap();

let rcc = dp.RCC.constrain();

let clocks = rcc
    .cfgr
    .use_hse(8.MHz())
    .sysclk(48.MHz())
    .hclk(48.MHz())
    .require_pll48clk()
    .pclk1(24.MHz())
    .pclk2(24.MHz())
    .freeze();

let gpioa = dp.GPIOA.split();

// initialize usb
let usb = USB {
    usb_global: dp.OTG_FS_GLOBAL,
    usb_device: dp.OTG_FS_DEVICE,
    usb_pwrclk: dp.OTG_FS_PWRCLK,
    pin_dm: gpioa.pa11.into_alternate(),
    pin_dp: gpioa.pa12.into_alternate(),
    hclk: clocks.hclk(),
 };
unsafe {
     usb_init(usb);
     cortex_m::peripheral::NVIC::unmask(Interrupt::OTG_FS);
}

my USB task (stack size 2048) does something like this:

loop {
    let mut message_bytes = [0; 1024];
    usb_read(&mut message_bytes);
    // do something
    // print something over usb
}

I noticed that the SerialPort::new(&usb_bus) doc says: "Creates a new USB serial port with the provided UsbBus and 128 byte read/write buffers.". Whenever I send a message longer than 63 characters, the microcontroller gets stuck and requires a reset. How can I handle this case?

hacknus commented 1 year ago

I stepped through with the debugger and it seems that the chip does not panic or hardfault, it gets stuck somewhere in the interrupt routine, however by stepping through, I'm not really able to observe where (very long debug output):

cortex_m::interrupt::free<synopsys_usb_otg::bus::{impl#2}::poll::{closure_env#0}<stm32f4xx_hal::otg_fs::USB>, usb_device::bus::PollResult> (f=...)
    at C:\Users\ls21y493\.cargo\registry\src\index.crates.io-6f17d22bba15001f\cortex-m-0.7.7\src/interrupt.rs:64
64          let r = f(unsafe { &CriticalSection::new() });
(gdb) s

Program stopped.
synopsys_usb_otg::bus::{impl#2}::poll::{closure#0}<stm32f4xx_hal::otg_fs::USB> (cs=0x2001ff9c) at C:\Users\ls21y493\.cargo\registry\src\index.crates.io-6f17d22bba15001f\synopsys-usb-otg-0.3.2\src/bus.rs:579
579                 let core_id = read_reg!(otg_global, regs.global(), CID);
(gdb) s

Program stopped.
synopsys_usb_otg::target::UsbRegisters::global (self=0x200013ec <thermometry_ctrl_rust::usb::usb_init::USB_BUS+420>)
    at C:\Users\ls21y493\.cargo\registry\src\index.crates.io-6f17d22bba15001f\synopsys-usb-otg-0.3.2/src\target.rs:51
51              unsafe { &*(self.0 as *const _) }
(gdb) s
halted: PC: 0x080042ee
synopsys_usb_otg::bus::{impl#2}::poll::{closure#0}<stm32f4xx_hal::otg_fs::USB> (cs=0x2001ff9c) at C:\Users\ls21y493\.cargo\registry\src\index.crates.io-6f17d22bba15001f\synopsys-usb-otg-0.3.2\src/bus.rs:579
579                 let core_id = read_reg!(otg_global, regs.global(), CID);
(gdb) s
synopsys_usb_otg::ral::register::RWRegister<u32>::read<u32> (self=<optimized out>) at C:\Users\ls21y493\.cargo\registry\src\index.crates.io-6f17d22bba15001f\synopsys-usb-otg-0.3.2/src\ral\register.rs:23
23              unsafe { ::core::ptr::read_volatile(self.register.get()) }
(gdb) s
core::ptr::read_volatile<u32> (src=<optimized out>) at /rustc/88fb1b922b047981fc0cfc62aa1418b4361ae72e/library/core/src/ptr/mod.rs:1552
1552    /rustc/88fb1b922b047981fc0cfc62aa1418b4361ae72e/library/core/src/ptr/mod.rs: No such file or directory.
(gdb) s
halted: PC: 0x080042f2
halted: PC: 0x080042f4
585                 if reset != 0 {
(gdb) s
halted: PC: 0x080042f6
halted: PC: 0x080043bc
595                 if enum_done != 0 {
(gdb) s
halted: PC: 0x080043be
halted: PC: 0x080043c0
636                 } else if wakeup != 0 {
(gdb) s
halted: PC: 0x080043c4
halted: PC: 0x080043c6
641                 } else if suspend != 0 {
(gdb) s
halted: PC: 0x080043c8
halted: PC: 0x080043ca
653                     if rxflvl != 0 {
(gdb) s
halted: PC: 0x080043cc
halted: PC: 0x0800449a
654                         let (epnum, data_size, status) = read_reg!(otg_global, regs.global(), GRXSTSR, EPNUM, BCNT, PKTSTS);
(gdb) s
synopsys_usb_otg::ral::register::RWRegister<u32>::read<u32> (self=0x5000001c) at C:\Users\ls21y493\.cargo\registry\src\index.crates.io-6f17d22bba15001f\synopsys-usb-otg-0.3.2/src\ral\register.rs:23
23              unsafe { ::core::ptr::read_volatile(self.register.get()) }
(gdb) s
core::ptr::read_volatile<u32> (src=0x5000001c) at /rustc/88fb1b922b047981fc0cfc62aa1418b4361ae72e/library/core/src/ptr/mod.rs:1552
1552    /rustc/88fb1b922b047981fc0cfc62aa1418b4361ae72e/library/core/src/ptr/mod.rs: No such file or directory.
(gdb) s
halted: PC: 0x0800449c
synopsys_usb_otg::bus::{impl#2}::poll::{closure#0}<stm32f4xx_hal::otg_fs::USB> (cs=0x2001ff9c) at C:\Users\ls21y493\.cargo\registry\src\index.crates.io-6f17d22bba15001f\synopsys-usb-otg-0.3.2\src/bus.rs:654
654                         let (epnum, data_size, status) = read_reg!(otg_global, regs.global(), GRXSTSR, EPNUM, BCNT, PKTSTS);
AzazKamaz commented 2 months ago

Actually this crate has an internal buffer that is enough only for usb 1.1 mode which only allows small packets Here it is: https://github.com/rust-embedded-community/usbd-serial/blob/7331c49d7a6e432e1db66d350152116ca4437f3e/src/buffer.rs#L126

With usb 2.0 packet size can go up to 512 bytes I guess, so just put some bigger number in there (for example 1024) and it would be good (at least helped for me last time I tried)

AzazKamaz commented 2 months ago

Or you can just use imxrt_usbd::Speed::LowFull instead of imxrt_usbd::Speed::High

hacknus commented 2 months ago

@AzazKamaz thanks for the input! I am however not sure where I should place this import and/or adjust the buffer in my user-side code? Or would this require a PR / update in the library?

AzazKamaz commented 2 months ago

@hacknus so the easy way to just get it working reliably is to switch to lower speed. It is done on the user side when you init usbd. I'm not really sure if this is available in any of the implementations/mcus, I'm currently using imxrt. I changed it here: https://github.com/imxrt-rs/imxrt-hal/blob/0cb3b8d320e3d2a0e43304f4beca06efba261af1/examples/rtic_usb_serial.rs#L41

I made a patch just for me, maybe will upstream it later, it should fix the issue and get high speed work properly: https://github.com/AzazKamaz/usbd-serial/tree/30b6cff9906be68267fe0960ebfba5b4754c9c80

Also I'm not really sure if after this patch it will be able to act as a full speed device since settings are different... It should in theory do some dynamic configuration