Change DMA to have a non-static requirement

[andrewgazelka]

Background

Even though it is mentioned in the comments of some of the examples, the memory accessed by DMA does not necessarily need to be on the stack. If we defined memory.x to have our stack somewhere where DMA can access, we can pass in a stack buffer just fine.

Changes

• Update comments referencing stack requirement
• Change Static{Read,Write}Buffer -> Static{Read,Write}Buffer

[mattico]

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?

[andrewgazelka]

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

[andrewgazelka]

I added an unsafe variant: unsafe fn init_unsafe(...)

[richardeoin]

I see the usefulness of this, but I'm not entirely convinced this is "correct" to have in the HAL either. As noted in the embedded-dma docs, it's already possible to "escape" the 'static requirement by unsafe impling StaticWriteBuffer for a your own types. I think this method essentially does the same thing, but less verbose (no separate unsafe impl).

There's also a PR open rust-embedded/embedded-dma#10 that I think I support, which would remove the current WriteBuffer. If that PR was merged, then we'd actually have to do some more work to try to implement init_unsafe again (and I don't think we want to do that).

[richardeoin]

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

[andrewgazelka]

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

[andrewgazelka]

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

[andrewgazelka]

I see the usefulness of this, but I'm not entirely convinced this is "correct" to have in the HAL either. As noted in the embedded-dma docs, it's already possible to "escape" the 'static requirement by unsafe impling StaticWriteBuffer for a your own types. I think this method essentially does the same thing, but less verbose (no separate unsafe impl).

There's also a PR open rust-embedded/embedded-dma#10 that I think I support, which would remove the current WriteBuffer. If that PR was merged, then we'd actually have to do some more work to try to implement init_unsafe again (and I don't think we want to do that).

Ah. I wasn't aware of the StaticWriteBuffer pattern. I agree then that is probably that way to go then.

[andrewgazelka]

@richardeoin Correct me if I am wrong, but this is a valid concern. We could also wait to for this to get fixed in embedded-dma.

[andrewgazelka]

closing in favor of #313