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

Split Keyboard RGB Matrix Support #5998

Closed
wants to merge 8 commits into from

Conversation

XScorpion2
Copy link
Contributor

@XScorpion2 XScorpion2 commented May 27, 2019

Description

So this is a bit of the ground work to get RGB Matrix split working with Split Common similar to how CRKBD works. There are still some things to debate, so here is an overview of the changes:

  • Split common now also syncs master half matrix to slave, this allows slave to know the state of the keyboard
  • Split common slave now calls matrix_scan_quantum(), this allows the slave to update it's state correctly the same way master does
    • This also prevents needing to add one off support constantly for things like encoder updating or oled drivers on the slave
  • timer.c is now sync'd between halves
  • Small changes to RGB Matrix to know what indexes apply to which half.
  • Small changes to Zygomorph Keyboard as I needed something to test this on.

Still to do & need input from others:

  • Since the slave now processes keycodes, certain keycodes or systems will need checks to not run on the slave side. For example: RESET should not reset the slave as well as the master by default. I'm not sure what other keycodes or systems will need checks added for slave side, so taking suggestions.
  • There is a possibility the halves can get out of sync, so I want to see how folks get out of sync to know what the minimum to sync across to keep it operating correctly without just syncing everything (rgb matrix is big...)
  • i2c code to sync the both halves of the matrix

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.

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

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
Copy link
Member

drashna commented Jun 12, 2019

Posting more for reference.

Works on the Corne when converted to split common (eg with #6001).
However, this breaks "normal" split keyboards. Serial is unstable, and i2c outright doesn't work.

quantum/quantum.c Outdated Show resolved Hide resolved
@XScorpion2 XScorpion2 force-pushed the features/rgb_matrix_split branch from b396f87 to 3d1b4d4 Compare June 30, 2019 18:37
@XScorpion2
Copy link
Contributor Author

XScorpion2 commented Jul 19, 2019

Tested this quite extensively over the last month on my Zygomorph, and outside of the power related delocks, I ran into no issues.

This does appear to cause power related delocks during usb enumeration, or rather more likely to cause that. IE: If the rgb power draw was too high on the zygo during usb enumeration it would fail to fully connect and deadlock (where before this change it wouldn't). Dropping the rgb values down slightly allowed it to connect, then I was able to put them back up to the higher values. Could use someone with more usb enumeration / hardware state debugging experience than I have to track that down.

@XScorpion2 XScorpion2 force-pushed the features/rgb_matrix_split branch from 3d1b4d4 to d28dbfa Compare July 20, 2019 18:36
@XScorpion2
Copy link
Contributor Author

Updated the i2c split code as it was wrong. Just need @drashna to wake up and test it.

tmk_core/common/keyboard.c Outdated Show resolved Hide resolved
@XScorpion2
Copy link
Contributor Author

i2c split code is in a much better state, however timer sync is not working out as I was hoping. Will implement a new synced_timer that wraps timer in non split keyboard mode so systems that need to be timer driven from the master half can just use that without needing to switch timers based on compile flags.

@drashna
Copy link
Member

drashna commented Jul 27, 2019

Well, typing works over i2c now!
But rgblight doesn't. Looks like the syncing for it doesn't work.

@XScorpion2
Copy link
Contributor Author

Ok cool, just need to resolve the sync timer then.

@Lenbok
Copy link
Contributor

Lenbok commented Jul 27, 2019

Just as a data point, I have converted my crkbd to use this PR plus #6001 and have switched my i2c redox (just using rgblight, not rgb_matrix) onto this as well for testing, can confirm that the rgblight syncing is not working. Happy to help test updates.

@drashna
Copy link
Member

drashna commented Jul 28, 2019

Serial works 100%, but i2c has issues. But only with animations. If it's solid color, it works just fine. So it's definitely an issue with timer syncing.

Though, it's getting there!

@Lenbok awesome!

@XScorpion2
Copy link
Contributor Author

Ok, if it is just i2c then that narrows down the issue. should be fixable tonight

@XScorpion2
Copy link
Contributor Author

I2C sync timer is fixed

@Lenbok
Copy link
Contributor

Lenbok commented Jul 28, 2019

Compiling: quantum/split_common/transport.c                                                        quantum/split_common/transport.c: In function ‘transport_master’:
quantum/split_common/transport.c:89:56: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
   i2c_writeReg(SLAVE_I2C_ADDRESS, I2C_SYNC_TIME_START, (void *)sync_time, sizeof(i2c_buffer->sync_time), TIMEOUT);
                                                        ^
cc1: all warnings being treated as errors
 [ERRORS]
 |
 |
 |

@XScorpion2
Copy link
Contributor Author

@Lenbok forgot the ref, pushed a fix

@Lenbok
Copy link
Contributor

Lenbok commented Jul 28, 2019

Compiles now, but the slave side is not syncing properly. e.g. for breathing mode, after a short period (sometimes <1s, sometimes several seconds), the slave starts flashing different colors (like it's changing hsv values). For moving modes like knight/snake, the slave side one goes massively fast. Then it might come right for a few seconds, then speed up again.

@XScorpion2
Copy link
Contributor Author

Hmm, ok might need more rgblight work then... Can you try rgb matrix?

@XScorpion2
Copy link
Contributor Author

I reverted the sync timer changes to rgblight as I'm not sure if they are even necessary.

@drashna
Copy link
Member

drashna commented Jul 29, 2019

that seems to work.

@Lenbok
Copy link
Contributor

Lenbok commented Jul 29, 2019

Yep, the rgblight redox is working fine for me now too.

I don't have my crkbd here (it's at work), so to test the rgb_matrix I hacked up a config for my redox. Compiled and flashed the same hex to each side. Both sides are running the animations, but they are not in the same mode?!? When I press RGB_MOD both sides advance through the modes (so they are consistently not in the same mode).

Video: https://photos.app.goo.gl/HmqYaxBvaGd6qnKY9
My test branch: Lenbok@0cd7037

@XScorpion2
Copy link
Contributor Author

XScorpion2 commented Jul 30, 2019

@Lenbok ya the halves can get out of sync as there isn't any actual state sync for rgb matrix. I'm relying on them staying in sync by ensuring what keycodes happen on master also happen on the slave. I suspect your slave didn't have the same initial eeprom state as the master causing the two sides to be incorrect. To sync them back up add a custom keycode that you can hit to reset the rgb eeprom to default:

        case RGBRST:
            {
                if (record->event.pressed)
                    eeconfig_update_rgb_matrix_default();
            }
            return false;

I'll think of a better way to keep the halves in sync with a limited amount of data.

@Lenbok
Copy link
Contributor

Lenbok commented Jul 30, 2019

Can you do something along the lines of what rgblight does to sync across changes to the config? At startup and whenever the config changes it syncs across. The slave is basically running off the version it gets from the master and doesn't care about (or modify) what's in it's eeprom config.

@drashna
Copy link
Member

drashna commented Jul 30, 2019

also, EEP_RST runs on both halves. I think it should stay that way.

And I agree with @Lenbok, we should sync the rgbmatrix config, the say way that the rgblight config is. Well, not exactly. It should be synced better, since that has bugs.

@XScorpion2
Copy link
Contributor Author

Ya, I plan on adding something, just need some free time.

@XScorpion2 XScorpion2 force-pushed the features/rgb_matrix_split branch from 586589f to e2ea750 Compare August 3, 2019 22:29
@XScorpion2 XScorpion2 force-pushed the features/rgb_matrix_split branch from 081912a to ca64bdf Compare January 5, 2020 02:24
@swanmatch swanmatch mentioned this pull request Jan 22, 2020
13 tasks
@stale
Copy link

stale bot commented Feb 19, 2020

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@stale stale bot removed the awaiting changes label Feb 19, 2020
@drashna drashna requested a review from a team February 19, 2020 22:53
@drashna
Copy link
Member

drashna commented Mar 23, 2020

There are some merge conflicts here. I can resolve them, if needed.

common_features.mk Outdated Show resolved Hide resolved
@drashna
Copy link
Member

drashna commented May 3, 2020

Sorry if I have overstepped, but I've updated a couple things (repeatedly now).

One thing that has been mentioned is that thi is missing the "get_slave_status" stuff, so that the matrix is only updated if there is an actual change.

#7732 is an implementation of that, but it would be nice to get in here. Though ... I am not sure if that wil help get this merged.

@XScorpion2
Copy link
Contributor Author

@drashna GASP how dare you sir!
No seriously you are fine. Saw #9001 and thought that would be a nice addition to this pr. still need to add a rgb config sync to prevent halves from possibly getting out of sync.

@tyalie
Copy link

tyalie commented Jun 30, 2020

I've now successfully merged the current master with this branch and would be ready to implement the state sync. I've actually done that before, got stuck at the timer sync and only later realized, that this pull request exists. As such I've the code from that approach still in my git repo and would be happy to implement it into this one.

Edit: I've thought about your approach and wouldn't it be better, when the master side would send the current led config, like it is done with rgblight and disable key evaluation on the slave side. The transferred keys would then only be used, when using some interactive effects.
That would probably solve a few bugs and save a your eeprom cycles.

@tyalie tyalie mentioned this pull request Jul 1, 2020
20 tasks
@tyalie
Copy link

tyalie commented Jul 1, 2020

I've created a new merge request with my changes. See #9613

@XScorpion2
Copy link
Contributor Author

Closed for the updated version #11055

@XScorpion2 XScorpion2 closed this Nov 28, 2020
@XScorpion2 XScorpion2 deleted the features/rgb_matrix_split branch November 29, 2020 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants