xinput_calibrator icon indicating copy to clipboard operation
xinput_calibrator copied to clipboard

Management of X/Y swap and inversions

Open tonio73 opened this issue 14 years ago • 29 comments

Detect and take into account for X/Y axes swap and inversion.

  • better make system (no more #include "xxx.cpp" )
  • only outputting calibration commands or data => can be wrapped into a script

tonio73 avatar Jan 28 '11 13:01 tonio73

I might be having a similar issue with a touch screen calibration. The values for x,y touchscreen coordinates are as follows as reported from evtest, It looks like the x and y cooridinates are swapped and that X decreases instead of increasing.

819,1203 824,440

242,1203 264,443

xinput_calibrator is not providing the correct figures for the Xorg calibration file. It provides 1203 443 242 852 as minX,maxX,minY,maxY. Do these need a swap xy and x inversion to work correctly?

I'm not sure if xinput_calibrator can handle this situation.

Im not sure exactly what the system does with these calibration figures and how it uses them.

How do you resolve this issue?

johnschimandle avatar Mar 19 '11 00:03 johnschimandle

Have you tried the code patch attached to this thread as a pull request ?

Antoine

tonio73 avatar Mar 19 '11 10:03 tonio73

I'll give it a try but I'm not a git master. I installed git and downloaded the master version using the following command

git clone https://github.com/tias/xinput_calibrator.git

Will that get me the pull request as well. Probably not. If not, can you give me some simple instructions to get the pull request.

git pull ??????

John

-----Original Message----- From: tonio73 [mailto:reply+i-558921-aa31ef1727fafcf5182e77c6379eeaff772b0687@reply.github .com] Sent: Saturday, March 19, 2011 3:54 AM To: [email protected] Subject: Re: [GitHub] Management of X/Y swap and inversions [tias/xinput_calibrator GH-27]

Have you tried the code patch attached to this thread as a pull request ?

Antoine

Reply to this email directly or view it on GitHub: https://github.com/tias/xinput_calibrator/pull/27#issuecomment-892342

johnschimandle avatar Mar 19 '11 18:03 johnschimandle

I think I figured it out. The tonio73 is a different master. So I cloned

git clone https://github.com/tonio73/xinput_calibrator.git

-----Original Message----- From: John Schimandle [mailto:[email protected]] Sent: Saturday, March 19, 2011 11:04 AM To: 'tonio73' Subject: RE: [GitHub] Management of X/Y swap and inversions [tias/xinput_calibrator GH-27]

I'll give it a try but I'm not a git master. I installed git and downloaded the master version using the following command

git clone https://github.com/tias/xinput_calibrator.git

Will that get me the pull request as well. Probably not. If not, can you give me some simple instructions to get the pull request.

git pull ??????

John

-----Original Message----- From: tonio73 [mailto:reply+i-558921-aa31ef1727fafcf5182e77c6379eeaff772b0687@reply.github .com] Sent: Saturday, March 19, 2011 3:54 AM To: [email protected] Subject: Re: [GitHub] Management of X/Y swap and inversions [tias/xinput_calibrator GH-27]

Have you tried the code patch attached to this thread as a pull request ?

Antoine

Reply to this email directly or view it on GitHub: https://github.com/tias/xinput_calibrator/pull/27#issuecomment-892342

johnschimandle avatar Mar 19 '11 18:03 johnschimandle

These changes are awesome. Just what I needed. Thanks a bunch.

For those of you like me who are not experts at this. Here's a list of commands in Ubuntu 10.10 which gets you what you need.

cd ~ sudo apt-get install git sudo apt-get install autoconf sudo apt-get install libtool sudo apt-get install x11proto-input-dev sudo apt-get install libxext-dev sudo apt-get install libxi-dev sudo apt-get install g++ mkdir tonio73 cd tonio73 git clone https://github.com/tonio73/xinput_calibrator.git cd xinput_calibrator ./autogen.sh make cd src ./xinput_calibrator >99-calibration.conf sudo cp 99-calibration.conf /usr/share/X11/xorg.conf.d

logout and log back in

test your touchscreen

reboot

test your touchscreen

-----Original Message----- From: John Schimandle [mailto:[email protected]] Sent: Saturday, March 19, 2011 11:17 AM To: 'tonio73' Subject: RE: [GitHub] Management of X/Y swap and inversions [tias/xinput_calibrator GH-27]

I think I figured it out. The tonio73 is a different master. So I cloned

git clone https://github.com/tonio73/xinput_calibrator.git

-----Original Message----- From: John Schimandle [mailto:[email protected]] Sent: Saturday, March 19, 2011 11:04 AM To: 'tonio73' Subject: RE: [GitHub] Management of X/Y swap and inversions [tias/xinput_calibrator GH-27]

I'll give it a try but I'm not a git master. I installed git and downloaded the master version using the following command

git clone https://github.com/tias/xinput_calibrator.git

Will that get me the pull request as well. Probably not. If not, can you give me some simple instructions to get the pull request.

git pull ??????

John

-----Original Message----- From: tonio73 [mailto:reply+i-558921-aa31ef1727fafcf5182e77c6379eeaff772b0687@reply.github .com] Sent: Saturday, March 19, 2011 3:54 AM To: [email protected] Subject: Re: [GitHub] Management of X/Y swap and inversions [tias/xinput_calibrator GH-27]

Have you tried the code patch attached to this thread as a pull request ?

Antoine

Reply to this email directly or view it on GitHub: https://github.com/tias/xinput_calibrator/pull/27#issuecomment-892342

johnschimandle avatar Mar 19 '11 18:03 johnschimandle

glad to see these modifications being tested : )

I'll merge it in master, but some patches contain multiple changes, which I'd like to split up first...

Kind regards, Tias

tias avatar Mar 19 '11 20:03 tias

Hi,

I tried to use the .deb package to calibrate an IDEACOM touch panel. We were using the calibration tool and driver supplied by ideacom; but after an X update it complained about ABImodule version not being correct. So after some googling I found your calibration tool.

With our default debian without the ideacom driver it worked kind of out of the box with the evdev tablet catchall driver. X and Y are swapped so I added Option "InvertY" "on" and the same for X to get the axes OK. When I calibrate it with your tool this is the

Section "InputClass" Identifier "evdev tablet catchall" MatchIsTablet "on" Option "InvertX" "on" Option "InvertY" "on" MatchDevicePath "/dev/input/event*" Driver "evdev" EndSection

Then i started the calib tool:

When I put the calibration file in /etc/X11/xorg.conf.d/99-calibration.conf nothing happens; so I added the calibration line to: /usr/share/X11/xorg.conf.d/10-evdev.conf : it has some effect on the calibration->but's it's now complety off. (i can reach one corner, and it didn't change anythin dynamic; had to restart X)

Output of your tool:

Warning: multiple calibratable devices found, calibrating last one (IDEACO IDC 6681) use --device to select another one. Calibrating EVDEV driver for "IDEACO IDC 6681" id=11 current calibration values (from XInput): min_x=0, max_x=65535 and min_y=0, max_y=65535

Doing dynamic recalibration: Setting new calibration data: 6983, 58920, 8447, 55777

--> Making the calibration permanent <-- copy the snippet below into '/etc/X11/xorg.conf.d/99-calibration.conf' Section "InputClass" Identifier "calibration" MatchProduct "IDEACO IDC 6681" Option "Calibration" "6983 58920 8447 55777" EndSection

Any help is appreciated; tomorrow I have time to test it with the latest GIT version.

Kind Regards

Rene Dohmen

acidjunk avatar Mar 23 '11 00:03 acidjunk

follow my list of commands above to get the latest code from tonio73 git repository and rebuild the xinput_calibrator binary and run it from your local directory. See if that fixes your problem.

johnschimandle avatar Mar 23 '11 13:03 johnschimandle

First I removed the .deb, and restored changes in the conffiles. The behaviour is exactly the same. Pointer goes to right bottom screen on touch, and keeps there. (when i touch the left corner I can seer the pointer move almost 1 cm horizontally)

99-calibration.conf: Section "InputClass" Identifier "calibration" MatchProduct "IDEACO^D IDC 6681" Option "Calibration" "6804 58299 10374 56739" Option "SwapAxes" "0" Option "InvertX" "1" Option "InvertY" "1" EndSection

acidjunk avatar Mar 25 '11 18:03 acidjunk

The device we currently use:

MSI AE1900 MSI AE1920 MSE AE 2020M

I can sent you one of our development machines?

On Wed, Mar 23, 2011 at 2:58 PM, johnschimandle < [email protected]>wrote:

follow my list of commands above to get the latest code from tonio73 git repository and rebuild the xinput_calibrator binary and run it from your local directory. See if that fixes your problem.

Reply to this email directly or view it on GitHub: https://github.com/tias/xinput_calibrator/pull/27#issuecomment-907623

acidjunk avatar Mar 28 '11 12:03 acidjunk

Sure that could work. Send me an e-mail at [email protected] and I'll send you my address.

johnschimandle avatar Mar 28 '11 13:03 johnschimandle

We're getting a bit of topic on this merge request...

acidjunk, please upon a new bug for your problem. Also, before taking any further steps it should be verified that your problem is indeed a calibration problem, and not a touchscreen driver problem.

  • Before applying any calibration, does the screen behave correctly, e.g. if you draw a line on the screen with your finger, does the pointer follow a line too? If no:
  • What kind of kernel module speaks to the hardware? (is it a usb touchscreen?) If yes:
  • What version of the Xserver, xinput and of evdev do you use?

Kind regards, Tias

(As to this merge request itself: its to dirty to apply as-is. it should be rebased cleanly against master, coding style should not be changed on code that is not modified, and file renames are preferably avoided if not necessary (makes applying other patches harder), some patches should also be split because they change multiple things, making bughunting/bisecting harder later on. I really want to merge the improvements, but it requires more work. Any help on that is appreciated ; )

tias avatar Mar 29 '11 01:03 tias

Hello Tias,

Your bracketed comment is hard and unjustified.

I have removed dirty things like #include "xxx.cxx", I have cleaned up includes and headers, I have created functions to handle debug traces and informational messages. I have recasted things when loosely typed. I remember a function that looked imported and that was taking string parameters instead of integers, as in command line. What I have done is definitively not dirtier but cleaner.

As for the file renaming, it is only simplification to redundant names like: src/gui/gui_gtkmm.cpp -> src/gui/gtkmm.cpp Since we are no longer using CVS but GIT, I do not see the issue.

On top, I have added correct swap and inversion detection. Swap detection was totally buggy.

If you wish to modify the proposal, I am kine to help but be more specific on your comments.

Antoine

tonio73 avatar Mar 29 '11 07:03 tonio73

On Tue, Mar 29, 2011 at 3:32 AM, tias < [email protected]>wrote:

We're getting a bit of topic on this merge request...

acidjunk, please upon a new bug for your problem. Also, before taking any further steps it should be verified that your problem is indeed a calibration problem, and not a touchscreen driver problem.

  • Before applying any calibration, does the screen behave correctly, e.g. if you draw a line on the screen with your finger, does the pointer follow a line too?

Yup. But calibration is off -> you can't reach the corners X and Y are swapped

If no:

  • What kind of kernel module speaks to the hardware? (is it a usb touchscreen?)

Yes it an USB touch panel, we had an closed src driver from IDEACOM; but after updating oud debian testing to unstable it doesn;t work anymore So evdev gets loaded (catch all tablet)

If yes:

  • What version of the Xserver, xinput and of evdev do you use?

xorg-server 2:1.9.4.901-1 xinput 1.5.3-1 evdev 1:2.6.0-2

The problem is not that urgent because I received a new IDEACOM driver (closed src) which works now. Next week i've some spare time; I will discuss the matter with our CEO about sending a machine straight to you so you can test with our hardware

Kind Regards

Rene

Kind regards, Tias

(As to this merge request itself: its to dirty to apply as-is. it should be rebased cleanly against master, coding style should not be changed on code that is not modified, and file renames are preferably avoided if not necessary (makes applying other patches harder), some patches should also be split because they change multiple things, making bughunting/bisecting harder later on. I really want to merge the improvements, but it requires more work. Any help on that is appreciated ; )

Reply to this email directly or view it on GitHub: https://github.com/tias/xinput_calibrator/pull/27#issuecomment-929031

acidjunk avatar Mar 31 '11 14:03 acidjunk

I haven't checked the source code yet but could it be a signed 16 bit integer issue. This touch screen reports numbers greater than 32767 as coordinates. Maybe the comparison algorithm is using signed 16 bit data types and thus causing a problem in the options setting algorithm.

It would also be intersting to get the output from evtest for touching the 4 corners of the screen. evtest returns the actual coordinates. We can then compare these coordinates to the output of the calibrator and see where the problem is. You would report them back as follows representing the corners of the screen.

(x,y) (x,y)

(x,y) (x,y)

-----Original Message----- From: acidjunk [mailto:reply+i-558921-aa31ef1727fafcf5182e77c6379eeaff772b0687@reply.github .com] Sent: Thursday, March 31, 2011 7:07 AM To: [email protected] Subject: Re: [GitHub] Management of X/Y swap and inversions [tias/xinput_calibrator GH-27]

On Tue, Mar 29, 2011 at 3:32 AM, tias < [email protected]>wrote:

We're getting a bit of topic on this merge request...

acidjunk, please upon a new bug for your problem. Also, before taking any further steps it should be verified that your problem is indeed a calibration problem, and not a touchscreen driver problem.

  • Before applying any calibration, does the screen behave correctly, e.g. if you draw a line on the screen with your finger, does the pointer follow a line too?

Yup. But calibration is off -> you can't reach the corners X and Y are swapped

If no:

  • What kind of kernel module speaks to the hardware? (is it a usb touchscreen?)

Yes it an USB touch panel, we had an closed src driver from IDEACOM; but after updating oud debian testing to unstable it doesn;t work anymore So evdev gets loaded (catch all tablet)

If yes:

  • What version of the Xserver, xinput and of evdev do you use?

xorg-server 2:1.9.4.901-1 xinput 1.5.3-1 evdev 1:2.6.0-2

The problem is not that urgent because I received a new IDEACOM driver (closed src) which works now. Next week i've some spare time; I will discuss the matter with our CEO about sending a machine straight to you so you can test with our hardware

Kind Regards

Rene

Kind regards, Tias

(As to this merge request itself: its to dirty to apply as-is. it should be rebased cleanly against master, coding style should not be changed on code that is not modified, and file renames are preferably avoided if not necessary (makes applying other patches harder), some patches should also be split because they change multiple things, making bughunting/bisecting harder later on. I really want to merge the improvements, but it requires more work. Any help on that is appreciated ; )

Reply to this email directly or view it on GitHub: https://github.com/tias/xinput_calibrator/pull/27#issuecomment-929031

Reply to this email directly or view it on GitHub: https://github.com/tias/xinput_calibrator/pull/27#issuecomment-941156

johnschimandle avatar Mar 31 '11 15:03 johnschimandle

Hello Antoine,

I'm sorry for the short comment, I did not intend to do you unjustice or step on your toes. I was travelling at the time, I hope this post clears some points.

I did not mean to say that the code is dirty. In fact, I really like many of the changes you did. What I meant is that the pull request is dirty: not the end result (the what) but the git-patchset (the how). In more detail:

  • There are 58 commits, but because of the debian merge/unmerge only 6 are relevant: ad47b76 9dae715 b217e59 e43e686 e294d7b e4ee724
  • The 'Managing correctly...' patch contains both the calibration changes and the logging changes
  • The 'Managing correctly...' patch contains changes part of the file reorganization (calib/Makef, gui/x11, main_x11)
  • In multiple places, the code is not change but the indenting or layout is

You may think that I am nitpicking. However, I simply wish to review your changes before accepting them. Because I want to verify the changes, learn from them, and be able to fix bugs should they occur. This is currently hard because:

  • the 58 commits do not allow me to get an overview of the changes you actually did
  • the logging changes should be separated out because your other patch touches the core of the calib, and this does not. Additionally, I want to discuss with you an alternative method for the logging patch that achieves the same goal of scriptability/writing to files.
  • not having the file-organization in an uninterupted sequence of patches does not allow me to apply those patches while still reviewing the rest
  • every style-changed line of code takes me time to realise it did not change, and makes the diff grow to sizes that makes it hard for me to keep the overview. Peter Hutterer (the xorg-input dev) once wrote a very interesting article about patches: http://who-t.blogspot.com/2009/12/on-commit-messages.html

To show more concretely what I mean, I started rebasing/modifying your patchset to address the above points (haven't started on splitting away the logging in a separate patch yet). Its available here: https://github.com/tias/xinput_calibrator/tree/tonio_rebase I hope you agree that it although the end result is the same, it is easier to understand the changes? e.g. compare https://github.com/tonio73/xinput_calibrator/commit/e43e686abcf1f178abfd4c62faedf2e5ca23d994 and https://github.com/tias/xinput_calibrator/commit/e4ffde5ab28397c3154a60d930bb15bed5cda2a3

Now about the patches:

  • 'Software engineering ...' and 'Completing file reorg...' I have a slight preference not to rename the files and join those two patches instead. I didn't know git could handle file renames so nicely, that's cool! On the other hand, why change if we don't have too? Many difftools seem to be a lot less smart then git about the file renames for example. If you feel strong about keeping the file renames I'll leave it as is.
  • 'Fixing .gitignore' I added a .gitignore change from the previous patch in it. Will merge
  • 'Managing correctly ...' The logging changes should go in the next patch. In the ideal case, the patch would be split further into the struct changes, the case/switch change and the actual calibration change. That would be the ideal, having the logging out would already be great : ) About the calibration: I intentionally did not include invertX/invertY stuff because xf86ScaleAxis and the math of the calibrator support it automatically: if X or Y are inverted then the min and max values are too (e.g. instead of 12 638 as min_x max_x values, you get 638 12 as min_x max_x values). Is there any reason why you did include it? Now about what was actually wrong with the code: is it correct if the bug in the previous code was that in case of swapXY, the X coordinates have to be scaled with the width instead of the height (and similar for Y), instead of just swapping them? [Additionally, the re-swap after the computation should not be done] I unfortunately don't have a swapped touchscreen; I tried to emulate it by setting swapXY on and doing a calibration. I suspect perhaps the code worked 'reasonably' fine in case the height and width of the screen are very similar... Great to see it being tested more thoroughly and fixed though : )
  • 'Fixing some error handling' All error handling changes should go into this patch. The motivation is making the output scriptable, the general goal could be to have the calibrator write config stuff to files instead of simply on stdout? https://github.com/an1ka/xinput_calibrator/commit/9195312000a98820cb7f145f99a997fb0c6885fc seems to want something similar too. An idea I had was to add a --output-file option, and have the output_* functions take a FILE* argument that they would write too. Would be stdout by default. Would that be an equally fine solution for you?
  • 'Fixing xinput_do_set_prop..' Nice cleanup!

Looking forward to hear back from you, Tias

tias avatar Apr 21 '11 21:04 tias

Hello Tias,

Thanks for this detailed answer. See below my comments.

On 04/21/2011 11:08 PM, tias wrote:

Hello Antoine,

I'm sorry for the short comment, I did not intend to do you unjustice or step on your toes. I was travelling at the time, I hope this post clears some points.

I did not mean to say that the code is dirty. In fact, I really like many of the changes you did. What I meant is that the pull request is dirty: not the end result (the what) but the git-patchset (the how). Part of it comes from the fact that it has been the first time I am using GIT, I am more accustomed to SVN. Feel free to reformat/split the version history as you wish. Moreover, while developing, it is sometimes difficult to organize beforehand the end result into structured commits. It is sometime "while doing" that things get modified. In more detail:

  • There are 58 commits, but because of the debian merge/unmerge only 6 are relevant: ad47b76 9dae715 b217e59 e43e686 e294d7b e4ee724
  • The 'Managing correctly...' patch contains both the calibration changes and the logging changes
  • The 'Managing correctly...' patch contains changes part of the file reorganization (calib/Makef, gui/x11, main_x11)
  • In multiple places, the code is not change but the indenting or layout is

I really apologize for that. Note that I tried to complete prototype doc when missing or unclear. About file renaming, it is really not a major aspect of my commits. I originally did the renaming while sorting out header and source files. I think removing includes to source files and refining class structure really make this software better for the maintainer and for people wishing to modify it.

Aside from that, you are right, the two main aspect of my contrib are management of swap and inversions and modification to the outputs. These two points were really missing before I could use xinput_calibrator on my testcase: adding calibration to an embedded PC on an industrial machine.

You may think that I am nitpicking. However, I simply wish to review your changes before accepting them. Because I want to verify the changes, learn from them, and be able to fix bugs should they occur. This is currently hard because:

  • the 58 commits do not allow me to get an overview of the changes you actually did
  • the logging changes should be separated out because your other patch touches the core of the calib, and this does not. Additionally, I want to discuss with you an alternative method for the logging patch that achieves the same goal of scriptability/writing to files.
  • not having the file-organization in an uninterupted sequence of patches does not allow me to apply those patches while still reviewing the rest
  • every style-changed line of code takes me time to realise it did not change, and makes the diff grow to sizes that makes it hard for me to keep the overview. Peter Hutterer (the xorg-input dev) once wrote a very interesting article about patches: http://who-t.blogspot.com/2009/12/on-commit-messages.html

To show more concretely what I mean, I started rebasing/modifying your patchset to address the above points (haven't started on splitting away the logging in a separate patch yet). Its available here: https://github.com/tias/xinput_calibrator/tree/tonio_rebase I hope you agree that it although the end result is the same, it is easier to understand the changes? e.g. compare https://github.com/tonio73/xinput_calibrator/commit/e43e686abcf1f178abfd4c62faedf2e5ca23d994 and https://github.com/tias/xinput_calibrator/commit/e4ffde5ab28397c3154a60d930bb15bed5cda2a3

Now about the patches:

  • 'Software engineering ...' and 'Completing file reorg...' I have a slight preference not to rename the files and join those two patches instead. I didn't know git could handle file renames so nicely, that's cool! On the other hand, why change if we don't have too? Many difftools seem to be a lot less smart then git about the file renames for example. If you feel strong about keeping the file renames I'll leave it as is. As said above, what is important is the class structure and the header-source split, the file renaming is minor.
  • 'Fixing .gitignore' I added a .gitignore change from the previous patch in it. Will merge
  • 'Managing correctly ...' The logging changes should go in the next patch. In the ideal case, the patch would be split further into the struct changes, the case/switch change and the actual calibration change. That would be the ideal, having the logging out would already be great : ) About the calibration: I intentionally did not include invertX/invertY stuff because xf86ScaleAxis and the math of the calibrator support it automatically: if X or Y are inverted then the min and max values are too (e.g. instead of 12 638 as min_x max_x values, you get 638 12 as min_x max_x values). Is there any reason why you did include it? Now about what was actually wrong with the code: is it correct if the bug in the previous code was that in case of swapXY, the X coordinates have to be scaled with the width instead of the height (and similar for Y), instead of just swapping them? [Additionally, the re-swap after the computation should not be done] I unfortunately don't have a swapped touchscreen; I tried to emulate it by setting swapXY on and doing a calibration. I suspect perhaps the code worked 'reasonably' fine in case the height and width of the screen are very similar... Great to see it being tested more thoroughly and fixed though : ) For my testcase, I have both inversion and swap and xinput_calibrator really failed. From what I remember calibration was moving to the wrong direction: touch to display offset was increasing every calibration attempt. Actually, modified version shows that inverted axis really requires a special handling: if ( previous.invert ) { updated.min = ( (2_screen_dim - clicked[2] - clicked[3]) * old_scale/2 ) + previous.min + 0.5; updated.max = ( (2_screen_dim - clicked[0] - clicked[1]) * old_scale/2 ) + previous.min + 0.5; } else { updated.min = ( (clicked[0] + clicked[1]) * old_scale/2 ) + previous.min + 0.5; updated.max = ( (clicked[2] + clicked[3]) * old_scale/2 ) + previous.min + 0.5; }

(clicked at index 0 and 1 are lowest clicked values, 2 and 3 are highest).

Also, I kind of remember that the fact that swap was already on/off was not carefully handled (reason to have XORs in my code).

While devloping, I was using the --fake mode on a standard desktop screen (no swap, no invert) but I was simulating X-Y swap and inversion when clicking on the calibration crosses (put some tape on the screen to locate 4 crosses location).

In my proposal, X-Y inversion and swap is managed following EVDEV order: // Evdev v2.3.2 order to compute coordinates from peripheral to screen: // - swap xy axis // - calibration (offset and scale) // - invert x, invert y axis

This feature change appears mainly in

bool Calibrator::finish(int width, int height) void Calibrator::process_axys( int screen_dim, const AxisInfo &previous, std::vector &clicked, AxisInfo &updated )

Main trick is in sorting X and Y vectors of clicked coordinates and then apply min and max computation of code excerpt above. This trick comes with a change to the data structure (in order to not reimplement a sort function). I have tried to consolidate as much as possible X/Y axis commonalities to make code simpler.

  • 'Fixing some error handling' All error handling changes should go into this patch. The motivation is making the output scriptable, the general goal could be to have the calibrator write config stuff to files instead of simply on stdout? https://github.com/an1ka/xinput_calibrator/commit/9195312000a98820cb7f145f99a997fb0c6885fc seems to want something similar too. An idea I had was to add a --output-file option, and have the output_* functions take a FILE* argument that they would write too. Would be stdout by default. Would that be an equally fine solution for you? Sorting outputs into info/trace/error is quite common and brings the advantage of removing a lot of "if (verbose)...". The output() method is also quite elegant and removes the need to pass a file pointer. About:
  • writing debug info to stdout and result optionally to file (your proposal),
  • or writing result to stdout and debug and info only if activated (my proposal) I have no strong point on this. Only the result matters to me: being able to save calibration to file directly or through an IO redirection.
  • 'Fixing xinput_do_set_prop..' Nice cleanup!

Thanks.

Hope you have enough information in you hands to go forward. In any case, do not hesitate to ask for more information.

Antoine

tonio73 avatar Apr 24 '11 18:04 tonio73

Hey Antoine,

I finally understand why we are seeing different behaviour and why you had to do the above changes! Frustratingly, the behaviour of calibration in Evdev has changed in version 2.3.2... Evdev commit 3772676fd65065b43a94234127537ab5030b09f8 'Swap axes before applying touch screen calibration.' has as comment swapaxes BEFORE scaling again, because 'the X and Y axes in calibration should be labelled as the user perceives them -- not as the kernel sends them'

For me, that reasoning is nonsense. Nonetheless, it changed; while the machine my touchscreen is hanging on still has an evdev version before 2.3.2...

I'm working on including your data and class structure changes separately, and will then add your calibration changes to the calibrator/evdev.cpp code, with a version check.

I'm glad things are really clearing up now, I hope to commit the changes in a reasonable timeframe...

tias avatar Jun 19 '11 09:06 tias

@tias Thank you for considering this pull request. Could you say what its status is please?

shoe avatar Jan 14 '12 01:01 shoe

It would be nice if the good part of the work could be merged onto master; this does fixes some issues people has and it seems that the project has no action since 2011's Jun :-(

otavio avatar Feb 28 '12 14:02 otavio

yes, please, @tias ?

shoe avatar Mar 07 '12 21:03 shoe

Hey All,

You are right, I should really pick this up again. If you're wondering, I haven't abandoned the project, I was just busy with other things...

The 'version check' was next to impossible from what I remember. Since so much time has passed since, I'll just add the (inversion) calibration code as proposed in the above patch. I'll have to make it clear in the announcement that from the next release, the code should only be used with evdev above 2.3.2

So, I intend to ramp up a new release in the coming weeks.

Kind regards, Tias

tias avatar Mar 08 '12 09:03 tias

Hi @tias ,

How is the progress toward a new release?

shoe avatar Mar 28 '12 19:03 shoe

OK, I finally made some good progress on this patchset!

In master you can find following aspects of it:

  • the file reorganisation (headers/cleaner compilation)
  • lots of refactorings and cleanups
  • struct changes for axis swapping and inversion

The real gist, the different handling of inversion and swapping from evdev 2.3.2 onwards, is not included yet though. Attempts to dynamically detect which evdev version a user is running have failed. I havent decided yet how to add the new behavior exactly.

tias avatar Jun 08 '12 00:06 tias

@tias Thanks for picking this back up!

I'd like to suggest that you simply declare that xinput_calibrator is only compatible with evdev >= 2.3.2. People who need backward compatibility still have the older versions of xinput_calibrator they can use, but meanwhile everyday there are more and more people using the newer evdev.

What do you think?

shoe avatar Jun 08 '12 01:06 shoe

@shoe and @tias another option is make it a configure option, and default to the newer evdev.

otavio avatar Jun 08 '12 12:06 otavio

@otavio That's an even better suggestion!

shoe avatar Jun 09 '12 00:06 shoe

OK, so I wrote a small testing framework that can emulate a standard Xorg driver as well as evdev 2.7.0 (which should be representative for evdev >= 2.3.2). I felt this was needed as I didn't have the hardware setup required to reproduce the reported behaviour, and didn't feel comfortable enough to just commit these changes to the calibration code.

The tests showed that the problem all along was the 'invertX/Y' handling that was added to evdev. This has now been fixed in the Evdev.cpp code (I should still #ifdef it with a compile option as @otavio suggested).

In retrospect, it seems that @tonio73 was aware of this from the beginning, I just didn't see it through all the other changes. I should really have handled this pull request/bug report differently... My apologies to @tonio73 and others who would like to have seen this stuff merged sooner.

To everybody following this: please test master on your hardware. If it works, let me know, if it doesn't, open a new bug report with --verbose output please.

Thanks, Tias

tias avatar Jun 26 '12 21:06 tias

Good; I will rebase our version here and give it a try.

Thank you very much by handling it; it was indeed a huge problem for all users and a difficult to handle one. Specially for user-friendness it is a nice milestone we'll accomplish if it works fine.

otavio avatar Jun 29 '12 16:06 otavio