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 retro tapping support #55

Merged
merged 3 commits into from
Jan 25, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion features/achordion.c
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,15 @@ bool process_achordion(uint16_t keycode, keyrecord_t* record) {
}

if (keycode == tap_hold_keycode && !record->event.pressed) {
#ifdef RETRO_TAPPING
bool is_retro_key = IS_QK_MOD_TAP(keycode) || IS_QK_LAYER_TAP(keycode);
#elif defined(RETRO_TAPPING_PER_KEY)
bool is_retro_key = get_retro_tapping(keycode, record);
#else
bool is_retro_key = false;
#endif
// The active tap-hold key is being released.
if (achordion_state == STATE_HOLDING) {
if (achordion_state == STATE_HOLDING || is_retro_key) {
Copy link
Owner

Choose a reason for hiding this comment

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

This is clever! It's a simpler change than I first expected.

I think the idea with the change on this line is that:

  • QMK core has determined the key as held, but Achordion is unsettled.
  • From QMK core's point of view, the key is held and no other keys have been pressed.

So by simply plumbing the hold release event, the subsequent retro tap handling in this condition will tap the tapping keycode. Is that accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep this is accurate, and your explanation with the two bullet points is actually the core idea of the newer implementation. This was more specific for retro tapping only, the newer version supports all kinds of features where no other keypress happens between the press and release of the key.

dprintln("Achordion: Key released. Plumbing hold release.");
tap_hold_record.event.pressed = false;
// Plumb hold release event.
Expand Down Expand Up @@ -257,4 +264,10 @@ __attribute__((weak)) uint16_t achordion_streak_timeout(uint16_t tap_hold_keycod
}
#endif

#ifdef RETRO_TAPPING_PER_KEY
__attribute__((weak)) bool get_retro_tapping(uint16_t keycode, keyrecord_t *record) {
Copy link
Owner

Choose a reason for hiding this comment

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

Is this needed? Core QMK has a weak definition of get_retro_tapping() already:
https://github.com/qmk/qmk_firmware/blob/0c160e1fbafbf477c74e64fd8ab9a9121eb0f42a/quantum/action.c#L63-L67

Duplicate definitions of a function normally cause a linking error. I'm not sure how that goes for weak definitions.

Come to think of it, it's interesting that a weak definition exists at all. Arguably, a user who set RETRO_TAPPING_PER_KEY could be expected to make a (strong) definition of get_retro_tapping(). So the value in having a weak definition is only in the edge case of a mistaken configuration where that is forgotten.

The function get_retro_tapping() should be accessible in this file: achordion.h includes quantum.h, which in turn includes action_tapping.h where the function is declared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right! This is removed in the newer version of this PR, since we no longer need to check the retro tapping key status.

return false;
}
#endif

#endif // version check