Various fixes & improvements #36

forestbond · 2012-05-22T15:26:37Z

This is the set of changes from my previous pull request, slightly reworked, plus a few new commits to improve evdev calibration. Comments welcome.

tias · 2012-06-07T23:53:10Z

Hi Forest,

Thanks for these patches, they look nice and clean.

Some comments:

axes swap & axis inversion variables: it would be cleaner if these would be put in XYinfo.
In fact, I have already worked on this based on the huge patchset of tonio73 (see pull request Management of X/Y swap and inversions #27). It took lots of patch-splitting and cleaning to get it in a reasonable shape, but I've just finished it and committed it.
Could you please rebase your patchset against these changes (e.g. the current master)? Thanks!
scale (and constrain) function: there is no need to (re)implement this. This is already provided by the X server through xf86ScaleAxis:
http://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/common/xf86Xinput.c
That function is used by all touchscreen drivers, so it would be good if our code would start using that as well. In fact, the closer our code to the code in the drivers, the better...

Now, a more general question regarding calibration:
are you using a recent evdev? (above 2.3.2) because there is a long-standing issue with recent evdev and axis swapping, discussed in #27. Did you also encounter such problems?
(e.g. is that the reason that you need explicit inversion and that you first reset evdev? afaik restoring a bad calibration should always be possible, unless the code has a wrong assumption about the calibration done in the driver, for example when a swap_axes flag is set before calibrating and we do not 'unswap', or re-swap wrongly)

Looking forward to hearing from you,
Tias

forestbond · 2012-06-25T03:33:23Z

Hi Tias,

Sorry for the delay. I have been very busy with work, but will take a look at
your feedback as soon as I have time.

Thanks,
Forest

On Thu, Jun 07, 2012 at 04:53:10PM -0700, Tias Guns wrote:

Hi Forest,

Thanks for these patches, they look nice and clean.

Some comments:

axes swap & axis inversion variables: it would be cleaner if these would be put in XYinfo.
In fact, I have already worked on this based on the huge patchset of tonio73 (see pull request Management of X/Y swap and inversions #27). It took lots of patch-splitting and cleaning to get it in a reasonable shape, but I've just finished it and committed it.
Could you please rebase your patchset against these changes (e.g. the current master)? Thanks!

scale (and constrain) function: there is no need to (re)implement this. This is already provided by the X server through xf86ScaleAxis:
http://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/common/xf86Xinput.c
That function is used by all touchscreen drivers, so it would be good if our code would start using that as well. In fact, the closer our code to the code in the drivers, the better...

Now, a more general question regarding calibration:
are you using a recent evdev? (above 2.3.2) because there is a long-standing issue with recent evdev and axis swapping, discussed in #27. Did you also encounter such problems?
(e.g. is that the reason that you need explicit inversion and that you first reset evdev? afaik restoring a bad calibration should always be possible, unless the code has a wrong assumption about the calibration done in the driver, for example when a swap_axes flag is set before calibrating and we do not 'unswap', or re-swap wrongly)

Looking forward to hearing from you,
Tias

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

forestbond · 2012-07-26T15:20:34Z

Hi Tias,

Sorry for the delayed response.

On Thu, Jun 07, 2012 at 04:53:10PM -0700, Tias Guns wrote:

Some comments:

axes swap & axis inversion variables: it would be cleaner if these would be
put in XYinfo.
In fact, I have already worked on this based on the huge patchset of tonio73
(see pull request Management of X/Y swap and inversions #27). It took lots of patch-splitting and cleaning to get it
in a reasonable shape, but I've just finished it and committed it.
Could you please rebase your patchset against these changes (e.g. the current
master)? Thanks!

I'm looking at this now.

scale (and constrain) function: there is no need to (re)implement this. This
is already provided by the X server through xf86ScaleAxis:
http://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/common/xf86Xinput.c
That function is used by all touchscreen drivers, so it would be good if our
code would start using that as well. In fact, the closer our code to the code
in the drivers, the better...

I agree. I see that you have updated master to use xf86ScaleAxis.

Now, a more general question regarding calibration:
are you using a recent evdev? (above 2.3.2) because there is a long-standing
issue with recent evdev and axis swapping, discussed in #27. Did you also
encounter such problems?

Yes, I had problems with axis swap due to swap detection not taking into
consideration the previous swap value. I fixed it in my pull request, but it
looks like you've also fixed it in master, so I will drop it.

(e.g. is that the reason that you need explicit inversion and that you first
reset evdev? afaik restoring a bad calibration should always be possible,
unless the code has a wrong assumption about the calibration done in the
driver, for example when a swap_axes flag is set before calibrating and we do
not 'unswap', or re-swap wrongly)

Inversion handling in xinput_calibrator was just broken. I fixed it in my pull
request. Evdev's inversion support works, although I see that we can avoid
using it and set min > max instead. I guess Evdev doesn't care if we do this,
but I'm not sure if this will always be the case (even for other drivers)?

Anyway, I think resetting the calibration parameters is necessary. Consider the
case where we've set the "Evdev Axis Calibration" property such that min == max.
We will not be able to calibrate properly. Even if min != max, the old
calibration parameters affect the precision of the Evdev driver transformations.
Consider min == (max - 1). We cannot guarantee accurate calibration without
resetting the Evdev parameters first.

Hope this makes sense. Let me know what you think and I will rebase my changes
as appropriate.

Thanks,

Forest

Forest Bond
http://www.alittletooquiet.net
http://www.rapidrollout.com

tias · 2012-07-31T20:37:42Z

On 07/26/2012 05:20 PM, Forest Bond wrote:

Hi Tias,

Sorry for the delayed response.

On Thu, Jun 07, 2012 at 04:53:10PM -0700, Tias Guns wrote:

Some comments:

axes swap& axis inversion variables: it would be cleaner if these would be
put in XYinfo.
In fact, I have already worked on this based on the huge patchset of tonio73
(see pull request Management of X/Y swap and inversions #27). It took lots of patch-splitting and cleaning to get it
in a reasonable shape, but I've just finished it and committed it.
Could you please rebase your patchset against these changes (e.g. the current
master)? Thanks!

I'm looking at this now.

scale (and constrain) function: there is no need to (re)implement this. This
is already provided by the X server through xf86ScaleAxis:
http://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/common/xf86Xinput.c
That function is used by all touchscreen drivers, so it would be good if our
code would start using that as well. In fact, the closer our code to the code
in the drivers, the better...

I agree. I see that you have updated master to use xf86ScaleAxis.

Now, a more general question regarding calibration:
are you using a recent evdev? (above 2.3.2) because there is a long-standing
issue with recent evdev and axis swapping, discussed in #27. Did you also
encounter such problems?

Yes, I had problems with axis swap due to swap detection not taking into
consideration the previous swap value. I fixed it in my pull request, but it
looks like you've also fixed it in master, so I will drop it.

(e.g. is that the reason that you need explicit inversion and that you first
reset evdev? afaik restoring a bad calibration should always be possible,
unless the code has a wrong assumption about the calibration done in the
driver, for example when a swap_axes flag is set before calibrating and we do
not 'unswap', or re-swap wrongly)

Inversion handling in xinput_calibrator was just broken. I fixed it in my pull
request. Evdev's inversion support works, although I see that we can avoid
using it and set min> max instead. I guess Evdev doesn't care if we do this,
but I'm not sure if this will always be the case (even for other drivers)?

Neither am I, so I prefer not to touch the code for other drivers
(assuming that somebody would have complained about it now if it wouldnt
work).

Anyway, I think resetting the calibration parameters is necessary. Consider the
case where we've set the "Evdev Axis Calibration" property such that min == max.
We will not be able to calibrate properly. Even if min != max, the old
calibration parameters affect the precision of the Evdev driver transformations.
Consider min == (max - 1). We cannot guarantee accurate calibration without
resetting the Evdev parameters first.

I agree. I still feel that resetting the calibration dynamically before
each calibration is a bit... invasive (the pointer will all of a sudden
be at a completely different location when you click)

May I suggest that you implement it as a '--reset' option or similar?
Would that make sense for you as well?

Kind regards,
Tias

Hope this makes sense. Let me know what you think and I will rebase my changes
as appropriate.

Thanks,
Forest

forestbond · 2012-08-21T16:52:38Z

Hi,

On Tue, Jul 31, 2012 at 01:37:51PM -0700, Tias Guns wrote:

On 07/26/2012 05:20 PM, Forest Bond wrote:

On Thu, Jun 07, 2012 at 04:53:10PM -0700, Tias Guns wrote:

Some comments:

axes swap& axis inversion variables: it would be cleaner if these would be
put in XYinfo.
In fact, I have already worked on this based on the huge patchset of tonio73
(see pull request Management of X/Y swap and inversions #27). It took lots of patch-splitting and cleaning to get it
in a reasonable shape, but I've just finished it and committed it.
Could you please rebase your patchset against these changes (e.g. the current
master)? Thanks!

I'm looking at this now.

scale (and constrain) function: there is no need to (re)implement this. This
is already provided by the X server through xf86ScaleAxis:
http://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/common/xf86Xinput.c
That function is used by all touchscreen drivers, so it would be good if our
code would start using that as well. In fact, the closer our code to the code
in the drivers, the better...

I agree. I see that you have updated master to use xf86ScaleAxis.

Now, a more general question regarding calibration:
are you using a recent evdev? (above 2.3.2) because there is a long-standing
issue with recent evdev and axis swapping, discussed in #27. Did you also
encounter such problems?

Yes, I had problems with axis swap due to swap detection not taking into
consideration the previous swap value. I fixed it in my pull request, but it
looks like you've also fixed it in master, so I will drop it.

(e.g. is that the reason that you need explicit inversion and that you first
reset evdev? afaik restoring a bad calibration should always be possible,
unless the code has a wrong assumption about the calibration done in the
driver, for example when a swap_axes flag is set before calibrating and we do
not 'unswap', or re-swap wrongly)

Inversion handling in xinput_calibrator was just broken. I fixed it in my pull
request. Evdev's inversion support works, although I see that we can avoid
using it and set min> max instead. I guess Evdev doesn't care if we do this,
but I'm not sure if this will always be the case (even for other drivers)?

Neither am I, so I prefer not to touch the code for other drivers
(assuming that somebody would have complained about it now if it wouldnt
work).

Maybe it would be good to ask the Evdev developers about axis inversion. They
must have had some reason for implementing it. I'm not sure we can assume that
setting min > max won't break with some future version of Evdev.

Anyway, I think resetting the calibration parameters is necessary. Consider the
case where we've set the "Evdev Axis Calibration" property such that min == max.
We will not be able to calibrate properly. Even if min != max, the old
calibration parameters affect the precision of the Evdev driver transformations.
Consider min == (max - 1). We cannot guarantee accurate calibration without
resetting the Evdev parameters first.

I agree. I still feel that resetting the calibration dynamically before
each calibration is a bit... invasive (the pointer will all of a sudden
be at a completely different location when you click)

It would not be hard to hide the cursor during calibration. In fact we should
probably be doing this anyway. Then the cursor jump would not be visible to the
user.

May I suggest that you implement it as a '--reset' option or similar?
Would that make sense for you as well?

I think it should be the default behavior as it is the only way to guarantee
accurate calibration. But I can implement the "--reset" option instead if that
is what you prefer.

Thanks,

Forest

Forest Bond
http://www.alittletooquiet.net
http://www.rapidrollout.com

The core calibration routines function ignore the XYinfo invert member, so drop it altogether and instead treat axis inversion as an input/output consideration for some drivers. This fixes axis inversion for XorgPrintCalibrator (inversion would not have been enabled before).

This method is no longer different from the Calibrator method it overrides (since axis inversion is ignored).

None of the drivers need to override this method any longer, and it is better not to as it leads to code duplciation so just don't allow it.

Refactor to move reading of calibration parameters into an explicit method ("detect_axys") in preparation. This is needed for some follow up fixes.

Previously, values specified by the user via the --precalib command-line option would be overridden by the detected values with EvdevCalibrator. Now, the detection of current parameters is not performed at all if the --precalib option was passed.

This can be used for dynamic recalibration if the driver supports it. Currently only EvdevCalibrator provides an implementation.

This forces calibration parameters to be set to the device defaults prior to calibration, which is need for accurate calibration in certain cases (i.e. when the previous values were off substantially).

forestbond · 2012-09-05T17:05:48Z

Hi Tias,

Here's a new set of commits for review. Let me know what you think.

Thanks,
Forest

forestbond · 2012-09-05T17:08:05Z

By the way, I have an touch screen with inverted axes that works with evdev, so I have tested that case. I've also run the tests and they pass.

forestbond · 2013-01-07T00:46:14Z

Hi Tias,

Did you have a chance to look at these changes?

Thanks,
Forest

jefflasslett · 2013-01-13T23:27:12Z

Hi Tias & Forest,

@forest: Just looking through your patches Forest and I can see that you and I have covered some of the same ground. For example, we both made finish() non-virtual and eliminated the evdev version.

In the current HEAD there is something quite broken with calibrations where the axes are swapped. You seem to have tackled that. I have too, but your patches look better to me.

@tias: I hope you get a chance to look at this stuff soon.

Cheers,
Jeff

forestbond · 2013-01-16T13:29:24Z

Hi Tias,

Just checking in. I know you are probably busy, but do you think you can give us some indication of whether or not you intend to consider these changes for merging?

Thanks,
Forest

forestbond · 2013-02-05T19:24:10Z

Hi Tias,

As of right now my plan is to create a fork of xinput_calibrator. I'd rather not, of course. But it has been nearly 2.5 years since the last release and this pull request has been open for 9 months. The current state of affairs is becoming burdensome in a variety of ways. We are maintaining too many local patches and our software must depend on command-line options that are not officially supported by xinput_calibrator. What can we do to fix this situation? Would you like to transfer maintainership or give commit access to me? Or should I proceed with the fork?

Thanks,
Forest

ghost · 2013-02-05T19:41:58Z

Hello again,

So, the bug I encountered is here forestbond@ee60066#L3R201. The pointer is not initialised.
Later down the code when it is asigned a value the initialisation is in try..catch blocks. If the first block fails the next one is entered only if the pointer value is null. But because the pointer was not initialised to begin with, if the driver is not evdev, it will fail the first block and then directly skip all 'is null' checks because of its random value. After that it will crash in the 'not null' check.
This happened on g++ 4.5 but not on 4.7. Probably because the newer versions initialise all pointers to null by default.

Best regards,
Plamen

forestbond · 2013-02-05T19:45:15Z

Thanks, that's a good catch. I will fix it.

tias · 2013-02-07T20:41:34Z

Can you tell me what version of evdev you are running? Thanks

forestbond · 2013-02-11T17:31:03Z

Hi Tias,

I am using evdev from Ubuntu 10.04 (2.3.2) and Ubuntu 12.04 (2.7.0).

Thanks,
Forest

forestbond added 10 commits September 5, 2012 12:10

Evdev: Don't override method "finish".

f6b6340

This method is no longer different from the Calibrator method it overrides (since axis inversion is ignored).

Calibrator: Make method "finish" non-virtual.

1e9ae6e

None of the drivers need to override this method any longer, and it is better not to as it leads to code duplciation so just don't allow it.

Skip devices with too many axes.

ccd50be

Separate detection of current calibration values.

ee60066

Refactor to move reading of calibration parameters into an explicit method ("detect_axys") in preparation. This is needed for some follow up fixes.

Calibrator: Add virtual method "apply".

4961547

This can be used for dynamic recalibration if the driver supports it. Currently only EvdevCalibrator provides an implementation.

Add command-line option --reset.

112fd9b

This forces calibration parameters to be set to the device defaults prior to calibration, which is need for accurate calibration in certain cases (i.e. when the previous values were off substantially).

gui_x11: Add on_key_press_event method to mirror gui_gtkmm.

7ffb2ff

Restore original calibration values on abort.

3523a45

ghost mentioned this pull request Feb 3, 2013

Broken left/right rotated screen calibration #39

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various fixes & improvements #36

Various fixes & improvements #36

forestbond commented May 22, 2012

tias commented Jun 7, 2012

forestbond commented Jun 25, 2012

forestbond commented Jul 26, 2012

tias commented Jul 31, 2012

forestbond commented Aug 21, 2012

forestbond commented Sep 5, 2012

forestbond commented Sep 5, 2012

forestbond commented Jan 7, 2013

jefflasslett commented Jan 13, 2013

forestbond commented Jan 16, 2013

forestbond commented Feb 5, 2013

ghost commented Feb 5, 2013

forestbond commented Feb 5, 2013

tias commented Feb 7, 2013

forestbond commented Feb 11, 2013

Various fixes & improvements #36

Are you sure you want to change the base?

Various fixes & improvements #36

Conversation

forestbond commented May 22, 2012

tias commented Jun 7, 2012

forestbond commented Jun 25, 2012

forestbond commented Jul 26, 2012

Forest

tias commented Jul 31, 2012

forestbond commented Aug 21, 2012

Forest

forestbond commented Sep 5, 2012

forestbond commented Sep 5, 2012

forestbond commented Jan 7, 2013

jefflasslett commented Jan 13, 2013

forestbond commented Jan 16, 2013

forestbond commented Feb 5, 2013

ghost commented Feb 5, 2013

forestbond commented Feb 5, 2013

tias commented Feb 7, 2013

forestbond commented Feb 11, 2013