Fastled: ESP32 RMT not working with WS2811 on 3.2.10

Created on 8 Jul 2019  Â·  94Comments  Â·  Source: FastLED/FastLED

Updated an older version of fastled - (maybe 3.2.x) and the LEDs stopped working. Only tried on my WS2811 strips. Stepped the library back to 3.2.8 and it worked again. I'm guessing something in PR #789 @samguyer? I checked with the oscilloscope and didn't see anything on the pins.

Switched over to use the I2S version and that works perfect with the same setup. Thanks for that! I was going to move forward with that anyways since I might as well use more the 8 parallel strings!

Most helpful comment

@samguyer got a pull request?

All 94 comments

I've also found that the RMT driver isn't working for 8 parallel strings in the latest version.

It works for 1-7 strips, but at 8 there's no output on any of the strips.

Here's the code I'm using to reproduce it, this doesn't output anything at all as is. However, if you comment out any one of the strips it will work again.

#include <FastLED.h>

#define NUM_LEDS_PER_STRIP 20
#define NUM_STRIPS 8

CRGB leds[NUM_STRIPS * NUM_LEDS_PER_STRIP];

void setup() {
  FastLED.addLeds<WS2812B, 27, GRB>(leds, 0 * NUM_LEDS_PER_STRIP, NUM_LEDS_PER_STRIP).setCorrection( TypicalSMD5050 );
  FastLED.addLeds<WS2812B, 14, GRB>(leds, 1 * NUM_LEDS_PER_STRIP, NUM_LEDS_PER_STRIP).setCorrection( TypicalSMD5050 );
  FastLED.addLeds<WS2812B, 12, GRB>(leds, 2 * NUM_LEDS_PER_STRIP, NUM_LEDS_PER_STRIP).setCorrection( TypicalSMD5050 );
  FastLED.addLeds<WS2812B, 13, GRB>(leds, 3 * NUM_LEDS_PER_STRIP, NUM_LEDS_PER_STRIP).setCorrection( TypicalSMD5050 );
  FastLED.addLeds<WS2812B, 26, GRB>(leds, 4 * NUM_LEDS_PER_STRIP, NUM_LEDS_PER_STRIP).setCorrection( TypicalSMD5050 );
  FastLED.addLeds<WS2812B, 25, GRB>(leds, 5 * NUM_LEDS_PER_STRIP, NUM_LEDS_PER_STRIP).setCorrection( TypicalSMD5050 );
  FastLED.addLeds<WS2812B, 33, GRB>(leds, 6 * NUM_LEDS_PER_STRIP, NUM_LEDS_PER_STRIP).setCorrection( TypicalSMD5050 );
  FastLED.addLeds<WS2812B, 32, GRB>(leds, 7 * NUM_LEDS_PER_STRIP, NUM_LEDS_PER_STRIP).setCorrection( TypicalSMD5050 );
  FastLED.setBrightness(32);

}

void loop() {
  static uint8_t hue = 0;
  for (int i = 0; i < NUM_STRIPS; i++) {
    for (int j = 0; j < NUM_LEDS_PER_STRIP; j++) {
      leds[(i * NUM_LEDS_PER_STRIP) + j] = CHSV((32 * i) + hue + j, 192, 255);
    }
  }

  hue++;

  LEDS.show();
  FastLED.delay(10);
}

Tested with FastLED 3.2.10 on esp32 doit devkit v1, with Arduino 1.8.9.

Hmmm. Sounds like I need to do some debugging!

Yes I was doing 8 strips as well. Never tried with less though.

I am having the same issue, but even does not work with 6 pins. I was pulling 90% of my hair out after 4 hours of finding the mistake in my code or to find a workaround. How to switch to I2S version? Quite lame question: actually what is that?

Ug. Sorry all. I will get cracking on this problem as soon as possible. In the meantime you can use the I2S implementation. It will work in most circumstances. Check the notes in clockless_esp32.h for details.

Thanks a lot in advance.

Hi all, I am trying to debug this problem, but I need your help. @jrhun can you try something for me? Modify your code to use only 7 strips -- and confirm that it works. Then add the following #define before you include fastled.h:

define FASTLED_RMT_MAX_CHANNELS 7

Let me know if it still works, or if that breaks it.

Adding #define FASTLED_RMT_MAX_CHANNELS 7 works for both 7 strips and 8 strips.

Something I didn't realize before, is that the code I posted above will output data for strips 1-7 for the initial ~4-10 updates after each reset (I hadn't noticed this since I wasn't turning the LEDs off completely during tests). During these updates, strip 8 will output the wrong colors and sometimes just random data or nothing at all.

@focalintent
Dan, do you have a few minutes to double-check the fix that I pushed to you for the color order problem (making those key methods virtual)? Other than that change, 3.2.10 should be identical to 3.2.9

I tried with my example, 7 or 8 channels. (see bug #839) It just freezes like before. It shows 1 frame random something, or part of the correct image, that is all. Interestingly sometimes it keeps updating with random stuff, but the first 100~200 LEDs of the first channel only.

Any progress? Can I help with testing?

Some additional info (looking at this code here as well as the discussion on Reddit)

Looking at the delta, there are two interesting and potentially relevant diffs:


This one in platforms/esp/32/clockless_rmt_esp32.h

Where
void IRAM_ATTR fillHalfRMTBuffer()
becomes
virtual void IRAM_ATTR fillHalfRMTBuffer()

This has no effect on the problem nor anything else that I can see, but I'm very curious why this was changed?

Edit: I see this is from https://github.com/FastLED/FastLED/pull/831 and it appears to be unique to FastLED's templating system.


This one in chipsets.h

Where
#define C_NS(_NS) (_NS * ((CLOCKLESS_FREQUENCY / 1000000L) + 999) / 1000)
becomes
#define C_NS(_NS) (((_NS * ((CLOCKLESS_FREQUENCY / 1000000L)) + 999)) / 1000)

This is leading to the problem

For the ESP32, F_CPU is at best 240000000 (and CLOCKLESS_FREQUENCY is == F_CPU). This macro sets the timings for the clockless mode chipsets (e.g., C_NS(1010)).


Here's how the calculations differ:

  #define C_NS0(_NS)   (_NS * ((CLOCKLESS_FREQUENCY / 1000000L)  + 999)  / 1000)
  #define C_NS1(_NS) (((_NS * ((CLOCKLESS_FREQUENCY / 1000000L)) + 999)) / 1000)
....
  Serial.println(F_CPU);
  Serial.println(CLOCKLESS_FREQUENCY);
  Serial.println(C_NS0(10));
  Serial.println(C_NS0(100));
  Serial.println(C_NS0(1000));
  Serial.println(C_NS1(10));
  Serial.println(C_NS1(100));
  Serial.println(C_NS1(1000));

We get:

240000000
12
123
1239
3
24
240

This appears to be a fix for #813, but this is also causing the RMT to act up. Without it, apparently the timings are wrong for an ESP8266. With it, the timings may be correct (for the 8266) but the RMT fails on the ESP32.

Edit: to be more exact, I think the timings end up so low that the RMT device is running faster than its interrupt handler can keep up with when all 8 channels are in use. It seems to become a problem when there are >=32 LEDs on the strips (for the purpose of this, each strip having the same number). That means the buffer is filled fully on init, but the interrupt-driven half-copies are failing after that point. 512 bytes of buffer / 8 channels / 3 colors * 1.5 (one and a half fills) = 32.

Bottom line - I'd revert that change, ensure it's stable on the ESP32 again at 8 channels, and come up with a way to handle ESP8266 that doesn't affect the '32.

@focalintent - FYI

@MartyMacGyver
Martin, thanks for the analysis.

I have always been a little concerned about the timings that result from the values passed in from chipsets.h. I'll take a look at the timing calculation more closely.

I can also look at the efficiency of the fill buffer function -- maybe there is some performance to be had there. We might even be able to detect online when the interrupt handler is dropping parts of the data.

The reason for making that method virtual is to handle cases where different kinds of strips are attached to the same microcontroller. The virtual dispatch will call the method that has the right template parameters baked into it. Does that make sense?

Also: why is the ESP8266 support an issue here? The ESP32 code should affect it at all.

All I know is that #813 was specific to an 8266 problem, but that change is what breaks the ESP32.

The commit in question:

https://github.com/FastLED/FastLED/commit/ca9e40b4e9e640ae3f50d1b89b3f3eca36e2496d

813 affected every platform with a clock rate above 24mhz. It’s not getting reverted

Fair enough - I was offering a possible short-term solution to the bug that change is causing for ESP32, but if it's too tightly connected to all the other changes then another solution will be needed.

The fact that the bug that caused #813 was allowing the RMT stuff to work makes me think there's a timing issue in the RMT driver, where when the output of C_NS was too high was giving the RMT driver enough extra cycles to do the flipping work that it was doing, and the 7 vs 8 thing is probably the difference between having enough time vs not.

In fact, I bet that's what's happening. The interrupt handler is only writing 32 bits of data into the buffers at a time for each RMT channel - but they're writing out in parallel so that interrupt handler needs to be finished in the time it takes to output 32 bits of WS2812 data. So - 32 bits of WS2812 data is about 40µs of wall clock time. Which means once the RMT driver starts going there's a 40µs window to fill in the other half of the buffer - at 240Mhz that's just under 10,000 instruction cycles. With 8 channels you're prepping 256 bits worth of data which gives you, on average, 37.5 clock cycles per bit - or 1200 clock cycles per channel and 300 clock cycles per byte in said channel. getNextByte is probably not an inlined function so you're going to have register saving/loading plus regular function overhead that's going to eat into that 300 clock cycle per byte. This line:

                RMTMEM.chan[mRMT_channel].data32[mCurPulse].val = val;

alone is probably going to chew up 20 clock cycles alone. mRMT_channel and mCurPulse are member variables which means they are a read from an offset in memory - and will need to be either shifted (1 cycle) or multiplied (2 cycles) to generate the proper offsets into their various structures - then computing the address for the value to be stored at is RMTMEM + chanoffset + (mRMT_channel *something) + data32offset + (mCurPulse * something) + valoffset - that's ~12 cycles right there then there's the cycle time to load each value from memory (mRMT_channel, mCurPulse). That's not including the cycles spent with the loop, bitshifting, or updating the member variable for mCurPulse.

With 7 channels you're prepping 224 bits of data, so that's more like 43 cycles per bit and with 4 channels you get 80 cycles per bit.

(Of course, those values are all halved if you're running at 120Mhz)

Also, if the interrupt handler is interrupted by other handlers, that will chew into the 9600 cycles that it has to finish filling all the RMT buffers. I wonder if there's some interplay with the setting/clearing of the interrupt bits that could create a state where the interrupt handler doesn't get called for a particular channel anymore - but I'd have to dig more into how the RMT device and it's flagging and interrupts work first.

(as an aside, to explain more about why the fix for #813 is correct - the C_NS macro is used to convert nanoseconds into clock cycles. With a 240Mhz clock, 1000 nanoseconds is 1µs is 240 clocks - so the 1239 value that C_NS0 was giving for 1000 nanoseconds was way off)

I do think it's possible that the change to have fillHalfRMTBuffer be virtual added enough overhead (preventing inlining, or extra extra memory dereferencing, or something more) to chew away what time headroom the code used to have.

I think having uint8_t IRAM_ATTR getNextByte() be marked always inline might help.

Also replacing these two things:

                RMTMEM.chan[mRMT_channel].data32[mCurPulse].val = val;
                mCurPulse++;

with something that stashes mCurPulse into a local variable and uses that (then writes that value back into mCurPulse at the end of fillHalfRMTBuffer as well as something that grabs, say, the address of RMTMEM.chan[mRMT_channel].data32 and stashes that into a local variable so that your write line becomes something more like:

      data32[curPulse] = val;

and save a whole bunch of multiple sets of indirections and loads happening for every bit}

@samguyer Thanks for the explanation WRT the virtual keyword addition.

As for the timings, yes, they can be tricky with the RMT channels (depending on the divider chosen). It's my theory that it's the time needed for servicing the interrupts that's causing this, but I'm not positive.

@focalintent I just ran the code and probed it with the logic analyzer and yes, I see your point now (clearly if the timings were 80% lower than spec they'd just be totally broken - it wouldn't work at all for anything.)

My core driver doesn't have as much going on in terms of indirections, so that may be why it still works despite what are pretty close tolerances. Getting that out of your core driver as you are suggesting should improve things.

I did the same to my code (which is for the most part still very similar to the clockless code you have there). For black (using the LED_WS2812B_V3 .T0H = 450, .T1H = 850, .T0L = 850, .T1L = 450 ns timing specs), I'm seeing numbers almost spot on for these (~452ns and ~852ns respectively). The total is ~769KHz (as expected for the timings).

For the bug test (using the WS2812/B timings aka WS2812Controller800Khz) I'm getting 248ns and ~1002ns respectively. The timings work out to exactly 800Khz

For the latter, the calculation appears to be:

T1 = 2 * FMUL
T2 = 5 * FMUL
T3 = 3 * FMUL

(Edit: I do see the non-clockless version defines these more exactly as NS(250), NS(625), NS(375) respectively.... yet again these look odd to me versus specs for the WS2812B.)

Where
T0H = f(T1)
T1H = f(T1+T2)
T0L = f(T2+T3)
T1L = f(T3)

DIVIDER = 2 (I use 4, and I think that slightly improves accuracy at the shortest timings but I don't think that's an issue here.... but the fact that 4 was problematic in FastLED is intriguing.)

I tried 3.2.8 and can clearly see the bug #813 fixed. 3.2.10 and 3.2.6 show the timings above.

What's odd to me is why I'm seeing 250ns/1000ns at all. That's close to marginal for the oldest WS2812/B variant I've got (which are 350ns/700ns), and out of spec for the latest ones (which are 450ns/850ns). The allowance for error is about +/-150ns on these and in my experience it's better to err on the plus side with the timings. Based on the way you calculate things I'd expect them to be much closer to 300ns/700ns (even though I still think that 300ns is uncomfortably low).

While I'm being more pedantic in my driver about LED sub-variants, I'd expect the happy medium timings here to be a little higher on the low side and a little lower on the high side. Or just make WS2812B a proper variant (the WS2812 had somewhat imbalanced timings, while the WS2812B is at least symmetric - it'd be a lot easier to come up with a singe timing for its subvariants without the old WS2812 in that calculation).

(FWIW, the WS2813 was even worse in terms of the published timings changing over time).

Going back to @samguyer's point, the way the timings are being defined and ultimately calculated is fairly coarse, which is another source of possible problems. There's plenty of room to scale the calculation such that one could specify more than a range of [1..12] (at a glance, these are the min/max of the n*FMUL calculations). If n was, say, 400, n*(FMUL/100) would be equivalent to n*FMUL, but the precision would survive all the way to the final duration values (at least down to the limit of the RMT's precision) for values like 450 as well.

And DIVIDER comes back up now that I look at all this... if you improve the granularity of the base timings, you should be able to bump that divider up to 4.

One other benefit... with integer ns timings, you could greatly simplify chipsets.h by having the NS macro (or a variant of it) do the conversion work for you based on FASTLED_HAS_CLOCKLESS. One source of truth per chipset would remove a significant source of variability and maintenance.

The FMUL multiplier timings are only for 24Mhz or slower - and I did that because the clock cycles per bit were so low that I couldn't trust the math to get things lined up right on its own and those timings were being slotted into hand cycle counted asm code so I needed it to be better spaced - when you only have 10 cycles per bit the rounding errors in a basic NS to clock cycle transform can throw you way out of spec stupidly fast. In a perfect world everyone would just use the NS timings and I'd ditch the FMUL*x ones- but that's hardly the only thing in the library that ends up getting special cased for low clock-rate devices.

Each of the timings that I have are the timings that I had (either from data sheet or vendor - because at the point I was adding some of these there weren't even real reliable data sheets running around, yet) at the point that I added the chipset - but one of the things that I'm doing with the RGBW rewrite is also making it stupidly easy to redefine the timings on the fly (on some platforms, it'll be adjustable at runtime as well - which will be useful for tuning). When I rebuild the equivalent of chipsets.h for that new version I will probably do a search/digging around for the most recent data sheets for chipsets/variants.

The funny thing is - the earliest WS2812's that I worked with were really tight around that first 250ns mark - letting T0H drift up even to 300 or 325ns would result in a 0 being read as a 1, which is why I biased towards the midpoint of what I was given. I'm glad to hear that modern runs of the chipsets at least skew in more intelligent directions.

@focalintent @MartyMacGyver
This is a great discussion -- thanks for digging into this issue. I'm thinking that the first step is for me to optimize the fill buffer code to get rid of the interrupt problem. Then we can revisit the calculation of the timing numbers.

I have to say that it would be a lot simpler if the controller template parameters were in units of nanoseconds rather than clock cycles, but it is mainly an issue for the ESP32.

If I understand the clock divider correctly, smaller numbers make the timing more accurate in the general, but there could be point cases where a different divider (e.g., 3 vs 2) could work out to be more accurate just because of rounding.

@samguyer it's clocks because, at a low level - that's what pretty much every implementation wants (whether it's counting clock cycles for a delay in low level asm, or clock cycles for a pwm driver - hell even the RMT drivers, etc... etc...) and it seemed better to do that conversion in one spot vs. having to do it in every implementation (of which esp32 is just one)-- going from clocks to NS is pretty straightforward - but you don't even need to do that - you can just convert from system clocks directly to RMT clocks:

#define CPU_TO_RMT_DIVIDER (F_CPU / (F_CPU_RMT/DIVIDER))
#define TO_RMT_CYCLES(_CLKS) ((CLKS) / (CPU_TO_RMT_DIVIDER))

I suppose the one place it would make sense to convert back to NS is if you could run the esp32's system clock at a rate lower than 80Mhz but still drive the RMT at 40-80Mhz... (why someone driving leds would want to under clock the esp32 is a ¯_(ツ)_/¯ but folks do strange things sometimes)

The FMUL multiplier timings are only for 24Mhz or slower - and I did that because the clock cycles per bit were so low that I couldn't trust the math to get things lined up right on its own and those timings were being slotted into hand cycle counted asm code so I needed it to be better spaced - when you only have 10 cycles per bit the rounding errors in a basic NS to clock cycle transform can throw you way out of spec stupidly fast. In a perfect world everyone would just use the NS timings and I'd ditch the FMUL*x ones- but that's hardly the only thing in the library that ends up getting special cased for low clock-rate devices.

Except the code is still being called for the ESP32:

#if (CLOCKLESS_FREQUENCY == 8000000 || CLOCKLESS_FREQUENCY == 16000000 || CLOCKLESS_FREQUENCY == 24000000)
#define FMUL (CLOCKLESS_FREQUENCY/8000000)

Which means FMUL = [1..3] for the ESP32 (will the fractional results even work right here, given integer math? Edit: I see the ones that'd be fractional are commented out.) Not a lot of granularity there after that gets divided back out. Perhaps using the NS settings if an RMT is being used would be a good compromise? Between that and DIVIDER = 4, you get a much better granularity in the tens of ns.

As for low freqs <=24 MHz), one could start with nanoseconds and do a compile-time or init-time ceiling calculation, but that may be prohibitive currently. That might just be part of the upcoming ability to fine-tune the timings anyway.

No, it isn't:

#if (CLOCKLESS_FREQUENCY == 8000000 || CLOCKLESS_FREQUENCY == 16000000 || 
CLOCKLESS_FREQUENCY == 24000000)

That's 8,000,000 (8Mhz), 16,000,000 (16Mhz) and 24,000,000 (24Mhz) - the esp32 is 240Mhz.

And with the low frequencies I did start with nanoseconds, and found that it was just easier to hand tune to get things lined up (because I had to make decisions with each chipset at 8Mhz on which side of the exact value they wanted I was going to be on). At somepoint I might redo the math and mapping. Not every chipset's mapping and tolerances line up nicely to 125µs, for better or worse.

I don't want to have a 3rd set of chipset definitions that are unconverted nanoseconds, as it is having two sets has caused problems occasionally - because the esp32 clock is such high frequency if it is really easier to work in NS - then it's easy to jump the values back up.

Of course - at 240Mhz a T0H of 250ns becomes 60 clocks - and converting that to RMT with an rmt divider of 4 would be a divider of 12 - or 5 RMT clocks for T0H ( 240Mhz / (80Mhz / 4)) == 12 ... 60 / 12 == 5). But really, whether you get to that value by converting the T1/T2/T3 values back to nanoseconds and then re-divide down to the RMT clocks or by just scaling the T1/T2/T3 down to the RMT clocks based on the ratio between RMT clocks to F_CPU - it's all pretty straightforward math that's entirely done at compile time : )

I was thinking you'd just leverage the existing ns-level defs for the RMT case, and convert accordingly.

Edit: it'd mean refactoring the NS macro a bit, but that second set of definitions is already starting from nanosecond values prior to the macro.

I still think ideally you'd have one set of ns-level definitions and deal with the math from there (ceiling functions are handy for low-granularity situations involving ints). Most of this is already in place already.

Again - you keep looking at this from a single platform point of view - I'm supporting close to a dozen different platforms that clockless is on - and the chipset timing definitions are done in a centralized place across all the platforms.

The problem that I ran into with trying to use ceiling or floor functions when doing the timings/asm for 8mhz avr is that _sometimes_ I wanted the floor and _sometimes_ I wanted the ceiling - and deciding between the two ended up coming down to running with both a scope and actual leds hooked up to figure out which direction I wanted to err in (and then possibly adjust the 3rd value to keep the total time in the "x ns" per bit window for the given chipset).

If the template values are in nanoseconds - then I'm having to do the nano second to clock conversion code in every other implementation - and since in every implementation what I was interested in was _clocks_ and not nanoseconds - it didn't make sense to push nanoseconds down to everything at the time. I'm not sure it does now for just a single driver on a single platform - at least not with this codebase. (I'm already doing one rewrite of the library at the moment - I'm not going to do a major stop-gap refactor for this, especially since it doesn't give any benefit to any other implementation)

I missed a reply - and I missed the missing zeroes there - I thought that was specifying exceptions for 240/160/80 MHz. So basically ignore part of what I said.

But since all the timings for the ESP32 RMT are coming from the same coarse-grained clockless block (with the problems as noted above), what's FMUL defined as when FASTLED_HAS_CLOCKLESS is defined, as it is for RMT in clockless_rmt_esp32.h? I don't see that defined anywhere.

FMUL is only used in that chunk of code that's ifdef'd for 8/16/24Mhz devices - because for the low clocks I'm defining the timings in 125µs chunks (so FMUL would be 1 for an 8Mhz device, 2 for a 16Mhz device and 3 for a 24Mhz device).

Everyone else gets a straight conversion from nanoseconds to clocks and gets CPU clocks shoved into their template definitions.

I don't think that's the case, looking at the code and the #if/#ifdef and #endif pairs, FASTLED_HAS_CLOCKLESS is for that entire block.

I assumed that was by design, which is why I've been pointing all that out.

#ifdef FASTLED_HAS_CLOCKLESS
/// @name clockless controllers

#if !defined(CLOCKLESS_FREQUENCY)
    #define CLOCKLESS_FREQUENCY F_CPU
#endif

#if (CLOCKLESS_FREQUENCY == 8000000 || CLOCKLESS_FREQUENCY == 16000000 || CLOCKLESS_FREQUENCY == 24000000) //  || CLOCKLESS_FREQUENCY == 48000000 || CLOCKLESS_FREQUENCY == 96000000) // 125ns/clock
#define FMUL (CLOCKLESS_FREQUENCY/8000000)

// GE8822
template <uint8_t DATA_PIN, EOrder RGB_ORDER = RGB>
class GE8822Controller800Khz : public ClocklessController<DATA_PIN, 3 * FMUL, 5 * FMUL, 3 * FMUL, RGB_ORDER, 4> {};
....

There's also more #if + #ifdef than #endif, FWIW....

Yes - FASTLED_HAS_CLOCKLESS means "this platform defines lockless controller"

it's more like:

#ifdef FASTLED_HAS_CLOCKLESS

#if (CLOCKLESS_FREQUENCY == 8000000 || CLOCKLESS_FREQUENCY == 16000000 || CLOCKLESS_FREQUENCY == 24000000) //  || CLOCKLESS_FREQUENCY == 48000000 || CLOCKLESS_FREQUENCY == 96000000) // 125ns/clock
#define FMUL (CLOCKLESS_FREQUENCY/8000000)// GE8822

template <uint8_t DATA_PIN, EOrder RGB_ORDER = RGB>
class GE8822Controller800Khz : public ClocklessController<DATA_PIN, 3 * FMUL, 5 * FMUL, 3 * FMUL, RGB_ORDER, 4> {};

...

#else 


// Similar to NS() macro, this calculates the number of cycles for
// the clockless chipset (which may differ from CPU cycles)
#define C_NS(_NS) (((_NS * ((CLOCKLESS_FREQUENCY / 1000000L)) + 999)) / 1000)

// GE8822 - 350ns 660ns 350ns
template <uint8_t DATA_PIN, EOrder RGB_ORDER = RGB>
class GE8822Controller800Khz : public ClocklessController<DATA_PIN, C_NS(350), C_NS(660), C_NS(350), RGB_ORDER, 4> {};

...

#endif // 8mhz/12mhz/16mhz ifdefs

#endif // HAS_CLOCKLESS

There's two sets of definitions the first set of definitions are the FMUL based timings (if F_CPU == 8Mhz/16Mhz/24Mhz) and the second set of definitions are the #else which is all those chipsets with NS to CLOCK adjustments done by math.

There's also more #ifs + #ifdefs than #endifs, FWIW..

Look at the closures for those. They aren't matching up.

Wait - they're not unbalanced. #ifndef is in there.

But they still do not match up right.

#ifndef __INC_CHIPSETS_H
  #if defined(ARDUINO) //&& defined(SoftwareSerial_h)
    #if defined(SoftwareSerial_h)
    #endif
  #endif

  #ifdef FASTLED_SPI_BYTE_ONLY
  #else
  #endif

  #if FASTLED_USE_GLOBAL_BRIGHTNESS == 1
  #else
  #endif

  #ifdef FASTLED_SPI_BYTE_ONLY
  #else
  #endif

  #if FASTLED_USE_GLOBAL_BRIGHTNESS == 1
  #else
  #endif

  #ifdef FASTLED_HAS_CLOCKLESS
    #if !defined(CLOCKLESS_FREQUENCY)
    #endif

    #if (CLOCKLESS_FREQUENCY == 8000000 || CLOCKLESS_FREQUENCY == 16000000 || CLOCKLESS_FREQUENCY == 24000000) //  || CLOCKLESS_FREQUENCY == 48000000 || CLOCKLESS_FREQUENCY == 96000000) // 125ns/clock
    #else
    #endif
  #endif
#endif

This is from taking that file piping it through a grep for #if, #endif, #else and then hand indenting to show the nesting lines up

Darn, nevermind, I see your point. It's pretty confusing without indentation or comments to hint what blocks are what.

Yeah - that code was laid out a lot more simply when the only clockless chipsets I was supporting were the TM1809 and the new fangled WS2811 : )

My apologies for getting confused about it. Most of what I wrote up was based on an incorrect understanding. :-(

For all that, I think the 250ns timing (for the common WS2812) might be pushing things too close to the margin for the existing driver across all 8 RMTs. I'm curious if the same issue is seen with the slightly longer timings I've found to be typical of the most common such LEDs? I can't even imagine how TM1829Controller1600Khz would work in that regime...

(Again, assuming it's all that close to the edge of the margins when running all 8 RMTs in parallel).

Yeah - the 250ms timing was _really_ ugly when I was working on the 8Mhz avr implementation (for those playing along at home that's _2_ clock cycles between the high and low for a 0) - and I had to get that nailed because 325ms was getting flagged as a 1 ... and fun bonus, the 8Mhz avr I was doing that implementation on had no hardware multiply, which made implementing the inline scaling (which is also part of the color balance settings) extra fun...

It occurred to me just now that we should try using the built in RMT driver, for comparison. With this feature turned on, the controller converts all of the pixel data to RMT items before sending any bits on the wire. It should be pretty close to zero overhead during transmission. You can enable it using this directive before including FastLED.h:

define FASTLED_RMT_BUILTIN_DRIVER true

If that works for 8-way parallelism, then the problem is definitely the overhead of the fill routine (and its impact on the interrupt handler).

(The reason this option isn't the default is that it requires a lot of extra memory -- 32X the pixel data)

OK, I just pushed a commit to my clone of FastLED with the following changes:
(1) Convert CPU cycles directly to RMT cycles by computing the ratio between them rather than converting to nanoseconds.
(2) Replaced the complex indexing of RMT memory with a pointer that is simply incremented each time a new RMT item is added.
(3) Inline the getNextByte support routine

(2) and (3) should improve performance, hopefully enough to avoid problems with the full 8-way parallelism.

Hey Sam,
I've just checked with your latest commit - I can confirm 8 strips are working again. I'm running 8 strips with the code I posted above with no issues.

@jrhun That's great. I will submit a pull request to Dan.

@samguyer got a pull request?

@focalintent @samguyer
It seems that new RMT method broke RMT_BUILTIN_DRIVER on ESP32, the culprit is
xSemaphoreGiveISR in doneOnChannel, it should be:

            if (FASTLED_RMT_BUILTIN_DRIVER)
            {
                xSemaphoreGive(gTX_sem);
            }
            else
            {
                xSemaphoreGiveFromISR(gTX_sem, &HPTaskAwoken);
                if (HPTaskAwoken == pdTRUE)
                    portYIELD_FROM_ISR();
            }

I'm also currently investigating why I2s is not working in my project (I'm using one channel for I2s microphone, so there may be some conflict). I'll make a pull request after I'm finished with my investigation

Just to document my findings for anyone googling flickering and/or random flashes:

  1. Using default RMT implementation causes random flashes/artifacts when using ESPAsyncWebServer and AsyncMqttClient (up to one blink every 30 sec)
  2. Defining FASTLED_RMT_BUILTIN_DRIVER helps a bit, but they are still visible (about one blink per minute)
  3. I2S mode seems to eliminate the problems

I'm using single strip with 72 LEDs.

I have also testes NeoPixelBus and the same artifacts are visible in RMT modes (same frequency as in FastLED), I2S mode in NeoPixelBus also flickers (every 2 minutes or so).

It seems that the main problem is with interrupts.

One additional finding is that somehow interruptHandler is in IRAM, but the functions that it uses are not (may be worth investigating further), when using esp_intr_alloc with ESP_INTR_FLAG_IRAM it fails with INVALID_ARG which according to doc means that the interruptHandler is not in IRAM, fixing this may make FLASH_LOCK obsolete, since according to doc ESP_INTR_FLAG_IRAM interrupts will run when flash is being used.

@drzony I merged your pull request -- thank you!

One issue that we have been fighting for a while now is the interaction between FastLED and the ESP-IDF support for flash memory. If you are still having trouble you can try my fix that temporarily disables flash operations until FastLED.show() is done. You can enable it by adding the following line before including FastLED.h

define FASTLED_ESP32_FLASH_LOCK 1

@samguyer I've tried FASTLED_ESP32_FLASH_LOCK and it still does not help with random blinks. Currently the most stable (but still not perfect) is the I2S implementation. I have a very demanding project which uses I2S microphone, FFT for visualisation, MQTT for control (and sometimes a Web Server).
My findings in the previous post were using FASTLED_ESP32_FLASH_LOCK.
It seems that moving RMT implementation out of templated class could help as the functions would be in IRAM.
However seeing that the same thing happens in NeoPixelBus (truth be told I haven't checked whether their implementation sits in IRAM) might mean that there is still something hidden in ESP-IDF that makes WiFi interfere with RMT.
For example this effect (everything below is run every 40 ms):

uint8_t frame = beat8(5);
fill_rainbow(_leds, 31, frame, 255 / 31);
FastLED.show()

Causes random blinks when using AsyncMQTT in all implementations
For RMT modes blinks occur even when refreshing all LEDs with orange color.

Which version of FastLED are you using? Would you mind trying the current
repo head?

On Mon, May 25, 2020 at 12:29 PM Drzony notifications@github.com wrote:

@samguyer https://github.com/samguyer I've tried
FASTLED_ESP32_FLASH_LOCK and it still does not help with random blinks.
Currently the most stable (but still not perfect) is the I2S
implementation. I have a very demanding project which uses I2S microphone,
FFT for visualisation, MQTT for control (and sometimes a Web Server).
My findings in the previous post were using FASTLED_ESP32_FLASH_LOCK.
It seems that moving RMT implementation out of templated class could help
as the functions would be in IRAM.
However seeing that the same thing happens in NeoPixelBus (truth be told I
haven't checked whether their implementation sits in IRAM) might mean that
there is still something hidden in ESP-IDF that makes WiFi interfere with
RMT.
For example this effect (everything below is run every 40 ms):

uint8_t frame = beat8(5);fill_rainbow(_leds, 31, frame, 255 / 31);
FastLED.show()

Causes random blinks when using AsyncMQTT in all implementations
For RMT modes blinks occur even when refreshing all LEDs with orange color.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/FastLED/FastLED/issues/835#issuecomment-633643920,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AATSHK5GVCUXRTZP7LR5IW3RTKMIPANCNFSM4H66VDJQ
.

Just checked with current master, effects are exactly the same. I'm pulling the repo regularly.

Let me put my 5 cents into discussion.
Async webserver/websocket influences RMT operation indeed. Binding FatLEDshow() to core 0 makes it even worse - lot's of flickerings, I2S doesn't work for me either, do not know why - same flickering all the time.
The best flicker-less approach I was able to find is this:
1) Bind cpu-heavy calculatons thread to CPU0
2) run FastLEDshow() on a thread binded to CPU1
3) before executing FastLEDshow() I'm halting arduino's loop() until output finished
It all works pretty good as a result. I'm able to drive 512 leds in series at 100 fps.
(Full project code is here)

@vortigont Thanks for the rundown of your experience. Have you tried the latest Git head, which includes the ability lock out the flash memory subsystem? If you look in FastLED/platforms/esp/32/clockless_rmt_esp32.h, you should see instructions. The short version is you need to add the following #define BEFORE including FastLED.h:

define FASTLED_ESP32_FLASH_LOCK 1

@samguyer I remember I was trying this option some time ago but got almost no improvement.
I'll try to test it with recent HEAD again. BTW, what is your recommended approach to run fastledshow() and other threads? pinned/not pinned, which core?
Also I'm interested why I2S does no good to me, it gives the most flickering of all other options.
Are there any specifics to use i2s? i.e. specific pins or amount of maximum LEDs in strip? There is not much info about this around.
Thanks!

@vortigont Right now, it doesn't much matter which core/task you use to run FastLED.show() because we force it to be synchronous anyway. (That might change in the future, but the problem is that if you start changing the underlying LED array, you'll get a race condition).

The problem that we discovered on ESP32 is an unfortunate interaction between three different things. First, the ESP-IDF core, which has a somewhat draconian approach to reading/writing the flash memory: it disables all user code on both cores until the flash operation completes. This is the primary reason people see flashing when they use the web server. There is a special exception for interrupt handlers residing in IRAM. BUT...

There is a bug in gcc. We mark all of the ESP32 support methods in FastLED (in the controller classes under platforms) with IRAM_ATTR, which would normally cause that code to reside in IRAM, avoiding the problem. But it turns out that gcc does not properly preserve attributes across template instantiation. (Apparently, it is a long standing bug, which open questions about what the right policy should be). That wouldn't impact us BUT...

FastLED pixel controllers are templated with things like the color order in order to achieve the necessary speed on platforms like AVR.

So, there we are. If you have expertise in any of these areas, we'd love to get advice/help!

@samguyer
Here is my take on those issues:

  1. RMT code spends too much time in interrupts. On controllers like AVR long interrupts may not matter, but in more complex systems like ESP32 this may break a lot of things when components start fighting for interrupts. We should think about some compromise between memory usage and speed. My idea would be to create a buffer that is filled in asynchronously by a background thread.
  2. Since we are on a much more powerful platform than AVR we can spare a few bytes to make a solid not-templated engine and then create wrapper classes compatible with current FastLED philosophy. Due to used HW and its limitations templating does not make any sense here.

@drzony Getting the interrupt handler into IRAM will address both problems. It will make the code run faster, and it will avoid the interaction with the flash subsystem. I'll think about ways we might be able to make that happen.

I'm not sure the driver spends that much time in interrupt handlers. All it does is convert 24 bits of data into 24 RMT signal items. We could certainly look at the inner loop, but I'm guessing there's no much more to do. And compared to the amount of time it takes to send those bits on the wire, it's very short.

@samguyer The problem here is that the more time we spend in the interrupt the bigger chance that some other interrupt will come and break the buffer or that we will not fill the buffer on time.
@vortigont I'm using I2S without any problem on PIN 23. From your code I can see that you are using ADC, this may be the cause of problems (maybe some config?)

@drzony and @vortigont I have just finished a major refactoring of the default ESP32 driver (the RMT-based driver). I posted information on the FastLED subreddit. I'm wondering if you guys can try it out and let me know if it helps. You will need to clone my personal fork:

https://github.com/samguyer/FastLED

The short version is that I separated the code into two classes: one that interacts with FastLED (the templated class called ClocklessController) and a new one that interacts with the RMT peripheral. The main benefit is that it gets around a long-standing issue in gcc where method-level attributes are not preserved across template instantiation. With the new design I can use IRAM_ATTR to ensure that all the relevant interrupt code resides in IRAM.

@samguyer
I've tested your new version. Unfortunately it did not help in terms of random blinks. Also it fails to compile on PlatformIO, the issue here is that PlatformIO only compiles .cpp files in the main folder of the library by default (moving .cpp file to main folder fixes that).

@drzony That's a shame. As far as PlatformIO goes, I'm not sure how to proceed. The Arduino IDE changed its policy regarding cpp files a few versions ago, but I suppose I could leave the cpp file in top-level directory (it has to be guarded by #ifdefs either way)

I'm interested to understand the random blink issue you're having. If you slow down the frame rate, does it also slow down the blinking? One telltale sign of interrupt interference is that the borked signal shows on the LEDs until the next call to FastLED.show().

For PlatformIO you can modify library.json:

"srcFilter": [
    "+<*.c>",
    "+<*.cpp>",
    "+<*.h>",
    "+<platforms/esp/32/*.cpp>"
]

I've run this at 25 and 50 fps, it seems that blinks happen during network activity (disabling network parts in my code seems to help). Currently I'm testing this with 72 LED strip and sending messages via AsyncMQTTClient every 2-3 seconds. The minimal test case on my side is to set all the LEDs to single color. After doing that I get random LEDs blinking with a random colors. Usually only one random LED blinks with a different color, sometimes there are more (like 4 or so). It's most visible on color where 2 of 3 colors are lit.

@samguyer OK! I will test it within my project and let you know the outcome.
@drzony Thanks for the tip with PlatformIO

@drzony I will update the library.json -- thanks for that info!

Is the AsyncMQTTClient the only other software that might be interfering with FastLED.show()? I can dig around in the code and see if anything jumps out at me.

Also: which core is running FastLED.show? Are you using the code that starts a separate task to do show?

@drzony I just pushed your change to library.json (to my repo).

@samguyer I've been looking into your code and maybe calling gpio_matrix_out makes the whole thing not stored into IRAM. Or maybe some other call messes up things. Maybe if I'll have some time I'll do it in "good old C" to make sure that everything is kept bug-free. (I've been looking into some comments and it seems calling any non-static method may also cause the gcc bug to appear)

@samguyer I found at least 2 functions that are not in IRAM - rmt_set_pin and rmt_tx_start which are used from the interrupt. Will look further into it tomorrow.

@drzony I can definitely "inline" those functions. They don't do anything special -- just set/clear registers. I was just calling them because it seemed better for long-term maintainability than writing that code myself.

@drzony Looking at the code more carefully, the call to rmt_tx_start will only be called from the interrupt handler if there are more than 8 strips added. I think I can get rid of the call to gpio_matrix_out -- I don't think it's necessary to actively shut off the output to the pin.

@drzony I pushed that one change to my repo. I just commented out the call to gpio_matrix_out, which I don't think was ever necessary in the first place.

@samguyer I tested your change and not much changes.
I've also tried running FastLED.show on a high priority task as advised here on both cores and FASTLED_ESP32_FLASH_LOCK=1 (It doesn't change anything)

Here are my findings:

  1. RMT Standard run: random LEDs flashing almost every time I send a MQTT message (every 3 secs)
  2. RMT Run in separate task (core does not matter): helps a lot, but there are still some random blinks (every 10-20 messages)
  3. I2S mode seems to work perfectly both normally and on a separate task.

Seems that some low level stuff in AsyncMqttClient messes up the RMT. I2S is also using interrupts and is not affected, so maybe there is some interference between network and RMT? I'm running out of ideas here.

@drzony That is very weird! Especially that I2S works so much better. We haven't even moved its methods into IRAM yet (like we did for the RMT driver).

I'd like to try to replicate your set up, if that's possible. Is there a minimal hardware/software config that exhibits the problem?

@drzony Actually, can you take a video of the behavior first? I just want to see the particular flashing pattern.

Here is the video:
LEDs.zip

In this example it seems that every 4th pixel is affected however not in the same way. Also it's only part of the strip. Capturing this on video is very difficult since it happens randomly depending on strip color, network traffic etc.

I'll try to make a minimal example for you to test.

@drzony Interesting. Is the pattern of flashy behavior pretty consistent, or does it vary?

I wonder if we could learn something by calculating the time between four pixels of data. I wonder if that might correspond to some timing element of the MQTT protocol.

Here is the minimal example to reproduce the issue:

#include <Arduino.h>

#include <AsyncTCP.h>
#include <FastLED.h>
#include <WiFi.h>

CRGB leds[72];
AsyncClient client;
char payload[1024];
double numbers[1024];

void dataHandler(void *arg __attribute((unused)),
                 AsyncClient *client __attribute((unused)),
                 void *data,
                 size_t len)
{
    char *ch_data = (char *)data;
    if (len == 1024)
    {
        for (int i = 0; i < 1024; i++)
        {
            payload[i] = ch_data[i];
        }
    }
}

void setup()
{
    Serial.begin(115200);
    delay(10);
    Serial.println('\n');

    WiFi.begin();
    Serial.println("Connecting");

    while (WiFi.status() != WL_CONNECTED)
    {
        delay(1000);
        Serial.print('.');
    }
    Serial.println();
    Serial.println("Connection established!");

    client.onData(dataHandler);
    for (int i = 0; i < 1024; i++)
    {
        payload[i] = 'A';
        numbers[i] = 123 * i;
    }
    payload[1023] = '\n';
    FastLED.setDither(DISABLE_DITHER);
    FastLED.addLeds<NEOPIXEL, 23>(leds, 72);
    fill_solid(leds, 72, CRGB(255, 27, 2));
}

void loop()
{
    static uint32_t frame = 0;

    if (frame == 0)
    {
        if (client.connected())
        {
            client.write(payload, 1024);
            Serial.println("Written");
        }
        else
        {
            Serial.println("Connecting TCP");
            while (!client.connected())
            {
                client.connect("192.168.0.1", 5500);
                yield();
                delay(1000);
                Serial.print('.');
            }
            Serial.println();
        }
    }
    FastLED.show();
    for (int i = 0; i < 1024; i++)
    {
        numbers[i] = 123 * numbers[i] / pow(numbers[i], 5) + sqrt(numbers[i]);
    }

    yield();
    delay(1);

    ++frame %= 25;
}

On a PC you can run socat -v tcp-l:5500,fork PIPE to act as a simple echo server
Here is a Windows version of socat - link

My observations:

  • disabling "double" calculations makes the problem almost disappear
  • disabling sending fixes the problem
  • as for MQTT timing, the protocol is a simple TCP protocol, there is no timing to speak of (at least not in terms of single pixel)
  • I've reduced the example to AsyncTCP only
  • The pattern seems consistent (currently I'm observing exactly the same LEDs flashing), but it appears randomly and it's reaaaaaly hard to tell because the blink lasts only 1 frame
  • With the color set as in the example I get green flashes (so it seems that red channel is not coming through, every 12th byte)

@drzony Thank you! I have to say, I'm having a very hard time even coming with a hypothesis for this problem. The RMT and I2S drivers are almost identical in terms of interrupt use (the new RMT driver is considerably better because it spends much less time in interrupts and is all in IRAM).

What I might do is instrument the driver so that it detects and reports any timing disruptions. That might help us diagnose the failure.

@samguyer I've updated my previous comment with a simpler reproduction

@drzony OK, thanks.

Here's another thing that puzzles me about this issue: why aren't LEDs later in the strip affected? Usually if the signal gets interrupted, the timing of everything after that is perturbed, causing the remaining LEDs on the strip to get wacky values. But that's not what I'm seeing here. One or two LEDs get a weird value, but then everything else seems correct. For a timing-sensitive serial protocol, that is very weird.

Not really, the LEDs are stopping and regenerating the signal. So in case of 2 LEDs (A, B) this would be something like this:

  1. A - is waiting on reset (long low), B - is waiting on reset (long low)
  2. A - is receiving wonky signal for A (it's blocking the signal on it's out), B - is waiting on reset (long low)
  3. A - Displays broken color (starts reproducing signals on out), B - is waiting on reset (long low)
  4. A - Receives good signal for B (and regenerates it on out), B - is receiving good signal

It depends a lot on what's going wrong. Remember that the bits are represented by timing, so changes in the timing (like an interval that's too long) can effectively "shift" the position of all the subsequent bits.

I think what we are seeing here (lack of red) means that instead of 1 (0.8us Hi + 0.45us Lo), we get 0 (0.4us Hi + 0.85us Lo) . Probably high signal is cut off too early. Reset is 50us, so that's why other LEDs can get the signal + I'm sending a single color, so the signal repeats itself very often.
Are you able to reproduce the effect using my example?

@drzony I have not had a chance to try it yet. Can you set the LEDs to different colors? One pattern that I find super-useful is just red, green, blue, red, green, blue, ... that way you easily can tell if anything is shifted or if any particular color channel is having a problem.

@drzony Here's another easy check we can do: time the call to FastLED.show() using the high-res timer, and look for anomalies. I can try that.

@drzony I was able to get your test code running -- thanks for that! I do not see any visual artifacts, but I am seeing occasional timing anomalies that I can't explain.

What I did is instrument the driver so that it records the time (number of CPU cycles, for high precision) between each interrupt/buffer-fill. That number should be exactly (or close to) 9600. What I'm seeing is that once in a while it gets up over 10,000 and occasionally up to 13,000. I'm not sure why, but it does seem worrisome. I'll keep digging.

The problem seems to happen right around the time the TCP transmission occurs, so that points to an interrupt issue. Maybe the FastLED interrupt is not high enough priority.

Currently it's LEVEL3, according to headers LEVEL4 and above are "High level interrupts. Need to be handled in assembly.", so that would be tough.

OK, I'm going to try a different tack: I'll just use the interrupt handler to signal to the user-level code to refill the RMT buffer. In some ways, that's more in the spirit of interrupt handlers anyway. It's going to require some restructuring of the code, but I'll let you know when I've got something working.

I also found a discussion on the ESP-IDF discussion forum that the WiFi task is pinned to Core 0, so it might be good to move the FastLED stuff to Core 1. See here:

https://www.esp32.com/viewtopic.php?t=9497

@samguyer
By default on ESP32 Arduino task is pinned to core 1, so adding another task does not change anything (however as I've noticed before creating a task with higher priority helps a bit). The interrupt should be created on current core so that also should be good.

I've created a repo with my tests: https://github.com/drzony/FastLED-tests
It includes the code for running FastLED.show() on a separate task pinned to any core.

In case you are not using PlatformIO (I highly recommend it), it's available as an extension for VSCode and it contains everything that is required to work with it. When using it, just clone your FastLED into lib/ in my repo and you should be good to go.

@drzony I just pushed a version to my FastLED repo (not the main one) that changes the strategy for refilling the RMT buffer. The interrupt handler just records which channels (which strips) need attention, but the actual work is done outside the interrupt handler. I'd be interested to see if this approach improves the situation for you.

@drzony Actually, hold off using that code. I don't think it works correctly. But I think I'm getting closer to understanding what's going on in the interaction between FastLED and the WiFi subsystem.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

CRCinAU picture CRCinAU  Â·  8Comments

marcmerlin picture marcmerlin  Â·  3Comments

ccoenen picture ccoenen  Â·  6Comments

scodavis picture scodavis  Â·  8Comments

blackketter picture blackketter  Â·  8Comments