pixel.show() crash with more than 73 pixel on ESP32s3

[tresler]

I have weird problem. I test example code or example with new operator and everything works fine if I have 73 or less RGB leds. If I use 74 or more, everything crash. Can anyone explain it?

Debug code: adafruit_neopixel_crash.txt

• Arduino board: ESP32s3-WROOM
• Arduino-ESP32: 3.0.0 alpha 3
• Arduino IDE: 2.2.1
• Adafruit_Neopoxel: 1.12

[ajkeeton]

Same problem here, also Esp32 (Heltec Wifi Kit), and at around 70 pixels

[teknynja]

I'm experiencing the same issue as well (crash/boot loop when driving more than around 70 pixels). This occurs on both my XIAO ESP32C3 boards and my ESP-WROOM-32 Dev Modules, with the following hardware/software:

• Boards: XIAO ESP32C3, Generic ESP-WROOM-32 38 Pin Dev Board
• Board Package: esp32 by Espressif (both v3.0.0 & v3.0.1)
• Adafruit NeoPixel v1.12.2
• Arduino IDE: 2.3.2

Digging around in the Adafruit NeoPixel Library, using the v3.x board package means were also using IDF v5, which is pretty simple, and the NeoPixel Library doesn't appear to be to blame for this issue. I've isolated the crash to where it's calling: rmtWrite(pin, led_data, numBytes * 8, RMT_WAIT_FOR_EVER);. The crash happens somewhere in there. Digging a little deeper, it doesn't look like the problem is in the IDF library either, since it doesn't really do much with the requested buffer size except to pass it along to the rmt_transmit method of the SDK. I expect somewhere downstream there a buffer is overrun and it eventually overwrites something important causing the whole thing to crash and reboot. 70 pixels turns out to be a fair amount of data: 70 pixels * 3 bytes for RGB, then each byte requires 8 RMT Items which themselves take 4 bytes each - 6720 bytes! This is far more than the ESP32-S3's 2048 bytes of RMT block memory (or the ESP32-C3's meager 768 bytes), so the SDK must be handling the buffering itself.

I'm not sure at this point where to turn for a fix though, since it looks like this is a problem with Espressif's v3 SDK. I'd really like to get this fixed, as I have a few projects with 150 or more pixels that I currently can't complete with this setup. I'm sure there's plenty of other people as well that expect to be able to drive larger pixel strings, so it'd be nice to get this working for everyone on the ESP32 platform.

[teknynja]

So I posted about this issue on the Espressif forums (https://www.esp32.com/viewtopic.php?f=13&t=40270) and they quite helpfully pointed me in the right direction; it appears that the culprit in the NeoPixel library is the espShow() method in the esp.c file is allocating the RMT Items Buffer (led_data) on the stack, and at about 70ish pixels it exceeds the available stack space and causes the crash. I'm guessing that allocating that buffer from the heap (or statically) would likely greatly increase the number of pixels that can be driven.

While looking at this code more closely, I also noticed that it's calling rmtInit() every time the pixels are "shown" - I'm assuming that it probably should only be called once (maybe in the .begin() method?). There doesn't appear to be a close/cleanup function in the Adafruit_NeoPixel driver, so there's probably no place or need to call the matching rmtDeinit() to release the RMT channel.

[robertlipe]

@teknynja , good thinking, but neither of the provided examples allocate the buffer on the stack. The first has it as a global in .data. The second has it allocated from the heap via operator new[]. So that's not it.

The single provided crash wasn't run through the symbolicator. See idf.py monitor and decoding at, e.g. https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-guides/tools/idf-monitor.html Many IDEs offer convenient ways to do this.

Note that the ESP32 parts have harsh rules about what registers can be poked at interrupt vs. base times and some other context rules. Is it possible that something is trying to refill a buffer and the hardware disappears from beneath it? That'll show that you'll be deep into a loop and the load/stores that have worked many, many times suddenly result in an invalid opcode (?) exception, thought the opcode hasn't changed.

Offhand, is this perhaps taking long enough that the watchdog is firing?

This caught my eye as I have a project that uses FastLED to actually feed the LEDs and it also blows up on s3, thought he breaking point is well into the many hundreds of LEDs. The breaking point doesn't seem to be absloute, but at X it'll run for hours and ad X+10 it'll crash within minutes. FastLED and NeoPixel are different code bases, so it's unlikely that they're bug-compatible.

The unfortunate memory footprints with either RMT or SPI are bad on long/many strands of WS2812's. It's annoying enough that I've started investigating slightly more expensive parts that have real DRAM - and lots of it - for larger pixel budgets. When 64 and 256MB parts are still under $10 - and predictably fast - I have to ask if the PSRAM pain is worth the pull.

[teknynja]

@robertlipe - I'm definitely not an expert C/C++ coder, but to my eye

void espShow(uint8_t pin, uint8_t *pixels, uint32_t numBytes, boolean is800KHz) {
  rmt_data_t led_data[numBytes * 8]; // <== stack allocation??

  if (!rmtInit(pin, RMT_TX_MODE, RMT_MEM_NUM_BLOCKS_1, 10000000)) {
    log_e("Failed to init RMT TX mode on pin %d", pin);
    return;
  }

  ...

the rather large led_data array is being allocated on the stack (plus in my demo code, once I change that to a static allocation outside the function, I'm able to send data to several hundred pixels without it crashing).

I haven't had a chance yet to try rigging up a test with allocating that buffer from heap in the actual driver, but I expect that should at least clear up the crashing issue.

[robertlipe]

Ah. That code wasn't in the original post. Yes, that code is being silly. That's very much going to blow up in a typical FreeRTOS (or most other multitasking) kind of environment. It's absolutely worth the effort to have pixels[] correctly sized and allocated (or at least have led_data allocated and reallocated if it grows) so that this additional buffer isn't necessary. That's indeed the root of this bug. Good find. Since you seem to have a test case in hand, you might be able to push this along with a (non-multithreaded) fix like changing void espShow(uint8_t pin, uint8_t *pixels, uint32_t numBytes, boolean is800KHz) { rmt_data_t led_data[numBytes * 8]; // <== stack allocation?? to void espShow(uint8_t pin, uint8_t *pixels, uint32_t numBytes, boolean is800KHz) { static uint32_t led_buf_size{0}; static rmt_data_t; if (numBytes * 8 > led_buf_size) { static led_data = malloc(numBytes * 8); assert(led_buf_size); led_buf_size numBytes * 8; //rmt_data_t led_data[numBytes * 8]; // <== stack allocation?? This isn't really thread-safe, but that's probably OK for 99% of the callers since it only grows. It also saves the malloc on every draw(), keeping it but so that it only grows over time to reduce the possibility of fragementation, failure, or flicker if its slow. It'll quickly reach a stable state and hold onto that buffer. Clean that up and propose it as a pull request if that basic approach works. I'm coding from bed (not feeling well) and not a debugger so there may be sloppiness in that. A far, far better approach is to make the caller aware of led_buf_size and let IT allocate and manage that. That solves the threading problem. This is a case of trying to hide something that the caller actually DOES need to know about. Premature abstraction. Good luck.

…

On Sun, Jun 9, 2024 at 7:37 AM teknynja ***@***.***> wrote: @robertlipe <https://github.com/robertlipe> - I'm definitely not an expert C/C++ coder, but to my eye void espShow(uint8_t pin, uint8_t *pixels, uint32_t numBytes, boolean is800KHz) { rmt_data_t led_data[numBytes * 8]; // <== stack allocation?? if (!rmtInit(pin, RMT_TX_MODE, RMT_MEM_NUM_BLOCKS_1, 10000000)) { log_e("Failed to init RMT TX mode on pin %d", pin); return; } ... the rather large led_data array is being allocated on the stack (plus in my demo code, once I change that to a static allocation outside the function, I'm able to send data to several hundred pixels without it crashing). I haven't had a chance yet to try rigging up a test with allocating that buffer from heap in the actual driver, but I expect that should at least clear up the crashing issue. — Reply to this email directly, view it on GitHub <#375 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD34T7VLRZYW6KK6Q73LZGREBPAVCNFSM6AAAAABAYU77RSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJWGUYTMOJVGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[teknynja]

I've submitted a pull-request with fixes for this issue. I'm able initialize the driver configured for 500 pixels and it seems to be working fine. Anyone who would like to try this code out can grab the changes at teknynja/Adafruit_NeoPixel/tree/esp32_rmt_memory_allocation_fix - let me know if it works for you or if you encounter any problems. Thanks again for the help from @robertlipe on getting me started on this patch.

[Pr77Pr77]

@teknynja Your code works in my case. Thank you and @robertlipe very much. Also the link seems to be dead. This link to teknynja's fixes should work: https://github.com/teknynja/Adafruit_NeoPixel/tree/esp32_rmt_memory_allocation_fix

[teknynja]

For anyone using this library in multi-threaded scenarios, I've created a new branch with the same fix but it also includes a mutex to make it thread safe (works just fine for single-threaded apps as well): teknynja/Adafruit_NeoPixel/tree/esp32_rmt_memory_allocation_fix_safe. This is the preferred update for both single and multi-threaded applications.

[robertlipe]

I like this a lot. There's a race. Condition at 51. Multiple threads could both enter and both find the muted null and bith create them and the operate with different lock instances in that first run. Pretty minor. Can it just be allocated in a larger scope where it'll only be allocated once? But overall, I really like this. Thanks for doing this and sharing it.

…

On Wed, Jun 12, 2024, 12:00 PM teknynja ***@***.***> wrote: For anyone using this library in multi-threaded scenarios, I've created a new branch with the same fix but it also includes a mutex to make it thread safe (works just fine for single-threaded apps as well): teknynja/Adafruit_NeoPixel/tree/esp32_rmt_memory_allocation_fix_safe <https://github.com/teknynja/Adafruit_NeoPixel/tree/esp32_rmt_memory_allocation_fix_safe>. This is the preferred update for both single and multi-threaded applications. — Reply to this email directly, view it on GitHub <#375 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD35RKD67MCB43FQXBY3ZHB5CJAVCNFSM6AAAAABAYU77RSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRTGUYTGMJUGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[teknynja]

Yeah, i wasn't sure how to avoid that without having a way to initialize with the instance...

[robertlipe]

Right, you'll have an allocated an unreachable object and never have a way to release it. That's not totally terrible. but we can do better. We need something that acts like a mutex, while we initialize our mutices. (mutexen? :-) mutexan? 🤠👢? ) This calls for what C++ nerds call a singleton. How can we enforce locking while bringing up our locks? We use a std library function that does it behind our back! static std::once_flag flag_; // Notice the flag isn't even really used; it just gives us an object we can put junk in.) If you want a peek at how pros like Google do this, we can see the source they used before C++11 was common in their super-useful abseil library here (and while I dig abseil, it's 2024 so C++11 should be everywhere that matters by now, so we'll use the version in the std:: library instead of linking against Google's We're just using their source for education. (Yes, I admit I had to look it up myself...): https://github.com/abseil/abseil-cpp/blob/master/absl/base/call_once.h Better still, we can see how they use it in their own tests. (foo_test.cc tests foo in Google-land.) https://github.com/abseil/abseil-cpp/blob/master/absl/base/call_once_test.cc So I think line that code starts to look something more like: static std::once_flag flag; static std::mutex show_mutex; std::call_once(flag, []() { show_mutex = xSemaphoreCreateMutex(); }); Now xSCM will be calle dexactly once and show_mutex will be set exactly once. Wait, what is this []() { stuff; } ? That is a Lamba. It lets you create and use a function in place without actually giving it a name. We could make a set_mutex(void) { show_mutext = xCSM() } ... but now we have to expose show_mutex to it. We could do that by passing in an pointer, and then set_mutex(std::mutex* foop;) *foop = xCSM() which for such a tiny function isn't a big deal for a one-liner, but if there were some heft to that function, and it neede access to lots of internals, you can probably see this becoming a getter/setter nightmare. There are ways to pass arguments to lambda:s auto addr = [](int x) {return x+1;} and exotic ways to 'capture' more arguments and possibly defer the binding until the actual call. We don't NEED any of that, so I'll keep the syntax simple and give you a few keywords to satisfy your curiousity, if I've not already sent you into the woods. Honestly, it's also pretty rarethat these would be statics in a Real Program, they'd be stored in some kind of a class holding espShow() and friends, but I'm trying to keep this light and not rearrange much, keeping with your original principles. In case it helps, since I had to look up the syntax myself, here's the standalone test program I used to sketch it out: #include <mutex> #include <thread> #include <iostream> void t() { static std::once_flag flag; static std::mutex show_mutex; #if 1 || TEST static void *p { nullptr }; std::call_once(flag, []() { p = malloc(1024); }); std::cout << p << std::endl;; #else std::call_once(flag, []() { show_mutex = xSemaphoreCreateMutex(); }); #endif } int main() { t(); t(); t(); return 0; } Here, when building -Dtest, you can see that p is calling malloc each time we're called (leaking) and we should see P dropping 1K each time if we just called malloc each time, but you'll see we call malloc once and the returned (OK, I didn't actually return it; I just printed 'p') actually stays the same upon every call: ➜ /tmp c++ -std=c++20 x.cc -o x && ./x 0x121808800 0x121808800 0x121808800 So, we could have built a real function and passed the address of the function to call_once. That function would then do the work and return - once. But I chose instead to do the exact same thing in a lambda because here, it's just a bit more tidy and keeps our internals internal without. Where else can you use lambda? Anywhere you'd use a function pointer. Perhaps obviously, they're handy for things like GUI callbacks, but qsort lends iteslf well to lambda use as they're "just" unnamed functions. The syntax is a little egg-headed, but knowing where to find good examples helps. YOu can capture by rference [&] (which aligns to pointer syntax) or bay value [=] which is fairly mnemonic. Varargs are possible, but let's leave those alone. IMO, they make code easier to read when implemented well, but the people that think that 1972 C was good enough for everyone will always scream about them. So today we learned about a Singleton - a function that always returns one, correctly initialized object. It's like a global and some people fuss about that design pattern. It's difficult to define destruction order, for example and they share lots of problems with globals like global mutable state and difficulty to mock for tests. But here, you're always dealing with one THING and dealing with it in a small, controlled way, so it seems OK. One reasonable description is given at https://refactoring.guru/design-patterns/singleton Lambda - Lambdas are just functions with no names (!) that can be used in line and have a capture list (the thing between []) and variables that are passed by reference & or by value = and a paramater list included in the (), just like a function does. Tutorials like https://smartbear.com/blog/c11-tutorial-lambda-expressions-the-nuts-and-bolts/ do a good job without just repeating the cppreference page, which aren't always for time before coffee. It's a lotta typing for what I think was like a three line "fix" to a problem we've agreed wasn't a totally terrible problem. My OS used to be used for the controls on submarines, for example, and my final employer used to have s (true) saying that around (t)here, things that happen "once in a million times" happen multiple times every day. So I like to tidy things up as tight as I can. As always, feel free to use that code in your PR (please) and I hope you (and anyone else reading this) learned something along the way. Maybe it'll help the reviewer recognize what we did and why. Enjoy

…

On Wed, Jun 12, 2024 at 1:23 PM teknynja ***@***.***> wrote: Yeah, i wasn't sure how to avoid that without having a way to initialize with the object... — Reply to this email directly, view it on GitHub <#375 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD3ZBGHRI7JH6EDH2XKDZHCGZDAVCNFSM6AAAAABAYU77RSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRTGY2TIMBSGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[teknynja]

hmm, it doesn't appear we have access to the std library in this project (perhaps because this is just a plain ol .c file?). At least when I try to build in the Arduino environment, it complains that it can't find the header files. Also not sure how the maintainers would feel about adding references to std:: since it doesn't appear they are using it anywhere else.

Your example did get me looking into possible solutions from within the FreeRTOS framework itself, and I discovered the xSemaphoreCreateMutexStatic function which looked very promising, but digging through the FreeRTOS source code, it looks like that call is probably not be thread-safe either. :-/

I'm wondering if I could just do something quick-n-dirty, like disabling interrupts around where I'm checking/creating the mutex... it should be pretty quick through that section, and is in keeping with the design philosophy of this library. (Edit: Ugh, disabling interrupts would only solve it on single core apps!)

I nags at me too, having even a tiny crack like this race condition in the code - my day job is working on industrial process control systems and we'd never let something like that into production.

Interestingly, I just started looking into c++ lambdas a short time ago to try and solve a different problem (I'm a big abuser user of lambdas in C# though!)