-
Notifications
You must be signed in to change notification settings - Fork 181
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
Better Circular DMA support #244
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about the delay.
As an overview, I think this looks good (though I may be a bit tired to notice every detail at the moment :) )
Would you mind adding a changelog entry for the circular DMA stuff (since it looks like the only entry is from #239 which I guess this is based on)
1 => DataSize::BITS8, | ||
2 => DataSize::BITS16, | ||
4 => DataSize::BITS32, | ||
_ => panic!("Unsupported element size.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably out of scope for this PR, but maybe we should consider returning a result here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think if this fails it's a configuration bug, so I'm not sure what a Result
would give us here.
|
||
|
||
/// Return the number of elements available to read | ||
pub fn len(&mut self) -> usize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Len doesn't sound like the right name for this, perhaps elements_available
is more descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work, I just left some suggestions and questions. Could you also fix CI ?
This one is a bit tricky because it's hard to test for overruns without producing false positives, but since we are just copying data quickly here I think we're mostly okay.
if pos + read <= blen { | ||
// the read operation does not wrap around the | ||
// circular buffer, perform a single read | ||
dat[0..read].copy_from_slice(&self.buffer[pos..pos + read]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need a compiler_fence
before reading the buffer, an Acquire
fence should do here.
// the read operation wraps around the circular buffer, | ||
let left = blen - pos; | ||
// copy until the end of the buffer | ||
dat[0..left].copy_from_slice(&self.buffer[pos..blen]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need a compiler_fence
before reading the buffer, an Acquire
fence should do here.
B: core::ops::Deref + 'static, | ||
B::Target: as_slice::AsSlice<Element = TS> + Unpin, | ||
B: StableDeref, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deref
is already implied by StableDeref
, so I don't think we need it here. Also, why do you think Unpin
is needed here ?
B: StableDeref + core::ops::Deref + 'static, | ||
B::Target: AsSlice<Element = u8> + Unpin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as the one in dma.rs
.
B: StableDeref + core::ops::Deref + 'static, | ||
B::Target: as_slice::AsSlice<Element = u8> + Unpin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as the one in dma.rs
.
This is my initial draft on PR on issue #242.
The example
serial-dma-circ-len.rs
shows how to use it in more detail, but the most important changes are demonstrate in the following snippet:One thing I have noticed is that currently a lot of the responsibility in setting up DMA is coded in the using module (e.g. in the
serial
module. I've introduced asetup
function, so that it happens within the DMA module, and the setup within the serial module looks like:I have renamed the old
CircBuffer
behaviour renamed into e.g.CircDoubleBuffer
.I have only implemented this for the serial module so far.