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

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

Open
ErinCall opened this issue Nov 26, 2017 · 17 comments
Open

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

ErinCall opened this issue Nov 26, 2017 · 17 comments

Comments

@ErinCall
Copy link
Contributor

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

  • There are i2c.c and i2c.h files in the quantum/ directory
  • Anything anywhere that needs to do I2C stuff uses those versions
  • quantum/matrix.c knows how to read from an I/O expander, if USES_I2C is defined.
  • All keyboards that have a custom matrix.c solely to provide I2C support get to go back to the quantum version of matrix.c

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

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

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

@drashna
Copy link
Member

drashna commented Dec 19, 2017

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
Copy link
Contributor Author

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

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

ErinCall added a commit to ErinCall/qmk_firmware that referenced this issue Feb 4, 2018
This incorporates the fixed/optimized debounce code added to
quantum/matrix.c in:

* 508eddf
* 4c69608
* 32f88c0
* f403028
* a06115d
ErinCall added a commit to ErinCall/qmk_firmware that referenced this issue Feb 4, 2018
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."
ErinCall added a commit to ErinCall/qmk_firmware that referenced this issue Feb 4, 2018
ErinCall added a commit to ErinCall/qmk_firmware that referenced this issue Feb 11, 2018
ErinCall added a commit to ErinCall/qmk_firmware that referenced this issue Feb 11, 2018
Bonus: saves one i2c transaction per matrix_scan!
ErinCall added a commit to ErinCall/qmk_firmware that referenced this issue Feb 11, 2018
@drashna
Copy link
Member

drashna commented Mar 25, 2018

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
Copy link
Contributor Author

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

drashna commented Mar 25, 2018

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

ErinCall added a commit to ErinCall/qmk_firmware that referenced this issue Mar 25, 2018
ErinCall added a commit to ErinCall/qmk_firmware that referenced this issue Mar 25, 2018
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.
ErinCall added a commit to ErinCall/qmk_firmware that referenced this issue Mar 25, 2018
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.
ErinCall added a commit to ErinCall/qmk_firmware that referenced this issue Mar 25, 2018
...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.
@seebs
Copy link
Contributor

seebs commented Apr 8, 2018

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.

jackhumbert pushed a commit that referenced this issue Apr 27, 2018
* 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.
carlpehrson pushed a commit to carlpehrson/qmk_firmware that referenced this issue May 7, 2018
* 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.
hauleth pushed a commit to hauleth/qmk_firmware that referenced this issue Jan 24, 2019
* 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.
@robertrosman
Copy link
Contributor

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
Copy link
Contributor Author

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

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

drashna commented Jun 22, 2019

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

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

myoung34 commented Sep 7, 2020

I was able to refactor this heavily throughout my PR @vladimir-aubrecht here: #9884

@vladimir-aubrecht
Copy link

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

@myoung34
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants