Use calloc to clear transmit buffers upon allocation #905
Conversation
|
After some more testing all |
- sometimes buffers were swapped before 1st Show()/Update()
|
If there aren't c++ constructors initializing every member, and there obviously isn't or you wouldn't be using malloc(), this is just good taste. Either this or a move to actual construction is surely necessary for robust performance in real world systems. If you need help with "money where your mouth is", I'll help wire up a build. I'll provide an interposed malloc that always returns buffers of 0xfd (or something else really annoying) and will help debug it if that's necessary and welcome. I'm not just running my mouth. This is a good idea and if that's challenged (maybe the buffers are otherwise initialized - great!; maybe there's a belief that PODs have initializers; they don't. I'll help pull out iso specs and/or help test/debug if there isn't overwhelming consensus that this is the Way To Go. |
|
@robertlipe the main issue I've been facing was "random" flashing when using This "pixel data", although originally This allows some freedom and creativity but with uninitialised second buffer and consistently skipping the same pixels in each frame produced unwanted "random" flashing of those unpainted pixels due to one of original buffers not being initialised. As mentioned there are two ways to initialise both buffers. I'm now convinced that it is better to use higher level clearing (in BTW, I am not a developer by trade so may have misinterpreted the |
|
I understand. I didn't analyze every call trace all the way to the top, but this seemed quite reasonable to me. (We've "met" before...) Your symptoms and solutions are spot-on. I won't say that a memset per alloc/frame is free, even on an ESP32 vs. a VAX. Unless it's absolutely, positively known that someone else (like C++ initializers) is going to DTRT behind the scenes, that memset/bzero/mmap from /dev/zero (clearly not on ESP32) is absolutely worth the water carried in the name of predictability and stability. Then again, if you're doing an alloc per frame, you've already lost some battles. I'm just saying that I pencil-whipped this, and it seems really sane, though I don't know every path through this code or if it could be assured that it's never doing a bzero twice. I'm saying that it's a good idea, and if the lead devs need help proving that, I'm willing to invest some brain and machine cycles to make it so, though it might be on a flaky schedule. That said, this code looks to be C++, so it MIGHT be the case that _dataEditing/_sizeData (which might be a std::span in modern C++) should instead just be a smarter (non-POD, non size_t/void* tuple) type that initializes itself to be a smarter/smaller type, potentially saving the bus terrorization of a memset/0. I can't test all the cases that you've touched, but I'm willing to wire up a similarly failing case (assuming I can match the hardware), try to profile/study the failing case, and help validate the maxmially awesome case because I think you're onto a good lead here. If the project/SOC has the bandwidth to just accept the change to calloc without blinking an eye, it's hard to argue that's not the right solution. IMO, you're onto something here that's an actual problem. I'm just hoping to help you get the attention of the right people and, in case you can't, offer some time to help pan for gold in the profiles. Blink on! |
|
To add some context (that I gathered during studying code): memory allocations only happen upon object construction (or during Begin() call as I propose in #906 ) and not later in the flow. |
|
A great argument is that initializing them "correctly" doesn't have a huge
overhead.
You could possibly defer the memset to happen upon the write to _sizeData
(so you don't have to zero all of a huge buffer, just the part that's
'valid') .
You could probably make MemorySize and friends use RAII and
automatic deleters, wire them to calloc for bzero functionality, and not
quite incur the overhead of a std::vector and wrangling that into the PSRAM
allocators.
#include <memory>
// Define a type for a pointer that automatically frees itself
// internal::NeoUtil::Free is likely just 'free', but fits the library style
using BufferPtr = std::unique_ptr<uint8_t, decltype(&free)>;
// usage inside the class
BufferPtr _pixels;
// In your initialization method:
size_t size = MemorySize();
// Use calloc (or heap_caps_calloc for ESP32 DMA) explicitly
uint8_t* raw_buffer = static_cast<uint8_t*>(calloc(1, size));
// Hand ownership to the smart pointer
_pixels = BufferPtr(raw_buffer, free);
But TBC: there's no shame in just making what you have use calloc instead
of malloc. It's correct, it's safe, and - if not done per frame or somthing
irresponsible - fast enough. Correctness trumps saving a microsecond that
_usually_ works.
RJL
…On Sat, Jan 31, 2026 at 4:02 AM Blaž Kristan ***@***.***> wrote:
*blazoncek* left a comment (Makuna/NeoPixelBus#905)
<#905 (comment)>
To add some context (that I gathered during studying code): memory
allocations only happen upon object construction (or during Begin() call as
I propose in #906 <#906> ) and
not later in the flow.
—
Reply to this email directly, view it on GitHub
<#905 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD3ZGJINAWDNTCYHTS3D4JR4RVAVCNFSM6AAAAACSMZJO76VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTQMRYGA3DAMRXG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I know some people that will object to that. Sadly now in charge of a well known OSS. 🤷♂️ |
|
I don't care how fast something doesn't work. "Fast, but glitches wrong
when a buffer has been used before" is broken in my book. That what you've
fixed.
Now if it's a case that a tail pointer is being advanced before a buffer
fill has fully arrived or something else where is really SHOULD be written
by something else before it's used or something, then the calloc is fixing
the wrong problem.
I'm pretty sure we're on the same side. I'm agreeing that this is important.
Especially if this isn't in the per-frame path, the difference in malloc vs
calloc is that one is correct. Given how the device will burst consecutive,
aligned, memory transfers, even/especially to PSRAM (it doesn't have to re
clock the address between words of a burst), it just doesn't cost that much
on modern arches like esp32.
…On Sat, Jan 31, 2026, 9:03 AM Blaž Kristan ***@***.***> wrote:
*blazoncek* left a comment (Makuna/NeoPixelBus#905)
<#905 (comment)>
Correctness trumps saving a microsecond that *usually* works.
I know some people that will object to that. Sadly now in charge of a well
known OSS. 🤷♂️
—
Reply to this email directly, view it on GitHub
<#905 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD32VWTUS2ZKYRZ7LPA34JS73NAVCNFSM6AAAAACSMZJO76VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTQMRYGY4DANJTHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Not specifically opposed to this, but it doesn't really solve the problem in all cases. NOW, consider if your LEDs use 255 for black (yes, there is one LED strip model that inverts the values for some reason). There are some that have per pixel config that must be a specific bit pattern. Now you just initialized to all ON rather than all off or a corrupt set per pixel config data. This is what ClearTo() was created for. You can't assume the color values or format. And you can't assume this per pixel config data didn't change by calling SetSettings (either the feature or the method, both can expose it). Now the case of the per pixel config data, that is set latter after construction so calling ClearTo() in the constructor won't fix the per pixel config corruption. This "old" model was either you write every pixel every frame and call Show(false), or you call Show(true). The "new" model is the Shader branch, where there is a single output buffer that is composed when you call Show(), so these per pixel config get recreated every time as the front buffer is translated into the hardware sending buffer all at once, rather than on the SetPixelColor(). |
Well, it sort of does. LEDs don't blink. However, not everyone will like "on" LEDs in case 0 is not black.
Figured out as much and hence reverted this PR to draft (to serve as a discussion). |
3 participants
RMT, RP2040x4 and 8266AsyncUart methods covered.
However I have also made 8322fce if that would be a better fit.
Resolves #904