-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(behaviors): Swapper implementation #1366
base: main
Are you sure you want to change the base?
Conversation
I have been testing the window swapper implementation as in the example for the last day and it is working well so far. It neatly fixes #997 using a combination of first and second approaches proposed in there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! Love the idea, and the general approach.
A few thoughts on implementation details/changes added here.
As far as naming.... Let me ponder. "Smart Interrupt" certainly feels better than "Swapper", but I'm curious if anyone has any other ideas for a better name. I also think "smart" is superfluous... A lot of what ZMK does is "smart", so it feels odd to include it in the naming for just this one behavior.
- "Three Phase Behavior"
- *Tri-Phase Behavior"
- "Start/Continue/End Behavior"
- "Tri-State Behavior"
Just a few that come to mind as food for thought.
struct zmk_behavior_binding *behaviors; | ||
int32_t shared_layers[32]; | ||
int32_t timeout_ms; | ||
int32_t shared_key_positions[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, could be a bitmask instead of this "sparse" array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the discussion on discord this is a bit tricky since the keymap can be any length. I've changed it to 8-bit ints for now but will keep working on a better solution.
93e215c
to
69a3948
Compare
About the "tri-state" naming: It might be confusable with the "tri-layer" ( |
Maybe three-phase would be better, or would 'triplex' or 'treble' work here? Start/Continue/End Behavior gets the point across, but I feel like it's a bit too specific. |
I like the "bookend"/"sandwich" suggestions from @petejohanson personally, since it is like the repeatable "continue" behavior is bookended by special start and end events. Three-phase also sounds good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An implementation comment... And then a bump again on naming... I'm on the fence on tri-state. @nickconway I didn't see you're thought on "bookend" concept for the name?
behavior_keymap_binding_pressed((struct zmk_behavior_binding *)&cfg->start_behavior, event); | ||
behavior_keymap_binding_released((struct zmk_behavior_binding *)&cfg->start_behavior, | ||
event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we do this for hold taps right now, but any reason not to use the behavior queue here?
event); | ||
tri_state->first_press = false; | ||
} | ||
behavior_keymap_binding_pressed((struct zmk_behavior_binding *)&cfg->continue_behavior, event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
behavior_keymap_binding_released( | ||
(struct zmk_behavior_binding *)&tri_state->config->continue_behavior, event); | ||
} | ||
behavior_keymap_binding_pressed( | ||
(struct zmk_behavior_binding *)&tri_state->config->end_behavior, event); | ||
behavior_keymap_binding_released( | ||
(struct zmk_behavior_binding *)&tri_state->config->end_behavior, event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
@petejohanson Apologies for the delay. I think the bookend name is much better. As for the behavior queue, my understanding was that the keymap_* functions would be used here since we want the action taken immediately? |
I'm trying to use |
I have this running on my Glove80 and it's working pretty well so far, but I'm running into an issue. I have the following definition:
In my layout, I have a key with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to compile zmk
with the latest main
and zephyr 3.5 with this behaviour, and had a few compilation errors.
for anyone interested, there are a few changes that made it work for me (you can find a complete patch in lttb@2b856c5)
} | ||
|
||
void behavior_tri_state_timer_handler(struct k_work *item) { | ||
struct active_tri_state *tri_state = CONTAINER_OF(item, struct active_tri_state, release_timer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from Zephyr v3.5.0 Migration Guide
CONTAINER_OF now performs type checking, this was very commonly misused to obtain user structure from k_work pointers without passing from k_work_delayable. This would now result in a build error and have to be done correctly using k_work_delayable_from_work().
struct active_tri_state *tri_state = CONTAINER_OF(item, struct active_tri_state, release_timer); | |
struct k_work_delayable *ditem = k_work_delayable_from_work(item); | |
struct active_tri_state *tri_state = CONTAINER_OF(ditem, struct active_tri_state, release_timer); |
|
||
#define _TRANSFORM_ENTRY(idx, node) \ | ||
{ \ | ||
.behavior_dev = DT_LABEL(DT_INST_PHANDLE_BY_IDX(node, bindings, idx)), \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- in Zephyr 3.2 Upgrade #1499,
DT_LABEL
was migrated toDT_PROP
- and then in refactor!: Remove deprecated label property #2028,
DT_PROP
was migrated toDEVICE_DT_NAME
.behavior_dev = DT_LABEL(DT_INST_PHANDLE_BY_IDX(node, bindings, idx)), \ | |
.behavior_dev = DEVICE_DT_NAME(DT_INST_PHANDLE_BY_IDX(node, bindings, idx)), \ |
.start_behavior = _TRANSFORM_ENTRY(0, n), \ | ||
.continue_behavior = _TRANSFORM_ENTRY(1, n), \ | ||
.end_behavior = _TRANSFORM_ENTRY(2, n)}; \ | ||
DEVICE_DT_INST_DEFINE(n, behavior_tri_state_init, NULL, NULL, &behavior_tri_state_config_##n, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in #2028 DEVICE_DT_INST_DEFINE
was migrated to BEHAVIOR_DT_INST_DEFINE
in behaviours
DEVICE_DT_INST_DEFINE(n, behavior_tri_state_init, NULL, NULL, &behavior_tri_state_config_##n, \ | |
BEHAVIOR_DT_INST_DEFINE(n, behavior_tri_state_init, NULL, NULL, &behavior_tri_state_config_##n, \ |
Thank you very much for all the effort. I gave it a try. Works when added directly to the keymap. When I add it via a combo though, it doesn't work. Does anybody have the same problem or a workaround? |
You have to include the combo locations in the ignore-positions. See #1366 (comment) |
@urob thank you very much!!! I just noticed my mistake. For others that want to use it with zmk-nodefree-config, here's how it works.
|
54a2252
to
0876cbc
Compare
6afccca
to
fb95060
Compare
Extracted from zmkfirmware#1366
Implementation of #997. Smart interrupt is probably not the name we should use, but I was out of ideas, help wanted!