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(combos): initial implementation #504

Merged
merged 1 commit into from Jan 14, 2021

Conversation

okke-formsma
Copy link
Collaborator

@okke-formsma okke-formsma commented Dec 11, 2020

An implementation of combos for #45

This has been tested by TJ and others for a while, so I think it's pretty good.

example of a keymap using combos:


/ {
	combos {
		compatible = "zmk,combo";
		combo_one {
			timeout-ms = <30>;
			key-positions = <0 1>;
			bindings = <&mt LEFT_CONTROL C>;
		};

		combo_two {
			timeout-ms = <50>;
			key-positions = <0 1 2>;
			bindings = <&kp D>;
		};
	};

	keymap {
		compatible = "zmk,keymap";
		label ="Default keymap";

		default_layer {
			bindings = <
				&kp A &kp B
				&kp Z &kp Y
			>;
		};
	};
};

Docs are still to do.

app/src/behaviors/behavior_combo.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_combo.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_combo.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_combo.c Outdated Show resolved Hide resolved
@okke-formsma
Copy link
Collaborator Author

@joelspadin thanks for your comments! I've addressed each of them.

@okke-formsma
Copy link
Collaborator Author

rebased on 2.4.0

@okke-formsma
Copy link
Collaborator Author

okke-formsma commented Dec 16, 2020

I've identified a minor issue with the combo keys in the ticket: #45 (comment)

I think we should wait merging until this is fixed. Reviews of the code are very welcome in the meantime.

@okke-formsma
Copy link
Collaborator Author

The issue between hold-taps and combos has been fixed, I'll merge in that commit after review.

@okke-formsma
Copy link
Collaborator Author

@petejohanson Can you review soon? I know quite a few people are really looking forward to combos. The branch has been tested by various people (@tominabox1, myself, various others in #testing).

@petejohanson
Copy link
Contributor

/ {
behaviors {
combo_one {
compatible = "zmk,behavior-combo";
label = "COMBO_ONE";
timeout-ms = <30>;
key-positions = <0 1>;
bindings = <&mt LEFT_CONTROL C>;
};

  combo_two {
  	compatible = "zmk,behavior-combo";
  	label = "COMBO_TWO";
  	timeout-ms = <50>;
  	key-positions = <0 1 2>;
  	bindings = <&kp D>;
  };

};

keymap {
compatible = "zmk,keymap";
label ="Default keymap";

  default_layer {
  	bindings = <
  		&kp A &kp B
  		&kp Z &kp Y
  	>;
  };

};
};

@okke-formsma A comment on the DT format: This is a scenario where I I would likely use the DT bindings child-binding approach, and end up with something like:

/ {
	combos {
		compatible = "zmk,behavior-combos";

		combo_one {
			timeout-ms = <30>;
			key-positions = <0 1>;
			bindings = <&mt LEFT_CONTROL C>;
		};

		combo_two {
			timeout-ms = <50>;
			key-positions = <0 1 2>;
			bindings = <&kp D>;
		};
	};

	keymap {
		compatible = "zmk,keymap";
		label ="Default keymap";

		default_layer {
			bindings = <
				&kp A &kp B
				&kp Z &kp Y
			>;
		};
	};
};

It avoids duplication, and leaves the DT more focused. Will review the code soon, but this stuck out as a user experience issue.

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.

A few cursory thoughts. Thanks!

app/Kconfig Outdated Show resolved Hide resolved
app/src/behaviors/behavior_combo.c Outdated Show resolved Hide resolved

struct combo_t {
int32_t key_positions[CONFIG_ZMK_BHV_COMBO_MAX_KEYS_PER_COMBO];
int32_t key_position_len;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead, can we fill the rest with MAX to avoid extra storage per combo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This prevents looping over the items to count in other places, so I think it's worth it.

app/src/behaviors/behavior_combo.c Outdated Show resolved Hide resolved
app/dts/bindings/zmk,combo.yaml Outdated Show resolved Hide resolved
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.

Let's do this! Plenty of end user testing on this, code is reasonable architecture and self contained, and can easily be put behind one Kconfig flag later as needed.

@petejohanson petejohanson merged commit feb0d5b into zmkfirmware:main Jan 14, 2021
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

3 participants