RGB Matrix: Custom effects on a kb/user level

[daniel5151]

The current method of creating custom RGB Matrix lighting effects from kb/user is by calling rgb_matrix_set_color from methods such as process_kb/user, matrix_scan_kb/user, or rgb_matrix_indicators_kb/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.

This PR introduces a cleaner, more unobtrusive way to extend the RGB Matrix system, adding new effects and having them act just like built in ones (i.e: accessed through RGB_TOG)

Description

See discussion below and updated docs for usage info.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

• RGB Matrix: Ability to define custom effects from keymaps  #5329

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[drashna]

if possible, could you add documentation for this, as well as an example of how to utilize this?

[daniel5151]

Due to the in-flux nature of the interface, I was going to write the Docs once the implementation was settled-on.

There is a brief example in the PR's body though, if you want to play around with it yourself.

[Lenbok]

I think it would be great if both rgb matrix and rgb underglow had more commonality in terms of how the user specifies which modes they want enabled in their firmware. Underglow is via defines which lets you disable those you aren't interested in but currently has no mechanism to add user modes, so something like this would be useful there.

Does this PR let you disable preexisting modes you don't like? (And ideally reclaim the firmware space)

[yanfali]

Not a huge fan of

const uint8_t rgb_matrix_effects_kb_count;
const uint8_t rgb_matrix_effects_user_count;

is there some way to look at size of array at compile time and avoid the manual book keeping?

[daniel5151]

@Lenbok I agree, but I don't think those changes fall under the scope of this PR.
AFAIK, RGB Matrix already has a way to disable individual effects using defines, see the docs here. That said, disabling the effect doesn't actually reclaim any firmware space, which is silly, and should be fixed at some point.

@yanfali I agree, it's not great, but unfortunately, the size of arrays can't be easily reconstructed in another compilation unit when using weakly-linked symbols. Some sort of count has to be manually maintained.

One potential alternative to get around this is to introduce a helper macro, something like the following:

#define EXPORT_RGB_MATRIX_EFFECTS_USER(...) \
  const rgb_matrix_effect_f rgb_matrix_effects_user[] = { __VA_ARGS__ }; \
  const uint8_t rgb_matrix_effects_user_count = sizeof(rgb_matrix_effects_user)/sizeof(rgb_matrix_effects_user[0]);
// same thing for KB

// usage (in keymap.c):
static void my_kool_effect(bool init) {}
static void my_kool_effect2(bool init) {}

EXPORT_RGB_MATRIX_EFFECTS_USER(
  my_kool_effect, 
  my_kool_effect2
);

Thoughts on this Macro based approach?

[yanfali]

@daniel5151 if it's less error prone for users I think it's probably worth considering. Managing a count for something that you've already defined just seems like a recipe for people coming to the discord help channel.

[XScorpion2]

So I applied this function pointer approach to rgb_matrix (branch here) as a test and here's my notes

• Function pointer array & array initialization takes up more firmware space than the switch statement does (~300 bytes, this seems very wrong, but I double checked it...will triple check later).
• It's extremely easy for a user to end up out of bounds, by accessing a null element or out of bounds.
• rgb_matrix_config.mode is allowed, and valid to be out of bounds for certain values, and the matrix code controls eeprom reading of the value. So changing keymap could cause eeprom value to be invalid after flashing.

I wonder if an approach similar to keycodes are handled might not be a safer & smaller solution. ex: bool process_rgb_effect(uint16_t effectcode). I still REALLY like the idea of not having a giant switch statement, but not sure the trade offs are worth it.

[daniel5151]

• ~300 bytes definitely seems like a lot. It can't imagine what changes could result in such a large codesize increase. We should look into this further at some point.
• OOB issues can be mitigated with the automatic bookkeeping macro (as proposed above), and with some sanity-checks in the rgb_matrix init methods (i.e: when reading from eeprom).

Just to clarify something though, this PR isn't aiming to do a full function-pointer conversion.
So while this exploration is useful, and will certainly help guide how we approach refactoring the system in the future, I'm not sure it's findings are relevant to this PR in particular.

Moreover, I'd just like to stress that this PR doesn't bloat the firmware size, given that it merely adds a couple of hooks, exports a couple of symbols, and tweaks some statements here and there.

[XScorpion2]

So I think the PR implementation is great, and want to approve it, but I think it is worth discussing the pros and cons a bit more and the limitations or issues of this approach is all.

• As for how my conversion test is relevant, it gives a comparison of similar approaches to a known problem: how to pick and call the correct animation function. In this pr case though, if a qmk user had the choice between the two options, function pointers vs switches with the trade offs that switches being smaller firmware size, tiny perf hit, I think it will depend on the board, there are many that are at the wire today for firmware sizes.
• As for OOB, yes it can be mitigated with book keeping, though we really should move certain globals to statics so there is less of an issue in outside manipulation. Mainly I was just bringing this up to ensure this part of the code is reviewed closely.

I'm going to look at the assembly and see what the hell caused that size increase, cause that is just crazy.

[daniel5151]

I've gone ahead an added in the helper macros as discussed above, along with some OOB checks that should prevent any nastiness when removing custom effects on boards with EEPROM.
These new helper macros and OOB checks make it a lot harder for users to shoot themselves in the foot, though please double check that I haven't missed any other obvious footguns :)

I've added docs on how to define custom effects, and also updated the PR's description to reflect the new recommended usage. There hasn't been too much pushback on this current design (weakly-linked array+len with bookkeeping macros), so I think we can push forward with it.

[XScorpion2]

Will be looking at the size increase a bit more this weekend, but wanted to follow back up before then with a request for a small api change on the function pointer:

typedef bool (*rgb_matrix_effect_f)(bool init, uint8_t iter);

This api change is needed to allow for rendering of a single frame over several update ticks. The animation function returns true if it needs to do more work, false if not. iter is a count of how many times it has been called while it still needed more work to be done. This allows us to break up calculating the led data over several ticks instead of all at once potentially causing scanning delays.

[daniel5151]

SGTM.

Could you give an example of how this should be used in the custom effect fn + how the the effects should be called from rgb_matrix.c? I'll update the API + docs accordingly.

[XScorpion2]

I still need to verify some ideas around this, task to do this weekend, but here's an example:

bool rgb_matrix_solid_splash(bool init, uint8_t iter) {
  // solid_splash is expensive, we know it ranges from 4ms to 10ms
  // so we want it to iterate 2 to 5 times based on the number of items we are tracking
  // So setup the range we need to operate over
  uint8_t count = 2 + (g_last_hit_tracker.count + 3 - 1) / 3;
  uint8_t led_count = (DRIVER_LED_TOTAL + count - 1) / count;
  uint8_t led_min = led_count * iter;
  uint8_t led_max = led_min + led_count;
  if (led_max > DRIVER_LED_TOTAL)
    led_max = DRIVER_LED_TOTAL;

  for (uint8_t i = led_min; i < led_max; i++) {
    // Do work on this led range
  }
  return led_max < DRIVER_LED_TOTAL;
}

The idea is that the animation time slices itself base on it's own knowledge of it's performance characteristics at the time of being called. It will do work each tick until it is complete, and once it is complete, the buffers (rgb_config_t, ticks, etc) that drive the animations will then be updated via a memcpy from an internal double buffer. Most of this loop setup can be extracted out into a utility function with the correct parameters for easier use in custom animations. So that way an animation like solid can just run normally without doing anything special, but the heavier ones can be designed in a way that lets them scale.

[daniel5151]

Ah, gotcha.

My one concern would be the feature-creep that this change entails. I don't think this PR should be the one to implement the (what I assume to be) non-trivial logic to support the returned bool + uint8_t iter parameter (i.e: the double-buffering of environment variables, iteration counting, etc.).

I don't mind changing the fn signature as a future-proofing measure for once your RGB Matrix overhaul PR lands, though I think it might be best to stub-out the extended functionality this modified API suggests, and add a note that the bool return value + uint8_t iter are ignored/unused at the moment.

Thoughts?

[XScorpion2]

Ya, I was thinking the same. Stub it out until a future PR lands with the work. I don't think my matrix refactor will contain this work either as it's already fairly long too. But I do want to use that refactor as a base for this timeslice implementation

[XScorpion2]

@daniel5151 Here's the final implementation I ended up with:
52055b9

[daniel5151]

@drashna status update on potentially getting this merged?

The interface has been agreed upon and stabilized, and the docs have been updated accordingly 😄

[drashna]

I'm waiting on other people to approve the changes here, actually.

[XScorpion2]

@daniel5151 mind updating the API one more time:

typedef struct {
  uint8_t iter;
  uint8_t flags;
  bool init; // debating on if this should be rolled into flags
} effect_params_t;

typedef bool (*rgb_matrix_effect_f)(effect_params_t* params);

The idea behind this change is so that, one, we can update animation parameters as needed without changing a bunch of code, and two, the additional flags parameter will go a long ways to allowing different animations running on different sets of LEDs.

For example, using flags I can mask out LEDs to have breathing effect on modifiers, cycle left to right on underglow, and reactive on the remainder.

I'll be stubbing it out in the rgb overhaul pr later tonight.

[daniel5151]

I was thinking about that single-macro approach, and while slick, I thought it was a bit too magical. I've gone ahead and pushed what I think is a good compromise between compactness and readability.

I'll copy-paste the relevant excerpt from the updated docs here:

---

By setting RGB_MATRIX_CUSTOM_USER (and/or RGB_MATRIX_CUSTOM_KB) in rule.mk, new effects can be defined directly from userspace, without having to edit any QMK core files.

To declare new effects, create a new rgb_matrix_user/kb.inc that looks something like this:

// !!! DO NOT ADD #pragma once !!! //

// Step 1.
// Declare custom effects using the RGB_MATRIX_EFFECT macro
// (note the lack of semicolon after the macro!)
RGB_MATRIX_EFFECT(my_cool_effect)
RGB_MATRIX_EFFECT(my_cool_effect2)

// Step 2.
// Define effects inside the `RGB_MATRIX_CUSTOM_EFFECT_IMPLS` ifdef block
#ifdef RGB_MATRIX_CUSTOM_EFFECT_IMPLS

// e.g: A simple effect, self-contained within a single method
static bool my_cool_effect(effect_params_t* params) {
  RGB_MATRIX_USE_LIMITS(led_min, led_max);
  for (uint8_t i = led_min; i < led_max; i++) {
    rgb_matrix_set_color(i, 0xff, 0xff, 0x00);
  }
  return led_max < DRIVER_LED_TOTAL;
}

// e.g: A more complex effect, relying on external methods and state, with
// dedicated init and run methods
static uint8_t some_global_state;
static void my_cool_effect2_complex_init(effect_params_t* params) {
  some_global_state = 1;
}
static bool my_cool_effect2_complex_run(effect_params_t* params) {
  RGB_MATRIX_USE_LIMITS(led_min, led_max);
  for (uint8_t i = led_min; i < led_max; i++) {
    rgb_matrix_set_color(i, 0xff, some_global_state++, 0xff);
  }

  return led_max < DRIVER_LED_TOTAL;
}
static bool my_cool_effect2(effect_params_t* params) {
  if (params->init) my_cool_effect2_complex_init(params);
  return my_cool_effect2_complex_run(params);
}

#endif // RGB_MATRIX_CUSTOM_EFFECT_IMPLS

[XScorpion2]

Looking good!

[drashna]

Well, one concern I have:

To make sure, the inc can be "anywhere" that is in the VPATH for QMK?
Or is there a specific folder that they will need to be in?

From the documentation, it's not exactly clear (or I am an idiot and completely missed it)

[daniel5151]

As long as #include "rgb_matrix_user.inc" can be resolved correctly, it will work. So yes, anywhere in the VPATH (though for consistency, kb.inc should go in the keyboard folder, and user.inc should be in the keymap folder.

I can update the documentation to make that more clear.

[daniel5151]

Heyo, any updates?

Also, I think we can remove the needs-docs tag 😄

[XScorpion2]

@drashna anything left before merging this pr?

[daniel5151]

Awesome! Great work everyone!

Hopefully we'll see some pretty interesting new lighting effects popping up in user-space now 😄