Skip to content
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: Allow LEDn_ON to be disabled by other modules #20833

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Aug 25, 2024

Contribution description

On the particle-xenon and similar boards, once SAUL is active, LED[012]_ON will not do anything, even though LED[012]_IS_PROVIDED is set. This may not surprise users of LEDn_ON (because it's the established behavior of those to no-op), but when explicitly inspecting LEDn_IS_PROVIDED (like Rust users will do), this behavior is confusing.

The cause of this is that SAUL's use of the LED pins is through PWM, and (at least on that family) that completely overrides GPIO input.

The change is two-fold:

  • On the particle-mesh family, the condition for when something else grabs the LED pins is written out as !(IS_ACTIVE(SAUL_AUTO_INIT) && IS_ACTIVE(SAUL_PWM)). If that is the case, LED0_ON is not defined, and consequently, led.h does not set LED0_IS_PRESENT.
  • The setup in init_leds.c checks for whether LEDn_PIN is set, and then sets up the GPIO and runs LEDn_OFF. That criterion is no longer accurate, and is changed to LEDn_IS_PRESENT. (Funnily, this required us to include led.h, and by the time that happened, LEDn_OFF is defined as a no-op -- but still going for the LEDn_IS_PRESENT condition because when something else uses the pin, the LED module should keep its hands off the pins).

Testing procedure

  • Run leds_shell -- and everything is as it always was.
  • USEMODULE+=saul_default and then run leds_shell -- and the LEDs are not shown any more.

Alternatives

  • LEDn_ON could be altered in the PWM case to set it to 100%. I have not chosen this path because the expectation with the LEDn functions is that they are very very fast.
  • Rather than doing the change in init_leds.c to LED0_IS_PRESENT, we could extend the is-used-by-SAUL guard to also disable LED0_PIN. This was not done because LED0_PIN is also used at the definition of the PWM channels. We could still do that, and either spell out the pins in the PWM setup, or use a new define (but it's in board.h and thus widely seen) for the channel pins. (That definition would then be used both to conditionally define LEDn_PIN and to set up the PWM).

@chrysn chrysn requested a review from benpicco August 25, 2024 12:18
@github-actions github-actions bot added Area: drivers Area: Device drivers Area: boards Area: Board ports labels Aug 25, 2024
@chrysn chrysn force-pushed the leds-provided-on-demand branch from 4813afc to 4fe4a0e Compare August 25, 2024 12:19
@chrysn
Copy link
Member Author

chrysn commented Aug 25, 2024

Test output for USEMODULE+="stdio_rtt usbus_cdc_ecm" make BOARD=particle-xenon all flash term PROGRAMMER=openocd OPENOCD_DEBUG_ADAPTER=stlink -j12:

2024-08-25 14:20:09,515 # main(): This is RIOT! (Version: 2024.10-devel-103-g4fe4a-leds-provided-on-demand)
2024-08-25 14:20:09,517 # This board has 3 LEDs
> led0 on
2024-08-25 14:20:10,625 # led 0 on
>

And with USEMODULE+="saul saul_default stdio_rtt usbus_cdc_ecm" make BOARD=particle-xenon all flash term PROGRAMMER=openocd OPENOCD_DEBUG_ADAPTER=stlink -j12:

2024-08-25 14:44:11,706 # main(): This is RIOT! (Version: 2024.10-devel-103-g4fe4a-leds-provided-on-demand)
2024-08-25 14:44:11,707 # This board has 0 LEDs
> 

(And by the way we're doing way too much with preprocessor macros, which have a terrible user experience in terms of typos and other errors -- I've gone through all kinds of how one could do things wrong in the ifdef, and never has any tool complained.)

Marking this as ready-for-review and to build.

@chrysn chrysn force-pushed the leds-provided-on-demand branch from 4fe4a0e to 94745c4 Compare August 25, 2024 12:48
@chrysn chrysn marked this pull request as ready for review August 25, 2024 12:49
@chrysn chrysn requested a review from MrKevinWeiss as a code owner August 25, 2024 12:49
@chrysn chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 25, 2024
@riot-ci
Copy link

riot-ci commented Aug 25, 2024

Murdock results

✔️ PASSED

739e269 drivers/leds: Make initialization conditional on presence, not pin

Success Failures Total Runtime
10180 0 10180 15m:11s

Artifacts

@chrysn chrysn force-pushed the leds-provided-on-demand branch from 94745c4 to c7d0285 Compare August 25, 2024 13:13
@chrysn
Copy link
Member Author

chrysn commented Aug 25, 2024

The latest update fixes building on native (which I had not tested), where the new condition would have led to an attempt to initialize a pin from an undefined macro. The new condition for setup is "the LED is present and has a pin associated". I ran the described tests again, let's see if CI comes up with anything more.

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for finding and fixing! Just a minor comment below.

boards/common/particle-mesh/include/board.h Show resolved Hide resolved
drivers/periph_common/init_leds.c Show resolved Hide resolved
chrysn added 2 commits August 26, 2024 11:12
Otherwise they show as provided but do not have any effect.
The LEDx_PIN may still be defined (eg. on the particle-mesh family
because it is used to configure the PWM pins); the IS_PRESENT macro
performs the stricter check ov testing for an _ON function.
@chrysn chrysn force-pushed the leds-provided-on-demand branch from 731a839 to 739e269 Compare August 26, 2024 09:13
@chrysn chrysn enabled auto-merge August 26, 2024 09:18
@chrysn chrysn added this pull request to the merge queue Aug 26, 2024
Merged via the queue into RIOT-OS:master with commit 48f156f Aug 26, 2024
27 checks passed
@chrysn chrysn deleted the leds-provided-on-demand branch August 26, 2024 13:18
@benpicco benpicco added this to the Release 2024.10 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants