Add support for RX DMA always enabled UART

[Neil-alter]

I have observed a 16 bytes hang issue in UART reading. The root cause is that DMA is being enabled and disabled on every read() call, which leads to byte loss when the incoming packet is large, which leads to 16‑byte hang when the incoming packet is large.
During DeepSleep, the UART sometimes encounters a 16‑byte hang. To resolve this, the following method want to inject in:

1. Add read_buffered logic
   This allows continuous data retrieval from DMA without toggling the DMA state for every read, preventing data loss on large packets.
2. Handle interrupts correctly
   When enabling TXLVL and RXLVL in FIFOINTENSET to make FCWAKE in HWAKE effective, the corresponding TXLVL and RXLVL interrupts must be cleared in the interrupt handler to prevents an interrupt storm, and clear RXERR and TXERR bit if it is set.

[Copilot]

Pull request overview

This PR addresses a critical UART data loss issue during DMA-enabled reception by implementing continuous circular buffer reception and fixing FIFO interrupt handling. The root cause was DMA being toggled on every read() call, leading to byte loss on large incoming packets and a 16-byte hang during DeepSleep.

Key Changes:

• Implemented buffered read functionality with circular DMA buffer to maintain continuous reception without toggling DMA state
• Added FIFO interrupt clearing (TXLVL/RXLVL) in the interrupt handler to prevent interrupt storms when these are enabled for deep sleep wake functionality
• Enhanced DMA channel configuration to support continuous/circular transfers with descriptor linking

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.

File	Description
src/uart.rs	Adds BufferConfig struct and read_buffered() method for continuous DMA reception; adds three new_async_with_buffer constructors; updates interrupt handler to clear FIFO interrupts
src/dma/transfer.rs	Adds is_continuous and is_sw_trig fields to TransferOptions for circular buffer support
src/dma/mod.rs	Renames reserved field to reserved_reloadcfg to reflect its usage in reload configuration
src/dma/channel.rs	Restructures descriptor configuration to support circular transfers with descriptor self-linking and reload configuration

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jerrysxie]

How much testing was done using this code? Reworking the example a bit to output 1024 byte of repeating pattern of 0-255 and make read size different every single iteration. I am seeing a data corruption:

6354.921461 [INFO ] Total bytes read: 3686364 (uart_async_with_buffer src/bin/uart-async_with_buffer.rs:26)
6354.924497 [INFO ] buf: [220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 135] (uart_async_with_buffer src/bin/uart-async_with_buffer.rs:33)
6354.924622 [ERROR] panicked at src\bin\uart-async_with_buffer.rs:35:17:
Data mismatch at byte 3686388: expected 244, got 109 (panic_probe panic-probe-1.0.0/src/lib.rs:104)

[jerrysxie]

How much testing was done using this code? Reworking the example a bit to output 1024 byte of repeating pattern of 0-255 and make read size different every single iteration. I am seeing a data corruption:

6354.921461 [INFO ] Total bytes read: 3686364 (uart_async_with_buffer src/bin/uart-async_with_buffer.rs:26)
6354.924497 [INFO ] buf: [220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 135] (uart_async_with_buffer src/bin/uart-async_with_buffer.rs:33)
6354.924622 [ERROR] panicked at src\bin\uart-async_with_buffer.rs:35:17:
Data mismatch at byte 3686388: expected 244, got 109 (panic_probe panic-probe-1.0.0/src/lib.rs:104)

@Neil-alter @felipebalbi In running longer test with this test program, I see that we still might get rxerr set on the fifostat register which indicates that DMA was not able to drain data fast enough from RX FIFO to cause it to overflow. This is with execution in place straight out of flash.

#![no_std]
#![no_main]

use defmt::info;
use embassy_executor::Spawner;
use embassy_imxrt::pac;
use embassy_imxrt::uart::{Async, Uart, UartRx};
use embassy_imxrt::{bind_interrupts, peripherals, uart};
use embassy_time::Timer;
use {defmt_rtt as _, embassy_imxrt_examples as _, panic_probe as _};

const BUFLEN: usize = 1024;
const POLLING_RATE_US: u64 = 1000;

bind_interrupts!(struct Irqs {
    FLEXCOMM4 => uart::InterruptHandler<peripherals::FLEXCOMM4>;
});

static mut RX_BUF: [u8; BUFLEN] = [0; BUFLEN];

#[embassy_executor::task]
async fn reader(mut rx: UartRx<'static, Async>) {
    info!("Reading...");
    let mut byte_counter: usize = 0;
    let mut loop_counter: usize = 0;
    let mut buf = [0; BUFLEN / 2];

    loop {
        let read_len = loop_counter % (BUFLEN / 2) + 1;

        info!("Total bytes read: {}, read length: {}", byte_counter, read_len);

        rx.read(&mut buf[..read_len]).await.unwrap();

        for (i, b) in buf[..read_len].iter().enumerate() {
            if *b != (byte_counter % 256) as u8 {
                info!("buf: {:?}", &buf[..read_len]);

                info!(
                    "Data mismatch at index {}: expected {}, got {}",
                    i,
                    (byte_counter % 256) as u8,
                    *b
                );

                info!("dma_buffer: {:?}", unsafe { RX_BUF });

                for (j, s) in unsafe { RX_BUF }.iter().enumerate() {
                    if *s != (j % 256) as u8 {
                        info!(
                            "RX_BUF mismatch at index {}: expected {}, got {}",
                            j,
                            (j % 256) as u8,
                            *s
                        );
                    }
                }

                let regs = unsafe { &*crate::pac::Usart4::ptr() };

                info!("fifostat = 0x{:X}", regs.fifostat().read().bits());

                panic!(
                    "data corruption detected at byte_counter {} (loop_counter {})",
                    byte_counter, loop_counter
                );
            }
            byte_counter = byte_counter.wrapping_add(1);
        }
        loop_counter = loop_counter.wrapping_add(1);
    }
}

#[embassy_executor::main]
async fn main(spawner: Spawner) {
    let p = embassy_imxrt::init(Default::default());

    info!("UART test start");

    let uart = Uart::new_async_with_buffer(
        p.FLEXCOMM4,
        p.PIO0_29,
        p.PIO0_30,
        Irqs,
        p.DMA0_CH9,
        p.DMA0_CH8,
        Default::default(),
        unsafe { &mut *core::ptr::addr_of_mut!(RX_BUF) },
        POLLING_RATE_US,
    )
    .unwrap();
    let (mut tx, rx) = uart.split();
    spawner.must_spawn(reader(rx));

    info!("Writing...");

    let mut data: [u8; BUFLEN / 2] = [0; BUFLEN / 2];
    for (i, b) in data.iter_mut().enumerate() {
        *b = (i % 256) as u8;
    }

    info!("data = {:?}", data);

    loop {
        tx.write(&data).await.unwrap();
    }
}

[Copilot]

Pull request overview

Copilot reviewed 3 out of 5 changed files in this pull request and generated 11 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 5 changed files in this pull request and generated 6 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Neil-alter]

Hi @jerrysxie , I've tested this code with approximately 50,000 bytes of data transmission. However, you're right that more comprehensive testing is needed, especially with varying read sizes and pattern verification. I think it might cause by when using error xfer_count to read from partially filled buffers, and I have updated this change.

[JamesHuard]

In terms of strategy, here's my thoughts.

We've tested this PR in its current form and it does solve the performance issues we're facing. It also introduces a few new issues, particularly around power consumption.

There's overall design/constructive problems. A few have been called out directly in comments -- there's probably a few more worth discussing too.

There's quite a few code quality improvements that can be made on top of the design considerations, or even in isolation.

The only flat-out logic error that stands out to me is the one that Jerry caught: https://github.com/OpenDevicePartnership/embassy-imxrt/pull/525/files#r2696858157

Interestingly, we haven't hit this case.

Here's my suggestion-- let's fix the logic error, then merge this in. Then, we should have at least 4 more follow up PRs or work streams, for the following:

• Address the structural/design in a less time constrained, more design-review setting. In particular, there's a couple of different directions here that warrant at least discussion (new type BufferedUart instead of embedding in existing; rework existing DMA Read to allow early exit when bus is "idle" instead of doing polling internally; etc..)
• Address the code quality. There're around 35 comments pertaining to this already. There's a fair bit of complexity that can be removed with some more careful loops/conditionals. There's a lot of state scattered about, making maintenance difficult due to tripping hazards.
• Fix the full duplex UART bug -- the current waker model in Info will cause us to drop the Rx waker if a Tx occurs while Rx is being polled. An easy solution here is to have separate Rx and Tx wakers within the Info struct.
• Fix the power consumption problem introduced by adding constant background UART polling. There's a number of ways we can go about this, leveraging the start bit detection interrupt is one path that's been investigated. Adjusting the design so that we don't have to poll at this level is another.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 5 changed files in this pull request and generated 9 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Outdated.