qmk / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
18.27k stars 39.37k forks source link

Generalized matrix.c for keyboards with I/O expanders & TWI #2065

Open ErinCall opened 6 years ago

ErinCall commented 6 years ago

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 custom matrix.c. The result is a big ol' mess. Any features or improvements to quantum/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 a config.h that looks something like

#include i2c.h

//...

#define USES_I2C
#define ONBOARD_MATRIX_ROW_PINS { F0, F1, F4, F5, F6, F7 }
#define ONBOARD_MATRIX_COL_PINS { B1, B2, B3, D2, D3, C6 }
#define I2C_MATRIX_ROW_PINS {B0, B1, B2, B3, B4, B5}
#define I2C_MATRIX_COL_PINS {A0, A1, A2, A3, A4, A5}

Proposal

I'm going to start working toward this proposal.

Details on the mess

The oldest I2C-using matrix.c I can find is in ergodox_ez. There are more-or-less-exact copies in handwired/dactyl and handwired/frenchdev. ergodone has I2C support derived from ergodox_ez's, with a few nice refactors. All of these keyboards have identical2 copies of twimaster.c and i2cmaster.h.3

The deltasplit75, iris, lets_split, levinson, minidox, nyquist, orthodox, and viterbi keyboards use a separate approach (first seen in the lets_split). All of these keyboards have identical copies of i2c.h and i2c.c. hadron also has this version, but as far as I can see it calls i2c_master_start and never does anything else with it.

The bfake, bmini, mechmini, mt40, pearl, ps2avrGB, and ymd96 keyboards use i2c to control LEDs. They have nearly-identical4 copies of a third implementation of i2c.c (first seen in ps2avrGB). The jj40 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 like i2c_master_start, as compared to i2c_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 is twimaster.h, which makes my heart weep. 4Some of them exclude the line // Please do not modify this file

jackhumbert commented 6 years ago

Yeah! Thanks for writing this up :) #1600 touches on this a bit too, and we've been starting with things in #1977, in hopes that the core matrix file can be used for both AVR and ARM boards.

sdbarker commented 6 years ago

@ErinCall Have you made any progress? This would be a huge win for a project I'm currently working on.

drashna commented 6 years ago

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

ErinCall commented 6 years ago

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

sdbarker commented 6 years ago

@ErinCall No worries. I wish you the best, take care of yourself!

drashna commented 6 years ago

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?

ErinCall commented 6 years ago

I have! I’ve been working on this branch. The plan is to bring dactyl/matrix.c into line with quantum/matrix.c, then move the I/O expander code into quantum/matrix.c.

I’m most of the way done with the first part. dactyl/matrix.c has the updated denounce algorithm and DIODE_DIRECTION support, and uses COLUMN_PINS/ROW_PINS for the onboard puns. I’m about halfway done with COLUMN_PINS for the expander pins, at which point it should be ready to start moving into quantum/matrix.c.

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 🤞🏻

drashna commented 6 years ago

Fantastic! Both on your health, and on the progress!

seebs commented 6 years ago

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.

robertrosman commented 5 years ago

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:

// code found in dactyl/config.h
#define EXPANDER_COL_REGISTER 0
#define MATRIX_EXPANDER_COL_PINS {0, 1, 2, 3, 4, 5}
#define MATRIX_EXPANDER_ROW_PINS {0, 1, 2, 3, 4, 5}

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?

ErinCall commented 5 years ago

There are two things that could be throwing you off—

  1. Someone (me) introduced an error into that diagram. The column pins should be on A0 through A5, not INTA through A4. There's a PR to fix it but it's never been merged.
  2. That diagram is based on ergodox-firmware, which numbers the rows in the opposite direction to this code. See qmk's dactyl.h vs. ergodox-firmware's matrix.h: In this code the topmost row is row 0, whereas in ergodox-firmware it's row 5. That's not good, but it will work correctly as long as you wire the topmost row to B0, the thumb cluster "row" to B5, etc.
robertrosman commented 5 years ago

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:

Pin direction and pull-up depends on both the diode direction and on whether the column register is 0 ("A") or 1 ("B"):

Maybe a short comment like this in config.h would make things more clear:

#define EXPANDER_COL_REGISTER 0 // 0 = columns on port A, 1 = columns on port B

drashna commented 5 years ago

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.

vladimir-aubrecht commented 4 years ago

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! 😊

myoung34 commented 4 years ago

I was able to refactor this heavily throughout my PR @vladimir-aubrecht here: https://github.com/qmk/qmk_firmware/pull/9884

vladimir-aubrecht commented 4 years ago

Thank you @myoung34! Is there a plan to have matrix.c with support for extenders also directly in Core?

myoung34 commented 4 years ago

I know there's traction, but I'm definitely not the person to attempt it 😬