Layer change event zmk_layer_state_changed for layer 0 inconsistent

[ergodone]

In looking at Layer change event I noticed there is a zmk_layer_state_changed event for layer 0 the first time a transition is made back to it but no reports when it is initially activated or when it is subsequently returned to.

I noticed this when I was trying to set layer leds based on the zmk_layer_state_changed event.

My key board has the mapping between layers set to: &to 1 -> &to 2 -> &to 3 -> &to 0

In the logs below you can see the first transition from layer 3->0 is reported but the subsequent one is not. Note I added the _zmk_layer_state_changedlistener log message just before the event is sent.

Here's the patch I'd propose, see any issues with this?

diff --git a/app/src/keymap.c b/app/src/keymap.c
index 75ec6bf..7567d7f 100644
--- a/app/src/keymap.c
+++ b/app/src/keymap.c
@@ -19,8 +19,9 @@ LOG_MODULE_DECLARE(zmk, CONFIG_ZMK_LOG_LEVEL);
 #include <zmk/events/layer_state_changed.h>
 #include <zmk/events/sensor_event.h>

-static zmk_keymap_layers_state_t _zmk_keymap_layer_state = 0;
-static uint8_t _zmk_keymap_layer_default = 0;
+static const uint8_t _zmk_keymap_layer_default = 0;
+static zmk_keymap_layers_state_t _zmk_keymap_layer_state = BIT(_zmk_keymap_layer_default);
+

 #define DT_DRV_COMPAT zmk_keymap

@@ -87,7 +88,7 @@ static inline int set_layer_state(uint8_t layer, bool state) {
     zmk_keymap_layers_state_t old_state = _zmk_keymap_layer_state;
     WRITE_BIT(_zmk_keymap_layer_state, layer, state);
     // Don't send state changes unless there was an actual change
-    if (old_state != _zmk_keymap_layer_state) {
+    if (old_state != _zmk_keymap_layer_state || layer == _zmk_keymap_layer_default) {
         LOG_DBG("layer_changed: layer %d state %d", layer, state);
         ZMK_EVENT_RAISE(create_layer_state_changed(layer, state));
     }

[00:00:14.607,574] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:14.607,604] <dbg> zmk.zmk_event_manager_handle_from: Listener captured the event
[00:00:14.711,944] <dbg> zmk.zmk_keymap_apply_position_state: layer: 0 position: 17, binding name: TO_LAYER
[00:00:14.711,975] <dbg> zmk.to_keymap_binding_pressed: position 17 layer 1
[00:00:14.711,975] <dbg> zmk.set_layer_state: layer_changed: layer 1 state 1
[00:00:14.712,005] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 1 1
[00:00:15.220,367] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state off
[00:00:15.220,458] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: false

[00:00:15.220,489] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:15.220,520] <dbg> zmk.zmk_keymap_apply_position_state: layer: 0 position: 17, binding name: TO_LAYER
[00:00:15.220,550] <dbg> zmk.to_keymap_binding_released: position 17 layer 1
[00:00:15.911,407] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state on
[00:00:15.911,468] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: true

[00:00:15.911,499] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:15.911,529] <dbg> zmk.zmk_event_manager_handle_from: Listener captured the event
[00:00:16.015,686] <dbg> zmk.zmk_keymap_apply_position_state: layer: 1 position: 17, binding name: TO_LAYER
[00:00:16.015,747] <dbg> zmk.to_keymap_binding_pressed: position 17 layer 2
[00:00:16.015,747] <dbg> zmk.set_layer_state: layer_changed: layer 1 state 0
[00:00:16.015,777] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 1 0
[00:00:16.015,777] <dbg> zmk.set_layer_state: layer_changed: layer 2 state 1
[00:00:16.015,808] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 2 1
[00:00:16.523,895] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state off
[00:00:16.523,986] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: false

[00:00:16.524,017] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:16.524,047] <dbg> zmk.zmk_keymap_apply_position_state: layer: 1 position: 17, binding name: TO_LAYER
[00:00:16.524,078] <dbg> zmk.to_keymap_binding_released: position 17 layer 2
[00:00:16.727,386] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state on
[00:00:16.727,447] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: true

[00:00:16.727,478] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:16.727,478] <dbg> zmk.zmk_event_manager_handle_from: Listener captured the event
[00:00:16.831,695] <dbg> zmk.zmk_keymap_apply_position_state: layer: 2 position: 17, binding name: TO_LAYER
[00:00:16.831,756] <dbg> zmk.to_keymap_binding_pressed: position 17 layer 3
[00:00:16.831,756] <dbg> zmk.set_layer_state: layer_changed: layer 2 state 0
[00:00:16.831,787] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 2 0
[00:00:16.831,787] <dbg> zmk.set_layer_state: layer_changed: layer 3 state 1
[00:00:16.831,817] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 3 1
[00:00:17.339,904] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state off
[00:00:17.339,996] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: false

[00:00:17.340,026] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:17.340,057] <dbg> zmk.zmk_keymap_apply_position_state: layer: 2 position: 17, binding name: TO_LAYER
[00:00:17.340,087] <dbg> zmk.to_keymap_binding_released: position 17 layer 3
[00:00:17.657,531] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state on
[00:00:17.657,562] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: true

[00:00:17.657,592] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:17.657,623] <dbg> zmk.zmk_event_manager_handle_from: Listener captured the event
[00:00:17.762,207] <dbg> zmk.zmk_keymap_apply_position_state: layer: 3 position: 17, binding name: TO_LAYER
[00:00:17.762,268] <dbg> zmk.to_keymap_binding_pressed: position 17 layer 0
[00:00:17.762,268] <dbg> zmk.set_layer_state: layer_changed: layer 3 state 0
[00:00:17.762,298] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 3 0
[00:00:17.762,329] <dbg> zmk.set_layer_state: layer_changed: layer 0 state 1
[00:00:17.762,329] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 0 1
[00:00:18.270,599] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state off
[00:00:18.270,690] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: false

[00:00:18.270,721] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:18.270,751] <dbg> zmk.zmk_keymap_apply_position_state: layer: 3 position: 17, binding name: TO_LAYER
[00:00:18.270,782] <dbg> zmk.to_keymap_binding_released: position 17 layer 0
[00:00:24.537,322] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state on
[00:00:24.537,353] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: true

[00:00:24.537,414] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:24.537,414] <dbg> zmk.zmk_event_manager_handle_from: Listener captured the event
[00:00:24.641,632] <dbg> zmk.zmk_keymap_apply_position_state: layer: 0 position: 17, binding name: TO_LAYER
[00:00:24.641,662] <dbg> zmk.to_keymap_binding_pressed: position 17 layer 1
[00:00:24.641,693] <dbg> zmk.set_layer_state: layer_changed: layer 1 state 1
[00:00:24.641,723] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 1 1
[00:00:25.149,810] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state off
[00:00:25.149,902] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: false

[00:00:25.149,932] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:25.149,963] <dbg> zmk.zmk_keymap_apply_position_state: layer: 0 position: 17, binding name: TO_LAYER
[00:00:25.149,993] <dbg> zmk.to_keymap_binding_released: position 17 layer 1
[00:00:25.820,587] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state on
[00:00:25.820,617] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: true

[00:00:25.820,648] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:25.820,678] <dbg> zmk.zmk_event_manager_handle_from: Listener captured the event
[00:00:25.924,987] <dbg> zmk.zmk_keymap_apply_position_state: layer: 1 position: 17, binding name: TO_LAYER
[00:00:25.925,018] <dbg> zmk.to_keymap_binding_pressed: position 17 layer 2
[00:00:25.925,018] <dbg> zmk.set_layer_state: layer_changed: layer 1 state 0
[00:00:25.925,048] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 1 0
[00:00:25.925,079] <dbg> zmk.set_layer_state: layer_changed: layer 2 state 1
[00:00:25.925,109] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 2 1
[00:00:26.433,197] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state off
[00:00:26.433,288] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: false

[00:00:26.433,319] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:26.433,319] <dbg> zmk.zmk_keymap_apply_position_state: layer: 1 position: 17, binding name: TO_LAYER
[00:00:26.433,380] <dbg> zmk.to_keymap_binding_released: position 17 layer 2
[00:00:26.623,748] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state on
[00:00:26.623,779] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: true

[00:00:26.623,809] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:26.623,840] <dbg> zmk.zmk_event_manager_handle_from: Listener captured the event
[00:00:26.728,240] <dbg> zmk.zmk_keymap_apply_position_state: layer: 2 position: 17, binding name: TO_LAYER
[00:00:26.728,302] <dbg> zmk.to_keymap_binding_pressed: position 17 layer 3
[00:00:26.728,302] <dbg> zmk.set_layer_state: layer_changed: layer 2 state 0
[00:00:26.728,332] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 2 0
[00:00:26.728,332] <dbg> zmk.set_layer_state: layer_changed: layer 3 state 1
[00:00:26.728,363] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 3 1
[00:00:27.236,450] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state off
[00:00:27.236,541] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: false

[00:00:27.236,572] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:27.236,602] <dbg> zmk.zmk_keymap_apply_position_state: layer: 2 position: 17, binding name: TO_LAYER
[00:00:27.236,633] <dbg> zmk.to_keymap_binding_released: position 17 layer 3
[00:00:27.600,219] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state on
[00:00:27.600,250] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: true

[00:00:27.600,280] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:27.600,311] <dbg> zmk.zmk_event_manager_handle_from: Listener captured the event
[00:00:27.704,528] <dbg> zmk.zmk_keymap_apply_position_state: layer: 3 position: 17, binding name: TO_LAYER
[00:00:27.704,589] <dbg> zmk.to_keymap_binding_pressed: position 17 layer 0
[00:00:27.704,589] <dbg> zmk.set_layer_state: layer_changed: layer 3 state 0
[00:00:27.704,620] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 3 0
[00:00:27.751,647] <dbg> zmk.kscan_gpio_read_0: Sending event at 5,13 state off
[00:00:27.751,800] <dbg> zmk.zmk_kscan_process_msgq: Row: 5, col: 13, position: 17, pressed: false

[00:00:27.751,831] <dbg> zmk.position_state_changed_listener: 17 bubble (no undecided hold_tap active)
[00:00:27.751,861] <dbg> zmk.zmk_keymap_apply_position_state: layer: 3 position: 17, binding name: TO_LAYER
[00:00:27.751,892] <dbg> zmk.to_keymap_binding_released: position 17 layer 0

[okke-formsma]

layer 0 is never deactivated because it's the base layer. As such, you won't see these events, because the layer is not actually toggled off.

[ergodone]

Except it does report it - on the first transition back to 0.

[okke-formsma]

Hm yeah that's a bug. The actual fix is to set static zmk_keymap_layers_state_t _zmk_keymap_layer_state = 1;.

Instead of trying to keep track of which layers are active, could use these methods to get the current state:

bool zmk_keymap_layer_active(uint8_t layer)
uint8_t zmk_keymap_highest_layer_active()

[ergodone]

To fix the initial state I did this: +static const uint8_t _zmk_keymap_layer_default = 0; +static zmk_keymap_layers_state_t _zmk_keymap_layer_state = BIT(_zmk_keymap_layer_default); that doesn't assume layer 0 is default.

[ergodone]

Is there a reason for not reporting layer 0 is the topmost active layer? Granted it's state did not change but in effect it has gone from inactive to active.

[ergodone]

Not reporting 0 as active results in consumers of zmk_layer_state_changed seeing intermediate transitions to layer 0 that don't actually occur from a key processing standpoint.

For example (looking at the log) the transition from 1->2 results in

[00:00:16.015,747] <dbg> zmk.set_layer_state: layer_changed: layer 1 state 0
[00:00:16.015,777] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 1 0
[00:00:16.015,777] <dbg> zmk.set_layer_state: layer_changed: layer 2 state 1
[00:00:16.015,808] <dbg> zmk.layer_event_listener: *****zmk_layer_state_changed listener: 2 1

From the zmk_layer_state_changed consumer perspective (where layer 0 is the assumed default) the layer transition becomes: 1 -> 0 -> 2 At the point where the transition from 1 -> 0 is made it's unclear if this is an intermediate state or there is a following 0->2 transition. In the log this is shown in the 3->0 transition where it is unclear if this is the final transition to 0 or an intermediate transition to 4.

By reporting transitions to layer 0 all these issues are resolved. That is the reason for the second part of the patch:

-    if (old_state != _zmk_keymap_layer_state) {
+    if (old_state != _zmk_keymap_layer_state || layer == _zmk_keymap_layer_default) {

Note this does not remove any existing zmk_layer_state_changed events but only adds the transition back to default layer event.

[mcrosson]

As the human who coded layer state stuff...

@ergodone 's solution is more appropriate than @okke-formsma 's IMO. If the default layer can be non-zero at a later point it'll be more flexible of a solution and we won't have to re-visit some of this discussion.

The 'upper most active layer' should report 0 as active if no other layers are also active. The tweaked conditional in the patch above and other minor adjustments are what I would approve if the PR is open.

I think PR #702 doesn't properly address the issue in the broader context of what was reported or for the future if the default layer can be changed.

[ergodone]

I think a better way of reporting the layer changes would be to send one event for the single logical layer change. Currently the code sends a layer change event for each layer that changes state in within a single logical layer change. For example the &to behavior could result in a layer change event sent for every layer even though it is a single logical layer change:

int zmk_keymap_layer_to(uint8_t layer) {
    for (int i = ZMK_KEYMAP_LAYERS_LEN - 1; i >= 0; i--) {
        zmk_keymap_layer_deactivate(i);
    }

    zmk_keymap_layer_activate(layer);

    return 0;
}

The zmk_keymap_layer_activate() and zmk_keymap_layer_deactivate() call set_layer_state() which then sends the layer change event.

I think it would make more sense to send a single layer change event, matching the logical layer change, and include the current layer state in the event. The code change would be to move the ZMK_EVENT_RAISE from set_layer_state() to zmk_keymap_layer_to() / zmk_keymap_layer_toggle() and refactor zmk_keymap_layer_activate() / zmk_keymap_layer_deactivate() for the momentary layer switching calls.

[mcrosson]

I think a better way of reporting the layer changes would be to send one event for the single logical layer change.

This won't work as other things may be tracking individual layer states so you need to send an event for all of the state changes.

This was discussed with @petejohanson during the initial implementation and we agreed that sending a change event for all affected layers was the right call.

There is already the 'highest active layer' method for code that doesn't need to be bothered with every state change.

The proposed change in #704 is the right fix IMO and that should be all we need.

[ergodone]

This won't work as other things may be tracking individual layer states so you need to send an event for all of the state changes. That was the reason for including the current layer state in the event. Is something actually responding to each individual event that could not be handled by a logical layer event change that includes the state?

The underlying problem is the consumer of the events can not tell when the logical layer change has completed or if there's more events coming.

[mcrosson]

That was the reason for including the current layer state in the event. Is something actually responding to each individual event that could not be handled by a logical layer event change that includes the state?

No, if you include old state in the event it should be OK. That'd reduce the number of events and solve the 'are there more' that may be problematic in the future.

Are you willing to update #704 with the changes?

[ergodone]

I can make the changes. It'll be a bit before I can get to them.

[okke-formsma]

I love the proposal to send the new layer state instead of the 'changed layers' one-by-one. It also nicely circumvents the problem concerning whether or not to trigger an event for the default layer.

[mcrosson]

@ergodone Would it be wise to have a 'did layer change' method that takes previous state and a layer id to tell if there was a change?

That'd allow other areas of the code to quickly check for state changes and increase code re-use

[ergodone]

We could pass the old and new state in the event and provide functions to see what changed. I don't need that for my use case - I'm just interested in the highest active layer.

I don't know how the events are used in general so I don't have an informed opinion on whether a 'did layer change' method would be useful.

[mcrosson]

We could pass the old and new state in the event and provide functions to see what changed

That would be good. They are used lightly at the moment, mainly for widget stuff but I would expect that to change as additional features are added to zmk.

[ergodone]

I only added the old state to the event since the current state is readily available. To compare the state I added the function: bool zmk_keymap_layer_changed_from_prior_state(uint8_t layer, zmk_keymap_layers_state_t prior_state) which was modeled after the existing function: bool zmk_keymap_layer_active_with_state(uint8_t layer, zmk_keymap_layers_state_t state_to_test)