From 1e3e62c13d0666d98831ee302ae2fb17e68196c9 Mon Sep 17 00:00:00 2001 From: Pete Johanson Date: Fri, 28 Feb 2025 18:37:55 -0700 Subject: [PATCH] fix(pointing): Temp layer threading protection. (#2729) fix(pointing): Temp layer threading protection. Ensure all layer operations occur on the system work queue thread. Fixes: #2719 fix(pointing): Handle layer events to disable events Make the temp layer input processor propely handle layers getting deactivated externally before the temp layer timeout. Co-authored-by: Nicolas Munnich <98408764+Nick-Munnich@users.noreply.github.com> --- app/src/pointing/Kconfig | 5 + app/src/pointing/input_processor_temp_layer.c | 139 +++++++++++++++--- .../keycode_events.snapshot | 2 + .../keycode_events.snapshot | 2 + .../keycode_events.snapshot | 1 + .../keycode_events.snapshot | 2 + .../events.patterns | 6 + .../keycode_events.snapshot | 31 ++++ .../native_posix_64.conf | 6 + .../native_posix_64.keymap | 43 ++++++ 10 files changed, 219 insertions(+), 18 deletions(-) create mode 100644 app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/events.patterns create mode 100644 app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/keycode_events.snapshot create mode 100644 app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/native_posix_64.conf create mode 100644 app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/native_posix_64.keymap diff --git a/app/src/pointing/Kconfig b/app/src/pointing/Kconfig index b4051e1fc..43bc784fa 100644 --- a/app/src/pointing/Kconfig +++ b/app/src/pointing/Kconfig @@ -40,6 +40,11 @@ config ZMK_INPUT_PROCESSOR_TEMP_LAYER default y depends on DT_HAS_ZMK_INPUT_PROCESSOR_TEMP_LAYER_ENABLED +config ZMK_INPUT_PROCESSOR_TEMP_LAYER_MAX_ACTION_EVENTS + int "Temporary Layer Input Processor Max Action Events" + default 4 + depends on ZMK_INPUT_PROCESSOR_TEMP_LAYER + endif config ZMK_INPUT_PROCESSOR_TRANSFORM diff --git a/app/src/pointing/input_processor_temp_layer.c b/app/src/pointing/input_processor_temp_layer.c index b10652711..8a13a3ef2 100644 --- a/app/src/pointing/input_processor_temp_layer.c +++ b/app/src/pointing/input_processor_temp_layer.c @@ -14,6 +14,7 @@ #include #include #include +#include LOG_MODULE_DECLARE(zmk, CONFIG_ZMK_LOG_LEVEL); @@ -34,6 +35,7 @@ struct temp_layer_state { struct temp_layer_data { const struct device *dev; + struct k_mutex lock; struct temp_layer_state state; }; @@ -78,28 +80,88 @@ static void update_layer_state(struct temp_layer_state *state, bool activate) { } } +struct layer_state_action { + uint8_t layer; + bool activate; +}; + +K_MSGQ_DEFINE(temp_layer_action_msgq, sizeof(struct layer_state_action), + CONFIG_ZMK_INPUT_PROCESSOR_TEMP_LAYER_MAX_ACTION_EVENTS, 4); + +static void layer_action_work_cb(struct k_work *work) { + + const struct device *dev = DEVICE_DT_INST_GET(0); + struct temp_layer_data *data = (struct temp_layer_data *)dev->data; + + int ret = k_mutex_lock(&data->lock, K_FOREVER); + if (ret < 0) { + LOG_ERR("Error locking for updating %d", ret); + return; + } + + struct layer_state_action action; + + while (k_msgq_get(&temp_layer_action_msgq, &action, K_MSEC(10)) >= 0) { + if (!action.activate) { + if (zmk_keymap_layer_active(action.layer)) { + update_layer_state(&data->state, false); + } + } else { + update_layer_state(&data->state, true); + } + } + + k_mutex_unlock(&data->lock); +} + +static K_WORK_DEFINE(layer_action_work, layer_action_work_cb); + /* Work Queue Callback */ static void layer_disable_callback(struct k_work *work) { struct k_work_delayable *d_work = k_work_delayable_from_work(work); int layer_index = ARRAY_INDEX(layer_disable_works, d_work); - const struct device *dev = DEVICE_DT_INST_GET(0); - struct temp_layer_data *data = (struct temp_layer_data *)dev->data; + struct layer_state_action action = {.layer = layer_index, .activate = false}; - if (zmk_keymap_layer_active(layer_index)) { - update_layer_state(&data->state, false); - } + int ret = k_msgq_put(&temp_layer_action_msgq, &action, K_MSEC(10)); + k_work_submit(&layer_action_work); } /* Event Handlers */ -static int handle_position_state_changed(const zmk_event_t *eh) { +static int handle_layer_state_changed(const struct device *dev, const zmk_event_t *eh) { + struct temp_layer_data *data = (struct temp_layer_data *)dev->data; + int ret = k_mutex_lock(&data->lock, K_FOREVER); + if (ret < 0) { + return ret; + } + if (data->state.toggle_layer == 0) { + return ZMK_EV_EVENT_BUBBLE; + } + if (!zmk_keymap_layer_active(zmk_keymap_layer_index_to_id(data->state.toggle_layer))) { + LOG_DBG("Deactivating layer that was activated by this processor"); + data->state.is_active = false; + k_work_cancel_delayable(&layer_disable_works[data->state.toggle_layer]); + } + ret = k_mutex_unlock(&data->lock); + if (ret < 0) { + return ret; + } + + return ZMK_EV_EVENT_BUBBLE; +} + +static int handle_position_state_changed(const struct device *dev, const zmk_event_t *eh) { const struct zmk_position_state_changed *ev = as_zmk_position_state_changed(eh); if (!ev->state) { return ZMK_EV_EVENT_BUBBLE; } - const struct device *dev = DEVICE_DT_INST_GET(0); struct temp_layer_data *data = (struct temp_layer_data *)dev->data; + int ret = k_mutex_lock(&data->lock, K_FOREVER); + if (ret < 0) { + return ret; + } + const struct temp_layer_config *cfg = dev->config; if (data->state.is_active && cfg->excluded_positions && cfg->num_positions > 0) { @@ -110,35 +172,64 @@ static int handle_position_state_changed(const zmk_event_t *eh) { } LOG_DBG("Position excluded, continuing"); + k_mutex_unlock(&data->lock); + return ZMK_EV_EVENT_BUBBLE; } -static int handle_keycode_state_changed(const zmk_event_t *eh) { +static int handle_keycode_state_changed(const struct device *dev, const zmk_event_t *eh) { const struct zmk_keycode_state_changed *ev = as_zmk_keycode_state_changed(eh); if (!ev->state) { return ZMK_EV_EVENT_BUBBLE; } - const struct device *dev = DEVICE_DT_INST_GET(0); struct temp_layer_data *data = (struct temp_layer_data *)dev->data; + + int ret = k_mutex_lock(&data->lock, K_FOREVER); + if (ret < 0) { + return ret; + } + LOG_DBG("Setting last_tapped_timestamp to: %d", ev->timestamp); data->state.last_tapped_timestamp = ev->timestamp; + ret = k_mutex_unlock(&data->lock); + if (ret < 0) { + return ret; + } + return ZMK_EV_EVENT_BUBBLE; } -static int handle_state_changed_dispatcher(const zmk_event_t *eh) { - if (as_zmk_position_state_changed(eh) != NULL) { +static int handle_state_changed_dispatcher(const struct device *dev, const zmk_event_t *eh) { + if (as_zmk_layer_state_changed(eh) != NULL) { + LOG_DBG("Dispatching handle_layer_state_changed"); + return handle_layer_state_changed(dev, eh); + } else if (as_zmk_position_state_changed(eh) != NULL) { LOG_DBG("Dispatching handle_position_state_changed"); - return handle_position_state_changed(eh); + return handle_position_state_changed(dev, eh); } else if (as_zmk_keycode_state_changed(eh) != NULL) { LOG_DBG("Dispatching handle_keycode_state_changed"); - return handle_keycode_state_changed(eh); + return handle_keycode_state_changed(dev, eh); } return ZMK_EV_EVENT_BUBBLE; } +#define DISPATCH_EVENT(inst) \ + { \ + int err = handle_state_changed_dispatcher(DEVICE_DT_INST_GET(inst), eh); \ + if (err < 0) { \ + return err; \ + } \ + } + +static int handle_event_dispatcher(const zmk_event_t *eh) { + DT_INST_FOREACH_STATUS_OKAY(DISPATCH_EVENT) + + return 0; +} + /* Driver Implementation */ static int temp_layer_handle_event(const struct device *dev, struct input_event *event, uint32_t param1, uint32_t param2, @@ -149,23 +240,37 @@ static int temp_layer_handle_event(const struct device *dev, struct input_event } struct temp_layer_data *data = (struct temp_layer_data *)dev->data; + + int ret = k_mutex_lock(&data->lock, K_FOREVER); + if (ret < 0) { + return ret; + } + const struct temp_layer_config *cfg = dev->config; data->state.toggle_layer = param1; if (!data->state.is_active && !should_quick_tap(cfg, data->state.last_tapped_timestamp, k_uptime_get())) { - update_layer_state(&data->state, true); + struct layer_state_action action = {.layer = param1, .activate = true}; + + int ret = k_msgq_put(&temp_layer_action_msgq, &action, K_MSEC(10)); + k_work_submit(&layer_action_work); } if (param2 > 0) { k_work_reschedule(&layer_disable_works[param1], K_MSEC(param2)); } + k_mutex_unlock(&data->lock); + return ZMK_INPUT_PROC_CONTINUE; } static int temp_layer_init(const struct device *dev) { + struct temp_layer_data *data = (struct temp_layer_data *)dev->data; + k_mutex_init(&data->lock); + for (int i = 0; i < MAX_LAYERS; i++) { k_work_init_delayable(&layer_disable_works[i], layer_disable_callback); } @@ -183,10 +288,8 @@ static const struct zmk_input_processor_driver_api temp_layer_driver_api = { #define NEEDS_KEYCODE_HANDLERS(n, ...) (DT_INST_PROP_OR(n, require_prior_idle_ms, 0) > 0) /* Event Handlers Registration */ -#if DT_INST_FOREACH_STATUS_OKAY_VARGS(NEEDS_POSITION_HANDLERS, ||) || \ - DT_INST_FOREACH_STATUS_OKAY_VARGS(NEEDS_KEYCODE_HANDLERS, ||) -ZMK_LISTENER(processor_temp_layer, handle_state_changed_dispatcher); -#endif +ZMK_LISTENER(processor_temp_layer, handle_event_dispatcher); +ZMK_SUBSCRIPTION(processor_temp_layer, zmk_layer_state_changed); /* Individual Subscriptions */ #if DT_INST_FOREACH_STATUS_OKAY_VARGS(NEEDS_POSITION_HANDLERS, ||) diff --git a/app/tests/pointing/mouse-move/processors/temp_layer/1-deactivate-layer-timeout/keycode_events.snapshot b/app/tests/pointing/mouse-move/processors/temp_layer/1-deactivate-layer-timeout/keycode_events.snapshot index e2b9544e5..55fb9d57e 100644 --- a/app/tests/pointing/mouse-move/processors/temp_layer/1-deactivate-layer-timeout/keycode_events.snapshot +++ b/app/tests/pointing/mouse-move/processors/temp_layer/1-deactivate-layer-timeout/keycode_events.snapshot @@ -1,4 +1,5 @@ layer_changed: layer 1 state 1 +Dispatching handle_layer_state_changed movement_set: Mouse movement set to -1/0 scroll_set: Mouse scroll set to 0/0 movement_set: Mouse movement set to 0/0 @@ -12,3 +13,4 @@ movement_set: Mouse movement set to -3/0 scroll_set: Mouse scroll set to 0/0 movement_set: Mouse movement set to 0/0 layer_changed: layer 1 state 0 +Dispatching handle_layer_state_changed diff --git a/app/tests/pointing/mouse-move/processors/temp_layer/2a-deactivate-layer-position-trigger/keycode_events.snapshot b/app/tests/pointing/mouse-move/processors/temp_layer/2a-deactivate-layer-position-trigger/keycode_events.snapshot index b421bad28..9520a4b4e 100644 --- a/app/tests/pointing/mouse-move/processors/temp_layer/2a-deactivate-layer-position-trigger/keycode_events.snapshot +++ b/app/tests/pointing/mouse-move/processors/temp_layer/2a-deactivate-layer-position-trigger/keycode_events.snapshot @@ -1,6 +1,7 @@ Dispatching handle_position_state_changed Position excluded, continuing layer_changed: layer 1 state 1 +Dispatching handle_layer_state_changed movement_set: Mouse movement set to -1/0 scroll_set: Mouse scroll set to 0/0 movement_set: Mouse movement set to 0/0 @@ -17,5 +18,6 @@ Dispatching handle_position_state_changed Dispatching handle_position_state_changed Position not excluded, deactivating layer layer_changed: layer 1 state 0 +Dispatching handle_layer_state_changed Position excluded, continuing Dispatching handle_position_state_changed diff --git a/app/tests/pointing/mouse-move/processors/temp_layer/2b-deactivate-layer-position-not-trigger/keycode_events.snapshot b/app/tests/pointing/mouse-move/processors/temp_layer/2b-deactivate-layer-position-not-trigger/keycode_events.snapshot index 3cf641daa..f082de9fa 100644 --- a/app/tests/pointing/mouse-move/processors/temp_layer/2b-deactivate-layer-position-not-trigger/keycode_events.snapshot +++ b/app/tests/pointing/mouse-move/processors/temp_layer/2b-deactivate-layer-position-not-trigger/keycode_events.snapshot @@ -1,6 +1,7 @@ Dispatching handle_position_state_changed Position excluded, continuing layer_changed: layer 1 state 1 +Dispatching handle_layer_state_changed movement_set: Mouse movement set to -1/0 scroll_set: Mouse scroll set to 0/0 movement_set: Mouse movement set to 0/0 diff --git a/app/tests/pointing/mouse-move/processors/temp_layer/3-require-prior-idle-ms/keycode_events.snapshot b/app/tests/pointing/mouse-move/processors/temp_layer/3-require-prior-idle-ms/keycode_events.snapshot index 57d5dfc8c..b528b8d07 100644 --- a/app/tests/pointing/mouse-move/processors/temp_layer/3-require-prior-idle-ms/keycode_events.snapshot +++ b/app/tests/pointing/mouse-move/processors/temp_layer/3-require-prior-idle-ms/keycode_events.snapshot @@ -28,6 +28,7 @@ movement_set: Mouse movement set to -1/0 scroll_set: Mouse scroll set to 0/0 movement_set: Mouse movement set to 0/0 layer_changed: layer 1 state 1 +Dispatching handle_layer_state_changed movement_set: Mouse movement set to -2/0 scroll_set: Mouse scroll set to 0/0 movement_set: Mouse movement set to 0/0 @@ -146,3 +147,4 @@ movement_set: Mouse movement set to -9/0 scroll_set: Mouse scroll set to 0/0 movement_set: Mouse movement set to 0/0 layer_changed: layer 1 state 0 +Dispatching handle_layer_state_changed diff --git a/app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/events.patterns b/app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/events.patterns new file mode 100644 index 000000000..9fab690e8 --- /dev/null +++ b/app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/events.patterns @@ -0,0 +1,6 @@ +s/.*hid_mouse_//p +s/.*set_layer_state: //p +s/.*handle_state_changed_dispatcher: //p +s/.*handle_layer_state_changed: //p +s/.*handle_position_state_changed: //p +s/.*handle_keycode_state_changed: //p diff --git a/app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/keycode_events.snapshot b/app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/keycode_events.snapshot new file mode 100644 index 000000000..a4b01e85f --- /dev/null +++ b/app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/keycode_events.snapshot @@ -0,0 +1,31 @@ +layer_changed: layer 1 state 1 +Dispatching handle_layer_state_changed +movement_set: Mouse movement set to -1/0 +scroll_set: Mouse scroll set to 0/0 +movement_set: Mouse movement set to 0/0 +movement_set: Mouse movement set to -2/0 +scroll_set: Mouse scroll set to 0/0 +movement_set: Mouse movement set to 0/0 +movement_set: Mouse movement set to -2/0 +scroll_set: Mouse scroll set to 0/0 +movement_set: Mouse movement set to 0/0 +movement_set: Mouse movement set to -3/0 +scroll_set: Mouse scroll set to 0/0 +movement_set: Mouse movement set to 0/0 +layer_changed: layer 1 state 0 +Dispatching handle_layer_state_changed +Deactivating layer that was activated by this processor +layer_changed: layer 1 state 1 +Dispatching handle_layer_state_changed +movement_set: Mouse movement set to -1/0 +scroll_set: Mouse scroll set to 0/0 +movement_set: Mouse movement set to 0/0 +movement_set: Mouse movement set to -2/0 +scroll_set: Mouse scroll set to 0/0 +movement_set: Mouse movement set to 0/0 +movement_set: Mouse movement set to -2/0 +scroll_set: Mouse scroll set to 0/0 +movement_set: Mouse movement set to 0/0 +movement_set: Mouse movement set to -3/0 +scroll_set: Mouse scroll set to 0/0 +movement_set: Mouse movement set to 0/0 diff --git a/app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/native_posix_64.conf b/app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/native_posix_64.conf new file mode 100644 index 000000000..fa514727b --- /dev/null +++ b/app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/native_posix_64.conf @@ -0,0 +1,6 @@ +CONFIG_GPIO=n +CONFIG_ZMK_BLE=n +CONFIG_LOG=y +CONFIG_LOG_BACKEND_SHOW_COLOR=n +CONFIG_ZMK_LOG_LEVEL_DBG=y +CONFIG_ZMK_POINTING=y diff --git a/app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/native_posix_64.keymap b/app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/native_posix_64.keymap new file mode 100644 index 000000000..6ae1d2285 --- /dev/null +++ b/app/tests/pointing/mouse-move/processors/temp_layer/4-deactivated-layer-externally/native_posix_64.keymap @@ -0,0 +1,43 @@ + +#include +#include + +#include +#include +#include +#include +#include + +&mmv_input_listener { + input-processors = <&zip_temp_layer 1 500>; +}; + +/ { + keymap { + compatible = "zmk,keymap"; + label ="Default keymap"; + + default_layer { + bindings = < + &mmv MOVE_LEFT &mmv MOVE_UP + &mo 1 &none + >; + }; + + mkp_layer { + bindings = <&mkp LCLK &mkp RCLK &trans &trans>; + }; + }; +}; + + +&kscan { + events = < + ZMK_MOCK_PRESS(0,0,100) + ZMK_MOCK_RELEASE(0,0,150) + ZMK_MOCK_PRESS(1,0,100) + ZMK_MOCK_RELEASE(1,0,100) + ZMK_MOCK_PRESS(0,0,100) + ZMK_MOCK_RELEASE(0,0,150) + >; +};