Additional members of ws2811_t structure need initialization

[davthomaspilot]

I was getting segfaults until I initialized:

ledstring.channel[0].gamma =0

Maybe some compilers initialize structures to zero, but it didn't in my environment. If it's non-zero, the gamma array never gets allocated, and sigsegv occurs on first attempt to index into it.

Similarly, until I set ledstring.render_wait_time=0, bad things occur.

But, I have no idea what render_wait_time is for--it would be nice to know what all members of the structure do.

[davthomaspilot]

Initializing channel[0].gamma to zero, fixed my segfaults, but was still getting them when I did the ws2811_fini() call to clean up.

I tracked that down to an attempt free the channel[1].gamma memory. Which had never been allocated since channel[1].gamma had not been initialized to zero.

So, BOTH chanel[0].gamma and channel[1].gamma must be initialized to zero before memory gets allocated.

I'm curious why this isn't a issue for more people. I'm linking to libws2811.a using gcc in QtCreator, cross compiling from Ubuntu running in a vm session on windows.

Structures don't default to zero in my environment. Maybe, there's a compiler flag in the Scons stuff for the test executable? (I don't like that I can't see the compiler and linker invocations during build and haven't figured out.)

Having said that, thanks for providing this! I'm ready to give it a try on my 360 LED light tube.

[penfold42]

Does #219 fix this ?

[BruceMardle]

I haven't looked at the source so this is a bit vague, but C compilers only have to initialise variables to 0, which aren't explicity initialised, if they're allocated statically (i.e. either global to all functions or in a function with 'static' in the declaration. 'static' in a global declaration means something else [rolls eyes]).A MacOS fan once boasted to me that the results of malloc were always zero'd, but that's not part of the C standard. Maybe the people not getting SIGSEGVs had 0 in the offending variables by luck.I'm always unimpressed when a developer responds to a bug report with "It works on my machine" :-/ Not saying that's the case here. From: davthomaspilot <[email protected]> To: jgarff/rpi_ws281x <[email protected]> Cc: Subscribed <[email protected]> Sent: Friday, 3 August 2018, 2:16 Subject: [jgarff/rpi_ws281x] Additional members of ws2811_t structure need initialization (#310) I was getting segfaults until I initialized:ledstring.channel[0].gamma =0Maybe some compilers initialize structures to zero, but it didn't in my environment. If it's non-zero, the gamma array never gets allocated, and sigsegv occurs on first attempt to index into it.Similarly, until I set ledstring.render_wait_time=0, bad things occur.But, I have no idea what render_wait_time is for--it would be nice to know what all members of the structure do.— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[davthomaspilot]

Bruce,

[quote]
Maybe the people not getting SIGSEGVs had 0 in the offending variables by luck.I'm always unimpressed when a developer responds to a bug report with "It works on my machine" :-/ Not saying that's the case here.
[/quote/
The main.c is the example source. It doesn't initialize all the members of the structure that need it. The structure is tatic.

I think we agree... don't think one should ever count on structures (or memory allocated on the heap) being all zero. I was just following the example in main.c as suggested in the README. I know of no other documentation.

[davthomaspilot]

Does #219 fix this ?

The issue is in the example main.c. Issue #219 seems to suggest using calloc somewhere instead of malloc. But, the problem is that a member of the structure passed for initialization has to be zero for memory to be allocated, If not, no memory and writes are done using a wild pointer.

[penfold42]

@Gadgetoid
Is there a cleaner way to catch this ?
Or do we document the requirement ?

[davthomaspilot]

The initialization of ledstring in main.c looks like this:

ws2811_t ledstring =
{
    .freq = TARGET_FREQ,
    .dmanum = DMA,
    .channel =
    {
        [0] =
        {
            .gpionum = GPIO_PIN,
            .count = LED_COUNT,
            .invert = 0,
            .brightness = 255,
            .strip_type = STRIP_TYPE,
        },
        [1] =
        {
            .gpionum = 0,
            .count = 0,
            .invert = 0,
            .brightness = 0,
        },
    },
};

It should have at least these two additional lines:

ledstring.channel[0].gamma = 0;
ledstring.channel[1].gamma = 0;

Maybe "gamma" was added to the library code to implement a new feature which hasn't been documented nor exploited in main.c?

Also, render_wait_time needs to be initialized:

ledstring.render_wait_time = 0;

Maybe not to zero, but what should it be? 0 seems to work, but maybe I should set it to something other than zero to make sure kernel doesn't have problem with too much dma activity or to avoid some other limitation?

When render_wait_time was not initialized, its random contents was a very large number and the code appeared to just hang.

Here's the ws2811_channel_t structure (from ws2811.h):

typedef struct
{
    int gpionum;                                 //< GPIO Pin with PWM alternate function, 0 if unused
    int invert;                                  //< Invert output signal
    int count;                                   //< Number of LEDs, 0 if channel is unused
    int strip_type;                              //< Strip color layout -- one of WS2811_STRIP_xxx constants
    ws2811_led_t *leds;                          //< LED buffers, allocated by driver based on count
    uint8_t brightness;                          //< Brightness value between 0 and 255
    uint8_t wshift;                              //< White shift value
    uint8_t rshift;                              //< Red shift value
    uint8_t gshift;                              //< Green shift value
    uint8_t bshift;                              //< Blue shift value
    uint8_t *gamma;                              //< Gamma correction table
} ws2811_channel_t

So, what about those other member structures that aren't used by the main.c example? Brightness? If its less than 255 are all the LED brightness values scaled? rshift, gshift, bshift.... Can they used for color balance?

I'm going to explore the code to answer these questions, unless someone can post them or refer to some documentation.

[penfold42]

I can speak for the shifts - treat them as private. They’re used internally to calculate how to move the colours around based on the strip type. Whatever you put in will be overwritten and if you subsequently changed them “weird stuff” will happen to your colours

[davthomaspilot]

"move colors around based on the strip type..."

I think the integers in ws2811_led_t array (ledstring.channel[0].leds in the main.c example) represent the relative duty cycle of each led. 0xff = 100% duty, 0x10 = 16/255 duty, etc.

Versus "brightness", since apparent brightness of the led may not be directly proportional to duty cycle. Does the code somehow attempt to calculate a duty cycle based on the value in the leds element, or is it just the duty cycle as I've been assuming?

"move colors around based on the strip type..."
Or maybe...

I see this in ws2811.h:

#define SK6812_STRIP_RGBW                        0x18100800
#define SK6812_STRIP_RBGW                        0x18100008
#define SK6812_STRIP_GRBW                        0x18081000
#define SK6812_STRIP_GBRW                        0x18080010
#define SK6812_STRIP_BRGW                        0x18001008
#define SK6812_STRIP_BGRW                        0x18000810
#define SK6812_SHIFT_WMASK                       0xf0000000

// 3 color R, G and B ordering
#define WS2811_STRIP_RGB                         0x00100800
#define WS2811_STRIP_RBG                         0x00100008
#define WS2811_STRIP_GRB                         0x00081000
#define WS2811_STRIP_GBR                         0x00080010
#define WS2811_STRIP_BRG                         0x00001008
#define WS2811_STRIP_BGR                         0x00000810

// predefined fixed LED types
#define WS2812_STRIP                             WS2811_STRIP_GRB
#define SK6812_STRIP                             WS2811_STRIP_GRB
#define SK6812W_STRIP                            SK6812_STRIP_GRBW

I've been assuming I need to order the bytes in each element of the ws2811_led_t array based on my strip color ordering (RGB versus GRB, etc). Or maybe I order always as something like WRGB and the code "moves colors around"?

Just trying to understand your comment...

[penfold42]

The point of the shifting is to allow someone to write code with a common byte ordering of R, G, B and then let the library worry about how to shift them around based on the strip type you select.

In the days when led strips used ws2811 chips that were separate from
The actual leds, different manufacturers used whatever color order they liked so this made adapting code much easier.

It’s much less of s problem these days with ws2812 and other strips that have integrated the leds and the chip and have a fixed colour order

[davthomaspilot]

Ok, makes sense.

Thanks

[Gadgetoid]

I believe the read on this issue above is correct- gamma correction was added as a feature (by me), but there was no corresponding documentation or example update to properly demonstrate its use.

.gamma should be supplied either as 0 (or NULL) or as an array of 256 unsigned bytes representing a table that remaps brightness values.

@davthomaspilot did you do any further exploration? I'm keen to get this fixed in both documentation/examples but I'm also unsure of some of the other struct members, and also note your point with ledstring.render_wait_time. However I think 0 is appropriate in this case, since the render_wait_time is calculated after every call to ws2811_render from the protocol_time plus the LED_RESET_WAIT_TIME here:

rpi_ws281x/ws2811.c

Line 1250 in e4a05d6

ws2811->render_wait_time = protocol_time + LED_RESET_WAIT_TIME;

[davthomaspilot]

Sorry, I haven't checked back on this sooner.

So, I'm still not clear on what can be done with ledstring.render_wait_time.

If I didn't set it to zero, the code would hang. So, I've been setting it to zero.

However, I can crash if I doing consecutive "hardware_render() commands too fast. So, I was thinking maybe that render_wait_time could somehow be used to figure out when it safe to start another hardware_render?

Or, is there some other way I can protect from doing them too fast? And, how to calculate how fast is too fast?

[davthomaspilot]

Here's the code fragment that lead me to speculate about render_wait_time:

if (ws2811->render_wait_time != 0) {
    const uint64_t current_timestamp = get_microsecond_timestamp();
    uint64_t time_diff = current_timestamp - previous_timestamp;

    if (ws2811->render_wait_time > time_diff) {
        usleep(ws2811->render_wait_time - time_diff);
    }
}

So, it seems the code sleeps if the write is faster than specified in render_wait_time? Why would one do that?