Rework Uart constructors, add UartTx and UartRx constuctors.

[SergioGasquez]

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

• I have updated existing examples or added new ones (if applicable).
• I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
• My changes were added to the CHANGELOG.md in the proper section.
• My changes are in accordance to the esp-rs API guidelines

Extra:

• I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

• Uart constructors now take tx and rx pins.
• Added new_with_default_pins constructor which uses the defaults Serial/UART gpios.
  • This made some examples/tests a bit noisy with some cfgs arrounds.
• Added constructor for UartTx and UartRx`.
• Introduced the following methods: with_cts, with_rts
• Removed the configure_pins methods.
• Fix Double check handling of pins used by drivers #1544

Testing

• Tested that Double check handling of pins used by drivers #1544 is fixed with the following reproducer:

Reproducer example

//! Issue 1544

//% CHIPS: esp32 esp32c2 esp32c3 esp32c6 esp32h2 esp32s2 esp32s3
#![no_std]
#![no_main]

use esp_backtrace as _;
use esp_hal::{
    clock::ClockControl,
    delay::Delay,
    gpio::{Io, Level, Output},
    peripherals::Peripherals,
    prelude::*,
    system::SystemControl,
    uart::{config::Config, Uart},
};
use esp_println::println;
use nb::block;

#[entry]
fn main() -> ! {
    let peripherals = Peripherals::take();
    let system = SystemControl::new(peripherals.SYSTEM);
    let clocks = ClockControl::boot_defaults(system.clock_control).freeze();

    let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);

    // Issue - This built before
    // let pins = TxRxPins::new_tx_rx(&mut io.pins.gpio4, &mut io.pins.gpio2);
    // let mut serial1 = Uart::new_with_config(
    //     peripherals.UART1,
    //     Config::default(),
    //     Some(pins),
    //     &clocks,
    //     None,
    // );
    // let _led = Output::new(io.pins.gpio4, Level::High);

    // Solution
    let mut serial1 = Uart::new(peripherals.UART1, &clocks, io.pins.gpio4, io.pins.gpio2);

    // let _led = Output::new(io.pins.gpio4, Level::High); // This fails to build

    let delay = Delay::new(&clocks);

    println!("Start");
    loop {
        serial1.write_byte(0x42).ok();
        let read = block!(serial1.read_byte());

        match read {
            Ok(read) => println!("Read 0x{:02x}", read),
            Err(err) => println!("Error {:?}", err),
        }

        delay.delay_millis(250);
    }
}

• Run HIL workflow: https://github.com/esp-rs/esp-hal/actions/runs/9399078057
  • uart test is failing for Xtensa devices (S2 and S3)
• Locally tested the uart, uart_async, uart_tx_rx, and uart_tx_rx_async test for H2.

[MabezDev]

In light of #1544 I wonder if we want to take the time now to always take the tx/rx pins and only make the flow control pins optional?

[bjoernQ]

In light of #1544 I wonder if we want to take the time now to always take the tx/rx pins and only make the flow control pins optional?

I guess having only TX or only RX is a valid use-case?

[MabezDev]

I guess having only TX or only RX is a valid use-case?

True, but if you only passed TX for example, you can still call read_byte() which will use the default RX pin. I wonder how other HALs handle this 🤔

[bjoernQ]

I guess having only TX or only RX is a valid use-case?

True, but if you only passed TX for example, you can still call read_byte() which will use the default RX pin. I wonder how other HALs handle this 🤔

Maybe adding type-state but that gets annoying later

[MabezDev]

I guess having only TX or only RX is a valid use-case?

So I think we should add constructors to UartTx and UartRx to take the uart instance + the relevant pin. We should also optionally add the ability to add the rts/cts pin respectively on each struct. We should then change Uart to always take the tx and rx pins.

@SergioGasquez could you make those changes?

[MabezDev]

One final note, we could also offer a Uart::new_with_default_pins, where you still need to pass the pins in, but the pins are the correct default to print out to the uart serial converter. Not sure if this is a good idea though?

[bjoernQ]

One final note, we could also offer a Uart::new_with_default_pins, where you still need to pass the pins in, but the pins are the correct default to print out to the uart serial converter. Not sure if this is a good idea though?

That would be convenient in a couple of situations - especially since the pins are different for different targets

[MabezDev]

Thanks! I think this is pretty close, just a couple more things :)

[MabezDev]

We also need UartTx/Rx::new_async methods I think looking at the generated documentation (sorry I missed that earlier!).

[MabezDev]

LGTM, thanks for working on this!