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

Unify matrix for split common and regular matrix #13330

Merged
merged 6 commits into from
Jul 11, 2021

Conversation

drashna
Copy link
Member

@drashna drashna commented Jun 24, 2021

Description

This merges the split matrix.c with the regular matrix.c, since the files are nearly identical anyways. This will make management of the matrix code simpler, in theory, as there will only be one source of truth for matrix scanning (aside from custom matrices).

Tested on a corne, and verified that it does work properly.

Types of Changes

  • Core
  • Enhancement/optimization

Issues Fixed or Closed by This PR

  • Probably an item on tzarc's to-do list

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • 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 drashna requested a review from a team June 24, 2021 22:30
@github-actions github-actions bot added the core label Jun 24, 2021
@drashna drashna force-pushed the split/unified_matrix branch 2 times, most recently from 6da9c35 to 87a3237 Compare June 24, 2021 22:35
@tzarc tzarc requested a review from a team July 5, 2021 23:02
@mtei
Copy link
Contributor

mtei commented Jul 8, 2021

To my liking, it would be better to move the definition of matrix_post_scan () to the new file quantum/split_common/matrix_split.c. Because I think it's more natural for code that is closely related to transport.c to be in the same hierarchy.

@drashna
Copy link
Member Author

drashna commented Jul 8, 2021

matrix_post_scan

Would it make sense to throw the function into split_util.c instead?
Especially since there are some other functions there already that are being called?

I'm all for doing this, but a quick look, there are a few other things that could also be moved out. I think I'd rather get this in, and then work on separating out more of the split specific stuff (such as the init code, etc)

quantum/matrix.c Outdated Show resolved Hide resolved
@mtei
Copy link
Contributor

mtei commented Jul 10, 2021

I find the following code to be a bit easier to read.

#ifdef DIRECT_PINS_RIGHT
#    define SPLIT_MUTABLE
#else
#    define SPLIT_MUTABLE const
#endif
#ifdef MATRIX_ROW_PINS_RIGHT
#    define SPLIT_MUTABLE_ROW
#else
#    define SPLIT_MUTABLE_ROW const
#endif
#ifdef MATRIX_COL_PINS_RIGHT
#    define SPLIT_MUTABLE_COL
#else
#    define SPLIT_MUTABLE_COL const
#endif

#ifdef DIRECT_PINS
static SPLIT_MUTABLE pin_t direct_pins[MATRIX_ROWS][MATRIX_COLS] = DIRECT_PINS;
#elif (DIODE_DIRECTION == ROW2COL) || (DIODE_DIRECTION == COL2ROW)
#    ifdef MATRIX_ROW_PINS
static SPLIT_MUTABLE_ROW pin_t row_pins[MATRIX_ROWS] = MATRIX_ROW_PINS;
#    endif // MATRIX_ROW_PINS
#    ifdef MATRIX_COL_PINS
static SPLIT_MUTABLE_COL pin_t col_pins[MATRIX_COLS] = MATRIX_COL_PINS;
#    endif // MATRIX_COL_PINS
#endif

@drashna
Copy link
Member Author

drashna commented Jul 10, 2021

It's definitely a lot more concise, and I believe that tzarc mentioned doing something similar.

Applied.

@drashna
Copy link
Member Author

drashna commented Jul 10, 2021

@mtei
Looks like this breaks a couple of the helix boards, due to name overlapping.
Added a commit to rename the files to prevent this issue.

@mtei
Copy link
Contributor

mtei commented Jul 11, 2021

quantum/matrix.c must include quantum/split_common/split_util.h and the other split_util.h must be ignored.
The best way to do this is to explicitly specify split_common/ as follows:

diff --git a/quantum/matrix.c b/quantum/matrix.c
index 7b6954e54..ed4643f81 100644
--- a/quantum/matrix.c
+++ b/quantum/matrix.c
@@ -22,8 +22,8 @@ along with this program.  If not, see <https://www.gnu.org/licenses/>.
 #include "debounce.h"
 #include "quantum.h"
 #ifdef SPLIT_KEYBOARD
-#    include "split_util.h"
-#    include "transactions.h"
+#    include "split_common/split_util.h"
+#    include "split_common/transactions.h"
 
 #    ifndef ERROR_DISCONNECT_COUNT
 #        define ERROR_DISCONNECT_COUNT 5

So you don't need to modify the files under keyboards/helix/.

See the URL below for the behavior of the C preprocessor include.

@drashna
Copy link
Member Author

drashna commented Jul 11, 2021

thanks for the info and correction, @mtei!

I've reverted the commit and replaced it with this.

@drashna drashna merged commit ccc0b23 into qmk:develop Jul 11, 2021
@drashna drashna deleted the split/unified_matrix branch July 11, 2021 21:31
@drashna
Copy link
Member Author

drashna commented Jul 11, 2021

And now I'll see about pulling out more of the split code from the matrix, as well.

firetech added a commit to firetech/qmk_firmware that referenced this pull request Jul 12, 2021
Two occurrences of `MATRIX_ROWS` weren't properly changed to
`ROWS_PER_HAND` in qmk#13330, causing a crash during boot on at least my
Ergodox Infinity (including qmk#13481).
drashna pushed a commit that referenced this pull request Jul 12, 2021
Two occurrences of `MATRIX_ROWS` weren't properly changed to
`ROWS_PER_HAND` in #13330, causing a crash during boot on at least my
Ergodox Infinity (including #13481).
nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
Two occurrences of `MATRIX_ROWS` weren't properly changed to
`ROWS_PER_HAND` in qmk#13330, causing a crash during boot on at least my
Ergodox Infinity (including qmk#13481).
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
Two occurrences of `MATRIX_ROWS` weren't properly changed to
`ROWS_PER_HAND` in qmk#13330, causing a crash during boot on at least my
Ergodox Infinity (including qmk#13481).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants