From bfddc1e14af366eaf2dcc27c670ad52b478dd645 Mon Sep 17 00:00:00 2001 From: Pascal Getreuer Date: Sun, 26 Jun 2022 10:52:06 -0700 Subject: [PATCH 1/8] Add Caps Word + Combos test. Motivated by https://github.com/qmk/qmk_firmware/pull/17240, this commit adds a unit test of using Caps Word together with Combos. The test checks that combo chord keys and result keys work as they should for several combos, including overlapping combos and combos that chord tap-hold keys. To check thoroughly, each scenario is repeated to press the combo chord keys with a few different orders and timings. Surprisingly, all tests of this kind that I have tried pass (on master and develop as of 2022-06-26). Maybe the underlying problem has already been fixed, or the real problem is a scenario that hasn't been covered with this test? --- tests/caps_word/caps_word_combo/config.h | 21 ++ tests/caps_word/caps_word_combo/test.mk | 18 ++ .../caps_word_combo/test_caps_word_combo.cpp | 275 ++++++++++++++++++ 3 files changed, 314 insertions(+) create mode 100644 tests/caps_word/caps_word_combo/config.h create mode 100644 tests/caps_word/caps_word_combo/test.mk create mode 100644 tests/caps_word/caps_word_combo/test_caps_word_combo.cpp diff --git a/tests/caps_word/caps_word_combo/config.h b/tests/caps_word/caps_word_combo/config.h new file mode 100644 index 000000000000..c1227044a207 --- /dev/null +++ b/tests/caps_word/caps_word_combo/config.h @@ -0,0 +1,21 @@ +// Copyright 2022 Google LLC +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 2 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +#pragma once + +#include "test_common.h" + +#define TAPPING_TERM 200 + diff --git a/tests/caps_word/caps_word_combo/test.mk b/tests/caps_word/caps_word_combo/test.mk new file mode 100644 index 000000000000..162f9cf3b483 --- /dev/null +++ b/tests/caps_word/caps_word_combo/test.mk @@ -0,0 +1,18 @@ +# Copyright 2022 Google LLC +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +CAPS_WORD_ENABLE = yes +COMBO_ENABLE = yes + diff --git a/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp b/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp new file mode 100644 index 000000000000..b1608f8b068b --- /dev/null +++ b/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp @@ -0,0 +1,275 @@ +// Copyright 2022 Google LLC +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 2 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +#include +#include +#include + +#include "keyboard_report_util.hpp" +#include "keycode.h" +#include "test_common.hpp" +#include "test_fixture.hpp" +#include "test_keymap_key.hpp" + +using ::testing::AnyNumber; +using ::testing::AnyOf; +using ::testing::InSequence; +using ::testing::TestParamInfo; + +extern "C" { +// Define some combos to use for the test, including overlapping combos and +// combos that chord tap-hold keys. +enum combo_events { + AB_COMBO, + BC_COMBO, + AD_COMBO, + DE_COMBO, + FGHI_COMBO, + COMBO_LENGTH +}; +uint16_t COMBO_LEN = COMBO_LENGTH; + +const uint16_t ab_combo[] PROGMEM = {KC_A, KC_B, COMBO_END}; +const uint16_t bc_combo[] PROGMEM = {KC_B, KC_C, COMBO_END}; +const uint16_t ad_combo[] PROGMEM = {KC_A, LCTL_T(KC_D), COMBO_END}; +const uint16_t de_combo[] PROGMEM = {LCTL_T(KC_D), LT(1, KC_E), COMBO_END}; +const uint16_t fghi_combo[] PROGMEM = {KC_F, KC_G, KC_H, KC_I, COMBO_END}; + +combo_t key_combos[] = { + [AB_COMBO] = COMBO(ab_combo, KC_SPC), + [BC_COMBO] = COMBO(bc_combo, KC_X), + [AD_COMBO] = COMBO(ad_combo, KC_Y), + [DE_COMBO] = COMBO(de_combo, KC_Z), + [FGHI_COMBO] = COMBO(fghi_combo, KC_W), +}; +} // extern "C" + +namespace { + +// To test combos thorougly, we test them with pressing the chord keys with +// a few different orders and timings. +struct ComboTapParams { + std::string name; + // True => all chord keys pressed in the same matrix scan. + // False => chord keys pressed one by one in separate scans. + bool all_pressed_in_same_matrix_scan; + // If true, reverse the order in which chord keys are pressed. + bool reverse_press_order; + // True => release chord keys in opposite order in which they are pressed. + // False => release chord keys in the same order as pressed. + bool first_pressed_last_released; + + static const std::string& GetName(const TestParamInfo& info) { + return info.param.name; + } +}; + +class CapsWord : public ::testing::WithParamInterface, public TestFixture { + public: + void SetUp() override { + caps_word_off(); + } + + template + void tap_combo(Ts... chord_keys) { + std::vector keys({chord_keys...}); + std::vector order(keys.size()); + std::iota(order.begin(), order.end(), 0); + + if (GetParam().reverse_press_order) { + std::reverse(order.begin(), order.end()); + } + + if (GetParam().all_pressed_in_same_matrix_scan) { + // Press all keys at once in the same matrix scan. + for (int i : order) { + keys[i].press(); + } + + idle_for(keys.size()); + } else { + // Press keys one by one in separate matrix scans. + for (int i : order) { + keys[i].press(); + run_one_scan_loop(); + } + } + + idle_for(COMBO_TERM + 1); + + if (GetParam().first_pressed_last_released) { + std::reverse(order.begin(), order.end()); + } + + if (GetParam().all_pressed_in_same_matrix_scan) { + // Release all keys at once in the same matrix scan. + for (int i : order) { + keys[i].release(); + } + + // Run enough scan loops so that all release events are processed. + idle_for(keys.size()); + } else { + // Release keys one by one in separate matrix scans. + for (int i : order) { + keys[i].release(); + run_one_scan_loop(); + } + } + } +}; + + + +// Test pressing the keys in a combo with different orders and timings. +TEST_P(CapsWord, SingleCombo) { + TestDriver driver; + KeymapKey key_b(0, 0, 1, KC_B); + KeymapKey key_c(0, 0, 2, KC_C); + set_keymap({key_b, key_c}); + + // Allow any number of reports with no keys or only KC_LSFT. + EXPECT_CALL(driver, send_keyboard_mock(AnyOf( + KeyboardReport(), + KeyboardReport(KC_LSFT)))) + .Times(AnyNumber()); + EXPECT_REPORT(driver, (KC_LSFT, KC_X)); + + caps_word_on(); + tap_combo(key_b, key_c); + + EXPECT_TRUE(is_caps_word_on()); + testing::Mock::VerifyAndClearExpectations(&driver); +} + +// Test a longer 4-key combo. +TEST_P(CapsWord, LongerCombo) { + TestDriver driver; + KeymapKey key_f(0, 0, 0, KC_F); + KeymapKey key_g(0, 0, 1, KC_G); + KeymapKey key_h(0, 0, 2, KC_H); + KeymapKey key_i(0, 0, 3, KC_I); + set_keymap({key_f, key_g, key_h, key_i}); + + // Allow any number of reports with no keys or only KC_LSFT. + EXPECT_CALL(driver, send_keyboard_mock(AnyOf( + KeyboardReport(), + KeyboardReport(KC_LSFT)))) + .Times(AnyNumber()); + EXPECT_REPORT(driver, (KC_LSFT, KC_W)); + + caps_word_on(); + tap_combo(key_f, key_g, key_h, key_i); + + EXPECT_TRUE(is_caps_word_on()); + testing::Mock::VerifyAndClearExpectations(&driver); +} + +// Test with two overlapping combos on regular keys: +// KC_A + KC_B = KC_SPC, +// KC_B + KC_C = KC_X. +TEST_P(CapsWord, ComboRegularKeys) { + TestDriver driver; + KeymapKey key_a(0, 0, 0, KC_A); + KeymapKey key_b(0, 0, 1, KC_B); + KeymapKey key_c(0, 0, 2, KC_C); + set_keymap({key_a, key_b, key_c}); + + // Allow any number of reports with no keys or only KC_LSFT. + // clang-format off + EXPECT_CALL(driver, send_keyboard_mock(AnyOf( + KeyboardReport(), + KeyboardReport(KC_LSFT)))) + .Times(AnyNumber()); + // clang-format on + { // Expect: "A, B, C, X, space, a". + InSequence s; + EXPECT_REPORT(driver, (KC_LSFT, KC_A)); + EXPECT_REPORT(driver, (KC_LSFT, KC_B)); + EXPECT_REPORT(driver, (KC_LSFT, KC_C)); + EXPECT_REPORT(driver, (KC_LSFT, KC_X)); + EXPECT_REPORT(driver, (KC_SPC)); + EXPECT_REPORT(driver, (KC_A)); + } + + caps_word_on(); + tap_key(key_a); + tap_key(key_b); + tap_key(key_c); + tap_combo(key_b, key_c); // BC combo types "x". + tap_combo(key_a, key_b); // AB combo types space. + tap_key(key_a); + + EXPECT_FALSE(is_caps_word_on()); + testing::Mock::VerifyAndClearExpectations(&driver); +} + +// Test where combo chords involve tap-hold keys: +// KC_A + LCTL_T(KC_D) = KC_Y, +// LCTL_T(KC_D) + LT(1, KC_E) = KC_Z, +TEST_P(CapsWord, ComboModTapKey) { + TestDriver driver; + KeymapKey key_a(0, 0, 0, KC_A); + KeymapKey key_modtap_d(0, 0, 1, LCTL_T(KC_D)); + KeymapKey key_layertap_e(0, 0, 2, LT(1, KC_E)); + set_keymap({key_a, key_modtap_d, key_layertap_e}); + + // Allow any number of reports with no keys or only KC_LSFT. + // clang-format off + EXPECT_CALL(driver, send_keyboard_mock(AnyOf( + KeyboardReport(), + KeyboardReport(KC_LSFT)))) + .Times(AnyNumber()); + // clang-format on + { // Expect: "A, D, E, Y, Z". + InSequence s; + EXPECT_REPORT(driver, (KC_LSFT, KC_A)); + EXPECT_REPORT(driver, (KC_LSFT, KC_D)); + EXPECT_REPORT(driver, (KC_LSFT, KC_E)); + EXPECT_REPORT(driver, (KC_LSFT, KC_Y)); + EXPECT_REPORT(driver, (KC_LSFT, KC_Z)); + } + + caps_word_on(); + tap_key(key_a); + tap_key(key_modtap_d); + tap_key(key_layertap_e); + tap_combo(key_a, key_modtap_d); // AD combo types "y". + tap_combo(key_modtap_d, key_layertap_e); // DE combo types "z". + + EXPECT_TRUE(is_caps_word_on()); + testing::Mock::VerifyAndClearExpectations(&driver); +} + +// clang-format off +INSTANTIATE_TEST_CASE_P( + Combos, + CapsWord, + ::testing::Values( + ComboTapParams{ + "PressInSeparateScans", false, false, false}, + ComboTapParams{ + "ReversePressOrder", false, true, false}, + ComboTapParams{ + "FirstPressedLastReleased", false, false, true}, + ComboTapParams{ + "PressInSameScan", true, false, false} + ), + ComboTapParams::GetName + ); +// clang-format on + +} // namespace + From d7f14884cef477118a84a90f4023c3837d0046f3 Mon Sep 17 00:00:00 2001 From: Pascal Getreuer Date: Sat, 2 Jul 2022 13:26:04 -0700 Subject: [PATCH 2/8] Fix Caps Word + Combos + Auto Shift --- quantum/process_keycode/process_combo.c | 10 ++ tests/caps_word/caps_word_combo/test.mk | 1 + .../caps_word_combo/test_caps_word_combo.cpp | 105 ++++++------------ 3 files changed, 45 insertions(+), 71 deletions(-) diff --git a/quantum/process_keycode/process_combo.c b/quantum/process_keycode/process_combo.c index d5a649adb360..f00ad6202726 100644 --- a/quantum/process_keycode/process_combo.c +++ b/quantum/process_keycode/process_combo.c @@ -227,7 +227,17 @@ static inline void dump_key_buffer(void) { #endif } record->event.time = 0; + +#if defined(CAPS_WORD_ENABLE) && defined(AUTO_SHIFT_ENABLE) + // Edge case: preserve the weak Left Shift mod if both Caps Word and + // Auto Shift are on. Caps Word capitalizes by setting the weak Left + // Shift mod during the press event, but Auto Shift doesn't send the + // key until it receives the release event. + del_weak_mods((is_caps_word_on() && get_autoshift_state()) + ? ~MOD_BIT(KC_LSFT) : 0xff); +#else clear_weak_mods(); +#endif // CAPS_WORD_ENABLE #if TAP_CODE_DELAY > 0 // only delay once and for a non-tapping key diff --git a/tests/caps_word/caps_word_combo/test.mk b/tests/caps_word/caps_word_combo/test.mk index 162f9cf3b483..9f2e157189fe 100644 --- a/tests/caps_word/caps_word_combo/test.mk +++ b/tests/caps_word/caps_word_combo/test.mk @@ -15,4 +15,5 @@ CAPS_WORD_ENABLE = yes COMBO_ENABLE = yes +AUTO_SHIFT_ENABLE = yes diff --git a/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp b/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp index b1608f8b068b..c5a6287c8029 100644 --- a/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp +++ b/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp @@ -13,6 +13,9 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . + +// Test Caps Word + Combos, with and without Auto Shift. + #include #include #include @@ -60,79 +63,39 @@ namespace { // To test combos thorougly, we test them with pressing the chord keys with // a few different orders and timings. -struct ComboTapParams { +struct TestParams { std::string name; - // True => all chord keys pressed in the same matrix scan. - // False => chord keys pressed one by one in separate scans. - bool all_pressed_in_same_matrix_scan; - // If true, reverse the order in which chord keys are pressed. - bool reverse_press_order; - // True => release chord keys in opposite order in which they are pressed. - // False => release chord keys in the same order as pressed. - bool first_pressed_last_released; - - static const std::string& GetName(const TestParamInfo& info) { + bool autoshift_on; + + static const std::string& GetName(const TestParamInfo& info) { return info.param.name; } }; -class CapsWord : public ::testing::WithParamInterface, public TestFixture { +class CapsWord : public ::testing::WithParamInterface, public TestFixture { public: void SetUp() override { caps_word_off(); - } - - template - void tap_combo(Ts... chord_keys) { - std::vector keys({chord_keys...}); - std::vector order(keys.size()); - std::iota(order.begin(), order.end(), 0); - - if (GetParam().reverse_press_order) { - std::reverse(order.begin(), order.end()); - } - - if (GetParam().all_pressed_in_same_matrix_scan) { - // Press all keys at once in the same matrix scan. - for (int i : order) { - keys[i].press(); - } - - idle_for(keys.size()); + if (GetParam().autoshift_on) { + autoshift_enable(); } else { - // Press keys one by one in separate matrix scans. - for (int i : order) { - keys[i].press(); - run_one_scan_loop(); - } + autoshift_disable(); } + } - idle_for(COMBO_TERM + 1); - - if (GetParam().first_pressed_last_released) { - std::reverse(order.begin(), order.end()); + void tap_combo(const std::vector& chord_keys) { + for (KeymapKey key : chord_keys) { // Press each key. + key.press(); + run_one_scan_loop(); } - if (GetParam().all_pressed_in_same_matrix_scan) { - // Release all keys at once in the same matrix scan. - for (int i : order) { - keys[i].release(); - } - - // Run enough scan loops so that all release events are processed. - idle_for(keys.size()); - } else { - // Release keys one by one in separate matrix scans. - for (int i : order) { - keys[i].release(); - run_one_scan_loop(); - } + for (KeymapKey key : chord_keys) { // Release each key. + key.release(); + run_one_scan_loop(); } } }; - - // Test pressing the keys in a combo with different orders and timings. TEST_P(CapsWord, SingleCombo) { TestDriver driver; @@ -148,9 +111,11 @@ TEST_P(CapsWord, SingleCombo) { EXPECT_REPORT(driver, (KC_LSFT, KC_X)); caps_word_on(); - tap_combo(key_b, key_c); + tap_combo({key_b, key_c}); EXPECT_TRUE(is_caps_word_on()); + caps_word_off(); + testing::Mock::VerifyAndClearExpectations(&driver); } @@ -171,9 +136,11 @@ TEST_P(CapsWord, LongerCombo) { EXPECT_REPORT(driver, (KC_LSFT, KC_W)); caps_word_on(); - tap_combo(key_f, key_g, key_h, key_i); + tap_combo({key_f, key_g, key_h, key_i}); EXPECT_TRUE(is_caps_word_on()); + caps_word_off(); + testing::Mock::VerifyAndClearExpectations(&driver); } @@ -208,8 +175,8 @@ TEST_P(CapsWord, ComboRegularKeys) { tap_key(key_a); tap_key(key_b); tap_key(key_c); - tap_combo(key_b, key_c); // BC combo types "x". - tap_combo(key_a, key_b); // AB combo types space. + tap_combo({key_b, key_c}); // BC combo types "x". + tap_combo({key_a, key_b}); // AB combo types space. tap_key(key_a); EXPECT_FALSE(is_caps_word_on()); @@ -246,10 +213,12 @@ TEST_P(CapsWord, ComboModTapKey) { tap_key(key_a); tap_key(key_modtap_d); tap_key(key_layertap_e); - tap_combo(key_a, key_modtap_d); // AD combo types "y". - tap_combo(key_modtap_d, key_layertap_e); // DE combo types "z". + tap_combo({key_a, key_modtap_d}); // AD combo types "y". + tap_combo({key_modtap_d, key_layertap_e}); // DE combo types "z". EXPECT_TRUE(is_caps_word_on()); + caps_word_off(); + testing::Mock::VerifyAndClearExpectations(&driver); } @@ -258,16 +227,10 @@ INSTANTIATE_TEST_CASE_P( Combos, CapsWord, ::testing::Values( - ComboTapParams{ - "PressInSeparateScans", false, false, false}, - ComboTapParams{ - "ReversePressOrder", false, true, false}, - ComboTapParams{ - "FirstPressedLastReleased", false, false, true}, - ComboTapParams{ - "PressInSameScan", true, false, false} + TestParams{"AutoshiftDisabled", false}, + TestParams{"AutoshiftEnabled", true} ), - ComboTapParams::GetName + TestParams::GetName ); // clang-format on From 6413582db8b97e95a17935fd8df9d81222bf738e Mon Sep 17 00:00:00 2001 From: Pascal Getreuer Date: Sat, 2 Jul 2022 18:22:39 -0700 Subject: [PATCH 3/8] Code formatting --- quantum/process_keycode/process_combo.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/quantum/process_keycode/process_combo.c b/quantum/process_keycode/process_combo.c index f00ad6202726..3066167f2bce 100644 --- a/quantum/process_keycode/process_combo.c +++ b/quantum/process_keycode/process_combo.c @@ -211,7 +211,7 @@ static inline void dump_key_buffer(void) { key_buffer_next = key_buffer_i + 1; queued_record_t *qrecord = &key_buffer[key_buffer_i]; - keyrecord_t * record = &qrecord->record; + keyrecord_t *record = &qrecord->record; if (IS_NOEVENT(record->event)) { continue; @@ -233,11 +233,10 @@ static inline void dump_key_buffer(void) { // Auto Shift are on. Caps Word capitalizes by setting the weak Left // Shift mod during the press event, but Auto Shift doesn't send the // key until it receives the release event. - del_weak_mods((is_caps_word_on() && get_autoshift_state()) - ? ~MOD_BIT(KC_LSFT) : 0xff); + del_weak_mods((is_caps_word_on() && get_autoshift_state()) ? ~MOD_BIT(KC_LSFT) : 0xff); #else clear_weak_mods(); -#endif // CAPS_WORD_ENABLE +#endif // defined(CAPS_WORD_ENABLE) && defined(AUTO_SHIFT_ENABLE) #if TAP_CODE_DELAY > 0 // only delay once and for a non-tapping key @@ -312,7 +311,7 @@ void apply_combo(uint16_t combo_index, combo_t *combo) { for (uint8_t key_buffer_i = 0; key_buffer_i < key_buffer_size; key_buffer_i++) { queued_record_t *qrecord = &key_buffer[key_buffer_i]; - keyrecord_t * record = &qrecord->record; + keyrecord_t *record = &qrecord->record; uint16_t keycode = qrecord->keycode; uint8_t key_count = 0; @@ -347,7 +346,7 @@ static inline void apply_combos(void) { // Apply all buffered normal combos. for (uint8_t i = combo_buffer_read; i != combo_buffer_write; INCREMENT_MOD(i)) { queued_combo_t *buffered_combo = &combo_buffer[i]; - combo_t * combo = &key_combos[buffered_combo->combo_index]; + combo_t *combo = &key_combos[buffered_combo->combo_index]; #ifdef COMBO_MUST_TAP_PER_COMBO if (get_combo_must_tap(buffered_combo->combo_index, combo)) { @@ -452,7 +451,7 @@ static bool process_single_combo(combo_t *combo, uint16_t keycode, keyrecord_t * combo_t *drop = NULL; for (uint8_t combo_buffer_i = combo_buffer_read; combo_buffer_i != combo_buffer_write; INCREMENT_MOD(combo_buffer_i)) { queued_combo_t *qcombo = &combo_buffer[combo_buffer_i]; - combo_t * buffered_combo = &key_combos[qcombo->combo_index]; + combo_t *buffered_combo = &key_combos[qcombo->combo_index]; if ((drop = overlaps(buffered_combo, combo))) { DISABLE_COMBO(drop); From 809571d690dbef5ef0df59ab9dc747344db7e1eb Mon Sep 17 00:00:00 2001 From: Pascal Getreuer Date: Sat, 2 Jul 2022 18:31:32 -0700 Subject: [PATCH 4/8] Add unshifted KC_1 key to test --- .../caps_word_combo/test_caps_word_combo.cpp | 66 ++++++++----------- 1 file changed, 26 insertions(+), 40 deletions(-) diff --git a/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp b/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp index c5a6287c8029..6dda5dcc75d1 100644 --- a/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp +++ b/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp @@ -13,7 +13,6 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . - // Test Caps Word + Combos, with and without Auto Shift. #include @@ -34,30 +33,19 @@ using ::testing::TestParamInfo; extern "C" { // Define some combos to use for the test, including overlapping combos and // combos that chord tap-hold keys. -enum combo_events { - AB_COMBO, - BC_COMBO, - AD_COMBO, - DE_COMBO, - FGHI_COMBO, - COMBO_LENGTH -}; +enum combo_events { AB_COMBO, BC_COMBO, AD_COMBO, DE_COMBO, FGHI_COMBO, COMBO_LENGTH }; uint16_t COMBO_LEN = COMBO_LENGTH; -const uint16_t ab_combo[] PROGMEM = {KC_A, KC_B, COMBO_END}; -const uint16_t bc_combo[] PROGMEM = {KC_B, KC_C, COMBO_END}; -const uint16_t ad_combo[] PROGMEM = {KC_A, LCTL_T(KC_D), COMBO_END}; -const uint16_t de_combo[] PROGMEM = {LCTL_T(KC_D), LT(1, KC_E), COMBO_END}; +const uint16_t ab_combo[] PROGMEM = {KC_A, KC_B, COMBO_END}; +const uint16_t bc_combo[] PROGMEM = {KC_B, KC_C, COMBO_END}; +const uint16_t ad_combo[] PROGMEM = {KC_A, LCTL_T(KC_D), COMBO_END}; +const uint16_t de_combo[] PROGMEM = {LCTL_T(KC_D), LT(1, KC_E), COMBO_END}; const uint16_t fghi_combo[] PROGMEM = {KC_F, KC_G, KC_H, KC_I, COMBO_END}; combo_t key_combos[] = { - [AB_COMBO] = COMBO(ab_combo, KC_SPC), - [BC_COMBO] = COMBO(bc_combo, KC_X), - [AD_COMBO] = COMBO(ad_combo, KC_Y), - [DE_COMBO] = COMBO(de_combo, KC_Z), - [FGHI_COMBO] = COMBO(fghi_combo, KC_W), + [AB_COMBO] = COMBO(ab_combo, KC_SPC), [BC_COMBO] = COMBO(bc_combo, KC_X), [AD_COMBO] = COMBO(ad_combo, KC_Y), [DE_COMBO] = COMBO(de_combo, KC_Z), [FGHI_COMBO] = COMBO(fghi_combo, KC_W), }; -} // extern "C" +} // extern "C" namespace { @@ -65,7 +53,7 @@ namespace { // a few different orders and timings. struct TestParams { std::string name; - bool autoshift_on; + bool autoshift_on; static const std::string& GetName(const TestParamInfo& info) { return info.param.name; @@ -73,7 +61,7 @@ struct TestParams { }; class CapsWord : public ::testing::WithParamInterface, public TestFixture { - public: + public: void SetUp() override { caps_word_off(); if (GetParam().autoshift_on) { @@ -84,12 +72,12 @@ class CapsWord : public ::testing::WithParamInterface, public TestFi } void tap_combo(const std::vector& chord_keys) { - for (KeymapKey key : chord_keys) { // Press each key. + for (KeymapKey key : chord_keys) { // Press each key. key.press(); run_one_scan_loop(); } - for (KeymapKey key : chord_keys) { // Release each key. + for (KeymapKey key : chord_keys) { // Release each key. key.release(); run_one_scan_loop(); } @@ -104,10 +92,7 @@ TEST_P(CapsWord, SingleCombo) { set_keymap({key_b, key_c}); // Allow any number of reports with no keys or only KC_LSFT. - EXPECT_CALL(driver, send_keyboard_mock(AnyOf( - KeyboardReport(), - KeyboardReport(KC_LSFT)))) - .Times(AnyNumber()); + EXPECT_CALL(driver, send_keyboard_mock(AnyOf(KeyboardReport(), KeyboardReport(KC_LSFT)))).Times(AnyNumber()); EXPECT_REPORT(driver, (KC_LSFT, KC_X)); caps_word_on(); @@ -129,10 +114,7 @@ TEST_P(CapsWord, LongerCombo) { set_keymap({key_f, key_g, key_h, key_i}); // Allow any number of reports with no keys or only KC_LSFT. - EXPECT_CALL(driver, send_keyboard_mock(AnyOf( - KeyboardReport(), - KeyboardReport(KC_LSFT)))) - .Times(AnyNumber()); + EXPECT_CALL(driver, send_keyboard_mock(AnyOf(KeyboardReport(), KeyboardReport(KC_LSFT)))).Times(AnyNumber()); EXPECT_REPORT(driver, (KC_LSFT, KC_W)); caps_word_on(); @@ -152,7 +134,8 @@ TEST_P(CapsWord, ComboRegularKeys) { KeymapKey key_a(0, 0, 0, KC_A); KeymapKey key_b(0, 0, 1, KC_B); KeymapKey key_c(0, 0, 2, KC_C); - set_keymap({key_a, key_b, key_c}); + KeymapKey key_1(0, 0, 3, KC_1); + set_keymap({key_a, key_b, key_c, key_1}); // Allow any number of reports with no keys or only KC_LSFT. // clang-format off @@ -161,12 +144,14 @@ TEST_P(CapsWord, ComboRegularKeys) { KeyboardReport(KC_LSFT)))) .Times(AnyNumber()); // clang-format on - { // Expect: "A, B, C, X, space, a". + { // Expect: "A, B, 1, X, 1, C, space, a". InSequence s; EXPECT_REPORT(driver, (KC_LSFT, KC_A)); EXPECT_REPORT(driver, (KC_LSFT, KC_B)); - EXPECT_REPORT(driver, (KC_LSFT, KC_C)); + EXPECT_REPORT(driver, (KC_1)); EXPECT_REPORT(driver, (KC_LSFT, KC_X)); + EXPECT_REPORT(driver, (KC_1)); + EXPECT_REPORT(driver, (KC_LSFT, KC_C)); EXPECT_REPORT(driver, (KC_SPC)); EXPECT_REPORT(driver, (KC_A)); } @@ -174,9 +159,11 @@ TEST_P(CapsWord, ComboRegularKeys) { caps_word_on(); tap_key(key_a); tap_key(key_b); + tap_key(key_1); + tap_combo({key_b, key_c}); // BC combo types "x". + tap_key(key_1); tap_key(key_c); - tap_combo({key_b, key_c}); // BC combo types "x". - tap_combo({key_a, key_b}); // AB combo types space. + tap_combo({key_a, key_b}); // AB combo types space. tap_key(key_a); EXPECT_FALSE(is_caps_word_on()); @@ -213,8 +200,8 @@ TEST_P(CapsWord, ComboModTapKey) { tap_key(key_a); tap_key(key_modtap_d); tap_key(key_layertap_e); - tap_combo({key_a, key_modtap_d}); // AD combo types "y". - tap_combo({key_modtap_d, key_layertap_e}); // DE combo types "z". + tap_combo({key_a, key_modtap_d}); // AD combo types "y". + tap_combo({key_modtap_d, key_layertap_e}); // DE combo types "z". EXPECT_TRUE(is_caps_word_on()); caps_word_off(); @@ -234,5 +221,4 @@ INSTANTIATE_TEST_CASE_P( ); // clang-format on -} // namespace - +} // namespace From 5ece236ecf64ddc9a4617254ab16f632c61df8b4 Mon Sep 17 00:00:00 2001 From: Pascal Getreuer Date: Sat, 2 Jul 2022 18:47:01 -0700 Subject: [PATCH 5/8] Move tap_combo to TestFixture --- .../caps_word_combo/test_caps_word_combo.cpp | 12 ------------ tests/test_common/test_fixture.cpp | 16 ++++++++++++++++ tests/test_common/test_fixture.hpp | 7 +++++++ 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp b/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp index 6dda5dcc75d1..df822afee3ff 100644 --- a/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp +++ b/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp @@ -70,18 +70,6 @@ class CapsWord : public ::testing::WithParamInterface, public TestFi autoshift_disable(); } } - - void tap_combo(const std::vector& chord_keys) { - for (KeymapKey key : chord_keys) { // Press each key. - key.press(); - run_one_scan_loop(); - } - - for (KeymapKey key : chord_keys) { // Release each key. - key.release(); - run_one_scan_loop(); - } - } }; // Test pressing the keys in a combo with different orders and timings. diff --git a/tests/test_common/test_fixture.cpp b/tests/test_common/test_fixture.cpp index 5fc6964054bb..44694cd390c3 100644 --- a/tests/test_common/test_fixture.cpp +++ b/tests/test_common/test_fixture.cpp @@ -108,6 +108,22 @@ void TestFixture::tap_key(KeymapKey key, unsigned delay_ms) { run_one_scan_loop(); } +void TestFixture::tap_combo(const std::vector& chord_keys, unsigned delay_ms) { + for (KeymapKey key : chord_keys) { // Press each key. + key.press(); + run_one_scan_loop(); + } + + if (delay_ms > 1) { + idle_for(delay_ms - 1); + } + + for (KeymapKey key : chord_keys) { // Release each key. + key.release(); + run_one_scan_loop(); + } +} + void TestFixture::set_keymap(std::initializer_list keys) { this->keymap.clear(); for (auto& key : keys) { diff --git a/tests/test_common/test_fixture.hpp b/tests/test_common/test_fixture.hpp index 81906f76c7c3..2590acd006e1 100644 --- a/tests/test_common/test_fixture.hpp +++ b/tests/test_common/test_fixture.hpp @@ -53,6 +53,13 @@ class TestFixture : public testing::Test { } } + /** + * @brief Taps a combo with `delay_ms` delay between press and release. + * + * Example: `tap_combo({key_a, key_b})` to tap the chord A + B. + */ + void tap_combo(const std::vector& chord_keys, unsigned delay_ms = 1); + void run_one_scan_loop(); void idle_for(unsigned ms); From ded5ff5a411f80c354c74a8035c940e7df06bba2 Mon Sep 17 00:00:00 2001 From: Pascal Getreuer Date: Sat, 2 Jul 2022 22:26:10 -0700 Subject: [PATCH 6/8] Fix formatting --- tests/caps_word/caps_word_combo/config.h | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/caps_word/caps_word_combo/config.h b/tests/caps_word/caps_word_combo/config.h index c1227044a207..92dbe045b298 100644 --- a/tests/caps_word/caps_word_combo/config.h +++ b/tests/caps_word/caps_word_combo/config.h @@ -18,4 +18,3 @@ #include "test_common.h" #define TAPPING_TERM 200 - From b43790cbd185d192cd75a9b8ba17343b7ffb5e2b Mon Sep 17 00:00:00 2001 From: Pascal Getreuer Date: Wed, 13 Jul 2022 09:35:56 -0700 Subject: [PATCH 7/8] Fix rolled presses with Caps Word + Auto Shift. If Caps Word and Auto Shift are both active, then currently a rolled press like "A down, B down, A up, B up" will incorrectly produce "Ab". This is because Auto Shift clears the weak shift mod when the A key is released, which in the case of a roll is *after* Caps Word has set the weak shift mod for B when it was pressed. This commit incorporates https://github.com/zsa/qmk_firmware/commit/42519ed513a6f07791d6a0e6012ef8ffa4d93e9f which fixes it. An additional test `AutoShiftRolledShiftedKeys` is added, which would previously fail but passes with this fix. --- quantum/process_keycode/process_auto_shift.c | 7 +- quantum/process_keycode/process_caps_word.c | 4 ++ .../test_caps_word_autoshift.cpp | 64 +++++++++++++++---- .../caps_word_combo/test_caps_word_combo.cpp | 38 +++++------ 4 files changed, 79 insertions(+), 34 deletions(-) diff --git a/quantum/process_keycode/process_auto_shift.c b/quantum/process_keycode/process_auto_shift.c index e6a7c01f2ae3..6f447ccf8130 100644 --- a/quantum/process_keycode/process_auto_shift.c +++ b/quantum/process_keycode/process_auto_shift.c @@ -123,7 +123,12 @@ bool get_autoshift_shift_state(uint16_t keycode) { /** \brief Restores the shift key if it was cancelled by Auto Shift */ static void autoshift_flush_shift(void) { autoshift_flags.holding_shift = false; - del_weak_mods(MOD_BIT(KC_LSFT)); +# ifdef CAPS_WORD_ENABLE + if (!is_caps_word_on()) +# endif // CAPS_WORD_ENABLE + { + del_weak_mods(MOD_BIT(KC_LSFT)); + } if (autoshift_flags.cancelling_lshift) { autoshift_flags.cancelling_lshift = false; add_mods(MOD_BIT(KC_LSFT)); diff --git a/quantum/process_keycode/process_caps_word.c b/quantum/process_keycode/process_caps_word.c index ffd509a9142e..bc4369846cb8 100644 --- a/quantum/process_keycode/process_caps_word.c +++ b/quantum/process_keycode/process_caps_word.c @@ -131,7 +131,11 @@ bool process_caps_word(uint16_t keycode, keyrecord_t* record) { #endif // SWAP_HANDS_ENABLE } +#ifdef AUTO_SHIFT_ENABLE + del_weak_mods(get_autoshift_state() ? ~MOD_BIT(KC_LSFT) : 0xff); +#else clear_weak_mods(); +#endif // AUTO_SHIFT_ENABLE if (caps_word_press_user(keycode)) { send_keyboard_report(); return true; diff --git a/tests/caps_word/caps_word_autoshift/test_caps_word_autoshift.cpp b/tests/caps_word/caps_word_autoshift/test_caps_word_autoshift.cpp index deb4d95766e0..ba21c527a67d 100644 --- a/tests/caps_word/caps_word_autoshift/test_caps_word_autoshift.cpp +++ b/tests/caps_word/caps_word_autoshift/test_caps_word_autoshift.cpp @@ -19,6 +19,14 @@ #include "test_fixture.hpp" #include "test_keymap_key.hpp" +// Allow reports with no keys or only KC_LSFT. +// clang-format off +#define EXPECT_EMPTY_OR_LSFT(driver) \ + EXPECT_CALL(driver, send_keyboard_mock(AnyOf( \ + KeyboardReport(), \ + KeyboardReport(KC_LSFT)))) +// clang-format on + using ::testing::_; using ::testing::AnyNumber; using ::testing::AnyOf; @@ -39,13 +47,7 @@ TEST_F(CapsWord, AutoShiftKeys) { KeymapKey key_spc(0, 1, 0, KC_SPC); set_keymap({key_a, key_spc}); - // Allow any number of reports with no keys or only KC_LSFT. - // clang-format off - EXPECT_CALL(driver, send_keyboard_mock(AnyOf( - KeyboardReport(), - KeyboardReport(KC_LSFT)))) - .Times(AnyNumber()); - // clang-format on + EXPECT_EMPTY_OR_LSFT(driver).Times(AnyNumber()); { // Expect: "A, A, space, a". InSequence s; EXPECT_REPORT(driver, (KC_LSFT, KC_A)); @@ -65,6 +67,46 @@ TEST_F(CapsWord, AutoShiftKeys) { testing::Mock::VerifyAndClearExpectations(&driver); } +// Test Caps Word + Auto Shift where keys A and B are rolled. +TEST_F(CapsWord, AutoShiftRolledShiftedKeys) { + TestDriver driver; + KeymapKey key_a(0, 0, 0, KC_A); + KeymapKey key_b(0, 0, 1, KC_B); + set_keymap({key_a, key_b}); + + EXPECT_EMPTY_OR_LSFT(driver).Times(AnyNumber()); + { // Expect: "A, B, A, B". + InSequence s; + EXPECT_REPORT(driver, (KC_LSFT, KC_A)); + EXPECT_REPORT(driver, (KC_LSFT, KC_B)); + EXPECT_REPORT(driver, (KC_LSFT, KC_A)); + EXPECT_REPORT(driver, (KC_LSFT, KC_B)); + } + + caps_word_on(); + + key_a.press(); // Overlapping taps: A down, B down, A up, B up. + run_one_scan_loop(); + key_b.press(); + run_one_scan_loop(); + key_a.release(); + run_one_scan_loop(); + key_b.release(); + run_one_scan_loop(); + + key_a.press(); // Nested taps: A down, B down, B up, A up. + run_one_scan_loop(); + key_b.press(); + run_one_scan_loop(); + key_b.release(); + run_one_scan_loop(); + key_a.release(); + run_one_scan_loop(); + + caps_word_off(); + testing::Mock::VerifyAndClearExpectations(&driver); +} + // Tests that with tap-hold keys with Retro Shift, letter keys are shifted by // Caps Word regardless of whether they are retroshifted. TEST_F(CapsWord, RetroShiftKeys) { @@ -73,13 +115,7 @@ TEST_F(CapsWord, RetroShiftKeys) { KeymapKey key_layertap_b(0, 1, 0, LT(1, KC_B)); set_keymap({key_modtap_a, key_layertap_b}); - // Allow any number of reports with no keys or only KC_LSFT. - // clang-format off - EXPECT_CALL(driver, send_keyboard_mock(AnyOf( - KeyboardReport(), - KeyboardReport(KC_LSFT)))) - .Times(AnyNumber()); - // clang-format on + EXPECT_EMPTY_OR_LSFT(driver).Times(AnyNumber()); { // Expect: "B, A, B, A". InSequence s; EXPECT_REPORT(driver, (KC_LSFT, KC_B)); diff --git a/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp b/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp index df822afee3ff..3a0530b8543b 100644 --- a/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp +++ b/tests/caps_word/caps_word_combo/test_caps_word_combo.cpp @@ -25,6 +25,14 @@ #include "test_fixture.hpp" #include "test_keymap_key.hpp" +// Allow reports with no keys or only KC_LSFT. +// clang-format off +#define EXPECT_EMPTY_OR_LSFT(driver) \ + EXPECT_CALL(driver, send_keyboard_mock(AnyOf( \ + KeyboardReport(), \ + KeyboardReport(KC_LSFT)))) +// clang-format on + using ::testing::AnyNumber; using ::testing::AnyOf; using ::testing::InSequence; @@ -42,9 +50,15 @@ const uint16_t ad_combo[] PROGMEM = {KC_A, LCTL_T(KC_D), COMBO_END}; const uint16_t de_combo[] PROGMEM = {LCTL_T(KC_D), LT(1, KC_E), COMBO_END}; const uint16_t fghi_combo[] PROGMEM = {KC_F, KC_G, KC_H, KC_I, COMBO_END}; +// clang-format off combo_t key_combos[] = { - [AB_COMBO] = COMBO(ab_combo, KC_SPC), [BC_COMBO] = COMBO(bc_combo, KC_X), [AD_COMBO] = COMBO(ad_combo, KC_Y), [DE_COMBO] = COMBO(de_combo, KC_Z), [FGHI_COMBO] = COMBO(fghi_combo, KC_W), + [AB_COMBO] = COMBO(ab_combo, KC_SPC), // KC_A + KC_B = KC_SPC + [BC_COMBO] = COMBO(bc_combo, KC_X), // KC_B + KC_C = KC_X + [AD_COMBO] = COMBO(ad_combo, KC_Y), // KC_A + LCTL_T(KC_D) = KC_Y + [DE_COMBO] = COMBO(de_combo, KC_Z), // LCTL_T(KC_D) + LT(1, KC_E) = KC_Z + [FGHI_COMBO] = COMBO(fghi_combo, KC_W) // KC_F + KC_G + KC_H + KC_I = KC_W }; +// clang-format on } // extern "C" namespace { @@ -79,8 +93,7 @@ TEST_P(CapsWord, SingleCombo) { KeymapKey key_c(0, 0, 2, KC_C); set_keymap({key_b, key_c}); - // Allow any number of reports with no keys or only KC_LSFT. - EXPECT_CALL(driver, send_keyboard_mock(AnyOf(KeyboardReport(), KeyboardReport(KC_LSFT)))).Times(AnyNumber()); + EXPECT_EMPTY_OR_LSFT(driver).Times(AnyNumber()); EXPECT_REPORT(driver, (KC_LSFT, KC_X)); caps_word_on(); @@ -101,8 +114,7 @@ TEST_P(CapsWord, LongerCombo) { KeymapKey key_i(0, 0, 3, KC_I); set_keymap({key_f, key_g, key_h, key_i}); - // Allow any number of reports with no keys or only KC_LSFT. - EXPECT_CALL(driver, send_keyboard_mock(AnyOf(KeyboardReport(), KeyboardReport(KC_LSFT)))).Times(AnyNumber()); + EXPECT_EMPTY_OR_LSFT(driver).Times(AnyNumber()); EXPECT_REPORT(driver, (KC_LSFT, KC_W)); caps_word_on(); @@ -125,13 +137,7 @@ TEST_P(CapsWord, ComboRegularKeys) { KeymapKey key_1(0, 0, 3, KC_1); set_keymap({key_a, key_b, key_c, key_1}); - // Allow any number of reports with no keys or only KC_LSFT. - // clang-format off - EXPECT_CALL(driver, send_keyboard_mock(AnyOf( - KeyboardReport(), - KeyboardReport(KC_LSFT)))) - .Times(AnyNumber()); - // clang-format on + EXPECT_EMPTY_OR_LSFT(driver).Times(AnyNumber()); { // Expect: "A, B, 1, X, 1, C, space, a". InSequence s; EXPECT_REPORT(driver, (KC_LSFT, KC_A)); @@ -168,13 +174,7 @@ TEST_P(CapsWord, ComboModTapKey) { KeymapKey key_layertap_e(0, 0, 2, LT(1, KC_E)); set_keymap({key_a, key_modtap_d, key_layertap_e}); - // Allow any number of reports with no keys or only KC_LSFT. - // clang-format off - EXPECT_CALL(driver, send_keyboard_mock(AnyOf( - KeyboardReport(), - KeyboardReport(KC_LSFT)))) - .Times(AnyNumber()); - // clang-format on + EXPECT_EMPTY_OR_LSFT(driver).Times(AnyNumber()); { // Expect: "A, D, E, Y, Z". InSequence s; EXPECT_REPORT(driver, (KC_LSFT, KC_A)); From e1c290d487807d23f68906ca1671e56f4d1f165a Mon Sep 17 00:00:00 2001 From: Pascal Getreuer Date: Wed, 13 Jul 2022 09:57:35 -0700 Subject: [PATCH 8/8] Fix formatting --- quantum/process_keycode/process_combo.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/quantum/process_keycode/process_combo.c b/quantum/process_keycode/process_combo.c index 3066167f2bce..e5135e5a645f 100644 --- a/quantum/process_keycode/process_combo.c +++ b/quantum/process_keycode/process_combo.c @@ -211,7 +211,7 @@ static inline void dump_key_buffer(void) { key_buffer_next = key_buffer_i + 1; queued_record_t *qrecord = &key_buffer[key_buffer_i]; - keyrecord_t *record = &qrecord->record; + keyrecord_t * record = &qrecord->record; if (IS_NOEVENT(record->event)) { continue; @@ -311,7 +311,7 @@ void apply_combo(uint16_t combo_index, combo_t *combo) { for (uint8_t key_buffer_i = 0; key_buffer_i < key_buffer_size; key_buffer_i++) { queued_record_t *qrecord = &key_buffer[key_buffer_i]; - keyrecord_t *record = &qrecord->record; + keyrecord_t * record = &qrecord->record; uint16_t keycode = qrecord->keycode; uint8_t key_count = 0; @@ -346,7 +346,7 @@ static inline void apply_combos(void) { // Apply all buffered normal combos. for (uint8_t i = combo_buffer_read; i != combo_buffer_write; INCREMENT_MOD(i)) { queued_combo_t *buffered_combo = &combo_buffer[i]; - combo_t *combo = &key_combos[buffered_combo->combo_index]; + combo_t * combo = &key_combos[buffered_combo->combo_index]; #ifdef COMBO_MUST_TAP_PER_COMBO if (get_combo_must_tap(buffered_combo->combo_index, combo)) { @@ -451,7 +451,7 @@ static bool process_single_combo(combo_t *combo, uint16_t keycode, keyrecord_t * combo_t *drop = NULL; for (uint8_t combo_buffer_i = combo_buffer_read; combo_buffer_i != combo_buffer_write; INCREMENT_MOD(combo_buffer_i)) { queued_combo_t *qcombo = &combo_buffer[combo_buffer_i]; - combo_t *buffered_combo = &key_combos[qcombo->combo_index]; + combo_t * buffered_combo = &key_combos[qcombo->combo_index]; if ((drop = overlaps(buffered_combo, combo))) { DISABLE_COMBO(drop);