Fix rapid changes to colored_status_led#2938
Conversation
…. prior to this fix, setting color/on/off in rapid succession would just send the updated values "down the string" Whilst it would be nice to have the PIO program take care of updating the latest FIFO value after the relevant delay, that costs a fair amount of PIO instrution space, so I have decided to "fix" the issue on the CPU side. Since the delays in question are non trivial (e.g. 50us), i've decided not to have the call block, but instead set a timer when necessary (using the default alarm pool) - if that isn't available or the alarm add fails, then the caller blocks instead. Note the implementation uses a spin lock, because it needs to work with multicore, but also to be fair you might expect to be able to set an LED from an IRQ
| #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 |
There was a problem hiding this comment.
| // 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 |
| 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 |
There was a problem hiding this comment.
| 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 |
| 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; |
There was a problem hiding this comment.
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?
|
|
||
| // 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 |
There was a problem hiding this comment.
This define doesn't seem to actually be used anywhere??
There was a problem hiding this comment.
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
| } | ||
| } | ||
|
|
||
| static int64_t deferred_set_ws2812(__unused alarm_id_t id, __unused void *user_data) { |
There was a problem hiding this comment.
Looks like this function could also be wrapped by #if PICO_COLORED_STATUS_LED_USE_DEFAULT_ALARM_POOL ?
|
I guess the PR addresses this comment 🙂 |
| #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) |
There was a problem hiding this comment.
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();
}3 participants
WS2812 style LEDs require a reset time before setting the color againprior to this fix, setting color/on/off
in rapid succession would just send the updated values "down the string"
Whilst it would be nice to have the PIO program take care of updating the latest FIFO value after the relevant delay, that costs a fair amount of PIO instrution space, so I have decided to "fix" the issue on the CPU side.
Since the delays in question are non trivial (e.g. 50us), i've decided not to have the call block, but instead set a timer when necessary (using the default alarm pool) - if that isn't available or the alarm add fails, then the caller blocks instead.
Note the implementation uses a spin lock, because it needs to work with multicore, but also to be fair you might expect to be able to set an LED from an IRQ