Skip to content

fix "pulseio.PulseIn maxlen is limited to 128 in esp32" #10397

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

Merged
merged 4 commits into from
Jun 11, 2025

Conversation

Sola85
Copy link

@Sola85 Sola85 commented Jun 2, 2025

Sets .flags.with_dma = 1 in RMT config that is used in pulseio.PulseIn and fixes #9234.

Quoting the documentation:

The receiver saves the incoming signals into its internal memory block or DMA buffer, in the format of rmt_symbol_word_t.
Due to the limited size of the memory block, the RMT receiver can only save short frames whose length is not longer than the memory block capacity

By my understandig, this removes the limit only being able to receive 128 pulses, which arises due to the "internal memory" being limited to a size given by SOC_RMT_MEM_WORDS_PER_CHANNEL.

In DMA-mode, self->raw_symbols has to be located in internal RAM, not PSRAM, hence the remaining changes.

I have tested this on a Waveshare ESP32-S3-Zero and in my setup I can now receive pulse sequences of length 250, which was not possible before.

The documentation also says:

However, the DMA backend is not available on all ESP chips, please refer to [TRM] before you enable this option. Or you might encounter a ESP_ERR_NOT_SUPPORTED error.

But I checked the esp-idf documentation of all esp variants, and they all reference DMA mode, if RMT hardware is available, so I'm not sure what the documentation is referring to here... If there really are some variants that support RMT but not DMA mode, then this PR would need some guards for that.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thank you very much!

I've suggested a few changes to use an API we have internally that can be used to request DMA-capable memory. It uses the same routines you called explicitly. Would you be willing to test again with these changes? Thanks.

@Sola85
Copy link
Author

Sola85 commented Jun 10, 2025

@dhalbert Thanks for the suggestions. I've applied them and will try them using the build artifacts, once they are done. I'll let you know when I've confirmed that this still works in my setup.

@Sola85
Copy link
Author

Sola85 commented Jun 10, 2025

@dhalbert Actually, port_malloc(size, true) doesn't seem to work here. I get a espidf.IDFError: Invalid argument in the constructor of PulseIn.

I know that the esp-idf rmt functions check whether the buffer is in internal ram and return this error when it is not, and I got the same error before I switched to heap_caps_malloc. I havn't checked whether the same is happening now, but thats my guess.

I noticed some funny looking logic in port_malloc for espressif:

void *port_malloc(size_t size, bool dma_capable) {
size_t caps = MALLOC_CAP_8BIT;
if (dma_capable) {
caps |= MALLOC_CAP_DMA;
}
void *ptr = NULL;
// Try SPIRAM first when available.
#ifdef CONFIG_SPIRAM
ptr = heap_caps_malloc(size, caps | MALLOC_CAP_SPIRAM);
#endif
if (ptr == NULL) {
ptr = heap_caps_malloc(size, caps);
}
return ptr;

If CONFIG_SPIRAM is defined, then MALLOC_CAP_SPIRAM is added to the caps flag, regardles of whether dma_capable is true or not.
So just by looking at the code (and not having done any further debugging) I believe this is by heap_caps_malloc worked, but port_malloc now doesn't work.

Not sure whether the logic in port_malloc is wrong (since I'm not sure whether "dma capable" and "in psram" are mutually exclusive), but I'm fairly confident that the rmt hardware requires memory that is "not in psram" and not just "dma capable".

@dhalbert
Copy link
Collaborator

I was thinking PSRAM was not DMA capable, but that's on RP2xxxx, let me rethink this! I think you may be right about the logic.

@Sola85
Copy link
Author

Sola85 commented Jun 11, 2025

Just checked the documentation here and here:

Use the MALLOC_CAP_DMA flag to allocate memory which is suitable for use with hardware DMA engines (for example SPI and I2S). This capability flag excludes any external PSRAM.

and

Because some buffers can only be allocated in internal memory, a second configuration item CONFIG_SPIRAM_MALLOC_RESERVE_INTERNAL defines a pool of internal memory which is reserved for only explicitly internal allocations (such as memory for DMA use). Regular malloc() will not allocate from this pool. The MALLOC_CAP_DMA and MALLOC_CAP_INTERNAL flags can be used to allocate memory from this pool.

So I believe that the logic in port_malloc() currently results in undefined behaviour, since we are passing MALLOC_CAP_DMA and MALLOC_CAP_SPIRAM, if dma_capable is true and CONFIG_SPIRAM is defined.

@dhalbert
Copy link
Collaborator

Thanks, so I agree that the current code in port_malloc() needs to be fixed. I'll probably push a commit to your PR to fix it.

@dhalbert
Copy link
Collaborator

@Sola85 I reworked port_malloc(), and did not test (except for a build smoke test), but see how it works for you.

@Sola85
Copy link
Author

Sola85 commented Jun 11, 2025

Thanks! I just flashed the artifacts and everything seems to be working again. No more Invalid Argument and collecting more than 128 pulses also works again.

Copy link
Collaborator

@dhalbert dhalbert 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 this fix and for working out the storage allocation questions!

@dhalbert dhalbert merged commit dae4962 into adafruit:main Jun 11, 2025
241 checks passed
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.

pulseio.PulseIn maxlen is limited to 128 in esp32
2 participants