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

feat(underglow): Add support for configurable min/max brightness #944

Merged
merged 2 commits into from
Oct 11, 2021

Conversation

malinges
Copy link
Contributor

This PR introduces two new Kconfig options ZMK_RGB_UNDERGLOW_BRT_MIN and ZMK_RGB_UNDERGLOW_BRT_MAX to define minimum and maximum brightness levels in percents.

Use case for minimum brightness > 0: setting a >0 minimum brightness prevents keeping underglow in the "ON" state by mistake and emptying the battery very quickly on boards that have an external power mosfet.

Use case for maximum brightness < 100: limiting the maximum current consumption, especially for boards/shields that have a high number of LEDs.

@malinges
Copy link
Contributor Author

I just noticed #669...

There are a few differences however:

#669 treats CONFIG_ZMK_RGB_UNDERGLOW_BRT_STEP as a percentage of the available (not total) brightness range, which means that setting MIN=10 MAX=50 STEP=10 would still result in 11 steps in total, instead of the 5 steps that one would get with this PR. On one hand I think that having 11 steps with this config would be unintuitive, on the other hand I can see that being able to modify the min&max brightness values independently of the number of steps could actually be useful in practice. Opinions welcome!

#669 treats CONFIG_ZMK_RGB_UNDERGLOW_BRT_MIN as a hard lower brightness cap at all times, and scales all HSB brightness values linearly between the min and max values. This prevents RGB effects from using lower brightness values if they want to. This PR only uses CONFIG_ZMK_RGB_UNDERGLOW_BRT_MIN as a lower cap for clamping the user-configured global brightness value (state.color.b, which is modified by &rgb_ug RGB_BRI and &rgb_ug RGB_BRD) which means that with this PR:

  • effects that manage brightness by themselves (i.e. don't use the user-configured global brightness state.color.b, like the breathing effect) are able to use lower brightness values if they want to for aesthetic reasons (note: this doesn't defeat the purpose of having a >0 minimum brightness to know when RGB is enabled, since the breathing effect cycles through brightness values, so it would still be visible)
  • no scaling at all in hsb_to_rgb() (so no additional float computation, considering that AFAIK the FPU is currently disabled in all but 2 boards) except for the breathing effect (but then it is integer scaling)

Finally, since #669 applies brightness scaling in a central place, it's almost impossible to "forget" to do it. But the different approach used by this PR requires scaling/clamping to be applied in a few different places, so the risk of introducing mistakes during future code changes is comparatively higher (although they would be easy to fix).

@petejohanson
Copy link
Contributor

I just noticed #669...

There are a few differences however:

#669 treats CONFIG_ZMK_RGB_UNDERGLOW_BRT_STEP as a percentage of the available (not total) brightness range, which means that setting MIN=10 MAX=50 STEP=10 would still result in 11 steps in total, instead of the 5 steps that one would get with this PR. On one hand I think that having 11 steps with this config would be unintuitive, on the other hand I can see that being able to modify the min&max brightness values independently of the number of steps could actually be useful in practice. Opinions welcome!

I think I favor the solution in #669 on this one. Having it be a percentage of the available brightness range ensures that if I tweak the minx/max a tad to adjust, I still always have the same number of steps available.

#669 treats CONFIG_ZMK_RGB_UNDERGLOW_BRT_MIN as a hard lower brightness cap at all times, and scales all HSB brightness values linearly between the min and max values. This prevents RGB effects from using lower brightness values if they want to. This PR only uses CONFIG_ZMK_RGB_UNDERGLOW_BRT_MIN as a lower cap for clamping the user-configured global brightness value (state.color.b, which is modified by &rgb_ug RGB_BRI and &rgb_ug RGB_BRD) which means that with this PR:

* effects that manage brightness by themselves (i.e. don't use the user-configured global brightness `state.color.b`, like the breathing effect) are able to use lower brightness values if they want to for aesthetic reasons (note: this doesn't defeat the purpose of having a >0 minimum brightness to know when RGB is enabled, since the breathing effect cycles through brightness values, so it would still be visible)

* no scaling at all in `hsb_to_rgb()` (so no additional float computation, considering that AFAIK the FPU is currently disabled in all but 2 boards) except for the breathing effect (but then it is integer scaling)

Finally, since #669 applies brightness scaling in a central place, it's almost impossible to "forget" to do it. But the different approach used by this PR requires scaling/clamping to be applied in a few different places, so the risk of introducing mistakes during future code changes is comparatively higher (although they would be easy to fix).

Tad torn here. I can see the value of not having the minimum value for the breathing effect limited here, but also I can see wanting the max for the breathing effect clamped by this settings.

@Nicell Thoughts?

@malinges malinges force-pushed the underglow/min-max-brightness branch from 22e67d9 to 7adfea7 Compare October 7, 2021 03:54
@malinges
Copy link
Contributor Author

malinges commented Oct 7, 2021

I think I favor the solution in #669 on this one. Having it be a percentage of the available brightness range ensures that if I tweak the minx/max a tad to adjust, I still always have the same number of steps available.

Fixed in the latest push.

Tad torn here. I can see the value of not having the minimum value for the breathing effect limited here, but also I can see wanting the max for the breathing effect clamped by this settings.

Actually, the breathing effect does scale its output according to the max brightness, it simply doesn't use the min brightness at all to be able to go down to brightness 0 (I thought it was more aesthetically pleasing this way, but this could be changed easily, especially now with the scaling functions).

@malinges malinges force-pushed the underglow/min-max-brightness branch from c4fc586 to 7adfea7 Compare October 7, 2021 04:13
@malinges
Copy link
Contributor Author

malinges commented Oct 7, 2021

I pushed unrelated commits to the wrong branch by mistake, sorry about that, I just reverted the change...

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.

I just left a comment on #669, but it seems like this PR achieves what I was thinking for that one. Now that it's a percentage of the steps, and the breathing effect is still clamped, this PR looks like a proper solution to me.

@Nicell Nicell force-pushed the underglow/min-max-brightness branch from 7adfea7 to a0f71c9 Compare October 11, 2021 00:26
@Nicell
Copy link
Member

Nicell commented Oct 11, 2021

@malinges I've run clang-format on your branch as well as added @jrhrsmit as a co-author, as they put a lot of time into iterating #669. Will merge once all checks pass.

@jrhrsmit thanks for your work on #669! This PR has combined the good parts of both, so I'm going to go ahead with this one. I still like the ranges you added for the Kconfig, so I'm open to having a separate PR for that.

@Nicell Nicell merged commit f23f427 into zmkfirmware:main Oct 11, 2021
@malinges malinges deleted the underglow/min-max-brightness branch October 12, 2021 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants