Skip to content
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

Add RMT support to ESP32 #253

Merged
merged 2 commits into from Nov 5, 2020
Merged

Add RMT support to ESP32 #253

merged 2 commits into from Nov 5, 2020

Conversation

hierophect
Copy link
Contributor

Replaces the ESP32's espShow function with one that uses the RMT (Remote Control) peripheral to send timed pulses on the Neopixel line. Using the RMT should result in less flaky pixel performance when used with long Neopixel strings or in applications with heavy use of interrupts. Tested on a HUZZAH32 Featherwing.

This PR should resolve #159 and #139. However, I don't have a long enough string of Neopixels to reliably reproduce those errors on the original code, so any extra testing would be greatly appreciated to confirm this fixes all instability. This code should be easily portable to the ESP32-S2 and S3 when support is officially added to the Arduino core, since it is based off the existing implementation for the S2 in Circuitpython.

Note that the ESP8266 still uses the old timing method, as it does not have the RMT peripheral.

@ladyada
Copy link
Member

ladyada commented Oct 30, 2020

neopixels look great!
however, if you have two neopixel objects (say, one internal pixel and some external strip) they all update the exact same, rather than being separate. so its some RMT allocation thingy. perhaps increment thru so folks have up to 10?

@IAmOrion
Copy link

IAmOrion commented Oct 31, 2020

So do we just replace the current library with this version? As great as NeoPixelBus is, I found it much easier making animation classes (Extending on neopatterns) using the Adafruit Library so would be great to get this working without issues on ESP32's. I will be controlling 2 independent strips (32 each strip) but am I understanding correctly from Ladyada's comment that this currently only supports 1 singular strip/object? MTIA

Edit: After some testing, it would appear what Ladyada said is correct. Only 1 strip is active and effectively it controls BOTH of my strips. So in my case, where I have LEFT strip and RIGHT strip.... Left strip animation happen on both strips, anything "right strip" does nothing

@ladyada
Copy link
Member

ladyada commented Oct 31, 2020

yah if you can verify that your strip works (even if only one) we will fix it up to add multiple strip support when @hierophect is around next :)

@IAmOrion
Copy link

IAmOrion commented Oct 31, 2020

Well, it certainly worked for me with a single strip yes :) I was able to use multiple NeoPatterns etc and control it from web page too. I also re-uploaded sketch using Adafruit NeoPixel library to verify that the RMT version was acting different and it most certainly was. The Non-RMT version had many artefacts or erroneous pixel behaviour it was annoying. The RMT version worked great, albeit only for 1 strip

@ladyada
Copy link
Member

ladyada commented Oct 31, 2020

awesome, thak you for testing.

@IAmOrion
Copy link

No problem. I've been eagerly awaiting a fix as I prefer the Adafruit NeoPixel library over the NeoPixelBus library. Mostly because it's what I'm used to , but I also prefer NOT having the animation api thast neopixelbus has, I prefer having the NeoPattern classes method etc so I look forward to this being accepted and an update soon :)

@MustBeArt
Copy link

I'm excited that @hierophect has taken on this long-standing need. Thanks!

I'm also doing some testing, and I agree with most of what you've all found. It does solve the interrupt latency problem from my #139, and it seems to work fine with one strip, and it does not work correctly for multiple strips.

I get a slightly different result than @IAmOrion in the case of two strips. I find that activity intended for either strip ends up on both, for a test case where they are not trying to be active simultaneously.

The dynamic allocation of RMT channels in this code isn't actually doing anything. That's because it always blocks and waits for the transfer to complete, and then releases the channel allocation. So the next call can and does allocate that same channel again. (The channel it always allocates is 7, rather than the expected 0, because there's a break missing from the loop that looks for a free channel.)

That would be non-fatal if the new allocation and configuration completely overrode the old one, but evidently that's not the case. If we want to re-use an RMT channel, we need to figure out how to get it disconnected from the old pin. I have not gotten to the bottom of that rabbit hole, yet.

Blocking and waiting for transfer to complete is probably fine in a lot of cases, and is consistent with a lot of other Arduino libraries, but it breaks the promise made in a comment in Adafruit_NeoPixel::show() that "multiple instances on different pins can be quickly issued in succession (each instance doesn't delay the next)" and hurts performance for multi-strip applications. I think we eventually need to avoid blocking. That's probably going to involve abandoning the default ISR and writing our own to handle the transmit done interrupt.

We could allocate RMT channels more statically, so that the number of available RMT channels limits the number of instantiated strips. That would avoid the need to re-use an RMT channel. Or, assuming we can figure out how to re-use an RMT channel, we can be more clever and allow unlimited strips (with blocking when all the RMT channels are busy). Neither option is trivial.

@MustBeArt
Copy link

Turns out that, if my examination of the ESP IDF source code is correct, there is no way to share a RMT channel entirely through the RMT driver, because it has no way to remove the connection between the RMT channel and the pin.

You can, however, do it by going directly to the GPIO API, using the undocumented (?) function gpio_matrix_out() (which is in ROM!) and its stealthy feature mentioned in gpio.h: if you pass in the magic value 0x100 for the signal index, it cancels output to the pin. This is what FastLED does in their RMT implementation.

So, a minimal repair for the problem with multiple strips is to add this line of code after the call to rmt_driver_uninstall:

gpio_matrix_out(pin, 0x100, 0, 0);

That doesn't achieve the performance possible by allowing multiple RMTs to run in parallel, but it does restore logical correctness for multiple strips, and it doesn't restrict the number of strips that can be instantiated. It works on my testbed, though I wouldn't call my testing thorough.

@hierophect
Copy link
Contributor Author

hierophect commented Nov 2, 2020

Was off for the weekend, I'll work on the dual strip issue today. Thank you for the suggestions - some of these will also apply to the implementation in Circuitpython that we didn't find in testing.

@ladyada
Copy link
Member

ladyada commented Nov 2, 2020

welcome back & thanks :) if possible please support n=8 or more strips (if we can!) note that the RMT peripheral isnt really used for anything else so you can use it freely and we'll warn people that they can't also have other code with RMT

@IAmOrion
Copy link

IAmOrion commented Nov 2, 2020

@hierophect Awesome, thank you so much for your work on this - I've been eagerly awaiting a fix for the Adafruit Official NeoPixel Library + ESP32 support using RMT. Thanks again

@hierophect
Copy link
Contributor Author

Pushed a change that works for the ESP32S2 on Circuitpython, will test on ESP32 tomorrow once I get more Neopixel lines in.

Since it is undocumented, I'm not sure why FastLED goes for gpio_matrix_out(pin, 0x100, 0, 0);, it may have simply been the best option in the API at the time they designed their implementation. I've found gpio_set_direction and gpio_reset_pin work just as well, though I'm waffling on which is more appropriate for this particular case.

I understand the concern about performance. Circuitpython, which this implementation is based off of, tries to treat each Neopixel transaction as a discrete event that inits and de-inits itself in a single transaction. Since the expectation for Arduino is that it'll be a bit faster, I'll think about how to speed things up a bit in the absence of the pin and peripheral reservation system CPy uses.

@IAmOrion
Copy link

IAmOrion commented Nov 3, 2020

This still seems buggy for me with 2 strips. Some neopatterns don't function on 2nd strip at all, and passing a speed value does seem to make much difference. Eg, I have a knight rider animation, but the speed difference between 200ms per led, and 15000 per led is minimal. It's barely noticeable - but as you can imagine, I would've expected it to be going either really slow or super fast at those 2 different speeds

UPDATE/EDIT: - I'm an idiot, this works perfectly fine, it was my own buggy code

@hierophect
Copy link
Contributor Author

@IAmOrion thanks for testing! As I mentioned, I still don't have the hardware to test thoroughly on ESP32, so I'll take another look as soon as it comes in the mail today.

@IAmOrion
Copy link

IAmOrion commented Nov 3, 2020

Excuse my sloppy code, it's a WIP but here's my project if you want to see what I was doing code/strip wise
BMW_Z4_Wing_Mirrors_ESP32_AdafruitNeoPixel.zip

@MustBeArt
Copy link

@IAmOrion It looks like your speed parameters are squeezing through a uint8_t method parameter, so a value of 15000 isn't going to arrive intact. That probably explains your speed observations. Try values like 10 and 250 to see if you can see a big speed difference.

Can you construct a simplified example that just uses two Neopatterns on two strips and demonstrates the failure of some patterns to work on a second strip?

I was able to modify a version of the AllPatternsOnMultiDevices example sketch that ships with NeoPatterns and run it on two strips on ESP32, and everything appeared to work reasonably. Which patterns are failing for you?

@IAmOrion
Copy link

IAmOrion commented Nov 3, 2020

@MustBeArt

When I talk of NeoPatterns, I refer to my modified version of the NeoPatterns class file found in Adafruits "Multi Tasking Part 3" guide found here
https://learn.adafruit.com/multi-tasking-the-arduino-part-3/put-it-all-together-dot-dot-dot
The knight rider speed issue will be my own stupidity there so will check that shortly. -- update, fixed my speed, I was having a "senior moment" ha. In terms of patterns not working, Using my sketch, if I open Serial monitor and type 99 and press enter (NO LINE ENDING) I get into a test mode.

When first booting, BOTH strips work using "Scanner" effect. Once in the main loop, the following occurs using test mode

1 turns all lights white and works on both,
2 is a "LEFT" turn signal and works and Strip 1 does as expected,
3 is a "RIGHT" turn signal and DOESN'T work - Strip 2 does nothing,
4 is "Hazards" so left + right at same time and DOESN'T work - Strip 1 works as expected, Strip 2 does not
5 is Scanner again - Strip 1 works, Strip 2 flashes first pixel but then does nothing and remains off
6 is a Rainbow pattern - this appears to work perfectly, BOTH strips appear to function as expected
7 is a police flash effect alternating red/blue/off 50/50 pattern. Strip 1 works as expected. Strip 2 does not work correctly. Whilst it does alternate the red blue flashes, the blue part never goes off like it should.

It's worth noting that this NeoPattern class is used in a few of my projects. If using an Arduino Nano, EVERY test mode works exactly as expected and intended. The problems ONLY occur with ESP32

I do have the NeoPatterns library that's not "Adafruit Official" as far as I know, so will give the examples a try and see how I get on with them.

I get this error trying the example AllPatternsOnMultiDevices

Screenshot 2020-11-03 at 19 06 25

As for my own code, I'm an idiot and used a neopatterns class I'd made for something else that multiple animations on 1 strip, starting at a center point... I just realised and face palmed. So actually, this code DOES work perfectly for me

@hierophect
Copy link
Contributor Author

I've tested this on an ESP32 feather with multiple strips of Neopixels, and things seem to be functioning as expected. Since @IAmOrion has confirmed they're no longer seeing issues, should be viable to merge, if the slight performance loss is deemed acceptable for now. I can investigate some performance fixes and follow up with another PR.

@ladyada, want to take another look?

@ModsNSods
Copy link

ModsNSods commented Nov 5, 2020

The only animation I'm having trouble with in my code (See previous post for attachment) is that the POLICE pattern I made works fine on one strip but not on the other - the blue lights stay on permanently instead of flashing. The animation is pretty fast and if I run the animation on 1 at a time, it works perfectly so I'm guessing that's potentially a good example of the slight performance issue.

That being said I plan to amend it anyways so will figure a workaround (or just re-code)

EDIT: Was signed into a new business account - I am IAmOrion but cba to logout/back in/copy paste etc

@ladyada
Copy link
Member

ladyada commented Nov 5, 2020

tested, this is good to go for now! can always improve later :)

@ladyada ladyada merged commit b3d6477 into adafruit:master Nov 5, 2020
@hierophect hierophect deleted the rmt-support branch November 5, 2020 19:47
@jpasqua
Copy link

jpasqua commented Nov 28, 2020

FYI: I have some WS2812D's that still have timing issues with this code (many other strips work fine). While testing and inspecting the code and comparing to the FastLED library, the ESP32-Digital-RGB-LED-Drivers, and the ESP-IDF code, it seems that this library does not send the final reset signal (low time). Perhaps I'm missing it. In the other libraries this is achieved by pronging the duration1 value of the final rmt pulse pair to include the reset time (see examples below).

From ESP32-Digital-RGB-LED-Drivers:

    // Handle the reset bit by stretching duration1 for the final bit in the stream
    if (i + pState->buf_pos == pState->buf_len - 1) {
      RMTMEM.chan[pStrand->rmtChannel].data32[i * 8 + offset + 7].duration1 =
        ledParams.TRS / (RMT_DURATION_NS * DIVIDER);
    }

From FastLED:

        mBuffer[mCurPulse-1].duration1 = RMT_RESET_DURATION;

In both cases this is done in the ws2812_rmt_adapter() function (or equivalent). I noticed that the led_strip_rmt_ws2812.c code (which you mentioned that you adapted) is also missing the final reset, though it does have a constant defined for the reset time in microseconds: WS2812_RESET_US.

@ladyada
Copy link
Member

ladyada commented Nov 30, 2020

@jpasqua hmm! could you add a pull request to add it? we don't have any pixels that dont work :)

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.

Wrong color on first pixel with ESP32
6 participants