Moddable-OpenSource / moddable

Tools for developers to create truly open IoT products using standard JavaScript on low cost microcontrollers.
http://www.moddable.com
1.31k stars 236 forks source link

ESP32 RMT driver exposes no errors when reading #1032

Open tve opened 1 year ago

tve commented 1 year ago

Build environment: Arch Linux Target device: ESP32

Description When using an RMT device in rx mode there is no provision for errors. The HW is pretty cryptic but the ESP-IDF does expose an rmt_get_status call (which is not in the esp32 tech ref man...) and the rmt_reg.h header file shows that there are bits for "full" and "owner error", plus a couple more that are probably not relevant (yet).

The problem I'm having is that the RMT memory is overflowing due to what looks like too long a packet to it. The result is that when I read I get 0 samples. There's no way for my code to detect that it actually got too many samples.

In my use-case this is a problem because I need to tune the sensitivity of the radio feeding the signal in response to the number of pulses detected. E.g., if there are "too many" pulses it's too sensitive and picks up too much noise and I need to decrease the sensitivity. The reverse for "too few" pulses. The problem then is that 0 pulses could mean way too sensitive or way too insensitive.

Steps to Reproduce This problem is not entirely easy to reproduce. The best I know is to generate a PWM burst on one pin and feed that into an RMT pin. Generate a burst of <64 PWM transitions and you get the expected result reading. Generate >64 PWM transitions and read returns 0.

Expected behavior I expect some error indication.

Part of me posting here as opposed to just adding the necessary code is that I'm wondering what the right way to add error indications is. In an ideal world:

Given the current state of the interface, I see a couple of options:

NB: I'm not 100% sure of this, but I believe the ringbufferSize is a bit of a mis-feature. Given that the RMT driver hard-codes config.mem_block_num = 1; it is impossible to receive any transmission with more than 64 pulses (128 transitions). Each call to read pulls off one entry from the ring buffer, which is one "packet", which is limited to those 64 pulses. I don't see a way to fill the default ringbuffer size of 1024. (Being able to increase the mem_block_num would be highly desirable.)

Update: it looks like the problem is more complicated than I thought. "Packets" longer than 64 pulses get silently truncated to 64 and I'm not seeing that the truncation is in any way signaled by ESP-IDF. The problem I'm really hitting is "infinite packets", i.e., never hitting the idle threshold and therefore never getting any packet.

I added a rmt_get_status call to read and throw an error if the mem_full bit is set. That does not get triggered by packets longer than 64 pulses (presumably the status gets cleared rapidly by esp-idf reading the packet into the ring buffer). However, it gets triggered by infinitely long packets (e.g. just put a PWM signal into the RMT pin). So it helps detect infinite streams of pulses but not overruns.

Other users have hit this problem (in ESP-IDF) trying to receive servo PWM pulses or measuring PWM duty cycle, e.g. https://esp32.com/viewtopic.php?f=13&t=5122&start=10

Trying to step back...

Since I mentioned that I added the status check, here's my diff:

--- a/modules/pins/rmt/esp32/rmt.c
+++ b/modules/pins/rmt/esp32/rmt.c
@@ -190,6 +190,12 @@ void xs_rmt_read(xsMachine *the){
        int bufferByteLength = xsmcGetArrayBufferLength(xsArg(0));
        data = (uint8_t*)xsmcToArrayBuffer(xsArg(0));

+       uint32_t status;
+       rmt_get_status(rmt->channel, &status);
+       if (status & (1<<28)) {
+               xsUnknownError("rx overrun");
+       }
+
        err = receive(rmt, data, bufferByteLength, &count, &phase);
        if (err == -1) xsRangeError("ringbuffer has too much data");
        xsResult = xsmcNewObject();
phoddie commented 1 year ago

The Espressif RMT I/O unit is a strange beast. It does a lot of useful stuff, but it never quite feels obvious to work with.

This module is pre-dates ECMA-419 and so doesn't follow that style. There's a lot that would be different if it did. For any large changes, reworking the implementation for that world would be my preference.

For a quick fix for your immediate problems, reporting the overrun as you propose seems OK (as much as I dislike throwing from read unless the caller made a mistake). An alternative could be to add a status or error property to the returned object. That wouldn't change the behavior for any existing use and would quietly propagate the information you want.

All that said, @acarle knows more about this code than I do. I defer to him.