From 309359b32f2ce46adc2b720b1cca20d9ac333d6c Mon Sep 17 00:00:00 2001 From: Purdea Andrei Date: Mon, 10 Apr 2023 10:27:19 +0300 Subject: [PATCH] fix(keymaps): fix keypresses that are not in the transform Before this change, if a matrix position was not present in the transform, various incorrect behaviors would happen: 1) In some cases out-of-bounds accesses: Note that the size of the`transform[]` array does not necessarily match the size of the matrix. So for example if key position (ZMK_MATRIX_COLS-1, ZMK_MATRIX_ROWS-1) is not present in the transform, but ends up being pressed, then the array will be accessed beyond its size, and any data could be returned. 2) In other cases the 0th position in the keymap will be used because the `transform[]` array is initialized to all zeros. --- app/include/zmk/matrix_transform.h | 2 +- app/src/kscan.c | 9 ++++++- app/src/matrix_transform.c | 43 +++++++++++++++++++++++------- 3 files changed, 43 insertions(+), 11 deletions(-) diff --git a/app/include/zmk/matrix_transform.h b/app/include/zmk/matrix_transform.h index 413678a7e..ffd3e3f1d 100644 --- a/app/include/zmk/matrix_transform.h +++ b/app/include/zmk/matrix_transform.h @@ -6,4 +6,4 @@ #pragma once -uint32_t zmk_matrix_transform_row_column_to_position(uint32_t row, uint32_t column); \ No newline at end of file +int32_t zmk_matrix_transform_row_column_to_position(uint32_t row, uint32_t column); \ No newline at end of file diff --git a/app/src/kscan.c b/app/src/kscan.c index b07133203..62d0cf075 100644 --- a/app/src/kscan.c +++ b/app/src/kscan.c @@ -47,7 +47,14 @@ void zmk_kscan_process_msgq(struct k_work *item) { while (k_msgq_get(&zmk_kscan_msgq, &ev, K_NO_WAIT) == 0) { bool pressed = (ev.state == ZMK_KSCAN_EVENT_STATE_PRESSED); - uint32_t position = zmk_matrix_transform_row_column_to_position(ev.row, ev.column); + int32_t position = zmk_matrix_transform_row_column_to_position(ev.row, ev.column); + + if (position < 0) { + LOG_WRN("Not found in transform: row: %d, col: %d, pressed: %s", ev.row, ev.column, + (pressed ? "true" : "false")); + continue; + } + LOG_DBG("Row: %d, col: %d, position: %d, pressed: %s", ev.row, ev.column, position, (pressed ? "true" : "false")); ZMK_EVENT_RAISE(new_zmk_position_state_changed( diff --git a/app/src/matrix_transform.c b/app/src/matrix_transform.c index 47256608f..6c616d5e8 100644 --- a/app/src/matrix_transform.c +++ b/app/src/matrix_transform.c @@ -11,17 +11,32 @@ #ifdef ZMK_KEYMAP_TRANSFORM_NODE -#define _TRANSFORM_ENTRY(i, _) \ - [(KT_ROW(DT_PROP_BY_IDX(ZMK_KEYMAP_TRANSFORM_NODE, map, i)) * ZMK_MATRIX_COLS) + \ - KT_COL(DT_PROP_BY_IDX(ZMK_KEYMAP_TRANSFORM_NODE, map, i))] = i +/* the transform in the device tree is a list of (row,column) pairs that is + * indexed by by the keymap position of that key. We want to invert this in + * order to be able to quickly determine what keymap position a particular + * row,column pair is associated with, using a single lookup. + * + * We do this by creating the `transform` array at compile time, which is + * indexed by (row * ZMK_MATRIX_COLS) + column, and the value contains an + * encoded keymap index it is associated with. The keymap index is encoded + * by adding INDEX_OFFSET to it, because not all row,column pairs have an + * associated keymap index (some matrices are sparse), C globals are + * initialized to 0, and the keymap index of 0 is a valid index. We want to + * be able to detect the condition when an unassigned matrix position is + * pressed and we want to return an error. + */ -static uint32_t transform[] = {LISTIFY(ZMK_KEYMAP_LEN, _TRANSFORM_ENTRY, (, ), 0)}; +#define INDEX_OFFSET 1 + +#define TRANSFORM_ENTRY(i, _) \ + [(KT_ROW(DT_PROP_BY_IDX(ZMK_KEYMAP_TRANSFORM_NODE, map, i)) * ZMK_MATRIX_COLS) + \ + KT_COL(DT_PROP_BY_IDX(ZMK_KEYMAP_TRANSFORM_NODE, map, i))] = i + INDEX_OFFSET + +static uint32_t transform[] = {LISTIFY(ZMK_KEYMAP_LEN, TRANSFORM_ENTRY, (, ), 0)}; #endif -uint32_t zmk_matrix_transform_row_column_to_position(uint32_t row, uint32_t column) { - uint32_t matrix_index; - +int32_t zmk_matrix_transform_row_column_to_position(uint32_t row, uint32_t column) { #if DT_NODE_HAS_PROP(ZMK_KEYMAP_TRANSFORM_NODE, col_offset) column += DT_PROP(ZMK_KEYMAP_TRANSFORM_NODE, col_offset); #endif @@ -30,10 +45,20 @@ uint32_t zmk_matrix_transform_row_column_to_position(uint32_t row, uint32_t colu row += DT_PROP(ZMK_KEYMAP_TRANSFORM_NODE, row_offset); #endif - matrix_index = (row * ZMK_MATRIX_COLS) + column; + const uint32_t matrix_index = (row * ZMK_MATRIX_COLS) + column; #ifdef ZMK_KEYMAP_TRANSFORM_NODE - return transform[matrix_index]; + if (matrix_index >= ARRAY_SIZE(transform)) { + return -EINVAL; + } + + const uint32_t value = transform[matrix_index]; + + if (!value) { + return -EINVAL; + } + + return value - INDEX_OFFSET; #else return matrix_index; #endif /* ZMK_KEYMAP_TRANSFORM_NODE */