atsamd: USB methods disable interrupts for too long

I’m working on a SAMD21 based USB device, which does the USB work in the USB ISR, along the lines of the usb_echo example. The USB interrupt is set to a lower priority (higher number) than another interrupt which needs to be serviced fairly quickly, but sometimes that doesn’t happen quickly enough. The issue seems to be that UsbBus’s methods do all their work in interrupt free contexts. For example, these.

Might it make sense for UsbBus to have an “in use” flag in the mutex rather than the RefCell, so that the interrupts only need to be disabled for setting/checking that flag?

About this issue

Original URL
State: open
Created 3 years ago
Comments: 20 (20 by maintainers)

Most upvoted comments

dirbaio on Matrix suggested that the stm32-usbd doesn’t have this particular issue, perhaps it’s possible to make UsbBus and/or Inner not by Sync, so that the user either picks their own mutex or only accesses the USB from one particular context.

ianrrees on Mar 23, 2021

Is this the reason we’re currently disabling interrupts?

I think it was just to prevent us being interrupted by a different USB interrupt while servicing a USB interrupt. I don’t think there is anything wrong with being preempted by a higher priority interrupt as long as its not USB-related, but theres only one way to find out xD

twitchyliquid64 on Feb 2, 2021

Something like this maybe:

struct UsbCS {
    other_active: bool, // USB_OTHER irq
    trcpt0_active: bool, // USB_TRCPT0 irq
    trcpt1_active: bool, // USB_TRCPT1 irq
}

impl core::ops::Drop for UsbCS {
    fn drop(&mut self) {
        use cortex_m::peripheral::NVIC;
        use crate::target_device::interrupt;
        unsafe {
            if self.other_active {
                NVIC::unmask(interrupt::USB_OTHER);
            }
            if self.trcpt0_active {
                NVIC::unmask(interrupt::USB_TRCPT0);
            }
            if self.trcpt1_active {
                NVIC::unmask(interrupt::USB_TRCPT1);
            }
        }
    }
}

impl UsbCS {
    fn mutex_cs(&self) -> cortex_m::interrupt::CriticalSection {
        unsafe { cortex_m::interrupt::CriticalSection::new() }
    }
}

/// Disables IRQ lines related to USB, returning a token that resets the
/// masking state when dropped.
fn usb_critical_section() -> UsbCS {
    use cortex_m::peripheral::NVIC;
    use crate::target_device::interrupt;

    let cs = UsbCS {
        other_active: NVIC::is_enabled(interrupt::USB_OTHER),
        trcpt0_active: NVIC::is_enabled(interrupt::USB_TRCPT0),
        trcpt1_active: NVIC::is_enabled(interrupt::USB_TRCPT1),
    };

    NVIC::mask(interrupt::USB_OTHER);
    NVIC::mask(interrupt::USB_TRCPT0);
    NVIC::mask(interrupt::USB_TRCPT1);
    cs
}

twitchyliquid64 on Feb 2, 2021