Skip to content

drivers: led_strip: ws2812_i2s: add full pre reset if active low #89203

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
wants to merge 1 commit into
base: main
Choose a base branch
from
+9 −3
Open

drivers: led_strip: ws2812_i2s: add full pre reset if active low #89203

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 9 additions & 3 deletions drivers/led_strip/ws2812_i2s.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ LOG_MODULE_REGISTER(ws2812_i2s);
#include <zephyr/kernel.h>
#include <zephyr/sys/util.h>

#define WS2812_I2S_PRE_DELAY_WORDS 1
#define WS2812_I2S_PRE_DELAY_WORDS_DEFAULT 1

struct ws2812_i2s_cfg {
struct device const *dev;
Expand Down Expand Up @@ -69,6 +69,8 @@ static int ws2812_strip_update_rgb(const struct device *dev, struct led_rgb *pix
const uint8_t sym_one = cfg->nibble_one;
const uint8_t sym_zero = cfg->nibble_zero;
const uint32_t reset_word = cfg->active_low ? ~0 : 0;
const uint16_t pre_delay_words =
cfg->active_low ? cfg->reset_words : WS2812_I2S_PRE_DELAY_WORDS_DEFAULT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that having a signal active at idle state can also happen with a signal active high.

Copy link
Author

@jeppenodgaard jeppenodgaard May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

True. It should probably have its own property (unless it can be resolved at a lower level as you suggest).

uint32_t *tx_buf;
uint32_t flush_time_us;
void *mem_block;
Expand All @@ -83,7 +85,7 @@ static int ws2812_strip_update_rgb(const struct device *dev, struct led_rgb *pix
tx_buf = (uint32_t *)mem_block;

/* Add a pre-data reset, so the first pixel isn't skipped by the strip. */
for (uint16_t i = 0; i < WS2812_I2S_PRE_DELAY_WORDS; i++) {
for (uint16_t i = 0; i < pre_delay_words; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So if the signal is active low, we are waiting for a reset delay before AND after sending data ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. Otherwise the first LED is misbehaving.

*tx_buf = reset_word;
tx_buf++;
}
Expand Down Expand Up @@ -218,9 +220,13 @@ static DEVICE_API(led_strip, ws2812_i2s_api) = {

#define WS2812_I2S_NUM_PIXELS(idx) (DT_INST_PROP(idx, chain_length))

#define WS2812_I2S_PRE_DELAY_WORDS(idx) \
COND_CODE_1(DT_INST_PROP(idx, out_active_low), (WS2812_RESET_DELAY_WORDS(idx)), \
(WS2812_I2S_PRE_DELAY_WORDS_DEFAULT))

#define WS2812_I2S_BUFSIZE(idx) \
(((WS2812_NUM_COLORS(idx) * WS2812_I2S_NUM_PIXELS(idx)) + \
WS2812_I2S_PRE_DELAY_WORDS + WS2812_RESET_DELAY_WORDS(idx)) * 4)
WS2812_I2S_PRE_DELAY_WORDS(idx) + WS2812_RESET_DELAY_WORDS(idx)) * 4)

#define WS2812_I2S_DEVICE(idx) \
\
Expand Down