DMA traits proposition

[thalesfragoso]

After some discussing in the focus project for a safe DMA api, we came up with an initial proposition for the necessary buffers traits.

The traits can be found in this gist: https://gist.github.com/thalesfragoso/6279a186dd1f842ea08acdee1e889dba
The gist is a slightly upgraded version from the traits found in https://github.com/ra-kete/dma-poc where more information on the problem and decisions can be found.

I'm opening this issue to discuss the flexibility and soundness of these proposed traits. I'm not sure if this is a RFC matter, if it's I can create one.

Implementations using these traits can be found here stm32-rs/stm32f3xx-hal#86 and here stm32-rs/stm32f4xx-hal#186.

CC @ra-kete

[andre-richter]

Thanks!

u64 should also get a Word impl, or is there a reason it’s not there?

[thalesfragoso]

Thanks!

u64 should also get a Word impl, or is there a reason it’s not there?

Yeah, I don't see a reason to not having it there, I just forgot about it since I usually only deal with 32bits, heh.

In another note, I would like to bring attention to some subtleties of these traits that seem to be needed in order to be flexible for HAL implementors.

• This trait behaves like StableDeref in the sense that it only guarantees a stable location if no &mut methods are called upon Self (with the exception of write_buffer in our case). This is here to allow types like Vec, this restriction doesn't apply to Self::Target.
• The location is only stable for the duration of Self, that means that Self doesn't need to be 'static, i.e. &'a &[u8]. This can be a bit subtle for most DMA APIs, because they almost always require 'static because of mem::forget, those APIs must also bound to 'static and not only to WriteBuffer/ReadBuffer (that should be properly documented). The reason we don't require 'static in the traits themselves is because it would block implementations that can deal with stack allocated buffers, like APIs that use closures to prevent memory corruption (there are some scoped threads implementations that do exactly that).

I'm okay with these points and I think they are valid, but if anyone has something against them I would like to hear their opinions.

[eldruin]

Maybe some of these explanations can be added to the documentation. It is probably helpful for implementers.

[hannobraun]

Wouldn't the Word implementations depend on what the DMA hardware supports, and need to be provided by the implementer?

[thalesfragoso]

Wouldn't the Word implementations depend on what the DMA hardware supports, and need to be provided by the implementer?

The HAL implementors will need to restrict what Word they accept, one example of doing this can be found here: https://github.com/thalesfragoso/stm32f4xx-hal/blob/dma-take2/src/dma/mod.rs#L827

I don't think leaving the trait to be only implemented by HALs would work, in the end each HAL would have their own set of traits, unless I didn't understand what you mean exactly.

[hannobraun]

You're right, I realize now that I didn't think this through.

[teskje]

We have one open bike-shedding question left: How should we name those traits?

@thalesfragoso and me have pretty much agreed to ReadBuffer and WriteBuffer, but we are unsure if we should give them a Dma prefix (making them DmaReadBuffer and DmaWriteBuffer).

The argument for having the prefix is that it avoids ambiguity, the buffers are specific to use with DMA after all. If we just used {Read,Write}Buffer in the source code that might confuse people.

The arguments against having the prefix are a) more succinct names are better and b) these traits will mostly be used in a DMA implementation context where the relation to DMA should be pretty clear anyway.

Would be good to get a few more opinions on that and the naming in general.

[hannobraun]

I think the shorter names are appropriate. Where more context is required, the user can do something like this:

use embedded_dma as dma;

impl<Buffer> ...
    where Buffer: dma::ReadBuffer

Or even this:

use embedded_dma::ReadBuffer as DmaReadBuffer;

[thalesfragoso]

New concerns and suggestions should now be directed to the new repo https://github.com/rust-embedded/embedded-dma, closing this.