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

Update adamtabrams keymap #21344

Closed
wants to merge 46 commits into from
Closed

Update adamtabrams keymap #21344

wants to merge 46 commits into from

Conversation

adamtabrams
Copy link

Description

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

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

* handle mod tap for keys on the same side

* update config values

* remove some tapping force hold

* update keymap and config
@drashna drashna requested a review from a team June 24, 2023 07:50
@dunk2k
Copy link
Contributor

dunk2k commented Jun 26, 2023

CI build failures showing error: 'RGBLED_NUM' undeclared here (not in a function) is because your keymap has specified "rgblight" as enabled but hasn't specified corresponding pin and number of leds (rgblight driver would be good also)

@adamtabrams
Copy link
Author

https://github.com/qmk/qmk_firmware/pull/21344/checks?check_run_id=14573543723

note: include '<math.h>'

Seems a bit strange that I would need to include a library I have no use for to make my keymap compatible with a hardware revision I'm not using.

@fauxpark
Copy link
Member

You can move your keymap under your particular revision folder; it will then only be built for that rev.

@adamtabrams
Copy link
Author

I noticed that QMK CI Build only runs if the latest commit for the PR has modified keymap.c. So a user can make it look like all the checks are passing by making a new commit that only modifies other files. Just FYI.

@adamtabrams
Copy link
Author

keyboards/planck/rev7/matrix.c:26:40: note: include '<math.h>' or provide a declaration of 'log'

If rev7 relies on the log function from math.h shouldn't that be imported in the matrix.c file?

@dunk2k
Copy link
Contributor

dunk2k commented Jun 29, 2023

If rev7 relies on the log function from math.h shouldn't that be imported in the matrix.c file?

I'm in agreement that the build failure for rev7 should be resolved at keyboard level, not keymap level.

@sigprof
Copy link
Contributor

sigprof commented Jun 29, 2023

#21408 should fix the failure for planck/rev7.

Comment on lines +238 to +244
bool is_left_key(int row) {
return row < 4 || row == 7;
}

bool is_right_key(int row) {
return row > 2;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This part is actually specific to planck rev6 (and rev7, which apparently has the same matrix). And technically the handling of rows 3 and 7 is wrong (row 3 is used for both far left and far right keys on the bottom row, and row 7 is used by the central 6 keys of the bottom row).

clear_mods();
for (int i = 0; i < num_mods; i++) {
if (counts[i] == 0) {
tap_code16(mods[i]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This effectively does

Suggested change
tap_code16(mods[i]);
tap_code(QK_MOD_TAP_GET_TAP_KEYCODE(mods[i]));

However, that peculiarity of the tap_code16() behavior (keycodes outside of the QK_BASICQK_MODS_MAX range are truncated to the low byte) does not seem to be documented anywhere.

@github-actions
Copy link

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 bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Oct 14, 2023
Copy link

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-action-closed]

@github-actions github-actions bot closed this Nov 13, 2023
@adamtabrams
Copy link
Author

adamtabrams commented Mar 5, 2024

@drashna @fauxpark I noticed this never go merged in. What changes do I need to make for that to happen?

@zvecr
Copy link
Member

zvecr commented Mar 5, 2024

What changes do I need to make for that to happen?

It wont be merged #22724.

Unfortunately QMK has changed its stance on accepting user keymaps to the repository.

qmk/qmk_firmware is no longer accepting PRs for keymaps other than for manufacturer-supported keymaps. User keymap workflow has been documented here for several years. This change is to progressively reduce the maintenance burden on the project, and to allow us to focus on the core features of QMK.

QMK Firmware now officially supports storing user keymaps outside of the normal QMK Firmware repository, allowing users to maintain their own keymaps without having to fork, modify, and maintain a copy of QMK Firmware themselves.

While we appreciate the time that was taken on this PR, hopefully you understand we are closing it, so that QMK can improve in the long term.

@adamtabrams
Copy link
Author

QMK Firmware now officially supports storing user keymaps outside of the normal QMK Firmware repository,

@zvecr Thanks for the quick response. And I'm glad to hear this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keymap stale Issues or pull requests that have become inactive without resolution.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants