qmk_firmware icon indicating copy to clipboard operation
qmk_firmware copied to clipboard

Add ps2_mouse pointing device

Open joh opened this issue 2 years ago • 5 comments

Description

WIP for migrating ps2_mouse to the pointing device framework.

  • Migrate ps2_mouse to pointing device (POINTING_DEVICE_DRIVER=ps2_mouse). This effectively removes PS2_MOUSE_ENABLE. Many PS2_MOUSE features have been replaced by pointing device equivalents (see table below).
  • The pointing device code still lives in drivers/ps2/ps2_mouse.[ch].
  • The ps2_mouse_pointing_device_{get,set}_cpi() implementations use counts/mm instead of counts/inch, since this is the unit employed by the PS/2 mouse protocol.

The code has been tested on RP2040 with PS2_DRIVER=vendor and an SK8707 trackpoint.

To be completed

  • [x] Refactor ps2_mouse code
  • [x] Port ps2_mouse keyboards over to pointing device
  • [ ] Update documentation, add migration guide
  • [ ] Test on the other PS/2 implementations(?)
  • [x] Compile a list of keyboards using PS2_MOUSE and what features are in use.
  • [x] Compile a list of which PS2_MOUSE features have pointing device replacements, and which features need to be retained.

Keyboards using PS2_MOUSE feature

From discussions on Discord, there seems to be a consensus to move PS2_MOUSE over to pointing device, since the code is very much overlapping. A first step is then to compile a list of keyboards currently using PS2_MOUSE, to get an overview over where and how the feature is used.

A simple git grep PS2_MOUSE_ENABLE keyboards/ reveals that there are only a handful of keyboards in the qmk codebase using the feature. Below I have compiled a list of them, and noted which features are in use and why.

buzzard/rev1:

  PS2_DRIVER = interrupt
  PS2_MOUSE_ROTATE 270
  keymap default,crehmann:
    PS2_MOUSE_SCROLL_BTN_MASK (1<<PS2_MOUSE_BTN_RIGHT)
    PS2_MOUSE_SCROLL_BTN_SEND 500

evyd13/gh80_3700:

  keymap ps2:
    PS2_DRIVER = usart
    PS2_MOUSE_ENABLE_SCROLLING
    PS2_MOUSE_INIT_DELAY 1000
    PS2_MOUSE_X_MULTIPLIER 2
    PS2_MOUSE_Y_MULTIPLIER 2
    PS2_MOUSE_V_MULTIPLIER 1
    PS2_MOUSE_SCROLL_BTN_MASK 0
    PS2_MOUSE_USE_REMOTE_MODE

frobiac/blackbowl:

  PS2_DRIVER = usart
  PS2_MOUSE_USE_REMOTE_MODE
  PS2_MOUSE_INIT_DELAY 1000

handwired/108key_trackpoint: PS2_DRIVER = usart

handwired/promethium:

  PS2_DRIVER = interrupt
  PS2_MOUSE_INIT_DELAY 2000
  keymap default,priyadi:
    ps2_mouse_init_user() to set trackpoint settings
    PS2_MOUSE_SEND()
    ps2_host_send()
    ps2_host_recv_response()

handwired/trackpoint: PS2_DRIVER = usart

pierce:

  PS2_DRIVER = usart
  keymap durken1:
    ps2_mouse_moved_user() for implementing AUTO_BUTTONS (auto mouse layer)

PS2_MOUSE features and pointing device equivalents:

A lot of the PS2_MOUSE features listed above have pointing device equivalents, but not all.

PS2_MOUSE pointing device
PS2_MOUSE_SCROLL_MASK none
PS2_MOUSE_ENABLE_SCROLLING none
PS2_MOUSE_USE_2_1_SCALING none
PS2_MOUSE_INIT_DELAY pointing_device_init_user()?
PS2_MOUSE_[XYV]_MULTIPLIER pointing_device_set_cpi()?
PS2_MOUSE_SCROLL_DIVISOR_[HV] Only used for PS2_MOUSE_SCROLL_BTN_*
PS2_MOUSE_INVERT_BUTTONS none?
PS2_MOUSE_INVERT_[XY] POINTING_DEVICE_INVERT_[XY]
PS2_MOUSE_INVERT_[HV] Only used for PS2_MOUSE_SCROLL_BTN_*
PS2_MOUSE_ROTATE POINTING_DEVICE_ROTATION_*
PS2_MOUSE_SCROLL_BTN_* pointing_device_task_user(), example in docs
PS2_MOUSE_SEND none
PS2_MOUSE_USE_REMOTE_MODE none
PS2_MOUSE_DEBUG_{RAW,HID} POINTING_DEVICE_DEBUG
ps2_mouse_init_user() pointing_device_init_user()
ps2_mouse_moved_user() pointing_device_task_user()
ps2_mouse_set_resolution() pointing_device_set_cpi()
ps2_mouse_{enable,disable}_data_reporting() none
ps2_mouse_set_{remote,stream}_mode() none
ps2_mouse_set_scaling_{1_1,2_1}() none
ps2_mouse_set_sample_rate() none?

Types of Changes

  • [x] Core
  • [ ] Bugfix
  • [x] New feature
  • [ ] Enhancement/optimization
  • [ ] Keyboard (addition or update)
  • [ ] Keymap/layout/userspace (addition or update)
  • [x] Documentation

Checklist

  • [x] My code follows the code style of this project: C, Python
  • [ ] I have read the PR Checklist document and have made the appropriate changes.
  • [x] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [x] I have read the CONTRIBUTING document.
  • [ ] I have added tests to cover my changes.
  • [x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

joh avatar Nov 22 '23 21:11 joh

I'm so excited to see this PR. Thanks for pushing this along @joh! Svalboard isn't in the QMK codebase yet, but uses PS2 for trackpoint with similar feature utilization as buzzard, and I'd love to be able to handle left/right installs or dual installs easily. Anything that gets it out of orphan status and into pointing devices would be a mitzvah in my eyes even with the loss of some specialized PS/2 feature-fu.

From my experience with Trackpoint, the ability to change the sensitivity registers seems like the most critical tuning parameter I'd like to somehow retain, whatever the method -- since the native scaling in the trackpoint itself operates before too much other gain is applied.

morganvenable avatar Nov 29 '23 23:11 morganvenable

Thank you for your contribution! This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready. For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

github-actions[bot] avatar Jan 28 '24 01:01 github-actions[bot]

Status: waiting for review :)

joh avatar Jan 28 '24 18:01 joh

Thank you for your contribution! This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready. For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

github-actions[bot] avatar Mar 23 '24 01:03 github-actions[bot]

Status: waiting for review

joh avatar Apr 01 '24 08:04 joh

The code migration should now be complete. PR Lint complains about missing licenses for some keyboards that were ported over to pointing device.

I've been using this as my daily driver for some time, no issues on my hardware (RP2040 + SK8707 trackpoint module) . The remaining work is documentation and perhaps testing on additional hardware. It would be great to get the code reviewed before proceeding with the docs :)

joh avatar Aug 23 '24 20:08 joh

I have now migrated and reworked the documentation, so this should be good to go for review. Also tested successfully on ATmega32U4 with USART.

joh avatar Dec 21 '24 23:12 joh

Can you make the linters happy?

bosd avatar Mar 02 '25 09:03 bosd

Can you make the linters happy?

Linters are happy as far as I can see. Some unrelated complaints about missing license headers in some keyboard files, and one false positive related to indentation.

joh avatar Mar 02 '25 10:03 joh

Hmm, now it looks like the linter changed its mind?

Run git diff
diff --git a/drivers/sensors/ps2_mouse.c b/drivers/sensors/ps2_mouse.c
index ca78d2fe76..9aefca3e89 100644
--- a/drivers/sensors/ps2_mouse.c
+++ b/drivers/sensors/ps2_mouse.c
@@ -97,7 +97,7 @@ report_mouse_t ps2_mouse_get_report(report_mouse_t mouse_report) {
         ps2_report.x      = ps2_host_recv_response();
         ps2_report.y      = ps2_host_recv_response();
 #    ifdef PS2_MOUSE_ENABLE_SCROLLING
-        ps2_report.z = ps2_host_recv_response();
+        ps2_report.z      = ps2_host_recv_response();
 #    endif
     }
 #endif
File 'drivers/sensors/ps2_mouse.c' Requires Formatting
Error: Requires Formatting
Error: Process completed with exit code 1.

And the build failures are strange, as they passed previously, and the only change now is indentation. I'm unable to reproduce them locally :thinking:

joh avatar May 07 '25 11:05 joh