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

Feature/improve caf leds #19892

Closed
wants to merge 2 commits into from

Conversation

nrbrook
Copy link
Contributor

@nrbrook nrbrook commented Jan 14, 2025

Improved LED effects in the following ways:

  • LEDs module now uses one work queue which updates all LEDs every 10ms
    (by default).
    This ensures accurate timing and smooth animations.
  • Added several built-in easing functions, and ability to provide a custom
    easing function
  • Added several LED effects.
  • LED effects are composed with macros which makes it easier to create new
    effects.
  • Defined several colors, with brightness parameters.
    Makes LED state defs more easily readable.

One issue I'm unsure about is licensing. The built-in easing functions are copied from the FastLED library. I'm not sure if that is allowed, and if so, the correct way to provide attribution.

@nrbrook nrbrook requested review from a team as code owners January 14, 2025 10:31
@github-actions github-actions bot added doc-required PR must not be merged without tech writer approval. changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Jan 14, 2025
@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 Jan 14, 2025
Improved LED effects in the following ways:
- LEDs module now uses one work queue which updates all LEDs every 10ms
  (by default).
  This ensures accurate timing and smooth animations.
- Added several built-in easing functions, and ability to provide a
  custom easing function
- Added several LED effects.
- LED effects are composed with macros which makes it easier to create
  new effects.
- Defined several colors, with brightness parameters.
  Makes LED state defs more easily readable.
This commit includes the core LEDs module improvements. A following
commit will implement the changes in other files.

Signed-off-by: Nick Brook <[email protected]>
Implements the LED macro changes in other project which use the LED module.

Signed-off-by: Nick Brook <[email protected]>
@nrbrook nrbrook force-pushed the feature/improve_caf_leds branch from 0f2980e to 9e393d6 Compare January 14, 2025 10:48
@pdunaj
Copy link
Contributor

pdunaj commented Jan 14, 2025

Hi @nrbrook , thank you for the suggestions. I don't think we will be able to add all of the changes as these seems quite invasive.
We have not been able to update this part for some time and it would be neat to improve the effects. I will try to have a look in the coming weeks.

This module was created to allow effects definition for nrf_desktop. I imagined additional effects could be defined outside of the module. Maybe there is something missing that could be added quickly and simplify app-specific animation definitions.

The built-in easing functions are copied from the FastLED library.

Thanks for letting us know.

@pdunaj pdunaj added the DNM label Jan 14, 2025
Copy link
Contributor

@pdunaj pdunaj left a comment

Choose a reason for hiding this comment

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

This will need to be properly reviewed and broken down. If anything, changes should be introduced without a need to update any of the existing applications.

@nrbrook
Copy link
Contributor Author

nrbrook commented Jan 14, 2025

Thanks for the feedback.
I can certainly remove the changes to other applications and restore the original macro names so those changes are not required.

Otherwise, the changes are only limited to the LEDs module itself.

The previous implementation was not sufficient to be able to define effects with easing, and could not maintain accurate timing of effects.

@nrbrook
Copy link
Contributor Author

nrbrook commented Jan 14, 2025

Closing this PR in favour of #19904

@nrbrook nrbrook closed this Jan 14, 2025
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. DNM doc-required PR must not be merged without tech writer approval. external External contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants