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

digitizer: set physical size #24759

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

digitizer: set physical size #24759

wants to merge 1 commit into from

Conversation

windo
Copy link

@windo windo commented Dec 29, 2024

Fix digitizer cursor movements on platforms that require a physical maximum value.

Description

At least on linux with X11, without a physical size set the cursor does not move. Setting the maximum phyiscal value to anything non-zero seems to work equally well, so setting it to 1024 which I think should imply a ~10 inch physical touchpad size.

I assume this works as-is on other OSs, so it would probably be wise to try to test this to make sure it does not cause a regression on some other platform. I've tested on a an Ubuntu laptop with X11 (broken before change, fixed after the change) and on a Windows laptop and a Chromebook (both working both before and after the change). I don't have an Apple laptop to test on.

Types of Changes

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

Issues Fixed or Closed by This PR

I haven't filed an issue, but happy to add one if you think one is needed.

Checklist

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

@github-actions github-actions bot added the core label Dec 29, 2024
@windo
Copy link
Author

windo commented Dec 29, 2024

@a-chol @fauxpark - as people who might have some expertise, does this look reasonable to you?

@fauxpark
Copy link
Member

fauxpark commented Jan 8, 2025

Might be better to have the physical size configurable; the Microsoft documentation does mention that these tags are necessary.

@windo windo force-pushed the windo branch 2 times, most recently from e4f68a3 to 7c01c4a Compare January 11, 2025 22:39
@windo
Copy link
Author

windo commented Jan 11, 2025

I set the default to be 10 inches and added a DIGITIZER_PHYSICAL_INCHES define that can be overridden (IIUIC, with the exponent of -2 that should be 10000?).

Seems to work equally well at least for me.

Copy link
Member

@fauxpark fauxpark left a comment

Choose a reason for hiding this comment

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

Should also be done for the V-USB code for parity.

tmk_core/protocol/usb_descriptor.h Outdated Show resolved Hide resolved
@windo
Copy link
Author

windo commented Jan 13, 2025

Should also be done for the V-USB code for parity.

I made a similar change to vusb.c. Is this a USB driver for different hardware or should I be able to run that change somehow on my keyboard (ergodox ez) as well?

@fauxpark
Copy link
Member

V-USB is mainly for the ATmega32A and 328(P), which don't have native USB. https://www.obdev.at/products/vusb/index.html

As you can see, the V-USB code in QMK has its own set of USB descriptors, for reasons, whereas LUFA (USB AVR) and ChibiOS (ARM) share the ones housed in usb_descriptor.c.

Copy link
Member

@fauxpark fauxpark left a comment

Choose a reason for hiding this comment

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

Since this define is overridable, it will need some documentation. Also, the reported physical size of the digitizer surface will always be square, as it applies to both the X and Y usages. Maybe they should be separately configurable.

@drashna drashna requested a review from a team January 14, 2025 05:28
At least on Linux with X11 (Ubuntu), without a physical size in the
descriptor the cursor does not move. Setting a maximum phyiscal value to
anything non-zero seems to work equally well, so setting it to 10 inches
for a reasonable default value.

This change makes the digitizer work on my Ubuntu laptop. A Windows
laptop and a Chromebook worked both with and without this change.
@windo
Copy link
Author

windo commented Jan 15, 2025

Since this define is overridable, it will need some documentation. Also, the reported physical size of the digitizer surface will always be square, as it applies to both the X and Y usages. Maybe they should be separately configurable.

I can do that... But I don't know if all of this matters for anything? Does it matter what the physical dimensions are? I know that I needed it to be set to something for the pointer to work on my system and I can see changing the physical maximum changes the resolution that evtest reports. But I'm not sure what it would affect?

Anyway, I've done that for now.

@tzarc tzarc changed the base branch from master to develop January 15, 2025 22:13
@tzarc tzarc requested review from a team and fauxpark January 15, 2025 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants