espressif: pulseio.PulseIn - Use dma only when necessary#10499
Open
Sola85 wants to merge 2 commits intoadafruit:mainfrom
Open
espressif: pulseio.PulseIn - Use dma only when necessary#10499Sola85 wants to merge 2 commits intoadafruit:mainfrom
Sola85 wants to merge 2 commits intoadafruit:mainfrom
Conversation
* enable dma in rmt only when necessary * Document limitations of PulseIn
dhalbert
requested changes
Aug 29, 2025
| self->raw_symbols = (rmt_symbol_word_t *)m_malloc_without_collect(self->raw_symbols_size); | ||
| self->raw_symbols_size = (maxlen / 2 + 1) * sizeof(rmt_symbol_word_t); | ||
| // RMT DMA mode cannot access PSRAM -> ensure raw_symbols is in internal ram | ||
| self->raw_symbols = (rmt_symbol_word_t *)port_malloc(self->raw_symbols_size, use_dma); |
Collaborator
There was a problem hiding this comment.
Since port_malloc() allocates outside of the CircuitPython heap, I think you're going to have a storage leak here if the program stops and you restart the VM. But since PulseIn has a finaliser, you could add a port_free() for raw_symbols to common_hal_pulseio_pulsein_deinit() .
Collaborator
There was a problem hiding this comment.
Sorry for the delay in the review.
Author
No worries, actually now it's me who is delaying things. I'm still interested in this as I do need this for one of my projects, but I'm too busy at the moment. I hope to revisit this in a month or two. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a second attempt at allowing longer pulse sequences to be recorded using
pulseio.PulseInon espressif ports using the DMA backend of the RMT hardware.This is essentially the same code as in the original PR #10397, but the new logic is now only activated in cases where the existing logic didn't work, i.e. if
maxlen > 128. I believe this fixes all issues raised in #10466 and #10470:maxlen <= 128, then the old logic works fine on all ports.maxlen > 128and the DMA backend is available (i.e. the hardware supports it and it is not allocated), then recording longer pulse sequences works.maxlen > 128and the DMA backend is NOT available (i.e. the hardware doesn't support it or it is already allocated, e.g. due to an existingPulseInobject), then a"Invalid maxlen"error is raised. In this case, usingmaxlen <= 128still works.I tested all of this on a Lolin-S2-mini and a Waveshare-S3-Zero and added a sentence to the documentation regarding this.
Notes:
This is a breaking change to anyone previously using
maxlen > 128on a variant that did not support it. Previously this silently just returned shorter sequences, now this raises an error.I think it might be possible to get
maxlen > 128to work also on variants without DMA hardware because the documentation says:But this might introduce more limitations regarding use of multiple
PulseInandPulseOut, which is a can-of-worms I did not want to open in this PR.Strictly speaking the value of 128 above should be replaced by
2*SOC_RMT_MEM_WORDS_PER_CHANNEL. But since this was previously hard-coded to a maximum of 128 pulses (with only worked due to Note 2), and changing this would be another breaking change, I left this at 128.Maybe @jbrelwof and @bill88t want to try this out before it gets merged.