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(beahviors): Allow encoders to use any behavior #1275

Closed

Conversation

nickconway
Copy link
Contributor

This PR adds the sensor-rotate behavior, which will allow for encoders to use any behavior in addition to basic keypresses.

In the documentation I have assumed this will deprecate the existing &inc_dec_kp behavior.

This has not been tested extensively but is confirmed to be working with RGB behaviors.

@eXsoR65
Copy link

eXsoR65 commented Apr 28, 2022

Working with rgb behavior:

IMG_3420.mov

Copy link
Contributor

@kurtis-lew kurtis-lew left a comment

Choose a reason for hiding this comment

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

Looking good so far! I'm currently unable to get this feature working on my local hardware, but in the meantime I have a few comments.

app/dts/bindings/behaviors/zmk,behavior-sensor-rotate.yaml Outdated Show resolved Hide resolved
app/src/behaviors/behavior_sensor_rotate.c Outdated Show resolved Hide resolved
docs/docs/behaviors/sensor-rotate.md Outdated Show resolved Hide resolved
docs/docs/features/encoders.md Outdated Show resolved Hide resolved
@kurtis-lew
Copy link
Contributor

kurtis-lew commented Apr 28, 2022

image

Another thing that I'm picking up when I build firmware locally using this PR is the error in the image above. In my case, when I rotate my encoder clockwise or counterclockwise, I'm unable to get the correct behavior binding to register. Perhaps the incompatible pointer types may be the reason for this issue. I'll continue to debug things from my end and see if I can confirm this.

Edit: Can confirm that the updated sensor-rotate behavior works with &kp as a binding. Just had to rename my encoder on my test board. Thanks for this contribution!

@eXsoR65
Copy link

eXsoR65 commented Apr 28, 2022

image

Another thing that I'm picking up when I build firmware locally using this PR is the error in the image above. In my case, when I rotate my encoder clockwise or counterclockwise, I'm unable to get the correct behavior binding to register. Perhaps the incompatible pointer types may be the reason for this issue. I'll continue to debug things from my end and see if I can confirm this.

I too get the same warning during the build process though Its still working.
https://github.com/eXsoR65/bdn9_zmk_config/runs/6198384676?check_suite_focus=true#step:9:65

Here my config: https://github.com/eXsoR65/bdn9_zmk_config

Copy link
Contributor

@kurtis-lew kurtis-lew left a comment

Choose a reason for hiding this comment

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

Did another pass through this PR to make some more minor suggestions. Thanks for the updates based on the first review!

docs/docs/features/encoders.md Outdated Show resolved Hide resolved
docs/docs/behaviors/sensor-rotate.md Outdated Show resolved Hide resolved
docs/docs/behaviors/sensor-rotate.md Outdated Show resolved Hide resolved
docs/docs/behaviors/sensor-rotate.md 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.c 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.

Thanks for working on this! A few minor comments, and one higher level change we might want to roll into this.

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.c Outdated Show resolved Hide resolved
docs/docs/features/encoders.md Outdated Show resolved Hide resolved
@nickconway nickconway force-pushed the configurable-sensor-bindings branch 6 times, most recently from 733da73 to cb7da72 Compare May 18, 2022 21:49
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.

Comment on the behavior queue usage, naming tweaks.

I don't love the code duplication between the parameterized version and not, but the amount duplicated is so minimal, I'd say it's fine.

app/src/behaviors/behavior_sensor_rotate_var.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_sensor_rotate_var.c Outdated Show resolved Hide resolved
app/dts/bindings/behaviors/zmk,behavior-sensor-rotate.yaml 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.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_sensor_rotate_var.c Outdated Show resolved Hide resolved
@nickconway nickconway force-pushed the configurable-sensor-bindings branch 3 times, most recently from c4cbbaf to e7fd119 Compare May 23, 2022 20:33
@nickconway
Copy link
Contributor Author

I don't love the code duplication between the parameterized version and not, but the amount duplicated is so minimal, I'd say it's fine.

We could the duplicated code to another file that gets included in each 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 more thought...

Comment on lines +58 to +61
zmk_behavior_queue_add(0, triggered_binding, true, cfg->tap_ms);
zmk_behavior_queue_add(0, triggered_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.

Instead of simulating this at the zeroth key position, perhaps better to simulate it at "max key position + sensor position" so there are actually unique? May be necessary for some code that tracks state tied to key position (or might actually break them.... hmmm...)

Copy link
Contributor

Choose a reason for hiding this comment

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

This got fixed in the non-var version, but not this one.

@nickconway nickconway force-pushed the configurable-sensor-bindings branch from 848a15e to 507fdcf Compare June 16, 2022 05:23
@ReFil
Copy link
Contributor

ReFil commented Jun 22, 2022

Excited to see this as it will make the defeault layer shifting stuff i'vebeen working on much neater. Iimplemented a rudimentary sensor behaviour but universall sensor beaviours are even better

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.

Couple minor things. Want to give this a test before merging though.

Thanks for the work on this!

app/src/keymap.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
@petejohanson petejohanson self-assigned this Oct 27, 2022
@petejohanson
Copy link
Contributor

@nickconway can you please rebase to pull in the docs build fixes?

@nickconway
Copy link
Contributor Author

@petejohanson done.

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 minor fixes needed.

Also, we probably need to remove app/dts/bindings/behaviors/zmk,behavior-sensor-rotate-key-press.yaml in this PR as well as the driver code itself.

Comment on lines +13 to +16
### Configuration

An example implementation of an encoder that changes RGB brightness is shown below:

Copy link
Contributor

Choose a reason for hiding this comment

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

We need an example for the -var and non-var variants here.

Comment on lines +58 to +61
zmk_behavior_queue_add(0, triggered_binding, true, cfg->tap_ms);
zmk_behavior_queue_add(0, triggered_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 got fixed in the non-var version, but not this one.


LOG_DBG("Sensor binding: %s", log_strdup(binding->behavior_dev));

zmk_behavior_queue_add(ZMK_KEYMAP_LEN + event.position, *triggered_binding, true, cfg->tap_ms);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will overlap with combo positions?

@petejohanson
Copy link
Contributor

Merged in the updated PR #1758

@nickconway nickconway deleted the configurable-sensor-bindings branch June 6, 2023 20:20
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.

None yet

8 participants