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

Management of X/Y swap and inversions #27

Open
wants to merge 58 commits into
base: master
Choose a base branch
from
Open

Conversation

tonio73
Copy link
Contributor

@tonio73 tonio73 commented Jan 28, 2011

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

tias and others added 30 commits May 11, 2010 00:23
xinput_calibrator

Motivated to ease packaging (eg one binary, default name)
Use --with-gui=default|gtkmm|x11 option to specify.
screen and triangle from tangogps
hand from humanity
file

What a silly bug in gnome/xfce4-terminal that's already open for 5 years...
https://bugzilla.gnome.org/show_bug.cgi?id=324407
Avoids unnecessary linking, as suggested by Eugene Paskevich
This reverts commit fb22ed1.
It changed the source, which would differ from a 'pristine tarball'
Will upload the patch the quilt way.
introduces the debian/patches quilt dir and a first patch in it
@tonio73
Copy link
Contributor Author

tonio73 commented Mar 19, 2011

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

Antoine

@johnschimandle
Copy link

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: john_schimandle@hotmail.com
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:
#27 (comment)

@johnschimandle
Copy link

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:john_schimandle@hotmail.com]
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: john_schimandle@hotmail.com
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:
#27 (comment)

@johnschimandle
Copy link

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:john_schimandle@hotmail.com]
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:john_schimandle@hotmail.com]
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: john_schimandle@hotmail.com
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:
#27 (comment)

@tias
Copy link
Owner

tias commented Mar 19, 2011

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

@acidjunk
Copy link

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

@johnschimandle
Copy link

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.

@acidjunk
Copy link

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
Copy link

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 <
reply@reply.github.com>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:
#27 (comment)

@johnschimandle
Copy link

Sure that could work. Send me an e-mail at john_schimandle@hotmail.com and I'll send you my address.

@tias
Copy link
Owner

tias commented Mar 29, 2011

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 ; )

@tonio73
Copy link
Contributor Author

tonio73 commented Mar 29, 2011

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

@acidjunk
Copy link

On Tue, Mar 29, 2011 at 3:32 AM, tias <
reply@reply.github.com>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:
#27 (comment)

@johnschimandle
Copy link

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: john_schimandle@hotmail.com
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 <
reply@reply.github.com>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:
#27 (comment)

Reply to this email directly or view it on GitHub:
#27 (comment)

Process return 1 instead of 0 in case of timeout.
@tias
Copy link
Owner

tias commented Apr 21, 2011

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 tonio73@e43e686 and e4ffde5

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? an1ka@9195312 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

@tonio73
Copy link
Contributor Author

tonio73 commented Apr 24, 2011

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 tonio73@e43e686 and e4ffde5

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? an1ka@9195312 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

@tias
Copy link
Owner

tias commented Jun 19, 2011

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...

@shoe
Copy link

shoe commented Jan 14, 2012

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

@otavio
Copy link

otavio commented Feb 28, 2012

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 :-(

@shoe
Copy link

shoe commented Mar 7, 2012

yes, please, @tias ?

@tias
Copy link
Owner

tias commented Mar 8, 2012

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

@shoe
Copy link

shoe commented Mar 28, 2012

Hi @tias ,

How is the progress toward a new release?

@tias
Copy link
Owner

tias commented Jun 8, 2012

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.

@shoe
Copy link

shoe commented Jun 8, 2012

@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?

@otavio
Copy link

otavio commented Jun 8, 2012

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

@shoe
Copy link

shoe commented Jun 9, 2012

@otavio That's an even better suggestion!

@tias
Copy link
Owner

tias commented Jun 26, 2012

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

@otavio
Copy link

otavio commented Jun 29, 2012

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants