Add embedded_hal::serial implementation for uarte #281
Conversation
|
This is unsound. A user can call To make this sound you need to require UarteRx and UarteTx to be pinned before use. |
|
You also need to stop DMA on drop in UarteRx/UarteTx. |
|
You could also wait for the DMA to complete and have a blocking implementation that never returns |
|
In that case we should just implement |
|
@timokroeger @jonas-schievink @Dirbaio Thank you for the review. I had not considered the case where it could be moved. Could an alternative approach to pinning, be to pass the tx and rx buffers to split() as I have pushed now? I realize this move more work to the user on initialization, but it seems more flexible and could potentially allow implementations to do larger DMA transfers if buffer sizes allow it. I'm not sure if it would still be sound/unsound (intuitively I think the compiler would prevent moving the mutable tx/rx buf slices?). I have also made an attempt at implementing Drop. |
|
They can still be leaked, and then the mutable slices can be freely used again, so this is still unsound |
|
@jonas-schievink I've added a required 'static lifetime for the buffers, I would like to avoid introducing the heapless crate as a dependency. I wasn't sure if it would be more appropriate to rely on the embedded_dma traits instead, wdyt? I've also made it so that one can provide a larger tx buffer and do multibyte DMA if desirable within the contract of the serial::Write trait. |
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.
… does not affect memory location for DMA.
* 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.
lulf
force-pushed
the
uarte-embedded-serial
branch
from
d8d5ea1 to
7fef6b2
Compare
Co-authored-by: Jonas Schievink <[email protected]>
|
Hi, I stumbled upon this PR during the rewrite of the discovery book to micro:bit v1/v2. Basically I need to write code that is compatible with both the microbit v1 (nrf51) and v2 (nrf52833). Since the nrf52833-hal exposes only the UARTE peripheral the read() methods of both HALs are however not compatible with each other (UARTE requires an rx buffer, nrf51 just returns a single byte). Is there any chance this (or the other two PRs mentioned in the original PR message...which I couldn't find) will be merged soon / can I do something to make that happen? It's sadly really blocking me on the discovery book at the moment. |
| pub trait Instance: Deref<Target = uarte0::RegisterBlock> + sealed::Sealed { | ||
| fn ptr() -> *const uarte0::RegisterBlock; |
There was a problem hiding this comment.
We shouldn't need both the Deref supertrait and the ptr method here
There was a problem hiding this comment.
Is that it already? Shall I just fork @lulf then and "re PR" this I guess?
There was a problem hiding this comment.
I should take a closer look at this, but that's one thing I noticed
There was a problem hiding this comment.
So after reading a bit through the current implementation I saw that it still uses this Deref super trait https://github.com/nrf-rs/nrf-hal/blob/master/nrf-hal-common/src/uarte.rs#L452 (as well as other peripherals) so I'm quite unsure how to get rid off it? (I do get why the ptr() can be get rid off of course). I'm unfortunately not really familiar with the project so could you go into a bit of detail as to why we don't need it here?
There was a problem hiding this comment.
I'd like to remove the Deref supertrait, since it's essentially redundant with the ptr method. I guess this doesn't have to be done in this PR though.
| pub fn split( | ||
| self, | ||
| tx_buf: &'static mut [u8], | ||
| rx_buf: &'static mut [u8; 1], |
There was a problem hiding this comment.
The docs should explain why this is a fixed-size array instead of a slice
|
|
||
| let in_progress = uarte.events_txstarted.read().bits() == 1; | ||
| // Stop any ongoing transmission | ||
| if in_progress { |
There was a problem hiding this comment.
Shouldn't we reset events_txstarted in this if block? Otherwise it seems to me that it'd stay on indefinitely
|
|
||
| let in_progress = uarte.events_rxstarted.read().bits() == 1; | ||
| // Stop any ongoing reception | ||
| if in_progress { |
There was a problem hiding this comment.
Same here for events_rxstarted
|
Continued in #343. |
|
@hargoniX Sorry, but the PR was not really abandoned from my POV. I didn't get any new reviews on it and was told it would need to wait for a breaking release in order to be merged. Then I didn't really see that it had conflicts until I saw your comment just now. I've been on holiday the past few weeks, so I haven't been able to respond until now on your comment. However, I'm happy if you want to do the remaining fixes :) Next time, please ping/mention the author directly and add a longer "timeout", in case the person you ping is temporarily away and actually intended to work on it. |
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]>
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]>
63: Initial UART(E) implementation r=robyoung a=hargoniX This implements an abstraction for UARTE, very similar to the general I2C abstracttion. At the moment the example doesn't compile for nrf52833 yet, once nrf-rs/nrf-hal#281 is finally merged that should be fairly trivial to do though. Co-authored-by: Henrik Böving <[email protected]>
I figured 2 existing PRs on the UARTE interface wasn't enough, so I created another one 😆
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.
This is somewhat inspired by #102 and the TX/RX split is similar, but a lot more "dumb" without any advanced DMA and simplified only with the goal of having something that works with the embedded_hal traits with separate tx and rx.