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

samples: fade_led: use all available LEDs #80496

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

Conversation

yishai1999
Copy link
Contributor

Refactored the sample code to use all available PWM LEDs

@zephyrbot zephyrbot added the area: Samples Samples label Oct 28, 2024
@zephyrbot zephyrbot requested review from kartben and nashif October 28, 2024 07:39
@yishai1999 yishai1999 force-pushed the use_all_pwm_leds branch 5 times, most recently from 5ecc8c6 to 0d6f7c4 Compare October 28, 2024 13:24
@yishai1999
Copy link
Contributor Author

@kartben Can we merge?

@kartben kartben added the area: LED Label to identify LED subsystem label Nov 10, 2024
@kartben kartben assigned simonguinot and unassigned kartben Nov 10, 2024
@simonguinot
Copy link
Collaborator

Isn't this sample superseded by samples/drivers/led/pwm ? Maybe it should simply be removed ?

@yishai1999
Copy link
Contributor Author

yishai1999 commented Nov 11, 2024

Isn't this sample superseded by samples/drivers/led/pwm ? Maybe it should simply be removed ?

Not sure but the fade_led sample uses the pwm API while the led_pwm sample uses the led driver API.
Also, I'm testing this using the nucleo_l4r5zi and stm32f4_disco boards and the led_pwm sample doesn't work on either of them.

Comment on lines +21 to +22
/* Support up to 4 LEDs*/
static const struct pwm_dt_spec pwm_leds[] = {LISTIFY(4, PWM_LED, ())};
Copy link
Member

Choose a reason for hiding this comment

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

Why limit up to 4?
I don't think there's any problem with not placing any limitation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can raise the number but there needs to be some cap because there needs to be some sort of iteration over the LEDs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use the DT macros to run for each entry and should be able to construct any length array from that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you give an example? In this case there is no matching compatible string for a pwm LED or something like that so I don't understand how this would be done.

ret = pwm_set_pulse_dt(&pwm_leds[i], pulse_widths[i]);
if (ret) {
printk("Error %d: failed to set pulse width for LED %d\n", ret, i);
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

It should good to proceed with only what is available here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Refactored the sample code to use all available PWM LEDs

Signed-off-by: Yishai Jaffe <[email protected]>
@simonguinot
Copy link
Collaborator

simonguinot commented Nov 25, 2024

Isn't this sample superseded by samples/drivers/led/pwm ? Maybe it should simply be removed ?

Not sure but the fade_led sample uses the pwm API while the led_pwm sample uses the led driver API. Also, I'm testing this using the nucleo_l4r5zi and stm32f4_disco boards and the led_pwm sample doesn't work on either of them.

@yishai1999 Please can you investigate and eventually fill a bug report ? The led_pwm driver relies on the PWM API. If the fade_led sample works for you, then led_pwm should too.

@yishai1999
Copy link
Contributor Author

Isn't this sample superseded by samples/drivers/led/pwm ? Maybe it should simply be removed ?

Not sure but the fade_led sample uses the pwm API while the led_pwm sample uses the led driver API. Also, I'm testing this using the nucleo_l4r5zi and stm32f4_disco boards and the led_pwm sample doesn't work on either of them.

@yishai1999 Please can you investigate and eventually fill a bug report ? The led_pwm driver relies on the PWM API. If the fade_led sample works for you, then led_pwm should too.

Ok, I understood the problem and open a PR (#82383).
After diving deeper into the samples I would agree that this sample is superceded by samples/drivers/led/pwm, but so is samples/basic/blinky and samples/basic/blinky_pwm. They do overlap with their logic, but they use different API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: LED Label to identify LED subsystem area: Samples Samples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants