Skip to content

Fix rapid changes to colored_status_led #2938

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.

Sign up for GitHub

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

Open
kilograham wants to merge 2 commits into
base: develop
Choose a base branch
from
+76 −3
Open

Fix rapid changes to colored_status_led #2938

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15ab7ad
WS2812 style LEDs require a reset time before setting the color again…
kilograham May 14, 2026
2c4250f
PICO_CONFIG fix
kilograham May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 76 additions & 3 deletions src/rp2_common/pico_status_led/status_led.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

#include "pico/status_led.h"

#include "hardware/sync/spin_lock.h"

#if PICO_STATUS_LED_AVAILABLE && defined(CYW43_WL_GPIO_LED_PIN) && !defined(PICO_DEFAULT_LED_PIN)
#define STATUS_LED_USING_WL_GPIO 1
#else
Expand Down Expand Up @@ -33,35 +35,101 @@ static uint32_t colored_status_led_on_color = PICO_DEFAULT_COLORED_STATUS_LED_ON
static bool colored_status_led_on;

#if COLORED_STATUS_LED_USING_WS2812_PIO
#include <hardware/pio.h>
#include "hardware/pio.h"
#include "pico/time.h"
#include "ws2812.pio.h"

// PICO_CONFIG: PICO_COLORED_STATUS_LED_WS2812_FREQ, Frequency per bit for the WS2812 colored status LED, type=int, default=800000, group=pico_status_led
#ifndef PICO_COLORED_STATUS_LED_WS2812_FREQ
#define PICO_COLORED_STATUS_LED_WS2812_FREQ 800000
#endif

// PICO_CONFIG: PICO_COLORED_STATUS_LED_RESET_DELAY_US, Required reset delay in microseconds for the WS2812 colored status LED , type=int, default=50, group=pico_status_led
Copy link
Copy Markdown
Contributor

@lurch lurch May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// PICO_CONFIG: PICO_COLORED_STATUS_LED_RESET_DELAY_US, Required reset delay in microseconds for the WS2812 colored status LED , type=int, default=50, group=pico_status_led
// PICO_CONFIG: PICO_COLORED_STATUS_LED_RESET_DELAY_US, Required reset delay in microseconds for the WS2812 colored status LED, type=int, default=50, group=pico_status_led

#ifndef PICO_COLORED_STATUS_LED_RESET_DELAY_US
#define PICO_COLORED_STATUS_LED_RESET_DELAY_US 50
Copy link
Copy Markdown
Contributor

@lurch lurch May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This define doesn't seem to actually be used anywhere??

Copy link
Copy Markdown
Contributor

@will-v-pi will-v-pi May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the calculated MINIMUM_WS2812_DELAY_US seems to be used instead.

If it wants to be configurable, could use #define PICO_COLORED_STATUS_LED_RESET_CYCLES which defaults to PICO_COLORED_STATUS_LED_USES_WRGB ? 32 : 24, and then use PICO_COLORED_STATUS_LED_RESET_CYCLES in the MINIMUM_WS2812_DELAY_US calculation?

Equally, could just remove this define, if the value is expected to be fixed

#endif

#ifndef PICO_COLORED_STATUS_LED_USE_DEFAULT_ALARM_POOL
#define PICO_COLORED_STATUS_LED_USE_DEFAULT_ALARM_POOL !PICO_TIME_DEFAULT_ALARM_POOL_DISABLED
#endif

static PIO pio;
static uint sm;
static uint offset;
static uint32_t next_value;
static uint64_t next_safe_set_time;
#if PICO_COLORED_STATUS_LED_USE_DEFAULT_ALARM_POOL
static alarm_id_t alarm_id;
static int8_t alarm_pending;
#else
#define alarm_id 0
#endif

#define MINIMUM_WS2812_DELAY_US (1 + (1000000 * (PICO_COLORED_STATUS_LED_USES_WRGB ? 32 : 24)) / PICO_COLORED_STATUS_LED_WS2812_FREQ)
Copy link
Copy Markdown
Contributor

@will-v-pi will-v-pi May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the boards I have on my desk require 3x this delay value to work correctly - where did this value come from?

My test is just a modified color_blink example to send colours as often as possible - when working it is white (ie cycling through red-green-blue), when not working it is just red (the first colour set)

#include "pico/stdlib.h"
#include "pico/status_led.h"

int main() {
    status_led_init();
    uint32_t count = 0;
    while (true) {
        // flash red then green then blue
        uint32_t color = PICO_COLORED_STATUS_LED_COLOR_FROM_RGB(count % 3 == 0 ? 0xaa : 0, count % 3 == 1 ? 0xaa : 0, count % 3 == 2 ? 0xaa : 0);
        colored_status_led_set_on_with_color(color);
        count++;
    }
    status_led_deinit();
}


// Extract from 0xWWRRGGBB
#define RED(c) (((c) >> 16) & 0xff)
#define GREEN(c) (((c) >> 8) & 0xff)
#define BLUE(c) (((c) >> 0) & 0xff)
#define WHITE(c) (((c) >> 24) && 0xff)

bool set_ws2812(uint32_t value) {
static void unsafe_set_ws2812(uint32_t value, uint64_t now) {
if (pio) {
pio_sm_drain_tx_fifo(pio, sm); // want to jump passed any previous queued values
Copy link
Copy Markdown
Contributor

@lurch lurch May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pio_sm_drain_tx_fifo(pio, sm); // want to jump passed any previous queued values
pio_sm_drain_tx_fifo(pio, sm); // want to jump past any previous queued values

#if PICO_COLORED_STATUS_LED_USES_WRGB
// Convert to 0xWWGGRRBB
pio_sm_put_blocking(pio, sm, WHITE(value) << 24 | GREEN(value) << 16 | RED(value) << 8 | BLUE(value));
#else
// Convert to 0xGGRRBB00
pio_sm_put_blocking(pio, sm, GREEN(value) << 24 | RED(value) << 16 | BLUE(value) << 8);
#endif
return true;
next_safe_set_time = now + MINIMUM_WS2812_DELAY_US;
}
}

static int64_t deferred_set_ws2812(__unused alarm_id_t id, __unused void *user_data) {
Copy link
Copy Markdown
Contributor

@lurch lurch May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this function could also be wrapped by #if PICO_COLORED_STATUS_LED_USE_DEFAULT_ALARM_POOL ?

spin_lock_t *spin_lock = spin_lock_instance(PICO_SPINLOCK_ID_ATOMIC);
uint32_t save = spin_lock_blocking(spin_lock);
unsafe_set_ws2812(next_value, time_us_64());
alarm_id = 0;
alarm_pending--;
spin_unlock(spin_lock, save);
return 0;
}

static bool set_ws2812(uint32_t value) {
spin_lock_t *spin_lock = spin_lock_instance(PICO_SPINLOCK_ID_ATOMIC);
uint32_t save = spin_lock_blocking(spin_lock);
next_value = value;
while (true) {
if (alarm_pending) {
// we defer the set to the already waiting alarm
break;
} else {
uint64_t now = time_us_64();
if (now >= next_safe_set_time) {
unsafe_set_ws2812(value, now);
break;
} else {
// we want to defer the set until it is safe to do so
//
// note we use alarm_pending separate from alarm_id, as alarm_id may be returned even if the
// alarm fires during the add_alarm_at. and don't use a boolean because if we fail
// to add the alarm, we don't know what has happened in between since we unlock the spin lock
// before adding the alarm since that is a slowish call
alarm_pending++;
spin_unlock(spin_lock, save);
#if PICO_COLORED_STATUS_LED_USE_DEFAULT_ALARM_POOL
alarm_id = add_alarm_at(next_safe_set_time, deferred_set_ws2812, NULL, true);
if (alarm_id > 0) break;
Copy link
Copy Markdown
Contributor

@lurch lurch May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the break here means that there's a code-path where the spin_lock_blocking on line 127 won't get hit. Does that matter?

#endif
busy_wait_until(next_safe_set_time);
save = spin_lock_blocking(spin_lock);
alarm_pending--;
}
}
}
spin_unlock(spin_lock, save);
return false;
}
#endif
Expand Down Expand Up @@ -165,6 +233,11 @@ void status_led_deinit(void) {
status_led_context = NULL;
#endif
#if COLORED_STATUS_LED_USING_WS2812_PIO
#if PICO_COLORED_STATUS_LED_USE_DEFAULT_ALARM_POOL
if (alarm_id > 0) {
cancel_alarm(alarm_id);
}
#endif
if (pio) {
pio_remove_program_and_unclaim_sm(&ws2812_program, pio, sm, offset);
pio = NULL;
Expand Down
Loading