-
Notifications
You must be signed in to change notification settings - Fork 1.3k
RP2040/235x: Push DMA data to PIO TX FIFO through ping-pong #4784
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
base: main
Are you sure you want to change the base?
Conversation
ff2cd1b to
bd96479
Compare
|
Maybe add your example above as an actual example in the repository (i.e. as part of examples/rp235x). |
c3625e6 to
d4e1f0c
Compare
|
I've added the rp235x example and tested it locally to ensure it works, but I haven't tested with the rp2040 yet because I'd prefer to get some feedback on this PR first. |
d30909d to
c1e89aa
Compare
c1e89aa to
7d00d65
Compare
| if let ControlFlow::Break(()) = fill_buffer_callback(data1) { | ||
| break; | ||
| } |
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.
I'm not sure this is sound. If the callback is too slow and the ch1 is triggered while it's still executing we're holding onto a mutable borrow while another immutable access (by the DMA) is happening.
Perhaps the channel should be disabled for the duration of this callback? Or the function should be unsafe?
If you went with marking the function as unsafe, I'm not sure how could you even guarantee soundness of this without analyzing the whole system, as other executing tasks could block the CPU for long enough to make the callback unable to meet its deadline.
| } | ||
|
|
||
| trace!("Waiting for pong transfer"); | ||
| Transfer::new(ch2.reborrow()).await; |
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.
How does this behave in case the callback runs for too long? Can it potentially hand forever?
Can it immediately fall-through even though ch2 is currently running (because we missed it finishing)?
| break; | ||
| } | ||
|
|
||
| trace!("Waiting for pong transfer"); |
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.
Technically you should have a fence here, before the other transfer is started. Though I'm quite sure that an await point makes for a fence anyway, so perhaps it's not an issue.
If you disabled the chanel for the duration of the callback then you do need a fence before re-enabling the channel. Otherwise the compiler could reorder some writes from inside the callback to after the re-enable.
This PR adds a function on
embassy_rp::pio::StateMachineTxto allow pushing continuous data to a PIO state machine through a ping-pong/double buffering mechanism, allowing one buffer to be filled while the other is being sent. This method uses 2 DMA channels withCHAIN_TOallowing no downtime between transfers (assuming the user fills their buffers in time using the given callback).Example usage to generate a 440Hz sine wave over 8 digital pins at a desired sample rate:
At 16kHz, this results in a buffer of ~8ms which looks clean (or as clean as you can get with a messy 8bit R-2R "DAC" 😄) on the oscilloscope:

Some open questions:
dma_push. I'm not knowledgeable enough about compiler internal to know how/when the compiler will re-order instructions to know where to insert them. Feedback is appreciated, but the current implementation seems to workControlFlowtoControlFlow::<(), Option<u32>>to let the user specify how much of their buffer they want to send, but that makes the API a bit ugly. Not sure what to do here, open to feedback.embassy_rp::dma, which seems to contain some duplicate code toembassy_rp::dma::StateMachineTx. I'm not sure how the maintainers want to approach this.I haven't added any examples yet. If desired, I can do that when the general design of this function is approved.
I'm pretty new to embedded dev in general, so I may have missed something obvious. Thorough code review would be appreciated.
Fixes #4190