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

Pointer Core: HID, mouse keys behavior, etc. #1991

Merged

Conversation

petejohanson
Copy link
Contributor

Taking the amazing starting points in #778 which was then updated in #1430 and trying to get this across the finish line!

Still needs squashing, but getting this up as a draft for others to focus testing on.

In particular, I have addressed the issues with Windows compatibility by:

  • Limiting to 5 buttons, as someone on Discord noted might help,
  • Re-adding X/Y/Wheel data/descriptors; without this Windows is grumpy still.

In addition, this is rebased on latest ZMK main to avoid any conflicts. Now that I've gottem basics working locally building on others' great work, I will start reviewing more deeply with the intent of marking this non-draft, then getting this starting point in.

Testers welcome!

@petejohanson petejohanson added enhancement New feature or request core Core functionality/behavior of ZMK labels Nov 7, 2023
@petejohanson petejohanson self-assigned this Nov 7, 2023
@ReFil
Copy link
Contributor

ReFil commented Nov 7, 2023

Would changing this to allow the x, y and wheel reports to be easily set and sent be within the scope of this PR? I think that would be really handy for pointer device implementation

@petejohanson
Copy link
Contributor Author

Would changing this to allow the x, y and wheel reports to be easily set and sent be within the scope of this PR? I think that would be really handy for pointer device implementation

Given how long the pointer work has stalled, I'm hesitant to increase scope here much more. I'd rather we get the working baseline with mouse keys in, then iterate on the API for x/y/wheel. My main concern is whether we need to handle absolute reporting any time soon, and if so, designing this flexibly from the get go for it.

app/include/zmk/endpoints.h Show resolved Hide resolved
app/src/endpoints.c Show resolved Hide resolved
app/include/zmk/hid.h Show resolved Hide resolved
app/include/zmk/hid.h Outdated Show resolved Hide resolved
app/src/hog.c Outdated Show resolved Hide resolved
@petejohanson petejohanson force-pushed the features/pointer-hid-core branch 2 times, most recently from ca73b7a to 9e8797f Compare November 8, 2023 19:44
@petejohanson petejohanson marked this pull request as ready for review November 8, 2023 19:45
@petejohanson petejohanson requested a review from a team as a code owner November 8, 2023 19:45
@petejohanson petejohanson force-pushed the features/pointer-hid-core branch 3 times, most recently from 625cfb2 to 02777d4 Compare November 8, 2023 20:27
@caksoylar
Copy link
Contributor

@petejohanson Are you intending to update the docs before merging this? I think it would be good to either (a) hide the docs for now, until full mouse keys support is merged (move/scroll included), or (b) note more specifically that this is currently WIP and only supports button presses.

I also don't like linking to the header file for the actually useful information, but we can fix that later.

@petejohanson
Copy link
Contributor Author

@petejohanson Are you intending to update the docs before merging this? I think it would be good to either (a) hide the docs for now, until full mouse keys support is merged (move/scroll included), or (b) note more specifically that this is currently WIP and only supports button presses.

I also don't like linking to the header file for the actually useful information, but we can fix that later.

I haven't had a chance to even look at the docs yet after grabbing the existing work as a starting point. Definitely will review and adjust/remove/update as necessary. Thanks for pointing it out.

@petejohanson petejohanson force-pushed the features/pointer-hid-core branch 2 times, most recently from a6226fd to cb7a548 Compare November 11, 2023 06:36
@petejohanson
Copy link
Contributor Author

@petejohanson Are you intending to update the docs before merging this? I think it would be good to either (a) hide the docs for now, until full mouse keys support is merged (move/scroll included), or (b) note more specifically that this is currently WIP and only supports button presses.

I also don't like linking to the header file for the actually useful information, but we can fix that later.

@caksoylar Took a look and cleaned up the added page a ton. Thoughts?

@caksoylar
Copy link
Contributor

caksoylar commented Nov 11, 2023

@petejohanson Are you intending to update the docs before merging this? I think it would be good to either (a) hide the docs for now, until full mouse keys support is merged (move/scroll included), or (b) note more specifically that this is currently WIP and only supports button presses.
I also don't like linking to the header file for the actually useful information, but we can fix that later.

@caksoylar Took a look and cleaned up the added page a ton. Thoughts?

The wording looks great, but I think we should include a table like below under "Behavior Binding," what do you think?

Define Action
MB1, LCLK Left click
MB2, RCLK Right click
MB3, MCLK Middle click
MB4 Mouse button 4
MB5 Mouse button 5

Mouse buttons 4 and 5 typically map to "back" and "forward" actions in most applications.

@petejohanson petejohanson force-pushed the features/pointer-hid-core branch 3 times, most recently from bd2dd9e to a0af943 Compare November 13, 2023 19:53
@petejohanson
Copy link
Contributor Author

@petejohanson Are you intending to update the docs before merging this? I think it would be good to either (a) hide the docs for now, until full mouse keys support is merged (move/scroll included), or (b) note more specifically that this is currently WIP and only supports button presses.
I also don't like linking to the header file for the actually useful information, but we can fix that later.

@caksoylar Took a look and cleaned up the added page a ton. Thoughts?

The wording looks great, but I think we should include a table like below under "Behavior Binding," what do you think?
Define Action
MB1, LCLK Left click
MB2, RCLK Right click
MB3, MCLK Middle click
MB4 Mouse button 4
MB5 Mouse button 5

Mouse buttons 4 and 5 typically map to "back" and "forward" actions in most applications.

Great call. Updated that section of that page to include that table and the note about mouse buttons 4 and 5.

Copy link
Contributor

@caksoylar caksoylar left a comment

Choose a reason for hiding this comment

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

A couple more comments on the docs side, sorry about the piecemeal review!

As an aside, I haven't had a chance to test it yet but I can try to make some time tonight.

Comment on lines 11 to 22
## Configuration Option

This feature can be enabled via a config option:

```
CONFIG_ZMK_MOUSE=y
```

If you use the mouse key press behavior in your keymap, the feature will automatically be enabled for you.

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
## Configuration Option
This feature can be enabled via a config option:
```
CONFIG_ZMK_MOUSE=y
```
If you use the mouse key press behavior in your keymap, the feature will automatically be enabled for you.

Do you think we should remove this section altogether? It doesn't sound relevant to the end users if it gets automatically enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was considering that.. Only reason I could see it being useful is folks wanting to explicitly disable it for some reason, TBH.

docs/docs/behaviors/mouse-emulation.md Show resolved Hide resolved
@caksoylar
Copy link
Contributor

Tested and working over USB and BLE on a nice!nano split on Windows. I did have to do the https://deploy-preview-1991--zmk.netlify.app/docs/features/bluetooth#windows-connected-but-not-working trick twice to get the keyboard working over BLE after the mouse keys flash; I think I might not have waited enough for BT to turn off the first time.

* Add HID report/descriptor for a new report with mouse buttons,
  and x/y/wheel deltas.
* New mouse key press behavior for press/release of mouse keys.
* Add constants for HID main item values (e.g. data/array/absolute)
* Define and use constants for our HID report IDs.
@petejohanson petejohanson merged commit d7d9eed into zmkfirmware:main Nov 15, 2023
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core functionality/behavior of ZMK enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants