Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.53%. Comparing base (8987b39) to head (e277f57).
Report is 64 commits behind head on master.

@@           Coverage Diff           @@
##           master   #16785   +/-   ##
=======================================
  Coverage   98.53%   98.53%           
=======================================
  Files         169      169           
  Lines       21822    21822           
=======================================
  Hits        21502    21502           
  Misses        320      320           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:   +16 +0.002% RPI_PICO_W
       samd:   +20 +0.007% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

In order to make this discoverable the documentation on this needs to be updated as well.

https://docs.micropython.org/en/latest/library/machine.USBDevice.html#machine.USBDevice.submit_xfer

Can you add that in a separate commit?

The former cannot be used in ISR because it causes memory allocation, and allocating in ISR causes MemoryError.

I'm surprised that you can call submit_xfer() from within a hard ISR, there should be other reasons why that doesn't work. Do you have some code that does this, calls it from an ISR, and has a MemoryError?

An alternative way around this, that doesn't allocate memory, is to pre-create a memoryview of the data, eg:

self.mv = memoryview(self.in_buf)
self.mv_slice = self.mv[:xfer_bytes]

...

# then use `self.mv_slice` later on, it won't allocate any memory

I'm surprised that you can call submit_xfer() from within a hard ISR, there should be other reasons why that doesn't work. Do you have some code that does this, calls it from an ISR, and has a MemoryError?

my demo code:

class CDC_UART(USB_CDC.USB_CDC):
    def __init__(self):
        super().__init__(PID=CDC_UART.PID, EP_IN=CDC_UART.EP_IN, EP_OUT=CDC_UART.EP_OUT, product_string='CDC UART')
        
        self.out_buf = bytearray(64)
        self.in_buf = bytearray(64)

        self.ser = UART(1, 115200, txd='PB0', rxd='PN5', irq=UART.IRQ_RXAny|UART.IRQ_TIMEOUT, callback=lambda ser: self.on_ser_rx())

        self.usbd.active(1)

    def on_ser_rx(self):
        if self.ser.any() >= 64 or self.ser.irq_flags() & UART.IRQ_TIMEOUT:
            xfer_bytes = min(self.ser.any(), 64)
            
            self.ser.readinto(self.in_buf, xfer_bytes)
            
            self.usbd.submit_xfer(self.EP_IN, self.in_buf, xfer_bytes)

An alternative way around this, that doesn't allocate memory, is to pre-create a memoryview of the data, eg:

self.mv = memoryview(self.in_buf)
self.mv_slice = self.mv[:xfer_bytes]

...

# then use `self.mv_slice` later on, it won't allocate any memory

Slice operation on memoryview causes allocation

I think self.mv[:xfer_bytes] will create a new slice object.

my demo code:

Thanks for posting your demo code. Note that the UART IRQ is actually a soft interrupt that runs at thread level, so it's possible to allocate memory in that callback.

Did you ever see a MemoryError in the callback?

I think self.mv[:xfer_bytes] will create a new slice object.

Yes, you are correct, it will allocate. But the idea is to do that slice outside the IRQ callback and store it for later use.

Well, in this case you need a slice object for all 63 different slice lengths, so it's not very optimal.

my demo code:

Thanks for posting your demo code. Note that the UART IRQ is actually a soft interrupt that runs at thread level, so it's possible to allocate memory in that callback.

thank you for response.

I'm not familiar with soft interrupt in micropython. In my port, the UART ISR is a hardware interrupt:

static void UARTX_Handler(uint32_t uart_id)
{
    pyb_uart_obj_t *self = &pyb_uart_obj[uart_id];

    if(UART_INTStat(self->UARTx, UART_IT_RX_THR | UART_IT_RX_TOUT))
    {
        while(!UART_IsRXFIFOEmpty(self->UARTx))
        {
            fetch_one_char(self);
        }

        if((self->irq_trigger & UART_IRQ_RXAny) && uart_rx_any(self))
            self->irq_flags |= UART_IRQ_RXAny;
        
        if(UART_INTStat(self->UARTx, UART_IT_RX_TOUT))
        {
            UART_INTClr(self->UARTx, UART_IT_RX_TOUT);

            if(self->irq_trigger & UART_IRQ_TIMEOUT)
                self->irq_flags |= UART_IRQ_TIMEOUT;
        }
    }

    if(self->irq_flags && (self->irq_callback != mp_const_none))
    {
        gc_lock();
        nlr_buf_t nlr;
        if(nlr_push(&nlr) == 0)
        {
            mp_call_function_1(self->irq_callback, self);
            nlr_pop();
        }
        else
        {
            self->irq_callback = mp_const_none;

            printf("uncaught exception in UART(%u) interrupt handler\n", self->uart_id);
            mp_obj_print_exception(&mp_plat_print, (mp_obj_t)nlr.ret_val);
        }
        gc_unlock();
    }

    self->irq_flags = 0;
}

void UART0_Handler(void)
{
    UARTX_Handler(PYB_UART_0);
}

void UART1_Handler(void)
{
    UARTX_Handler(PYB_UART_1);
}

I will learn soft interrupt to see if I can change the ISR of peripherals to soft inerrrupt.

In my port, the UART ISR is a hardware interrupt

Oh, you have a custom port! I thought you were using the rp2 port and a Pico board.

You can try to implement soft interrupts, just call mp_sched_schedule(self->irq_callback, self) instead of the mp_call_function_1 call.

I don't think calling submit_xfer() is thread safe, I don't think it can be called from within a hard interrupt (although I could be wrong here). So you may be required to use a soft interrupt.

In general though, I do support improving things so there is less memory allocation. But in the case here, I think we should instead improve memoryview so you can slice it without memory allocation, see eg #12518.

You can try to implement soft interrupts, just call mp_sched_schedule(self->irq_callback, self) instead of the mp_call_function_1 call.

I don't think calling submit_xfer() is thread safe, I don't think it can be called from within a hard interrupt (although I could be wrong here). So you may be required to use a soft interrupt.

In general though, I do support improving things so there is less memory allocation. But in the case here, I think we should instead improve memoryview so you can slice it without memory allocation, see eg #12518.

Thanks for your reply, I think what you said is very reasonable.