Bug 27688

Created attachment 35086 [details]
Excerpt from the xorg mouse driver

It'd be better to make this an X server functionality (exported through device properties) so that other drivers can use it too. Otherwise we end up with each driver making up their own solution to this.

(In reply to comment #2)
> It'd be better to make this an X server functionality (exported through device
> properties) so that other drivers can use it too. Otherwise we end up with each
> driver making up their own solution to this.

Ok, but what can I do in the meanwhile? Is there anything I can do in order to help "fixing" this?

Peter, I do really need that option (and also other logitech trackball owners), so I wrote a patch for my Debian sid and it worked.

I think it can be useful to add the Axes Rotation option into the evdev driver, because, as far as I understood, it is going to become the "universal" input 
driver.
So, here is the patch (I followed the instructions on http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches).


(In reply to comment #3)
> (In reply to comment #2)
> > It'd be better to make this an X server functionality (exported through device
> > properties) so that other drivers can use it too. Otherwise we end up with each
> > driver making up their own solution to this.
> 
> Ok, but what can I do in the meanwhile? Is there anything I can do in order to
> help "fixing" this?

Created attachment 39479 [details] [review]
evdev patch for new property "Evdev Axes Rotation"

Created attachment 43746 [details] [review]
Updated patch

Here is the updated patch, including changes suggested by Peter Huttere

Review of attachment 43746 [details] [review]:

looks good, thanks. a few minor comments:
- AxesRotation vs AxisRotation, not sure which one is better here
- I am tempted to up this to a range of 0..3600, just to cover ourselves for future devices with fine-grained resolution
- if you're caching radians, why not cache sin/cos instead? seems like that's the one we need all the time, so recalculating on each event could be spared
- no // comments please, use /* */ instead
- maybe (as follow-up patch) add a deprecation warning when SwapAxes is used?
- rotation for absolute axes seems to be missing. when adding that, please move rotation into a separate function instead of copying the sin/cos multiplication over

as you may have noticed from the list, there is still some discussion on how to handle device rotation. the server supports the transformation matrix that can handle rotation for absolute devices (but not relative ones). But no verdict on that yet, so I'm not sure myself which approach we'll be merging.

Hi Peter,

(In reply to comment #7)
> Review of attachment 43746 [details] [review]:
> 
> looks good, thanks. a few minor comments:
> - AxesRotation vs AxisRotation, not sure which one is better here
for me AxesRotation is appropriate because the transformation is rotating both the axes, not only one

> - I am tempted to up this to a range of 0..3600, just to cover ourselves for
> future devices with fine-grained resolution
ok, I'll modify the patch so that the option value is interpreted as 1/10 of degree instead of 1 degree; remind that also negative values are accepted (positive value = clockwise rotation, negative value = counterclockwise rotation)

> - if you're caching radians, why not cache sin/cos instead? seems like that's
> the one we need all the time, so recalculating on each event could be spared
you're right, I'll do that

> - no // comments please, use /* */ instead
ok, sorry

> - maybe (as follow-up patch) add a deprecation warning when SwapAxes is used?
I agree

> - rotation for absolute axes seems to be missing. when adding that, please move
> rotation into a separate function instead of copying the sin/cos multiplication
> over
I think that the option AxesRotation is targeted to relative axes devices only. In fact I wrote that for my Logitech Trackball. I don't think it make sense to allow a rotation to absolute axes devices that is not multiple of 90 degrees. I also use a wacom tablet, and it does make sense to have +/- 90 degree rotation if the monitor is vertical.

> as you may have noticed from the list, there is still some discussion on how to
> handle device rotation. the server supports the transformation matrix that can
> handle rotation for absolute devices (but not relative ones). But no verdict on
> that yet, so I'm not sure myself which approach we'll be merging.
Yes, I read the discussion. At the moment, since the rotation is not yet handled neither by the server nor by the driver, and I do need the AxesRotation property in order to use my trackball, I will continue working on the patch.

Created attachment 44929 [details] [review]
added "AxesRotation" property

Here is the updated patch:
- added property "AxesRotation"
- option value interpreted as 1/10 of degree
- caching rotatin sine and cosine values

I think this is no longer needed.

The XInput now has Coordinate Transformation Matrix property which can handle this.

What is needed are tools for manipulating this matrix in an accessible way.

eg. setting a matrix that does -35 degree rotation is needlessly challenging, the xinput tool should include options for simplified matrix presets, and perhaps also an option to multiply the existing matrix with one of these presets rather than replacing it.

I tried Paolo's patch on top of evdev 2.6. Works for me.  I am really happy that I can use my Logitech Optical Trackman together with xorg now. (AxesRotation is 400.)

I would recommend to include this patch into evdev.

evdev will _not_ get any driver-specific rotation property. The server transformation matrix will take care of that in a driver-independent fashion, once we get all the required changes done.

Probably you will drop InvertX, InvertY and SwapAxes from evdev, too? They could be replaced by the same matrix operation, afaics. 

I don't know your implementation schedule, but I would suggest to add Paolo's patch until the transformation matrix and the supporting tools have been completed and verified. The patch doesn't seem too risky to me. It would be a shame to see it rejected without a verified alternative.

(In reply to comment #13)
> Probably you will drop InvertX, InvertY and SwapAxes from evdev, too? They
> could be replaced by the same matrix operation, afaics. 

yes. Not 100% sure about the best way to drop them yet, some backwards compatibility would of course be good.

> I don't know your implementation schedule, but I would suggest to add Paolo's
> patch until the transformation matrix and the supporting tools have been
> completed and verified. The patch doesn't seem too risky to me. It would be a
> shame to see it rejected without a verified alternative.

I'd rather not. The inertia we have is rather big; adding any option that will be replaced by the next server version is painful. By the time distributions start adopting the next evdev release we'll have the new server out and upstream will have deprecated the option. So while users are just getting used to a new option we're already moving away from it again.

(In reply to comment #10)
> The XInput now has Coordinate Transformation Matrix property which can handle
> this.
> 
> What is needed are tools for manipulating this matrix in an accessible way.
Indeed, and I would be happy to help.

So, just for example I tried to apply a rotation using the transformation matrix. For a 90 degrees clockwise rotation the transformation matrix is:
[ cos(90) -sin(90) 0; sin(90) cos(90) 0; 0 0 1]

Hence, being cos(90)=0 and sin(90)=1, the matrix is set using the command:
xinput set-prop "Device Name" --type=float "Coordinate Transformation Matrix" 0 -1 0 1 0 0 0 0 1

The matrix did change, but I got no effect on mouse movements. I tried on four different pointer devices with the same effect (no rotation). 
Am I doing something wrong or not?

I'm running a Debian sid with the following xorg and xinput versions:
(Xorg -version)
  X.Org X Server 1.9.5
  Release Date: 2011-03-17
  X Protocol Version 11, Revision 0
(xinput --version)
  xinput version 1.5.3
  XI version on server: 2.0

Indeed, this does not work for me either with 

X.Org X Server 1.10.2
Release Date: 2011-05-28

xinput version 1.5.3
XI version on server: 2.1

This should be fixed in the X server I guess.

(In reply to comment #15)
> (In reply to comment #10)
> > The XInput now has Coordinate Transformation Matrix property which can handle
> > this.
> > 
> > What is needed are tools for manipulating this matrix in an accessible way.

fwiw, I have patches for xinput --rotate device in my trees here, they will go in once the server patches are done. no need to duplicate the efforts.

> The matrix did change, but I got no effect on mouse movements. I tried on four
> different pointer devices with the same effect (no rotation). 
> Am I doing something wrong or not?

rotation is just not supported yet.

(In reply to comment #17)
> fwiw, I have patches for xinput --rotate device in my trees here, they will go
> in once the server patches are done. no need to duplicate the efforts.
ok, happy to know this
 
> > The matrix did change, but I got no effect on mouse movements. I tried on four
> > different pointer devices with the same effect (no rotation). 
> > Am I doing something wrong or not?
> 
> rotation is just not supported yet.
Please take a look at my comment on the related bug #38490 (https://bugs.freedesktop.org/show_bug.cgi?id=38490#c1).
Peter, I suspect that the transformation matrix doesn't work yet, does it?

The ftp server still shows version 1.5.3. Is there a schedule to include the implementation of 'xinput --rotate' into a new official version? 

I would be glad to verify it.

the patches were around at some point but then disappeared into the memory hole. Not sure where they're at at this point, at this point don't expect the changes to happen before 1.12, sorry.

I've got a question: xinput is a user command, but the axis rotation is needed to work around a hardware design issue. Is it wise to ask a user to define a (pretty complex) 3dimensional transformation matrix to make the hardware work?

Instead of hoping to crack a nut with a sledge hammer at some time in the future, I would vote for the evdev patch. I am running this patch for months now. There are no issues. It is a low risk to add. And its easy to find: The link is in _this_ bug report.

Even if you find your patch again and add it to a later version of xinput, the combined transformations of evdev and xinput can be mapped to a single matrix operation, i.e. they wouldn't break each other.

(In reply to comment #21)
> I've got a question: xinput is a user command, but the axis rotation is needed
> to work around a hardware design issue. Is it wise to ask a user to define a
> (pretty complex) 3dimensional transformation matrix to make the hardware work?

xinput should simply add a "xinput --rotate" option that fills in the matrix based on the user-provided values.

> Instead of hoping to crack a nut with a sledge hammer at some time in the
> future, I would vote for the evdev patch. I am running this patch for months
> now. There are no issues. It is a low risk to add.

sooner or later we need generic rotation support. I have to support the evdev option (which can't easily be deprecated once it's widespread enough) and handle bugreports for both implementation. And that doesn't even include the impact it has on the client stack which have to support two different options depending on which driver/server version is running. Adding this is high impact, we've seen exactly this happening with other options already.

> Even if you find your patch again and add it to a later version of xinput, the
> combined transformations of evdev and xinput can be mapped to a single matrix
> operation, i.e. they wouldn't break each other.

http://lists.x.org/archives/xorg-devel/2011-May/022688.html
that's the starting point. As I said above, I don't want an evdev-specific rotation property when we already have support for much of it in the server.

So there are patches.

Are they in any X server release? git?

Is there something missing?

The patches are only on the list so far. The follow-up from Simon is here:
http://lists.x.org/archives/xorg-devel/2011-June/022798.html
IIRC, after that I got preempted and the whole thing fell under the table again. If you have the time, please pick this up from there - getting generic rotation support into the transformation matrix is the correct solution to this problem.

Any news about this?

afaict, nobody has been working on this

At least for me, the patch is no longer working (neither in Ubuntu 12.04 nor in Debian unstable, both with evdev 2.7). 
It seems that something has changed in the server and the patch causes the pointer movements to be quite sloppy when rotation is set (I think due to rounding issues but I'm not sure).

Transformation matrix is still not working either.

Peter, didn't you mention that you already have some code?

http://patchwork.freedesktop.org/patch/12871/

Fixed in the server:


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.

I am more than eager to try this out. Is there some documentation about how to use this feature? Unfortunately your commit didn't show.

Many thanx

Thanks for the commit, I will try asap.

Peter, this means that the patch will be included with the next release, isn't it?
Also, I suppose one can directly set the transformation matrix with the xinput tool, but is there any helper like 'xinput --rotate'?

there's no helper function yet, but you're welcome to help with that (please send a patch to the list, or file a separate bug against xinput).

Some explanation is here: http://www.x.org/wiki/XInputCoordinateTransformationMatrixUsage
tough this page doesn't yet focus on relative devices. The matrix is a standard transformation matrix, with the translation component being 0.

The patch is included in server 1.14.

Ok I will try to help with that.
When ready I will file a new bug against xinput including a patch for the rotate option (I also have a wacom tablet so I can test both relative and absolute devices).

Submitted path to xinput:
https://bugs.freedesktop.org/show_bug.cgi?id=63534

Finally I have been able to try this on Ubuntu 13.10 (xorg 1.14.5 and evdev 2.7.3) and I can confirm that the coordinate transformation matrix is working.