qmk / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
18.28k stars 39.38k forks source link

RGB Underglow command issue on matrix init #2623

Closed drashna closed 6 years ago

drashna commented 6 years ago

Specifically, when calling rgblight_setrgb on matrix_init_*, it doesn't seem to honor it if you change it later on.

The Ergodox EZ has code that does this, and I can confirm that it "has issues".

I do this as well, but I use rgblight_sethsv which doesn't have this issue.

Calling rgblight_enable or rgblight_init doesn't help here. It still exhibits this behavior. So there is some issue with the actual command and how it's called ... and I'm not sure what it is.

fauxpark commented 6 years ago

rgblight_setrgb doesn't seem to call eeconfig_update_rgblight as rgblight_sethsv does:

void rgblight_setrgb(uint8_t r, uint8_t g, uint8_t b) {
  if (!rgblight_config.enable) { return; }

  for (uint8_t i = 0; i < RGBLED_NUM; i++) {
    led[i].r = r;
    led[i].g = g;
    led[i].b = b;
  }
  rgblight_set();
}

As far as I can tell it is not used to create animations so I think it should be safe-ish to just add it in.

drashna commented 6 years ago

More complicated than that, unfortunately. I tested "just adding it", and ... it disables the lights after a flash.

And the eeconfig command requires HSV values, so it needs to be converted.

But I think this is in the right direction.

drashna commented 6 years ago

On the plus side, found a bug in the RGB Sleep code ... Issued PR to fix.

Also, for layer indication, maybe it would be best to use _sethsv instead.

colemarkham commented 6 years ago

I was looking over this, and it seems that rgblight_setrgb is missing the extra functionality that rgblight_sethsv has, as @fauxpark mentioned. In addition to not setting the values to eeprom, it also doesn't set them to rgblight_config. Perhaps rgblight_setrgb should just convert to HSV and call rgblight_sethsv?

fauxpark commented 6 years ago

rgblight_setrgb is called at the end of rgblight_sethsv_noeeprom, which is used by rgblight_sethsv and the breathing and rainbow animations. It seems it is used in place of sethsv (used in the "rainbow swirl" effect) because it does not require a LED position and those two animations only need to set all LEDs at once. They can be modified to instead use sethsv as the other animations do.

IMO the "main" methods should be rgblight_setrgb/_noeeprom/_at and rgblight_sethsv/_noeeprom/_at, where only one set actually does the work and the other just converts and calls the first. The animations can then call upon these as necessary instead of hacking at the LED array.

drashna commented 6 years ago

@fauxpark I finally had a chance to really sit down and look at this.... and learn enough to understand what is going on here. rgblight_sethsv_noeeprom has some issues, and the inmem_config structure is never actually used anywhere.

I've cleaned up the code, and added a bunch of noeeprom functions in #3070. That should fix this here, since I can then use the hsv and mode without writing to eeprom.

In fact, I've tested this out already and it works great.

drashna commented 6 years ago

Change is merged now. using rgblight_sethsv_noeeprom works best here, now.