Add event trace_id to fix bug in sticky keys and simplify hold-taps

[okke-formsma]

The 'event trace_id' is a unique identifier that identifies the keypress-keyrelease combination. It can be used to identify which position-state-changed events, behavior calls, and keycode-state-changed events were the result of which keypress (and which release).

The main benefactor of this functionality is sticky keys, where just a position & layer ID combination is not enough to correctly identify a behavior invocation.

Additionally, this is useful for hold-taps, where keycode events generated by the behavior itself must be ignored. So far this was solved with a clunky workaround.

This fixes #508

[joelspadin]

Going to bed now. Will take a look at the rest of this later.

[joelspadin]

Maybe add "get" somewhere in the name? Having a verb somewhere in the name of a function usually makes it easier to understand what it's doing (unless you mean for "trace" to be the verb, but I'm assuming not given that it's used in variable names too).

Also, I'm generally not a fan of boolean parameters since in code like

zmk_event_trace_id(position, TRUE);

you have no idea what the 2nd parameter does without looking at the definition of the function. That said, it is convenient for the couple places this is being called, though if it starts getting used elsewhere you may run into the same readability problem.

Consider something like this, which is more verbose but tells you exactly what it's doing:

uint32_t zmk_event_trace_id_get(uint32_t position) {
  return zmk_event_trace_ids[position];
}
uint32_t zmk_event_trace_id_create(uint32_t position) {
  zmk_last_event_trace_id++;
  zmk_event_trace_ids[position] = zmk_last_event_trace_id;
  return zmk_event_trace_id_get(position);
}

...
event = {
  .trace_id = pressed ? zmk_event_trace_id_create(position) : zmk_event_trace_id_get(position),
};

Alternatively:

uint32_t zmk_event_trace_id_get(uint32_t position) {
  return zmk_event_trace_ids[position];
}
void zmk_event_trace_id_update(uint32_t position) {
  zmk_last_event_trace_id++;
  zmk_event_trace_ids[position] = zmk_last_event_trace_id;
}

...
if (pressed) {
  zmk_event_trace_id_update(position);
}
event = {
  .trace_id = zmk_event_trace_id_get(position),
};

[okke-formsma]

Thanks for your extensive reply! I've added a commit that implements the create/get version. I like how the intent is clearer when calling with a hardcoded value.

On the other hand this solution exposes an implementation detail of trace_ids to the caller, by making the caller responsible for deciding if a new trace_id should be created or an existing one retrieved. Having this decision within the trace_id method puts the responsibility in place in my opinion. (Maybe 'press' would be a better name for the parameter than 'generate', so the intent is clearer).

The update/get variety may be the best middle ground between the two.

I see arguments for each option, I'm not quite sure what the best one is yet.

[joelspadin]

On the other hand this solution exposes an implementation detail of trace_ids to the caller, by making the caller responsible for deciding if a new trace_id should be created or an existing one retrieved.

I'd argue that the caller is still responsible for deciding that in the version that takes a boolean parameter. You just one have one function that does one of two things based on a parameter instead of two functions that each do one thing.

If you want to hide away the fact that new trace IDs get generated on key press, then renaming the parameter to something like is_press (like you mentioned) would be better, though it would still be difficult to understand if any later code ends up needing to pass just TRUE or FALSE for that parameter. You could also rename the "update" function to something like zmk_event_trace_id_notify_pressed(position) (though then it's not super clear if you're notifying the trace ID system of a press, as intended, or if you want to send a notification). Naming things is hard.

[okke-formsma]

I've decided to go with zmk_get_event_trace_id(uint32_t position, bool pressed), as I think it's the clearest code for now. I don't expect other call sites that will use literal booleans, and if that will be the case we can always refactor this method.

[joelspadin]

I'm not super familiar with the problem being solved here, so I'll let Pete give the final approval, but this looks reasonable to me.

[joelspadin]

Note that after 4 billion key presses, the trace ID will eventually wrap and this will temporarily break. That's also 13 years straight of hitting ten keys a second, so it's probably not something to worry about, but here's a solution that always works, even if it wraps (so long as you don't manage to press 2 billion keys between the times the two trace IDs are generated):

if ((int32_t)(ev->trace_id - undecided_hold_tap->trace_id) < 0) {

https://godbolt.org/z/cjs41G

All that said, it looks like you picked max uint32 as a special value below, so you'd need other changes for things to work if the number wraps. Probably not worth the effort.

[okke-formsma]

Yeah I made a similar calculation and decided that if someone hits this bug they're due for some undefined behavior.

[okke-formsma]

@petejohanson could you have a look at this?

[petejohanson]

I need to consider this more, but I think the trace ID isn't really part of every payload, but part of some "event context" instead, and should be pervasive, and surfaced in the general event manager API, not hidden in some event payloads.

Thoughts?

[okke-formsma]

I need to consider this more, but I think the trace ID isn't really part of every payload, but part of some "event context" instead, and should be pervasive, and surfaced in the general event manager API, not hidden in some event payloads.

I'm not sure if it's applicable to many events outside the position, (future) behavior and keypress events. If we'd put the trace ID on the zmk_event_t, that would complicate construction for all events without clear benefit. Also, pointers to zmk_event_t would need to be kept around instead of pointers to the events themselves (e.g. zmk_position_state_changed), adding a bit of complexity too. So, at least for now, I'd opt to keep the scope limited.

[okke-formsma]

example of a function signature that would become more complex if we'd keep a zmk_event_t around instead of zmk_position_state_changed.