qmk_firmware icon indicating copy to clipboard operation
qmk_firmware copied to clipboard

[core] Feature: high resolution scrolling and extended scroll reports

Open Alabastard-64 opened this issue 2 years ago • 2 comments

Current version of this PR requires that PR #18218 be merged first

From feature request #17585 and partially based on @fauxpark 's test code, this adds an option to enable high resolution scrolling which allows some apps to have sub line scrolling (down to pixel resolution if desired).

This requires that scrolling sensitivity is adjusted based on what MOUSE_SCROLL_MULTIPLIER is set to. Currently most mainstream applications such as firefox, chrome and discord support the feature and most other applications that let the OS handle scroll input will behave the same as they did before (as long as scroll sensitivity is adjusted properly). Unfortunately there are a few applications that do not handle this well, and the scroll sensitivity adjustment to compensate for hires scrolling can make those apps oversensitive to scroll inputs, this can be mitigated by using a precision toggle when scrolling in these apps or just adapting to the increased sensitivity.

Drag scroll mode included with the pointing modes feature will automatically adjust scroll sensitivity if it detects that hires scrolling has been enabled (which has to be done by the OS by sending a set_report feature request to the keyboard).

Also the option to extend scroll reports to 16 Bit has also been included which I have actually found is pretty useful and reduces code size a bit with pointing modes enabled (PR #18218 ).

Currently only Atmega U4 chip code has been tested and is working with both MOUSE_SHARED_EP = yes/no

USB Code has been ported to vusb.c and ChibiOS/usb_main.c to allow for U2 and ARM chip support but is currently untested so I will leave this in draft until I can confirm from someone it is working.

Description

Types of Changes

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

Issues Fixed or Closed by This PR

  • #17585

Checklist

  • [x] My code follows the code style of this project: C, Python
  • [x] I have read the PR Checklist document and have made the appropriate changes.
  • [x] My change requires a change to the documentation.
  • [x] 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).

Alabastard-64 avatar Jan 08 '23 05:01 Alabastard-64

I can confirm this works really well on an RP2040. Just would like to add as well that the high res scrolling also affects scroll keycodes (wheel up, down, left, right) in that even those mapped to keys/encoder gets affected.

freznel10 avatar Jan 09 '23 14:01 freznel10

Testing this with ploopyco/trackball with a multiplier of 120.

On my scroll wheel, when going from a standstill I have to scroll a lot in one direction before it starts scrolling at all, at which point it will first scroll 30 pixels in that direction after which it will start scrolling one pixel at a time after that until I stop scrolling, after which point it resets back to that initial state.

I used https://cpstest.org/scroll-test.php to see how much it was scrolling. For reference, when I don't have this feature enabled, my scroll values on that site are in increments of 60. I'm also on Linux, unsure if that's relevant.

The same seems to be true for drag scroll as well (tested from manually mapping x/y to h/v in pointing_device_task_user, I tried with PM_MO(PM_DRG) as well but that didn't do anything, my mouse pointer remained active), but it's a bit harder to get it to start scrolling smoothly consistently, but I don't think that's because of any unique problem occuring with drag scroll so much as it's just harder to observe when compared to the scroll wheel which only updates after moved a certain distance rather than with every movement.

As an aside to this bug report, how would one set it up so that it doesn't effect their scroll wheel while still allowing drag scroll to use it?

BanchouBoo avatar Jan 09 '23 17:01 BanchouBoo

This seems like pointing_modes is not working properly for you... pointer should be stationary when a key with PM_MO(PM_DRG) pressed. Did you #define POINTING_DEVICE_MODES_ENABLE somewhere in one of your config.h file's (_currently DD is unsupported for pointing_modes _)?

Ah, I had _ENABLED rather than _ENABLE, pointing modes work now.

Thanks this is helpful! seems that hires scrolling is not working properly for you as I am able to get increments of 1 when hires is enabled using pointing_modes drag scroll will need to review linux implementation (I hate how poorly documented this is on all OS's).

Without changing the divisor, 30 is still the lowest value I'm getting, then it scrolls in increments of 15.

Interesting idea! not sure if it would work as it is unclear when the OS will request hires scrolling settings from the device (the OS sets these settings during USB initialization and then checks them ) but it could be worth testing! setting the resolution_multiplier to zero should work but I have no idea when the best time to re-enable it would be (you would at least need to wait until after the report is sent).

Would it be possible to have it set up so that instead of MOUSE_SCROLL_MULTIPLIER being a macro at compile time, it's a modifiable runtime value?

Alternatively, would it be viable to use pointing_device_task_user to mathematically undo whatever is going on to apply resolution_multiplier?

BanchouBoo avatar Jan 09 '23 21:01 BanchouBoo

Getting some error now when I try to compile

quantum/pointing_device/pointing_device_modes.c:366:13: error: a label can only be part of a statement and a declaration is not a statement
  366 |             uint8_t cur_divisor = pointing_mode.divisor;
      |             ^~~~~~~
quantum/pointing_device/pointing_device_modes.c:367:13: error: expected expression before 'uint8_t'
  367 |             uint8_t drag_multiplier = MAX(MOUSE_SCROLL_MULTIPLIER / cur_divisor, 1);
      |             ^~~~~~~
quantum/pointing_device/pointing_device_modes.c:369:36: error: 'drag_multiplier' undeclared (first use in this function)
  369 |                 pointing_mode.x *= drag_multiplier;
      |                                    ^~~~~~~~~~~~~~~
quantum/pointing_device/pointing_device_modes.c:369:36: note: each undeclared identifier is reported only once for each function it appears in
 [ERRORS]
 |
 |
 |
make[1]: *** [builddefs/common_rules.mk:360: .build/obj_ploopyco_trackball_rev1_005_boo/quantum/pointing_device/pointing_device_modes.o] Error 1
Make finished with errors

BanchouBoo avatar Jan 11 '23 19:01 BanchouBoo

I'm on Void Linux, kernel version 6.1.3

BanchouBoo avatar Jan 12 '23 01:01 BanchouBoo

What do you get for pixel counts when high res scrolling is off?

I get increments of 60 for both my scroll wheel and drag scroll when hi-res scrolling is turned off

Did you also enable MOUSE_SCROLL_EXTENDED_REPORT?

Yes

You mentioned that you are getting the slow down from the "/120" that seems to be happening but not the per pixel scrolling right? (Now that I've tested them side by side I realize that it is pretty obvious when it is working properly but subtle)

With hi-res scrolling turned on, scrolling doesn't do anything for awhile and then jumps 30 units (always exactly 30) all at once, for both the wheel and drag scroll, after that point if I keep scrolling I get what seems to be proper precision if I continue scrolling, and if I stop then I have to break through that barrier again and get that 30 unit jump again, though drag scroll in these conditions still doesn't feel as smooth as it should, but that might be that it's resetting to whatever that base state is more often than the wheel for whatever reason, but the wheel absolutely scrolls pixel by pixel per each scroll tick once I break that barrier.

With the PM_DRAG pointing mode with the default divisor, instead of increments of 1 it's increments of 15. With a divisor of 120 or by mapping the x/y values to h/v in pointing_device_task_user instead of using pointing modes, it's increments of 1.

So, once past whatever that barrier is, I believe it's behaving correctly, it's just the barrier itself that's the problem.

Unsure if it's potentially relevant but probably worth noting, I'm using the pointing_device_task_user callback in my firmware. It's not doing anything to the scrolling values, I'm only using it to apply pointer acceleration, but once the PR is in a buildable state again I can try removing any extra behavior that isn't directly relevant to hi-res scrolling and see if that changes anything.

BanchouBoo avatar Jan 12 '23 04:01 BanchouBoo

Looks like if you use MOUSE_SHARED_EP = no to disable the shared EP:

Compiling: tmk_core/protocol/lufa/lufa.c                                                           tmk_core/protocol/lufa/lufa.c: In function 'EVENT_USB_Device_ControlRequest':
tmk_core/protocol/lufa/lufa.c:463:26: error: 'MOUSE_INTERFACE' undeclared (first use in this function)
                     case MOUSE_INTERFACE:
                          ^
tmk_core/protocol/lufa/lufa.c:463:26: note: each undeclared identifier is reported only once for each function it appears in

drashna avatar Jan 12 '23 20:01 drashna

Unsure if it's potentially relevant but probably worth noting, I'm using the pointing_device_task_user callback in my firmware. It's not doing anything to the scrolling values, I'm only using it to apply pointer acceleration, but once the PR is in a buildable state again I can try removing any extra behavior that isn't directly relevant to hi-res scrolling and see if that changes anything.

Tested to be sure, doesn't change anything. Removed everything except the keymap and the necessary portions of config.h.

BanchouBoo avatar Jan 16 '23 21:01 BanchouBoo

Confirm new HID Descriptor still works on Linux (could use @BanchouBoo 's help here)

Drag scroll with the default divisor is smoother, but beyond that everything is the same, that 30 unit barrier still exists, so while it hasn't gotten any worse, it's still not fixed.

BanchouBoo avatar Jan 17 '23 19:01 BanchouBoo

The older version you tested and the latest behave the same? Increasing the POINTING_DRAG_DIVISOR above the default of 8 decreases the step sizes in scrolling you get but never goes below 15?

Drag scroll with the default divisor is smoother, no longer in increments of 15, but everything else seems the same.

You have step sizes of 60 pixels/s in scrolling when using only POINTING_DEVICE_MODES_ENABLE?

Correct, all scrolling without hi-res scrolling enabled are in increments of 60.

When enabling both MOUSE_SCROLL_HIRES_ENABLE and POINTING_DEVICE_MODES_ENABLE with the default divisor you have step sizes of 30 pixels/s in scrolling?

Not quite. I scroll in one direction, for a bit nothing happens, then it scrolls 30 units at once, then if I keep scrolling after that it is seemingly pixel-precise beyond that point. If I stop scrolling it happens all over again the next time I scroll.

Do you have MOUSE_SHARED_EP = no in rules.mk or usb.shared_endpoint.mouse : false in info.json?(i.e. is your mouse report shared?)

My rules.mk file is empty and the info.json for ploopyco/trackball does not contain anything related to shared_endpoint. The default rules.mk also does not contain an entry for MOUSE_SHARED_EP so I'm unsure what value it would be set to in my mouse. If you'd like me to try it with a particular value (or values), let me know.

What is the polling rate you get when testing on this page here? (I've noticed some latency effects on the scrolling rate at least in windows, should be ~800-1000 Hz by qmk defaults)

Max value I hit was 910, and some additional research indicates that the hardware for the trackball can do 1000.

Sorry for all the questions, and thanks for helping with this!

Don't be sorry! I want to get this working as much as you do, smooth drag scroll is the only pointer feature I have to rely on a software for currently, so if I can get it working in firmware that removes the need to rely on software at all and makes all my customization portable between computers.

BanchouBoo avatar Jan 21 '23 01:01 BanchouBoo

It's a feature in libinput. Works perfectly through software, but only applies to drag scroll.

Not specifically useful or anything but this is the script I use to toggle it on and off.

#!/bin/sh

device="PloopyCo Trackball Mouse"
current_scroll_button=$(xinput --list-props "$device" | grep 'libinput Button Scrolling Button' | head -n1 | cut -f3)
scroll_button=8
current_value=$(xinput --list-props "$device" | grep 'libinput Scroll Method Enabled' | head -n1 | cut -d' ' -f7)
if [ "$current_scroll_button" -ne "$scroll_button" ]; then
    xinput --set-prop "$device" 'libinput Scroll Method Enabled' 0 0 1
    xinput --set-prop "$device" 'libinput Button Scrolling Button' "$scroll_button"
else
    new_value=$((current_value == 0))
    xinput --set-prop "$device" 'libinput Scroll Method Enabled' 0 0 "$new_value"
    xinput --set-prop "$device" 'libinput Button Scrolling Button' "$scroll_button"
fi

BanchouBoo avatar Jan 21 '23 05:01 BanchouBoo

Hi! I used to run this PR. Just updated and now I am getting this error while compiling:

tmk_core/protocol/chibios/usb_main.c:960: error: unterminated #ifdef
  960 | #ifdef MOUSE_ENABLE
      | 
tmk_core/protocol/chibios/usb_main.c:718: error: unterminated #if
  718 | #if defined(MOUSE_ENABLE) && !defined(MOUSE_SHARED_EP)
      | 
tmk_core/protocol/chibios/usb_main.c:716:29: error: expected declaration or statement at end of input
  716 |                             case SHARED_INTERFACE:
      |                             ^~~~
tmk_core/protocol/chibios/usb_main.c:716:29: error: expected declaration or statement at end of input
tmk_core/protocol/chibios/usb_main.c:716:29: error: expected declaration or statement at end of input
tmk_core/protocol/chibios/usb_main.c:716:29: error: expected declaration or statement at end of input
tmk_core/protocol/chibios/usb_main.c:716:29: error: expected declaration or statement at end of input
tmk_core/protocol/chibios/usb_main.c:624:26: warning: unused variable 'dp' [-Wunused-variable]
  624 |     const USBDescriptor *dp;
      |                          ^~
tmk_core/protocol/chibios/usb_main.c: At top level:
tmk_core/protocol/chibios/usb_main.c:73:13: warning: 'keyboard_idle_timer_cb' declared 'static' but never defined [-Wunused-function]
   73 | static void keyboard_idle_timer_cb(struct ch_virtual_timer *, void *arg);
      |             ^~~~~~~~~~~~~~~~~~~~~~
tmk_core/protocol/chibios/usb_main.c:623:13: warning: 'usb_request_hook_cb' defined but not used [-Wunused-function]
  623 | static bool usb_request_hook_cb(USBDriver *usbp) {
      |             ^~~~~~~~~~~~~~~~~~~
tmk_core/protocol/chibios/usb_main.c:615:13: warning: 'set_multiplier_cb' defined but not used [-Wunused-function]
  615 | static void set_multiplier_cb(USBDriver *usbp) {
      |             ^~~~~~~~~~~~~~~~~
tmk_core/protocol/chibios/usb_main.c:603:13: warning: 'set_led_transfer_cb' defined but not used [-Wunused-function]
  603 | static void set_led_transfer_cb(USBDriver *usbp) {
      |             ^~~~~~~~~~~~~~~~~~~
tmk_core/protocol/chibios/usb_main.c:580:17: warning: 'get_hword' defined but not used [-Wunused-function]
  580 | static uint16_t get_hword(uint8_t *p) {
      |                 ^~~~~~~~~
tmk_core/protocol/chibios/usb_main.c:503:13: warning: 'usb_event_cb' defined but not used [-Wunused-function]
  503 | static void usb_event_cb(USBDriver *usbp, usbevent_t event) {
      |             ^~~~~~~~~~~~
tmk_core/protocol/chibios/usb_main.c:120:29: warning: 'usb_get_descriptor_cb' defined but not used [-Wunused-function]
  120 | static const USBDescriptor *usb_get_descriptor_cb(USBDriver *usbp, uint8_t dtype, uint8_t dindex, uint16_t wIndex) {
      |                             ^~~~~~~~~~~~~~~~~~~~~
tmk_core/protocol/chibios/usb_main.c:71:24: warning: 'keyboard_idle_timer' defined but not used [-Wunused-variable]
   71 | static virtual_timer_t keyboard_idle_timer;
      |                        ^~~~~~~~~~~~~~~~~~~
 [ERRORStmk_core/protocol/chibios/usb_main.c:960: error: unterminated #ifdef
  960 | #ifdef MOUSE_ENABLE
      | 
tmk_core/protocol/chibios/usb_main.c:718: error: unterminated #if
  718 | #if defined(MOUSE_ENABLE) && !defined(MOUSE_SHARED_EP)
      | 
tmk_core/protocol/chibios/usb_main.c:716:29: error: expected declaration or statement at end of input
  716 |                             case SHARED_INTERFACE:
      |                             ^~~~
tmk_core/protocol/chibios/usb_main.c:716:29: error: expected declaration or statement at end of input
tmk_core/protocol/chibios/usb_main.c:716:29: error: expected declaration or statement at end of input
tmk_core/protocol/chibios/usb_main.c:716:29: error: expected declaration or statement at end of input
tmk_core/protocol/chibios/usb_main.c:716:29: error: expected declaration or statement at end of input
tmk_core/protocol/chibios/usb_main.c:624:26: warning: unused variable 'dp' [-Wunused-variable]
  624 |     const USBDescriptor *dp;
      |                          ^~
tmk_core/protocol/chibios/usb_main.c: At top level:
tmk_core/protocol/chibios/usb_main.c:73:13: warning: 'keyboard_idle_timer_cb' declared 'static' but never defined [-Wunused-function]
   73 | static void keyboard_idle_timer_cb(struct ch_virtual_timer *, void *arg);
      |             ^~~~~~~~~~~~~~~~~~~~~~
tmk_core/protocol/chibios/usb_main.c:623:13: warning: 'usb_request_hook_cb' defined but not used [-Wunused-function]
  623 | static bool usb_request_hook_cb(USBDriver *usbp) {
      |             ^~~~~~~~~~~~~~~~~~~
tmk_core/protocol/chibios/usb_main.c:615:13: warning: 'set_multiplier_cb' defined but not used [-Wunused-function]
  615 | static void set_multiplier_cb(USBDriver *usbp) {
      |             ^~~~~~~~~~~~~~~~~
tmk_core/protocol/chibios/usb_main.c:603:13: warning: 'set_led_transfer_cb' defined but not used [-Wunused-function]
  603 | static void set_led_transfer_cb(USBDriver *usbp) {
      |             ^~~~~~~~~~~~~~~~~~~
tmk_core/protocol/chibios/usb_main.c:580:17: warning: 'get_hword' defined but not used [-Wunused-function]
  580 | static uint16_t get_hword(uint8_t *p) {
      |                 ^~~~~~~~~
tmk_core/protocol/chibios/usb_main.c:503:13: warning: 'usb_event_cb' defined but not used [-Wunused-function]
  503 | static void usb_event_cb(USBDriver *usbp, usbevent_t event) {
      |             ^~~~~~~~~~~~
tmk_core/protocol/chibios/usb_main.c:120:29: warning: 'usb_get_descriptor_cb' defined but not used [-Wunused-function]
  120 | static const USBDescriptor *usb_get_descriptor_cb(USBDriver *usbp, uint8_t dtype, uint8_t dindex, uint16_t wIndex) {
      |                             ^~~~~~~~~~~~~~~~~~~~~
tmk_core/protocol/chibios/usb_main.c:71:24: warning: 'keyboard_idle_timer' defined but not used [-Wunused-variable]
   71 | static virtual_timer_t keyboard_idle_timer;
      |                        ^~~~~~~~~~~~~~~~~~~
 [ERRORS]

This is using the unmodified branch from this PR.

MatthiasGrandl avatar Jan 21 '23 09:01 MatthiasGrandl