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

Draft: [Core] Register multiple key events/presses per USB report #12686

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

KarlK90
Copy link
Member

@KarlK90 KarlK90 commented Apr 25, 2021

Description

This is a continuation of #10955 by @hongaaronc (great work!) and addresses open issues with the implementation. Namely tapped key codes and shifted key codes. Please see the original PR for discussion and issues related to it.

Changes to adapt for tapped and subsequent shifted key events

Tapped key events by default work only on key release by sending a key press event and a subsequent key release event to the host. Now with deferring the send event after the complete matrix scan these events are merged and erase each other becoming effectively a nop. The idea is to buffer the unregister event of the key release for all tapped keys (normal keys are not buffered) and send these in a separate keyboard report just after the key press events have been send.

Unregister events/key codes are stored in the unregister_keycodes struct by unregister_code_buffered() which is called for all tapped key codes and send to the host after a complete scan is completed by send_keyboard_report_buffered_unregister_keys().

Subsequent shifted key codes like pressing KC_EQL and KC_PLUS (KC_LSHIFT + KC_EQL) while holding the former need an additional key release event of KC_EQL before sending the combination again. This release event isn't deferred anymore but send as a report immediately.

All failing unit-tests have been fixed or rather adapted for this feature. As it turns out most of the failing key press cases expect one report per key event so they are false negatives. ATM the adapted unit-tests are behind a #if defined guard, I'm not familiar with the CI infrastructure and this needs addressing, currently two binaries would have to be compiled one with the default processing enabled and one with REGISTER_MULTIPLE_KEYEVENTS_ENABLE. I have added a unit-test to ensure that this behavior works as intended.

DEFER_KEYBOARD_REPORT_ENABLE was renamed to REGISTER_MULTIPLE_KEYEVENTS_ENABLE because it rather states a implementation detail but doesn't communicate purpose very well in my opinion.

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

@daskygit
Copy link
Member

This may be intentional, but this breaks TAP_CODE_DELAY when it's enabled with REGISTER_MULTIPLE_KEYEVENTS = yes in rules.mk

I have the polling rate set to 1000hz so taps from LT(_LOWER, KC_TAB) have a very short period between TAB registering and unregistering and some applications don't play nicely with such a short keypress.

Setting #define TAP_CODE_DELAY 5 increases the delay before the unregister event occurs but with this feature enabled it appears to be ignored.

If I disable the feature with REGISTER_MULTIPLE_KEYEVENTS = no in rules.mk TAP_CODE_DELAY works as expected.

I tested the following keycodes:
LT(_LOWER, KC_TAB)
MT(MOD_LCTL, KC_NUBS)

@KarlK90
Copy link
Member Author

KarlK90 commented Apr 26, 2021

Thanks for the feedback!

The breakage is not intentional, I have added tap delays to the unregister process and those keycodes should be working again!

@KarlK90 KarlK90 force-pushed the register-multiple-keyevents-per-report branch 4 times, most recently from cc6c5b6 to da7269f Compare April 29, 2021 22:49
@KarlK90 KarlK90 force-pushed the register-multiple-keyevents-per-report branch from da7269f to 6375202 Compare April 29, 2021 22:50
@github-actions github-actions bot removed the keymap label Apr 29, 2021
@daskygit
Copy link
Member

I had a nice long message typed out about a issue I was having but it made me think a little, it was partly user error. I didn't have QMK_KEYS_PER_SCAN defined but noticed it should be defaulting to 4 and I wasn't seeing a warning message.

@KarlK90
Copy link
Member Author

KarlK90 commented May 2, 2021

Fixed both the typo and the warning. Thank you for the feedback.

@KarlK90 KarlK90 force-pushed the register-multiple-keyevents-per-report branch from 0a4b1d2 to e036bb7 Compare May 5, 2021 13:45
@stale
Copy link

stale bot commented Jun 26, 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.

@daskygit
Copy link
Member

The stale bot brought me here, so just thought I'd add I've been using this for sometime now and not noticed any issues. Thanks again.

@stale stale bot removed the awaiting changes label Jul 1, 2021
@KarlK90 KarlK90 force-pushed the register-multiple-keyevents-per-report branch from 0f6ff08 to b4c70cf Compare July 11, 2021 14:13
tmk_core/common/action.c Outdated Show resolved Hide resolved
@drashna
Copy link
Member

drashna commented Jul 14, 2021

Looks like there are some merge conflicts here that need to be resolved.

@KarlK90 KarlK90 force-pushed the register-multiple-keyevents-per-report branch from 54911d6 to 8e5c489 Compare July 14, 2021 17:34
@KarlK90
Copy link
Member Author

KarlK90 commented Jul 14, 2021

Rebased my commits onto develop and squashed my commits into a single one. Should be good to merge now.

@drashna
Copy link
Member

drashna commented Jul 22, 2021

This definitely causes issues for me.

I'm not sure about other keys, but oneshot mods... if I hit both the OSM key and a regular key at the same time, neither are registered. Disabling this option immediately fixes the issue.

I'm not sure if this effects LT/MT keys too, but I suspect that it may, depending on the tap-hold config options set.

@KarlK90
Copy link
Member Author

KarlK90 commented Jul 23, 2021

Oh well, I guess this is the point where more unit-tests have to be added to spot regressions like this.

I'll see what the root cause is and add those tests.

@daskygit
Copy link
Member

daskygit commented Sep 24, 2021

Just going to give this a little bump, been using this for quite some time with no noticeable problems. Merging it early this cycle would give it plenty of extra testing time. 😉

#endif

#if !defined(QMK_KEYS_PER_SCAN)
# define UNREGISTER_KEYCODES_BUFFER_SIZE 1
Copy link
Member

Choose a reason for hiding this comment

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

not documented?

Copy link
Member

Choose a reason for hiding this comment

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

Not configurable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I'll would like to drop QMK_KEYS_PER_SCAN entirely and give the main keyboard loop a facelift :-), will add documentation nevertheless.

@drashna
Copy link
Member

drashna commented Sep 27, 2021

Just going to give this a little bump, been using this for quite some time with no noticeable problems. Merging it early this cycle would give it plenty of extra testing time. 😉

'm consistently having problems with this, actually.

Se previos statements for... n/m.... yeah. You can see exactly what 'm talking about.

Edit: Note, I do use oneshot mods, and that seems to be where a majority of the issue occurs. but once in a while, mod tap also triggers it.

@KarlK90
Copy link
Member Author

KarlK90 commented Sep 27, 2021

I would very much like to merge #13789 first, afterwards I'll rebase and continue with these issues with proper unit-tests.

@ecerulm
Copy link

ecerulm commented Nov 25, 2021

Also I think it would be great if you could a test case that shows how this is different from the current behaviour. Something like this (not tested and not event sure this is the current behaviour because I can't make it compile with QMK_KEYS_PER_SCAN) :

TEST_F(KeyPress, CorrectKeysAreReportedWhenTwoKeysArePressedX) {
    TestDriver driver;
    press_key(1, 0);
    press_key(0, 3);
   // two keys are pressed how many usb reports will be generated and the contents depends on the features enabled
#if defined(REGISTER_MULTIPLE_KEYEVENTS_ENABLE)
    // single usb report with both keycodes/scancodes
    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_B, KC_C)));
    keyboard_task();
#else
    // two  usb reports each one containing each individual keypress
    // Note that QMK only processes one key at a time
    // See issue #1476 for more information
    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_B)));
    keyboard_task();
    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_B, KC_C)));
    keyboard_task();
#endif
    release_key(1, 0);
    release_key(0, 3);
#if defined(REGISTER_MULTIPLE_KEYEVENTS_ENABLE)  
    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
    keyboard_task();
#else
    // Note that the first key released is the first one in the matrix order
    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_C)));
    keyboard_task();
    EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
    keyboard_task();
#endif
}

This can be run with

make test:basic DEBUG=1 CC=gcc-11 OPT_DEFS=-DREGISTER_MULTIPLE_KEYEVENTS_ENABLE # test with this feature enabld
make test:basic DEBUG=1 CC=gcc-11          # old behaviour,

In my opinion this test is good to have although is more for reference/documenting actual behaviour (more than to test regressions).

On closer inspection, you already have this in the current PR although the code is separated, in the two section of the big #if #else , so nevermind

@KarlK90 KarlK90 changed the title [Core] Register multiple key events/presses per USB report Draft: [Core] Register multiple key events/presses per USB report Nov 25, 2021
@KarlK90 KarlK90 marked this pull request as draft November 25, 2021 09:42
@KarlK90
Copy link
Member Author

KarlK90 commented Nov 25, 2021

Hi @ecerulm I'm currently working on #15292 which deprecates QMK_KEYS_PER_SCAN and makes it the default behavior. Now that we have at least some unit-tests in place I also intend to make the behavior of this PR the default of QMK to reduce the amount of complexity. So basically there won't be any REGISTER_MULTIPLE_KEYEVENTS_ENABLE compile-time switch anymore.

Some Unit-tests are already in place in this PR but more shall be added of course.

@ecerulm
Copy link

ecerulm commented Nov 25, 2021

Looking forward to see this as the standard and only behaviour I don't think there is any need to keep the old behaviour around.

I think it should be mentioned in the documentation in this PR that QMK does not suffer from "chord splitting" anymore and that is suitable for gaming and for rythm games in particular where pressing multiple keys at once with accurate timing is essential (especially at lower polling rates). So that it's easy to find this information for people that google it.

@KarlK90
Copy link
Member Author

KarlK90 commented Nov 25, 2021

Yeah, If you look in the Issues Fixed or Closed by This PR section you can see that the original motivation for this PR was people complained that QMK is not suitable for rhythm games. 🙂 I agree, once this has been merged we should promote it more and get the word out there.

@stale
Copy link

stale bot commented Jan 9, 2022

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.

@XScorpion2
Copy link
Contributor

bump?

@KarlK90
Copy link
Member Author

KarlK90 commented Jan 9, 2022

bump?

Still WIP, I intend to continue with it after #15292

@openglfreak
Copy link

It looks like this code does not work when the matrix scan rate is significantly higher than the USB polling rate

@KarlK90
Copy link
Member Author

KarlK90 commented Feb 12, 2023

It looks like this code does not work when the matrix scan rate is significantly higher than the USB polling rate

You're right. My (yet to be implemented) approach would be to just cap/couple the matrix scanning frequency to the USB polling frequency. The state of the implementation as it is won't be merged tough. I want to revisit the design choices and make the multiple key events behavior the standard/default one.

@zerodividesbyyou

This comment was marked as spam.

@longnguyen2004

This comment was marked as spam.

@StrangePeanut
Copy link

Pardon me for being naïve but for anyone subscribed to this PR in anticipation, what is keeping this draft from being merged as is in a separate commit?

@tzarc
Copy link
Member

tzarc commented Nov 15, 2023

Pardon me for being naïve but for anyone subscribed to this PR in anticipation, what is keeping this draft from being merged as is in a separate commit?

#12686 (comment)

@StrangePeanut
Copy link

@tzarc I get that but it needs to be explicitly enabled and shouldn't affect existing configurations at all. Why couldn't it benefit users who knowingly enable it today?

@tzarc
Copy link
Member

tzarc commented Nov 15, 2023

@tzarc I get that but it needs to be explicitly enabled and shouldn't affect existing configurations at all. Why couldn't it benefit users who knowingly enable it today?

You’re more than welcome to apply the PR to your own fork if you’re happy with its current state.

The original author is not, and that’s enough reason for it not to go to QMK mainline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.