-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
docs: Add documentation for rp2 DMA support. #7670
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
Conversation
My apologies, @nickovs , I tagged "approve" by accident and am unaware of any way of rectifying this mistake. |
77dca35
to
6291f90
Compare
8ac1501
to
75b726b
Compare
@nickovs A picky note: you might do a global search/substitute of 'crtl' to 'ctrl'. In quite a few places, the letters are swapped. |
75b726b
to
1e6a356
Compare
Code size report:
|
aadbbb9
to
b95f0cb
Compare
@NewWheelTech The code as merged defaults to word transfers, so the docs are correct for what we've got. I guess there might be an argument for changing the code to match the C SDK. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7670 +/- ##
==========================================
+ Coverage 98.39% 98.40% +0.01%
==========================================
Files 161 159 -2
Lines 21156 21088 -68
==========================================
- Hits 20816 20752 -64
+ Misses 340 336 -4 ☔ View full report in Codecov by Sentry. |
b95f0cb
to
d46228e
Compare
It's not the SDK that defaults to byte transfers, but rather the register is reset to 0 and that means byte transfer size. I think it's sensible for the default CTRL value (DEFAULT_DMA_CONFIG) to default to word-sized transfers, because that's the most common use (I guess, eg for peripheral access). Then maybe the |
I would recommend not making the |
The problem with making |
Yes, agreed. |
@nickovs let me know if/when this is ready for final review and merge. |
With #7650 merged I think that this now accurately reflects the code, so please go ahead with a review and merge. |
Thanks for your efforts @nickovs, much appreciated! However, is it possible to come up with a more tangible example? It'd be great to be able to point folks at, say, a Wokwi-hosted example that provided a concise, clear and compelling use case - the examples in the docs either execute practically immediately or are quite low-level and difficult to appreciate. Maybe something like a PIO/DMA solution to control a WS2812B... |
@pi-mst If there is interest, I have exactly the the sample code requested: WS2812B run from DMA and PIO, which is running my Christmas light display. @nickovs If you want a block of the code to paste into your docs as an example, I can attach a simplified skeleton. It will take a little while to provide (few weeks before I get around to it), since right now my display is running off an older DMA interface |
I guess the question is how much code do we want to include in the docs themselves? Most of the reference pages do not include a great deal of example code. I've tried to keep the three examples here short enough that you can see what they are doing, without having a ton of code that is doing something other than the DMA functionality. As it stands there are more examples in here than in the page for Most more complex examples will end up more being an example of, say, programming the PIO, rather than using DMA. I'd argue now that DMA is merged, it might be better to start using DMA in the examples elsewhere, rather than having examples for use of PIO, SPI and other devices in the DMA docs. |
@nickovs The question about how much code is entirely valid. The other possibility, given the unusual nature of the PIO and DMA devices on the RP2, is to create a special examples section for the rp2 port. Maybe each port should have an examples section that show off specific features of that port. That way, the examples are maintained as good code, and the documentation would link to the appropriate examples section. |
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 really useful documentation, thanks for contributing it @nickovs!
docs/library/rp2.DMA.rst
Outdated
The keys for the keyword arguments can be any key returned by the :meth:`DMA.unpack_ctrl()` | ||
method. The writable values are: | ||
|
||
:param enable: ``BOOL`` Set to enable the channel (default: ``True``). |
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.
Nitpick: Suggest bool
or boolean
for these, to be inline with other docs and/or the Python type.
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 have made this change during the rebase/merge.
There is already an examples I think it would be OK to merge this now and add a more detailed DMA example later, though (maybe at the same time can remove the longer example from the .rst) - depending on Nick's schedule. EDIT: Especially as I just noted this PR is approaching its third birthday later this year! 🙀 |
Signed-off-by: Nicko van Someren <nicko@nicko.org>
Now merged. Thanks @nickovs for writing this up, it's always hard doing the docs. |
This PR provides documentation for the rp2.DMA support in PR #7641 and the associated additions to rp2.PIO in #7650.