add support for buffered uart #314
Conversation
huseinmsft
requested review from
felipebalbi,
jerrysxie,
JamesHuard and
madeleyneVaca
as code owners
| /// Transfer is intended to be done continuously, such as for a circular buffer | ||
| pub is_continuous: bool, | ||
|
|
||
| /// Transfer is software triggerred |
There was a problem hiding this comment.
nit: typo, triggered*
| T::info().regs.fiford().as_ptr() as *const u8 as *const u32, | ||
| buffer as *mut [u8] as *mut u32, |
There was a problem hiding this comment.
Can you add a comment here describing why you're casting twice like this? If I'm following, this looks like you've got potentially a reference to some bytes (slice?) for which you're getting a pointer (instead of the ref). And then you're converting that into a u32 pointer presumably by grouping the bytes?
| // sleep until next polling epoch, or if we detect a UART transfer error event | ||
| let res = select( | ||
| // use embassy_time to enable polling the bus for more data | ||
| embassy_time::Timer::after_micros(buffer_config.polling_rate), |
There was a problem hiding this comment.
FYI: our timer implementation is still based on the RTC so we can only sleep in increments of milliseconds. This will basically just be after_millis(1)
There was a problem hiding this comment.
if we're able to get the baudrate of the UART up to 4 mbps we'll likely need to dedicate some time to configuring the time_driver with a different, faster clock
| .await; | ||
|
|
||
| match res { | ||
| Either::First(()) | Either::Second(Ok(())) => (), |
There was a problem hiding this comment.
do we need an on_drop() function in the event that we return an error from the second future?
| @@ -218,10 +219,22 @@ impl<'a> UartTx<'a, Blocking> { | |||
| } | |||
| } | |||
|
|
|||
| struct BufferConfig { | |||
| #[cfg(feature = "time")] | |||
There was a problem hiding this comment.
we should consider adding GH tasks/issues to track:
- Using a different/more flexible/dedicated hardware timer for background polling of the UART DMA buffer
- Refactoring the circular buffer to use one of the off-the-shelf embassy solutions if possible
| unsafe { | ||
| DESCRIPTORS.list[channel].reserved = 0; | ||
| if dir == Direction::MemoryToPeripheral { | ||
| DESCRIPTORS.list[channel].dst_data_end_addr = dstbase as u32; | ||
| } else { | ||
| DESCRIPTORS.list[channel].dst_data_end_addr = dstbase as u32 + (xfercount * xferwidth) as u32; | ||
| } | ||
| if dir == Direction::PeripheralToMemory { | ||
| DESCRIPTORS.list[channel].src_data_end_addr = srcbase as u32; | ||
| } else { | ||
| DESCRIPTORS.list[channel].src_data_end_addr = srcbase as u32 + (xfercount * xferwidth) as u32; | ||
| } | ||
| DESCRIPTORS.list[channel].nxt_desc_link_addr = 0; | ||
| } |
There was a problem hiding this comment.
Should be part of its own commit. Why is this needed?
| if options.is_continuous { | ||
| w.reload().enabled(); | ||
| w.clrtrig().clear_bit(); | ||
| } else { | ||
| w.reload().disabled(); | ||
| w.clrtrig().set_bit(); | ||
| } | ||
| if options.is_sw_trig { | ||
| w.swtrig().set_bit(); | ||
| } else { | ||
| w.swtrig().clear_bit(); | ||
| } |
There was a problem hiding this comment.
how about using variant()?
w.reload().variant(options.is_continuous).
clrtrig().variant(!options.is_continuous).
swtrig().variant(options.is_sw_trig)| @@ -49,6 +49,7 @@ pub struct UartTx<'a, M: Mode> { | |||
| /// Uart RX driver. | |||
| pub struct UartRx<'a, M: Mode> { | |||
| info: Info, | |||
| _buffer_config: Option<BufferConfig>, | |||
There was a problem hiding this comment.
IMO, you should produce new structs that contain a UartRx. In fact, adding a new directory uart and moving the current driver to uart/mod.rs would allow you to introduce all your new code within uart/buffered.rs. Then, you can have a new struct BufferedUartRx, struct BufferedUartTx, and BufferedUart all isolated from the regular, unbuffered uart. Additionally, you would not need a BufferConfig. A regular Config would suffice, because you would already have a separate buffered module.
| { | ||
| if self._buffer_config.is_some() { | ||
| self.read_buffered(buf).await | ||
| } else { | ||
| self.read_unbuffered(buf).await | ||
| } | ||
| } | ||
|
|
||
| #[cfg(not(feature = "time"))] | ||
| { | ||
| self.read_unbuffered(buf).await | ||
| } |
There was a problem hiding this comment.
If you had your separate buffered.rs submodule, this wouldn't be necessary. You would have your isolated read() implementation.
Implement a circular buffer for UART DMA for RX mode to allow continuous reading or data on UART RX