RGB Matrix support for Massdrop CTRL/ALT

[daniel5151]

Building off of the excellent work done by @helluvamatt a few months ago, this PR switches the Massdrop CTRL/ALT to use QMK's built-in RGB Matrix LED lighting system.

It does also implement 2 small RGB Matrix tweaks: a new breathing effect, and a RGB_MATRIX_EXTRA_TOG option. This brings RGB Matrix a bit closer feature-wise to the old Massdrop system.

Description

• [ALT/CTRL keyboards] Add RGB Matrix compatible LED location definitions
• [ALT/CTRL keymaps] Update keymaps to use RGB Matrix keycodes
  • Tries to keep the layout as-close as possible to the old system
• [Massdrop Configurator] Maintain backwards compatibility with existing configurator
  • Defining USE_MASSDROP_CONFIGURATOR enables overriding RGB Matrix effects with effects defined using led_instruction_t led_instructions[].
  • This code is ported over from Massdrop's qmk_firmware fork
• [RGB Matrix] Add Breathing effect
• [RGB Matrix] Add RGB_MATRIX_EXTRA_TOG option
  • When defined, RGB_TOG cycles through 4 lighting modes
    • ON > JUST_KEYS > JUST_UNDERGLOW > OFF vs. just ON/OFF
  • This is on by default for the ALT/CTRL

Known Issues

• This PR breaks two keymaps: massdrop/alt:reywood and massdrop/ctrl:responsive_pattern

  • They both hooked into the old Massdrop system for custom lighting effects
    • I've added README files to the two keymaps mentioning that they need to be updated
  • Fortunately, there are similar effects in RGB Matrix, though they are not enabled by default (see notes about performance below)

• Performance

  • Some effects (notably those hidden behind RGB_MATRIX_KEYPRESSES) are pretty intense, and can interfere with the timings of the CTRL/ALT
  • As a result, this PR does not enable RGB_MATRIX_KEYPRESSES for the CTRL/ALT by default
  • @XScorpion2 is working on some architectural improvements to the RGB Matrix system which should improve performance, and so, a future PR might enable these effects to run by default

• No EEPROM support (see EEPROM support for Massdrop arm_atsam (CTRL & ALT) #4625)

  • i.e: no saving configuration between reboots.
  • Not a major issue, just annoying haha

Future Improvements

• Switching over to the existing IS31FL3733 driver
  • There already exists a IS31FL3733 driver, why not use it?
  • One major issue is that the current driver implementation only supports a single driver, while the CTRL & ALT both have two drivers.
  • Also, Massdrop's driver seems to be using DMA, while the RGB Matrix doesn't?
  • I took a crack at getting it working and couldn't figure it out... someone better versed in embedded development should take a stab at this. @patrickmt has it on his radar, but it's not high-priority
• Removing legacy Massdrop lighting support entirely
  • This would require the Massdrop configurator to be updated to use RGB Matrix's patterns
  • Moreover, there would have to be a good way to layer several effect per-layer, something the current RGB Matrix system doesn't support
    • This is something that might be solved in the future through @XScorpion2's work

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 support for Massdrop arm_atsam (CTRL & ALT) #4567

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).

[daniel5151]

@patrickmt, when you get the chance, could you check if these changes have reasonable performance?
If not, see if adjusting RGB_MATRIX_SKIP_FRAMES in config_led.h has any effect.

[skullydazed]

This is looking pretty good. :)

Before we merge this I'd like to see all the affected users contacted first. There is a tension here between not breaking travis and not breaking a user's git workspace, and it would be best if we could resolve that tension by getting sign-off from everyone involved.

By my reckoning we need sign-off from @abishalom, @reywood, and @valen214. Have I missed anyone?

This also seems to add new keymaps like massdrop/alt:prilik and massdrop/ctrl/mac_md. Generally we add keymaps separately from core changes unless there's a good reason to include them. Is there a reason here, or could those be moved to a separate PR?

[daniel5151]

Good to hear!

The massdrop/alt:prilik keymap is my own, and yea, we can definately push it out into a separate PR if need be. The massdrop/(alt|ctrl):(default|mac)_md keymaps should be included in this PR, as they are keymaps that use USE_MASSDROP_CONFIGURATOR for backwards compatibility with the Massdrop configurator.

@abishalom should be okay, since he didn't mess around with any of the Massdrop lighting stuff. I did tweak his keymap to use the new RGB Matrix system keycodes, and I recall flashing it to confirm that it worked fine. Though, as I'm writing this now, it might make sense to revert those changes, and active the Massdrop compatibility mode flag, as it could be jarring to wake up one morning, flash your keymap, and see that your default lighting effects have changed :) Up to him if he wants me to revert those changes.

@patrickmt attempted to fix @reywood's and @valen214's keymaps to work with the Massdrop compatibility mode, and had some success in doing so. IIRC, reywood's effect is working, but valen214's was far too complicated to try and debug. That said, due to the nature of this PR, these breakages are almost unavoidable, since the whole point is to transition off of the unique Massdrop system, and onto the standard RGB Matrix system. Plus, once XScorpion2's RGB Matrix performance improvements get merged, and RGB_MATRIX_KEYPRESSES can be enabled for these boards, a whole wealth of reactive effects similar to these custom keymaps will open up to everyone :)

[skullydazed]

Those reasons are good, but they don't address our primary concern- that these people may wake up one day, try to update qmk, and then have a merge conflict on their hand that they have no clue how to fix. Our preferred way to address that is for them sign off to let us know they're aware of and ok with this change to their files.

[daniel5151]

Ah, gotcha.

I'm was just trying to offer some background as to why things have changed the way they have 😄

[reywood]

Consider me warned 😃👍

[abishalom]

Fine with the changes; don't really care about the RGB customization tbh.

[valen214]

Consider me warned 😃👍

same here 🙂

[drashna]

@skullydazed well, we have the sign offs from affected users.

And I believe that @patrickmt was going to test these changes too.
And if there are performance issues, it may be a good idea to test with #5372, as that PR seems to significantly improve performance of the core qmk rgb matrix code.

[XScorpion2]

Merged this into #5372 locally, and tested it with my CTRL. The performance was fantastic with the two worst offenders rgb_matrix_multisplash and rgb_matrix_digital_rain only taking 6ms of time to completed compared to the 35ms without the perf improvements.

[drashna]

@patrickmt if you could check out this PR, and make sure it's okay?
And approve the PR, if it's good.

Though, like @XScorpion2 mentions, it may be worth testing this PR with #5372, as that PR drastically improves performance.

[XScorpion2]

https://github.com/XScorpion2/qmk_firmware/tree/dev/rgb_ctrl has all the goodies merged in if you want to use that for testing. Though I did a crap job merging in ws2812 support so don't try rgblight on that branch...

[patrickmt]

I've taken a quick look at XScorpion2's branch:

default_md keymaps for both CTRL and ALT do not compile. The main reason is that for #5096, there existed a global variable in rgb_matrix.h which replaced the Massdrop version's frame counter. To remedy this for my tests, I declared g_tick in tmk_core/protocol/arm_atsam/led_matrix.c and increment this counter once per flush.

RGB updates are happening as fast as possible instead of their intended 10ms intervals. This is derailing other timings on the system. Again, methods were in place in #5096 that solved this issue.

@daniel5151 please look into this again

[XScorpion2]

Hmm, thats odd that it's affecting system timings that much. Can you provide info on what you are seeing that is affected and on which board? I've got a CTRL that I've been using for testing, haven't had compile errors though, but just been using the Default keymap. Didn't even realize there was a default_md as well. Will try and repro it as well tonight and see if there are changes in rgb matrix that I should change too.

Thanks for looking at this PR and my branch as well.

[daniel5151]

@patrickmt does everything work as expected on this PR (i.e: without XScorpion2's changes)?

[XScorpion2]

Thought about it a bit more, I suspect the timings are animation speed related, which is actually expected with my PR as I did a lot to ensure the following:

• animations use a real-time based timer.
• All config (hue, val, sat, SPD) values are used and respected in all animations.

So this means board performance will no longer affect animation speed, and you can adjust any value including speed to levels that you lile and the animation will respect it. The end result is that you can no longer use animation speed as an indication of performance, and the compared to previously the animations may appear faster or slower. So use the key code to increase or decrease speed to your desired level.

[XScorpion2]

OK, I see the issue with default_md. It is using a combination of RGB_MATRIX_SKIP_FRAMES 10, the old 32 bit g_tick timer, and USE_MASSDROP_CONFIGURATOR to ensure the 10ms interval animation timing. This is an easy fix and is entirely on my PR #5372. That being said, I'd suggest merging just this PR as is and don't merge my PR until I fixed the above issue.

@patrickmt thanks again for the review.

[patrickmt]

@daniel5151 I will be testing this PR alone as well soon.

@XScorpion2 That sounds good if it is an easy fix and exactly what is missing. Thanks!

[drashna]

That's why I don't necessarily trust Travis.

Specifically, it's an issue with the recent zeal code change (sorry), and doesn't affect this.

So aside from the Travis issues, this is good to go, correct?

[patrickmt]

Good job everyone!

[drashna]

RGB Matrix support for Massdrop Boards