diff options
author | Jes Klinke <jbk@google.com> | 2019-06-06 15:11:14 -0700 |
---|---|---|
committer | Commit Bot <commit-bot@chromium.org> | 2019-06-11 20:09:29 +0000 |
commit | b20056a70b8ad1f0f07dad0790aeab6dc7aa24b9 (patch) | |
tree | 831c49e8528564560c2178db3781590b02c15b9b | |
parent | b662af8066a01502429b84879c4e3a885ec10e1b (diff) | |
download | chrome-ec-b20056a70b8ad1f0f07dad0790aeab6dc7aa24b9.tar.gz |
keyboard_scan: Send kb events at start of debouncing period.
Current behavior is to start a timer upon first detecting a transition of
any one cell of the keyboard matrix, and then generate a keyboard event
(press or release) only after that timer has expired. Due to the fact that
the release timer has a longer period than the press timer, in some cases,
e.g. releasing the shift key right before depressing another key, events
could end up getting re-ordered.
BUG=chromium:547131
BRANCH=master
TEST=make run-kb_scan
Signed-off-by: Jes Klinke <jbk@google.com>
Change-Id: If3de2e629dc9df4325d8c17590d6624a41e27187
Bug: 547131
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1579905
Tested-by: Jes Klinke <jbk@chromium.org>
Reviewed-by: Jett Rink <jettrink@chromium.org>
Commit-Queue: Jes Klinke <jbk@chromium.org>
-rw-r--r-- | common/keyboard_scan.c | 67 | ||||
-rw-r--r-- | test/kb_scan.c | 114 |
2 files changed, 105 insertions, 76 deletions
diff --git a/common/keyboard_scan.c b/common/keyboard_scan.c index a8f843e0a2..1c0f9759d1 100644 --- a/common/keyboard_scan.c +++ b/common/keyboard_scan.c @@ -86,8 +86,6 @@ uint8_t keyboard_cols = KEYBOARD_COLS_MAX; /* Debounced key matrix */ static uint8_t __bss_slow debounced_state[KEYBOARD_COLS_MAX]; -/* Matrix from previous scan */ -static uint8_t __bss_slow prev_state[KEYBOARD_COLS_MAX]; /* Mask of keys being debounced */ static uint8_t __bss_slow debouncing[KEYBOARD_COLS_MAX]; /* Keys simulated-pressed */ @@ -475,46 +473,27 @@ 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 = new_state[c] ^ prev_state[c]; + int diff; - if (!diff) - continue; - - for (i = 0; i < KEYBOARD_ROWS; i++) { - if (diff & BIT(i)) - scan_edge_index[c][i] = scan_time_index; - } - - debouncing[c] |= diff; - prev_state[c] = new_state[c]; - } - - /* Check for keys which are done debouncing */ - for (c = 0; c < keyboard_cols; c++) { - int debc = debouncing[c]; - - if (!debc) - continue; - - for (i = 0; i < KEYBOARD_ROWS; i++) { - int mask = 1 << i; - int new_mask = new_state[c] & mask; - - /* Are we done debouncing this key? */ - if (!(debc & mask)) - continue; /* Not debouncing this key */ + /* Clear debouncing flag, if sufficient time has elapsed. */ + for (i = 0; i < KEYBOARD_ROWS && debouncing[c]; i++) { + if (!(debouncing[c] & BIT(i))) + continue; if (tnow - scan_time[scan_edge_index[c][i]] < - (new_mask ? keyscan_config.debounce_down_us : + (state[c] ? keyscan_config.debounce_down_us : keyscan_config.debounce_up_us)) continue; /* Not done debouncing */ + debouncing[c] &= ~BIT(i); + } - debouncing[c] &= ~mask; - - /* Did the key change from its previous state? */ - if ((state[c] & mask) == new_mask) - continue; /* No */ - - state[c] ^= mask; + /* Recognize change in state, unless debounce in effect. */ + diff = (new_state[c] ^ state[c]) & ~debouncing[c]; + if (!diff) + continue; + for (i = 0; i < KEYBOARD_ROWS; i++) { + if (!(diff & BIT(i))) + continue; + scan_edge_index[c][i] = scan_time_index; any_change = 1; /* Inform keyboard module if scanning is enabled */ @@ -522,9 +501,19 @@ static int check_keys_changed(uint8_t *state) /* This is no-op for protocols that require a * full keyboard matrix (e.g., MKBP). */ - keyboard_state_changed(i, c, new_mask ? 1 : 0); + keyboard_state_changed( + i, c, !!(new_state[c] & BIT(i))); } } + + /* For any keyboard events just sent, turn on debouncing. */ + debouncing[c] |= diff; + /* + * Note: In order to "remember" what was last reported + * (up or down), the state bits are only updated if the + * edge was not suppressed due to debouncing. + */ + state[c] ^= diff; } if (any_change) { @@ -681,7 +670,6 @@ void keyboard_scan_init(void) /* Initialize raw state */ read_matrix(debounced_state); - memcpy(prev_state, debounced_state, sizeof(prev_state)); #ifdef CONFIG_KEYBOARD_LANGUAGE_ID /* Check keyboard ID state */ @@ -982,7 +970,6 @@ static int command_ksstate(int argc, char **argv) } print_state(debounced_state, "debounced "); - print_state(prev_state, "prev "); print_state(debouncing, "debouncing"); ccprintf("Keyboard scan disable mask: 0x%08x\n", diff --git a/test/kb_scan.c b/test/kb_scan.c index b364d19368..3e066c041f 100644 --- a/test/kb_scan.c +++ b/test/kb_scan.c @@ -40,6 +40,15 @@ static int hibernated; static int reset_called; #endif +/* + * Helper method to wake a given task, and provide immediate opportunity to run. + */ +static void task_wake_then_sleep_1ms(int task_id) +{ + task_wake(task_id); + msleep(1); +} + #ifdef CONFIG_LID_SWITCH int lid_is_open(void) { @@ -195,59 +204,83 @@ static int deghost_test(void) static int debounce_test(void) { int old_count = fifo_add_count; + int i; + + /* One brief keypress is detected. */ + msleep(40); mock_key(1, 1, 1); - task_wake(TASK_ID_KEYSCAN); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); mock_key(1, 1, 0); - task_wake(TASK_ID_KEYSCAN); - CHECK_KEY_COUNT(old_count, 0); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + CHECK_KEY_COUNT(old_count, 2); + /* Brief bounce, followed by continuous press is detected as one. */ + msleep(40); mock_key(1, 1, 1); - task_wake(TASK_ID_KEYSCAN); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); mock_key(1, 1, 0); - task_wake(TASK_ID_KEYSCAN); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); mock_key(1, 1, 1); - task_wake(TASK_ID_KEYSCAN); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); CHECK_KEY_COUNT(old_count, 1); + /* Brief lifting, then re-presseing is detected as new keypress. */ + msleep(40); mock_key(1, 1, 0); - task_wake(TASK_ID_KEYSCAN); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); mock_key(1, 1, 1); - task_wake(TASK_ID_KEYSCAN); - CHECK_KEY_COUNT(old_count, 0); - - mock_key(2, 2, 1); - task_wake(TASK_ID_KEYSCAN); - mock_key(2, 2, 0); - task_wake(TASK_ID_KEYSCAN); - CHECK_KEY_COUNT(old_count, 0); - - mock_key(2, 2, 1); - task_wake(TASK_ID_KEYSCAN); - mock_key(2, 2, 0); - task_wake(TASK_ID_KEYSCAN); - mock_key(2, 2, 1); - task_wake(TASK_ID_KEYSCAN); - CHECK_KEY_COUNT(old_count, 1); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + CHECK_KEY_COUNT(old_count, 2); + /* One bouncy re-contact while lifting is ignored. */ + msleep(40); mock_key(1, 1, 0); - task_wake(TASK_ID_KEYSCAN); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); mock_key(1, 1, 1); - task_wake(TASK_ID_KEYSCAN); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); mock_key(1, 1, 0); - task_wake(TASK_ID_KEYSCAN); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); CHECK_KEY_COUNT(old_count, 1); - mock_key(2, 2, 0); - task_wake(TASK_ID_KEYSCAN); - mock_key(2, 2, 1); - task_wake(TASK_ID_KEYSCAN); - mock_key(2, 2, 0); - task_wake(TASK_ID_KEYSCAN); - mock_key(2, 2, 1); - task_wake(TASK_ID_KEYSCAN); - mock_key(2, 2, 0); - task_wake(TASK_ID_KEYSCAN); + /* + * Debounce interval of first key is not affected by continued + * activity of other keys. + */ + msleep(40); + /* Push the first key */ + mock_key(0, 1, 0); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + /* + * Push down each subsequent key, until all 8 are pressed, each + * time bouncing the former one once. + */ + for (i = 1 ; i < 8; i++) { + mock_key(i, 1, 1); + task_wake(TASK_ID_KEYSCAN); + msleep(3); + mock_key(i - 1, 1, 0); + task_wake(TASK_ID_KEYSCAN); + msleep(1); + mock_key(i - 1, 1, 1); + task_wake(TASK_ID_KEYSCAN); + msleep(1); + } + /* Verify that the bounces were. ignored */ + CHECK_KEY_COUNT(old_count, 8); + /* + * Now briefly lift and re-press the first one, which should now be past + * its debounce interval + */ + mock_key(0, 1, 0); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); + CHECK_KEY_COUNT(old_count, 1); + mock_key(0, 1, 1); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); CHECK_KEY_COUNT(old_count, 1); + /* For good measure, release all keys before proceeding. */ + for (i = 0; i < 8; i++) + mock_key(i, 1, 0); + task_wake_then_sleep_1ms(TASK_ID_KEYSCAN); return EC_SUCCESS; } @@ -256,12 +289,17 @@ static int simulate_key_test(void) { int old_count; + task_wake(TASK_ID_KEYSCAN); + msleep(40); /* Wait for debouncing to settle */ + old_count = fifo_add_count; host_command_simulate(1, 1, 1); TEST_ASSERT(fifo_add_count > old_count); + msleep(40); old_count = fifo_add_count; host_command_simulate(1, 1, 0); TEST_ASSERT(fifo_add_count > old_count); + msleep(40); return EC_SUCCESS; } @@ -330,8 +368,11 @@ static int runtime_key_test(void) #ifdef CONFIG_LID_SWITCH static int lid_test(void) { + msleep(40); /* Allow debounce to settle */ + lid_open = 0; hook_notify(HOOK_LID_CHANGE); + msleep(1); /* Allow hooks to run */ mock_key(1, 1, 1); TEST_ASSERT(expect_no_keychange() == EC_SUCCESS); mock_key(1, 1, 0); @@ -339,6 +380,7 @@ static int lid_test(void) lid_open = 1; hook_notify(HOOK_LID_CHANGE); + msleep(1); /* Allow hooks to run */ mock_key(1, 1, 1); TEST_ASSERT(expect_keychange() == EC_SUCCESS); mock_key(1, 1, 0); |