-
-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
Generalized matrix.c for keyboards with I/O expanders & TWI #2065
Comments
@ErinCall Have you made any progress? This would be a huge win for a project I'm currently working on. |
I know that it isn't directly related, but could add onboard/i2c callouts for RGB underglow, audio, and backlights. That would allow for control of both without burdening the TRRS cable (though would require rewriting of aforementioned code). |
@sdbarker I'm sorry to say I've made no progress. Been dealing with some health problems. I'm hoping to get at it while I'm off work next week. |
@ErinCall No worries. I wish you the best, take care of yourself! |
With a column-driven keyboard, reading from the mcp23081 returns a column-state, which takes some extra work to translate into the row-state used in the actual matrix. The ergodox_ez code sidestepped that problem by calling rows "columns" and columns "rows." With this change, the dactyl now calls rows "rows" and columns "columns."
Bonus: saves one i2c transaction per matrix_scan!
Miss that reply. Hope you're doing better! And do you have some progress that you could link to, for others to continue, if need be? |
I have! I’ve been working on this branch. The plan is to bring I’m most of the way done with the first part. I’ve been too busy to work on it lately, but I have time this afternoon and I’m going to sit down with it in an hour or so. With any luck I’ll be able to finish up phase one today 🤞🏻 |
Fantastic! |
I honestly don't know whether/how well this code works with other I/O expanders, but at least in theory, it should be generic enough to work with others. Given that, the variable names shouldn't refer to a specific model of expander.
It's commented out in quantum/matrix.c, and the dactyl has no power up/down behavior beyond being unplugged (which goes to matrix_init), so there's no sense keeping it around.
...and rename input_mask to expander_input_mask, since now that it isn't scoped to init_expander it isn't clear that it's only for the expander.
Interesting! I've just done a patch to clean up debouncing in the ergodox, and I have some experiments along those lines. One marginal concern: In the specific case where you know things about the layouts of the pins in registers, you can make things a lot faster. And it's not a huge difference, but it's a measurable difference in performance. And I don't see a good way to express that as a generalization. Except, possibly, some kind of "select_row_n" or "read_row_n". Note also, at least in the Ergodox case, there was potential for a very significant performance improvement (about 2x overall) from interleaving local and i2c rows, because that lets us skip a ton of 30us delays and eliminate a lot of unneeded i2c ops. |
* Use the new debounce algorithm in dactyl/matrix.c [#2065] This incorporates the fixed/optimized debounce code added to quantum/matrix.c in: * 508eddf * 4c69608 * 32f88c0 * f403028 * a06115d * Fix the row/column swap in dactyl [#2065] With a column-driven keyboard, reading from the mcp23081 returns a column-state, which takes some extra work to translate into the row-state used in the actual matrix. The ergodox_ez code sidestepped that problem by calling rows "columns" and columns "rows." With this change, the dactyl now calls rows "rows" and columns "columns." * Cleanup: variable names, documentation [#2065] * Support MATRIX_MASKED in dactyl/matrix.c [#2065] * Only unselect one col in unselect_col [#2065] Bonus: saves one i2c transaction per matrix_scan! * Implement COL2ROW in dactyl/matrix.c [#2065] * Fix a typo in dactyl/matrix.c This entirely doesn't matter. The PORT values are set during init_keyboard and never change. They're repeatedly set to the same thing. These PORT lines shouldn't even exist, but since they do, they should at least look right. * Implement COL_PINS/ROW_PINS for dactyl [#2065] * Rename "mcp23018" to "expander" [#2065] I honestly don't know whether/how well this code works with other I/O expanders, but at least in theory, it should be generic enough to work with others. Given that, the variable names shouldn't refer to a specific model of expander. * Remove matrix_power_up from dactyl/matrix.c [#2065] It's commented out in quantum/matrix.c, and the dactyl has no power up/down behavior beyond being unplugged (which goes to matrix_init), so there's no sense keeping it around. * Only initialize expander_input_mask once [#2065] ...and rename input_mask to expander_input_mask, since now that it isn't scoped to init_expander it isn't clear that it's only for the expander.
* Use the new debounce algorithm in dactyl/matrix.c [qmk#2065] This incorporates the fixed/optimized debounce code added to quantum/matrix.c in: * 508eddf * 4c69608 * 32f88c0 * f403028 * a06115d * Fix the row/column swap in dactyl [qmk#2065] With a column-driven keyboard, reading from the mcp23081 returns a column-state, which takes some extra work to translate into the row-state used in the actual matrix. The ergodox_ez code sidestepped that problem by calling rows "columns" and columns "rows." With this change, the dactyl now calls rows "rows" and columns "columns." * Cleanup: variable names, documentation [qmk#2065] * Support MATRIX_MASKED in dactyl/matrix.c [qmk#2065] * Only unselect one col in unselect_col [qmk#2065] Bonus: saves one i2c transaction per matrix_scan! * Implement COL2ROW in dactyl/matrix.c [qmk#2065] * Fix a typo in dactyl/matrix.c This entirely doesn't matter. The PORT values are set during init_keyboard and never change. They're repeatedly set to the same thing. These PORT lines shouldn't even exist, but since they do, they should at least look right. * Implement COL_PINS/ROW_PINS for dactyl [qmk#2065] * Rename "mcp23018" to "expander" [qmk#2065] I honestly don't know whether/how well this code works with other I/O expanders, but at least in theory, it should be generic enough to work with others. Given that, the variable names shouldn't refer to a specific model of expander. * Remove matrix_power_up from dactyl/matrix.c [qmk#2065] It's commented out in quantum/matrix.c, and the dactyl has no power up/down behavior beyond being unplugged (which goes to matrix_init), so there's no sense keeping it around. * Only initialize expander_input_mask once [qmk#2065] ...and rename input_mask to expander_input_mask, since now that it isn't scoped to init_expander it isn't clear that it's only for the expander.
* Use the new debounce algorithm in dactyl/matrix.c [qmk#2065] This incorporates the fixed/optimized debounce code added to quantum/matrix.c in: * 508eddf * 4c69608 * 32f88c0 * f403028 * a06115d * Fix the row/column swap in dactyl [qmk#2065] With a column-driven keyboard, reading from the mcp23081 returns a column-state, which takes some extra work to translate into the row-state used in the actual matrix. The ergodox_ez code sidestepped that problem by calling rows "columns" and columns "rows." With this change, the dactyl now calls rows "rows" and columns "columns." * Cleanup: variable names, documentation [qmk#2065] * Support MATRIX_MASKED in dactyl/matrix.c [qmk#2065] * Only unselect one col in unselect_col [qmk#2065] Bonus: saves one i2c transaction per matrix_scan! * Implement COL2ROW in dactyl/matrix.c [qmk#2065] * Fix a typo in dactyl/matrix.c This entirely doesn't matter. The PORT values are set during init_keyboard and never change. They're repeatedly set to the same thing. These PORT lines shouldn't even exist, but since they do, they should at least look right. * Implement COL_PINS/ROW_PINS for dactyl [qmk#2065] * Rename "mcp23018" to "expander" [qmk#2065] I honestly don't know whether/how well this code works with other I/O expanders, but at least in theory, it should be generic enough to work with others. Given that, the variable names shouldn't refer to a specific model of expander. * Remove matrix_power_up from dactyl/matrix.c [qmk#2065] It's commented out in quantum/matrix.c, and the dactyl has no power up/down behavior beyond being unplugged (which goes to matrix_init), so there's no sense keeping it around. * Only initialize expander_input_mask once [qmk#2065] ...and rename input_mask to expander_input_mask, since now that it isn't scoped to init_expander it isn't clear that it's only for the expander.
Nice work @ErinCall! I’ve looked at the updated dactyl code and it is so much easier to read and reason about! One thing that is not obvious to me is how these values map to the pins on the expander:
If this wiring scheme is still correct, I'm a bit confused: https://github.com/adereth/dactyl-keyboard/blob/master/guide/circuit-diagram.png Would you mind taking a minute or two to clarify? |
There are two things that could be throwing you off—
|
Thanks, those two puzzle pieces really helps! I also found this comment in matrix.c which is really helpful to understand how the firmware distinguishes the two sides from each other:
Maybe a short comment like this in config.h would make things more clear:
|
Hmm, this seems like it's pretty close to being supported in the core (eg, quantum). It looks like it wouldn't take much to hook this into the "split common" code, using a different transport. |
Hi @ErinCall, thank you so much for this effort! Did you have chance to finish it? What is current status? I was checking Dactyl keyboard and it seems it’s still using custom matrix and its own implementation of i2c... 😕 I am working on my first keyboard and expanders are exactly what I need! 😊 |
I was able to refactor this heavily throughout my PR @vladimir-aubrecht here: #9884 |
Thank you @myoung34! Is there a plan to have matrix.c with support for extenders also directly in Core? |
I know there's traction, but I'm definitely not the person to attempt it 😬 |
Background
Many split keyboards (especially experimental/handwired ones) use the Ergodox's design, where the right-hand keys are read directly from the onboard registers and the left-hand keys are read from an I/O expander via I2C (or "TWI," if you prefer1).
quantum/matrix.c
has no support for this, so any keyboard that uses the Ergodox's design must use a custommatrix.c
. The result is a big ol' mess. Any features or improvements toquantum/matrix.c
that were made since the keyboard forked off of it are unavailable in that keyboard. For example, the ergodox_ez is missing the improved debounce algorithm from #860.The Dream
It would be really great if
quantum/matrix.c
had support for reading key states via I2C. I'm envisioning aconfig.h
that looks something likeProposal
i2c.c
andi2c.h
files in thequantum/
directoryquantum/matrix.c
knows how to read from an I/O expander, ifUSES_I2C
is defined.matrix.c
solely to provide I2C support get to go back to thequantum
version ofmatrix.c
I'm going to start working toward this proposal.
Details on the mess
The oldest I2C-using
matrix.c
I can find is inergodox_ez
. There are more-or-less-exact copies inhandwired/dactyl
andhandwired/frenchdev
.ergodone
has I2C support derived fromergodox_ez
's, with a few nice refactors. All of these keyboards have identical2 copies oftwimaster.c
andi2cmaster.h
.3The
deltasplit75
,iris
,lets_split
,levinson
,minidox
,nyquist
,orthodox
, andviterbi
keyboards use a separate approach (first seen in thelets_split
). All of these keyboards have identical copies ofi2c.h
andi2c.c
.hadron
also has this version, but as far as I can see it callsi2c_master_start
and never does anything else with it.The
bfake
,bmini
,mechmini
,mt40
,pearl
,ps2avrGB
, andymd96
keyboards use i2c to control LEDs. They have nearly-identical4 copies of a third implementation ofi2c.c
(first seen inps2avrGB
). Thejj40
also has that version, but doesn't seem to use it.All three I2C implementations provide the same feature set. The
lets_split
-derived version has names likei2c_master_start
, as compared toi2c_start
in the other two versions.1 TWI and I2C are the same protocol. "I2C" was trademarked by Philips, so 3rd party manufacturers duplicated the protocol under a different name. The trademark and patent have lapsed, so we might as well say I2C now.
2 Except the version in
handwired/dactyl
has trailing whitespace stripped and a single typo fix.3 Even though the functions are prefixed
i2c
, the filename istwi
master.h
, which makes my heart weep.4Some of them exclude the line
// Please do not modify this file
The text was updated successfully, but these errors were encountered: