Skip to content

esp32: repeated RMT pulses when using loop(True) #6167

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

Closed
jonathanhogg opened this issue Jun 18, 2020 · 18 comments
Closed

esp32: repeated RMT pulses when using loop(True) #6167

jonathanhogg opened this issue Jun 18, 2020 · 18 comments

Comments

@jonathanhogg
Copy link
Contributor

As per original issue mattytrentini#11, in which this bug was more fully discussed, using loop(True) with the esp32 RMT object will cause repeated initial pulses.

Simple example:

from esp32 import RMT
from machine import Pin

pin = Pin(12, Pin.OUT)
rmt = RMT(0, pin=pin, clock_div=8)

rmt.loop(True)
rmt.write_pulses([2500, 7500, 5000, 5000, 7500, 2500])

With loop(False) this will produce the output show in Figure 1 below, but with loop(True) it will produce the – erroneous – output shown in Figure 2. The first of the three pulses is repeated.

72544860-04757f80-3880-11ea-9896-5416f9220e89

This, it turns out, is a bug in the ESP32 IDF (see espressif/esp-idf#4664) which has a fix committed for it. In the meantime, the accepted workaround is to call rmt_driver_install before rmt_config when initialising the RMT peripheral. This should still be fine after the fix makes a mainline IDF release.

jonathanhogg added a commit to jonathanhogg/micropython that referenced this issue Jun 18, 2020
@jonathanhogg
Copy link
Contributor Author

jonathanhogg commented Jun 18, 2020

I have forked micropython/micropython to jonathanhogg/micropython and committed this change to the rmt_fix branch (commit 05a53e5).

With the same above code, I now get the following – correct – output:

Screenshot 2020-06-18 at 12 43 32

@jonathanhogg
Copy link
Contributor Author

Note that I suspect that #5787 is also caused by this bug

dpgeorge pushed a commit that referenced this issue Jun 19, 2020
Otherwise the RMT will repeat pulses when using loop(True).  This repeating
is due to a bug in the IDF which will be fixed in an upcoming release, but
for now the accepted workaround is to swap these calls, which should still
work in the fixed version of the IDF.

Fixes issue #6167.
@dpgeorge
Copy link
Member

Fixed by 3a9d948

mattytrentini pushed a commit to mattytrentini/micropython that referenced this issue Jun 20, 2020
@jonathanhogg
Copy link
Contributor Author

jonathanhogg commented Jun 26, 2020

Hmmm… bad news. It doesn't look like this has properly fixed the problem. The write_pulses() function is hanging on a second call – with or without looping being enabled. Going to do some more digging and report back here in a bit.

[Props to @mogplus88 for alerting me to this over in mattytrentini#11]

@jonathanhogg
Copy link
Contributor Author

Small failing example:

from esp32 import RMT
from machine import Pin

pin = Pin(12, Pin.OUT)
rmt = RMT(0, pin=pin, clock_div=8)

rmt.write_pulses([2500, 7500, 5000, 5000, 7500, 2500])
rmt.write_pulses([2500, 7500, 5000, 5000, 7500, 2500])

First set of three pulses appear on pin 12, the second call starts but there are no further pulses and the interpreter locks up until a hard reset.

I swapped the rmt_driver_install() to be after rmt_config() again and re-tested and the previous behaviour is restored, i.e., write_pulses() and loop() work fine except for the repeated initial pulse when looping. So this is clearly a regression introduced by this "fix."

So far, this is all as tested on IDF v4.0.1.

[@dpgeorge can you re-open this issue for me?]

@jonathanhogg
Copy link
Contributor Author

So I've gone back to my previous fix for this problem – turning off the TX interrupt when looping is enabled – and everything operates as I would expect it to: repeated calls to write_pulses() work; wait_done() seems to work; loop(True) results in correct output (no extra pulses. After loop(False), output stops and then new pulses can be written with write_pulses() and then looping can be restarted.

If write_pulses() is called and then loop(True), then wait_done() will always return True. This would be consistent I assume with the initial set of pulses being written and the TX interrupt triggering done, then hardware looping being enabled with no reset of the TX semaphore.

Using loop(True) before write_pulses() appears to be The Wrong Thing to do: first attempt seems to work OK, but second attempt locks the interpreter without changing the output. I'm unconvinced that it is safe to call write_pulses() while looping is enabled. Note that reenabling the TX interrupt does not reset the TX semaphore, so loop(False) followed by wait_done() will not work in the way you would hope – i.e., blocking / returning False until the last set of repeating pulses has been transmitted.

So it looks like:

  • we have misunderstood what Espressif's apparent fix to ESP32 RMT TX repeating first few items on loop (IDFGH-2579) espressif/esp-idf#4664 actually fixed and what we are supposed to do with it;
  • disabling the TX interrupt works for making looping functional, but may just be a workaround for whatever we really are meant to be doing – it is, however, the way they demonstrated to do looping in that issue;
  • it might be worth looking at playing some more with the TX semaphore in loop() to see if sensible behaviour can be produced for wait_done() with looping;
  • having write_pulses() raise an exception if looping is enabled might be wise regardless.

@mogplus88: I am going to push my changes to https://github.com/jonathanhogg/micropython/tree/rmt_fix if you are able to rebuild MicroPython and try them. Or let me know if you want me to send some pre-built firmware to you.

@jonathanhogg
Copy link
Contributor Author

OK, with some more digging I have found that calling rmt_driver_install() before rmt_config() results in the TX interrupt either not being enabled in rmt_driver_install() or being disabled by rmt_config() – I suspect the former as I can't see how it could be the latter (unless it's a hardware thing). Manually re-enabling the interrupt (with a call to rmt_set_tx_intr_en()) after calling rmt_config() causes normal rmt_write_items() behaviour to resume, but looping goes back to having the repeated initial pulses.

Whichever way you dice it, the real problem here appears to be with the TX interrupt. The hanging of rmt_write_items() is because it decrements the TX semaphore on entry on the assumption that the interrupt handler will increment it when TX is complete. The semaphore starts at 1, so the first call goes through fine, but then a subsequent call will block indefinitely. This is the same reason that while turning off the TX interrupt fixes looping behaviour, it then makes it unsafe to call rmt_write_items() – again, first call will be OK and next will block.

In terms of getting something approaching sane behaviour for wait_done() together with looping, the closest I can get is this (using the original rmt_config() before rmt_driver_install() ordering and controlling the TX interrupt in loop()):

>>> from esp32 import RMT
>>> from machine import Pin
>>> 
>>> pin = Pin(12, Pin.OUT)
>>> rmt = RMT(0, pin=pin, clock_div=128)
>>> 
>>> rmt.write_pulses([0,0])
>>> rmt.loop(True)
>>> rmt.write_pulses([2500, 7500, 5000, 5000, 7500, 2500])
>>> rmt.wait_done()
False
>>> rmt.loop(False)
>>> rmt.wait_done()
True

Calling loop(True) before doing any write_pulses() results in a few spasms of the output pin and then nothing, which is why I'm doing the [0, 0] pulse trick here. In general, I suspect it is an unsafe idea to call loop(True) before doing write_pulses() as it will immediately begin looping whatever was last in the buffer. Calling it after, however, means wait_done() doesn't work. This is, of course, all what you'd expect looking at the way the semaphore is incremented and decremented.

Really, I think that enabling looping should be accompanied by decrementing the semaphore and then turning off the interrupt. Decrementing the semaphore would force a wait until the current pulse train is complete – if you've just called write_pulses() – and then tell the hardware to start looping the last train. It would also mean that wait_done() would then work. Unfortunately, the semaphore is not exposed by the IDF and there's no call that would accomplish this.

In the absence of that, it may be that a loop(True) / loop(False) design is fundamentally flawed and that a better approach would be something like write_pulses(…, loop=True) / stop_looping(). That would allow you to correctly sequence the looping activation within write_pulses() so that the semaphore behaviour is controlled. However, making a call on an API change like that is above my pay grade 😉.

Handing further consideration over to @dpgeorge and @mattytrentini

@jonathanhogg
Copy link
Contributor Author

Had a brainwave while doing the washing-up and realised that the existing API can be retained by simply disconnecting the action of calling loop() from the underlying enabling or otherwise of the hardware looping. By setting a flag in the RMT object instead, one can then handle the looping behaviour at the right places in a sensible way. To wit:

  • calling loop(True) sets an internal flag to enable looping on the next write;
  • calling write_pulses() turns off looping if it is active and re-enables the TX interrupt, starts the next train (which can then safely block waiting for the last train to complete), then immediately enables hardware looping if the flag is set and turns off the TX interrupt;
  • calling loop(False) resets the flag, turns off looping if it is active and re-enables the TX interrupt.

This means that calling write_pulses() multiple times with looping enabled will do The Right Thing instead of hard blocking the interpreter (i..e, wait for current train of pulses to complete before starting a new looping train) and wait_done() returns a sensible result at all times.

>>> from esp32 import RMT
>>> from machine import Pin
>>> 
>>> pin = Pin(12, Pin.OUT)
>>> rmt = RMT(0, pin=pin, clock_div=16)
>>> 
>>> rmt.wait_done()
True
>>> rmt.loop(True)
>>> rmt.write_pulses([2500, 7500, 5000, 5000, 7500, 2500])
>>> rmt.wait_done()
False
>>> rmt.write_pulses([5000, 5000])
>>> rmt.wait_done()
False
>>> rmt.write_pulses([10000, 10000])
>>> rmt.wait_done()
False
>>> rmt.loop(False)
>>> rmt.wait_done()
True
>>> rmt.write_pulses([10000, 10000])
>>> rmt.write_pulses([5000, 5000])
>>> 

I've confirmed that all of the above works against esp-idf v3.3.2 as well (and verified actual results with an oscilloscope).

Code changes in my rmt_fix branch.

@mogplus88
Copy link

@jonathanhogg thanks for the offer but I have never tried Micropython. All my testing has been done in C. Maybe I should give it a shot. I've got a Pycom WiPy...

;-) Ian

@jonathanhogg
Copy link
Contributor Author

@mogplus88 yes, sorry, I realised after making the offer that your messages only referred to the C API! Still, my results above should apply the same to your code and I'd be interested to hear your results.

Key things I am doing first time around:

  • call rmt_config() before rmt_driver_install();
  • call rmt_set_tx_intr_en(…, false) to turn off the TX interrupt;
  • call rmt_write_items() to start the train;
  • call rmt_set_tx_loop_mode(…, true) to enable hardware looping.

Then for each time I want to update the pulses:

  • call rmt_set_tx_intr_en(…, true) to turn on the TX interrupt;
  • call rmt_set_tx_loop_mode(…, false) to disable hardware looping;
  • call rmt_wait_tx_done(…, portMAX_DELAY) to wait for the current train to complete;
  • call rmt_set_tx_intr_en(…, false) to disable the TX interrupt again;
  • call rmt_write_items() to start the next train;
  • call rmt_set_tx_loop_mode(…, true) to enable hardware looping.

This last soft-shoe-shuffle is to ensure there's no glitches in the output around switching trains.

Calling rmt_driver_install() before rmt_config() appears to implicitly result in the TX interrupt being disabled, but this is behaviour I am not sure is intended or desirable. I'm still a bit confused by it and what Espressif's fix was actually fixing.

@mogplus88
Copy link

@jonathanhogg thanks for spelling it out for me. You may have guessed I am not a techhead! But I'm okay at following instructions.
I eventually got it to work as expected, with a couple of (weird) mods. The spreadsheet shows the combinations I tested. Note that the "wait" parameter on the write command seems to be critical. If it's set to true (wait for function to return) it never does.
ESP32testing.xlsx

FWIW here is my output
image

Cheers,
Ian

PS dunno if it helps, but I have implemented similar functionality with an Atmel processor using PWM. I got it to loop around an array and load the next pulse width from the next member of the array (done in an ISR). It was then possible to update the array at any time as the pwm compare registers are "double buffered", i.e. not updated until the pwm pulse has been generated, then the new value is loaded into the compare register. Bewdiful! (works on both 8-bit and 32-bit Atsam processors). So that's what I am hoping the ESP32 will do too only better. Output a full pulse train, then update the values from an array somewhere (which has been modified by the program), then output that train, and so ad nauseum. It seems to me this is the way it should work.

@mogplus88
Copy link

Actually, having studied your code again, I think that's exactly what it is doing. Yippee!

@jonathanhogg
Copy link
Contributor Author

@mogplus88 if you don’t actually need hardware looping then all you should need to do is rmt_config/rmt_driver_install and then repeated calls to rmt_write_items. With the wait flag off the call will return immediately allowing you to do other work, but if the next time you call it the previous sequence is still being output then it will correctly block waiting on that before queuing the next.

If you do need hardware looping then it appears you have to disable the TX interrupt to get correct output. In that case, I’d stick with the ordering in my comment above to avoid blocking.

@dpgeorge dpgeorge reopened this Jul 20, 2020
dpgeorge pushed a commit that referenced this issue Jul 20, 2020
A previous commit 3a9d948 can cause
lock-ups of the RMT driver, so this commit reverses that, adds a loop_en
flag, and explicitly controls the TX interrupt in write_pulses().  This
provides correct looping, non-blocking writes and sensible behaviour for
wait_done().

See also #6167.
@dpgeorge
Copy link
Member

Should now be fixed properly by 7dbef53

amirgon pushed a commit to lvgl/lv_micropython that referenced this issue Sep 4, 2020
A previous commit 3a9d948 can cause
lock-ups of the RMT driver, so this commit reverses that, adds a loop_en
flag, and explicitly controls the TX interrupt in write_pulses().  This
provides correct looping, non-blocking writes and sensible behaviour for
wait_done().

See also micropython#6167.
@bulletmark
Copy link
Contributor

I know it is over 3 years ago but I'm running stock ESP32_GENERIC-20231005-v1.21.0.bin and getting the same problem as described here. In fact, running the trivial example code stated in the first post here gives the same error, i.e. there is a first erroneous pulse. Should I raise a new bug or should we reopen this one?

@jonathanhogg
Copy link
Contributor Author

Curious. I've not looked at the RMT code (or used this feature) for an age. Looking at a blame of the source file, I would guess that it is this commit that has changed things: 7ea06a3

I spent a significant amounf of time staring at the code to come up with the ordering I settled on. Unfortunately, I don't have enough time at the moment to figure out what the impact of swapping these operations around is.

I suspect that you'll get further opening a new bug saying that there appears to be a regression and link to this one.

@mogplus88
Copy link

Ditto. Haven't played with this since the original conversation years ago. I'm using the Raspberry Pi Pico for my application now, (generating a PPM stream) using its PIO feature. Happy to share code if you are interested. There are also many websites (related to diy radio control transmitters for model aircraft) that use an Arduino Nano or similar to do the same thing with a few lines of code. I'm a bit over the esp32 I have to say. I'm finding the Pico a much friendlier board.
;-)

@bulletmark
Copy link
Contributor

Thanks for replying you two.

@jonathanhogg FYI, the bug is due to commit 7ea06a3 that you reference. Using latest MP master, if I put that single line back (i.e. above the if) then my ESP32 works but my ESP32S3 breaks, and vice-versa when I move it back. Putting it in both places lets ESP32 work, but not ESP32S3. I may submit a PR with a conditional compile around the two locations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants