Skip to content

Change DMA to have a non-static requirement #271

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion examples/dma.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use stm32h7xx_hal::dma::{

use log::info;

// DMA1/DMA2 cannot interact with our stack. Instead, buffers for use with the
// DMA1/DMA2 cannot interact with our stack (using the example memory.x configuration). Instead, buffers for use with the
// DMA must be placed somewhere that DMA1/DMA2 can access.
//
// The runtime does not initialise these SRAM banks.
Expand Down
2 changes: 1 addition & 1 deletion examples/serial-dma.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use stm32h7xx_hal::dma::{

use log::info;

// DMA1/DMA2 cannot interact with our stack. Instead, buffers for use with the
// DMA1/DMA2 cannot interact with our stack (using the example memory.x configuration). Instead, buffers for use with the
// DMA must be placed somewhere that DMA1/DMA2 can access. In this case we use
// AXI SRAM.
//
Expand Down
2 changes: 1 addition & 1 deletion examples/spi-dma-rtic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use stm32h7xx_hal as hal;
// The number of bytes to transfer.
const BUFFER_SIZE: usize = 100;

// DMA1/DMA2 cannot interact with our stack. Instead, buffers for use with the
// DMA1/DMA2 cannot interact with our stack (using the example memory.x configuration). Instead, buffers for use with the
// DMA must be placed somewhere that DMA1/DMA2 can access. In this case we use
// AXI SRAM.
//
Expand Down
2 changes: 1 addition & 1 deletion examples/spi-dma.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use stm32h7xx_hal::dma::{

use log::info;

// DMA1/DMA2 cannot interact with our stack. Instead, buffers for use with the
// DMA1/DMA2 cannot interact with our stack (using the example memory.x configuration). Instead, buffers for use with the
// DMA must be placed somewhere that DMA1/DMA2 can access. In this case we use
// AXI SRAM.
//
Expand Down
88 changes: 80 additions & 8 deletions src/dma/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,13 @@ use core::{
ptr,
sync::atomic::{fence, Ordering},
};
use embedded_dma::{StaticReadBuffer, StaticWriteBuffer};

use embedded_dma::{StaticReadBuffer, StaticWriteBuffer, WriteBuffer};

use traits::{
sealed::Bits, Direction, DoubleBufferedConfig, DoubleBufferedStream,
MasterStream, Stream, TargetAddress,
};

#[macro_use]
mod macros;
Expand All @@ -106,10 +112,6 @@ pub mod bdma;
pub mod mdma;

pub mod traits;
use traits::{
sealed::Bits, Direction, DoubleBufferedConfig, DoubleBufferedStream,
MasterStream, Stream, TargetAddress,
};

/// Errors.
#[derive(PartialEq, Debug, Copy, Clone)]
Expand Down Expand Up @@ -374,7 +376,7 @@ where
}

macro_rules! db_transfer_def {
($Marker:ident, $init:ident, $Buffer:tt, $rw_buffer:ident $(, $mut:tt)*;
($([$prepend: ident],)?$Marker:ident, $init:ident, $Buffer:tt, $rw_buffer:ident $(, $mut:tt)*;
$($constraint:stmt)*) => {
impl<STREAM, CONFIG, PERIPHERAL, DIR, BUF>
Transfer<STREAM, PERIPHERAL, DIR, BUF, $Marker>
Expand Down Expand Up @@ -415,7 +417,40 @@ macro_rules! db_transfer_def {
/// * When double buffering is enabled but the `double_buf` argument is
/// `None`.
/// * When the transfer length is greater than (2^16 - 1)
pub fn $init(
///
/// # Safety
///
/// * If using an unsafe variant of this function (i.e., [Transfer::init_unsafe]),
/// the buffer must live as long as the
/// hardware DMA transfer. A case where this **wouldn't be true** is demonstrated below:
/// ```
/// use stm32h7xx_hal::dma::{PeripheralToMemory, Transfer};
///
/// let spi = spi.disable();
/// let mut short_buffer = [0u8; 1024];
/// let streams = StreamsTuple::new(dp.DMA1, ccdr.peripheral.DMA1);
/// let config = DmaConfig::default().memory_increment(true);
/// let mut transfer: Transfer<_, _, PeripheralToMemory, _, _> = unsafe {
/// Transfer::init_unsafe(streams.0, spi, &mut short_buffer[..], None, config)
/// };
///
/// transfer.start(|spi| {
/// spi.enable_dma_tx();
/// spi.inner_mut()
/// .cr1
/// .write(|w| w.ssi().slave_not_selected().spe().enabled());
/// spi.inner_mut().cr1.modify(|_, w| w.cstart().started());
/// });
///
/// mem::forget(transfer); // <<<<<<<<
///
/// loop {
/// // &ref to buffer while DMA transfer is still writing to buffer
/// info!("short_buffer = {:?}", short_buffer);
/// }
/// ```
#[allow(unused_unsafe)]
pub $($prepend)? fn $init(
mut stream: STREAM,
peripheral: PERIPHERAL,
$($mut)* memory: BUF,
Expand Down Expand Up @@ -743,7 +778,44 @@ macro_rules! db_transfer_def {
};
}

db_transfer_def!(DBTransfer, init, StaticWriteBuffer, write_buffer, mut;);
impl<STREAM, CONFIG, PERIPHERAL, DIR, BUF>
Transfer<STREAM, PERIPHERAL, DIR, BUF, DBTransfer>
where
STREAM: DoubleBufferedStream + Stream<Config = CONFIG>,
CONFIG: DoubleBufferedConfig,
DIR: Direction,
PERIPHERAL: TargetAddress<DIR>,
BUF: StaticWriteBuffer<Word = <PERIPHERAL as TargetAddress<DIR>>::MemSize>,
BUF: WriteBuffer<Word = <PERIPHERAL as TargetAddress<DIR>>::MemSize>,
Copy link
Member

Choose a reason for hiding this comment

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

The WriteBuffer bound is redundant here, so it should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so as well, but for some reason, the code doesn't compile without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it is because StaticWriteBuffer and WriteBuffer both have separate associated type Words. Weird...

{
/// Configures the DMA source and destination and applies supplied
/// configuration. In a memory to memory transfer, the `double_buf` argument
/// is the source of the data. If double buffering is enabled, the number of
/// transfers will be the minimum length of `memory` and `double_buf`.
///
/// # Panics
///
/// * When the FIFO is disabled or double buffering is enabled in
/// `DmaConfig` while initializing a memory to memory transfer.
/// * When double buffering is enabled but the `double_buf` argument is
/// `None`.
/// * When the transfer length is greater than (2^16 - 1)
pub fn init(
stream: STREAM,
peripheral: PERIPHERAL,
memory: BUF,
double_buf: Option<BUF>,
config: CONFIG,
) -> Self {
unsafe {
Transfer::init_unsafe(
stream, peripheral, memory, double_buf, config,
)
}
}
}

db_transfer_def!([unsafe], DBTransfer, init_unsafe, WriteBuffer, write_buffer, mut;);
db_transfer_def!(ConstDBTransfer, init_const, StaticReadBuffer, read_buffer;
Copy link
Contributor

@mattico mattico Nov 2, 2021

Choose a reason for hiding this comment

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

Can you explain this change more?

My understanding is that 'static is required because the hardware DMA transfer can continue regardless if the Transfer is forgotten. For example:

    let spi = spi.disable();
    let mut short_buffer = [0u8; 1024];
    let streams = StreamsTuple::new(dp.DMA1, ccdr.peripheral.DMA1);
    let config = DmaConfig::default().memory_increment(true);
    let mut transfer: Transfer<_, _, PeripheralToMemory, _, _> =
        Transfer::init(streams.0, spi, &mut short_buffer[..], None, config);
    
    transfer.start(|spi| {
        spi.enable_dma_tx();
        spi.inner_mut()
            .cr1
            .write(|w| w.ssi().slave_not_selected().spe().enabled());
        spi.inner_mut().cr1.modify(|_, w| w.cstart().started());
    });

    mem::forget(transfer); // <<<<<<<<

    loop {
        // &ref to buffer while DMA transfer is still writing to buffer
        info!("short_buffer = {:?}", short_buffer);
    }

This is undefined behavior because the compiler can assume that the memory doesn't change between subsequent reads of &-references. From a quick search I can't find anything documenting this explicitly, but it's apparently an open question if it's even valid to use volatile reads on memory altered by external "threads" - which wouldn't be a question if it were valid for &-references.

Perhaps there could be an unsafe constructor for non-'static buffers which has the invariant that the buffer must live as long as the hardware DMA transfer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot about this. I think an unsafe constructor is a good solution, I can't think of a safe way to do this.

assert!(DIR::direction() != DmaDirection::PeripheralToMemory));

Expand Down