RGB Matrix: Ability to define custom effects from keymaps

[daniel5151]

The current method of creating custom lighting effects through RGB Matrix from user keymaps is by calling rgb_matrix_set_color from methods such as process_user, matrix_scan_user, or rgb_matrix_indicators_user. While this works fine for overriding a few LEDs here-and-there (to indicate caps-lock, or indicating the current layer), it's quite inefficient when overriding all LEDs, since RGB Matrix is still running it's own effects in the background.

For example, my preferred lighting setup is a smooth red-orange gradient, with a on-keypress fade effect from green to the underlying color. Since there is no built-in RGB Matrix effect that can do this, I would have to write my own. This leaves me with 2 options:

1. Write it in rgb_matrix.c directly

• Since this is a fixed-effect, tailored to my specific tastes, it doesn't make sense to merge it upstream. Thus, I'd have to maintain a branch of QMK with my special effect

2. Use the hacky method outlined above

• While localized to the keymap, it's inefficient, and the implementation would be pretty jank. Moreover, if I wanted to still have access to the other lighting effects in RGB Matrix (because who doesn't want to flex on their friends sometime with a rainbow-keyboard 😉), I'd have wire up some extra logic to disable my overrides. Yeah, it works, but it's not pretty.

Instead, it would be nice if there was a way to extend the built-in effects provided by RGB Matrix with new ones, and do so in a efficient and ergonomic manner.

---

I've actually taken a stab at this already (in the ill-fated PR #5096), and I'd like to reintroduce the feature in it's own standalone PR.

My proposed interface would be something akin to the following:

// rgb_matrix.h
// ...
typedef void (*rgb_matrix_custom_mode_f)(bool init);
extern const rgb_matrix_custom_mode_f* rgb_matrix_custom_modes;
extern const uint8_t rgb_matrix_custom_modes_count;

// rgb_matrix.c
// ...
__attribute__((weak))
const rgb_matrix_custom_mode_f* rgb_matrix_custom_modes = NULL;

__attribute__((weak))
const uint8_t rgb_matrix_custom_modes_count = 0;

Users could then write their own effects, export those symbols, and have them show up in RGB Matrix like any other effect. i.e: RGB_TOG would cycle through the built-in effects, and once it reaches the end of the built-in effects, it would continue cycling through any custom-defined effects.

A simple example of how one might use this interface in a keymap:

// keymap.c
// ...
static void rgb_matrix_custom(bool init) {
  for (int led_i = 0; led_i < DRIVER_LED_TOTAL; led_i++) {
    // a bunch of fancy logic
    rgb_matrix_set_color(led_i, rgb.r, rgb.g, rgb.b);
  }
}

static const rgb_matrix_custom_mode_f custom_modes[] = {
    rgb_matrix_custom,
};

// override null-defns in rgb_matrix.c
const rgb_matrix_custom_mode_f* rgb_matrix_custom_modes = custom_modes;
const uint8_t rgb_matrix_custom_modes_count = sizeof(custom_modes) / sizeof(custom_modes[0]);

---

I'll be putting up a PR soon-ish with the actual implementation, but I just wanted to throw up an issue first (to reference in the PR)

[e11i0t23]

Hi,

I have read through your proposal and there is one part i think should be adjusted to male it easier for a user to add RGB effects, this is the overrides in keymap.c, would there be a way where we could automatically do this say by the use of an ifdef or just an if/else statement so that when the user has rgb_matrix_custom_mode_f_custom_modes[] it automatically overides.

Im also not sure whether that name is quite suitable or should be more like rgb_matrix_custom_user and also therefore possibly should have a rgb_matrix_custom_kb to follow the naming scheme for kb/user.

Other than those two aspects this is a feature that i personally see good use for however also needs to be made simpler for all drivers similar to what was said in your original PR

[daniel5151]

Thanks for the feedback!

rgb_matrix_custom_kb and rgb_matrix_custom_user are definately better names. Plus, it does make sense to have custom effects on a per-keyboard level too (not just a per-user level)

I'm a bit confused by what you mean when you say you want overrides to be done "automatically," or how a ifdef-based system might work. With the current proposal, things are already pretty automatic, since once the user defines the two variables (the array, and the length), things would "just work."

Could you elaborate on what you had in mind?

[e11i0t23]

Hi, apologies, when i was on mobile it looked like there where 4/5 different varibles a user had to define however as its only two ignore me about that as 2 variables are fine.

[XScorpion2]

@daniel5151 I really like where you are going with this approach and think we should also apply a similar array / function pointer system to rgb matrix for the base effects eventually. I have a few thoughts on the approach:

1. It might be better to use a define instead of a const field to define the array size as then we can use that define to predefine the necessary arrays, loops, etc at compile time instead of runtime if the compiler fails on inline of the const count.
2. Not a big fan of the pointer to array of pointers, I get that the array could be not implemented thus needing to check for null, but couldn't we just have a default empty array in that case? This would reduce null checking.
3. Thinking a bit more along the lines of 2, if we update rgb_matrix to use an array of function pointers as well, maybe we could just all users to append their custom anim functions to that array instead as part of an init matrix call. rgb_matrix-init_user(rgb_matrix_anims* animArray, uint8_t size) size in that array would be base anim enabled count + keyboard / user defined custom anim count.
4. Should animations have their own interface similar to LED drivers? This way they have a defined separate init & tick functions.

[daniel5151]

Glad to hear! I definitely think that an array of effect pointers would be nicer than the current switch-statement based system (I hate the sea of ifdef-endifs the current system requires haha)

1. My rationale for using a const over a define was twofold: 1) it avoids the case where a user accidentally forgets to update the define when adding/removing effects (assuming they stick to using sizeof(a)/sizeof(a[0]), and 2) it localizes all custom effect code in a single file (as opposed to having a define present in the config headers, and the effects themselves in the keymap.c). That said, having a define is probably a better idea, even if it is less ergonomic, given the optimization opportunities is opens up.
2. I recall running into some pretty funky compiler errors when using weak linking alongsize zero-length arrays. I can't remember the exact issues I ran into, but they were weird enough that past-me decided to switched over to having a separate pointer+length based system. Feel free to sanity check me on this.
3. Having a single contiguous array for builtin+custom effects would be great, since it makes the effect-cycling logic in rgb_matrix.c really easy. Having separate arrays would result is some pretty nasty logic to iterate through effects, as you would have to simulate a single contiguous array by maintaining multiple counters (see rgb_matrix.c in RGB Matrix Overhaul + support for Massdrop ALT/CTRL #5096 for what I mean). My one qualm with this approach is that it does push some boilerplate into userspace, since I'd assume that all rgb_matrix_init_user methods will end up looking almost identical (push some function pointers into the animArray). And yes, while this method could serve double-duty to also initialize custom effect states, I think having each effect accept a bool init is a better way to do that, since it localizes effect instantiation logic to each effect.
4. Hmm, what would be the benefit of having a separate tick function for custom effects? What would it be responsible for, given that effects themselves would dispatched through function-pointers?

[XScorpion2]

1. This was just a thought, honestly it could go either way in my mind, it just depends on ease of use for all involved. Keyboard Developers, Users, and Devs all need to be able to do the right thing easily.
2. If we move rgb_matrix anims into the same array, I doubt it will every be 0 length so I don't think this will be much of an issue.
3. Not sure I understand what you mean by serve double-duty to also initialize custom effect states and how that goes back to the rgb_matrix_init_user and by keyboard extension rgb_matrix_init_kb functions. As for the duplicated boiler plate, I suspect the rgb_matrix_init_kb version would implement what they need, and the rgb_matrix_init_user would be for user specific versions. Not sure I see where duplicated boiler plate code would be in this case.
4. Today we have several effects that take a bool into the tick functions to tell the anim that we switched from a previous effect to this one. So do stuff. Look at the raindrop animations, they initialize the first time by randomizing the rgbs of all the leds. The idea was that this logic shouldn't be part of the tick code imo. Maybe it's not bad but still not very ideal either. Another longer term crazy idea I had was that we could pass in a fixed size void* buffer for the animation to use as a reusable scratchpad that would get wiped out when we switch to a new animation so each animation wouldn't need to use global fields to store data they need to keep tick tick. It's a bit dangerous being a void* fixed buffer though expecting users to use a blank page correctly though.

[daniel5151]

1. Eh, it's not a critical issue. I can draft up an implementation, and we can run some benchmarks to see what works best
2. Like I said, I forget why I did it. I'll end up rewriting the implementation anyhow, so we'll see ¯\_(ツ)_/¯
3. I'm assuming the methods will end up looking an awful lot like:

static void my_effect(bool init) { /**/ }
static void another_effect(bool init) { /**/ }
void rgb_matrix_init_user(rgb_matrix_anims* anims, uint8_t size) {
  anims[0] = my_effect;
  anims[1] = another_effect;
}

If that's the case, then it might make sense to just stick with defining an array.

As for the double-duty thing, I just reread my comment, and I realize I go tripped up on rgb_matrix subsystem instantiation, and rgb_matrix effect instantiation, so disregard my comments

4. Hmm, I'm not sure if a separate init method is the best idea though. Consider all the simple effects that don't require instantiation (i.e: most existing effects). Would each of them specify a NULL init method, and have rgb_matrix run a null-check before calling it? Is it worth the space and cognitive overhead?
   Plus, the separation-of-concerns issue can be solved on a per effect level by having complicated effect do something like:

static inline void my_complex_effect_init() { /**/ }
static inline void my_complex_effect_tick() { /**/ }

void my_complex_effect(bool init) {
  if (init) my_complex_effect_init();
  my_complex_effect_tick();
}

Which the compiler should properly inline.
This is actually what I did with my Snake game (see my keymap from the original PR)

Your void* scratchpad idea intrigues me...
Maybe there is some way to mitigate user error by using some sort of union-based system, where custom effects use a #define to specify the struct they want to use, and through some fancy macro hackery, it gets union'd with all the other effect's scratchpads? I'm just spitballin' here, I haven't thought too much about the details, but it might be possible.

[daniel5151]

Ahh, I remember why I was running into issues when using the weakly-linked arrays!

Consider this (super simplified) scenario:

// somewhere in rgb_matrix.c
if (rgb_matrix_effects_user_count > 0) rgb_matrix_effects_user[i]();

// but later on, in the same file
__attribute__ ((weak))
const rgb_matrix_custom_effect_f rgb_matrix_effects_user[] = {};

__attribute__ ((weak))
const uint8_t rgb_matrix_effects_user_count[] = 0;

Even if the symbol is redefined to a non-empty array, the compiler will still emit the following warning (warnings = errors in QMK):

warning: array subscript is above array bounds [-Warray-bounds]
   if (rgb_matrix_effects_user_count > 0) rgb_matrix_effects_user[i]();
                                          ~~~~~~~~~~~~~~~~~~~~~~~^~~

It's not smart enough to recognize the check / see the weak linking, and prevents things from compiling.

After playing around with it a bit more, it looks a workaround for this issue would be to move those default weak instantiations into a separate compilation unit (something like rgb_matrix_custom_effects.c). It's not great, but works.

[drashna]

I think this can be closed.