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): Turbo Key #1414

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

Conversation

nickconway
Copy link
Contributor

Functionality for continuous repeating of any behavior.

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.

One main question on the state management here. Thanks for the PR!

app/src/behaviors/behavior_turbo_key.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_turbo_key.c Outdated Show resolved Hide resolved
@petejohanson petejohanson added behaviors enhancement New feature or request labels Oct 8, 2022
@petejohanson petejohanson self-assigned this Oct 8, 2022
@urob urob mentioned this pull request Mar 18, 2023
@nickconway nickconway requested a review from a team as a code owner August 29, 2023 14:58
@nickconway nickconway force-pushed the turbo-key branch 2 times, most recently from e42c8ba to d38b4d3 Compare August 29, 2023 15:15

## Summary

The turbo behavior will repeatedly trigger a behavior after a specified amount of time.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit unclear on how exactly this works and it would be good to clarify further here. I think by default, (no toggle term) it only repeats while the key is held, and cancels repeating when it is released, is it?

But if toggle term is set and tap duration is shorter than that, it stays active and the next press cancels it?

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.

Sorry for the delay in review. A few thoughts on the implementation and a high level question on parameterizing this.


LOG_DBG("%d started new turbo", event.position);
data->press_time = k_uptime_get();
k_work_init_delayable(&data->release_timer, behavior_turbo_timer_handler);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be initialized in the init function for the behavior instance, not every time here.

Comment on lines +102 to +104
zmk_behavior_queue_add(event.position, cfg->binding, true, cfg->tap_ms);
zmk_behavior_queue_add(event.position, cfg->binding, false, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little duplicated from the same code in the timer handler, Can we pull that out to avoid duplication?


#define _TRANSFORM_ENTRY(idx, node) \
{ \
.behavior_dev = DT_LABEL(DT_INST_PHANDLE_BY_IDX(node, bindings, idx)), \
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be refactored to:

Suggested change
.behavior_dev = DT_LABEL(DT_INST_PHANDLE_BY_IDX(node, bindings, idx)), \
.behavior_dev = DT_PROP(DT_INST_PHANDLE_BY_IDX(node, bindings, idx), label), \

For some Zephyr changes.

Comment on lines +35 to +38
int tap_ms;
int wait_ms;
struct zmk_behavior_binding binding;

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any reason this is duplicated in the data, and in the config. Should just be in the config?

tap-ms = <5>;
wait-ms = <300>;
toggle-term-ms = <50>;
bindings = <&kp C>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason we couldn't "parameterize" this the same way we did for macros recently? Seems a shame to force a new turbo for every keycode you want to use like this, for instance.

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.

3 participants