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

feature(split): add split-side encoder support #728

Closed
wants to merge 3 commits into from

Conversation

stephen
Copy link
Contributor

@stephen stephen commented Mar 16, 2021

This PR adds support for encoders on the peripheral side. It adds a new characteristic on the GATT service that the central subscribes to.

I only tested this on my own split Kyria.

@stephen
Copy link
Contributor Author

stephen commented Mar 30, 2021

@petejohanson friendly ping - could you take a look at this when you have some time? i've been happily using it on my kyria split for a couple weeks.

@smshields
Copy link

I can't comment on the code, but I used this branch successfully on my v1 Sofle; haven't had any issues. I do not have oled or underglow so I can't regression test those.

@filoxo
Copy link
Contributor

filoxo commented Apr 21, 2021

I was able to use these changes and have tested that the encoder works on the secondary side of my wireless Kyria (with OLEDs). Everything appears to work as expected. The build container does report some warnings due to this code but the build doesn't fail so maybe that's ok?

Screen Shot 2021-04-20 at 6 14 35 PM

@filoxo
Copy link
Contributor

filoxo commented Apr 21, 2021

I also just noticed that this also worked for having different encoder keys defined per layer. Very nice!

@jamesramsay
Copy link

I've been using without issue on Sofle keyboard - thanks @stephen ❤️

@karnadii
Copy link

I have test it with sofle and worked great. thanks

@rtrajano
Copy link

Didn't work on my BT nice!nano wireless Kyria. I'd test wired but i think it shorts the battery if I attach a TRRS cable.

@edgelesscorner
Copy link

I have tested this for a month+ on Rollow and it works great. I did need to have both halves encoders on different pin out's, other than that everything works as assumed.

@yannickjmt
Copy link

Works great on nice!nano Kyria.
Only comment is that my encoders have a different resolution than default. So I put on my .keymap

&left_encoder { resolution = <2>; };
&right_encoder { resolution = <2>; };

Somehow the &right_encoder parameter was ignored. I had to change the .dtsi file to have desired encoder resolution.

@rtrajano
Copy link

Are you running this on a wireless nice!nano Kyria setup @yannickjmt ?

I wasn't able to get it working on my wireless nice!nano Kyria and I'm wondering if I did something wrong while building it.

Thanks for the encoder resolution tip btw, I was wondering why my left encoder took 2 clicks per key press.

Works great on nice!nano Kyria.
Only comment is that my encoders have a different resolution than default. So I put on my .keymap

&left_encoder { resolution = <2>; };
&right_encoder { resolution = <2>; };

Somehow the &right_encoder parameter was ignored. I had to change the .dtsi file to have desired encoder resolution.

@yannickjmt
Copy link

yannickjmt commented Jun 16, 2021

Are you running this on a wireless nice!nano Kyria setup @yannickjmt ?

I wasn't able to get it working on my wireless nice!nano Kyria and I'm wondering if I did something wrong while building it.

Thanks for the encoder resolution tip btw, I was wondering why my left encoder took 2 clicks per key press.

Yes, be careful to have the proper bindings, like for example

sensor-bindings = <&inc_dec_cp C_VOL_DN C_VOL_UP &inc_dec_cp C_BRI_DN C_BRI_UP>;
sensor-bindings = <&inc_dec_kp PG_UP PG_DN &inc_dec_kp LC(LEFT) LC(RIGHT)>;

I also put

CONFIG_EC11=y
CONFIG_EC11_TRIGGER_GLOBAL_THREAD=y

in kyria_left.conf, kyria_right.conf and app/prj.conf (not sure if all are necessary)

@halfurness
Copy link

halfurness commented Jun 16, 2021

Amazing work @stephen , I've NEARLY got it working on both sides.

OK my bad, wasn't working but I had changed the encoder.status to 'okay' in the kyria.dtsi because I was trying to get them working but that was stopping them from being recognized independently! All working now :D

@yannickjmt
Copy link

@yannickjmt forgive my ignorance but could you explain your comment about the sensor bindings? You have two lines there, each one using either inc_dec_cp or inc_dec_kp but won't this give you 4 different assignments all at the same time?
Have a look at the doc for the difference between kp and cp, otherwise you can go on the discord, not sure what your second problem is.

@halfurness
Copy link

Yeah got it all working thanks!

@karnadii karnadii mentioned this pull request Jul 1, 2021
@jhelvy
Copy link

jhelvy commented Jul 1, 2021

Working great on my Corne!

@truonglong88
Copy link

truonglong88 commented Jul 20, 2021

worked well on my Bluetooth Sofle, please bring it to the main 🙏

@kylegl
Copy link

kylegl commented Jul 20, 2021

I have it working on my Sofle v2.

When I rotate the encoders, they don't pick up every tick. This an issue for anyone else?
@truonglong88

@efyang
Copy link

efyang commented Jul 20, 2021

Working well on my modified ferris sweep! @linkdevk You might have to change your resolution to a lower value, that's what I did and it fixed it.

@truonglong88
Copy link

I have it working on my Sofle v2.

When I rotate the encoders, they don't pick up every tick. This an issue for anyone else?
@truonglong88

Put this into your keymap file, below include codes:

&left_encoder { resolution = <1>; };
&right_encoder { resolution = <1>; };

@freznel10
Copy link

I'm tried using this after the latest update and I can't build .

@kylegl
Copy link

kylegl commented Aug 10, 2021

Did the latest update break it? I know the original developer of this PR is not planning on working on it anymore. RIP dual encoders for now

@freznel10
Copy link

I get this error when i switch to this PR -- Configuring incomplete, errors occurred!
FATAL ERROR: command exited with status 1: 'C:\Program Files\CMake\bin\cmake.EXE' '-DWEST_PYTHON=c:\python39\python.exe' '-BF:\GitHubDesktop\app-2.8.3\zmk\app\build' '-SF:\GitHubDesktop\app-2.8.3\zmk\app' -GNinja -DBOARD=nice_nano -DSHIELD=bone_right

I switch back to main and I could build again

@caksoylar
Copy link
Contributor

I rebased this PR (it was a clean rebase) and pushed it to https://github.com/caksoylar/zmk/tree/split-encoder. I don't have a board with encoders but at least one user on Discord reported it working with the Github Actions workflow using https://zmk.dev/docs/features/beta-testing/#testing-features. I'd imagine that building locally would be no different -- it could be an issue with Zephyr version differences if you switched from main to this branch.

@Necronar
Copy link

Just tested this with a Sofle RGB using nice!nano. Working as intended!

@freznel10
Copy link

Just tested this with a Sofle RGB using nice!nano. Working as intended!

Are you on 2.5?

@Necronar
Copy link

Just tested this with a Sofle RGB using nice!nano. Working as intended!

Are you on 2.5?

Yeah. Just forked it yesterday morning.

@truonglong88
Copy link

Up 😭😭😭

mjrusso added a commit to mjrusso/zmk-config that referenced this pull request Oct 18, 2021
On the left half, I currently have:

https://splitkb.com/products/linear-rotary-encoder

> This encoder actuates thirty times on a full cycle, so one full twist will
> perform thirty actions.

> The resolution of this encoder in QMK is 4, which is the default setting for
> encoders.

And on the right:

https://splitkb.com/products/industrial-rotary-encoder

> This encoder actuates thirty times on a full cycle, so one full twist will
> perform thirty actions.

> The resolution of this encoder in QMK is 2.

According to
https://github.com/zmkfirmware/zmk/blob/01d2102c2326b86b0f87bb008c2a3eb3871e3963/app/boards/shields/kyria/kyria.dtsi#L61-L77,
the default resolutions in ZMK for the Kyria are set to 4 for both halves, but
I'm only seeing 15 actuations per full cycle with the left encoder, so
hopefully this change fixes that. (Perhaps the units differ between ZMK and
QMK.)

Note that I haven't tried the right encoder yet, as it isn't
functional (zmkfirmware/zmk#728 hasn't landed yet).
@link6155
Copy link

Does the fork of infused-kim still work for you guys? I recently made changes to my keymap but now the split-side encoder doesn't work anymore.

Doesn't work for me, the right side with encoder works, but none of the keys on my left side (central) side works

@DiogoDoreto
Copy link

Hey folks, I've rebased infused-kim's branch and updated the shield config for my Waterfowl keyboard. I hope it can be useful to someone else:

https://github.com/DiogoDoreto/zmk/tree/waterfowl-encoders

@schmjop
Copy link

schmjop commented Mar 21, 2023

Hey folks, I've rebased infused-kim's branch and updated the shield config for my Waterfowl keyboard. I hope it can be useful to someone else:

https://github.com/DiogoDoreto/zmk/tree/waterfowl-encoders

Thanks a lot!
Unfortunately my split-side encoder still doesn't work with my Kyria. Does it work with your Waterfowl?

@DiogoDoreto
Copy link

@schmjop yes, all 4 encoders (2 each side) are working fine in Waterfowl within my branch. Maybe check my last commit for how I updated the shield files and check whether the equivalent ones for Kyria are set up properly too.

@JWhitleyWork
Copy link

I have mine working from your branch on a Sofle RGB from Keyhive. However, I noticed that if I set the encoders to status = "okay" in the DTSI file, it doesn't work. I have to specifically set them to disabled in the DTSI and okay in the overlay files.

@DiogoDoreto
Copy link

However, I noticed that if I set the encoders to status = "okay" in the DTSI file, it doesn't work. I have to specifically set them to disabled in the DTSI and okay in the overlay files.

Yes, that's how it's described in the docs and I had to do the same change to have my keyboard working.

https://zmk.dev/docs/development/new-shield#encoders

@FrostKiwi
Copy link

@matuck

@FrostKiwi I was wondering the same thing. I also did some searching in discord. From what I read there @petejohanson is doing some refactoring on sensors. He will not merge this until that is complete. I imagine once the refactor on sensors is done the things done in these will have to have some work done in them as well. I don't mind assisting with any help with code on this once he is done with the sensor refactor so it gets merged in. Until then Im using my own custom repo with fixes from others in for various items. I would like to see this merged eventually.

With #1499 merged and having read Sensor refactors for DTS label changes. as a feature, I inquired.
@petejohanson mentioned on discord

That's coming up, yeah, I have some refactors for saner handling of how sensor data/events are handled, that'll make that PR clearer.
Definitely near the top of my list, it's been long requested, yeah.

In case any specific tasks are required I'm sure people in here are happy to help out...

@matuck
Copy link

matuck commented Apr 6, 2023

@FrostKiwi I did a rebase of the previous work onto the merge. Had to do a few things to get everything to compile but the encoder on my split is not working. I will continue to work on it to see if I can get things running.

@schmjop
Copy link

schmjop commented Apr 12, 2023

@schmjop yes, all 4 encoders (2 each side) are working fine in Waterfowl within my branch. Maybe check my last commit for how I updated the shield files and check whether the equivalent ones for Kyria are set up properly too.

@DiogoDoreto thank you for the advice. I just checked and it already seems to be configured correctly both in your fork and the official ZMK repo:

Any other ideas?

@tklzg
Copy link

tklzg commented May 2, 2023

Rebase work at 2023-04-26 base source.

https://github.com/tklzg/zmk/tree/encoder

If you want to apply the change in the version you are currently using, simply apply infused-kim's patch and if the build is successful, you can just change the two below.

[app/include/zmk/split/bluetooth/uuid.h]
-#define ZMK_SPLIT_BT_CHAR_SENSOR_STATE_UUID ZMK_BT_SPLIT_UUID(0x00000002)
+#define ZMK_SPLIT_BT_CHAR_SENSOR_STATE_UUID ZMK_BT_SPLIT_UUID(0x00000003)

[app/src/split/bluetooth/service.c]
-int err = bt_gatt_notify(NULL, &split_svc.attrs[5], &ev, sizeof(ev));
+int err = bt_gatt_notify(NULL, &split_svc.attrs[7], &ev, sizeof(ev));

@JulianGodd
Copy link

Works on aurora lily58 with nice!nanov2

Thanks for the fix

@petejohanson
Copy link
Contributor

Please everyone test #1841 which is updated and is targeted to merge once code reviewed.

@petejohanson petejohanson force-pushed the stephen/split-encoder branch 2 times, most recently from 0f9ba15 to 4f45ae2 Compare June 28, 2023 16:46
@jaksiri
Copy link

jaksiri commented Jul 2, 2023

I have mine working from your branch on a Sofle RGB from Keyhive. However, I noticed that if I set the encoders to status = "okay" in the DTSI file, it doesn't work. I have to specifically set them to disabled in the DTSI and okay in the overlay files.

Could you please share the zmk-config you used for this? I'm trying to get my Keyhive Sofle RGB to work but my right encoder is still not working.

stephen and others added 3 commits August 27, 2023 14:04
This commit adds a new GATT characteristics on the peripheral side
and wires it up to read sensor values. The central side subscribes
to this new characteristics and replays sensor values on its side.

Co-authored-by: Peter Johanson <peter@peterjohanson.com>
* Don't accept data for the same behavior on multiple layers more than
  once, to avoid duplicate/extraneous triggers.
@petejohanson
Copy link
Contributor

Closing this since the refactored work was merged into main in #1841 just now. Thanks @stephen for the original PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet