feat(underglow): maximum brightness in Kconfig

[jrhrsmit]

Current draw over USB is dominated by the underglow in every board that has it (or the battery charger).
Defining a maximum brightness to limit the maximum current can prevent the keyboard from exceeding the USB spec of 500mA (or at least reduce it a little).

I also added ranges to ZMK_RGB_UNDERGLOW_BRT_START and ZMK_RGB_UNDERGLOW_BRT_MAX, but this is not done for the other options. Should I add/remove them for consistency?

[petejohanson]

I have some work to refactor this in #626, we'll probably need to rebase this once that's merged, the code changes should be pretty obvious, but might need tweaks since I replaced all uses of that 100 constant w/ a preprocessor define.

[jrhrsmit]

Alright I replaced BRT_MAX by CONFIG_ZMK_RGB_UNDERGLOW_BRT_MAX.

The stuff about the inconsistency with the ranges in the Kconfig still stands though.

[joric]

Ack. Good fix, my first move when the keyboard lights up is to set the brightness to minumum, nRFMicro heats up pretty substantially with 28 LEDs (not sure why, maybe it's just SOD-323 vs SOD-123 as on 1.1 and on Pro Micros) There's another related issue, there's like zero brightness it's very confusing and I don't think it's intended. Need to be capped at 1 or 8 or whatever the lowest setting is, not at 0 (unless it's the same as turning LEDs off, which is complicated considering EXT_VCC).

[jrhrsmit]

There's another related issue, there's like zero brightness it's very confusing and I don't think it's intended. Need to be capped at 1 or 8 or whatever the lowest setting is, not at 0 (unless it's the same as turning LEDs off, which is complicated considering EXT_VCC).

That seems logical, I'll add it.

[joric]

@jrhrsmit maybe add ZMK_RGB_UNDERGLOW_BRT_MIN then?

[jrhrsmit]

@joric Even though that would be a simple and small change, I don't really see the added value of that option to be honest. It seems to me the issue you're describing is a hardware issue. I'd be willing to add it though if there are more people who could use it.

Although it would be nice if the LED strip would turn off automatically when the brightness is set to 0, but that's for another PR I guess.

[joric]

@jrhrsmit I'm using the most popular shield of all time, corne keyboard so no it's not just my hardware issue. I'm not sure about the lowest value for LEDs (they start working at something way above 1 AFAIR) so having a user variable there would be nice.

[jrhrsmit]

@joric Although the shield you're using might be very popular, transistors are usually not supposed to heat up when there is no current flowing. Anyway, I looked at it a little, it seems the implementation of a minimum brightness setting is non-trivial, as something like

     if (b < CONFIG_ZMK_RGB_UNDERGLOW_BRT_MIN) {
        b = 0;

would not be enough to fix your problem (if I understood correctly, your transistors heat up at every value below 8 or so, so setting it to 0 wouldn't help). So instead of b = 0;, zmk_rgb_underglow_off() would have to be called then, which also implies that turning up the brightness afterwards wouldn't turn on the LEDs in the current state of the code, so in the end it would look something like this:

struct zmk_led_hsb zmk_rgb_underglow_calc_brt(int direction) {
    struct zmk_led_hsb color = state.color;

    int b = color.b + (direction * CONFIG_ZMK_RGB_UNDERGLOW_BRT_STEP);
    if (state.on) {
        if (b < CONFIG_ZMK_RGB_UNDERGLOW_BRT_MIN) {
            b = 0;
            zmk_rgb_underglow_off();
        } else if (b > CONFIG_ZMK_RGB_UNDERGLOW_BRT_MAX) {
            b = CONFIG_ZMK_RGB_UNDERGLOW_BRT_MAX;
        }
        color.b = b;
    } else {
        if (direction > 0) {
            if ( b <CONFIG_ZMK_RGB_UNDERGLOW_BRT_MIN) {
                b =  CONFIG_ZMK_RGB_UNDERGLOW_BRT_MIN;
            }
            color.b = b;
            zmk_rgb_underglow_on();
        }
    }
    
    return color;
}

Which thus implies turning the rgb on and off inside zmk_rgb_underglow_calc_brt(). Let me know what your thoughts are on this.

edit; I think it would be better to move the evaluation of state.on and the calling of zmk_rgb_underglow_off() and zmk_rgb_underglow_on() to zmk_rgb_underglow_change_brt(), but it's the same idea.

[joric]

Why would I set them to 0 at all? I won't set them to 0 ever, it's confusing (there should be a single point of entry) and it's power inefficient (they consume up to 1 mA each in standby mode at 0 brightness). They dont "heat up" at every value below 8 or so they just don't light up. I need minimum brightness value to see if they're enabled or not.

The code is complicated, not sure I follow. I just wanted to have some lower value for the brightness, not just ZMK_RGB_UNDERGLOW_BRT_START and ZMK_RGB_UNDERGLOW_BRT_MAX but ZMK_RGB_UNDERGLOW_BRT_MIN as well because it's kind of vital to know if RGB is enabled or not.

Upd: Ah ok code tries to turn them off. I don't think I like that very much (again, single point of entry) and it probably conflicts with the EXT_VCC switch (or maybe not, not sure). I thought min value would be easier and more logical.

[jrhrsmit]

@joric Alright, then the most recent change which just adds the ZMK_RGB_UNDERGLOW_BRT_MIN should be just fine. I also realised the changing of the brightness doesn't have to imply turning the LEDs on or off, so the code snippet from earlier can be disregarded.

Btw I also took the liberty of adding the ranges to the other underglow Kconfig options and changing the descriptions a little.

[Nicell]

I'm wondering if CONFIG_ZMK_RGB_UNDERGLOW_BRT_MAX should be used as a cap here rather than an actual hard limit. For example, if someone has a key that sets the HSB to 0, 100, 100, should that be 100% of the cap, or should it really just error?

[Megamannen]

100% of CONFIG_ZMK_RGB_UNDERGLOW_BRT_MAX, I see that as a hardware limitation implemented in software. (don't know what BRT_MAX is though ;) )

You can also draw a paralell to dimmers in houses, they have an "installation setting" MIN and MAX that depends on the installed lightning.
From the user side a dimmer value of 100% = MAX and an user value of 1% = MIN (0% is off)

[jrhrsmit]

I feel like that scenario should be impossible. I proposed this change with hardware limitations in mind rather than a preference or something.

[Nicell]

This scenario is definitely possible. All someone needs to do is add a &rgb_ug RGB_COLOR_HSB(100, 100, 100) and now that key silently fails to do anything because of this check.

[Nicell]

I think we should also consider the possibility of a max/min brightness that's saved in settings that's outside of this range. On retrieval of RGB settings, we should probably do a check on if it's outside the Kconfig range and correct that.

[jrhrsmit]

I think we should also consider the possibility of a max/min brightness that's saved in settings that's outside of this range. On retrieval of RGB settings, we should probably do a check on if it's outside the Kconfig range and correct that.

In theory it would be impossible to set a brightness value in the settings that is outside of the Kconfig range, right?

[Nicell]

In theory it would be impossible to set a brightness value in the settings that is outside of the Kconfig range, right?

It's impossible if the Kconfig range is set from the first start-up of the device and never changes. That's a rather big assumption in my opinion. I can definitely see people tweaking this value as they see fit.

[jrhrsmit]

It's impossible if the Kconfig range is set from the first start-up of the device and never changes. That's a rather big assumption in my opinion. I can definitely see people tweaking this value as they see fit.

Ah true, I've added a check for that on lines 252-256, which should fix that.

[EmilFlach]

Hi @Nicell & @Megamannen!

Could this be merged? I've accidentally set my maximum brightness too high, causing some warnings. This PR would help me out. All feedback was implemented and all checks have passed, any reason not to merge?

[jrhrsmit]

@malinges should be good like this right?

[Nicell]

I like how simple this solution is, but it feels like this min/max conversion of brightness should probably be preprocessed before being sent to the hsb_to_rgb function, as now this function does more than hsb_to_rgb functionality.

[Nicell]

As stated in #944, thanks for your work on this PR @jrhrsmit. I'm happy with the final outcome of this feature. The other PR does lack the Kconfig updates to add ranges and update the descriptions that you've done here, so if you'd like to make a separate PR for that, that'd be awesome!

[jrhrsmit]

Well at least I'm happy we finally have this feature after eight months