Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UART owned pins + reader/writer split #210

Merged
merged 7 commits into from
Dec 25, 2021
Merged

UART owned pins + reader/writer split #210

merged 7 commits into from
Dec 25, 2021

Conversation

VictorKoenders
Copy link
Contributor

@VictorKoenders VictorKoenders commented Nov 25, 2021

This PR makes the UartPeripheral take ownership of the pins that it needs.

const XOSC_CRYSTAL_FREQ: u32 = 12_000_000; // Typically found in BSP crates

let mut peripherals = pac::Peripherals::take().unwrap();
let sio = Sio::new(peripherals.SIO);
let pins = Pins::new(peripherals.IO_BANK0, peripherals.PADS_BANK0, sio.gpio_bank0, &mut peripherals.RESETS);
let mut watchdog = Watchdog::new(peripherals.WATCHDOG);
let mut clocks = init_clocks_and_plls(XOSC_CRYSTAL_FREQ, peripherals.XOSC, peripherals.CLOCKS, peripherals.PLL_SYS, peripherals.PLL_USB, &mut peripherals.RESETS, &mut watchdog).ok().unwrap();

// This is new:
// Set up UART on GP0 and GP1 (Pico pins 1 and 2)
let pins = (
    pins.gpio0.into_mode::<FunctionUart>(),
    pins.gpio1.into_mode::<FunctionUart>(),
);

let uart = UartPeripheral::new(peripherals.UART0, pins, &mut peripherals.RESETS)
    .enable(
        uart::common_configs::_9600_8_N_1,
        clocks.peripheral_clock.into(),
    )
    .unwrap();

uart.write_full_blocking(b"Hello World!\r\n");

// this is also new
let (reader, writer) = uart.split();

writer.write_full_blocking(b"Hello World!\r\n");

UartPeripheral takes any type that implements trait ValidUartPinout<UART: UartDevice>. This is implemented for:

  • tuple (TX, RX)
  • tuple (TX, RX, CTS, RTS)
  • the new Pins struct, in which developers can hand-pick the pins they want to use.

There are 4 traits that constrain the pins to the 4 functions they can have: Tx<UART>, Rx<UART>, Cts<UART> and Rts<UART>.

These traits are also implemented for (), so each one of the fields is optional. This is desired because some boards/devices have a single-wire UART connection that can only transfer or receive, so developers can use e.g. (Gpio1, ())

@VictorKoenders
Copy link
Contributor Author

VictorKoenders commented Nov 25, 2021

TODO (self):

  • split the UART into a reader/writer struct
  • Test the new uart on real hardware
  • Test CTS and RTS

@VictorKoenders
Copy link
Contributor Author

I don't have time to work on this for a while and I'm starting a new job in January, so I'm going to open this PR without being able to test if the CTS/RTS actually works.

@VictorKoenders VictorKoenders marked this pull request as ready for review December 12, 2021 09:16
@VictorKoenders
Copy link
Contributor Author

Rebased onto the latest main

@VictorKoenders VictorKoenders changed the title [WIP] UART owned pins UART owned pins + reader/writer split Dec 12, 2021
@9names
Copy link
Member

9names commented Dec 24, 2021

Sorry it took me so long to review this one, needed to find a block of time to properly investigate + test it.
I have tested with UART0 in a loopback scenario with:

  • GPIO 0 and 1 connected, GPIO 2 and 3 connected
  • GPIO 12 and 13 connected, GPIO 14 and 15 connected
  • GPIO 28 and 1 connected, GPIO 14 and 15 connected

I also tested with UART1 in a loopback scenario with

  • GPIO 4 and 5 connected, GPIO 6 and 7 connected

And I checked with it configured for 0,1,2,3 when 12,13,14,15 were connected (just to verify that it wasn't im

TX + RX are working just fine.
Split works as expected, very minimal changes to get a TX and RX pair (very nice!)
CTS / RTS work, but we aren't handling them well (elsewhere).

Further notes while I'm here:
Disconnecting CTS stops transmission as expected, reconnecting it resumes transmission as expected.
Disconnecting TX from RX and TX keeps working, but first character after reconnecting RX results in RTS being asserted.

I did try to clear errors but no joy. Test code is here:
https://gist.github.com/9names/0a670e374097f36bff704733d25afc83

Copy link
Member

@9names 9names left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also tested with embedded-hal alpha.
Looks good to me.

@VictorKoenders would you like me to fix the merge conflicts, or will you have time to do this?

@VictorKoenders
Copy link
Contributor Author

Rebased onto the latest main

@9names 9names merged commit cc53c17 into rp-rs:main Dec 25, 2021
jannic added a commit to jannic/rp-hal that referenced this pull request Dec 26, 2021
The change to UartPeripheral in rp-rs#210 was a breaking change:
Bump the version of the HAL and all dependent BSP crates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants