Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RGB Matrix Support for Split Keyboards #9613

Closed
wants to merge 47 commits into from

Conversation

tyalie
Copy link

@tyalie tyalie commented Jul 1, 2020

This is an advancement from #5998
The idea is to implement the rgb matrix module for split keyboards, as it is currently missing.

Description

This is some basics to get RGB matrix support onto split keyboards. The main change of idea between this and #5998
is, that this feature line does not enable keyboard evaluation on the slave side, but instead the config is always synced from master to slave only if changes occur. (very similar to how the RGB lighting module works).
As such both keyboards cannot get out of sync, has it can happen on #5998.

For interactive animations, the state of the master is transferred to slave, but only rgb matrix specific functions for keyboard evaluation are triggered.

  • rgb matrix config is synced between both halves
  • timer.c is now sync'd between halves
  • Small changes to RGB Matrix to know what indexes apply to which half.
  • introduction of Lily58l keyboard as it is the one I'm working with (see https://github.com/BenRoe/qmk_firmware/tree/Lily58L as he's the original author)
  • "fixed" bug, where Elite-C flash failed, even though it worked. Probably my fault

Still to do

  • suspend isn't synced
  • a interactive effect triggered from the slave takes longer to get to the master side, than when an effect that is triggered on the master is transferred to the slave. But this effect is for me only visible, when LT(_LOWER, KC_BSPC) was used. (an LT modifier on master side doesn't seem to be so problematic)
  • i2c testers as I can only test it with a serial connection
  • As of now timer.c is synced with an static offset to account for transfer delay. If issues occur it might be more plausible to implement a real timer sync protocol similar to Cristian's Algorithm
  • Split common also syncs master half matrix to slave, this allows slave to know the state of the keyboard
    • transfer of keyboard state to rgb_matrix on slave side
  • doesn't compile with make all as definitions from sync_timer weren't found

Known issues

  • LT(layer, kc) with KC_CAPS causes reactive animation stutter. This is because it has an inline wait_ms(200). Not much can be done to improve this situation as it hard locks all processing of the keyboard. (maybe. Didn't test it)

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

  • a lot

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

noroadsleft and others added 10 commits June 19, 2020 21:31
* Initial work for consolidation of board files and default ChibiOS configs.

* Migrate F401/F411 black pills for testing.

* Add early init bootloader jump flag.

* Add support for I2C in order to use i2c_scanner keymap.

* Add F401/F411 HSE bypass to get things booting.

* Exempt "hooked" ChibiOS conf files from updater script.

* Fix up ordering for bootloader_defs file check.

* Match previous $(KEYBOARD_PATHS) value for Proton-C, updated for all board configs.
@tyalie tyalie changed the title Rgb matrix support RGB Matrix Support for Split Keyboards Jul 1, 2020
@XScorpion2
Copy link
Contributor

Good first start, but there is a reason why the slave half needs to enable keyboard evaluation which is reactive rgb matrix animations. With out that hook, the reactive effects will not work, and I don't see anything in this PR to improve that.

@tyalie
Copy link
Author

tyalie commented Jul 1, 2020

I'll sync the states and directly tap into rgb_matrix, circumventing all other methods that are normally called

Working on that right now

@XScorpion2
Copy link
Contributor

Thanks for taking this over. RL has been a major issue lately for me.

@tyalie
Copy link
Author

tyalie commented Jul 1, 2020

You're welcome. I'm happy to take this over. First mayor contribution to an OS project for me.

I'm also really lucky that the whole situation rn hasn't affected me really at all. Wishing you the best!

@tyalie
Copy link
Author

tyalie commented Jul 1, 2020

Works now, but I'm unhappy with the solution, as keypress evaluation is turned on.
This would create more problems, the more features qmk gets. Because now everytime somebody introduces a feature like RESET, one would need to create a new special case for that when using split keyboards.

A solution would be to make extensive use of c macros to remove all keypress evaluations that are unwanted.
Another solution would be to mirror the matrix scanning code from keyboard_task and call rgb_matrix_process_keycode there directly.

Both solutions are very unsatisfying.

@rhruiz
Copy link
Contributor

rhruiz commented Jul 1, 2020

common split code already has a check for RESET. Keypress evaluation on the slave side has a nice benefit of allowing things like making the slave OLED react to layer change and keypresses (this works in #5998)

@tyalie
Copy link
Author

tyalie commented Jul 1, 2020

But RESET is not the only problem. As for now the slave side also does evaluation of RGB MATRIX commands. That is a very dirty approach especially, since the slave side config is overwritten in the next frame. Using EEPROM cycles, ...

But to my understanding #5998 has a lot bugs, because of slave side keyboard evaluation.

@tyalie
Copy link
Author

tyalie commented Jul 1, 2020

Thanks to @sigprof I found a solution that works nicely. Sadly slave still can't show the current layer information

@tyalie
Copy link
Author

tyalie commented Jul 1, 2020

It's now working as expected. There are only some major problems with the design and how future proof it might be.

@tzarc tzarc changed the base branch from master to develop July 1, 2020 20:13
@tzarc tzarc changed the base branch from develop to master July 1, 2020 20:46
@tzarc tzarc requested a review from a team July 1, 2020 20:46
@tzarc tzarc added breaking_change Changes that need to wait for a version increment core enhancement feature labels Jul 1, 2020
@drashna
Copy link
Member

drashna commented Jul 5, 2020

But to my understanding #5998 has a lot bugs, because of slave side keyboard evaluation.

It's less of that. But it's a lot of changes.

Additionally it's missing some things like state syncing, so it's always sending the matrix code.

Breaking up the PR into chunks (like the sync timers) would make it easier to get merged in, as it would be easier to review, which would make it faster to get in.

@tyalie
Copy link
Author

tyalie commented Jul 6, 2020

Okay. Didn't opened an issue for the sync timer yet, if people wanted to suggest a radically different way for this.

EDIT: Issue is open. See #9665.

@tzarc
Copy link
Member

tzarc commented Jul 16, 2020

Looks like we're going to need a rebase on top of develop -- lots of irrelevant changes here.

@tyalie
Copy link
Author

tyalie commented Jul 18, 2020

Whatya mean? My idea honestly was to do #9665 first from a clean slate on (develop branch). I know what to change and it isn't much. My troubles are:
a) I'd like some feedback on it before delving into it 😅
b) I've exams and other time sensitive stuff in the next 1½ months (after that I'm fine)

@KarlK90
Copy link
Member

KarlK90 commented Jul 22, 2020

I am running your code successfully on my split keyboard. Communication is via serial link on an stm32f103. Please let me know if I can help testing the changes. 🙂

@tzarc
Copy link
Member

tzarc commented Jul 22, 2020

Whatya mean? My idea honestly was to do #9665 first from a clean slate on (develop branch). I know what to change and it isn't much. My troubles are:
a) I'd like some feedback on it before delving into it 😅
b) I've exams and other time sensitive stuff in the next 1½ months (after that I'm fine)

Yeah that's fine - it's more that in this PR's files list above, it's got tons of changes from the branch which isn't part of your PR.
Rebasing the changes to the top of develop will make it so that there's only the appropriate changes here.

But... if you're going to refactor with a clean slate, then I'll mark this as "on hold" until #9665 and any other prerequisites are sorted.

EDIT: Scratch that, "awaiting pr" instead.

@tzarc tzarc added the awaiting_pr Relies on another PR to be merged first label Jul 22, 2020
@noroadsleft noroadsleft force-pushed the develop branch 2 times, most recently from 550bb6a to 9d827f9 Compare July 31, 2020 19:44
@noroadsleft noroadsleft force-pushed the develop branch 2 times, most recently from 1d4c0cd to 65c27a3 Compare August 29, 2020 21:41
@robhaswell
Copy link
Contributor

I can confirm this is working great with my Lily58L.

Also I noticed that this branch includes Lily58L support, which is strictly beyond the scope of this branch. I will submit a PR for basic Lily58L support with RGBLIGHT, at least then the matrix modifications in this branch for the keyboard will be relevant.

@XScorpion2
Copy link
Contributor

Should be closed for the updated pr: #11055

@noroadsleft noroadsleft deleted the branch qmk:develop November 28, 2020 20:02
@tzarc tzarc reopened this Nov 28, 2020
@github-actions github-actions bot added cli qmk cli command python labels Nov 28, 2020
@fauxpark fauxpark closed this Nov 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting_pr Relies on another PR to be merged first breaking_change Changes that need to wait for a version increment cli qmk cli command core enhancement feature keyboard keymap needs doc needs testing python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants