Skip to content
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

Add embedded_dma feature #362

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

qwerty19106
Copy link
Contributor

I added embedded_dma feature to one can send Vec, pool::object::Object<Vec> and pool::boxed::Box<Vec> to embedded DMA as read/write buffers.
It allows to use Vec with embedded DMA of HAL crates, for example stm32f4xx_hal::dma::Transfer.

Unfortunately there is a issue that does not allow to implement embedded_dma::WriteBuffer and embedded_dma::ReadBuffer transparently. Thus it requires optional embedded_dma dependency now.

@qwerty19106 qwerty19106 force-pushed the embedded_dma_vec branch 2 times, most recently from e73b851 to e0289a4 Compare March 16, 2023 09:39
@qwerty19106
Copy link
Contributor Author

cfail failed on file cfail/ui/not-send.stderr: $DIR replaced on ui.
@korken89

@qwerty19106 qwerty19106 force-pushed the embedded_dma_vec branch 3 times, most recently from de6c859 to 0465cce Compare March 22, 2023 07:59
@qwerty19106
Copy link
Contributor Author

CI is fixed: $DIR replaced on ui within cfail/ui/not-send.stderr.
It is ready for review.

@Dirbaio
Copy link
Member

Dirbaio commented Oct 20, 2023

the impl for Vec is not sound, it doesn't satisfy this safety precondition from here:

As long as no &mut self method is called on the implementing object:

  • read_buffer must always return the same value, if called multiple times.

because you can move the vec without calling any &mut self method, which will cause the address returned by read_buffer to change.

@Dirbaio Dirbaio marked this pull request as draft October 30, 2023 23:34
@qwerty19106
Copy link
Contributor Author

qwerty19106 commented Feb 15, 2024

ReadBuffer implemented for [T; N], which can be moved without calling any &mut self method also. See ReadTarget for details.

I don't understand ReadBuffer requirements, you can give an example of type, which can implement ReadBuffer safely?

P.S. Rebased.

@Dirbaio
Copy link
Member

Dirbaio commented Feb 15, 2024

No, ReadBuffer is implemented for types that deref to [T; N] and additionally implement StableDeref, which means the deref'd-to array doesn't move even if you move the ReadBuffer.

ie it's implemented for things like &'static mut [T; N] and Box<[T; N]>, but not the raw [T; N] because that one can move. heapless::Vec is like the latter.

@qwerty19106
Copy link
Contributor Author

qwerty19106 commented Feb 16, 2024

From ReadTarget documentation:

as_read_buffer must adhere to the safety requirements documented for ReadBuffer::read_buffer.

I think that it is not valid, because StableDeref provides some of ReadBuffer requirements already.
I suggest:

  1. fix ReadTarget and WriteTarget documentation
  2. Implement ReadTarget and WriteTarget for Vec

@Dirbaio
Copy link
Member

Dirbaio commented Feb 16, 2024

I think the embedded-dma docs are incomplete. They say "as long as no &mut self methods are called", but they should also say "and self is not moved". That's the original intent of the traits. The way HALs in the wild use embedded-dma needs it, or they would be unsound.

So it's not sound to implement the traits for heapless::Vec (or for any other container that stores the data inline, not behind a pointer).

@qwerty19106
Copy link
Contributor Author

Box<[T; N]> implements ReadBuffer now, but Box<Vec<T, N>> not implements.
It is very oddly.

I think than ReadBuffer should require "and self is not moved", but ReadTarget no, because StableDeref provides it already.

@qwerty19106
Copy link
Contributor Author

Requirements of ReadBuffer trait:

The implementing type must be safe to use for DMA reads. This means:

It must be a pointer that references the actual buffer.
As long as no &mut self method is called on the implementing object:
    read_buffer must always return the same value, if called multiple times.
    The memory specified by the pointer and size returned by read_buffer must not be freed during the transfer it is used in as long as self is not dropped.

Probably need to add "and buffer is not moved" here.

Additional requirements of ReadBuffer::read_buffer method:

Once this method has been called, it is unsafe to call any &mut self methods on this object as long as the returned value is in use (by DMA).

Taking into account previous requirements it is enough.

The ReadTarget != ReadBuffer, ReadTarget requirements is the same as ReadBuffer::read_buffer method. The ReadBuffer requirements gaurantees by StableDeref trait:

impl<B, T> ReadBuffer for B where
    B: Deref<Target = T> + StableDeref + 'static,
    T: ReadTarget + ?Sized,

Thus the ReadTarget can be implemented for any newtype around [u8; N], [u16; N], et.al.
The heapless::Box<Vec<u8, N>> should implement ReadBuffer already, because heapless::Box implement Deref to Vec<u8, N>, and Vec<u8, N> implement Deref to [u8], and [u8] implement ReadTarget. But it not works factically, see embedded-dma#25 for details.

Thus I consider that we should implement ReadTarget for Vec manually, although this will have to be removed when ReadBuffer API will be changed.

@qwerty19106 qwerty19106 marked this pull request as ready for review March 13, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants