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

fix(split): Make locality work for nested behaviors #2409

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

Conversation

caksoylar
Copy link
Contributor

@caksoylar caksoylar commented Aug 7, 2024

This is another attempt to solve #1494. My understanding is that #1630 only covers the case of macros invoking global behaviors. I incorporated the changes in it and attempted to address the existing comments.

In addition to the other PR, this should cover sticky keys, hold-taps, mod-morphs and work with source+global locality behaviors. I tested the reset behaviors with hold-taps and tap dances so far.

I am not familiar with these parts of the code; so far I stuck an extra field source wherever I saw a position and passed it around. Specifically, it is a new field in zmk_behavior_binding_event. Any recommendations are welcome on how better to do it.

TODO:

  • Test source local behaviors (reset)
    • macros
    • mod-morphs
    • hold-taps
    • tap dances
    • sticky keys
  • Test global behaviors (backlight)
    • macros
    • mod-morphs
    • hold-taps
    • tap dances
    • sticky keys
  • Handle combos (at least get them working with global behaviors)

@caksoylar caksoylar added core Core functionality/behavior of ZMK behaviors split labels Aug 7, 2024
@caksoylar caksoylar marked this pull request as ready for review August 9, 2024 07:11
@caksoylar caksoylar requested a review from a team as a code owner August 9, 2024 07:11
@caksoylar
Copy link
Contributor Author

caksoylar commented Aug 9, 2024

This is ready for review: I tested all the combinations I can. (And fixed one bug where I learned the purpose of the struct copy the hard way.)

One thing missing is sensor rotate since I don't have hardware with encoders but it is using the behavior queue just like the macros, so I assume it should be fine. I'll ask for testers on Discord just in case.

@Nick-Munnich
Copy link
Contributor

Nick-Munnich commented Aug 9, 2024

Missing a docs adjustment to the new split keyboards page

@caksoylar caksoylar requested a review from a team as a code owner August 9, 2024 18:31
@caksoylar
Copy link
Contributor Author

@Nick-Munnich removed the note, thanks!

One thing missing is sensor rotate since I don't have hardware with encoders but it is using the behavior queue just like the macros, so I assume it should be fine.

This was naive, I looked into it and passing through the source information in the sensor events is not trivial and I don't feel confident enough to do it given I can't even test them. In 4e12392 I hardcoded them to trigger on the central; compared to status quo it should fix using global behaviors like &rgb_ug, at least. I think making source local ones work can be tackled in the future.

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 comments after a first pass. Thanks!

app/include/zmk/behavior.h Show resolved Hide resolved
app/src/behaviors/behavior_hold_tap.c Show resolved Hide resolved
app/src/keymap.c Outdated Show resolved Hide resolved
@caksoylar caksoylar force-pushed the fix/split-locality branch 2 times, most recently from bdc2610 to 0b50828 Compare August 14, 2024 05:10
@caksoylar caksoylar force-pushed the fix/split-locality branch 3 times, most recently from c2564d2 to 2bc4f23 Compare August 18, 2024 05:10
caksoylar added a commit to caksoylar/zmk-tri-state that referenced this pull request Aug 19, 2024
@caksoylar
Copy link
Contributor Author

I got confirmation from a user on Discord that encoders also work with a global behavior now. To sum up, this is the current state after this PR, according to my testing and others'.

Working with global and source-local behaviors:

  • Mod-morphs
  • Hold-taps
  • Macros
  • Tap dances
  • Sticky keys

Working with global behaviors:

  • Combos1
  • Sensors/encoders2

I am not planning to tackle above two for source-local in this PR (see below) so it is complete from my POV.

Footnotes

  1. Making source local work with combos requires deciding the locality of combos from the set of key positions that trigger it, some discussion here

  2. The code change required for this is separate and since I don't have the hardware to iterate on, I think it'd be better to tackle this separately

@petejohanson petejohanson self-assigned this Sep 6, 2024
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 sticking with this! A few thoughts on the implementation.

app/include/zmk/behavior_queue.h Outdated Show resolved Hide resolved
app/include/zmk/split/bluetooth/service.h Show resolved Hide resolved
Comment on lines +20 to +21
#include <zmk/ble.h>
#if ZMK_BLE_IS_CENTRAL
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include <zmk/ble.h>
#if ZMK_BLE_IS_CENTRAL
#if ZMK_BLE_IS_CENTRAL
#include <zmk/ble.h>

Copy link
Contributor Author

@caksoylar caksoylar Sep 7, 2024

Choose a reason for hiding this comment

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

This #include <zmk/ble.h> is actually to get ZMK_BLE_IS_CENTRAL itself (and I believe nothing else, actually).

app/include/zmk/behavior.h Outdated Show resolved Hide resolved
app/src/behavior.c Outdated Show resolved Hide resolved
@caksoylar caksoylar force-pushed the fix/split-locality branch 4 times, most recently from 4ce0453 to 8013892 Compare September 7, 2024 05:31
caksoylar added a commit to caksoylar/zmk-tri-state that referenced this pull request Sep 7, 2024
@caksoylar
Copy link
Contributor Author

Went over the review items, squashed commits to be a bit saner and gave it a drive on my test split to check the previously tested items (no encoder, but everything else).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behaviors core Core functionality/behavior of ZMK split
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants