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

[Core] Add bidirectional and duplex matrix code #8160

Closed
wants to merge 2 commits into from

Conversation

elfmimi
Copy link
Contributor

@elfmimi elfmimi commented Feb 13, 2020

Description

Two things.

1st) Now that we have more than one keyboards that utilize duplexing of matrix by deploying both COL2ROW and ROW2COL at the same time. Namely those keyboards are:

I suggest bringing the so to called COL2ROW2COL code into the core. I labeled it BOTHWAYS .
I've asked the author of those keyboards to test this code. and the result was perfect.

2nd) Using the fact that the technique above fully functions without ghost and with NKRO-bility, it can be proved that key-matrix with inconsistent diode direction can also be made to function, provided proper lines of code. It's worth noting the code for this is also compatible with both COL2ROW and ROW2COL at the same time. I labeled it EITHERWAY .

I suggest using EITHERWAY as a new default for when DIODE_DIRECTION is not defined.
This part is taken back.

This PR will not increase footprints of flash-rom or ram for any of the existing keyboards. It even reduces a few bytes for some split keyboards utilizing split_common.

I've also tested BOTHWAYS and EITHERWAY with my one-off experimental keyboard which has STM32F303, and it too worked great.

Minimum extent of documentation is included.
I'm planning to do extended and in-detail guide later on. after this is accepted.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

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

@elfmimi
Copy link
Contributor Author

elfmimi commented Feb 13, 2020

Schematic drawing of Angel64 can be found here.
https://github.com/kakunpc/angel/blob/master/angel64/debug/README.md

Concerning documentation the part I want to change the most is this line.

So the fullsize matrix shown above would be possible on a Proton C or Teensy++, but not on a regular Teensy or Pro Micro

https://docs.qmk.fm/#/hand_wire

Fullsize keyboard is possible with a single ProMicro.

quantum/config_common.h Outdated Show resolved Hide resolved
quantum/matrix.c Outdated Show resolved Hide resolved
quantum/matrix.c Outdated Show resolved Hide resolved
@elfmimi elfmimi force-pushed the bidirectional-and-duplex-matrix branch from 802cb90 to a1d5b93 Compare Feb 28, 2020
@elfmimi elfmimi marked this pull request as ready for review Feb 28, 2020
@elfmimi elfmimi force-pushed the bidirectional-and-duplex-matrix branch from a1d5b93 to 73e66d7 Compare Feb 28, 2020
@elfmimi
Copy link
Contributor Author

elfmimi commented Feb 28, 2020

Updated according to the requests. (except the symbol names)
Minimal extent of document is added.

@elfmimi elfmimi force-pushed the bidirectional-and-duplex-matrix branch from 73e66d7 to 78ffea9 Compare Feb 29, 2020
@elfmimi elfmimi requested a review from fauxpark Feb 29, 2020
@drashna drashna requested a review from a team Mar 1, 2020
@skullydazed
Copy link
Member

skullydazed commented Mar 1, 2020

This is pretty awesome, thank you for bringing it to core. I haven't reviewed the code in detail yet but I'm excited about what it will mean for custom keyboards.

I'm planning to do documentation later on. after this is accepted.

We'll be able to review the code and test it out, but we'll need documentation on how EITHERWAY and BOTHWAY work before we can merge this.

@elfmimi
Copy link
Contributor Author

elfmimi commented Mar 2, 2020

description updated.

@elfmimi elfmimi force-pushed the bidirectional-and-duplex-matrix branch from 78ffea9 to b2aae55 Compare Mar 2, 2020
@elfmimi
Copy link
Contributor Author

elfmimi commented Mar 2, 2020

cope with qmk cformat (clang-format).

@elfmimi
Copy link
Contributor Author

elfmimi commented Mar 2, 2020

For reference, here is an example how to apply BOTHWAYS to the-dog-keyboard.
elfmimi@d1b7505

The matrix of this keyboard can be found here. #7401 (comment)
https://user-images.githubusercontent.com/15257475/69113624-de717000-0ac6-11ea-8f0b-d603c023e05b.png

@emilytrau
Copy link

emilytrau commented Mar 4, 2020

The matrix of this keyboard can be found here. #7401 (comment)
https://user-images.githubusercontent.com/15257475/69113624-de717000-0ac6-11ea-8f0b-d603c023e05b.png

Hi! Won't this matrix ghost after you press 3 keys? eg. SW1, SW57 and SW8 would cause SW7 to trigger.

@elfmimi
Copy link
Contributor Author

elfmimi commented Mar 8, 2020

That's a real good question. The answer is, It won't! Because Vf * 3 > Vih .
(for commodity diodes like 1N4148)
@angustrau

@sowbug
Copy link
Contributor

sowbug commented Apr 10, 2020

The matrix of this keyboard can be found here. #7401 (comment)
https://user-images.githubusercontent.com/15257475/69113624-de717000-0ac6-11ea-8f0b-d603c023e05b.png

I might be missing something obvious. That link shows a standard matrix, not a duplex one. It has six rows and twenty columns, all the diodes go in the same direction, and every switch belongs to exactly one physical row and column. Did you mean to post a different image?

@elfmimi
Copy link
Contributor Author

elfmimi commented Apr 10, 2020

@sowbug Please take a look once again closely. In the left half of the matrix diodes are pointing upward (ROW2COL) and in the right half downward (COL2ROW) .

@sowbug
Copy link
Contributor

sowbug commented Apr 10, 2020

Yes, I see now. I was looking at adjacent diodes expecting them to alternate direction. Thank you!

quantum/config_common.h Show resolved Hide resolved
@elfmimi
Copy link
Contributor Author

elfmimi commented May 6, 2020

@hineybush's episode is a strikingly well suited example which showcase the enhancement this PR enables.
Though it surely is unwanted accident for him, I'm very proud that I could lend him a helping hand in this.

I heard some voices of anticipation hoping this PR get merged.
So I'm here to provide a quick recap.

At first there was no document, but I've already done decent documentation in the later commit.
Thus 'need doc' label does not apply anymore.

I beleave we've had more than enough time for people to swallow and consume the concept of this PR.
Just in case, there may be someone reading this who find oneself feeling kind of not fully understanding. It's okay. It is not because of lack of documentation. It is because of lack of knowledge in electronics. Just let it go. (possibly offensive language censored)
It really is as simple as "Doing COL2ROW and ROW2COL at the same time." , really.

What left was @fauxpark's statement about his fondness toward the wacky name of COL2ROW2COL.
I think that listing COL2ROW2COL next to COL2ROW and ROW2COL will make those two basic options look more complicated than it is. Especially for someone who is new to QMK. On the other hand, EITHERWAY and BOTHWAYS will definitely look as advanced options. Also you can speculate what they do from their names.
The bottom line is I'm not going to change those names.
I'll welcome usages of COL2ROW2COL as a nickname to BOTHWAYS.

This PR is ready to merge.

Copy link
Member

@skullydazed skullydazed left a comment

Just in case, there may be someone reading this who find oneself feeling kind of not fully understanding. It's okay. It is not because of lack of documentation. It is because of lack of knowledge in electronics. Just let it go.

Please be careful with statements like this. QMK is welcoming to people of all skills levels, and one of our primary missions is to educate. Making confrontational statements runs counter to our code of conduct.

The bottom line is I'm not going to change those names.

I understand your frustration with this process. We've learned through hard-won experience that names matter, and are nearly impossible to change. (See the various ways the word keymap is used in different context as just one example.) That's why we have these conversations, to find the names that are going to be the least confusing in the long run.

As things stand right now we feel that there is not enough distinction between the plain meaning of EITHERWAY and BOTHWAYS for those terms to be used. In particular, people with reading disabilities like dyslexia may have a hard time distinguishing between them. I've suggested names that do not suffer from this ambiguity.

* COL2ROW , ROW2COL , EITHERWAY or BOTHWAYS - how your matrix is configured.
* COL2ROW means the black mark on your diode is facing to the rows or facing away from the cols. This is considered the preferred choice in QMK when you are planning a new design.
* ROW2COL means the opposide. The black mark on your diode is facing to the cols or facing away from the rows.
* EITHERWAY is useful as compensation when a matrix has diodes messed up with their directions. This works prorerly even when arbitrary number of diodes are soldered wrong way. That means this setting is compatible with COL2ROW matrix and ROW2COL matrix at the same time.
* BOTHWAYS is a new strategy to reduce number of pins required to drive a matrix. Compared to a conventional matrix twice the number of keys can be handled. This is considered an advanced topic and will be discussed separately.
Copy link
Member

@skullydazed skullydazed May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been talking about this one internally and while we aren't in total agreement on the names we think these will work better.

Suggested change
* COL2ROW , ROW2COL , EITHERWAY or BOTHWAYS - how your matrix is configured.
* COL2ROW means the black mark on your diode is facing to the rows or facing away from the cols. This is considered the preferred choice in QMK when you are planning a new design.
* ROW2COL means the opposide. The black mark on your diode is facing to the cols or facing away from the rows.
* EITHERWAY is useful as compensation when a matrix has diodes messed up with their directions. This works prorerly even when arbitrary number of diodes are soldered wrong way. That means this setting is compatible with COL2ROW matrix and ROW2COL matrix at the same time.
* BOTHWAYS is a new strategy to reduce number of pins required to drive a matrix. Compared to a conventional matrix twice the number of keys can be handled. This is considered an advanced topic and will be discussed separately.
* `COL2ROW`, `ROW2COL`, `EITHER` or `DUPLEX_SCAN` - how your matrix is configured.
* `COL2ROW` means the black mark on your diode is facing to the rows or facing away from the cols. This is considered the preferred choice in QMK when you are planning a new design.
* `ROW2COL` means the opposite. The black mark on your diode is facing to the cols or facing away from the rows.
* `EITHER` is useful as compensation when a matrix has diodes messed up with their directions. This works properly even when arbitrary number of diodes are soldered wrong way. That means this setting is compatible with `COL2ROW` matrix and `ROW2COL` matrix at the same time.
* `DUPLEX_SCAN` is a new strategy to reduce number of pins required to drive a matrix. Compared to a conventional matrix twice the number of keys can be handled. See the section below on using `DUPLEX_SCAN`.

@@ -31,7 +31,7 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.
#define MATRIX_COLS 11

#define MATRIX_ROW_PINS { B0, B1, B2, B3, B4, B5, B6, B7 }
#define MATRIX_COL_PINS { A0, A1, A2, A3, A4, A5, A6, A7, C7, C6}
#define MATRIX_COL_PINS { A0, A1, A2, A3, A4, A5, A6, A7, C7, C6, C5 }
Copy link
Member

@skullydazed skullydazed May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#define MATRIX_COL_PINS { A0, A1, A2, A3, A4, A5, A6, A7, C7, C6, C5 }
#define MATRIX_COL_PINS { A0, A1, A2, A3, A4, A5, A6, A7, C7, C6}

This change is not relevant to this PR.

@hineybush
Copy link
Contributor

hineybush commented May 13, 2020

That's a real good question. The answer is, It won't! Because Vf * 3 > Vih .
(for commodity diodes like 1N4148)
@angustrau

it actually does cause ghosting. I shipped a few of my "broken" hbcp PCBs out and a customer has had an issue with keys triggering while holding down three others (left shift, left ctrl and left arrow triggers right shift).

luckily I can fix this with the "fixed" PCBs with the correct diodes, once I get them in..

@elfmimi
Copy link
Contributor Author

elfmimi commented May 13, 2020

@hineybush Thank you for sharing your experience with us! I really appreciate it.
You are right. from AT90USB128s' datasheet

31.2 DC characteristics
Vih (min) = 0.6VCC

This will showcase a failure example. Ironically.
Too bad I couldn't help you, @hineybush .

Definitely, I will add a description on this matter.

@stale
Copy link

stale bot commented Jun 27, 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.

@tzarc tzarc added the on hold label Jul 16, 2020
@stale stale bot removed the awaiting changes label Jul 16, 2020
@tzarc tzarc requested review from skullydazed and nooges Oct 30, 2021
@tzarc tzarc changed the base branch from master to develop Oct 30, 2021
@tzarc tzarc removed the on hold label Oct 30, 2021
@tzarc
Copy link
Member

tzarc commented Oct 30, 2021

Has conflicts, and unapplied suggestions.
Removed the on-hold label so stalebot can eventually close it.

@tzarc tzarc added the stale Issues or pull requests that have become inactive without resolution. label Nov 5, 2021
@stale
Copy link

stale bot commented Jan 3, 2022

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

stale bot commented Apr 17, 2022

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.

@stale stale bot closed this Apr 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes core enhancement feature needs doc stale Issues or pull requests that have become inactive without resolution.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants