nrf-rs / nrf-hal

A Rust HAL for the nRF family of devices
Apache License 2.0
503 stars 139 forks source link

Add embedded_hal::serial implementation for uarte (continuation) #343

Closed hargoniX closed 3 years ago

hargoniX commented 3 years ago

Since #281 has been abandoned by the author this is a continuation of his work in a seperate PR.

hargoniX commented 3 years ago

I tested the UARTE with this little program on the micro:bit:

#![no_main]
#![no_std]

use panic_halt as _;

use core::fmt::Write;

use microbit::{
    hal::uarte,
    hal::uarte::{Baudrate, Parity},
    hal::prelude::*,
};

static mut TX_BUF: [u8; 1] = [0; 1];
static mut RX_BUF: [u8; 1] = [0; 1];

use cortex_m_rt::entry;

#[entry]
fn main() -> ! {
    let board = microbit::Board::take().unwrap();

    let (mut tx, mut rx) = {
        let serial = uarte::Uarte::new(
            board.UARTE0,
            board.uart.into(),
            Parity::EXCLUDED,
            Baudrate::BAUD115200,
        );
        serial.split(
            unsafe { &mut TX_BUF },
            unsafe { &mut RX_BUF }
        ).unwrap()
    };

    loop {
        write!(tx, "Hello World:\r\n").unwrap();
        let input = nb::block!(rx.read()).unwrap();
        write!(tx, "You wrote: {}\r\n", input as char).unwrap();
    }
}

As well as bigger variants of the TX_BUF size and it all seems to work. You were indeed right about the write not being functional though so I added some more logic that should make sure the flags are always properly reset and we only return WouldBlock if the TX transaction has both been started and is not finished yet.

One thing I am not 100% sure about is the implementation of the Write trait on the UarteTx struct, at the moment it will stick to the behaviour of waiting until the buffer is full before flushing it so it can happen that the output of a write! call is not at all or only partially printed depending on the buffer and input size. This could however of course simply be fixed by having a .flush() call in the Write trait implemenation. However I am not quite sure about whether that's something we want? One could argue that if a user needs this behaviour they can just write a newtype wrapper around the current UarteTx struct and add the explicit flush (of course we could do that as well). That being said I don't know which of these two options (no flush or autoflush) should be the default for this implementation.

jonas-schievink commented 3 years ago

Yeah, I'm not entirely sure what the intended behavior from embedded-hal is there, but it seems fine to just write to a buffer for now. If this is wrong, the embedded-hal docs should be clarified to forbid this.

This PR still needs a rebase, but other than that I think it's ready to land. Thanks!

hargoniX commented 3 years ago

Rebased!

jonas-schievink commented 3 years ago

bors r+

jonas-schievink commented 3 years ago

bors retry

jonas-schievink commented 3 years ago

bors r+

bors[bot] commented 3 years ago

Build succeeded: