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): Add reusable sensor behaviors. #1758

Merged

Conversation

petejohanson
Copy link
Contributor

@petejohanson petejohanson commented Apr 17, 2023

  • Add new sensor behaviors that either take full bindings add definition, or accept parameters when bound in the keymap.
  • Remove existing hard-coded key press sensor behavior and instead leverage new generic sensor behaviors to achieve the same functionality.

I've taken the amazing work from @nickconway in #1275 and done the following:

Still on my list is the docs updates I'd noted, but this should be nearly there, and is ready for review.

@petejohanson petejohanson requested a review from joelspadin April 17, 2023 08:11
@petejohanson petejohanson self-assigned this Apr 17, 2023
@petejohanson petejohanson force-pushed the configurable-sensor-bindings branch 3 times, most recently from a3712d6 to 77370f3 Compare April 17, 2023 08:37
Comment on lines -274 to +275
ret = behavior_sensor_keymap_binding_triggered(binding, sensor, position, timestamp);
struct zmk_behavior_binding_event event = {
.position = ZMK_VIRTUAL_KEY_POSITION_SENSOR(sensor_number), .timestamp = timestamp};
ret = behavior_sensor_keymap_binding_triggered(binding, sensor, event);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joelspadin Thoughts on if we should pass behaviors the actual sensor position, then let them generate the virtual key position if they need to queue behaviors?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned on Discord, my main concern with that is that it would be very easy to just pass the sensor position along when queueing behaviors, which could cause very subtle bugs.

app/Kconfig Outdated Show resolved Hide resolved
app/src/behaviors/behavior_sensor_rotate.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_sensor_rotate.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_sensor_rotate_var.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_sensor_rotate.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_sensor_rotate.c Outdated Show resolved Hide resolved

struct behavior_sensor_rotate_var_config {
char *cw_behavior_dev;
char *ccw_behavior_dev;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might be able to share a lot of code between this and the other version if you use the previous file's config struct for both version, but just initialize the params to 0 in this version. Then inside the "binding triggered" function, everything before and after the switch block could be identical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, so moved the whole callback to a "common" code, and added an bool override_params field to the config struct to determine if the parameters get updated or not.

* Add new sensor behaviors that either take full bindings
  add definition, or accept parameters when bound in the
  keymap.
* Remove existing hard-coded key press sensor behavior
  and instead leverage new generic sensor behaviors to
  achieve the same functionality.

Co-authored-by: [email protected]
@petejohanson petejohanson force-pushed the configurable-sensor-bindings branch from ac96787 to 23f8cd6 Compare April 22, 2023 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants