raspberrypi / pico-sdk

BSD 3-Clause "New" or "Revised" License
3.74k stars 928 forks source link

Rx timeout interrupt should be controlled individually #568

Open xiongyu0523 opened 3 years ago

xiongyu0523 commented 3 years ago

I saw this PR https://github.com/raspberrypi/pico-sdk/pull/504/commits/6e6cb52192b3a9455d0714e606633d82283062b4 and some discussion in issue https://github.com/raspberrypi/pico-sdk/issues/500.

It is not flexible to have both RX and RX timeout interrupts enabled together. In some cases when UART is used together with DMA, we just want to have the RX timeout (IDLE) to tell CPU that a complete frame is received without triggering a DMA interrupt, and definitely, we do not want each character or FIFO threshold trigger a interrupt

I would suggest using a single function to control each interrupt bit. Even the original one is breaking the single-purpose design principle.

Will be something like, what's your thoughts?

uart_set_tx_irq_enable uart_set_rx_irq_enable uart_set_idle_irq_enable uart_set_error_irq_enable

xiongyu0523 commented 3 years ago

@Wren6991 @MustBeArt

Wren6991 commented 3 years ago

The RX IRQ function is trying to give a simplified view of RX interrupts, satisfying these rules:

Which can be serviced with simple interrupt handler logic, "when I get an interrupt, read all characters and return". It's not perfect but it hides some of the surprising behaviour details of the PL011, and we could write similar functions for another UART (say, PIO) which satisfy the same conditions above.

The functions you suggest give better control, but they seem quite specific to the way PL011 interrupts work.

It is not flexible to have both RX and RX timeout interrupts enabled together. In some cases when UART is used together with DMA, we just want to have the RX timeout (IDLE) to tell CPU that a complete frame is received without triggering a DMA interrupt, and definitely, we do not want each character or FIFO threshold trigger a interrupt

Sorry, could you give a little more detail here? I'm not sure I fully understand the use case. The DREQ glue does handle SINGLE requests, so there should be no need for the processor to come and tidy up odd bytes from the FIFO when the receiver goes idle.

Are you saying the DMA is handling all of the data movement, and you want to use the idle IRQ to prompt the processor to scan through the DMA RX buffer at meaningful times?

At some point, if you are using PL011-specific functionality, you may just want to write your own wrappers

void pl011_set_idle_irq(uart_hw_t *hw, bool irq) {
    if (irq) {
        hw_set_bits(hw->imsc, UART_UARTIMSC_RTIM_BITS);
    } else {
        hw_clear_bits(hw->imsc, UART_UARTIMSC_RTIM_BITS);
   }
}
xiongyu0523 commented 3 years ago

Are you saying the DMA is handling all of the data movement, and you want to use the idle IRQ to prompt the processor to scan through the DMA RX buffer at meaningful times?

Yes exactly.