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 macro consistency check at compile time #12111

Closed
wants to merge 9 commits into from

Conversation

mtei
Copy link
Contributor

@mtei mtei commented Mar 4, 2021

Description

Test the consistency of (MATRIX_ROW_PINS and MATRIX_ROWS) and (MATRIX_COL_PINS and MATRIX_COLS) at compile time.

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

@github-actions github-actions bot added the core label Mar 4, 2021
@mtei
Copy link
Contributor Author

mtei commented Mar 4, 2021

@mtei
Copy link
Contributor Author

mtei commented Mar 4, 2021

@mtei mtei requested a review from a team March 4, 2021 14:31
@tzarc
Copy link
Member

tzarc commented Mar 7, 2021

Apparently I also had a quick stab at similar validation without realising you'd started doing something similar:
https://github.com/qmk/qmk_firmware/compare/develop...tzarc:pin-validations?expand=1

Mine certainly doesn't cater for all the cases, but it is done purely at compile-time without requiring a function definition. Unsure if you'd like to go down this route?

Either way, there are a fair few build errors as a result, with mismatching definitions -- some of which you've also listed yourself.

@mtei
Copy link
Contributor Author

mtei commented Mar 7, 2021

Test_for_matrix_macro_consistency_at_compile_time() is not included in the binary because the compile flag -ffunction-sections and the link flag --gc-sections exclude unused functions from the binary. So we shouldn't have to worry about the binaries getting bigger.

In this PR, Travis-CI reported only the above three cases. I don't think this PR merge will be possible without resolving at least these three cases.

And I'm not sure if this PR should be merged.

@elfmimi
Copy link
Contributor

elfmimi commented Mar 7, 2021

Sorry for interrupting. I made mine into another PR. #12151

@skullydazed
Copy link
Member

I like the idea for this feature but I think it might be better to implement it in python so that more situations can check this. Adding a validation check somewhere around here would be the way to do that.

Now that matrix data can be located in info.json instead of config.h and keyboard.h a lot of users will run qmk info before they ever attempt to compile. Placing the check there will both catch the error earlier and still catch it at compile time.

@mtei
Copy link
Contributor Author

mtei commented Mar 7, 2021

I have organized the implementation of this PR based on the implementation of validations.c done by tzarc. Further revisions are probably not necessary.

This PR only adds _Static_assert(), so it does not affect the generated binary at all. However, because of the three build errors mentioned above, I will leave this PR as draft until these are resolved.

The macros MATRIX_COLS and MATRIX_ROWS are referenced in a number of source files for tmk_core and quantum.

In contrast, the three macros DIRECT_PINS, MATRIX_ROW_PINS, and MATRIX_COL_PINS are referenced only in quantum/matrix.c and quantum/split_common/matrix.c. In other words, DIRECT_PINS, MATRIX_ROW_PINS, and MATRIX_COL_PINS are macros dedicated to the matrix.c implementation that uses only the GPIOs of the current MCU.

Therefore, I think it is most appropriate to do this test in quantum/matrix.c and quantum/split_common/matrix.c.

@skullydazed
Copy link
Member

Therefore, I think it is most appropriate to do this test in quantum/matrix.c and quantum/split_common/matrix.c.

We're starting to validate information like this in more places. Even if this code is in place we will still want to check and highlight this situation at other times, EG qmk info. Validation will help us as we work towards data driven QMK.

Additionally, in python we can choose to throw warnings instead so that currently working but technically incorrect keyboards don't block the PR.

@mtei
Copy link
Contributor Author

mtei commented Mar 8, 2021

I like the direction of the transition to data-driven QMK. However, as of today, I'm not sure how much of that has been achieved and what is being done now.

Even if the check can be done in Python, I think it is worth checking in quantum/matrix.c for the reasons I mentioned above.

@stale
Copy link

stale bot commented Jun 2, 2021

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 Jul 8, 2021

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 Jul 8, 2021
@mtei mtei deleted the matrix_rows_cols_check_consistency branch July 8, 2021 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants