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 backlight #904

Merged
merged 8 commits into from
Feb 6, 2022
Merged

Feature backlight #904

merged 8 commits into from
Feb 6, 2022

Conversation

bortoz
Copy link
Contributor

@bortoz bortoz commented Aug 9, 2021

This PR adds support for single color LED backlighting. The current implementation supports basic behaviors, such as increase/decrease brightness and toggle on/off.

I also added some useful configurations to increase power efficiency such as turning off the backlight when the USB is disconnected or when the board goes into idle state.

Finally I updated the documentation and the power profiler with the new changes.

Copy link
Member

@Nicell Nicell left a comment

Choose a reason for hiding this comment

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

Nice job on this @bortoz, clean implementation of the feature. Here are some initial changes/thoughts on this 👍

app/Kconfig Outdated Show resolved Hide resolved
app/Kconfig Outdated Show resolved Hide resolved
app/Kconfig Outdated Show resolved Hide resolved
docs/src/components/power-estimate.js Outdated Show resolved Hide resolved
app/src/backlight.c Outdated Show resolved Hide resolved
app/src/backlight.c Outdated Show resolved Hide resolved
app/src/backlight.c Outdated Show resolved Hide resolved
docs/docs/features/backlight.md Outdated Show resolved Hide resolved
docs/docs/features/backlight.md Outdated Show resolved Hide resolved
docs/docs/features/backlight.md Outdated Show resolved Hide resolved
@bortoz
Copy link
Contributor Author

bortoz commented Aug 10, 2021

@Nicell I have updated the code based on your comments.

Copy link
Member

@Nicell Nicell left a comment

Choose a reason for hiding this comment

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

A few more things I noticed. Excited to get this added to ZMK. Thanks for the quick update on this.

docs/src/data/power.js Show resolved Hide resolved
docs/src/pages/power-profiler.js Outdated Show resolved Hide resolved
docs/src/data/power.js Outdated Show resolved Hide resolved
docs/src/pages/power-profiler.js Outdated Show resolved Hide resolved
@Nicell Nicell added core Core functionality/behavior of ZMK enhancement New feature or request lighting labels Aug 10, 2021
@htang014
Copy link

FWIW, I built your fork to my board, works like a charm

@bortoz
Copy link
Contributor Author

bortoz commented Aug 11, 2021

Thanks for your feedback @htang014!

Copy link
Member

@Nicell Nicell left a comment

Choose a reason for hiding this comment

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

Two small link changes (#911 related) and a merge conflict fix. Otherwise, this all looks good to me. I'd like to have one more set of eyes over it before a merge though.

docs/docs/features/backlight.md Outdated Show resolved Hide resolved
docs/docs/intro.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@joelspadin joelspadin left a comment

Choose a reason for hiding this comment

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

I think there may be some off by one errors in the brightness level configs and/or calculations.

Also, check that ZMK_BACKLIGHT_USB works. I might be missing something, but it looks like the current code won't compile with that option enabled.

app/Kconfig Outdated Show resolved Hide resolved
app/Kconfig Outdated Show resolved Hide resolved
app/include/dt-bindings/zmk/bl.h Outdated Show resolved Hide resolved
app/include/dt-bindings/zmk/bl.h Outdated Show resolved Hide resolved
app/src/backlight.c Show resolved Hide resolved
app/src/backlight.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_backlight.c Outdated Show resolved Hide resolved

static int on_keymap_binding_released(struct zmk_behavior_binding *binding,
struct zmk_behavior_binding_event event) {
return ZMK_BEHAVIOR_OPAQUE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not something to change in this commit, but it seems a bit silly that we require a released callback for behaviors that only do something on press.

@petejohanson maybe we could change z_impl_behavior_keymap_binding_released so it returns ZMK_BEHAVIOR_OPAQUE instead of -ENOTSUP if no release callback is set?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think perhaps returning that is still valid, but the keymap code could better handle that errno specifically.

docs/docs/behaviors/lighting.md Outdated Show resolved Hide resolved
docs/docs/features/backlight.md Outdated Show resolved Hide resolved
@bortoz bortoz force-pushed the backlight branch 7 times, most recently from 315b21a to b4f8773 Compare August 14, 2021 14:05
@bortoz
Copy link
Contributor Author

bortoz commented Jan 6, 2022

@petejohanson I made a PR: zmkfirmware/zephyr#7

@bortoz bortoz force-pushed the backlight branch 2 times, most recently from 24a22b0 to d83f220 Compare January 7, 2022 13:15
Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

On a whole, this looks great!

A few minor comments.

app/src/backlight.c Outdated Show resolved Hide resolved

static int on_keymap_binding_released(struct zmk_behavior_binding *binding,
struct zmk_behavior_binding_event event) {
return ZMK_BEHAVIOR_OPAQUE;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think perhaps returning that is still valid, but the keymap code could better handle that errno specifically.

app/west.yml Outdated Show resolved Hide resolved
docs/docs/features/backlight.md Outdated Show resolved Hide resolved
@bortoz bortoz force-pushed the backlight branch 3 times, most recently from 1fbe79e to 05013ec Compare January 10, 2022 08:41
Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

A few minor follow up thoughts. Thanks again!

I'll have test hardware for this soon.

app/dts/behaviors/backlight.dtsi Outdated Show resolved Hide resolved
Comment on lines 34 to 36
Backlight is always added to a board, not a shield.
If you have a shield with backlight, you must add a `boards/` directory within your shield folder to define the backlight individually for each board that supports the shield.
Inside the `boards/` folder, you define a `<board>.overlay` for each different board.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about this more, since we have the GPIO backed driver available that should be board agnostic, I wonder if we should be documenting and suggesting:

  1. Add GPIO backlight nodes/compatibles to shields.
  2. For tested boards used w/ the shield, add a shield/foo/boards/bar.overlay to override and add the board-specific PWM nodes.

Doing so will ensure a shield would have reasonable "fallback" backlight on some new board used w/ a given shield.

Thoughts?

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 think it's a good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a note about that

@ReFil
Copy link
Contributor

ReFil commented Jan 20, 2022

A few minor follow up thoughts. Thanks again!

I'll have test hardware for this soon.

Hi Pete, we can send you one of our boards equipped for this backlighting stuff if you want

@petejohanson
Copy link
Contributor

A few minor follow up thoughts. Thanks again!

I'll have test hardware for this soon.

Hi Pete, we can send you one of our boards equipped for this backlighting stuff if you want

I actually snagged a revlp recently so that:

  • I have a non-split for testing non-wireless controllers like kb2040
  • Has backlighting!

I have the resistors en route right now so I can solder the final piece and test this.

Thanks though!

@bortoz bortoz force-pushed the backlight branch 2 times, most recently from 3291ae9 to 06d6daa Compare January 22, 2022 10:58
Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

Few follow up thoughts, tested this and it works great!

docs/docs/features/backlight.md Outdated Show resolved Hide resolved
docs/docs/features/backlight.md Outdated Show resolved Hide resolved
@bortoz bortoz force-pushed the backlight branch 2 times, most recently from a3fc437 to 7efbd98 Compare January 31, 2022 10:25
@bortoz
Copy link
Contributor Author

bortoz commented Jan 31, 2022

Rebased to resolve a conflict

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

Thanks so much for your work on this!

@petejohanson petejohanson merged commit be94e04 into zmkfirmware:main Feb 6, 2022
@bortoz bortoz deleted the backlight branch February 6, 2022 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core functionality/behavior of ZMK enhancement New feature or request lighting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants