-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[bounty] feature(split): add split-side encoder support #1743
Comments
hello @auipga I am coming here from bountysource. Do you intend to get that feature merged by authors of repository or you just want to get that to be the part of the code? I can help for the latter case where I can fork the repo and add the patch from the PR. |
Hello @Amit0617, thank you for being interested in helping. The PR needs to be updated to be ready to get merged. If it isn't merged in the main repo, there is the risk of future incompatibilities. If you can provide a solution that I could just edit the yaml file to use a branch from you, this would be okay for me too. |
Just FYI, I've been actively working on the necessary refactors before #728 can be merged: The planned sequence is:
The reason for the sequence is to be sure the sensor foundations are actually nicely designed to handle the split encoder work, which should end up being once smaller once everything is said and done. |
Please let me know if I can help with this as I'm somewhat familiar with the ZMK codebase and have a SOFLE v2 with dual encoders. |
this is good to see progress. I tried to merge in the PR's in my own fork to get it working, but I think #1039 added enough changes to make it difficult for me to see how to merge #728 for my own uses. Is it possible to also rebase #728 on top of 1039 for those of us that are trying to keep up to date and have this feature enabled ourselves? |
@petejohanson thank you for putting effort in this issue. Test result 1: The right encoder doesn't seem to respond yet. sensor-bindings = <&inc_dec_kp C_VOL_DN C_VOL_UP &inc_dec_kp C_VOL_DN C_VOL_UP>; west.ymlmanifest:
remotes:
- name: zmkfirmware
url-base: https://github.com/zmkfirmware
- name: stephen/split-encoder
url-base: https://github.com/petejohanson
projects:
- name: zmk
remote: stephen/split-encoder
revision: main
import: app/west.yml
self:
path: config hillside52.confCONFIG_EC11=y
CONFIG_EC11_TRIGGER_GLOBAL_THREAD=y |
Can you enable logging and get the log results from both sides? |
TLDR: yes, read the next comment. I followed https://zmk.dev/docs/development/usb-logging and added $ sudo tio /dev/ttyACM0
$ sudo dmesg -HW
I disabled bluetooth and used |
After a reboot it worked. sudo tio /dev/ttyACM0 # left half
sudo tio /dev/ttyACM0 # right half
|
If I am not wrong, this is referring to Pete's main branch, not the PR branch. |
Yeah, that's definitely not right. |
Thanks. west.yml (updated)manifest:
remotes:
- name: zmkfirmware
url-base: https://github.com/zmkfirmware
- name: petejohanson
url-base: https://github.com/petejohanson
projects:
- name: zmk
remote: petejohanson
revision: stephen/split-encoder
import: app/west.yml
self:
path: config Now playing with the |
As I don't use ec11 but this ec12 and I don't know about the differences in this regard I hope it's still applicable. The encoder I use has 12 detents and it used to work well with These are the results of 12 'clicks', separated by a new line: left half, clockwise:
left half, anti-clockwise:
right half, clockwise:
right half, anti-clockwise:
|
That should, in theory, work. Can you get the logs for each scenario as well? |
sudo tio /dev/ttyACM0
|
Result 3: The previous misbehaviour of ~4 keystrokes isn't present anymore. What I experience now is that when I |
Result 4:
|
Result 5: SUCCESS
Thank you. It's soooo good. Using my Hillside52 is much more productive now. Well done @petejohanson. You made me very happy! |
@petejohanson claim your bounty |
Please add support for encoders on the peripheral side.
There is already a PR here
Edit: That's why I needed an /issue/url -> https://app.bountysource.com/issues/118967332
Hopefully others will join and bring this feature to life.
The text was updated successfully, but these errors were encountered: