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

Add matrix.c to sources for Keychron Q2 #16200

Closed
wants to merge 1 commit into from

Conversation

selmanj
Copy link

@selmanj selmanj commented Feb 4, 2022

Description

This change simply re-adds matrix.c to SRC in the Keychron Q2's rules.mk file for all keyboard revisions. It was removed in #15946, although I don't fully understand why. The PR comment says:

Delete matrix.c, Because we have replaced the resistor R7 from 10KΩ to 100KΩ on hardware so that we can reuse the boot pin of stm32l432 as a gerneral IO.

However without this file included the arrow keys no longer work (as reported in #16097). It would be ideal if @lalalademaxiya1 could confirm this file shouldn't have been removed.

Types of Changes

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

Issues Fixed or Closed by This PR

  • Fixes one issue in 16097

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

This file was removed in qmk#15946 - however without it the arrow keys on the Q2 fail to work.

Partially fixes qmk#16097
@selmanj selmanj force-pushed the fix_missing_matrix_c branch from d12b04f to ce046ea Compare February 4, 2022 22:50
@drashna drashna requested review from fauxpark and a team February 5, 2022 01:59
@tzarc
Copy link
Member

tzarc commented Feb 8, 2022

Would it be possible to give this commit a try instead of your fix?
aebece5

It's a core-side change, but should be much more maintainable than requiring a custom matrix implementation.

@selmanj
Copy link
Author

selmanj commented Feb 9, 2022

@tzarc Just tried it on my Q2 and it works. That's a much cleaner solution - I'm happy to close this PR in favor of that commit.

@tzarc
Copy link
Member

tzarc commented Feb 9, 2022

@tzarc Just tried it on my Q2 and it works. That's a much cleaner solution - I'm happy to close this PR in favor of that commit.

Perfect, I'll sort out a corresponding PR. Would be great if you could give it a thumbs-up after!

@tzarc
Copy link
Member

tzarc commented Feb 9, 2022

Raised PR #16278.

@selmanj
Copy link
Author

selmanj commented Feb 9, 2022

Closing in favor of #16278

@selmanj selmanj closed this Feb 9, 2022
@selmanj selmanj deleted the fix_missing_matrix_c branch February 9, 2022 07:13
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.

2 participants