From 4e16344c709edff8a91af614978449a75af85465 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Mon, 8 Jun 2026 20:30:26 +0200 Subject: [PATCH] improve safety --- .../examples/embassy/src/bin/async-uart-tx.rs | 13 +- va108xx/flashloader/src/main.rs | 5 +- .../examples/embassy/src/bin/async-uart-tx.rs | 13 +- vorago-shared-hal/CHANGELOG.md | 6 +- vorago-shared-hal/src/spi/asynch.rs | 123 +++++++----------- vorago-shared-hal/src/uart/mod.rs | 9 +- vorago-shared-hal/src/uart/tx_async.rs | 44 +++---- 7 files changed, 89 insertions(+), 124 deletions(-) diff --git a/va108xx/examples/embassy/src/bin/async-uart-tx.rs b/va108xx/examples/embassy/src/bin/async-uart-tx.rs index 3ef103f..67bee9b 100644 --- a/va108xx/examples/embassy/src/bin/async-uart-tx.rs +++ b/va108xx/examples/embassy/src/bin/async-uart-tx.rs @@ -62,7 +62,8 @@ async fn main(_spawner: Spawner) { InterruptConfig::new(pac::Interrupt::OC2, true, true), ); let (tx, _rx) = uarta.split(); - let mut async_tx = TxAsync::new(tx); + // Safety: We do not cancel futures. + let mut async_tx = unsafe { TxAsync::new(tx) }; let mut ticker = Ticker::every(Duration::from_secs(1)); let mut idx = 0; loop { @@ -71,12 +72,10 @@ async fn main(_spawner: Spawner) { led1.toggle(); led2.toggle(); // Safety: We are sending static lifetime slices, and not cancelling the futures. - unsafe { - async_tx - .write_all(STR_LIST[idx].as_bytes()) - .await - .expect("writing failed"); - } + async_tx + .write_all(STR_LIST[idx].as_bytes()) + .await + .expect("writing failed"); idx += 1; if idx == STR_LIST.len() { idx = 0; diff --git a/va108xx/flashloader/src/main.rs b/va108xx/flashloader/src/main.rs index 60d7686..cd57b48 100644 --- a/va108xx/flashloader/src/main.rs +++ b/va108xx/flashloader/src/main.rs @@ -101,7 +101,8 @@ mod app { tc_handler::spawn().unwrap(); tm_tx_handler::spawn().unwrap(); - let tx_async = TxAsync::new(tx); + // Safety: We do not cancel futures. + let tx_async = unsafe { TxAsync::new(tx) }; static TC_PIPE: static_cell::ConstStaticCell< embassy_sync::pipe::Pipe, @@ -285,7 +286,7 @@ mod app { loop { let read_len = cx.local.tm_rx.read(&mut buf).await; // Safety: The buffer outlives the UART TX structure. - if let Err(e) = unsafe { cx.local.uart_tx.write_all(&buf[0..read_len]).await } { + if let Err(e) = cx.local.uart_tx.write_all(&buf[0..read_len]).await { defmt::warn!("UART TX overrun error: {}", e); } } diff --git a/va416xx/examples/embassy/src/bin/async-uart-tx.rs b/va416xx/examples/embassy/src/bin/async-uart-tx.rs index 62a01ac..34fcc86 100644 --- a/va416xx/examples/embassy/src/bin/async-uart-tx.rs +++ b/va416xx/examples/embassy/src/bin/async-uart-tx.rs @@ -68,19 +68,18 @@ async fn main(_spawner: Spawner) { let uart_config = uart::Config::new_with_clock_config(clock_config); let uarta = uart::Uart::new_for_uart0(dp.uart0, pinsg.pg0, pinsg.pg1, uart_config); let (tx, _rx) = uarta.split(); - let mut async_tx = TxAsync::new(tx); + // Safety: We do not cancel futures. + let mut async_tx = unsafe { TxAsync::new(tx) }; let mut ticker = Ticker::every(Duration::from_secs(1)); let mut idx = 0; loop { defmt::println!("Current time: {}", Instant::now().as_secs()); led.toggle(); // Safety: We are sending static lifetime slices, and not cancelling the futures. - unsafe { - async_tx - .write_all(STR_LIST[idx].as_bytes()) - .await - .expect("writing failed"); - } + async_tx + .write_all(STR_LIST[idx].as_bytes()) + .await + .expect("writing failed"); idx += 1; if idx == STR_LIST.len() { idx = 0; diff --git a/vorago-shared-hal/CHANGELOG.md b/vorago-shared-hal/CHANGELOG.md index 99c5d1b..5237875 100644 --- a/vorago-shared-hal/CHANGELOG.md +++ b/vorago-shared-hal/CHANGELOG.md @@ -10,11 +10,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed -- Async TX UART functions are explicitely marked `unsafe`. +- Async TX UART and Async SPI driver constructor is explicitely marked `unsafe`. - Async TX UART `write` now returns a `TxFuture` - Empty async TX write resolves to `Poll::Ready(0)` immediately. - Async SPI API now always returns futures instead of optional futures. +### Fixed + +- Asynch drivers now borrow the buffers properly for the lifetime of the future. + ## [v0.4.0] 2026-05-19 ### Changed diff --git a/vorago-shared-hal/src/spi/asynch.rs b/vorago-shared-hal/src/spi/asynch.rs index bec1e55..88a0e48 100644 --- a/vorago-shared-hal/src/spi/asynch.rs +++ b/vorago-shared-hal/src/spi/asynch.rs @@ -319,21 +319,27 @@ impl TransferContext { } } -pub struct SpiFuture<'spi> { +pub struct SpiFuture<'spi, 'read, 'write> { bank: super::Bank, spi: &'spi mut super::Spi, empty_buffer: bool, finished_regularly: core::cell::Cell, + phantom_read: core::marker::PhantomData<(&'read (), &'write ())>, } -impl<'spi> SpiFuture<'spi> { - fn new_for_read(spi: &'spi mut super::Spi, bank: super::Bank, words: &mut [u8]) -> Self { +impl<'spi, 'read, 'write> SpiFuture<'spi, 'read, 'write> { + fn new_for_read( + spi: &'spi mut super::Spi, + bank: super::Bank, + words: &'read mut [u8], + ) -> Self { if words.is_empty() { return Self { bank, spi, empty_buffer: true, finished_regularly: core::cell::Cell::new(false), + phantom_read: core::marker::PhantomData, }; } Self::generic_init_transfer(spi, bank); @@ -367,16 +373,22 @@ impl<'spi> SpiFuture<'spi> { spi, empty_buffer: false, finished_regularly: core::cell::Cell::new(false), + phantom_read: core::marker::PhantomData, } } - fn new_for_write(spi: &'spi mut super::Spi, bank: super::Bank, words: &[u8]) -> Self { + fn new_for_write( + spi: &'spi mut super::Spi, + bank: super::Bank, + words: &'write [u8], + ) -> Self { if words.is_empty() { return Self { bank, spi, empty_buffer: true, finished_regularly: core::cell::Cell::new(false), + phantom_read: core::marker::PhantomData, }; } let index = bank as usize; @@ -402,14 +414,15 @@ impl<'spi> SpiFuture<'spi> { spi, empty_buffer: false, finished_regularly: core::cell::Cell::new(false), + phantom_read: core::marker::PhantomData, } } fn new_for_transfer( spi: &'spi mut super::Spi, bank: super::Bank, - read: &mut [u8], - write: &[u8], + read: &'read mut [u8], + write: &'write [u8], ) -> Self { if read.is_empty() || write.is_empty() { return Self { @@ -417,6 +430,7 @@ impl<'spi> SpiFuture<'spi> { spi, empty_buffer: true, finished_regularly: core::cell::Cell::new(false), + phantom_read: core::marker::PhantomData, }; } let index = bank as usize; @@ -453,13 +467,14 @@ impl<'spi> SpiFuture<'spi> { spi, empty_buffer: false, finished_regularly: core::cell::Cell::new(false), + phantom_read: core::marker::PhantomData, } } fn new_for_transfer_in_place( spi: &'spi mut super::Spi, bank: super::Bank, - words: &mut [u8], + words: &'read mut [u8], ) -> Self { if words.is_empty() { return Self { @@ -467,6 +482,7 @@ impl<'spi> SpiFuture<'spi> { spi, empty_buffer: true, finished_regularly: core::cell::Cell::new(false), + phantom_read: core::marker::PhantomData, }; } let write_idx = Self::generic_init_transfer_write_transfer_in_place(spi, bank, words); @@ -491,6 +507,7 @@ impl<'spi> SpiFuture<'spi> { spi, empty_buffer: false, finished_regularly: core::cell::Cell::new(false), + phantom_read: core::marker::PhantomData, } } @@ -540,7 +557,7 @@ impl<'spi> SpiFuture<'spi> { } } -impl<'spi> Future for SpiFuture<'spi> { +impl<'spi> Future for SpiFuture<'spi, '_, '_> { type Output = Result<(), RxOverrunError>; fn poll( @@ -570,7 +587,7 @@ impl<'spi> Future for SpiFuture<'spi> { } } -impl<'spi> Drop for SpiFuture<'spi> { +impl<'spi> Drop for SpiFuture<'spi, '_, '_> { fn drop(&mut self) { if !self.finished_regularly.get() && !self.empty_buffer { // It might be sufficient to disable and enable the SPI.. But this definitely @@ -591,7 +608,13 @@ impl<'spi> Drop for SpiFuture<'spi> { pub struct SpiAsync(pub super::Spi); impl SpiAsync { - pub fn new( + /// Construct an asynchronous SPI driver for the given SPI peripheral. + /// + /// # Safety + /// + /// The user MUST ensure that the `Drop` method of all futures generated with this driver + /// is called on transfer cancellation. By default, this does not require any special handling. + pub unsafe fn new( mut spi: super::Spi, #[cfg(feature = "vor1x")] opt_irq_cfg: Option, ) -> Self { @@ -620,13 +643,7 @@ impl SpiAsync { /// Future which read `words` from the slave. /// /// Returns [None] if the provided buffer is empty. - /// - /// # Safety - /// - /// This function stores the raw pointer of the passed data buffer. The user MUST ensure - /// that the slice outlives the data structure. If the passed slice is stack-allocated, - /// the user also MUST ensure that the `Drop` method runs on transfer cancellation. - pub unsafe fn read(&mut self, words: &mut [u8]) -> SpiFuture<'_> { + pub fn read<'read>(&mut self, words: &'read mut [u8]) -> SpiFuture<'_, 'read, '_> { let id = self.0.id; SpiFuture::new_for_read(&mut self.0, id, words) } @@ -634,13 +651,7 @@ impl SpiAsync { /// Future which writes `words` to the slave, ignoring all the incoming words. /// /// Returns [None] if the provided buffer is empty. - /// - /// # Safety - /// - /// This function stores the raw pointer of the passed data. The user MUST ensure - /// that the slice outlives the data structure. If the passed slice is stack-allocated, - /// the user also MUST ensure that the `Drop` method runs on transfer cancellation. - pub unsafe fn write(&mut self, words: &[u8]) -> SpiFuture<'_> { + pub fn write<'write>(&mut self, words: &'write [u8]) -> SpiFuture<'_, '_, 'write> { let id = self.0.id; SpiFuture::new_for_write(&mut self.0, id, words) } @@ -654,13 +665,11 @@ impl SpiAsync { /// the value of words sent in MOSI after all `write` has been sent is 0. /// /// Returns [None] if either of the provided buffers is empty. - /// - /// # Safety - /// - /// This function stores the raw pointer of the passed slices. The user MUST ensure - /// that the slice outlives the data structure. If the passed slice is stack-allocated, - /// the user also MUST ensure that the `Drop` method runs on transfer cancellation. - pub unsafe fn transfer(&mut self, read: &mut [u8], write: &[u8]) -> SpiFuture<'_> { + pub fn transfer<'read, 'write>( + &mut self, + read: &'read mut [u8], + write: &'write [u8], + ) -> SpiFuture<'_, 'read, 'write> { let id = self.0.id; SpiFuture::new_for_transfer(&mut self.0, id, read, write) } @@ -670,13 +679,7 @@ impl SpiAsync { /// `words` buffer, overwriting it. /// /// Returns [None] if the provided buffer is empty. - /// - /// # Safety - /// - /// This function stores the raw pointer of the passed slice. The user MUST ensure - /// that the slice outlives the data structure. If the passed slice is stack-allocated, - /// the user also MUST ensure that the `Drop` method runs on transfer cancellation. - pub unsafe fn transfer_in_place(&mut self, words: &mut [u8]) -> SpiFuture<'_> { + pub fn transfer_in_place<'read>(&mut self, words: &'read mut [u8]) -> SpiFuture<'_, 'read, '_> { let id = self.0.id; SpiFuture::new_for_transfer_in_place(&mut self.0, id, words) } @@ -687,68 +690,32 @@ impl embedded_hal_async::spi::ErrorType for SpiAsync { } impl embedded_hal_async::spi::SpiBus for SpiAsync { - /// Read `words` from the slave. - // - /// # Safety - /// - /// This function stores the raw pointer of the passed data buffer. The user MUST ensure - /// that the slice outlives the data structure. If the passed slice is stack-allocated, - /// the user also MUST ensure that the `Drop` method runs on transfer cancellation. async fn read(&mut self, words: &mut [u8]) -> Result<(), Self::Error> { if words.is_empty() { return Ok(()); } - unsafe { self.read(words).await } + self.read(words).await } - /// Write `words` to the slave, ignoring all the incoming words. - /// - /// # Safety - /// - /// This function stores the raw pointer of the passed data. The user MUST ensure - /// that the slice outlives the data structure. If the passed slice is stack-allocated, - /// the user also MUST ensure that the `Drop` method runs on transfer cancellation. async fn write(&mut self, words: &[u8]) -> Result<(), Self::Error> { if words.is_empty() { return Ok(()); } - unsafe { self.write(words).await } + self.write(words).await } - /// Write and read simultaneously. `write` is written to the slave on MOSI and - /// words received on MISO are stored in `read`. - /// - /// It is allowed for `read` and `write` to have different lengths, even zero length. - /// The transfer runs for `max(read.len(), write.len())` words. If `read` is shorter, - /// incoming words after `read` has been filled will be discarded. If `write` is shorter, - /// the value of words sent in MOSI after all `write` has been sent is 0. - /// - /// # Safety - /// - /// This function stores the raw pointer of the passed slices. The user MUST ensure - /// that the slice outlives the data structure. If the passed slice is stack-allocated, - /// the user also MUST ensure that the `Drop` method runs on transfer cancellation. async fn transfer(&mut self, read: &mut [u8], write: &[u8]) -> Result<(), Self::Error> { if read.is_empty() && write.is_empty() { return Ok(()); } - unsafe { self.transfer(read, write).await } + self.transfer(read, write).await } - /// Write and read simultaneously. The contents of `words` are - /// written to the slave, and the received words are stored into the same - /// `words` buffer, overwriting it. - /// - /// # Safety - /// - /// This function stores the raw pointer of the passed slice. The user MUST ensure - /// that the slice outlives the data structure. If the passed slice is stack-allocated, - /// the user also MUST ensure that the `Drop` method runs on transfer cancellation. async fn transfer_in_place(&mut self, words: &mut [u8]) -> Result<(), Self::Error> { if words.is_empty() { return Ok(()); } - unsafe { self.transfer_in_place(words).await } + self.transfer_in_place(words).await } async fn flush(&mut self) -> Result<(), Self::Error> { diff --git a/vorago-shared-hal/src/uart/mod.rs b/vorago-shared-hal/src/uart/mod.rs index e99abf8..e3768e6 100644 --- a/vorago-shared-hal/src/uart/mod.rs +++ b/vorago-shared-hal/src/uart/mod.rs @@ -1249,8 +1249,13 @@ impl Tx { self.regs.write_data(Data::new_with_raw_value(data)); } - pub fn into_async(self) -> TxAsync { - TxAsync::new(self) + /// Create an asynchronous UART driver. + /// + /// # Safety + /// + /// See [TxAsync::new] for details. + pub unsafe fn into_async(self) -> TxAsync { + unsafe { TxAsync::new(self) } } } diff --git a/vorago-shared-hal/src/uart/tx_async.rs b/vorago-shared-hal/src/uart/tx_async.rs index 355a969..a28847c 100644 --- a/vorago-shared-hal/src/uart/tx_async.rs +++ b/vorago-shared-hal/src/uart/tx_async.rs @@ -113,23 +113,26 @@ impl TxContext { } #[derive(Debug)] -pub struct TxFuture { +pub struct TxFuture<'buf> { id: Bank, empty_buffer: bool, + // Phantom used to borrow the buffer for the lifetime of the future. + phantom: core::marker::PhantomData<&'buf ()>, } -impl TxFuture { +impl<'buf> TxFuture<'buf> { /// # Safety /// /// This function stores the raw pointer of the passed data slice. The user MUST ensure /// that the slice outlives the data structure. - pub unsafe fn new(tx: &mut Tx, data: &[u8]) -> Self { + pub unsafe fn new(tx: &mut Tx, data: &'buf [u8]) -> Self { if data.is_empty() { // We can just return a dummy future which is immediately ready, no need to set up // interrupts etc. return Self { id: tx.id, empty_buffer: true, + phantom: core::marker::PhantomData, }; } TX_DONE[tx.id as usize].store(false, core::sync::atomic::Ordering::Relaxed); @@ -160,11 +163,12 @@ impl TxFuture { Self { id: tx.id, empty_buffer: false, + phantom: core::marker::PhantomData, } } } -impl Future for TxFuture { +impl Future for TxFuture<'_> { type Output = Result; fn poll( @@ -189,7 +193,7 @@ impl Future for TxFuture { /// /// It is imperative that this `Drop` method is executed to avoid undefined behaviour on /// transfer. Do *NOT* use `core::mem::forget` on the `TxFuture`. -impl Drop for TxFuture { +impl Drop for TxFuture<'_> { fn drop(&mut self) { let mut reg_block = unsafe { self.id.steal_regs() }; if !TX_DONE[self.id as usize].load(core::sync::atomic::Ordering::Relaxed) @@ -205,7 +209,11 @@ impl Drop for TxFuture { pub struct TxAsync(Tx); impl TxAsync { - pub fn new(tx: Tx) -> Self { + /// # Safety + /// + /// The user MUST ensure that the `Drop` method of all futures generated with this driver + /// is called on transfer cancellation. By default, this does not require any special handling. + pub unsafe fn new(tx: Tx) -> Self { Self(tx) } @@ -218,12 +226,7 @@ impl TxAsync { /// /// This implementation is not side effect free, and a started future might have already /// written part of the passed buffer. - /// - /// # Safety - /// - /// This function stores the raw pointer of the passed data slice. The user MUST ensure - /// that the slice outlives the data structure. - pub unsafe fn write(&mut self, buf: &[u8]) -> TxFuture { + pub fn write<'buf>(&mut self, buf: &'buf [u8]) -> TxFuture<'buf> { unsafe { TxFuture::new(&mut self.0, buf) } } @@ -234,12 +237,7 @@ impl TxAsync { /// /// This function is not side-effect-free on cancel (AKA "cancel-safe"), i.e. if you cancel (drop) a returned /// future that hasn't completed yet, some bytes might have already been written. - /// - /// # Safety - /// - /// This function stores the raw pointer of the passed data slice. The user MUST ensure - /// that the slice outlives the data structure. - pub async unsafe fn write_all(&mut self, buf: &[u8]) -> Result<(), TxOverrunError> { + pub async fn write_all(&mut self, buf: &[u8]) -> Result<(), TxOverrunError> { let fut = ::write_all(self, buf); fut.await } @@ -274,16 +272,8 @@ impl Write for TxAsync { /// /// This implementation is not side effect free, and a started future might have already /// written part of the passed buffer. - /// - /// # Safety - /// - /// This function is not `unsafe` due to the trait definition. - /// This function stores the raw pointer of the passed data slice. The user MUST ensure - /// that the slice outlives the data structure. async fn write(&mut self, buf: &[u8]) -> Result { - // Safety: We documented the safety contract. Not much else we can do here as we are bound - // by the trait definition. - unsafe { self.write(buf).await } + self.write(buf).await } async fn flush(&mut self) -> Result<(), Self::Error> {