QMK RGB saturation bottoms out

Question

RGB keycodes RGB_HUI, RGB_HUD, RGB_SAI, RGB_SAD, RGB_VAI, and RGB_VAD allow me to increment or decrement hue, saturation, and value on my RGB underglow lights. But I wanted to be able to hold down a key to change the color continually. So I made these changes:

In config.h:

#ifdef RGBLIGHT_ENABLE
  #define RGBLIGHT_HUE_STEP 1
  #define RGBLIGHT_SAT_STEP 1
  #define RGBLIGHT_VAL_STEP 1
#endif

And then in keymap.c I made the custom keycodes:

enum custom_keycodes {
    KC_HUI = SAFE_RANGE,
    KC_HUD,
    KC_SAI,
    KC_SAD,
    KC_VAI,
    KC_VAD
};

And defined some variables:

// flags. 0 = no change, 1 = increment, -1 = decrement.
int8_t change_hue = 0;
int8_t change_saturation = 0;
int8_t change_value = 0;

// timers to control color change speed
uint16_t hue_timer = 0;
uint16_t saturation_timer = 0;
uint16_t value_timer = 0;

// seconds it takes to cycle through 0-255 or back
// technically 1 = 1.024 seconds, yielding even multiples of 4ms per tick (1024 / 256).
const int8_t hue_seconds = 5;
const int8_t saturation_seconds = 5;
const int8_t value_seconds = 5;

Then I use the keycodes to set the flags and timers:

bool process_record_user(uint16_t keycode, keyrecord_t *record) {
    switch (keycode) {
        case KC_HUI:
            if (record->event.pressed) {
                hue_timer = timer_read();
                change_hue = 1;
            } else {
                change_hue = 0;
            }
            break;
        case KC_HUD:
            if (record->event.pressed) {
                hue_timer = timer_read();
                change_hue = -1;
            } else {
                change_hue = 0;
            }
            break;
        case KC_SAI:
            if (record->event.pressed) {
                saturation_timer = timer_read();
                change_saturation = 1;
            } else {
                change_saturation = 0;
            }
            break;
        case KC_SAD:
            if (record->event.pressed) {
                saturation_timer = timer_read();
                change_saturation = -1;
            } else {
                change_saturation = 0;
            }
            break;
        case KC_VAI:
            if (record->event.pressed) {
                value_timer = timer_read();
                change_value = 1;
            } else {
                change_value = 0;
            }
            break;
        case KC_VAD:
            if (record->event.pressed) {
                value_timer = timer_read();
                change_value = -1;
            } else {
                change_value = 0;
            }
            break;
    }
    return true;
}

Then I use a matrix scan to change the color using those flags and timers:

void matrix_scan_user(void) {
    switch (change_hue) {
        case 0:
            break;
        case 1:
            if (timer_elapsed(hue_timer) > (hue_seconds * 4)) {
                hue_timer = timer_read();
                rgblight_increase_hue();
            }
            break;
        case -1:
            if (timer_elapsed(hue_timer) > (hue_seconds * 4)) {
                hue_timer = timer_read();
                rgblight_decrease_hue();
            }
            break;
    }
    switch (change_saturation) {
        case 0:
            break;
        case 1:
            if (timer_elapsed(saturation_timer) > (saturation_seconds * 4)) {
                saturation_timer = timer_read();
                rgblight_increase_sat();
            }
            break;
        case -1:
            if (timer_elapsed(saturation_timer) > (saturation_seconds * 4)) {
                saturation_timer = timer_read();
                rgblight_decrease_sat();
            }
            break;
    }
    switch (change_value) {
        case 0:
            break;
        case 1:
            if (timer_elapsed(value_timer) > (value_seconds * 4)) {
                value_timer = timer_read();
                rgblight_increase_val();
            }
            break;
        case -1:
            if (timer_elapsed(value_timer) > (value_seconds * 4)) {
                value_timer = timer_read();
                rgblight_decrease_val();
            }
            break;
    }
}

According to the docs, each of those should wrap around at the max/min hue/saturation/value, respectively.

• That seems to work exactly as expected with KC_HUI and KC_HUD: If I hold the key down long enough it rounds the bend and hue just keeps on cycling through.
• KC_SAI, KC_VAI, and KC_VAD work at least normal-ish. I can increase saturation or value to the max and it will just stop there, and likewise if I decrement the value it stops at zero.
• KC_SAD is the problem. When I hold it down long enough (just past white), the keyboard bombs out. It turns a weird orange color and nothing works.

For reference, here is the GitHub commit where I made all the changes, along with the rest of my code of course.

1. Any idea why KC_SAD would bomb out rather than either stopping or wrapping around?
2. Any idea why both saturation and value just stop, rather than wrapping around as I'd expect based on my read of the docs?
3. Might I do better to try this whole thing a different way (question to more seasoned users)?

Answer 1

This is a really nice bit of functionality!

1. I copied your code into one of my keyboards, and I am not able to reproduce the "bomb out" behavior that you are seeing on KC_SAD.
2. The documentation is incorrect. The increase/decrease functions for saturation and value do not wrap around, and to my knowledge, they are not intended to wrap. They max out at 255, and are bottom limited at 0. I'll be filing a PR to fix the documentation.
3. The only significant problem that I see with your implementation is that it results in very many writes to EEPROM, and that isn't healthy (they wear out over time). You should use, for instance, rgblight_increase_sat_noeeprom() to adjust the saturation (and its counterparts for hue / saturation, and for decrease). Then when you release the key, only then should you write the current HSV setting to EEPROM (using rgblight_sethsv() to set the final value into EEPROM.

Answer 2

I experimented with this some more, and came up with a different implementation which is much more efficient in terms of lines of code and bytes added to the firmware size. Note that instead of defining new custom keycodes, instead it hijacks the existing RGB_... ones for this new and improved purpose:

#include <lib/lib8tion/lib8tion.h>

// flags. 0 = no change, 1 = increment, -1 = decrement.
int8_t change_hue = 0;
int8_t change_sat = 0;
int8_t change_val = 0;

// timer to control color change speed
uint16_t change_timer = 0;
const uint16_t change_tick  = 15;

void matrix_scan_user(void) {
    if (change_hue != 0 || change_val != 0 || change_sat != 0) {
        if (timer_elapsed(change_timer) > change_tick) {
            HSV hsv = rgblight_get_hsv();
            hsv.h += change_hue;
            hsv.s = change_sat > 0 ? qadd8(hsv.s, (uint8_t) change_sat) : qsub8(hsv.s, (uint8_t) -change_sat);
            hsv.v = change_val > 0 ? qadd8(hsv.v, (uint8_t) change_val) : qsub8(hsv.v, (uint8_t) -change_val);
            rgblight_sethsv_noeeprom(hsv.h, hsv.s, hsv.v);
            change_timer = timer_read();
        }
    }
}

bool process_record_user(uint16_t keycode, keyrecord_t *record) {
    if (record->event.pressed) {
        switch (keycode) {

                // clang-format off
            case RGB_HUI: change_timer = timer_read(); change_hue =  1; return false;
            case RGB_HUD: change_timer = timer_read(); change_hue = -1; return false;
            case RGB_SAI: change_timer = timer_read(); change_sat =  1; return false;
            case RGB_SAD: change_timer = timer_read(); change_sat = -1; return false;
            case RGB_VAI: change_timer = timer_read(); change_val =  1; return false;
            case RGB_VAD: change_timer = timer_read(); change_val = -1; return false;
                // clang-format on
        }
    } else {
        bool rgb_done = false;
        switch (keycode) {
            case RGB_HUI:
            case RGB_HUD:
                change_hue = 0;
                rgb_done   = true;
                break;
            case RGB_SAI:
            case RGB_SAD:
                change_sat = 0;
                rgb_done   = true;
                break;
            case RGB_VAI:
            case RGB_VAD:
                change_val = 0;
                rgb_done   = true;
                break;
        }

        if (rgb_done) {
            HSV final = rgblight_get_hsv();
            rgblight_sethsv(final.h, final.s, final.v);
        }
    }

    return true;
}