RGB Matrix Overhaul + support for Massdrop ALT/CTRL #5096
Conversation
This is a _big one_. The base changes come from @helluvamatt, who got the CTRL working with RGB Matrix. I built on-top of that, both by adding support for the ALT, and by cleaning up the code. I had to rebase his changes on the newest master, which was also fun. Based on my testing, it seems to be working great! All the effects are performing as expected. I've updated the default keymaps for both keyboards to use the new RGB Matrix keycodes. That said, these changes do break some current keymaps, namely `alt/reywood` and `ctrl/responsive_pattern`. These keympas hooked into the custom massdrop lighting system, and need to be updated to work with RGB Matrix. That said, this commit _also_ introduces the ability to extend RGB Matrix lighting modes from keymaps! See alt/prilik/rgb_matrix_custom.c for examples of this!
the logic for cycling into/outof custom modes is surprisingly tricky. Seems to be working fine now though.
RGB Matrix handles brightness by operating on the RGB values themselves, though the Massdrop GCR code also tweaks the brightness. Since gcr_desired isn't being directly called anymore, the max brightness was set less than the desired max. It might be worth tweaking RGB Matrix to use gcr_desired, though it's not critical.
accidentally used MAX_X instead of MAX_Y when calculating the points, resulting in really low Y values. All good now though.
I ported over the system used in the Massdrop qmk fork, hidden behind the USE_MASSDROP_CONFIGURATOR preproccessor directive. Adding EXTRAFLAGS += -DUSE_MASSDROP_CONFIGURATOR enables the functionality. While this works, ideally, the Massdrop configurator should be rewritten to use RGB_Matrix's patterns. The only tricky bit might be figuring out how to support LED_FLAG_USE_PATTERN, as by default, RGB Matrix patterns are invoked on _all_ LEDs. That said, RGB Matrix _should_ switch to a system of batched LED updates. This would involve updating all existing patterns to operate on a per-LED basis, which would make it a lot easier to switch off the legacy system.
not sure why I removed this function... well, it's back now. no harm no foul?
daniel5151
force-pushed
the
massdrop_rgb_matrix
branch
from
ee1154d to
ea63624
Compare
|
These lines from the end of
should be added to the end of As tested on my CTRL keyboard, the current code causes animations to run too fast, and doesn't enable the 4-stage on/off toggle functionality implemented in this PR. Both are fixed by the above change. Nice work otherwise! |
|
Note I have only done testing on the CTRL as it is the more demanding than the ALT. This update has major impacts on the original timings for the keyboard in its current state. The main reason for this is the 10ms timer for the LED processing is removed and flush() is called very often. Because of this, a call while the I2C queue is running will wait 20ms thinking the queue has an issue and needs more time to resolve itself, and induce a 20ms delay in every other task such as power management (5ms timer which will be going to 1ms in a future update) and key matrix scanning. This is not a standard situation to be in and through designed timings and queue size, is not intended to ever be in. To fix this, be sure flush() is only called once every 10ms. For example in led_matrix.c at void flush(void): This will get power management and key scanning working more as they should, but they are still affected due to what it seems RGB calculations being done in one shot rather than chunks, not allowing time for other time sensitive code. I've seen up to 80ms of time between LED updates for some animations. Even with the above update, there are issues with animation speed, brightness variations, and LED driver resets due to overloads especially with white patterns. An area still working as intended is the I2C for LED updates running 6ms for two full driver updates through DMA. I have not gotten around to the backwards compatibility code for the Massdrop Configurator. |
this is a big one, and _might_ break other keyboards. I've tweaked all the effects in RGB Matrix to take a led index as an argument. This is better than the old system, since it allows implementing batced LED updates, and also opens up the possibility of having multiple effects running simultaneously! I've also tweaked how the RGB_MATRIX_SKIP_FRAMES parameter works, having it determine the number of ms between LED updates + flushes. For the ALT/CTRL, it's set to 10, and matches the behavior suggested by @patrickmt (hopefully fixing timings) Lastly, I've also tweaked two RGB Matrix implementation details: 1) calls to rgb_matrix_flush_pwm_buffers method have been moved from quantum.c and into rgb_matrix_task(). There was no reason not to, and it made the code cleaner. 2) the RGB Matrix driver no longer requires a set_color_all method, since under the hood, every single driver just looped through all LEDs and called set_color on each one. Moreover, it wouldn't make sense to have such a method anymore, since all LEDs are updated on and individual basis.
|
For the IS31FL3733 driver, it may be worth looking at the IS31FL3731 driver, and comparing the differences. |
daniel5151
force-pushed
the
massdrop_rgb_matrix
branch
from
ac108e8 to
cd3db5f
Compare
|
This last commit is a doozy, and changes things in RGB Matrix quite a bit. I've tweaked all the effects in RGB Matrix to take a led index as an argument. This is better than the old system, since it opens up the door to implementing batched LED updates and the ability to have multiple effects running simultaneously! It's a lot of changes, but they are, for the most part, fairly mechanical, the exception being There are also several smaller code-cleanliness tweaks I've made along the way. NOTE: This commit tweaks the RGB Matrix timings! It might have broken some keyboards / layouts! |
Why not just use the HSV struct as a member in rgb_config_t instead of having 3 seperate, differently named members to do the same thing? Also, instead of having each individual effect decide if it wants to respect the speed config, I tweaked it so that the speed config directly changes how quickly g_tick increments. This cleans up a lot of effect code, and makes the setting consistent across effects. I've also modified my personal keymap a bit. I've removed some ALT specific tables / timings from the snake effect's code, so theoretically, it could be moved into rgb_matrix.c and have it work across any supported keyboard! I might do so in a future commit.
|
Specifically, because of how the hardware is set up, it can't actually handle running at full speed, and basically makes the board unusable. If need be, I can test this out on my board, and let you know. |
|
To elaborate, I tweaked RGB_MATRIX_SKIP_FRAMES to represent the number of ms between RGB Matrix updates, specifically because the CTRL can't handle RGB Matrix updates every loop. The purpose of the flag hasn't changed, just the implementation. The specific value of RGB_MATRIX_SKIP_FRAMES might need to be bumped / reduced on a board-by-board basis to achieve the same speed / performance as the old implementation. Determining by how much to bump this value will require some testing. If you could test out the changes and tell me how it runs, that would be awesome! And if it doesn't work out of the box, see what happens if you bump RGB_MATRIX_SKIP_FRAMES in the board's config. Push comes to shove, I can add in a compatibility flag, though I think my timer-based implementation is cleaner than tying updates to the specific execution speeds of boards themselves. |
|
Okay. Well, one issue, is that you have removed I'm not sure this worked, but .... |
|
Wow, okay. This is a significant change to how the RGB Matrix feature works. For instance, you drastically changed how I'd really like to see documentation for how And worse, this outright breaks RGB Matrix for both my ergodox EZ and for the Planck Light. I tested with both my layouts, and with the defaults. Lights never turn on. :( |
|
Yep, I thought that would happen... Retrofitting the RGB Matrix system to work with the ALT/CTRL has been non-trivial, and has resulted in some pretty massive modifications to the RGB Matrix. Hell, you might even want to rename the PR, since a lot of it has ended up being RGB Matrix refactoring! The biggest change, as mentioned above, was changing the effects to run on each led individually (by passing the For some background: Why even implement this change?
So, action items:
I'll try to get keymaps working again. I don't want testers worrying about broken builds, only timing issues. |
|
Just to clarify, it breaks even the default layouts, with rgb matrix. As for testing all, Though, a really good test for this is to just make my keymaps.
|
|
I can only comment on the Massdrop CTRL, but currently it seems that almost none of the In addition, the keycodes And as usual, multisplash is very slow (but that is not unexpected). I'm very excited to see where this goes though! |
|
Just tested this on my ALT, and it seems to work very well. On a side-note, I have an interesting issue when I use the ALT with my Dell XPS 15 laptop. The keyboard will restart after about 10 seconds if I have the lighting level set at more than a couple of steps above the minimum level. I'm a bit afraid of frying my pcb or USB ports, so I've set the RGB to off by default for now. |
|
This is exciting!! Thanks @daniel5151 for the PR, and thank you @drashna for the awesome testing! |
|
@MatthewRobo I'm not sure what you mean that none of the effects are working in // in keymap.c
#include "rgb_matrix.h"
void rgb_matrix_breathing(uint16_t led_i); // declare fn prototype
void rgb_matrix_indicators_user(uint16_t led_i) {
rgb_matrix_breathing(led_i);
}I'll take a look at exporting the effects, since it makes sense to have them externally visible and available to use. Also, I agree about that the Hue should be wrapping, I'll change that in a upcoming commit. @FSund Interesting... not sure why that might be happening, especially since the max brightness is set well-below the maximum (iirc, 165 vs. a max of 255). I've been using my ALT with my workstation and laptop (Aero 15X) with no issues for weeks now. When @patrickmt gets back to me about CTRL timings, it might shed some light on the issue. |
|
@daniel5151 It was based on some chat on discord, actually. As for the hue wrapping around, the RGB Matrix code uses 255 as the max value, not 360, which bugs me to no end. It's not a huge difference, but it means that the rgb light and rgb matrix code handles hue slightly different. As for the effects, there is a PR that is aiming and "exporting" the effects to separate files, though I'm not sure how I feel about that. Also, could make the effects more modular. Instead of just taking the LED value, have an option for hue, as well. And then just pass |
|
Oh dang, didn't even realize there was a discord lol. I'll go ahead and join it, though I doubt I'll be keeping a close eye on it. Feel free to DM / mention me, and I'll try to respond in a timely manner :D I can see why Hue should be 0-360. 255 may be more natural for a CPU (just a standard uint8_t), but 360 makes more sense from a colorspace standpoint. Note: this PR changes As for splitting effects into separate files, that might be a bit overkill. A single separate file (rgb_matrix_effects.c/h) with all built-in effects might make more sense. I have no problem lumping that change in with this overhaul, since it's a pretty simple change. I definitely agree that more modular effects would be good. Global variables typically aren't the best idea :). Since I'm already mucking about with the system, I could take a stab at getting that implemented, though I'm not sure when I'll get around to that (or if it'd be part of this PR). |
Grouped related defns, removed unused defns, and exported builtin rgb effects. Also made RGB_HUI/D wrap.
daniel5151
force-pushed
the
massdrop_rgb_matrix
branch
2 times, most recently
from
da336e5 to
e43046b
Compare
I've done a quick pass through the effects, cleaning them up to use some of the new constructs I've introduced (e.g: helper functions to set LED color given an RGB / HSV struct). I've also optimized some of the effects that use sin/cos + g_tick to only run once a frame (i.e: when led_i == 0), which improves performance on weaker boards. I've also removed RGB_MATRIX_KEYRELEASES, since nobody uses it, and it does real bad stuff when enabled.
daniel5151
force-pushed
the
massdrop_rgb_matrix
branch
from
e43046b to
34f5b63
Compare
|
Here is my commit to get the Massdrop lighting support working. This is all looking very good! For testing, one can compile the default keymap for QMK RGB, and default_md for Massdrop lighting. |
|
I'm gone for the weekend, but at a cursory glance those changes look good! |
|
Also, not necessarily part of this PR, but later, would it be possible to incorporate the "Velocikey" feature into the matrix? Also, I think you've already handled this, but this PR may be worth incorporating: |
|
Hmm, I've noticed that the changes here actually reverse the direction of some of the animations on my boards (planck light and ergodox ez). It's not a major thing, but figured I should mention it. Edit: Also, I should note that with this changes, it causes some issue with the ergodox EZ's debouncing. I don't see the issue on the Planck Light, so I'm pretty sure it's related to the slowness of the ergodox's hardware and matrix. Previously, I had my debouncing set to 5ms, but even that was too much. Disabling it outright (eg, setting it to 0) seems to have fixed the issue for me. My guess is that the way the rgb matrix is being processed adds enough of a delay, that it effectively acts as debouncing, but I can't be 100% sure about that. So if @ezuk agrees, it may be worth changing the debounce, for the board, if the rgb matrix is enabled. Or set it to 1. Specifically: #ifdef RGB_MATRIX_ENABLE
#define DEBOUNCE 15
#else
#define DEBOUNCE 1
#endif |
| // TODO?: tweak effects and timings to works with nonstandard #leds per run | ||
| #define LED_PER_RUN DRIVER_LED_TOTAL | ||
|
|
||
| for (int i = 0; i < LED_PER_RUN; i++) { |
There was a problem hiding this comment.
I've got some serious concerns with the changes to rgb_matrix.c. You took the loop out of each animation function and placed it here instead. This has a very nasty performance side effect of putting all the math that was removed from loops in the animation functions right back into the loop. Even worse is that now you are looping over a giant branch statement, and calling functions pushing and popping data off the stack. This really needs to be undone.
There was a problem hiding this comment.
A fair point! But this apparent trade in performance offers up quite a few solid benefits:
- It allows easy composition of multiple effects on a per-led level. In the future, it might even be possible to extend the QMK configurator to allow for some sort of easy per-key lighting customization!
- The initial reason for pulling the for-loop out was because the initial Massdrop code batched LED updates, instead of how RGB Matrix did it, by having them all fire off at once. That's actually why there is that seemingly useless #define there, as it should be possible to set it to a smaller number, and enable batched LED updates. In the end, the other improvements to the system resulted in not having to enable this functionality on the CTRL/ALT, but hey, it's nice to have, in case an under-powered board wants to leave more time for other tasks, but still have lighting effects.
- Having a single loop is cleaner from a software-engineering point of view (IMHO)
As for the performance considerations, I have 2 responses
- I've wrapped particularly expensive operations (i.e: the ones that used to be outside of the for loop in each effect) in a
if (led_i == 0)block, and marked the variables as static, so they are only computed once-per-frame. - If you're worried about looping through a switch, it's pretty easy to just set a function pointer to the desired effect per-frame outside the loop. That should alleviate any serious performance penalty from having to traverse the switch statement (though if the switch is optimized into a jump table, it shouldn't be too bad to begin with (idk though, haven't checked myself))
There was a problem hiding this comment.
Those benefits sound great and all, but they are still achievable without hoisting the loop, moving variables into global scope, introducing more branching logic, and so on. This approach has very real performance impacts on AVR (something I've been working on getting working) and impact on ARM to a lesser extent. So lets work together and break up this pr into more manageable chunks.
This commit is to get the Massdrop lighting code working again through use of the compilation define USE_MASSDROP_CONFIGURATOR added to a keymap's rules.mk. Added keymaps for both CTRL and ALT named default_md and mac_md. These should be used if the Massdrop style lighting is desired. slightly modified by Daniel Prilik to fix some whitespace errors.
|
@XScorpion2 have been having some back and forth on what to do about the whole "hoisting the for loop" architectural change, and it's ballooned into a broader discussion about settling on a roadmap to improve RGB Matrix. We both agree that long-term, the system should be both extensible (i.e: custom effects, per-led effects, etc...) and performant. This PR handles the former, making the system more extensible. His PRs handle the latter, making the system more performant. We can't seem to agree who's PR should go in first, since as it stands, the two implementations heavily conflict with one another, and one of us will have to re-implement their code. We both have some pretty compelling arguments, and because of that, neither of us have been able to convince one another haha I'll start. XScorpion2 is right in his assessment that hoisting the loop isn't the ideal architecture from a performance POV. It will result in poor performance on AVR, and will have to be swapped out for something else to get RGB Matrix running on AVR. But here's the thing: the current implementation is also too slow to run on AVR. This PR's primary goal was never to improve RGB Matrix's performance to the point where it could run on AVR. This PR's primary goal is to improve the extensibility of the RGB Matrix system, while maintaining performance on currently supported platforms. While there are no quantifiable numbers, tests from @drashna and @patrickmt have shown no serious performance degradations stemming from my changes (as far as I understand) So, if this PR introduces new features while maintaining adequate performance, why not polish up and land this PR, and then shift gears to performance improvement? Regardless if this PR lands or not, there will be a lot of work required to get RGB Matrix both performant and extensible, but if we can knock out the extensibility first, that cuts down on the effort required down the line! I think it's fair to say that it'll be more effort re-implementing the feature set of this PR (user-defined effects, per-key effects, Massdrop support, etc...) on-top of some more optimized version of the current system than it would be to simply integrate this PR's new features, and then work on improving their performance. |
|
This seems like a murky situation. Let me make it murkier. ;) Right now we actually have 2 LED matrix systems, one for RGB and the other for single color. (I tried to use the RGB matrix for the Clueboard, but many of the animations are nonsensical and it's weird defining the same LED for 3 different locations.) One of the practical consequences of this is that we also have 2 IS31FL3731 drivers, one for RGB and one for single color. Similar single-color drivers will need to be written for any other chips people may want to use this way. ISTM that eventually we'll want to make all of this line up in a much better way. One driver for both LED and RGB, sharing animation code between the two, with all other differences being minimized and code shared as much as possible. This probably also affects the refactoring work both of you are doing today, so perhaps that can act as a forcing function to help you move past your current impasse? |
|
Can I throw some murky into this too? Addressables LEDs are also another 'driver' if you will that can and also should be merged into this system as well. As far as the nature of this PR is concerned, I'll reply later when I'm not on my phone. |
|
I feel the need to put a big caveat on "another driver" for this system. My naive assumption was that single color LEDs are just HSV where the S is always at 0. But there are a lot of animations where that results in little or no visible change. There are others where it's not clear what's going on. To have "good" support for single color will mean tailoring the list of animations, and maybe even letting animations modify themselves for the available colorspace. |
|
So first let me address a key topic that has come up a few times: "one of us will have to re-implement their code.". It's not about who has to rewrite code and who doesn't. In fact, it never should be, as we are all here for the same reason, to improve the code base and we do so with facts and numbers. My nature of issue with this PR stems from the architecture level changes to rgb matrix and that those changes are were initially fixes to improve performance that then morphed into features. To use a few exact details: First the loops that were in the animation code was hoisted up to work over the switch and then pass in an index on which led for the animation code to operate on. This was done so that the work of calculating rgb for an led can be amortize. This is a great technique, in fact one I used in the OLED driver rewrite in #5288. However using it here is the wrong application of this technique as the poor performance of the animation functions is not a physical hardware limitation, unlike sending data over i2c in the OLED driver. Simply put, the optimizations added hide poor performance, no fix it. The animation functions themselves are poorly implemented for the processor hardware QMK supports, then to add more to this issue is that the data structures the animation functions work with are not laid out in performant manor. So lets fix the actual problems, not the perceived problems. Second, hoisting the loop moves control of what data or led to operate on out of the animation function. This reduces performance for animations that operate on only 1 (or a subset) of leds each tick as the are now have to do extra checks or logic to bail out if the id passed in if it is not the id they want to operate on wasting time doing nothing. This also goes even further as many animations only need to calculate a subset of data before operating on the entire led matrix, so this data now needs to be moved to global scope from function stack scope so it is preserved over multiple calls. This increases firmware size, and adds yet again more logic to the animation function that just doesn't need to be done. |
|
One thing I would like to point out here .... While this is awesome, and fixes a few issues I had, it fundamentally changes a number of things. In many cases, the animations are running backwards, from what was default. As for rewriting their code, that happens. Frequently. Whether it's a complete change, or rewriting sections.... it's happened before. |
|
I've done some thinking, and I've come around to agree with @XScorpion2. It's always a shame to scrap work, but after stepping back and taking a more objective look at this PR, I can see the issues with it. After learning more about how the RGB Matrix system works (a lot more than I expected haha), I've come to realize that the poor performance of RGB Matrix on the CTRL/ALT (especially on MRGB_MATRIX_KEYPRESS-based effects) was mainly due to the inherent architectural inefficiencies present in the base system, something which will require a surprising amount of effort to improve, and something that should be undertaken in a separate series of PRs. So, with that said, I'll be closing this PR, and opening up a few new ones to gradually merge the good bits scattered around in this PR:
Support for per-key lighting is something that will requires more thought, as performance considerations should be at the forefront (i.e: don't just hoist out a for loop 😉) This has been my first proper open-source PR, and it's been a heck of a learning experience! I think a lot of issues stemmed from the fact that even from the start, this was a bulky PR. It combines RGB Matrix features with CTRL/ALT support for no good reason! I just thought the ALT was a awesome board, and that more owners should be able to use it to it's full potential. So, thanks for the feedback everyone, and I'll be back with some smaller changes in the coming weeks! Feel free to poke me if you want to bounce some ideas off me, or if you want to understand how / why some things work in RGB Matrix. Like I said, I've learned a lot about the subsystem, so I might be able to help 😄 |
This PR also introduces refactors, tweaks, and improves the RGB Matrix system, cleaning up the code and adding new features.
Also, building off of the excellent work done by @helluvamatt a few months ago, this PR switches the Massdrop ALT/CTRL QMK's standard RGB Matrix LED lighting system.
Description
[RGB Matrix] Ability to define custom RGB Matrix Modes from keymaps
massdrop/alt/keymaps/prilik/for an example[RGB Matrix] Refactors all effects to run per-led
[RGB Matrix] Add built-in Breathing effect
[RGB Matrix] Add RGB_MATRIX_EXTRA_TOG option
[ALT/CTRL keyboards] Add RGB Matrix LED location definitions
[ALT/CTRL keymaps] Replace custom keycodes with RGB Matrix keycodes
[Massdrop Configurator] Maintain backwards compatibility with existing configurators
led_instruction_t led_instructions[].Known Issues
This PR breaks two keymaps: massdrop/alt:reywood and massdrop/ctrl:responsive_pattern
Performance?
Future Improvements
led_matrix.c/h, and using the existing IS31FL3733 driversTypes of changes
Issues Fixed or Closed by this PR
Checklist: