Rp pico BufferedUart drop bug after split

[tact1m4n3]

I have noticed that after splitting the BufferedUart and dropping the tx, the uart stops reading. I have quickly traced the problem in the Drop implementation of both BufferedUartRx and BufferedUartTx.

        let state = T::buffered_state();
        unsafe { state.tx_buf.deinit() }
        
        // RX is inactive if the the buffer is not available.
        // We can now unregister the interrupt handler
        if state.rx_buf.is_empty() {
            T::Interrupt::disable();
        }

Interrupts get disabled when either the rx or tx are dropped. This happens because rx_buf.is_empty() does not actually check if the buffer is not available but if it is empty. It might be empty at a certain time but that does not mean it is not available.

        let state = T::buffered_state();
        unsafe { state.tx_buf.deinit() }

        // RX is inactive if the buffer is not available.
        // We can now unregister the interrupt handler
        if state.rx_buf.len() == 0 {
            T::Interrupt::disable();
        }

This is a relatively easy fix but we quickly arrive at another issue. The interrupt does not actually check if the buffer is initialized or not. And because the drop function deinitializes the buffer we get a panic:

0.119691 ERROR panicked at library/core/src/panicking.rs:220:5:
unsafe precondition(s) violated: slice::from_raw_parts_mut requires the pointer to be aligned and non-null, and the total size of the slice not to exceed `isize::MAX`

This comes from the ring buffer because after the buffer is uninitialized the pointer to the data becomes null. After adding the same checks as in the drop method in the interrupt handler we get rid of all the bugs and everything works as expected.

I suggest we change the RingBuffer struct by adding some methods for checking if it is initialized or not. Fun fact: clippy actually suggests to replace state.rx_buf.len() == 0 with s.tx_buf.is_empty() which is wrong. Maybe doing some renaming of those methods is also important.
In addition, maybe we can also do a rewrite of the RingBuffer as it is quite unsafe at the moment.

I would like to create a pull request to fix this bug by adding the necessary checks and maybe renaming some of the methods inside the RingBuffer. Is this ok with you?

[tact1m4n3]

To reproduce the bug use this code:

#![no_std]
#![no_main]

use embassy_executor::Spawner;
use embassy_rp::{bind_interrupts, peripherals, uart};
use embedded_io_async::Read;
use static_cell::StaticCell;
use {defmt_rtt as _, panic_probe as _};

bind_interrupts!(struct UartIrqs {
    UART0_IRQ => uart::BufferedInterruptHandler<peripherals::UART0>;
});

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

    static UART_TX_BUF: StaticCell<[u8; 64]> = StaticCell::new();
    let uart_tx_buf = &mut UART_TX_BUF.init([0; 64])[..];

    static UART_RX_BUF: StaticCell<[u8; 64]> = StaticCell::new();
    let uart_rx_buf = &mut UART_RX_BUF.init([0; 64])[..];

    let uart = uart::BufferedUart::new(
        p.UART0,
        UartIrqs,
        p.PIN_12,
        p.PIN_13,
        uart_tx_buf,
        uart_rx_buf,
        uart::Config::default(),
    );
    let (uart_rx, _uart_tx) = uart.split();
    spawner.spawn(rx_task(uart_rx)).unwrap();
}

#[embassy_executor::task]
async fn rx_task(mut uart_rx: uart::BufferedUartRx<'static, peripherals::UART0>) {
    let mut buf: [u8; 64] = [0; 64];
    loop {
        match uart_rx.read(&mut buf).await {
            Ok(n) => defmt::info!("{:?}", &buf[..n]),
            _ => defmt::error!("error"),
        }
    }
}

Nothing will be read from the uart.