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

Fix OSMs getting stuck #20034

Merged
merged 9 commits into from
Apr 3, 2023
Merged

Fix OSMs getting stuck #20034

merged 9 commits into from
Apr 3, 2023

Conversation

kasimir-k
Copy link
Contributor

Description

This PR fixes OSMs getting stuck with double tapping or holding another mod. The bug was caused by oneshot_mods being converted to regular mods with registed_mods and thus loosing possibility to unregister those mods, and also not clearing the mods.

The commits of this PR are:

  1. Remove handling of multiple taps: other than ONESHOT_TAP_TOGGLE functionality, single tap and double tap (or n-tap) should be treated equally
  2. Leave OSMs intact on press-and-hold: holding an OSM should behave exactly like a regular mod, and should have no effect on OSMs
  3. Remove redundant outer conditional block: this is mainly to improve readability and also to make it clearer that key-up of a tap has only effect with ONESHOT_TAP_TOGGLE
  4. Add function del_oneshot_locked_mods(): improves readability over set_oneshot_locked_mods(~mods & get_oneshot_locked_mods()); and also makes it clearer that releasing a held mod unlocks it, if it was locked
  5. Fix clearing OSMs: locking a mod with toggle should not clear all OSMs, only the toggled OSM
  6. Improve readability: add comments and change setting OSMs to explicit adding of one mod
  7. Add add_oneshot_locked_mods(): align utils for locked mods with other mods utils

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

@github-actions github-actions bot added the core label Mar 6, 2023
@drashna drashna changed the base branch from master to develop March 6, 2023 18:56
@drashna
Copy link
Member

drashna commented Mar 6, 2023

Could you add tests to verify that this behavior is being processed correctly?

(you can see some tests in tests/basic/test_one_shot_keys.cpp)

@drashna drashna requested a review from a team March 6, 2023 18:57
@kasimir-k
Copy link
Contributor Author

Before writing new tests I wanted to see what the existing tests say, but they said no.

Any pointers how to get them running on a M1 Mac?

❯ make test:basic
QMK Firmware 0.19.9
Making test basic

Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: arm64-apple-darwin22.2.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
Compiling: tests/test_common/test_driver.cpp                                                       In file included from tests/test_common/test_driver.cpp:17:
In file included from tests/test_common/test_driver.hpp:22:
tests/test_common/keyboard_report_util.hpp:38:81: error: non-constant-expression cannot be narrowed from type 'qk_keycode_defines' to 'unsigned char' in initializer list [-Wc++11-narrowing]
    return testing::MakeMatcher(new KeyboardReportMatcher(std::vector<uint8_t>({keys...})));
                                                                                ^~~~
tests/test_common/test_driver.cpp:63:5: note: in instantiation of function template specialization 'KeyboardReport<qk_keycode_defines, qk_keycode_defines, qk_keycode_defines>' requested here
    EXPECT_REPORT(driver, (KC_LCTL, KC_LSFT, KC_U));
    ^
tests/test_common/test_driver.hpp:64:80: note: expanded from macro 'EXPECT_REPORT'
#define EXPECT_REPORT(driver, report) EXPECT_CALL((driver), send_keyboard_mock(KeyboardReport report))
                                                                               ^
tests/test_common/keyboard_report_util.hpp:38:81: note: insert an explicit cast to silence this issue
    return testing::MakeMatcher(new KeyboardReportMatcher(std::vector<uint8_t>({keys...})));
                                                                                ^~~~
                                                                                static_cast<unsigned char>( )
tests/test_common/keyboard_report_util.hpp:38:81: error: non-constant-expression cannot be narrowed from type 'qk_keycode_defines' to 'unsigned char' in initializer list [-Wc++11-narrowing]
    return testing::MakeMatcher(new KeyboardReportMatcher(std::vector<uint8_t>({keys...})));
                                                                                ^~~~
tests/test_common/keyboard_report_util.hpp:38:81: note: insert an explicit cast to silence this issue
    return testing::MakeMatcher(new KeyboardReportMatcher(std::vector<uint8_t>({keys...})));
                                                                                ^~~~
                                                                                static_cast<unsigned char>( )
tests/test_common/keyboard_report_util.hpp:38:81: error: non-constant-expression cannot be narrowed from type 'qk_keycode_defines' to 'unsigned char' in initializer list [-Wc++11-narrowing]
    return testing::MakeMatcher(new KeyboardReportMatcher(std::vector<uint8_t>({keys...})));
                                                                                ^~~~
tests/test_common/keyboard_report_util.hpp:38:81: note: insert an explicit cast to silence this issue
    return testing::MakeMatcher(new KeyboardReportMatcher(std::vector<uint8_t>({keys...})));
                                                                                ^~~~
                                                                                static_cast<unsigned char>( )
tests/test_common/keyboard_report_util.hpp:38:81: error: non-constant-expression cannot be narrowed from type 'qk_keycode_defines' to 'unsigned char' in initializer list [-Wc++11-narrowing]
    return testing::MakeMatcher(new KeyboardReportMatcher(std::vector<uint8_t>({keys...})));
                                                                                ^~~~
tests/test_common/test_driver.cpp:78:5: note: in instantiation of function template specialization 'KeyboardReport<qk_keycode_defines>' requested here
    EXPECT_REPORT(driver, (KC_SPC));
    ^
tests/test_common/test_driver.hpp:64:80: note: expanded from macro 'EXPECT_REPORT'
#define EXPECT_REPORT(driver, report) EXPECT_CALL((driver), send_keyboard_mock(KeyboardReport report))
                                                                               ^
tests/test_common/keyboard_report_util.hpp:38:81: note: insert an explicit cast to silence this issue
    return testing::MakeMatcher(new KeyboardReportMatcher(std::vector<uint8_t>({keys...})));
                                                                                ^~~~
                                                                                static_cast<unsigned char>( )
4 errors generated.
 [ERRORS]
 |
 |
 |
make[1]: *** [.build/test_obj/basic/tests/test_common/test_driver.o] Error 1
Make finished with errors
make: *** [test:basic] Error 1

@fauxpark
Copy link
Member

Nope, clang doesn't like the tests.

@drashna
Copy link
Member

drashna commented Mar 14, 2023

Not for M1, but on mac, I've always had to do:

make test:basic CC=/usr/local/bin/gcc-12

Because, yeah, like fauxpark said, clang doesn't like the tests.

@kasimir-k
Copy link
Contributor Author

Added three tests for:

  • chaining OSMs (not part or my changes, but it was missing and related)
  • OSM double tap not locking previous OSMs
  • OSM holding not locking previous OSMs

Also verified that the two latter tests failed on the master branch as they should.

@drashna drashna requested a review from a team March 23, 2023 07:28
@tzarc tzarc merged commit 4684434 into qmk:develop Apr 3, 2023
coquizen pushed a commit to coquizen/qmk_firmware that referenced this pull request Jun 22, 2023
@mcp292
Copy link

mcp292 commented Oct 20, 2023

@kasimir-k Thank you for your hard, clean work on this! I really appreciate it. Very nice!

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.

5 participants