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

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

Merged
merged 21 commits into from
Aug 12, 2021

Conversation

hargoniX
Copy link
Contributor

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

Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

How was this code tested? This seems potentially tricky to get right, so I'm a bit hesitant to merge this without proper testing and review (I'd appreciate others to review this for correctness as well)

nrf-hal-common/src/uarte.rs Show resolved Hide resolved
@hargoniX
Copy link
Contributor Author

hargoniX commented Aug 10, 2021

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
Copy link
Contributor

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!

lulf and others added 21 commits August 12, 2021 16:19
This adds a sub-module named "serial" that splits TX and RX parts of
UART into separate types that implements the embedded_hal::serial
interfaces.

This allows using the nRF UART for drivers that rely on the
embedded_hal::serial traits.

The buffer management is primitive in order to support the semantics
of the more restrictive embedded_hal traits.
* Allow making multi-byte DMA transfers by starting DMA only on flush().
  Read is still limited by the embedded_hal trait, so will still read
  only 1 byte at a time.
* Auto derive blocking trait from non-blocking version.
* Remove explicit lifetime for tx/rx structs
* Refactor write/read logic into functions that are reused in the
  existing combined Uarte implementation and the UartTx/UarteRx
  implementation.
@hargoniX hargoniX force-pushed the uarte-embedded-serial branch from 5a16c71 to 0ebf91b Compare August 12, 2021 14:26
@hargoniX
Copy link
Contributor Author

Rebased!

@jonas-schievink
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Aug 12, 2021
343:  Add embedded_hal::serial implementation for uarte (continuation) r=jonas-schievink a=hargoniX

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

Co-authored-by: Ulf Lilleengen <[email protected]>
Co-authored-by: Henrik Böving <[email protected]>
@jonas-schievink
Copy link
Contributor

bors retry

@jonas-schievink
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 12, 2021

Build succeeded:

@bors bors bot merged commit d7aa29e into nrf-rs:master Aug 12, 2021
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.

3 participants