Traits discussion

[thalesfragoso]

As explained in the Open Questions, we need a system of traits to mark safe buffers types for DMA.

This issue is open for anyone to propose a trait system or evaluate one that has been proposed by others. The requirements can be found in the README.

[thalesfragoso]

I would like to propose these traits for feedback:
https://github.com/ra-kete/dma-poc/blob/43c148f7c0fc1389010c49c2259eb4077a754bc6/src/traits.rs

They are an iteration over a prototype proposed by @ra-kete. I added an associated type for allow implementations to be more strict on the types they accept.

[teskje]

The traits in the prototype have meanwhile been updated to also include this associated type. The two implementations are still sufficiently different, so I would suggest you take a look at both of them.

I also added a section to the README that talks about these traits and why we think they are necessary for safe DMA.

[adamgreig]

Just throwing a past experience into the discussion. I suspect this is out of bounds for this project but maybe it's worth considering in case I'm wrong. In summary: some memory regions are not accessible to DMA, but could safely contain types which we would consider OK for DMA. Is there any way to encode that into the type system, or do we rely on runtime checking by the specific DMA implementation?

I used to work on a project with an STM32F405 microcontroller, which has a "core-coupled memory" (CCM) in addition to the usual SRAM. It's popular to put the stack here (e.g., cortex-m-rt has an example of doing exactly this) because it's fast to access and doesn't have contention with other bus masters (DMA, USB, Ethernet, etc). However the CCM cannot be accessed by the DMA master, only by the CPU.

The HAL we were using used DMA for almost all peripheral read/write, so you would commonly write:

uint16_t read_sensor(uint8_t cmd) {
    uint8_t buf[4] = {cmd, 0, 0, 0};
    read_i2c(ADDR, &buf, sizeof(buf));
    return (buf[2]<<8) | buf[3];
}

... and immediately BusFault the CPU, because the DMA tried to access the stack-allocated buffer. You had to use a statically allocated buffer which would be placed into the main SRAM. The HAL we were using obviously didn't check that the DMA address was valid, though at best that would have been a runtime panic instead of a BusFault.

We already support custom memory sections in cortex-m-rt and hope to add better support for them in the future too. It seems likely we'll end up in a situation where people might commonly put their stacks in a memory segment which is inaccessible to DMA masters, and pass those stack allocated buffers to DMA. If we could catch that at compile time somehow, great!

Edit: I see that the docstrings on the proposed traits do more or less cover this by requiring that the pointer be accessible by DMA, but I don't know how implementors of these traits could possibly guarantee that, e.g. a default implementation for &'static [u8] could be wrong.

[teskje]

The STM32F3-DISCOVERY, which is used to run all the examples in this repo, also has a CCM not accessible by DMA (which is why we place the stack in normal RAM instead). However, for this board, if you give the DMA a buffer it cannot access, it generates a transfer error and aborts the transfer, so this case can be handled gracefully at runtime (e.g. by returning a Result from Transfer::wait). I had assumed that would be the case for every DMA implementation, but apparently not.

In any case, as far as I can tell, giving the DMA a buffer it cannot access will not lead to unsafety or unsoundness, just a crash in the worst case. Still, it would be great if we could prevent this issue from happening, or at least make it less likely. I just don't see a way to encode the notion of "a DMA accessible memory region" into Rust's type system. I could imagine some DmaMemory wrapper type with a constructor that ensures a valid memory range, but that would still be only a runtime check.

Edit: I see that the docstrings on the proposed traits do more or less cover this by requiring that the pointer be accessible by DMA, but I don't know how implementors of these traits could possibly guarantee that, e.g. a default implementation for &'static [u8] could be wrong.

I think you are right. Well, an implementation could guarantee it by a runtime check, like that DmaMemory type I imagined above. That would be very restrictive and inconvenient though. We absolutely want to allow &'static [u8] DMA buffers. So I guess that requirement should be remove from the traits and instead be handled by runtime checking (either before the transfer or afterwards, depending on whether the DMA implementation supports transfer error handling or not).

[thalesfragoso]

Thanks for the insight @adamgreig , like @ra-kete I also think that this is too microcontroller specific, so I don't think that there's a way to do it generically on this trait. I think that this should be leaved for the HAL implementations, the nrf-hal does something like this, it checks at runtime if the passed pointer is in a valid region for DMA, it panics otherwise.

[adamgreig]

I have another question which again I'm not totally sure is in scope but might impact trait design, although perhaps is more about the Transfer type than the Buffer type. So far most of the discussion seems to have been about one-shot DMA transfers, but another common use case is circular transfers. A typical underlying requirement is that the DMA engine is always able to receive new data from a peripheral (e.g. an ADC performing regular conversions), without ever being in a state where it might miss data between transfers.

Obviously this is much harder to model in safe Rust since the DMA engine can mutate the buffer while you're reading it, or read it while you're writing it. In some peripherals (STM32F4 for instance) it's possible to use two separate buffers instead, which basically solves all the same use-cases but could probably work much more easily in safe Rust: the user just needs to provide the 'next' buffer before the current one is used up, and keep swapping them around. In simpler DMA engines circular buffering is supported but only on a single buffer. Often there's a half-transfer-complete status bit and/or interrupt, which potentially could be used to virtually split the single large buffer into two smaller buffers, and perhaps try to manage ownership that way. In those cases the DMA HAL driver could probably model an underlying continuous transfer as a series of one-shot transfers which the user just needs to keep initiating before the current one expires, which fits the current trait model better.

Do you think the general concept of a DMA transfer which is perpetual and provides read access to the user while the DMA engine maintains write access is worth modelling in these traits, or should we only focus on single transfers which eventually complete/error?

[teskje]

Ah, yes, we kind of ignore circular transfers in the README. That's mostly because we've been focusing on the buffer type and using one-shot transfers in the examples makes things quite a bit simpler. However, I think we should certainly also support circular DMA transfers!

I think the proposed traits actually can support that use case too. The trick to defining such a CircularTransfer type would be declaring its buffer type as:

[B; 2]: DmaWriteBuffer + 'static

(as opposed to the B: DmaWriteBuffer + 'static of the one-shot Transfer)

This way, we can still use DmaWriteBuffer::dma_write_buffer to get a pointer and length to give to the DMA controller, but we can also make the CircularTransfer give out the half of the buffer that's not currently written to.

Here is a draft of how that could look like.

The peek function can still lead to unsafety though. If the provided function takes too long, the DMA could start writing to the buffer half that's currently being read. Which would violate Rusts aliasing guarantees and lead to UB I guess? I don't know how we can fix this, short of stopping the DMA while we peek, which would defeat the purpose of having a circular transfer. Maybe we need to mark peek as unsafe? Implementations I've seen in the wild (stm32f1xx-hal, stm32l4xx-hal) don't do this though, probably for ergonomic reasons.

In any case, I don't see anything we can change in the proposed traits to make them support the circular transfer scenario better. They are reasonably well suited to it already, the remaining issue doesn't seem solvable in the type system. That said, we should add default impls for two-halfed buffers, like [[u8; N]; 2].

[adamgreig]

Sounds good. I agree that peek on a circular buffer would have to be marked unsafe for the reasons you mentioned. I think on DMA peripherals supporting double buffering it should be possible to write an entirely safe driver, where the user and the driver exchange the second buffer, and if the user takes too long, the DMA operation is stopped. At no point would the DMA automatically start writing into the buffer the user was holding to read from.

On peripherals only supporting circular operations on a single buffer, we basically need to fake double buffering support in the driver, but since the driver can't stop the DMA engine eventually writing to the half the user holds in that case, it seems it would need to be unsafe (but otherwise functional).

That said, we should add default impls for two-halfed buffers, like [[u8; N]; 2].

Yes, I think this nicely supports the double/circular buffer case.

[thalesfragoso]

@ra-kete The problem with [B; 2]: DmaWriteBuffer + 'static is when you have wrapper types that implement DmaWriteBuffer, i.e. there is nothing enforcing that the real buffer location for the second B will be just after the first.

Edit: Sorry, I think I get it now, you're bounding the array itself, not B. Makes sense.

[teskje]

@thalesfragoso I think it's still true that we can't just allow B to be anything (or even just anything that impls Dma{Read,WriteBuffer}), due to the alignment issue you mentioned. So we should be conservative in selecting the default impls. It works for arrays of u8, u16, and u32, so we can add impls for those at least. And for their MaybeUninit versions too.

[thalesfragoso]

Yes, we can't just do

unsafe impl<B: DmaWriteBuffer> DmaWriteBuffer for [B; 2] {}

But since the trait will be unsafe, the implementer will be responsible for checking this, and hopefully will be clear which types can work in this arrangement.

[teskje]

I've updated the DMA PR for the stm32f3xx-hal to use the proposed buffer traits. Once that's merged, we can hopefully get some real-world experience using them.

stm32-rs/stm32f3xx-hal#86

[apgoetz]

I'd like to second the motion for traits for circular buffers. This is a common use case in any kind of signal processing, and it would be great of there was a reference implementation for the complicated parts of trait definitions.

[burrbull]

AsSlice won't be needed soon.
rust-lang/rust#74060