-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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(mouse): Added mouse emulation #778
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, looks good! I've added a few minor comments.
Maybe we should consider moving the mouse HID stuff into it's own file from hid.c.
app/src/hid_listener.c
Outdated
@@ -78,9 +132,19 @@ int hid_listener(const zmk_event_t *eh) { | |||
} else { | |||
hid_listener_keycode_released(ev); | |||
} | |||
} else { | |||
const struct zmk_mouse_state_changed *ev = as_zmk_mouse_state_changed(eh); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not big fan of this shadowing of ev
. Consider
struct zmk_keycode_state_changed *ev;
ev = as_zmk_keycode_state_changed(eh);
if (ev) {
...
return 0;
}
ev = as_zmk_mouse_state_changed(eh);
if (ev) {
...
return 0;
}
return 0;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't take the fact the events have different types into account. I've implemented the refactor a bit differently, is it better now?
app/include/dt-bindings/zmk/mouse.h
Outdated
|
||
/* Mouse move behavior */ | ||
|
||
#define MOVE_UP (0x0000FFFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should make the MOVE_UP take an argument to control the speed (e.g. MOVE_UP(100), MOVE_LEFT(50), etc)
Later we can make something that interprets/translates this speed based on an acceleration curve, but I think that doesn't have to be part of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also a MOUSE_MOVE(HORIZONTAL, VERTICAL)
macro which can combine a horizontal and vertical move in one go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about these, I initially thought we'd have to decide between one-way (horizontal-only or vertical-only) or two-way macros, but there's no reason not to implement both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an upper bound on mouse buttons or is MB8 arbitrarily chosen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
16 buttons, each bit in a uint_16
is basically a flag. MB8 was chosen basically arbitrarily, I suppose we should extend it to MB16 (although we haven't tested buttons 9-16).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added buttons up to MB16, as well as configurable movement and wheel macros. Please pay attention to the comments: we need some testing to determine usable value ranges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested the added mouse buttons, haven't been able to get 9-16 to work (tried adjusting the Usage Maximum in hid.h
, didn't help). Reverted the new macros. Buttons 1-8 work.
app/include/zmk/hid.h
Outdated
/* END COLLECTION */ | ||
HID_MI_COLLECTION_END, | ||
|
||
0x05, 0x01, /* Usage Page (Generic Desktop Ctrls) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there macros for these constants, so it's more readable similar to the rest of the HID report?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be, but I'm not sure the defines are in the files right now. I was planning to add them later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added them in, everything still works.
static int on_keymap_binding_pressed(struct zmk_behavior_binding *binding, | ||
struct zmk_behavior_binding_event event) { | ||
LOG_DBG("position %d keycode 0x%02X", event.position, binding->param1); | ||
int32_t x = (binding->param1 & 0xFF00) >> 8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider defining a macro to do the translation from param to x/y
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tricky part here is I'm not sure how exactly the HID report handles speeds. 0xFFFF
is the smallest possible speed going up, and 0x0001
is the smallest speed for going down, so does the speed increase towards the middle? Of course, I may be stupid and this may be how the uint32
to int32
conversion works: I'll admit I haven't looked into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0xFFFF = -1, have a look at "one's complement": https://www.electronics-tutorials.ws/binary/signed-binary-numbers.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew that, but the whole "converging towards the middle" part didn't click for some reason. I get it now, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented the macros.
app/src/hid.c
Outdated
static int8_t curr_hor = 0; | ||
static int8_t curr_vert = 0; | ||
|
||
#define SET_MOUSE_BUTTONS(butts) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
butts
-> buttons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to the way macros work, that would also impact the buttons
in mouse_report.body.buttons
. This was a bug we faced during development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah right, make those uppercase I'm not sure what the best naming convention is here.BUTTONS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btns would be a better shorthand than butts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented @mcrosson's suggestion.
app/src/hid.c
Outdated
return 0; | ||
} | ||
|
||
#define SET_MOUSE_MOVEMENT(coor_x, coor_y) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coor_x
, coor_y
-> x
, y
, they're named x and y everywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x
and y
elsewhere in the macro would be captured too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I'm not sure which naming convention is best to use in this case.
### Behavior Binding | ||
|
||
- Reference: `&mmv` | ||
- Parameter: A `uint32` with the first 16 bits relating to horizontal movement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you give the examples with macros, you don't have to explain the bit packing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make a note to refactor the docs once the macro format is settled.
app/include/dt-bindings/zmk/mouse.h
Outdated
|
||
/* Mouse wheel behavior */ | ||
|
||
#define WHEEL_UP (0x0001) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the value of 1 for wheel movement a good default, or should we define it more generally?
#define WHEEL_UP(x) WHEEL_HOR(x)
#define WHEEL_DOWN(x) WHEEL_VER(-x)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the same value your mouse wheel sends.
#define MB3 (0x04) | ||
#define MCLK (MB3) | ||
|
||
#define MB4 (0x08) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MPREV / MNEXT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As aliases? Sure, I'll add these in.
app/src/hid.c
Outdated
LOG_DBG("Mouse movement y set to 0x%02X", mouse_report.body.y); \ | ||
} | ||
|
||
int zmk_hid_mouse_movement_press(int16_t x, int16_t y) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function only returns 0 so it should probably be a void function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the analogous register/unregister mods
shoud be void too? I was going for consistency here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the other functions should be void too. Don't bother updating them in this PR, it's something we'll have to clean up separately.
app/src/hid.c
Outdated
return 0; | ||
} | ||
|
||
int zmk_hid_mouse_movement_release(int16_t x, int16_t y) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function only returns 0 so it should probably be a void function.
Any update on this feature? |
Hi. This is being worked on, but Bluetooth stability proved to be a difficult hurdle to clear. There's an experimental branch with further improvements (acceleration, less crashing, etc.), you can find it in the ZMK Discord's |
Just force-pushed the results of the last few months of work. Acceleration is fully implemented (at least as a first pass), and mouse movement over Bluetooth is much better, even usable by some reports. |
Hot to setup the pointer move speed, like KC_ACL0, KC_ACL1, KC_ACL2 in QMK? |
If you look at the changes, there's a documentation bit about acceleration there. Here's a direct link: |
I am new to zmk. I wonder how I can change the acceleration config by pressing different acceleration keys. |
&mmv {
time-to-max-speed-ms = <10>;
acceleration-exponent = <1>;
};
&mwh {
time-to-max-speed-ms = <10>;
acceleration-exponent = <1>;
};
/ { The pointer move and scroll speed is too slow with above config. I tested different acceleration-exponent(0 to 2 even 1000), they all goes too slow for me. Question:
|
After reading this pr, I find a workaround for my needs: I define some faster MOVE keys in the dt-bindgs file #define MOVE_UP1 MOVE_VERT(-1400)
#define MOVE_DOWN1 MOVE_VERT(1400)
#define MOVE_LEFT1 MOVE_HOR(-1400)
#define MOVE_RIGHT1 MOVE_HOR(1400) and use them in my .keymap file. |
That's not really a workaround, it's the intended way of setting maximum speeds. |
I think there should be a Currently I can not do something like:
I put some faster movement in layer 1, and want to use this way to do some workaround of tap-dance, e.g. tap to send LCLK key, hold to access faster mouse movement key. |
Seems there is a small bug, when I turn-off my right side of corne-wireless keyboard(nice!nano), the mouse movement becomes laggy. |
Maybe it should be put in the docs (but put defines possibly in the keymap file probably), as I also struggled how to do that. It was the first thing I wanted to change. First I tried looking for an option like But I tested and works like a charm, a life saver for me. Using with mac-os, on a nice-nano v2, default speed was waay to small. And a lifesaver as I try to not get my hands away from the keyboard. The smoothness had issues only with general connection issues (the key presses had also issues), it's super smooth when I sorted it out by setting bt power with |
Here is a proposal to drive this pull request to completion: Divide these changes into easier to review pull requests with someone taking charge of each one. Each pull request will have full functionality and may be reviewed independently. I propose the following four tasks but feedback is very welcome. Each of these tasks should be something that can be completed, tested, and reviewed on its own. This is made easier since @krikun98 has done most of the work, I think we just need people to commit to owning each task and driving it though code review and testing.
The full change set is over 1300 lines so we may want to cut things down even more. Each pull request may still depend on the changes from a previous pull request, but the development and review does not need to be blocked (If you search for "stacked pull requests" there are a fair number of articles on how to do this). I can take one of these pull requests if there are other people willing to pick up the other three. I am also willing to help anyone working on the other two or review pull requests. Unfortunately, I don't personally have the time to commit to driving the whole thing. @petejohanson (cc @krikun98): What are your thoughts on this? Does it sound reasonable? @DelusionalLogic , @gcgbarbosa, or anyone else: would you be willing to pick up one of the four tasks I listed? |
Hi @ftc, I can definitely work on the documentation... I don't think I'd have time to do 3, but 1 and 2 are also ok with me... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not found any bugs testing this PR
@mrlinuxfish I have not either so if you and the other reviewers decide to just merge it, I have no objections. However, you may want to use my rebased version. There is not a ton of work on top of what is already here, but I did fix a number of merge conflicts and update the HID code to be compatible with the new version of Zephyr. https://github.com/ftc/zmk/tree/mouse-ftc Please let me know if you would like me to open a pull request from my branch. |
Give the scope of this, I definitely think this is warranted. In particular, I know that for #2 and #3, @krikun98 had mentioned wanting to improve the timing/acceleration system to be smoother, so keeping this queued while we get some core in with item 1 would be good.
Agreed.
To be clear, I'd expect this to include some of the necessary core HID changes as well.
I believe both 2 and 3 will likely need some shared acceleration logic, we might want to have that be in a "Step 1b" piece independently.
Indeed.
Very reasonable. I would love for @krikun98 to weigh in, if possible, since this is definitely his baby. |
I totally agree here I want to make sure what I am proposing is aligned with @krikun98 's wishes. Hopefully my suggestion gives @krikun98 more leeway to work on the parts that excite him and offload some of the more boring tasks to the rest of us.
Factoring out an acceleration pull request seems like a really good idea.
My hope is that factoring out an interface for the accelerator could also be helpful here. I think getting exactly the right acceleration is going to be an ongoing task. A clean interface between the components could further unblock progress towards making this a main branch feature. The next improvement on acceleration could be pull request number 5 as long as @krikun98 is on board with the interface changes.
I think that is totally reasonable. This HID code also seems to be used by some of the other trackpad and trackball work. I am happy to take the first task with the HID stuff since I had to figure that out to rebase the pull request anyway. |
A squashed version of the original changeset from zmkfirmware#778 rebased on main
I went ahead and factored out the mouse button stuff from the mouse move/scroll stuff. I hope that will help drive the review process forwards. |
Squashed commit of the following: commit ee855f4 Author: Alexander Krikun <[email protected]> Date: Thu Sep 23 22:00:07 2021 +0300 prettier commit 2bf5e44 Author: Alexander Krikun <[email protected]> Date: Thu Sep 23 21:31:41 2021 +0300 clang-format commit ec85b7a Author: Alexander Krikun <[email protected]> Date: Thu Sep 23 21:30:43 2021 +0300 Moved tick duration commit cbce1db Author: Alexander Krikun <[email protected]> Date: Thu Sep 23 21:04:47 2021 +0300 Added documentation for new features commit 8088585 Author: Alexander Krikun <[email protected]> Date: Wed Sep 22 00:27:52 2021 +0300 Send mouse messages from dedicated thread commit 992bbc0 Author: krikun98 <[email protected]> Date: Thu Sep 2 04:09:07 2021 +0300 Cleanup and acceleration fixes commit 0bcd44a Author: krikun98 <[email protected]> Date: Tue Aug 31 01:49:28 2021 +0300 Add messages to BLE queue without a waiting interval commit 96660dc Author: krikun98 <[email protected]> Date: Fri Aug 27 03:19:57 2021 +0300 Simplified tick rate and made it configurable commit eb089b5 Author: krikun98 <[email protected]> Date: Fri Aug 27 01:31:30 2021 +0300 Added dedicated mouse work queue option commit 8fc5962 Author: Okke Formsma <[email protected]> Date: Fri May 14 20:32:14 2021 +0200 feat(mouse keys): add events, smoothing and acceleration commit 728f42e Author: krikun98 <[email protected]> Date: Sat May 15 00:42:29 2021 +0300 Modified mouse_timer_unref to account for errors commit b4ec49e Author: Okke Formsma <[email protected]> Date: Fri May 14 13:29:55 2021 +0200 Simplify binary arithmetic commit 4d08f97 Author: krikun98 <[email protected]> Date: Mon May 3 12:32:36 2021 +0300 Report refactor (added macros) commit 359f35b Author: krikun98 <[email protected]> Date: Mon May 3 11:48:19 2021 +0300 Reverted mouse buttons 9-16 commit 6de29af Author: krikun98 <[email protected]> Date: Mon May 3 11:14:00 2021 +0300 Mouse movement coordinate signedness consistency commit 9957b28 Author: krikun98 <[email protected]> Date: Mon May 3 11:13:33 2021 +0300 Review edits: macro, event override fix, cosmetics commit fddadb9 Author: krikun98 <[email protected]> Date: Mon May 3 09:54:35 2021 +0300 Added new mouse movement macros commit 0893c76 Author: krikun98 <[email protected]> Date: Sun May 2 18:36:09 2021 +0300 Add the doc page to the sidebar commit 571e457 Author: krikun98 <[email protected]> Date: Sun May 2 17:08:12 2021 +0300 Documentation refactor commit 58178a7 Author: krikun98 <[email protected]> Date: Sun May 2 16:47:30 2021 +0300 Raised BLE mouse report queue size commit 345ef42 Author: Dmitry Tsykunov <[email protected]> Date: Sun May 2 16:18:41 2021 +0300 Implemented Rinh's suggestion to remove deadlocks commit 141a525 Author: krikun98 <[email protected]> Date: Sun May 2 13:52:23 2021 +0300 clang-format commit a339a9a Author: krikun98 <[email protected]> Date: Sun May 2 13:30:57 2021 +0300 Cleaning out prototype traces commit e2b97f1 Author: Dmitry Tsykunov <> Date: Sat May 1 09:38:57 2021 +0300 Add mouse behaviour documentation commit ee51487 Author: Dmitry Tsykunov <> Date: Thu Apr 29 18:11:28 2021 +0300 Continuous mouse movement prototype commit dc2e30d Author: Dmitry Tsykunov <> Date: Thu Apr 29 17:52:22 2021 +0300 Add mouse movement event commit 3fb874f Author: Dmitry Tsykunov <> Date: Thu Apr 29 04:49:04 2021 +0300 Mouse-related behaviours commit 016256b Author: Alexander Krikun <[email protected]> Date: Thu Apr 29 11:29:53 2021 +0300 Bluetooth tuning, mouse wheel and movement backend commit 54ac765 Author: Alexander Krikun <[email protected]> Date: Wed Apr 28 15:07:54 2021 +0300 Fine-tuning report, 16 buttons commit b14b024 Author: Alexander Krikun <[email protected]> Date: Tue Apr 27 18:24:11 2021 +0300 Preliminary work for mouse click
FYI, the PR seems to work well with the upcoming Zephyr 3.2 upgrade. Starting with FTC's rebase, all it needs is resolving a few minor merge conflicts and updating the headers to the new include paths. I am keeping a branch around with those changes at https://github.com/urob/zmk/tree/mouse-3.2. Currently it merges smoothly into Pete's upgrade branch for #1499 (petejohanson:zephyr/3.1-upgrade). I am trying to keep up with Pete's force-pushes but in case anyone wants to try and it doesn't merge smoothly let me know and I'll push another rebase. NB: I also have a experimental |
A squashed version of the original changeset from zmkfirmware#778 rebased on main
A squashed version of the original changeset from zmkfirmware#778 rebased on main
A squashed version of the original changeset from zmkfirmware#778 rebased on main Remove everything not required for buttons I've stripped out everything not strictly required for mouse button HID support. This include dropping the worker thread completely, shaving down the USB HID descriptor. Flattening the mouse/ source directory structure and removing a bunch of event handling. I have kept the mouse event handling separate from the other HID event handling since I figured that was a pretty neat split. If that's a bad idea, do tell. I've also added a test case for mouse button emulation, since that was untested before. The changes have been tested on a corne (split) in usb mode. Bindings on both the left and the right side works (with the left side as master).
Can I ask how the merge of this is going? :) |
It seems like the mouse keys stopped working on macOS on Bluetooth. It’s still working over usb. At least one other user is experiencing it here: The old solution of unpairing and re-pairing doesn’t seem to work. I am also having this issue using an old branch that used to work. So I think this might be caused by a macOS update rather than a problem with this PR. Is anyone else experiencing this problem and does anyone have a solution? |
Quick correction: The mouse PR on urob's fork is working fien with macOS Ventura 13.5. My issue was caused, because I had another PR that added BT characteristics on top of this PR in If someone else encounters similar issues, you can check this discussion on the zmk discord: |
A squashed version of the original changeset from zmkfirmware#778 rebased on main
Closing since #2477 is merged. |
First of all, I'd like to thank @dtsykunov - this feature was our hackathon project.
Mouse button clicks, movement and scrolling from your keymap! There's even documentation available! This is still in alpha, though. Further development needs some architectural decisions, and some bugs need to be ironed out.
very(I've tried it, and it's actually usable. A MacOS tester on Discord reports not noticing a difference to USB) choppy. It's smooth at bd108c5, but it has frequent unpredictable deadlocks there, so it's unusable.I have a feeling it should be rewritted toThe rewrite is done, and the usable value range is small enough that I don't think overflow checks are necessary. Still may need testing, though.int16_t
s and overflow checks should be implemented there.I'll add to the list if I can think of anything else. In the meantime, testing and feedback would be very appreciated.