drivers/ws281x: Add saul support #20562

MrKevinWeiss · 2024-04-09T13:44:59Z

Contribution description

Support the ws281x driver on saul... starting with 🥁 the feather-nrf52840-sense .

Note that this only allows control of all LEDs at once since saul does not really have a way to handle subindex or array control (for example LED array). For our use case we would like to keep it simple now but if we did want to allow saul to control individual LEDs in the future here are some considerations:

saul_entries would be of size WS281X_NUM * WS281X_PARAM_NUMOF and one could iterate over this matrix, though there would still be no mechanism to address each one...
The ws281x_t should change to be for each LED and not an array (likely a bit of rework) but then each dev could be for each LED
Add a mechanism in saul to not only interact with an object, but an array of objects.

All of this is out of scope of this PR, but if people need individual control with saul, these issues must be considered.

Testing procedure

Play around with:

BOARD=feather-nrf52840-sense make -C examples/saul flash term

Issues/PRs references

MrKevinWeiss · 2024-04-09T13:45:41Z

I haven't tested yet but will do it tomorrow then make it a real PR

drivers/ws281x/ws281x_saul.c

MrKevinWeiss · 2024-04-10T12:24:43Z

There were a few bumps along the way... Since we have a whole bunch of backends in submodules we could not use the driver_with_saul.mk so I just copied an pasted the interesting bits (otherwise it would bring in all the backends).

Also, it seems there were some issues with the dep modelling, since xtimer is used in the header it should be mandatory for all backends... I didn't see this until I added the ws281x_write() that @benpicco suggested. Now I actually can see the colors.

I also accounted for the amount of LEDs a bit better... SAUL will only apply color to all LEDs and individual LEDs are not exposed.

maribu

Thx :)

IMHO this is not scary and could be merged during soft feature freeze. But let's give @teufelchen a chance to veto.

riot-ci · 2024-04-10T13:39:31Z

Murdock results

✔️ PASSED

b8ddac7 rust: Selectively update riot-wrappers dependency

Success	Failures	Total	Runtime
10045	0	10045	14m:51s

Artifacts

Documentation preview

Teufelchen1

Approve, just a nit pick.
Oh and murdock spotted a little type issue!

drivers/saul/init_devs/auto_init_ws281x.c

MrKevinWeiss · 2024-04-11T09:20:34Z

IMHO this is not scary and could be merged during soft feature freeze. But let's give @teufelchen a chance to veto.

I also don't mind waiting until the head freeze... Not so critical, just nice.

maribu · 2024-04-11T09:21:47Z

Please squash at will.

miri64 · 2024-04-15T12:13:01Z

drivers/ws281x/ws281x_saul.c

+    };
+    for (unsigned idx = 0; idx < ws281x->params.numof; idx++) {
+        puts("Setting LED");
+        ws281x_set(ws281x, idx, color);


Just from a perspective of practicality: Would it make sense to set the idx as an additional vector component (or address each LED as their own SAUL device)? It seems to me very restrictive that only one color can be set for a whole LED strip using SAUL.

I think an extra vector component would be weird; strand-as-a-whole is at least consistent with the data types and write mode.

A per-LED mode would make sense, possibly also a group-of-LEDs mode – but maybe that can grow from this as a starting point?

[…] but maybe that can grow from this as a starting point?

Fine by me, just wanted to give a possible discussion starter.

Maybe we should even distinguish from a single RGB LED and a LED strip in SAUL type. In any way it should be clarified (in the documentation) and unify that at the moment SAUL_ACT_LED_RGB sets just everything in the SAUL device to one color. Currently, it seems to be used for PWM-based RGB LEDs (where it is used to set the color of a single LED associated to a single PWM device) and by the Grove LED bar, that currently just seems to set the red compontent.

RIOT/drivers/saul/pwm_saul.c

Lines 102 to 123 in 680c5aa

static int write_rgb(const void *dev, const phydat_t *state)

{

const saul_pwm_rgb_params_t *p = dev;

int factor, shiftback, max;

int err = extract_scaling(state, &factor, &shiftback, &max);

if (err < 0) {

return err;

}

for (int i = 0; i < 3; ++i) {

if (state->val[i] < 0 || state->val[i] > max) {

return -ECANCELED;

}

}

for (int i = 0; i < 3; ++i) {

setchan(&p->channels[i], (state->val[i] * factor) >> shiftback);

}

return 3;

}

RIOT/drivers/grove_ledbar/grove_ledbar_saul.c

Lines 27 to 32 in 680c5aa

static int set_ledbar(const void *dev, const phydat_t *res)

{

uint8_t lvl = (uint8_t)res->val[0];

grove_ledbar_set((grove_ledbar_t *)dev, lvl);

return 1;

}

And a good starter it is indeed.

I don't think the SAUL type is the right place, [edit, added:] if visible the distinction belongs in the descriptive text. Single LED, LED strip or group of LEDs are just what RIOT sees, and may not correspond to the physical reality. (For example, on a PWM port there could be a strip of non-addressable LEDs, or all the many LEDs of a WS281x may be hidden behind a larger diffusor).

It is weird that in the Grove bar it uses one component -- anything monochromatic should be SAUL_ACT_DIMMER (and read one channel), and anything RGBish should be SAUL_ACT_LED_RGB and read the 3 components. (And I do not yet know how to best work with RGBW or RGBA LEDs in SAUL, but one step at a time).

Can we say this is for a different PR? This issues is already documented in the PR description.

I think yes.

chrysn · 2024-04-15T14:06:35Z

Build tests are blocked by the previously 3-year-dormant ws281x module coming back to life, as in this build failure -- apparently there was one of the non-inline-to-inline API changes that are nonbreaking for C but breaking for Rust. Looking into a quick fix.

This pulls in [88], and thus fixes building of Rust modules when the wS281x module is enabled. [88]: RIOT-OS/rust-riot-wrappers#88

chrysn · 2024-04-15T14:29:44Z

I have pushed a commit that updates the riot-wrappers version to include RIOT-OS/rust-riot-wrappers#88 which fixes building when combining WS281x and Rust.

github-actions bot added Area: drivers Area: Device drivers Area: boards Area: Board ports labels Apr 9, 2024

benpicco reviewed Apr 9, 2024

View reviewed changes

drivers/ws281x/ws281x_saul.c Show resolved Hide resolved

MrKevinWeiss force-pushed the pr/saulrgb branch from 247dddb to 87f75a5 Compare April 10, 2024 12:20

github-actions bot added the Area: SAUL Area: Sensor/Actuator Uber Layer label Apr 10, 2024

MrKevinWeiss marked this pull request as ready for review April 10, 2024 12:21

MrKevinWeiss requested a review from maribu as a code owner April 10, 2024 12:21

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 10, 2024

maribu approved these changes Apr 10, 2024

View reviewed changes

Teufelchen1 approved these changes Apr 10, 2024

View reviewed changes

drivers/saul/init_devs/auto_init_ws281x.c Show resolved Hide resolved

MrKevinWeiss added 2 commits April 11, 2024 11:43

drivers/ws281x: Add saul support

0a96e36

boards/feather-nrf52840-sense: Expose saul ws281x

8a15b8c

MrKevinWeiss force-pushed the pr/saulrgb branch from 361455a to 8a15b8c Compare April 11, 2024 09:43

MrKevinWeiss enabled auto-merge April 11, 2024 09:44

MrKevinWeiss added this pull request to the merge queue Apr 11, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 11, 2024

miri64 reviewed Apr 15, 2024

View reviewed changes

chrysn mentioned this pull request Apr 15, 2024

ws281x: Use static inline function through inline namespace RIOT-OS/rust-riot-wrappers#88

Merged

rust: Selectively update riot-wrappers dependency

b8ddac7

This pulls in [88], and thus fixes building of Rust modules when the wS281x module is enabled. [88]: RIOT-OS/rust-riot-wrappers#88

MrKevinWeiss requested review from leandrolanzieri, aabadie and MichelRottleuthner as code owners April 15, 2024 14:28

github-actions bot added Area: tests Area: tests and testing framework Area: sys Area: System labels Apr 15, 2024

github-actions bot added the Area: examples Area: Example Applications label Apr 15, 2024

maribu added this pull request to the merge queue Apr 17, 2024

Merged via the queue into RIOT-OS:master with commit fc271ea Apr 17, 2024
28 checks passed

MrKevinWeiss deleted the pr/saulrgb branch May 13, 2024 15:22

mguetschow added this to the Release 2024.07 milestone Jun 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/ws281x: Add saul support #20562

drivers/ws281x: Add saul support #20562

MrKevinWeiss commented Apr 9, 2024 •

edited

Loading

MrKevinWeiss commented Apr 9, 2024

MrKevinWeiss commented Apr 10, 2024

maribu left a comment

riot-ci commented Apr 10, 2024 •

edited

Loading

Teufelchen1 left a comment •

edited

Loading

MrKevinWeiss commented Apr 11, 2024

maribu commented Apr 11, 2024

miri64 Apr 15, 2024

chrysn Apr 15, 2024

miri64 Apr 15, 2024 •

edited

Loading

chrysn Apr 15, 2024 •

edited

Loading

MrKevinWeiss Apr 17, 2024

chrysn Apr 17, 2024

miri64 Apr 17, 2024

chrysn commented Apr 15, 2024

chrysn commented Apr 15, 2024

	static int write_rgb(const void dev, const phydat_t state)
	{
	const saul_pwm_rgb_params_t *p = dev;

	int factor, shiftback, max;

	int err = extract_scaling(state, &factor, &shiftback, &max);
	if (err < 0) {
	return err;
	}

	for (int i = 0; i < 3; ++i) {
	if (state->val[i] < 0 \|\| state->val[i] > max) {
	return -ECANCELED;
	}
	}

	for (int i = 0; i < 3; ++i) {
	setchan(&p->channels[i], (state->val[i] * factor) >> shiftback);
	}
	return 3;
	}

	static int set_ledbar(const void dev, const phydat_t res)
	{
	uint8_t lvl = (uint8_t)res->val[0];
	grove_ledbar_set((grove_ledbar_t *)dev, lvl);
	return 1;
	}

drivers/ws281x: Add saul support #20562

drivers/ws281x: Add saul support #20562

Conversation

MrKevinWeiss commented Apr 9, 2024 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

MrKevinWeiss commented Apr 9, 2024

MrKevinWeiss commented Apr 10, 2024

maribu left a comment

Choose a reason for hiding this comment

riot-ci commented Apr 10, 2024 • edited Loading

Murdock results

Artifacts

Teufelchen1 left a comment • edited Loading

Choose a reason for hiding this comment

MrKevinWeiss commented Apr 11, 2024

maribu commented Apr 11, 2024

miri64 Apr 15, 2024

Choose a reason for hiding this comment

chrysn Apr 15, 2024

Choose a reason for hiding this comment

miri64 Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

chrysn Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

MrKevinWeiss Apr 17, 2024

Choose a reason for hiding this comment

chrysn Apr 17, 2024

Choose a reason for hiding this comment

miri64 Apr 17, 2024

Choose a reason for hiding this comment

chrysn commented Apr 15, 2024

chrysn commented Apr 15, 2024

MrKevinWeiss commented Apr 9, 2024 •

edited

Loading

riot-ci commented Apr 10, 2024 •

edited

Loading

Teufelchen1 left a comment •

edited

Loading

miri64 Apr 15, 2024 •

edited

Loading

chrysn Apr 15, 2024 •

edited

Loading