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

DMA (alternative to #209) #457

Merged
merged 50 commits into from
Dec 20, 2022
Merged

DMA (alternative to #209) #457

merged 50 commits into from
Dec 20, 2022

Conversation

9names
Copy link
Member

@9names 9names commented Sep 23, 2022

Rebased #209 onto current trunk.
Updated to embedded-dma 0.2

Current plan is to drop everything except for SingleChannel, since I think that should end up being simpler and more flexible than the current approach.
To get bidirectional functionality from SingleChannel, will need to update peripherals to provide TX/RX endpoints so that the user can have 2 channels targeting the same peripheral for reading/writing.
For double-buffering I was thinking to pass the second DMA channel as an Option to the constructor, so the same interface is used regardless of single-shot/double buffering.

Will probably rename SingleChannel to Channel or Transfer or something to match the above changes.

EDIT:
After splitting it up, it looks like it already resembles the above somewhat so this might only require minor changes

Other goals:

@9names
Copy link
Member Author

9names commented Sep 25, 2022

embedded-dma constrains DMA traits to WORD sizes, where WORD is u8/u16/u32/u64/i8/i16/i32/i64.
64bit sizes compile fine but do the wrong thing at runtime. There are now tests that show this is broken, not sure about the best way to proceed to fix that.

@9names 9names force-pushed the dma-0.6 branch 2 times, most recently from f376153 to 6cd19b2 Compare September 25, 2022 10:30
@9names
Copy link
Member Author

9names commented Sep 25, 2022

Removed target tests from this branch since there's no practical way to keep CI happy with them in there. Will keep them in a separate repo for now.

@9names
Copy link
Member Author

9names commented Sep 25, 2022

Turns out the solution to the CI problem was to exclude the on-target test project from the workspace.
There's still the rest of the problems that come from having a top-level .cargo/config, but those are at least potentially resolvable.

rp2040-hal/src/pio.rs Outdated Show resolved Hide resolved
@jounathaen
Copy link

Another suggestion: Could you rename SingleBuffering to SingleBufferedTransfer (and Bidirectional accordingly)?
Found the other names a little clumsy when trying it out.

@9names
Copy link
Member Author

9names commented Dec 6, 2022

Bidir is now working for SPI, at least with my trivial tests/examples.
One blocker down.

rp2040-hal/src/dma/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@jannic jannic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still haven't had time to try this code. But as it reads nicely, contains test cases, and has been commented by multiple persons, I'd say we shouldn't wait for a more in-depth review before merging it. It has been waiting long enough.

rp2040-hal/src/dma/mod.rs Outdated Show resolved Hide resolved
@jounathaen
Copy link

I suggest resolving #528 before merging, as this would be a major API change afterwards.

@9names
Copy link
Member Author

9names commented Dec 19, 2022

Alright, it's now updated to the API discussed in #528.
Looks okay to me :) but would appreciate another pair of eyes to review how the API looks @jounathaen

@jounathaen
Copy link

I think it is very nice now! Couldn't find anything more to complain about 😉

Thank you so much for your efforts!

@9names
Copy link
Member Author

9names commented Dec 20, 2022

I did a test rebase and it looked sensible, so I'm going to trust that Github's "Squash and merge" is going to do the right thing 🤞

@9names 9names merged commit 38bb05e into rp-rs:main Dec 20, 2022
@9names 9names deleted the dma-0.6 branch June 18, 2023 09:29
This was referenced Jun 24, 2023
@9names 9names mentioned this pull request Jul 12, 2023
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.

6 participants