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

caf: modules: leds: fix support GPIO-based LED devicetree definitions #19703

Closed
wants to merge 1 commit into from

Conversation

jelledevleeschouwer
Copy link
Contributor

When CONFIG_CAF_LEDS_GPIO was chosen over CONFIG_CAF_LEDS_PWM, an assertion fails that checks whether a gpio-leds compatible node has either 1 or 3 child nodes to define multi-color LEDs. However, most Nordic development boards have 4 leds, which the CAF leds module does not support and hence breaks these builds. This commit adds support for up to a combination of 4 leds in what could be analogous to an RGBW LED. The changes are made as such that as little application code as possible is affected by the change. If user code wants to take full advantage of the full channel color support it will have to redefine the OFF, ON, ON_GO_OFF, BLINK, BLINK2, BREATH, and CLOCK effects since LED_NOCOLOR is only defined for 3 channels.

@jelledevleeschouwer jelledevleeschouwer requested review from a team as code owners December 23, 2024 14:07
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 23, 2024
@NordicBuilder
Copy link
Contributor

Thank you for your contribution!
It seems you are not a member of the nrfconnect GitHub organization. External contributions are handled as follows:
Large contributions, affecting multiple subsystems for example, may be rejected if they are complex, may introduce regressions due to lack of test coverage, or if they are not consistent with the architecture of nRF Connect SDK.
PRs will be run in our continuous integration (CI) test system.
If CI passes, PRs will be tagged for review and merged on successful completion of review. You may be asked to make some modifications to your contribution during review.
If CI fails, PRs may be rejected or may be tagged for review and rework.
PRs that become outdated due to other changes in the repository may be rejected or rework requested.
External contributions will be prioritized for review based on the relevance to current development efforts in nRF Connect SDK. Bug fix PRs will be prioritized.
You may raise issues or ask for help from our Technical Support team by visiting https://devzone.nordicsemi.com/.

Note: This comment is automatically posted and updated by the Contribs GitHub Action.

@NordicBuilder NordicBuilder added the external External contribution label Dec 23, 2024
When CONFIG_CAF_LEDS_GPIO was chosen over CONFIG_CAF_LEDS_PWM,
an assertion fails that checks whether a gpio-leds compatible
node has either 1 or 3 child nodes to define multi-color LEDs.
However, most Nordic development boards have 4 leds, which
the CAF leds module does not support and hence breaks these
builds. This commit adds support for up to a combination of
4 leds in what could be analogous to an RGBW LED. The changes
are made as such that as little application code as possible
is affected by the change. If user code wants to take full
advantage of the full channel color support it will have to
redefine the OFF, ON, ON_GO_OFF, BLINK, BLINK2, BREATH, and
CLOCK effects since LED_NOCOLOR is only defined for 3 channels.

Signed-off-by: Jelle De Vleeschouwer <jelle.devleeschouwer@classified-cycling.cc>
@jelledevleeschouwer jelledevleeschouwer changed the title subsys: caf: fix support GPIO-based LED devicetree definitions caf: modules: leds: fix support GPIO-based LED devicetree definitions Dec 23, 2024
@jelledevleeschouwer
Copy link
Contributor Author

CC: @MarekPieta, @nordicjm, @tejlmand, @pdunaj

@MarekPieta
Copy link
Contributor

Hi @jelledevleeschouwer, Thanks for the contribution.

The CAF LEDs module documentation states that:

The LEDs module assumes that a single DTS node is a separate and complete logical LED.
The LEDs module expects that the node will hold configuration of HW LEDs responsible for reproducing all required color channels.
The number of HW LEDs configured to reproduce color channels can be either one or three (either monochromatic or following the RGB order, with the red channel defined first, then the green one, then the blue one).
If only one HW LED is used for a monochromatic setting, the module converts the tri-channel color to a single value of brightness and passes it to this single HW LED.
The LED color channels are represented as child nodes of the compatible DTS node.

Ref: https://docs.nordicsemi.com/bundle/ncs-2.8.0/page/nrf/libraries/caf/leds.html#configuring_leds_in_dts

Controlling 4 monochromatic LEDs as a single RGBW LED is not consistent with the described assumptions. It would also be problematic if the monochromatic LEDs are controlled by multiple modules - for example if module A controls the first LED and module B controls the second LED for another purpose. Every module controlling any of the monochromatic LEDs would need to know what colors/effects are set to all 4 monochromatic LEDs to submit a proper LED event, because a LED event updates all color channels of a given logical LED (https://docs.nordicsemi.com/bundle/ncs-2.7.99-cs1/page/nrf/libraries/caf/caf_overview.html#c.led_event).
Maybe you could disable leds node and redefine the GPIO LED nodes in similar way as we do here: https://github.com/nrfconnect/sdk-nrf/blob/main/applications/nrf_desktop/configuration/nrf52833dk_nrf52820/app.overlay#L22-L45
See documentation for more details on how to configure GPIO LEDs used by CAF LEDs module in the DTS: https://docs.nordicsemi.com/bundle/ncs-2.8.0/page/nrf/libraries/caf/leds.html#configuring_gpio_leds

Would this approach be acceptable for your use-case?

@jelledevleeschouwer jelledevleeschouwer deleted the branch nrfconnect:main January 10, 2025 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. external External contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants