Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nickconway
Copy link
Contributor

Implementation of #997. Smart interrupt is probably not the name we should use, but I was out of ideas, help wanted!

@dxmh dxmh added behaviors enhancement New feature or request labels Jun 30, 2022
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Jul 20, 2022
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Jul 20, 2022
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Jul 20, 2022
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Jul 20, 2022
@caksoylar
Copy link
Contributor

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.

caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Jul 21, 2022
Copy link
Contributor

@petejohanson petejohanson left a 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.

docs/docs/behaviors/smart-interrupt.md Outdated Show resolved Hide resolved
app/src/behaviors/behavior_smart_interrupt.c Outdated Show resolved Hide resolved
struct zmk_behavior_binding *behaviors;
int32_t shared_layers[32];
int32_t timeout_ms;
int32_t shared_key_positions[];
Copy link
Contributor

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.

Copy link
Contributor Author

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.

app/src/behaviors/behavior_smart_interrupt.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_smart_interrupt.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_smart_interrupt.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_smart_interrupt.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_smart_interrupt.c Outdated Show resolved Hide resolved
@caksoylar
Copy link
Contributor

About the "tri-state" naming: It might be confusable with the "tri-layer" (lower+raise = adjust) pattern that we support with conditional layers.

caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Jul 31, 2022
@nickconway
Copy link
Contributor Author

About the "tri-state" naming: It might be confusable with the "tri-layer" (lower+raise = adjust) pattern that we support with conditional layers.

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.

@caksoylar
Copy link
Contributor

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.

caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Aug 8, 2022
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Aug 9, 2022
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Sep 1, 2022
caksoylar added a commit to caksoylar/zmk-config that referenced this pull request Sep 1, 2022
Copy link
Contributor

@petejohanson petejohanson left a 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?

Comment on lines 145 to 147
behavior_keymap_binding_pressed((struct zmk_behavior_binding *)&cfg->start_behavior, event);
behavior_keymap_binding_released((struct zmk_behavior_binding *)&cfg->start_behavior,
event);
Copy link
Contributor

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Comment on lines 262 to 268
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

@petejohanson petejohanson self-assigned this Oct 8, 2022
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Oct 15, 2022
@nickconway
Copy link
Contributor Author

@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?

caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Nov 1, 2022
johnm added a commit to johnm/zmk that referenced this pull request Nov 6, 2022
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Nov 7, 2022
jdart pushed a commit to jdart/zmk that referenced this pull request Nov 18, 2022
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Nov 27, 2022
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Dec 5, 2022
caksoylar pushed a commit to caksoylar/zmk that referenced this pull request Dec 11, 2022
@danielo515
Copy link

@danielo515 can you shed some light onto your magic alt-tab implementation? I looked into your config and I'm a bit lost :) would love just general explanation how you've managed to pull it off. Thanks!

It is very simple in reality. I have a key that moves to app switch layer, which has all keys to make me leave that layer, except the ones I want to use (left, right, and Q for quit). Here is the layer:
https://github.com/danielo515/glove80-zmk-config/blob/40d69811521b449946ec341afe3eadedeb2a846e/config/glove80.keymap#L460-L471

And the relevant macros are this ones:
https://github.com/danielo515/glove80-zmk-config/blob/40d69811521b449946ec341afe3eadedeb2a846e/config/glove80.keymap#L156-L180

@kaievns
Copy link

kaievns commented May 16, 2023

It is very simple in reality. I have a key that moves to app switch layer, which has all keys to make me leave that layer, except the ones I want to use (left, right, and Q for quit).

Ah, so it's not really a one key swapper like QMK, it's just a new layer with different keys that you use to switch between the windows, right?

@urob
Copy link
Contributor

urob commented May 16, 2023

You can make it a one-key swapper if you want. Just don't bind the arrows. The docs included with this PR feature an example.

@petejohanson
Copy link
Contributor

@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 say we go w/ bookend term, barring any major disagreement. Can you update this with naming, docs, etc that reflect that?

@burgessa23
Copy link

I'm trying to use &swap on an encoder and can't seem to get it to fire although it compiles fine... any ideas?

yuanbin added a commit to yuanbin/zmk that referenced this pull request Jun 15, 2023
allymparker added a commit to allymparker/zmk that referenced this pull request Jul 16, 2023
j-w-e added a commit to j-w-e/zmk that referenced this pull request Aug 17, 2023
pipex added a commit to pipex/zmk-corne that referenced this pull request Aug 31, 2023
@bryanforbes
Copy link

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:

macswap: macswap {
    label = "macswap";
    compatible = "zmk,behavior-tri-state";
    #binding-cells = <0>;
    bindings = <&kt LGUI>, <&kp TAB>, <&kt LGUI>;
    ignored-key-positions = <38>; // shift
}

In my layout, I have a key with &sk LGUI and a key with &macswap next to each other. The problem I'm running into is if I accidentally hit &sk LGUI before &macswap, LGUI is permanently sticky and I have to turn the keyboard off in order to reset it.

j-w-e added a commit to j-w-e/zmk that referenced this pull request Sep 23, 2023
allymparker added a commit to allymparker/zmk that referenced this pull request Oct 6, 2023
allymparker added a commit to allymparker/zmk that referenced this pull request Oct 6, 2023
allymparker added a commit to allymparker/zmk that referenced this pull request Oct 6, 2023
allymparker added a commit to allymparker/zmk that referenced this pull request Nov 21, 2023
allymparker added a commit to allymparker/zmk that referenced this pull request Jan 23, 2024
Copy link

@lttb lttb left a 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);
Copy link

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().

Suggested change
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)), \
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.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, \
Copy link

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

Suggested change
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, \

@davidsteinberger
Copy link

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?

@urob
Copy link
Contributor

urob commented Mar 21, 2024

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)

@davidsteinberger
Copy link

davidsteinberger commented Mar 21, 2024

@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.

ZMK_COMBO(sw_app, &swapper, 21 22 23, ALL, 100)
// Alt+Tab swapper, requires PR #1366
ZMK_BEHAVIOR(swapper, tri_state,
    bindings = <&kt LGUI>, <&kp TAB>, <&kt LGUI>;
    timeout-ms = <500>;
    ignored-key-positions = <21 22 23>;
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behaviors enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet