Various fixes & improvements

[forestbond]

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]

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 #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]

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 #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: https://github.com/tias/xinput_calibrator/pull/36#issuecomment-6191286

[forestbond]

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 #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]

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 #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]

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

[forestbond]

Hi Tias,

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

Thanks, Forest

[forestbond]

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]

Hi Tias,

Did you have a chance to look at these changes?

Thanks, Forest

[jefflasslett]

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]

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]

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]

Hello again,

So, the bug I encountered is here https://github.com/forest-bond/xinput_calibrator/commit/ee60066e18de066e00e097d2e0b430ead151f62b#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]

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

[tias]

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

[forestbond]

Hi Tias,

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

Thanks, Forest