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

Better Circular DMA support #244

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Conversation

rudihorn
Copy link
Contributor

@rudihorn rudihorn commented Jul 5, 2020

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:

    let mut circ_buffer = rx.circ_read(buf);

    // wait until we have 5 bytes
    while circ_buffer.len() < 5 {}

    let mut dat = [0 as u8; 4];
    assert!(circ_buffer.read(&mut dat[..]) == 4);

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 a setup function, so that it happens within the DMA module, and the setup within the serial module looks like:

                fn circ_read(self, buffer: &'static mut B) -> CircBuffer<u8, Self>{
                    let buffer = buffer.as_mut_slice();
                    let paddr = unsafe { &(*$USARTX::ptr()).dr as *const _ as u32 };

                    let mut buf = CircBuffer::new(buffer, self);
                    unsafe { buf.setup(paddr, Priority::MEDIUM) };
                    buf
                }

I have renamed the old CircBuffer behaviour renamed into e.g. CircDoubleBuffer.

I have only implemented this for the serial module so far.

Copy link
Member

@TheZoq2 TheZoq2 left a 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.")
Copy link
Member

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

Copy link
Member

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 {
Copy link
Member

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.

Copy link
Member

@thalesfragoso thalesfragoso left a 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]);
Copy link
Member

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]);
Copy link
Member

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.

Comment on lines +634 to +636
B: core::ops::Deref + 'static,
B::Target: as_slice::AsSlice<Element = TS> + Unpin,
B: StableDeref,
Copy link
Member

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 ?

Comment on lines +657 to +658
B: StableDeref + core::ops::Deref + 'static,
B::Target: AsSlice<Element = u8> + Unpin,
Copy link
Member

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.

Comment on lines +456 to +457
B: StableDeref + core::ops::Deref + 'static,
B::Target: as_slice::AsSlice<Element = u8> + Unpin,
Copy link
Member

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.

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.

None yet

3 participants