feat: add uart abstraction #978
Conversation
|
🚀 Documentation preview deployed to https://ariel-os-docs-preview-pr978.surge.sh/dev/docs/api/ariel_os/ |
| ppi_ch1_peripheral, | ||
| ppi_ch2_peripheral, | ||
| ppi_group_peripheral, | ||
| Irqs, |
There was a problem hiding this comment.
Signed-off-by: Koen Zandberg <koen@bergzand.net>
Signed-off-by: Koen Zandberg <koen@bergzand.net>
Signed-off-by: Koen Zandberg <koen@bergzand.net>
Signed-off-by: Koen Zandberg <koen@bergzand.net>
Signed-off-by: Koen Zandberg <koen@bergzand.net>
Signed-off-by: Koen Zandberg <koen@bergzand.net>
Signed-off-by: Koen Zandberg <koen@bergzand.net>
There was a problem hiding this comment.
I rebased on main and pushed fixup commits (you may actually need to split some of them as I don't think they're all properly attributed to the right commit).
Among other things, I made sure the docs were rendered, and added a UART column to the support matrix.
Successfully ran the test in hardware on:
bbc-microbit-v2espressif-esp32-c3-lcdkitespressif-esp32-c6-devkitc-1nrf52840dknrf5340dkrpi-pico-wst-nucleo-c031c6st-nucleo-h755zi-qst-nucleo-wb55
tests/uart-loopback/Cargo.toml
Outdated
There was a problem hiding this comment.
This test also needs a README.md instructing to connect the RX and TX pins together.
There was a problem hiding this comment.
I'm considering changing the test completely, the loopback test is flawed for uart for multiple reasons
- settings always match, so baud rate and stop / parity bits never get tested
- Most boards have some form of uart to usb bridge on the first uart pinout. Adding a jumper between the TX and RX connects the bridge TX and the mcu TX pins, causing issues.
| apps: | ||
| - name: uart-loopback | ||
| selects: | ||
| context: |
There was a problem hiding this comment.
This list must be in sync with the MCUs/boards actually supported.
| @@ -79,6 +80,8 @@ embassy-usb = { version = "0.4.0", default-features = false } | |||
|
|
|||
| embedded-hal = { version = "1.0.0", default-features = false } | |||
| embedded-hal-async = { version = "1.0.0", default-features = false } | |||
| embedded-io = { version = "0.6.1", default-features = false } | |||
There was a problem hiding this comment.
I may be mistaken, but I don't think it's actually used.
| embedded-io = { version = "0.6.1", default-features = false } |
| usb = ["ariel-os-embassy/usb"] | ||
| ## Enables USB HID support. | ||
| usb-hid = ["ariel-os-embassy/usb-hid"] | ||
| ## Enables UART support. | ||
| uart = ["ariel-os-embassy/uart"] |
There was a problem hiding this comment.
| usb = ["ariel-os-embassy/usb"] | |
| ## Enables USB HID support. | |
| usb-hid = ["ariel-os-embassy/usb-hid"] | |
| ## Enables UART support. | |
| uart = ["ariel-os-embassy/uart"] | |
| ## Enables UART support. | |
| uart = ["ariel-os-embassy/uart"] | |
| usb = ["ariel-os-embassy/usb"] | |
| ## Enables USB HID support. | |
| usb-hid = ["ariel-os-embassy/usb-hid"] |
| #[ariel_os::task(autostart, peripherals)] | ||
| async fn main(peripherals: pins::Peripherals) { | ||
| // Delay to segment individual runs on the logic analyzer. | ||
| Timer::after_millis(500).await; |
There was a problem hiding this comment.
I think we should remove these delays before merging; that's the kind of thing that'd make HIL testing take one more second for no reason.
| } | ||
|
|
||
| #[cfg(context = "esp32c3")] | ||
| define_uart_drivers!(UART0); |
There was a problem hiding this comment.
It seems there's also a UART1 on the C3; is there a reason for not supporting it? If so there should be a comment here explaining why.
| uart_tx: GPIO16, | ||
| uart_rx: GPIO17, |
There was a problem hiding this comment.
This does work for me on the dfrobot-firebeetle2-esp32-c6, but not on the espressif-esp32-c6-devkitc-1; it does however if I select other pins, I suppose this is because this board has a UART chip for communication with the host as well. I think we should pick other pins.
There was a problem hiding this comment.
Yup, see also #978 (comment)
| info!("Starting UART test"); | ||
|
|
||
| let mut config = hal::uart::Config::default(); | ||
| config.baudrate = 9600; |
There was a problem hiding this comment.
The following does work as it should (not suggesting we add this):
config.parity = ariel_os::uart::Parity::Even;| //! Provides HAL-agnostic UART-related types. | ||
|
|
||
| /// UART parity. | ||
| #[derive(Copy, Clone, PartialEq, Eq)] |
There was a problem hiding this comment.
These types need Display and conditionally Format as well
| ); | ||
| #[cfg(context = "stm32f401re")] | ||
| define_uart_drivers!( | ||
| USART1 => USART1, |
There was a problem hiding this comment.
Seems to have a USART2 and USART6 as well
| info!("Selected baud rate: {}", config.baudrate); | ||
|
|
||
| let mut rx_buf = [0u8; 32]; | ||
| let mut tx_buf = [0u8; 32]; |
There was a problem hiding this comment.
I need to think about whether we could move these buffers inside the drivers (and maybe make their sizes generics, with default values), and maybe remove the required lifetime annotation on the driver.
There was a problem hiding this comment.
If I understand correctly, there's some downsides to moving the buffers inside with generics when one of the instances need a large buffer (e.g. 2KB). When that's the case, all instances of the Uart enum are required to allocate this buffer size.
There was a problem hiding this comment.
Quick rust playground example
| @@ -235,7 +235,7 @@ define_spi_drivers!( | |||
| #[cfg(context = "nrf5340")] | |||
| define_spi_drivers!( | |||
| SERIAL2 => SERIAL2, | |||
| SERIAL3 => SERIAL3, | |||
| // SERIAL3 => SERIAL3, | |||
There was a problem hiding this comment.
We need to revisit our arbitrary peripherals allocation between I2C/SPI/UART.
|
Maybe we should also have timeouts? I think at least nRF and ESP32 currently block forever if no data is received when waiting for some. |
Description
This PR provides an abstraction layer for UART peripherals.
The focus is on the shared UART TX/RX and provide the buffered mode of the peripherals.
The drivers implement the
embedded-io-async::Read,embedded-io-async::ReadReady, andembedded-io-async::Writetraits. TheBufReadcan be added for peripherals that support it (esp_hal is the only one that doesn't implement it on the peripherals).Issues/PRs references
Closes #830
Open issues
nRF
Tested on the nRF52840 DK
stm32
Compiles, but untested
esp32
Tested on both the esp32-c6-devkitc-1 and esp32-c3-devkit-something-something
RX doesn't work yetrp
Compiles, but untested
Change checklist