diff options
| -rw-r--r-- | common/keyboard_scan.c | 49 | ||||
| -rw-r--r-- | include/config.h | 15 | ||||
| -rw-r--r-- | test/build.mk | 2 | ||||
| -rw-r--r-- | test/kb_scan.c | 158 | ||||
| l--------- | test/kb_scan_strict.tasklist | 1 | ||||
| -rw-r--r-- | test/test_config.h | 5 |
6 files changed, 217 insertions, 13 deletions
diff --git a/common/keyboard_scan.c b/common/keyboard_scan.c index 3e7e1c1d87..a1e17bc61d 100644 --- a/common/keyboard_scan.c +++ b/common/keyboard_scan.c @@ -496,6 +496,16 @@ static int has_ghosting(const uint8_t *state) return 0; } +/* Inform keyboard module if scanning is enabled */ +static void key_state_changed(int row, int col, uint8_t state) +{ + if (!keyboard_scan_is_enabled()) + return; + + /* No-op for protocols that require full keyboard matrix (e.g. MKBP). */ + keyboard_state_changed(row, col, !!(state & BIT(row))); +} + /** * Update keyboard state using low-level interface to read keyboard. * @@ -525,7 +535,7 @@ static int check_keys_changed(uint8_t *state) /* Check for changes between previous scan and this one */ for (c = 0; c < keyboard_cols; c++) { - int diff; + int diff = new_state[c] ^ state[c]; /* Clear debouncing flag, if sufficient time has elapsed. */ for (i = 0; i < KEYBOARD_ROWS && debouncing[c]; i++) { @@ -536,6 +546,20 @@ static int check_keys_changed(uint8_t *state) keyscan_config.debounce_up_us)) continue; /* Not done debouncing */ debouncing[c] &= ~BIT(i); + + if (!IS_ENABLED(CONFIG_KEYBOARD_STRICT_DEBOUNCE)) + continue; + if (!(diff & BIT(i))) + /* Debounced but no difference. */ + continue; + any_change = 1; + key_state_changed(i, c, new_state[c]); + /* + * This makes state[c] == new_state[c] for row i. + * Thus, when diff is calculated below, it won't + * be asserted (for row i). + */ + state[c] ^= diff & BIT(i); } /* Recognize change in state, unless debounce in effect. */ @@ -546,15 +570,10 @@ static int check_keys_changed(uint8_t *state) if (!(diff & BIT(i))) continue; scan_edge_index[c][i] = scan_time_index; - any_change = 1; - /* Inform keyboard module if scanning is enabled */ - if (keyboard_scan_is_enabled()) { - /* This is no-op for protocols that require a - * full keyboard matrix (e.g., MKBP). - */ - keyboard_state_changed( - i, c, !!(new_state[c] & BIT(i))); + if (!IS_ENABLED(CONFIG_KEYBOARD_STRICT_DEBOUNCE)) { + any_change = 1; + key_state_changed(i, c, new_state[c]); } } @@ -565,7 +584,8 @@ static int check_keys_changed(uint8_t *state) * (up or down), the state bits are only updated if the * edge was not suppressed due to debouncing. */ - state[c] ^= diff; + if (!IS_ENABLED(CONFIG_KEYBOARD_STRICT_DEBOUNCE)) + state[c] ^= diff; } if (any_change) { @@ -723,6 +743,15 @@ const uint8_t *keyboard_scan_get_state(void) void keyboard_scan_init(void) { + if (IS_ENABLED(CONFIG_KEYBOARD_STRICT_DEBOUNCE) && + keyscan_config.debounce_down_us != keyscan_config.debounce_up_us) { + /* + * Strict debouncer is prone to keypress reordering if debounce + * durations for down and up are not equal. crbug.com/547131 + */ + CPRINTS("KB WARN: Debounce durations not equal"); + } + /* Configure refresh key matrix */ keyboard_mask_refresh = KEYBOARD_ROW_TO_MASK( board_keyboard_row_refresh()); diff --git a/include/config.h b/include/config.h index d16a4a0388..11733d38df 100644 --- a/include/config.h +++ b/include/config.h @@ -2801,6 +2801,21 @@ #undef CONFIG_KEYBOARD_KEYPAD /* + * Enable strict debouncer. A strict debouncer waits until debounce is done + * before registering key up/down while a non-strict debouncer registers a key + * up/down as soon as a key is pressed or released. + * + * A strict debouncer is robust against unintentional key presses, caused by a + * device drop, for example. However, its latency isn't as fast as a non-strict + * debouncer. + * + * If a strict debouncer is used, it's recommended to set debounce_down_us and + * debounce_up_us to an equal value. This guarantees key events are registered + * in the order the keys are pressed. + */ +#undef CONFIG_KEYBOARD_STRICT_DEBOUNCE + +/* * Enable the 8042 AUX port. This is typically used for PS/2 mouse devices. * You will need to implement send_aux_data_to_device and lpc_aux_put_char. */ diff --git a/test/build.mk b/test/build.mk index 30b7c1ce55..6b156ccaf0 100644 --- a/test/build.mk +++ b/test/build.mk @@ -49,6 +49,7 @@ test-list-host += kasa test-list-host += kb_8042 test-list-host += kb_mkbp test-list-host += kb_scan +test-list-host += kb_scan_strict test-list-host += lid_sw test-list-host += lightbar test-list-host += mag_cal @@ -159,6 +160,7 @@ is_enabled-y=is_enabled.o kb_8042-y=kb_8042.o kb_mkbp-y=kb_mkbp.o kb_scan-y=kb_scan.o +kb_scan_strict-y=kb_scan.o lid_sw-y=lid_sw.o lightbar-y=lightbar.o mag_cal-y=mag_cal.o diff --git a/test/kb_scan.c b/test/kb_scan.c index 8ca81cad18..b3b42813e4 100644 --- a/test/kb_scan.c +++ b/test/kb_scan.c @@ -31,7 +31,16 @@ old = fifo_add_count; \ } while (0) +/* Emulated physical key state */ static uint8_t mock_state[KEYBOARD_COLS_MAX]; + +/* Snapshot of last known key state */ +static uint8_t key_state[KEYBOARD_COLS_MAX]; + +/* Counters for key state changes (UP/DOWN) */ +static int key_state_change[KEYBOARD_COLS_MAX][KEYBOARD_ROWS]; +static int total_key_state_change; + static int column_driven; static int fifo_add_count; static int lid_open; @@ -79,7 +88,24 @@ int keyboard_raw_read_rows(void) int mkbp_keyboard_add(const uint8_t *buffp) { + int c, r; + fifo_add_count++; + + for (c = 0; c < KEYBOARD_COLS_MAX; c++) { + uint8_t diff = key_state[c] ^ buffp[c]; + + for (r = 0; r < KEYBOARD_ROWS; r++) { + if (diff & BIT(r)) { + key_state_change[c][r]++; + total_key_state_change++; + } + } + } + + /* Save a snapshot. */ + memcpy(key_state, buffp, sizeof(key_state)); + return EC_SUCCESS; } @@ -105,13 +131,23 @@ void chipset_reset(void) static void mock_key(int r, int c, int keydown) { - ccprintf("%s (%d, %d)\n", keydown ? "Pressing" : "Releasing", r, c); + ccprintf(" %s (%d, %d)\n", keydown ? "Pressing" : "Releasing", r, c); if (keydown) mock_state[c] |= (1 << r); else mock_state[c] &= ~(1 << r); } +static void reset_key_state(void) +{ + memset(mock_state, 0, sizeof(mock_state)); + memset(key_state, 0, sizeof(key_state)); + memset(key_state_change, 0, sizeof(key_state_change)); + task_wake(TASK_ID_KEYSCAN); + msleep(NO_KEYDOWN_DELAY_MS); + total_key_state_change = 0; +} + static int expect_keychange(void) { int old_count = fifo_add_count; @@ -164,6 +200,8 @@ static int verify_key_presses(int old, int expected) static int deghost_test(void) { + reset_key_state(); + /* Test we can detect a keypress */ mock_key(1, 1, 1); TEST_ASSERT(expect_keychange() == EC_SUCCESS); @@ -205,11 +243,118 @@ static int deghost_test(void) return EC_SUCCESS; } +static int strict_debounce_test(void) +{ + reset_key_state(); + + ccprintf("Test key press & hold.\n"); + mock_key(1, 1, 1); + TEST_EQ(expect_keychange(), EC_SUCCESS, "%d"); + TEST_EQ(key_state_change[1][1], 1, "%d"); + TEST_EQ(total_key_state_change, 1, "%d"); + ccprintf("Pass.\n"); + + reset_key_state(); + + ccprintf("Test a short stroke.\n"); + mock_key(1, 1, 1); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + mock_key(1, 1, 0); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + TEST_EQ(expect_no_keychange(), EC_SUCCESS, "%d"); + TEST_EQ(key_state_change[1][1], 0, "%d"); + ccprintf("Pass.\n"); + + reset_key_state(); + + ccprintf("Test ripples being suppressed.\n"); + /* DOWN */ + mock_key(1, 1, 1); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + mock_key(1, 1, 0); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + mock_key(1, 1, 1); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + TEST_EQ(expect_keychange(), EC_SUCCESS, "%d"); + TEST_EQ(key_state_change[1][1], 1, "%d"); + TEST_EQ(total_key_state_change, 1, "%d"); + /* UP */ + mock_key(1, 1, 0); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + mock_key(1, 1, 1); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + mock_key(1, 1, 0); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + TEST_EQ(expect_keychange(), EC_SUCCESS, "%d"); + TEST_EQ(key_state_change[1][1], 2, "%d"); + TEST_EQ(total_key_state_change, 2, "%d"); + ccprintf("Pass.\n"); + + reset_key_state(); + + ccprintf("Test simultaneous strokes.\n"); + mock_key(1, 1, 1); + mock_key(2, 1, 1); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + TEST_EQ(expect_keychange(), EC_SUCCESS, "%d"); + TEST_EQ(key_state_change[1][1], 1, "%d"); + TEST_EQ(key_state_change[1][2], 1, "%d"); + TEST_EQ(total_key_state_change, 2, "%d"); + ccprintf("Pass.\n"); + + reset_key_state(); + + ccprintf("Test simultaneous strokes in two columns.\n"); + mock_key(1, 1, 1); + mock_key(1, 2, 1); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + TEST_EQ(expect_keychange(), EC_SUCCESS, "%d"); + TEST_EQ(key_state_change[1][1], 1, "%d"); + TEST_EQ(key_state_change[2][1], 1, "%d"); + TEST_EQ(total_key_state_change, 2, "%d"); + ccprintf("Pass.\n"); + + reset_key_state(); + + ccprintf("Test normal & short simultaneous strokes.\n"); + mock_key(1, 1, 1); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + mock_key(2, 1, 1); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + mock_key(1, 1, 0); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + TEST_EQ(expect_keychange(), EC_SUCCESS, "%d"); + TEST_EQ(key_state_change[1][1], 0, "%d"); + TEST_EQ(key_state_change[1][2], 1, "%d"); + TEST_EQ(total_key_state_change, 1, "%d"); + ccprintf("Pass.\n"); + + reset_key_state(); + + ccprintf("Test normal & short simultaneous strokes in two columns.\n"); + reset_key_state(); + mock_key(1, 1, 1); + task_wake(TASK_ID_KEYSCAN); + mock_key(1, 2, 1); + task_wake(TASK_ID_KEYSCAN); + mock_key(1, 1, 0); + task_wake(TASK_ID_KEYSCAN); + TEST_EQ(expect_keychange(), EC_SUCCESS, "%d"); + TEST_EQ(key_state_change[1][1], 0, "%d"); + TEST_EQ(key_state_change[2][1], 1, "%d"); + TEST_EQ(total_key_state_change, 1, "%d"); + ccprintf("Pass.\n"); + + return EC_SUCCESS; +} + static int debounce_test(void) { int old_count = fifo_add_count; int i; + reset_key_state(); + /* One brief keypress is detected. */ msleep(40); mock_key(1, 1, 1); @@ -293,6 +438,8 @@ static int simulate_key_test(void) { int old_count; + reset_key_state(); + task_wake(TASK_ID_KEYSCAN); msleep(40); /* Wait for debouncing to settle */ @@ -332,6 +479,8 @@ static int verify_variable_not_set(int *var) static int runtime_key_test(void) { + reset_key_state(); + /* Alt-VolUp-H triggers system hibernation */ mock_defined_key(LEFT_ALT, 1); mock_default_key(VOL_UP, 1); @@ -372,6 +521,8 @@ static int runtime_key_test(void) #ifdef CONFIG_LID_SWITCH static int lid_test(void) { + reset_key_state(); + msleep(40); /* Allow debounce to settle */ lid_open = 0; @@ -429,7 +580,10 @@ static void run_test_step1(void) RUN_TEST(deghost_test); - RUN_TEST(debounce_test); + if (IS_ENABLED(CONFIG_KEYBOARD_STRICT_DEBOUNCE)) + RUN_TEST(strict_debounce_test); + else + RUN_TEST(debounce_test); if (0) /* crbug.com/976974 */ RUN_TEST(simulate_key_test); diff --git a/test/kb_scan_strict.tasklist b/test/kb_scan_strict.tasklist new file mode 120000 index 0000000000..d4d4fa1efb --- /dev/null +++ b/test/kb_scan_strict.tasklist @@ -0,0 +1 @@ +kb_scan.tasklist
\ No newline at end of file diff --git a/test/test_config.h b/test/test_config.h index 9e74a8646b..8b98dc1087 100644 --- a/test/test_config.h +++ b/test/test_config.h @@ -59,10 +59,13 @@ #define CONFIG_MKBP_USE_GPIO #endif -#ifdef TEST_KB_SCAN +#if defined(TEST_KB_SCAN) || defined(TEST_KB_SCAN_STRICT) #define CONFIG_KEYBOARD_PROTOCOL_MKBP #define CONFIG_MKBP_EVENT #define CONFIG_MKBP_USE_GPIO +#ifdef TEST_KB_SCAN_STRICT +#define CONFIG_KEYBOARD_STRICT_DEBOUNCE +#endif #endif #ifdef TEST_MATH_UTIL |