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 GET_TAPPING_TERM macro to reduce duplicate code #16681

Merged
merged 2 commits into from
Apr 16, 2022

Conversation

joukewitteveen
Copy link
Contributor

@joukewitteveen joukewitteveen commented Mar 18, 2022

The macro gives the right tapping term depending on whether per-key
tapping terms and/or dynamic tapping terms are enabled. Unnecessary
function calls and variable resolution are avoided.

Description

The time when TAPPING_TERM was the tapping term is over. Per-key and dynamic tapping terms are possible now. This makes using the correct tapping term tricky and it requires some preprocessor-fu. Let's abstract all of that away in a GET_TAPPING_TERM macro modeled after the get_tapping_term function. Using this macro instead of TAPPING_TERM will produce the optimal and correct code for getting the tapping term.

Types of Changes

  • Core
  • Bugfix (edit: partially, as a dynamic tapping term is now respected more broadly)
  • 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).

Copy link
Contributor

@precondition precondition 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 and I didn't notice any firmware size regressions when checking out this PR. In fact, compiling a keymap with AUTO_SHIFT_ENABLE like qmk compile -kb nacly/splitreus62 -km scheiklp for example actually produced a smaller binary, so that's nice.

The macro gives the right tapping term depending on whether per-key
tapping terms and/or dynamic tapping terms are enabled. Unnecessary
function calls and variable resolution are avoided.

Fixes qmk#16472.
@joukewitteveen
Copy link
Contributor Author

I added a mention of the macro to the documentation.

@drashna: The new macro could replace TAP_CHECK in /users/drashna/pointing/pointing.c. Other than that, I did not find any user code refering to get_tapping_term.

@drashna drashna requested a review from a team April 14, 2022 17:04
@KarlK90
Copy link
Member

KarlK90 commented Apr 14, 2022

Very nice, action_tapping.c getting simpler not more complex is always a win.

I found one other place that would benefit from the usage of this macro, it is the cirque trackpoint driver in quantum. Could you add the simplification for CIRQUE_PINNACLE_TAPPING_TERM as well?

https://github.com/qmk/qmk_firmware/blob/develop/quantum/pointing_device_drivers.c#L100-L115

The else branch in in this condition in the cirque driver doesn't make any sense as TAPPING_TERM is always defined so this can just be discarded.

#            ifdef TAPPING_TERM
#                define CIRQUE_PINNACLE_TAPPING_TERM TAPPING_TERM
#            else
#                define CIRQUE_PINNACLE_TAPPING_TERM 200
#            endif

@joukewitteveen
Copy link
Contributor Author

it is the cirque trackpoint driver in quantum.

That looks similar to, but a litttle more complex than the Drashna pointing code. Unfortunately, I'm away from a decent computer for the coming week. Feel free to pile up a commit that takes care of those two pointer usages. Otherwise, I can do it at the end of next week at the earliest.

@KarlK90
Copy link
Member

KarlK90 commented Apr 14, 2022

@joukewitteveen done! @drashna and @precondition please review and feel free to merge if ok.

@drashna drashna merged commit 8f58515 into qmk:develop Apr 16, 2022
cadusk pushed a commit to cadusk/qmk_firmware that referenced this pull request Apr 18, 2022
* qmk/develop: (163 commits)
  Expose API for hardware unique ID (qmk#16869)
  [Keyboard] Add CrimsonKeyboards' Resume1800 (qmk#16842)
  [Keyboard] sandbox - fix keymaps (qmk#16873)
  [Keyboard] Add deskpad (qmk#15602)
  Fix one-shot locked modifiers (qmk#16114)
  Ploopy Trackball Mini: only define DPI options as needed (qmk#16160)
  [Keyboard] Add the Ciel (qmk#16816)
  [Keyboard] Add digicarpice (qmk#16791)
  [Keyboard] SharkPCB release Beta compatibility (qmk#16713)
  Add customizable snake and knight animation increments (qmk#16337)
  [Keyboard] Add sandbox keyboard (qmk#16021)
  Anne Pro 2 Refactor (qmk#16864)
  Refine LED indicator documentation (qmk#16304)
  Fix qmk#16859. (qmk#16865)
  [QP] Check BPP capabilities before loading the palette (qmk#16863)
  rgblight: Add functions to stop blinking one or all but one layer (qmk#16859)
  Heatmap incorrect matrix effect workaround (qmk#16315)
  [Keyboard] Add Phase One keyboard (qmk#16430)
  Fix Xorg segfault with KeebCats PCBs (qmk#16434)
  Add GET_TAPPING_TERM macro to reduce duplicate code (qmk#16681)
  ...
0xcharly pushed a commit to Bastardkb/bastardkb-qmk that referenced this pull request Jul 4, 2022
* Add GET_TAPPING_TERM macro to reduce duplicate code

The macro gives the right tapping term depending on whether per-key
tapping terms and/or dynamic tapping terms are enabled. Unnecessary
function calls and variable resolution are avoided.

Fixes qmk#16472.

* Use GET_TAPPING_TERM for Cirque trackpads

Co-authored-by: Stefan Kerkmann <karlk90@pm.me>
evantravers added a commit to evantravers/qmk_firmware that referenced this pull request Jul 6, 2022
- remove old capsword feature
- upgrade tapping term to new macro
- add new capword

References
- https://getreuer.info/posts/keyboards/caps-word/index.html
- qmk#16681 (comment)
- https://docs.qmk.fm/#/feature_caps_word
evantravers added a commit to evantravers/qmk_firmware that referenced this pull request Oct 27, 2023
- remove old capsword feature
- upgrade tapping term to new macro
- add new capword

References
- https://getreuer.info/posts/keyboards/caps-word/index.html
- qmk#16681 (comment)
- https://docs.qmk.fm/#/feature_caps_word
evantravers added a commit to evantravers/qmk_firmware that referenced this pull request Feb 6, 2024
- remove old capsword feature
- upgrade tapping term to new macro
- add new capword

References
- https://getreuer.info/posts/keyboards/caps-word/index.html
- qmk#16681 (comment)
- https://docs.qmk.fm/#/feature_caps_word
evantravers added a commit to evantravers/qmk_firmware that referenced this pull request Mar 16, 2024
- remove old capsword feature
- upgrade tapping term to new macro
- add new capword

References
- https://getreuer.info/posts/keyboards/caps-word/index.html
- qmk#16681 (comment)
- https://docs.qmk.fm/#/feature_caps_word
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.

4 participants