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

Allow the exclusions of key presses to not actuate the haptic feedback #12386

Merged
merged 15 commits into from
Jun 20, 2021

Conversation

darkcruix
Copy link

@darkcruix darkcruix commented Mar 26, 2021

Description

With the entry of HAPTIC_EXCLUSION_KEYS 1 in config.h exclusion keys can be enabled.
By default they contain modifier and layer keys, which won't activate the haptic feedback, if set.
Additional keys can be added in config.h in a comma separated list in the form of:
#define HAPTIC_EXCLUSION_KEY_ADDITIONAL { KC_A, KC_B, KC_C }
In case it is desired to exclude only the self-defined and comma-separated list (without having the modifier and layer keys excluded), it can be achieved by turning off the default set in config.h:
#define HAPTIC_EXCLUSION_KEY_DEFAULT(keycode) 0

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

Implementation for exclusion keys for the haptic feedback with the option to activate / de-activate a standard set and define own exclusion keys.
Copy link
Author

@darkcruix darkcruix left a comment

Choose a reason for hiding this comment

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

corrected incorrect formatting

@purdeaandrei
Copy link
Contributor

I am also interested in getting this change through.
I have tested this changeset with HAPTIC_ENABLE=SOLENOID, and works fine for me.
I don't have the hardware to test this with DRV2605L, but the code is generic, and I don't see why it wouldn't work.

@drashna
Copy link
Member

drashna commented Apr 3, 2021

This should be a define, not part of the rules.mk stuff.

Additionally, would rather a bool exclude_haptic_key(uint16_t, keyrecord_t *record) function for determining which keys to exclude or not, much like with how many of the other per key stuff works.

Moved the key exclusion in haptic.c into its own function
Function exclude_haptic_key contains the evaluation for firing the feedback. This includes the default exclusions and the possible self defined keys.
Code has been cleaned up and adjusted to QMK coding style
Indentation corrected
Indentation style
@darkcruix
Copy link
Author

@drashna The requested changes have been implemented (function that determines the keys to exclude and removed the rules.mk portion).

docs/feature_haptic_feedback.md Outdated Show resolved Hide resolved
docs/feature_haptic_feedback.md Outdated Show resolved Hide resolved
drivers/haptic/haptic.c Outdated Show resolved Hide resolved
drivers/haptic/haptic.c Outdated Show resolved Hide resolved
darkcruix and others added 3 commits April 20, 2021 14:29
Co-authored-by: Drashna Jaelre <drashna@live.com>
Co-authored-by: Drashna Jaelre <drashna@live.com>
Co-authored-by: Drashna Jaelre <drashna@live.com>
@darkcruix
Copy link
Author

@drashna Great recommendations! I have committed all changes and tested it against the haptic feedback against all option variations. Only change I'd suggest is to #define HAPTIC_EXCLUSION_KEY_DEFAULT(keycode, tapcount) for consistency in the documentation.

Re-implementation as function - entries in config.h have changed
Implemented as function. The changes have been adjusted to use the implementation style in other areas of QMK.
Since the previous implementation, the way of exclusions have changed and this was also reflected on the doc accordingly.
@darkcruix
Copy link
Author

@drashna Huge thanks for the guidance. I have re-implemented it according the other areas. Tests have been run against all options with a Solenoid.

@drashna drashna requested a review from a team May 9, 2021 03:46
@drashna
Copy link
Member

drashna commented Jun 20, 2021

Could this target develop, actually?
(you can edit the title to change the targeted branch)

@drashna drashna changed the base branch from master to develop June 20, 2021 02:28
@drashna drashna merged commit e4c5b1b into qmk:develop Jun 20, 2021
@drashna
Copy link
Member

drashna commented Jun 20, 2021

n/m, I've done it. And thanks!

Razerban pushed a commit to Razerban/qmk_firmware that referenced this pull request Jul 4, 2021
Co-authored-by: Drashna Jaelre <drashna@live.com>
nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
Co-authored-by: Drashna Jaelre <drashna@live.com>
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
Co-authored-by: Drashna Jaelre <drashna@live.com>
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