Bug 21129

Sebastian:
Thanks for the patch, it looks good. However, I'm quite hesitant to actually put this into the driver as I think it's the wrong place.

The situation we have now is that each driver has it's own invert/swap axes implementation, with bugs replicated across them. Now, with properties, the situation is even worse in that each driver would then have it's own property to handle that.

This feature is useful, but it should be implemented in the server. If you are willing to do this, please contact me and I'll help you get started.

So what you are proposing is to avoid duplicate code and ensure consistency? Sounds good!

More precisely, you are suggesting that the server adds and handles automatically the swap/invert axis properties of each pointer device? We could follow the interface used in evdev, but I wonder if pointer devices are restricted to 2 axes? If there can be more axes, the swap interface should be extended or modified.

Please, give me some clues about where to start looking in the server. I'm willing to try implementing the generic infrastructure.

dix/getevents.c,  GetPointerEvents, GetKeyboardValuatorEvents,
GetProximityEvents
- this is where the hooks to swap/invert the axes should be.
  beware that these three are called during the signal handler, so you need
  to protect yourself against being half-way stuck in changes to the
  property.

dix/devices.c, DeviceSetProperty and the end of AddInputDevice
- this is an example of a DIX-controlled property, the axis swap/axis
  inversion property would be similar to this.

dix/devices.c, EnableDevice
- in EnableDevice you finally know how many axes etc the device will have,
  so you can finish setting up the property here with the right number of
  booleans.

I think those three spots should be it. I'd be happy to see a boolean invert
property. The "swap" property should rather be an axis mapping property,
where any input axis can be mapped to any output axis under the constraint
that the axis should exist on the device*. So you can say that X maps to Z
as long as the device has 3 axes.

* it'd be nicer to get the general case but that includes some headache to
get the driver to do the right thing.

Created attachment 26167 [details] [review]
generic version of inversion and axis mapping properties

Here is my first attempt of patch, thanks to Petter's advices. Basically, it works. Each device which has axes, now has two more properties to specify inversion of the axes and the mapping between the physical valuators and the pointer axes. 

However, it seems there is a bug somewhere when the mapping is changed: in such case, moving the mouse slowly leads to some non-soft movements. Maybe it is related to acceleration.

Also, I have no idea of what Peter meant by "beware that these three are called during the signal handler, so you need to protect yourself against being half-way stuck in changes to the property." No protection was used. Any clarification would be welcome :-)

Finally, the update of the manpage is missing.

This patch is against server-1.6-branch.

BTW, I've tried to complete the manpage... but realized there is no manpage documenting the generic input properties. Maybe it should go in a "INPUT PROPERTIES" section of X.7 or Xorg.1? Or would there be a better place?

Thanks a lot for the patch. Here's a few comments regarding the style:
The indendation style is inconsistent. Please use spaces, no tabs. Only
exception is if you're changing an existing bit of code that's already
tab-indented. There's one whitespace-only hunk as well (6th from the bottom)
Avoid { } if you only have a single line in a for loop, if condition, etc.

In regards to functionality:

+           if (axis_map[i] >= prop->size)
+               return BadValue;

urgh. compare to numAxes please. also, this does not allow for axes outside
of the numAxes range which would be really useful. The first version of this
patch can be without this, for this feature to work you need XI2.

+    if ((dev->valuator != NULL) && (dev->valuator->numAxes > 0))

second condition is superfluous. In the same code block, split the int ret;
onto a separate line. That way, you get more symmetry with the
XIChangeDeviceProperty calls.

+
+        ret = XIChangeDeviceProperty(dev, XIGetKnownProperty(XI_PROP_MAP_AXES),
+                XA_INTEGER, 8, PropModeReplace, dev->valuator->numAxes,
+                dev->axis_map, FALSE);
+        XISetDevicePropertyDeletable(dev, XIGetKnownProperty(XI_PROP_MAP_AXES), FALSE);

avoid the double-call to XIGetKnownProperty in this hunk, it's fairly expensive

+invertAndMapValuators(DeviceIntPtr pDev, int num_valuators, int *valuators)

this function needs some fixing. It needs to honour first_valuator
and it accesses invalid valuators[i] if there is some higher axis mapping.
first_valuator is the first axis in valuators[], i.e. if a device only
posts the Y axis, first_valuator would be 1, num_valuators 1, and
valuators[0] = y. initAndMapValuators would overflow in exactly that case.

+    /* Axis inversion and mapping */
+    BOOL             invert[MAX_VALUATORS];
+    CARD8            axis_map[MAX_VALUATORS];
+

any reason why you didn't tack this to the end of the struct?

+#define XI_PROP_INVERT_AXES  "Axis Inversion"
+#define XI_PROP_MAP_AXES     "Axis Mapping"

Axis or Axes? (I'm not a native english speaker)

Other than that, I think it's fine though we'd want to get some extensive
testing on that to make sure the master/slave interaction stays correct.
Also, this patch will eventually end up in master, not in 1.6 so you'll have
to rebase it soon.

Updating bug title and component.

Eric, have you had any chance to address the comments in Comment #6? I'd really like to see this land soonish.

I've been very busy with other things, but I've started to fix the comments. Will try to send a new version this weekend.

Created attachment 26992 [details] [review]
generic version of inversion and axis mapping properties

Here is an updated version of the patch.
I'm not entirely sure I've done the right thing with "first_valuator", so you probably want to double check.

It's still against branch 1.6, because it's easier for me to compile and install on my machine. I'll try to update to master later on...

Created attachment 26995 [details] [review]
generic version of inversion and axis mapping properties against master

Here is a new version, against git master. It compiles, but I could not test it yet.

thanks for your work! 
these comments are for the 'master' version of the patch:

the axis map/inversion should be stored in the ValuatorClassRec (dev->valuator) rather than the device. Each axis has an AxisInfoPtr where the inverted bit and the map bit could be stored. This makes it a bit more complicated to check for the mapping etc, but it's the right place to store this bit of data.

+    else if (property == XIGetKnownProperty(XI_PROP_MAP_AXIS))
+    {
+        if (prop->format != 8 || prop->type != XA_INTEGER ||
+            dev->valuator == NULL || prop->size != dev->valuator->numAxes)
+            return BadValue;
+
+        CARD8 *axis_map = (CARD8*)prop->data;

no variable declarations after code please.

+invertAndMapValuators(DeviceIntPtr pDev, int first, int num, int *valuators)

this inversion only works for relative coordinates. for absolute coordinates you need to take min/max into account. formula: inverted = max - (value - min)

the same function is also prone to erroneous values after mapping to a higher axis since it doesn't adjust first_valuator and num_valuators. For example, if axis 2 is mapped to axis 8 and 2 values are passed in, this should be an array size 8 after the mapping, with the data in between filled with zeros (relative) or the current axis value (absolute).

there's also an unneeded hunk with a pure whitespace change in there.

Other than that, I think we're getting there :)

(In reply to comment #11)

> +invertAndMapValuators(DeviceIntPtr pDev, int first, int num, int *valuators)
> 
> this inversion only works for relative coordinates. for absolute coordinates
> you need to take min/max into account. formula: inverted = max - (value - min)
Ok. Do you have a hint for me how to know if the values are going to be used as absolute or relative?

> 
> the same function is also prone to erroneous values after mapping to a higher
> axis since it doesn't adjust first_valuator and num_valuators. For example, if
> axis 2 is mapped to axis 8 and 2 values are passed in, this should be an array
> size 8 after the mapping, with the data in between filled with zeros (relative)
> or the current axis value (absolute).
My original idea was to allow only remapping between existing axes, to avoid this trouble ;-) How should the valuator array be increased? Does it always have enough space (MAX_VALUATOR values allocated for every device), and then it would require just to change first and num_valuators, is a new allocation needed?

(In reply to comment #12)
> (In reply to comment #11)
> 
> > +invertAndMapValuators(DeviceIntPtr pDev, int first, int num, int *valuators)
> > 
> > this inversion only works for relative coordinates. for absolute coordinates
> > you need to take min/max into account. formula: inverted = max - (value - min)
> Ok. Do you have a hint for me how to know if the values are going to be used as
> absolute or relative?

GetPointerEvents takes a flag of POINTER_ABSOLUTE, the axis ranges are in dev->valuator->axes[i].min_value and max_value.

> > the same function is also prone to erroneous values after mapping to a higher
> > axis since it doesn't adjust first_valuator and num_valuators. For example, if
> > axis 2 is mapped to axis 8 and 2 values are passed in, this should be an array
> > size 8 after the mapping, with the data in between filled with zeros (relative)
> > or the current axis value (absolute).
> My original idea was to allow only remapping between existing axes, to avoid
> this trouble ;-) How should the valuator array be increased? Does it always
> have enough space (MAX_VALUATOR values allocated for every device), and then it
> would require just to change first and num_valuators, is a new allocation
> needed?

even if the axes exist on the device, the driver doesn't always pass all axes in at the same time. for example, if only the X axes changed, the array is size 1.
You can't alloc in GetPointerEvents (GPE) and from then on because we're in a signal handler. so you need an array of MAX_VALUATOR on the stack to deal with that. pass that around instead of the original valuators array and you'll be fine.



 

Eric: do you have any updates on this patch? I'd really like to get this into 1.7.

Actually, I hope to have time to work on this this weekend, but nothing is sure...

Created attachment 28368 [details] [review]
further improvements

Unfortunately I didn't have time to complete everything needed. As I will for sure not have time during the next three weeks. I'm posting the latest version here.

Created attachment 29154 [details] [review]
0001-input-provide-Button-mapping-DIX-property.patch

Generic button mapping property.

Created attachment 29155 [details] [review]
0001-input-provide-Axis-Mapping-DIX-property.patch

Generic axis mapping property.

Please test these two. I doubt they'll make 1.7 though.

Peter, your provided patches do no create a "Axis Inversion" property, meaning it's not possible to "rotate" the device. Do you have such additional patch? Or did I overlook something. Now I've got more time to test, so I'll try my best!

I like the proposed changes but IMO they're too convoluted. Why not solve the whole thing (except buttons) with a single (post or pre-accel?) matrix multiplication? The matrix setup could still be handled via Inversion/Mapping/Scaling props, so a user wouldn't see a difference.

If the axis mapping has a value of its own, then just ignore me. But I think this may lead to bloated signatures, e.g. now I need to know which axis is x and y to do accel properly.

The guesswork in place now (see accelPointerPredictable and thousands other places) ain't any better, but I'd prefer a clean single transform which can be optimized and mainly factored to a single source file. At least, that's the case I had in mind when I pushed for raw and cooked Xi2 events. So it might look like:

1 save raw from driver
2 apply matrix if non-unity
3 apply accel (may be swapped with matrix)
4 clip
5 generate events

The matrix would take up mapping, scaling (meaning I could move ConstDecel), inversion, rotation, whatnot. I guess since randr uses matrices there's some code in place already.

How does that sound?

fwiw, my most recent patch includes a protocol extension for XI2.1 to expose axis mapping and inversion to the clients. that patch is in broken state, it's quite tricky to get right and I've been rather busy lately with other stuff.

*** Bug 27797 has been marked as a duplicate of this bug. ***

Any progress, Peter? This would be great for rotating the screen to view PDF files and such.

some related work that could be extended for this purpose:
http://lists.freedesktop.org/archives/xorg-devel/2010-May/008116.html

XI2.1 is on backburner at the moment.

Closing as fixed, this should address the 99% use-case:

commit 6cccf0131c8464d8838cae2200730873d7dd9e45
Author: Peter Korsgaard <peter.korsgaard@barco.com>
Date:   Tue May 25 11:03:28 2010 +0200

    dix: add 3x3 transformation matrix xinput property for multi-head handling


for absolute devices, and then for relative devices we have:

commit b58221f9da8c549d979215271359c6cd88b5568a
Author: Peter Hutterer <peter.hutterer@who-t.net>
Date:   Fri Feb 8 14:52:02 2013 +1000

    dix: support the transformation matrix for relative devices.


The matrix is already well documented and in use by various clients.