_/qmk_firmware
1
0
Fork 0
Code Releases Activity

Fix pressing two keys with the same keycode but different modifiers (#2710)

Browse source 
* Fix extra keyboard report during test_fixture teardown

* Add tests for pressing two keys with only different modifers

* Fix #1708

When two keys that use the same keycode, but different modifiers were
pressed at the same time, the second keypress wasn't registered. This is
fixed by forcing a key release when we detect a new press for the same
keycode.

* Fix the NKRO version of is_key_pressed

* Fix uninitalized loop variable

Co-authored-by: Jack Humbert <jack.humb@gmail.com>
This commit is contained in:
fredizzimo 2020-03-13 20:09:38 +02:00 committed by GitHub
parent f89439ae09
commit 9e8767917d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 158 additions and 5 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
tests/basic/keymap.c
View file

@ -28,7 +28,7 @@ const uint16_t PROGMEM
{
// 0 1 2 3 4 5 6 7 8 9
{KC_A, KC_B, KC_NO, KC_LSFT, KC_RSFT, KC_LCTL, COMBO1, SFT_T(KC_P), M(0), KC_NO},
{KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO},
{KC_EQL, KC_PLUS, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO},
{KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO},
{KC_C, KC_D, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO, KC_NO},
},
@ -43,3 +43,4 @@ const macro_t *action_get_macro(keyrecord_t *record, uint8_t id, uint8_t opt) {
}
return MACRO_NONE;
};

116
tests/basic/test_keypress.cpp
View file

@ -18,6 +18,7 @@
using testing::_;
using testing::Return;
using testing::InSequence;
class KeyPress : public TestFixture {};
@ -122,3 +123,118 @@ TEST_F(KeyPress, RightShiftLeftControlAndCharWithTheSameKey) {
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
keyboard_task();
}
TEST_F(KeyPress, PressPlusEqualReleaseBeforePress) {
TestDriver driver;
InSequence s;
press_key(1, 1); // KC_PLUS
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
release_key(1, 1); // KC_PLUS
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
press_key(0, 1); // KC_EQL
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_EQL)));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
release_key(0, 1); // KC_EQL
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
}
TEST_F(KeyPress, PressPlusEqualDontReleaseBeforePress) {
TestDriver driver;
InSequence s;
press_key(1, 1); // KC_PLUS
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
press_key(0, 1); // KC_EQL
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_EQL)));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
release_key(1, 1); //KC_PLS
// BUG: Should really still return KC_EQL, but this is fine too
// It's also called twice for some reason
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(2);
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
release_key(0, 1); // KC_EQL
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
}
TEST_F(KeyPress, PressEqualPlusReleaseBeforePress) {
TestDriver driver;
InSequence s;
press_key(0, 1); // KC_EQL
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_EQL)));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
release_key(0, 1); // KQ_EQL
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
press_key(1, 1); // KC_PLUS
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
release_key(1, 1); // KC_PLUS
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
}
TEST_F(KeyPress, PressEqualPlusDontReleaseBeforePress) {
TestDriver driver;
InSequence s;
press_key(0, 1); // KC_EQL
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_EQL)));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
press_key(1, 1); // KC_PLUS
// BUG: The sequence is a bit strange, but it works, the end result is that
// KC_PLUS is sent
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT, KC_EQL)));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
release_key(0, 1); //KC_EQL
// I guess it's fine to still report shift here
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
release_key(1, 1); // KC_PLUS
// This report is not needed
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport(KC_LSFT)));
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport()));
run_one_scan_loop();
testing::Mock::VerifyAndClearExpectations(&driver);
}

7
tests/test_common/test_fixture.cpp
View file

@ -32,14 +32,15 @@ TestFixture::TestFixture() {}
TestFixture::~TestFixture() {
TestDriver driver;
layer_clear();
clear_all_keys();
// Run for a while to make sure all keys are completely released
EXPECT_CALL(driver, send_keyboard_mock(_)).Times(AnyNumber());
layer_clear();
clear_all_keys();
idle_for(TAPPING_TERM + 10);
testing::Mock::VerifyAndClearExpectations(&driver);
// Verify that the matrix really is cleared
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(Between(0, 1));
EXPECT_CALL(driver, send_keyboard_mock(KeyboardReport())).Times(0);
idle_for(TAPPING_TERM + 10);
}
void TestFixture::run_one_scan_loop() {

7
tmk_core/common/action.c
View file

@ -754,6 +754,13 @@ void register_code(uint8_t code) {
*/
#endif
{
// Force a new key press if the key is already pressed
// without this, keys with the same keycode, but different
// modifiers will be reported incorrectly, see issue #1708
if (is_key_pressed(keyboard_report, code)) {
del_key(code);
send_keyboard_report();
}
add_key(code);
send_keyboard_report();
}

26
tmk_core/common/report.c
View file

@ -68,6 +68,32 @@ uint8_t get_first_key(report_keyboard_t* keyboard_report) {
#endif
}
/** \brief Checks if a key is pressed in the report
*
* Returns true if the keyboard_report reports that the key is pressed, otherwise false
* Note: The function doesn't support modifers currently, and it returns false for KC_NO
*/
bool is_key_pressed(report_keyboard_t* keyboard_report, uint8_t key) {
if (key == KC_NO) {
return false;
}
#ifdef NKRO_ENABLE
if (keyboard_protocol && keymap_config.nkro) {
if ((key>>3) < KEYBOARD_REPORT_BITS) {
return keyboard_report->nkro.bits[key>>3] & 1<<(key&7);
} else {
return false;
}
}
#endif
for (int i=0; i < KEYBOARD_REPORT_KEYS; i++) {
if (keyboard_report->keys[i] == key) {
return true;
}
}
return false;
}
/** \brief add key byte
*
* FIXME: Needs doc

2
tmk_core/common/report.h
View file

@ -19,6 +19,7 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.
#define REPORT_H
#include <stdint.h>
#include <stdbool.h>
#include "keycode.h"
/* report id */
@ -236,6 +237,7 @@ static inline uint16_t KEYCODE2CONSUMER(uint8_t key) {
uint8_t has_anykey(report_keyboard_t* keyboard_report);
uint8_t get_first_key(report_keyboard_t* keyboard_report);
bool is_key_pressed(report_keyboard_t* keyboard_report, uint8_t key);
void add_key_byte(report_keyboard_t* keyboard_report, uint8_t code);
void del_key_byte(report_keyboard_t* keyboard_report, uint8_t code);