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

UART: Add wrapper around RIOT's UART-interface #39

Open
wants to merge 47 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
17995a0
Added uart
kbarning Feb 1, 2023
dd1372a
Fixed callback
kbarning Feb 1, 2023
9dad81a
Merge branch 'implement_uart' into '6-uart-wrapper'
kbarning Feb 1, 2023
5e3c472
Fix code according to @jaschman
kbarning Feb 6, 2023
fc644cf
Add init pins
kbarning Feb 6, 2023
88ff4a6
DAC: Add wrapper around RIOTs DAC-interface
Feb 6, 2023
c6fbd33
Merge branch '3-dac-pdi-wrapper' into 'main'
Feb 6, 2023
79e8e23
Fix comments
kbarning Feb 6, 2023
78d412a
Merge branch 'upstream-merge-080223' into 'main'
Flole998 Feb 7, 2023
fa26004
PWM: Add wrapper around RIOTs PWM-interface
Feb 8, 2023
4aa6841
Merge branch '2-pwm-peripheral-driver-interface-wrapper' into 'main'
Feb 8, 2023
9be601c
Fixed signature
kbarning Feb 8, 2023
cfbe7c1
Fixed closure type
kbarning Feb 8, 2023
9bff3de
Mark init pins as unsafe
kbarning Feb 8, 2023
793614b
Fix comments
kbarning Feb 8, 2023
c6ef6d5
Change drop
kbarning Feb 8, 2023
c7cc626
Introduce Phantom Data Lifetimes
kbarning Feb 9, 2023
b1f76f0
Add generics to rxstart
kbarning Feb 9, 2023
c3738ca
Add mode feature
kbarning Feb 9, 2023
581729d
Merge branch 'main' into '6-uart-wrapper'
kbarning Feb 9, 2023
0b965d2
Added feature
kbarning Feb 9, 2023
11014dd
Delete dac.rs
kbarning Feb 9, 2023
b81ee74
Delete pwm.rs
kbarning Feb 9, 2023
4d917bf
Removed wrong libs
kbarning Feb 9, 2023
2896752
Merge remote-tracking branch 'origin/impl_uart_wrapper' into impl_uar…
kbarning Feb 9, 2023
6b40cd9
Removed unused comments
kbarning Feb 10, 2023
fd5d9b3
Update uart.rs
kbarning Feb 10, 2023
fd70931
Fixed issues as suggested by chrysn
kbarning Feb 15, 2023
0c1f2dd
Add newline to cargo.toml
kbarning Feb 15, 2023
055fa14
Added new macro to init uart
kbarning Feb 19, 2023
66de5f0
Fix comments
kbarning Feb 19, 2023
1b87e04
Added scoped approach
kbarning Feb 19, 2023
a98b8e3
Add static new
kbarning Feb 19, 2023
6de785c
Added new static impl + fix doc
kbarning Feb 19, 2023
0fc9d44
Make get gpio optional
kbarning Feb 27, 2023
3ff8dd0
Remove colission detection for now
kbarning Feb 27, 2023
a9231a0
Add new scoped main
kbarning Mar 17, 2023
c3afaf4
Update src/uart.rs
kbarning Mar 21, 2023
7aff5f7
Update src/uart.rs
kbarning Mar 21, 2023
5a6ea25
Update src/uart.rs
kbarning Mar 21, 2023
cc63b4f
Update src/uart.rs
kbarning Mar 21, 2023
83856a1
Update src/uart.rs
kbarning Mar 21, 2023
b6eada9
Make internal construct fn unsafe
kbarning Mar 21, 2023
bbfa1c3
add test
kbarning Mar 21, 2023
cb8257f
Fix scope signature and add cfg to unused structs
kbarning Mar 21, 2023
e0cd794
Reordering
kbarning Mar 21, 2023
0b0c400
Cleanup
kbarning Mar 21, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,6 @@ actual_never_type = []
# This has some effects of its own (making ValueInThread fundamental), and also
# enables actual_never_type.
nightly_docs = [ "actual_never_type" ]

# On some plattforms, uart_set_mode isn't implemented yet
uart_set_mode = []
chrysn marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 3 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ pub mod msg;
#[cfg(riot_module_periph_spi)]
pub mod spi;

#[cfg(riot_module_periph_uart)]
pub mod uart;

#[cfg(riot_module_periph_adc)]
pub mod adc;

Expand Down
339 changes: 339 additions & 0 deletions src/uart.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,339 @@
//! Access to [RIOT's UART](https://doc.riot-os.org/group__drivers__periph__uart.html)
//!
//! Author: Kilian Barning <[email protected]>
#![allow(dead_code)]
kbarning marked this conversation as resolved.
Show resolved Hide resolved

use core::marker::PhantomData;
use core::ptr;

use riot_sys::libc::{c_int, c_void};
use riot_sys::*;

/// This enum representatives the status returned by various `UART`-functions
#[derive(Debug)]
#[non_exhaustive]
pub enum UartDeviceStatus {
Success,
chrysn marked this conversation as resolved.
Show resolved Hide resolved
InvalidDevice,
UnsupportedConfig,
Other(i32),
chrysn marked this conversation as resolved.
Show resolved Hide resolved
}

impl UartDeviceStatus {
/// Converts the given `c_int` into the matching Enum representation
fn from_c(n: c_int) -> Self {
const _ENODEV: c_int = -(ENODEV as c_int);
const _ENOTSUP: c_int = -(ENOTSUP as c_int);
match n {
0 => Self::Success,
_ENODEV => Self::InvalidDevice,
_ENOTSUP => Self::UnsupportedConfig,
other => Self::Other(other),
}
}
}

#[derive(Debug)]
chrysn marked this conversation as resolved.
Show resolved Hide resolved
pub enum DataBits {
Five,
Six,
Seven,
Eight,
}

impl DataBits {
fn to_c(self) -> uart_data_bits_t {
match self {
Self::Five => uart_data_bits_t_UART_DATA_BITS_5,
Self::Six => uart_data_bits_t_UART_DATA_BITS_6,
Self::Seven => uart_data_bits_t_UART_DATA_BITS_7,
Self::Eight => uart_data_bits_t_UART_DATA_BITS_8,
}
}
}

#[derive(Debug)]
pub enum Parity {
None,
Even,
Odd,
Mark,
Space,
}

impl Parity {
fn to_c(self) -> uart_parity_t {
match self {
Self::None => uart_parity_t_UART_PARITY_NONE,
Self::Even => uart_parity_t_UART_PARITY_EVEN,
Self::Odd => uart_parity_t_UART_PARITY_ODD,
Self::Mark => uart_parity_t_UART_PARITY_MARK,
Self::Space => uart_parity_t_UART_PARITY_SPACE,
}
}
}


#[derive(Debug)]
pub enum StopBits {
One,
Two,
}

impl StopBits {
fn to_c(self) -> uart_stop_bits_t {
match self {
Self::One => uart_stop_bits_t_UART_STOP_BITS_1,
Self::Two => uart_stop_bits_t_UART_STOP_BITS_2,
}
}
}

/// This struct contains the `UART` device and handles all operation regarding it
///
/// [UART implementation]: https://doc.riot-os.org/group__drivers__periph__uart.html
#[derive(Debug)]
pub struct UartDevice<'a> {
dev: uart_t,
_marker: PhantomData<&'a ()>, // We use this `PhantomData` here to make sure that the lifetime of the borrowed closure is equal to this struct
}

impl<'a> UartDevice<'a> {
/// Tries to initialize the given `UART`. Returns a Result with rather `Ok<Self>` if the UART was initialized successfully or a
chrysn marked this conversation as resolved.
Show resolved Hide resolved
/// `Err<UartDeviceStatus>` containing the error
///
/// # Arguments
///
/// * `dev` - The uart_t handle to the hardware device
/// * `baud` - The used baud rate
/// * `user_callback` The user defined callback that gets called from the os whenever new data is received from the `UART`
///
/// # Examples
/// ```
/// use riot_wrappers::uart::UartDevice;
/// let mut cb = |new_data| {
/// //do something here with the received data
/// };
/// let mut uart = UartDevice::new(uart_type_t_STM32_USART, 115200, &mut cb)
/// .unwrap_or_else(|e| panic!("Error initializing UART: {e:?}"));
/// uart.write(b"Hello from UART");
/// ```
pub fn new<F>(
dev: uart_t,
baud: u32,
user_callback: &'a mut F,
) -> Result<Self, UartDeviceStatus>
where
F: FnMut(u8),
chrysn marked this conversation as resolved.
Show resolved Hide resolved
{
unsafe {
match UartDeviceStatus::from_c(uart_init(
dev,
baud,
Some(Self::new_data_callback::<F>),
user_callback as *mut _ as *mut c_void,
Copy link
Member

Choose a reason for hiding this comment

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

This is where we still feel the shock of the leakpocalypse -- sadly, having a &a callback and a Drop implementation isn't good enough. Say I write this code:

fn init() {
    let mut f = || {...};
    let dev = UartDevice::new(0, 8600, &mut f).unwrap();
    core::mem::forget(dev);
}

then when data is received, f will be called even though it has long been deallocated.

There are two (mutually non-exclusive) ways around this:

  • Implement .new() on UartDeviceStatus<'static>, thus taking a &'static FnMut.
    • If we wanted to close the door on the second approach, we could remove the PhantomData and the lifetime, but let's not.
  • Implement a scoped approach (like scoped threads), where the UART is only active for the duration of a scope (which may easily be a fn(...) -> !), and drop gets called reliably.

I suggest to use the first approach, unless your application needs the second, in which case I'd suggest doing both. (Existing APIs in riot-wrappers often only do the second, but I've come to consider that impractical in many situations).

Copy link
Author

@kbarning kbarning Feb 17, 2023

Choose a reason for hiding this comment

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

I'm not sure if I'm misinformed here, but if I use a &'static mut F as parameter, then it's not possible anymore that the closure captures variables because you have to do something like this:

static mut CB: fn(u8) = |data| println!("New data received: {data}");
let mut uart = UartDevice::new(0, 115200, unsafe { &mut CB }).unwrap();

This is because static can only be function pointers afaik.

I also thought about taking ownership of the closure, but that does't work either because if we take the address of the closure and then return it with the Self, the address of the closure changed. All other wrappers avoid this by putting the closure on the heap e.g. with Box, but we don't have this option here because of the #![no_std] world.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, a 'static can not be such a closure -- that's what the scoped API would be for.

It's not only about not being able to Box something away. The thread's initial example would also not be sound if we had Box -- the Box would a Box<dyn FnMut() + 'a>, with the 'a ending when the function returns, but the function would need to demand + 'static to warrant passing it into the unsafe C function. The box would help with the issue of the callback not moving when stored, but as we need the drop issue solved anyway (which is most easily done with the body-runs-in-callback approach), we don't have the moving reference problem.

Copy link
Author

Choose a reason for hiding this comment

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

I have updated my old solution and have followed the scoped approach. I think this is a good compromise between comfort and safety. As long as the user doesn't get the idea to use something like core::mem::forget, he should be on the safe side. Furthermore I have now implemented an extra new method to inizialize the UART using a static callback.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, we can't rely on the user to not core::mem::forget when the consequences are unsound (use-after-free).

From the current state (which is good but does not go all the way yet), we'd need to:

  • move the safe new function from the impl<'scope> UartDevice<'scope> { ... } to a block that is impl UartDevice<'static> { ... }, because that's the scope-less situation that is sound
  • Have a function roughly like this (can be on UartDevice or free, you probably have a test application with which you can try out the ergonomics):
impl UartDevice<'scope> {
    /// ...
    ///
    /// This is the scoped version of [`new()`] that can be used if you want to use short-lived callbacks, such as
    /// closures or anything containing references. The UartDevice is deconfigured when the internal main function
    /// terminates. A common pattern around this kind of scoped functions is that `main` contains the application's
    /// main loop, and never terminates (in which case the clean-up code is eliminated during compilation).
    pub fn new_scoped<CB, M, R>(
        index: usize,
        baud: u32,
        user_callback: &'scope mut F,
        main: M,
    ) -> Result<R, UartDeviceError>
    where
        F: FnMut(u8) + Sync + 'scope,
        M: FnOnce(&mut Self) -> R,
    {
        let self_ = unsafe { ... }?;
        let result = (main)(&mut self_);
        drop(self_);
        result
    }
}

(One could also leave out the drop as it's implied -- what matters is that it happens reliably at this point, and that while the scope can safely forget the &mut, the self_ will be dropped reliably before all the references to 'scope it contains become invalid).

Copy link
Author

Choose a reason for hiding this comment

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

I thought about the problem for a bit. I still like my version better because in normal rust you can do something like this:

let vec = vec![0; 100];
std::mem::forget(vec);

This creates a memory leak without even using forget. You can do all sorts of things causing bad behavior in safe rust if you really wish. I think atm there is a good compromise between safety and ease of use. Furthermore, the normal use case would be that the user creates the UART once on startup, then it stays forever, and the destructor is never called (at least in my experience). And I also find it a bit awkward and unintuitive, that the main function runs inside the UART-Wrapper.

But that is only my opinion. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

(Reminder to never type longer text in browser windows... let's hope this second version is better)

Safe Rust code must never allow unsafe behavior. No matter how exotic or even malicious the safe code is: If it leads to undefined behavior (use-after-free, double-free, unaligned access, read while modify, or using uninitialized memory), it's always the fault of the crate that did the unsafe thing. (There are some corner cases, such as opening /proc/self/mem or overwriting libc symbols, but they're more the fault of the OS or linker).

Leaking memory is safe in Rust -- it's not undefined behavior, but more importantly, as was found out late during Rust's 1.0 phase, there are just too many ways in which one can leak memory that static analysis would not cover. Thus, our code must be safe even when memory is (intentionally or accidentally) leaked.

There are some APIs in riot-wrappers that need to deal with this already; I hope I've caught all by now that I've written before I learned the above. Using them is not too bad, I think, and falls in three categories:

  • Use in a main function.

    Yeah, it's wrapping main in possibly several layers of callbacks. But the compiler can inline expand them, and drop the cleanup code if the innermost function is an endless loop (which it often is), so it's not too bad. Indent-wise, a good pattern is to have a main function that sets up all these scopes, and then calls a fn inner_main(thread_scope, ...) -> ! { ... } that gets rid of all the indentation.

  • Use in startup functions.

    Startup functions can make use of static_cell as in the stdio_coap startup code to store their forever data. A downside you mentioned is that closures can't be named and thus not be stored in statics -- true, but these can be worked around using functions. Things get convoluted and type names truly horrible, so a viable alternative is:

  • Use workarounds:

    • Use with nightly. The naming issues of closures and the horrible type names around them can both be done away using the type_alias_impl_trait (TAIT) nightly feature. Then, the type names become simple, and anything can be stored statically easily.

      Given the current progress on async functions, I hope that TAIT is stabilized this year.

    • Put it in a box and use Box::leak to get a &'static.

      RIOT does support dynamic memory management, and while it's generally frowned upon, some dynamic allocation at startup is sometimes accepted as being a practical compromise.

Copy link
Author

Choose a reason for hiding this comment

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

Ok that makes sense. I have to say that I think I'm at the end of my knowledge here. What would you say is the best way to implement it?

Copy link
Member

Choose a reason for hiding this comment

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

The code outline is still at #39 (comment). Implementing either of the two options there would be sufficient to make this PR mergable. (I'll eventually want to have both, but do whichever works better with your use case).

Copy link
Author

Choose a reason for hiding this comment

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

Fixed :)

)) {
UartDeviceStatus::Success => Ok(Self {
dev,
_marker: PhantomData,
}),
status => Err(status),
}
}
}

/// Tries to initialize the given `UART`. Returns a Result with rather `Ok<Self>` if the UART was initialized successfully or a
/// `Err<UartDeviceStatus>` containing the error. As the name implies, the created `UART` device can <b>ONLY</b> send data
///
/// # Arguments
///
/// * `dev` - The uart_t handle to the hardware device
chrysn marked this conversation as resolved.
Show resolved Hide resolved
/// * `baud` - The used baud rate
///
/// # Examples
/// ```
/// use riot_wrappers::uart::UartDevice;
/// let mut uart = UartDevice::new_without_rx(uart_type_t_STM32_USART, 115200)
/// .unwrap_or_else(|e| panic!("Error initializing UART: {e:?}"));
/// uart.write(b"Hello from UART");
/// ```
pub fn new_without_rx(dev: uart_t, baud: u32) -> Result<Self, UartDeviceStatus> {
unsafe {
match UartDeviceStatus::from_c(uart_init(dev, baud, None, ptr::null_mut())) {
UartDeviceStatus::Success => Ok(Self {
dev,
_marker: PhantomData,
}),
status => Err(status),
}
}
}


/// Sets the mode according to the given parameters
/// Should the parameters be invalid, the function returns a Err<UartDeviceStatus::UnsupportedConfig>
/// # Arguments
/// * `data_bits` - Number of data bits in a UART frame
/// * `parity` - Parity mode
/// * `stop_bits` - Number of stop bits in a UART frame
///
/// # Examples
/// ```
/// use riot_wrappers::uart::{DataBits, Parity, StopBits, UartDevice};
/// let mut uart = UartDevice::new_without_rx(uart_type_t_STM32_USART, 115200)
/// .unwrap_or_else(|e| panic!("Error initializing UART: {e:?}"));
/// uart.set_mode(DataBits::Eight, Parity::None, StopBits::One)
/// .unwrap_or_else(|e| panic!("Error setting UART mode: {e:?}"));
/// ```
#[cfg(feature = "uart_set_mode")]
pub fn set_mode(
&mut self,
data_bits: DataBits,
parity: Parity,
stop_bits: StopBits,
) -> Result<(), UartDeviceStatus> {
unsafe {
match UartDeviceStatus::from_c(uart_mode(
self.dev,
data_bits.to_c(),
parity.to_c(),
stop_bits.to_c(),
)) {
UartDeviceStatus::Success => Ok(()),
status => Err(status),
}
}
}

/// Transmits the given data via the `UART`-device
///
/// # Examples
/// ```
/// use riot_wrappers::uart::UartDevice;
/// let mut uart = UartDevice::new_without_rx(uart_type_t_STM32_USART, 115200)
/// .unwrap_or_else(|e| panic!("Error initializing UART: {e:?}"));
/// uart.write(b"Hello from UART\n");
pub fn write(&mut self, data: &[u8]) {
chrysn marked this conversation as resolved.
Show resolved Hide resolved
unsafe {
uart_write(self.dev, data.as_ptr(), data.len() as size_t);
}
}

/// Turns on the power from the `UART-Device`
pub fn power_on(&mut self) {
unsafe { uart_poweron(self.dev) };
}

/// Turns off the power from the `UART-Device`
pub fn power_off(&mut self) {
unsafe { uart_poweroff(self.dev) };
}

/// Enables collision detection check
#[cfg(riot_module_periph_uart_collision)]
pub fn collision_detect_enable(&mut self) {
unsafe { uart_collision_detect_enable(self.dev) };
}

/// Disables collision detection check
#[cfg(riot_module_periph_uart_collision)]
pub fn collision_detect_disable(&mut self) {
unsafe { uart_collision_detect_disable(self.dev) };
}

/// Disables collision detection and returns if a collision occurred during last transfer
#[cfg(riot_module_periph_uart_collision)]
pub fn collision_detected(&mut self) -> bool {
chrysn marked this conversation as resolved.
Show resolved Hide resolved
unsafe { uart_collision_detected(self.dev) }
}

/// This function normally does not need to be called. But in some case, the pins on the `UART`
/// might be shared with some other functionality (like `GPIO`). In this case, it is necessary
/// to give the user the possibility to init the pins again.
#[cfg(riot_module_periph_uart_reconfigure)]
pub unsafe fn init_pins(&mut self) {
uart_init_pins(self.dev);
}

/// Change the pins back to plain GPIO functionality
#[cfg(riot_module_periph_uart_reconfigure)]
pub unsafe fn deinit_pins(&mut self) {
uart_deinit_pins(self.dev);
}

/// Get the RX pin
#[cfg(riot_module_periph_uart_reconfigure)]
pub fn get_pin_rx(&mut self) -> gpio_t {
chrysn marked this conversation as resolved.
Show resolved Hide resolved
unsafe { uart_pin_rx(self.dev) }
}

/// Get the TX pin
#[cfg(riot_module_periph_uart_reconfigure)]
pub fn get_pin_tx(&mut self) -> gpio_t {
unsafe { uart_pin_tx(self.dev) }
}

/// Configure the function that will be called when a start condition is detected
/// This will not enable / disable the generation of the RX start interrupt
/// # Arguments
/// * `user_fxopt` - The user defined callback function that gets called when a start condition is detected
#[cfg(riot_module_periph_uart_rxstart_irq)]
pub fn rxstart_irq_configure<F>(&mut self, user_fxopt: &'a mut F)
where
F: FnMut(),
chrysn marked this conversation as resolved.
Show resolved Hide resolved
{
unsafe {
uart_rxstart_irq_configure(
dev,
Self::rxstart_callback::<F>,
user_fxopt as *mut _ as *mut c_void,
)
};
}

/// Enable the RX start interrupt
#[cfg(riot_module_periph_uart_rxstart_irq)]
pub fn rxstart_irq_enable(&mut self) {
unsafe { uart_rxstart_irq_enable(self.dev) };
}

/// Disable the RX start interrupt
#[cfg(riot_module_periph_uart_rxstart_irq)]
pub fn rxstart_irq_disable(&mut self) {
unsafe { uart_rxstart_irq_disable(self.dev) };
}

/// This is the callback that gets called directly from the kernel if new data from the `UART` is received
/// # Arguments
/// * `user_callback` - The address pointing to the user defined callback
/// * `data` - The newly received data from the `UART`
unsafe extern "C" fn new_data_callback<F>(user_callback: *mut c_void, data: u8)
where
F: FnMut(u8),
{
(*(user_callback as *mut F))(data); //We cast the void* back to the closure and call it
chrysn marked this conversation as resolved.
Show resolved Hide resolved
}

/// This is the callback that gets called directly from the kernel when a start condition is detected
/// # Arguments
/// * `user_callback` - The address pointing to the user defined callback
#[cfg(riot_module_periph_uart_rxstart_irq)]
unsafe extern "C" fn rxstart_callback<F>(user_callback: *mut c_void)
where
F: FnMut(),
{
(*(user_callback as *mut F))(); //We cast the void* back to the closure and call it
chrysn marked this conversation as resolved.
Show resolved Hide resolved
}
}

impl<'a> Drop for UartDevice<'a> {
/// The `drop` method resets the `UART`, removes the interrupt and tries
/// to reset the `GPIO` pins if possible
fn drop(&mut self) {
unsafe {
uart_init(self.dev, 9600, None, ptr::null_mut());
#[cfg(riot_module_periph_uart_reconfigure)]
self.deinit_pins();
}
}
}