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.

Sign up for GitHub

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

Jump to bottom

Make the size of MATRIX_ROW_PINS and MATRIX_ROW_PINS_RIGHT the same #12203

Merged
merged 2 commits into from
Mar 25, 2021
Merged

Make the size of MATRIX_ROW_PINS and MATRIX_ROW_PINS_RIGHT the same #12203

merged 2 commits into from
Mar 25, 2021

Conversation

takai
Copy link
Contributor

@takai takai commented Mar 12, 2021

Description

As described in this documentation, the size of MATRIX_ROW_PINS and MATRIX_ROW_PINS_RIGHT must be the same.

Currently, the size of MATRIX_ROW_PINS must be the same as MATRIX_ROW_PINS_RIGHT and likewise for the definition of columns.

This PR will fix the problem.

Related PR: #12111

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

mtei
mtei approved these changes Mar 13, 2021
View reviewed changes
Copy link
Contributor

@mtei mtei left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@mtei mtei requested a review from a team March 13, 2021 08:37
fauxpark
fauxpark reviewed Mar 13, 2021
View reviewed changes
Copy link
Member

@fauxpark fauxpark left a comment

Choose a reason for hiding this comment

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

Perhaps using NO_PIN instead would be a little more self-explanatory.

@mtei
Copy link
Contributor

mtei commented Mar 13, 2021 •
edited
Loading

Perhaps using NO_PIN instead would be a little more self-explanatory.

Unfortunately, the current implementation of matrix.c makes NO_PIN meaningful only when DIRECT_PINS is used.

@mtei mtei requested a review from a team March 13, 2021 09:53
@fauxpark
Copy link
Member

That shouldn't matter as in this situation whatever the result of "reading" a NO_PIN would be gets discarded anyway, no?

@mtei
Copy link
Contributor

mtei commented Mar 13, 2021

@takai, I would say it would be better to define matrix_mask as well, just in case. Unfortunately, there is no description of matrix_mask in the documentation, so please see #11974 instead.

@mtei
Copy link
Contributor

mtei commented Mar 13, 2021

That shouldn't matter as in this situation whatever the result of "reading" a NO_PIN would be gets discarded anyway, no?

Is there any guarantee that readPin(NO_PIN) is safe to execute? It looks like there is no guarantee, at least on AVR.

@fauxpark
Copy link
Member

NO_PIN is essentially defined as 0xFF, which none of the supported AVRs use as a register. It would probably be a noop trying to read from it, at most you'd get garbage but again, it's basically ignored by the KC_NOs on those spots.

@drashna
Copy link
Member

drashna commented Mar 14, 2021

@mtei @fauxpark I had this sitting, but: #12238

That should solve any issues with using NO_PIN

@tzarc tzarc merged commit a5ecd4e into qmk:master Mar 25, 2021
violet-fish pushed a commit to violet-fish/qmk_firmware that referenced this pull request Mar 28, 2021
@takai @violet-fish
Make the size of MATRIX_ROW_PINS and MATRIX_ROW_PINS_RIGHT the same (q…
1f0f01c 
…mk#12203)

* Make the size of MATRIX_ROW_PINS and MATRIX_ROW_PINS_RIGHT the same

* Use NO_PIN instead of duplicate pin
mrtnee pushed a commit to mrtnee/qmk_firmware that referenced this pull request Nov 20, 2021
@takai @mrtnee
Make the size of MATRIX_ROW_PINS and MATRIX_ROW_PINS_RIGHT the same (q…
e5b7211 
…mk#12203)

* Make the size of MATRIX_ROW_PINS and MATRIX_ROW_PINS_RIGHT the same

* Use NO_PIN instead of duplicate pin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fauxpark fauxpark fauxpark left review comments

@mtei mtei mtei approved these changes

@drashna drashna drashna approved these changes

@tzarc tzarc tzarc approved these changes

Assignees
No one assigned
Labels
keyboard
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@takai @mtei @fauxpark @drashna @tzarc