Skip to content

drivers: led: shell: add blink cmd #90342

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

drivers: led: shell: add blink cmd #90342

wants to merge 1 commit into from
+45 −0

Conversation

yishai1999
Copy link
Contributor

Add blink command to led shell module

@github-actions github-actions bot added the area: LED Label to identify LED subsystem label May 22, 2025
simonguinot
simonguinot requested changes May 22, 2025
View reviewed changes
Copy link
Contributor

@simonguinot simonguinot left a comment

Choose a reason for hiding this comment

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

Hi @yishai1999,

Thanks for this patch. Please see my comment below.

@yishai1999 yishai1999 force-pushed the led-shell-blink branch 2 times, most recently from b028a6a to d9d39e1 Compare May 22, 2025 15:57
@yishai1999 yishai1999 requested a review from simonguinot May 25, 2025 13:45
@yishai1999
Copy link
Contributor Author

@simonguinot What do you think?

@yishai1999 yishai1999 requested review from thedjnK, bbilas and mbolivar June 5, 2025 09:10
@simonguinot
Copy link
Contributor

Hi @yishai1999,

Please can you rebase on #91612 ?

Sorry for the delay and the inconvenience.

@yishai1999
Copy link
Contributor Author

Hi @yishai1999,

Please can you rebase on #91612 ?

Sorry for the delay and the inconvenience.

Sure. Just waiting for it to be merged.

@yishai1999
Copy link
Contributor Author

Hi @yishai1999,

Please can you rebase on #91612 ?

Sorry for the delay and the inconvenience.

Rebased

@yishai1999 yishai1999 added the area: Shell Shell subsystem label Jun 19, 2025
simonguinot
simonguinot reviewed Jun 25, 2025
View reviewed changes
Copy link
Contributor

@simonguinot simonguinot left a comment

Choose a reason for hiding this comment

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

Hi @yishai1999,

Thanks for this PR. I tested the led blink command successfully.

Please see my comments below.

@yishai1999 yishai1999 force-pushed the led-shell-blink branch 3 times, most recently from 4615734 to fb78bfe Compare June 26, 2025 07:19
simonguinot
simonguinot previously approved these changes Jun 26, 2025
View reviewed changes
Copy link
Contributor

@simonguinot simonguinot left a comment

Choose a reason for hiding this comment

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

Thanks !

kartben
kartben requested changes Jun 26, 2025
View reviewed changes
Copy link
Contributor

@kartben kartben left a comment

Choose a reason for hiding this comment

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

I feel bad for requesting a change about that but I really think it would be nice if the delay_off argument would be optional, in which case it defaults to being the same a delay_on

Hopefully my comments/suggestions are close enough to achieving this, with minimal/no tweaking. Thank you!

@yishai1999
Copy link
Contributor Author

Rebased off main

@yishai1999
Copy link
Contributor Author

Applied suggestion

kartben
kartben previously approved these changes Jun 29, 2025
View reviewed changes
drivers/led/led_shell.c Outdated
@@ -353,6 +394,10 @@ SHELL_STATIC_SUBCMD_SET_CREATE(
3, 0),
SHELL_CMD_ARG(on, &dsub_device_name, SHELL_HELP("Turn on LED", "<device> <led>"), cmd_on, 3,
0),
SHELL_CMD_ARG(blink, &dsub_device_name,
SHELL_HELP("Blink LED on and off",
"<device> <led> <delay_on_in_ms> [delay_off_in_ms]"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Thought I had suggested it this way, apologies if not! Not blocking, I guess

Suggested change
"<device> <led> <delay_on_in_ms> [delay_off_in_ms]"),
"<device> <led> <delay_on_in_ms> [<delay_off_in_ms>]"),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right. Done. BTW, just so you know, there are a bunch of places where it's written [param] but I agree with you that it should be like this

@yishai1999
drivers: led: shell: add blink cmd
7d15f8a 
Add blink command to led shell module

Signed-off-by: Yishai Jaffe <yishai1999@gmail.com>
@yishai1999
Copy link
Contributor Author

Applied suggestion

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@yishai1999 yishai1999 added this to the v4.2.0 milestone Jun 30, 2025
@yishai1999
Copy link
Contributor Author

@simonguinot @kartben ping?

@kartben kartben modified the milestones: v4.2.0, v4.3.0 Jul 2, 2025
kartben
kartben approved these changes Jul 2, 2025
View reviewed changes
Copy link
Contributor

@kartben kartben left a comment

Choose a reason for hiding this comment

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

+1 of course, and thanks! But fwiw reviews on stuff that's now targetting 4.3 will likely be slower than usual for the next few weeks :)

@yishai1999
Copy link
Contributor Author

+1 of course, and thanks! But fwiw reviews on stuff that's now targetting 4.3 will likely be slower than usual for the next few weeks :)

Can this be merged for 4.2.0?

@kartben
Copy link
Contributor

kartben commented Jul 2, 2025

+1 of course, and thanks! But fwiw reviews on stuff that's now targetting 4.3 will likely be slower than usual for the next few weeks :)

Can this be merged for 4.2.0?

It being a new feature (albeit a small and reasonably trivial one), probably not, unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kartben kartben kartben approved these changes

@jeppenodgaard jeppenodgaard jeppenodgaard approved these changes

@Mani-Sadhasivam Mani-Sadhasivam Awaiting requested review from Mani-Sadhasivam

@thedjnK thedjnK Awaiting requested review from thedjnK

@bbilas bbilas Awaiting requested review from bbilas

@mbolivar mbolivar Awaiting requested review from mbolivar

@simonguinot simonguinot Awaiting requested review from simonguinot

Assignees

@Mani-Sadhasivam Mani-Sadhasivam

@simonguinot simonguinot

Labels
area: LED Label to identify LED subsystem area: Shell Shell subsystem
Projects
None yet
Milestone
v4.3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@yishai1999 @simonguinot @kartben @jeppenodgaard @Mani-Sadhasivam