-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
drivers/led: add LED_NUMOF and convenience inline functions #20783
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition.
drivers/led: add LED_NUMOF and convenience inline functions
0ca0ab5
to
5403ba5
Compare
@@ -20,6 +20,7 @@ | |||
#define BOARD_H | |||
|
|||
#include "cpu.h" | |||
#include "periph/gpio.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add this to led.h
instead to avoid changing all board definitions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we could, but if we want to follow the include-what-you-use paradigm (see #20570), it doesn't make sense to add it to led.h
. That file does not use the content of periph/gpio.h
, while the board.h
files indeed do (e.g., GPIO_PIN
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, running make -C examples/hello-world BOARD=adafruit-grand-central-m4-express all
with the following diff also fails on master
due to the missing include in board.h
, completely independent of led.h
:
diff --git a/examples/hello-world/main.c b/examples/hello-world/main.c
index 213128ac64..a34fcc00bf 100644
--- a/examples/hello-world/main.c
+++ b/examples/hello-world/main.c
@@ -21,6 +21,8 @@
#include <stdio.h>
+#include "board.h"
+
int main(void)
{
puts("Hello World!");
@@ -28,5 +30,7 @@ int main(void)
printf("You are running RIOT on a(n) %s board.\n", RIOT_BOARD);
printf("This board features a(n) %s CPU.\n", RIOT_CPU);
+ printf("board.h defined PIN: %d\n", WS281X_PARAM_PIN);
+
return 0;
}
This now only gets apparent because the macros are used in the inline functions.
But anyway, I feel like the board.h
include structure could benefit from a bigger overhaul in any case, so I'm not too strictly opposed to put the include in led.h
instead for now. What do you think?
5403ba5
to
bb97445
Compare
Contribution description
While reviewing #20782, I found some expected convenience functions for simple LED usage to be missing from "led.h". This PR adds the following:
LED_NUMOF
that provides the number of available LEDs at compile-timeled_{on,off,toggle}
with the LED id as run-time argument, mirroring theLED_{ON,OFF,TOGGLE}
compile-time macrosAlso adapts
tests/leds
to (partly) use the newly provided functionality.Testing procedure
make -C tests/leds
should still work with the board of your choice.Issues/PRs references
Could enable an easier application code for examples like #20782