Revert #10397

[bill88t]

This patch reverts #10397. See #10466.
I don't think the code the feature can be kept.

Ideally the underlying code could try to do it and fallback to the old behavior if it explodes,
but that would also require a good bit of documentation and testing for when and how it works.
(i.e. only first PulseIn object, only if maxlen > 128, on these chips)

[Sola85]

Yes feel free to revert until I have time to address the issues. I agree the limitations need to be documented.

However, the changes to supervisor/port.c were a genuine, unrelated bug fix by @dhalbert which just happened to be part of the PR and this fix might not be ideal to also revert.

[bill88t]

Alright, re-applied the respective commit. I'll test on S3 (that's all I have access to right now) and report how it goes.

EDIT: Works fine.

[dhalbert]

I don't think this needs to be reverted completely. I would conditionalize using DMA by checking for SOC_RMT_SUPPORT_DMA ( #10466 (comment)). If DMA is available, then I think one could always use DMA, without a length limit.

If DMA is not available (e.g. on ESP32-S2), then the length should be validated to be <=128 (or is it <128?). We would then document the length limits in a LImitations section in the shared-bindings documentation.

Am I missing something?

[bill88t]

Using small values (<128) doesn't work with the DMA logic even on s3. This broke all existing code.
And using more than one instance is also broken.

So yea, conditions would need to be checked, or a try-except logic should be applied, trying DMA then the original logic.

However when requesting >128 and DMA is unavailable, a new error would have to be shown.

In either case, since the reversion is not wanted, I'll be closing this.

[dhalbert]

OK, sorry, I missed the report in #10466 (comment) specifically. So a mostly-revert could be in order, but keeping my f999afd fix.

The "more than one instance" is also an issue, and I don't know wether the "only channels 3 and 7" is a thing

[jbrelwof]

I'm not quite sure where this is supposed to be at this point, but below are the changes I made in my build.

Specifically, take note of .mem_block_symbols = useDMA ? self->raw_symbols_size : SOC_RMT_MEM_WORDS_PER_CHANNEL,. The meaning of mem_block_symbols changes based on whether or not .flags.with_dma is set. (search for DMA backend is enabled in RMT API reference

I'm not sure whether SOC_RMT_MEM_WORDS_PER_CHANNEL is ideal for non-dma mode, but it appears to be what was used in common_hal_pulseio_pulsein_construct(...) until recently. I originally tried using self->raw_symbols_size instead with non-dma, but that caused problems with neopixel writes if I constructed a PulseIn. Probably related to neopixels potentially using RMT ram?

diff --git a/ports/espressif/common-hal/pulseio/PulseIn.c b/ports/espressif/common-hal/pulseio/PulseIn.c
index a90f9796ab..bbdf4c2f21 100644
--- a/ports/espressif/common-hal/pulseio/PulseIn.c
+++ b/ports/espressif/common-hal/pulseio/PulseIn.c
@@ -74,6 +74,11 @@ static bool _done_callback(rmt_channel_handle_t rx_chan,
 
 void common_hal_pulseio_pulsein_construct(pulseio_pulsein_obj_t *self, const mcu_pin_obj_t *pin,
     uint16_t maxlen, bool idle_state) {
+#if defined( SOC_RMT_SUPPORT_DMA ) && SOC_RMT_SUPPORT_DMA
+    bool useDMA = maxlen > 128);
+#else
+    bool useDMA = false;
+#endif    
     self->buffer = (uint16_t *)m_malloc_without_collect(maxlen * sizeof(uint16_t));
     if (self->buffer == NULL) {
         m_malloc_fail(maxlen * sizeof(uint16_t));
@@ -110,8 +115,8 @@ void common_hal_pulseio_pulsein_construct(pulseio_pulsein_obj_t *self, const mcu
         .clk_src = RMT_CLK_SRC_DEFAULT,
         // 2 us resolution so we can capture 65ms pulses. The RMT period is only 15 bits.
         .resolution_hz = 1000000 / 2,
-        .mem_block_symbols = self->raw_symbols_size,
-        .flags.with_dma = 1
+        .mem_block_symbols = useDMA ? self->raw_symbols_size : SOC_RMT_MEM_WORDS_PER_CHANNEL,
+        .flags.with_dma = useDMA
     };
     // If we fail here, the self->buffer will be garbage collected.
     esp_err_t result = rmt_new_rx_channel(&config, &self->channel);