Re: [PATCH evdev] Fix absolute events with swapped axes
On Thu, Dec 15, 2011 at 12:33:51PM -0200, Paulo Zanoni wrote:
> Hi
>
> 2011/12/14 Chase Douglas :
> > Would it be better to remove the evdev axis swap property?
> >
> > If we still want it around, it would probably be easier to have it set
> > the transformation matrix under the covers. It would make the
> > transformation matrix and evdev axis swapping properties dependent, but
> > I doubt anyone is really wanting to mix the two.
>
> So I went and tried to use the transformation matrix code, but it also had a
> bug: a similar one, but slightly different. I wrote a patch to fix it, but I
> believe it changes the input ABI. I'll paste it in the end of the email (and
> gmail will probably mess with the white spaces). We should probably still
> signal
> the ABI change if needed. Or, if you know a better way to implement this,
> please
> tell me.
> While I was looking at this, I also tested the relative devices. It seems the
> evdev property had the same bug, so I fixed it and sent the patch. Then I
> tried
> to set a transformation matrix and it Just Didn't Work (tm): the
> matrix is ignored.
> For absolute devices we have the transformAbsolute function. Don't we
> also need a
> transformRelative one? Can't we use the same? We just can't remove the evdev
> property while we can't have transformation matrices for relative devices.
see http://lists.x.org/archives/xorg-devel/2011-May/022688.html, I should
still have those patches around somehere.
> And here is the patch. Depending on your comments I can send a next
> version using git-send-email.
not pretty, but makes sense. I wonder if there's a trick that we can use in
transformAbsolute to avoid having to store this but if we can't find one,
ACK to the principle. Please use a valuator mask though.
I also suspect that figuring out a way to use last.valuators in
untransformed coordinates will generate even more headache...
Cheers,
Peter
>
> diff --git a/dix/getevents.c b/dix/getevents.c
> index 8798f64..73cf72d 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -1078,12 +1078,12 @@ transformAbsolute(DeviceIntPtr dev, ValuatorMask
> *mask)
> if (valuator_mask_isset(mask, 0))
> ox = x = valuator_mask_get_double(mask, 0);
> else
> -ox = x = dev->last.valuators[0];
> +ox = x = dev->last.untransformedValuators[0];
>
> if (valuator_mask_isset(mask, 1))
> oy = y = valuator_mask_get_double(mask, 1);
> else
> -oy = y = dev->last.valuators[1];
> +oy = y = dev->last.untransformedValuators[1];
>
> transform(&dev->transform, &x, &y);
>
> @@ -1167,6 +1167,8 @@ fill_pointer_events(InternalEvent *events,
> DeviceIntPtr pDev, int type,
> RawDeviceEvent *raw;
> double screenx = 0.0, screeny = 0.0; /* desktop coordinate system */
> double devx = 0.0, devy = 0.0; /* desktop-wide in device coords */
> +double untransformedAxesValues[2] = {0.0, 0.0};
> +int untransformedAxesIsset[2] = {0, 0};
> ValuatorMask mask;
> ScreenPtr scr;
>
> @@ -1207,6 +1209,11 @@ fill_pointer_events(InternalEvent *events,
> DeviceIntPtr pDev, int type,
> set_raw_valuators(raw, &mask, raw->valuators.data_raw);
> }
>
> +untransformedAxesIsset[0] = valuator_mask_isset(&mask, 0);
> +untransformedAxesIsset[1] = valuator_mask_isset(&mask, 1);
> +untransformedAxesValues[0] = valuator_mask_get(&mask, 0);
> +untransformedAxesValues[1] = valuator_mask_get(&mask, 1);
> +
> /* valuators are in driver-native format (rel or abs) */
>
> if (flags & POINTER_ABSOLUTE)
> @@ -1242,6 +1249,10 @@ fill_pointer_events(InternalEvent *events,
> DeviceIntPtr pDev, int type,
> pDev->last.valuators[0] = devx;
> if (valuator_mask_isset(&mask, 1))
> pDev->last.valuators[1] = devy;
> +if (untransformedAxesIsset[0])
> +pDev->last.untransformedValuators[0] = untransformedAxesValues[0];
> +if (untransformedAxesIsset[1])
> +pDev->last.untransformedValuators[1] = untransformedAxesValues[1];
>
> for (i = 2; i < valuator_mask_size(&mask); i++)
> {
> diff --git a/include/inputstr.h b/include/inputstr.h
> index 5634f3c..a18fd23 100644
> --- a/include/inputstr.h
> +++ b/include/inputstr.h
> @@ -538,6 +538,7 @@ typedef struct _DeviceIntRec {
> * desktop
> * for master devices, valuators is in desktop coordinates.
> * see dix/getevents.c
> + * untransformedValuators is used by the transformation matrix property
> * remainder supports acceleration
> */
> struct {
> @@ -545,6 +546,7 @@ typedef struct _DeviceIntRec {
> int numValuators;
> DeviceIntPtrslave;
> ValuatorMask*scroll;
> +double untransformedValuators[2];
> } last;
>
> /* Input device property handling. */
>
>
> Cheers,
> Paulo
>
>
> --
> Paulo Zanoni
___
xorg-devel@lists.x.org: X.Org development
A
Re: [PATCH evdev] Fix absolute events with swapped axes
Hi
2011/12/14 Chase Douglas :
> Would it be better to remove the evdev axis swap property?
>
> If we still want it around, it would probably be easier to have it set
> the transformation matrix under the covers. It would make the
> transformation matrix and evdev axis swapping properties dependent, but
> I doubt anyone is really wanting to mix the two.
So I went and tried to use the transformation matrix code, but it also had a
bug: a similar one, but slightly different. I wrote a patch to fix it, but I
believe it changes the input ABI. I'll paste it in the end of the email (and
gmail will probably mess with the white spaces). We should probably still signal
the ABI change if needed. Or, if you know a better way to implement this, please
tell me.
While I was looking at this, I also tested the relative devices. It seems the
evdev property had the same bug, so I fixed it and sent the patch. Then I tried
to set a transformation matrix and it Just Didn't Work (tm): the
matrix is ignored.
For absolute devices we have the transformAbsolute function. Don't we
also need a
transformRelative one? Can't we use the same? We just can't remove the evdev
property while we can't have transformation matrices for relative devices.
And here is the patch. Depending on your comments I can send a next
version using git-send-email.
diff --git a/dix/getevents.c b/dix/getevents.c
index 8798f64..73cf72d 100644
--- a/dix/getevents.c
+++ b/dix/getevents.c
@@ -1078,12 +1078,12 @@ transformAbsolute(DeviceIntPtr dev, ValuatorMask *mask)
if (valuator_mask_isset(mask, 0))
ox = x = valuator_mask_get_double(mask, 0);
else
-ox = x = dev->last.valuators[0];
+ox = x = dev->last.untransformedValuators[0];
if (valuator_mask_isset(mask, 1))
oy = y = valuator_mask_get_double(mask, 1);
else
-oy = y = dev->last.valuators[1];
+oy = y = dev->last.untransformedValuators[1];
transform(&dev->transform, &x, &y);
@@ -1167,6 +1167,8 @@ fill_pointer_events(InternalEvent *events,
DeviceIntPtr pDev, int type,
RawDeviceEvent *raw;
double screenx = 0.0, screeny = 0.0; /* desktop coordinate system */
double devx = 0.0, devy = 0.0; /* desktop-wide in device coords */
+double untransformedAxesValues[2] = {0.0, 0.0};
+int untransformedAxesIsset[2] = {0, 0};
ValuatorMask mask;
ScreenPtr scr;
@@ -1207,6 +1209,11 @@ fill_pointer_events(InternalEvent *events,
DeviceIntPtr pDev, int type,
set_raw_valuators(raw, &mask, raw->valuators.data_raw);
}
+untransformedAxesIsset[0] = valuator_mask_isset(&mask, 0);
+untransformedAxesIsset[1] = valuator_mask_isset(&mask, 1);
+untransformedAxesValues[0] = valuator_mask_get(&mask, 0);
+untransformedAxesValues[1] = valuator_mask_get(&mask, 1);
+
/* valuators are in driver-native format (rel or abs) */
if (flags & POINTER_ABSOLUTE)
@@ -1242,6 +1249,10 @@ fill_pointer_events(InternalEvent *events,
DeviceIntPtr pDev, int type,
pDev->last.valuators[0] = devx;
if (valuator_mask_isset(&mask, 1))
pDev->last.valuators[1] = devy;
+if (untransformedAxesIsset[0])
+pDev->last.untransformedValuators[0] = untransformedAxesValues[0];
+if (untransformedAxesIsset[1])
+pDev->last.untransformedValuators[1] = untransformedAxesValues[1];
for (i = 2; i < valuator_mask_size(&mask); i++)
{
diff --git a/include/inputstr.h b/include/inputstr.h
index 5634f3c..a18fd23 100644
--- a/include/inputstr.h
+++ b/include/inputstr.h
@@ -538,6 +538,7 @@ typedef struct _DeviceIntRec {
* desktop
* for master devices, valuators is in desktop coordinates.
* see dix/getevents.c
+ * untransformedValuators is used by the transformation matrix property
* remainder supports acceleration
*/
struct {
@@ -545,6 +546,7 @@ typedef struct _DeviceIntRec {
int numValuators;
DeviceIntPtrslave;
ValuatorMask*scroll;
+double untransformedValuators[2];
} last;
/* Input device property handling. */
Cheers,
Paulo
--
Paulo Zanoni
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH evdev] Fix absolute events with swapped axes
On Wed, Dec 14, 2011 at 03:23:36PM -0200, przan...@gmail.com wrote:
> From: Paulo Zanoni
>
> We were correctly swapping the valuator values, but we were not
> calling valuator_mask_unset() when needed, so the cursor kept jumping
> to the edges.
>
> This patch does the swapping before the main "for", so we don't need to
> store unswapped_{x,y} and unswapped_isset_{x,y} even when we don't need
> to swap.
>
> Signed-off-by: Paulo Zanoni
applied, thanks.
Cheers,
Peter
> ---
> src/evdev.c | 31 ++-
> 1 files changed, 22 insertions(+), 9 deletions(-)
>
> Another solution to the problem would involve keeping the unswapped_x
> and unswapped_y variables, and also adding unswapped_isset_{x,y}, but I
> believe this one looks better because it just computes these values if
> swap_axes is actually set.
>
> How I tested:
> - disabled synaptics
> - configured my touchpad to report absolute events with evdev
>- Option "Mode" "Absolute"
> - xinput --set-prop "SynPS/2 Synaptics TouchPad" "Evdev Axes Swap" 1
>
> If you move your finger across the touchpad when the axes are inverted
> the cursor keeps jumping to the edges (because we set the valuators to 0
> when what we really wanted was to call valuator_mask_unset). After the
> patch, the cursor acts as we expect. Also, the edges of the touchpad
> match the edges of the screen (the values are scaled correctly).
>
> diff --git a/src/evdev.c b/src/evdev.c
> index 428d3c1..b1f9b2e 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -436,10 +436,30 @@ EvdevProcessValuators(InputInfoPtr pInfo)
> * just works.
> */
> else if (pEvdev->abs_queued && pEvdev->in_proximity) {
> -int unswapped_x = valuator_mask_get(pEvdev->vals, 0);
> -int unswapped_y = valuator_mask_get(pEvdev->vals, 1);
> int i;
>
> +if (pEvdev->swap_axes) {
> +int swapped_isset[2] = {0, 0};
> +int swapped_values[2];
> +
> +for(i = 0; i <= 1; i++)
> +if (valuator_mask_isset(pEvdev->vals, i)) {
> +swapped_isset[1 - i] = 1;
> +swapped_values[1 - i] =
> +xf86ScaleAxis(valuator_mask_get(pEvdev->vals, i),
> + pEvdev->absinfo[1 - i].maximum,
> + pEvdev->absinfo[1 - i].minimum,
> + pEvdev->absinfo[i].maximum,
> + pEvdev->absinfo[i].minimum);
> +}
> +
> +for (i = 0; i <= 1; i++)
> +if (swapped_isset[i])
> +valuator_mask_set(pEvdev->vals, i, swapped_values[i]);
> +else
> +valuator_mask_unset(pEvdev->vals, i);
> +}
> +
> for (i = 0; i <= 1; i++) {
> int val;
> int calib_min;
> @@ -458,13 +478,6 @@ EvdevProcessValuators(InputInfoPtr pInfo)
> calib_max = pEvdev->calibration.max_y;
> }
>
> -if (pEvdev->swap_axes)
> -val = xf86ScaleAxis((i == 0 ? unswapped_y : unswapped_x),
> -pEvdev->absinfo[i].maximum,
> -pEvdev->absinfo[i].minimum,
> -pEvdev->absinfo[1 - i].maximum,
> -pEvdev->absinfo[1 - i].minimum);
> -
> if (pEvdev->flags & EVDEV_CALIBRATED)
> val = xf86ScaleAxis(val, pEvdev->absinfo[i].maximum,
> pEvdev->absinfo[i].minimum, calib_max,
> --
> 1.7.7.3
>
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH evdev] Fix absolute events with swapped axes
On Wed, Dec 14, 2011 at 12:09:23PM -0800, Chase Douglas wrote:
> On 12/14/2011 09:23 AM, przan...@gmail.com wrote:
> > From: Paulo Zanoni
> >
> > We were correctly swapping the valuator values, but we were not
> > calling valuator_mask_unset() when needed, so the cursor kept jumping
> > to the edges.
> >
> > This patch does the swapping before the main "for", so we don't need to
> > store unswapped_{x,y} and unswapped_isset_{x,y} even when we don't need
> > to swap.
>
> Would it be better to remove the evdev axis swap property?
>
> If we still want it around, it would probably be easier to have it set
> the transformation matrix under the covers. It would make the
> transformation matrix and evdev axis swapping properties dependent, but
> I doubt anyone is really wanting to mix the two.
The problem with "transparent" is it's just that. We could hook into the
property but then those who need swapped axis and their device bound to one
screen only have their configurations break. and if we do it under the
covers, that's only going to be hard to fix.
With the matrix available, all this could be set in client programs but
afaik we don't have any of those yet. so we need to leave this option
around.
Cheers,
Peter
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH evdev] Fix absolute events with swapped axes
On 12/14/2011 09:23 AM, przan...@gmail.com wrote:
> From: Paulo Zanoni
>
> We were correctly swapping the valuator values, but we were not
> calling valuator_mask_unset() when needed, so the cursor kept jumping
> to the edges.
>
> This patch does the swapping before the main "for", so we don't need to
> store unswapped_{x,y} and unswapped_isset_{x,y} even when we don't need
> to swap.
Would it be better to remove the evdev axis swap property?
If we still want it around, it would probably be easier to have it set
the transformation matrix under the covers. It would make the
transformation matrix and evdev axis swapping properties dependent, but
I doubt anyone is really wanting to mix the two.
-- Chase
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH evdev] Fix absolute events with swapped axes
From: Paulo Zanoni
We were correctly swapping the valuator values, but we were not
calling valuator_mask_unset() when needed, so the cursor kept jumping
to the edges.
This patch does the swapping before the main "for", so we don't need to
store unswapped_{x,y} and unswapped_isset_{x,y} even when we don't need
to swap.
Signed-off-by: Paulo Zanoni
---
src/evdev.c | 31 ++-
1 files changed, 22 insertions(+), 9 deletions(-)
Another solution to the problem would involve keeping the unswapped_x
and unswapped_y variables, and also adding unswapped_isset_{x,y}, but I
believe this one looks better because it just computes these values if
swap_axes is actually set.
How I tested:
- disabled synaptics
- configured my touchpad to report absolute events with evdev
- Option "Mode" "Absolute"
- xinput --set-prop "SynPS/2 Synaptics TouchPad" "Evdev Axes Swap" 1
If you move your finger across the touchpad when the axes are inverted
the cursor keeps jumping to the edges (because we set the valuators to 0
when what we really wanted was to call valuator_mask_unset). After the
patch, the cursor acts as we expect. Also, the edges of the touchpad
match the edges of the screen (the values are scaled correctly).
diff --git a/src/evdev.c b/src/evdev.c
index 428d3c1..b1f9b2e 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -436,10 +436,30 @@ EvdevProcessValuators(InputInfoPtr pInfo)
* just works.
*/
else if (pEvdev->abs_queued && pEvdev->in_proximity) {
-int unswapped_x = valuator_mask_get(pEvdev->vals, 0);
-int unswapped_y = valuator_mask_get(pEvdev->vals, 1);
int i;
+if (pEvdev->swap_axes) {
+int swapped_isset[2] = {0, 0};
+int swapped_values[2];
+
+for(i = 0; i <= 1; i++)
+if (valuator_mask_isset(pEvdev->vals, i)) {
+swapped_isset[1 - i] = 1;
+swapped_values[1 - i] =
+xf86ScaleAxis(valuator_mask_get(pEvdev->vals, i),
+ pEvdev->absinfo[1 - i].maximum,
+ pEvdev->absinfo[1 - i].minimum,
+ pEvdev->absinfo[i].maximum,
+ pEvdev->absinfo[i].minimum);
+}
+
+for (i = 0; i <= 1; i++)
+if (swapped_isset[i])
+valuator_mask_set(pEvdev->vals, i, swapped_values[i]);
+else
+valuator_mask_unset(pEvdev->vals, i);
+}
+
for (i = 0; i <= 1; i++) {
int val;
int calib_min;
@@ -458,13 +478,6 @@ EvdevProcessValuators(InputInfoPtr pInfo)
calib_max = pEvdev->calibration.max_y;
}
-if (pEvdev->swap_axes)
-val = xf86ScaleAxis((i == 0 ? unswapped_y : unswapped_x),
-pEvdev->absinfo[i].maximum,
-pEvdev->absinfo[i].minimum,
-pEvdev->absinfo[1 - i].maximum,
-pEvdev->absinfo[1 - i].minimum);
-
if (pEvdev->flags & EVDEV_CALIBRATED)
val = xf86ScaleAxis(val, pEvdev->absinfo[i].maximum,
pEvdev->absinfo[i].minimum, calib_max,
--
1.7.7.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
