[PATCH 1/2] joydev: Map ABS_{THROTTLE,GAS,BREAK} to positive values.

[PATCH 1/2] joydev: Map ABS_{THROTTLE,GAS,BREAK} to positive values.

[Benjamin Franzke]

Gamepads like XInput devices have triggers with a range from
e.g. 0 to 255.
When the conventional joydev AXES correction/mapping method is used,
the default state 0 (i.e. when the user does not press a button or trigger)
is mapped to -32767 (and 255 to 32767).
You'll get 0 only when pressing the trigger half-way down.
This has several drawbacks:
 - A trigger is not usable at all when configured to have 2 directions
   when there is physically only one.
 - Applications that let the user configure joystick input assignments
   may register the mapped non-zero default state as a press,
   preventing the user from configuring any sensible value.

Traditonal calibration e.g. with jscal does not help to get a usable
experience either, it'll map the default state to -32767 as well.
Only manually editing the calibration data does work currently.

This patch tries to fix this issue by calculating a positive-only range for
the throttle break and gas ABS bits.
Maybe other ABS types need to be added to this scheme as well(?)

Note:
Although joydev is considered obsolete and people are encouraged to just
use evdev, joydev should still be fixed, because
 1. many applications still use it.
 2. userspace already copies the joydev correction code to evdev (e.g. libSDL).

This bug should be fixed here in joydev first, and when we have a good
sensible state, this should be propagated to the application/libraries
that copied the joydev correction code.

Gamepad drivers will need to advertise triggers as THROTTLE,GAS,BREAK instead
of e.g. ABS_{RZ,Z} where phyisically appropriate, to make use of this.

I've considered changing the current joydev correction calculation for all ABS
types, to have only a scaling functionality by default, no translation
(all positive values remain positive, negatives negative and zero remains zero),
so that drivers would need to expose zero-centered ABS values.
It turned out to many drivers would need to be adjusted, which i cant
test.

Signed-off-by: Benjamin Franzke <benjaminfranzke@googlemail.com>
---
 drivers/input/joydev.c | 59 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 44 insertions(+), 15 deletions(-)

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index f362883..8352bee 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -761,11 +761,50 @@ static bool joydev_match(struct input_handler *handler, struct input_dev *dev)
 	return true;
 }
 
+static void joydev_calculate_correction(struct input_dev *dev, __u8 abs,
+					struct js_corr *corr)
+{
+	int t;
+
+	corr->type = JS_CORR_BROKEN;
+	corr->prec = input_abs_get_fuzz(dev, abs);
+
+	switch (abs) {
+	/* Map [min - max] => [0 - 32767]. */
+	case ABS_THROTTLE:
+	case ABS_GAS:
+	case ABS_BRAKE:
+		corr->coef[0] = input_abs_get_min(dev, abs);
+		corr->coef[1] = corr->coef[0] + input_abs_get_flat(dev, abs);
+		corr->coef[2] = 0;
+
+		t = input_abs_get_max(dev, abs) - input_abs_get_min(dev, abs)
+			- input_abs_get_flat(dev, abs);
+		if (t)
+			corr->coef[3] = (1 << 29) / t;
+		break;
+	/* Map [min - max] => [-32768 - 32767]. */
+	default:
+		t = (input_abs_get_max(dev, abs) +
+		     input_abs_get_min(dev, abs)) / 2;
+		corr->coef[0] = t - input_abs_get_flat(dev, abs);
+		corr->coef[1] = t + input_abs_get_flat(dev, abs);
+		t = (input_abs_get_max(dev, abs) -
+		     input_abs_get_min(dev, abs)) / 2
+			- 2 * input_abs_get_flat(dev, abs);
+		if (t) {
+			corr->coef[2] = (1 << 29) / t;
+			corr->coef[3] = (1 << 29) / t;
+		}
+		break;
+	}
+}
+
 static int joydev_connect(struct input_handler *handler, struct input_dev *dev,
 			  const struct input_device_id *id)
 {
 	struct joydev *joydev;
-	int i, j, t, minor, dev_no;
+	int i, j, minor, dev_no;
 	int error;
 
 	minor = input_get_new_minor(JOYDEV_MINOR_BASE, JOYDEV_MINORS, true);
@@ -826,23 +865,13 @@ static int joydev_connect(struct input_handler *handler, struct input_dev *dev,
 			joydev->abs[i] = input_abs_get_val(dev, j);
 			continue;
 		}
-		joydev->corr[i].type = JS_CORR_BROKEN;
-		joydev->corr[i].prec = input_abs_get_fuzz(dev, j);
 
-		t = (input_abs_get_max(dev, j) + input_abs_get_min(dev, j)) / 2;
-		joydev->corr[i].coef[0] = t - input_abs_get_flat(dev, j);
-		joydev->corr[i].coef[1] = t + input_abs_get_flat(dev, j);
+		joydev_calculate_correction(dev, j, &joydev->corr[i]);
 
-		t = (input_abs_get_max(dev, j) - input_abs_get_min(dev, j)) / 2
-			- 2 * input_abs_get_flat(dev, j);
-		if (t) {
-			joydev->corr[i].coef[2] = (1 << 29) / t;
-			joydev->corr[i].coef[3] = (1 << 29) / t;
+		joydev->abs[i] =
+			joydev_correct(input_abs_get_val(dev, j),
+				       joydev->corr + i);
 
-			joydev->abs[i] =
-				joydev_correct(input_abs_get_val(dev, j),
-					       joydev->corr + i);
-		}
 	}
 
 	joydev->dev.devt = MKDEV(INPUT_MAJOR, minor);
-- 
1.8.1.5

[PATCH 2/2] joystick/xpad: Advertise the triggers as throttle/break

[Benjamin Franzke]

They are considered positive only axes, so userspace and joydev can
handle them correctly.
It also fits semantically better, since these triggers are really intended
for throttle/break.

Signed-off-by: Benjamin Franzke <benjaminfranzke@googlemail.com>
---
 drivers/input/joystick/xpad.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 75e3b10..4e9de34 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -226,7 +226,7 @@ static const signed short xpad_abs_pad[] = {
 
 /* used when triggers are mapped to axes */
 static const signed short xpad_abs_triggers[] = {
-	ABS_Z, ABS_RZ,		/* triggers left/right */
+	ABS_BRAKE, ABS_THROTTLE, /* triggers left/right */
 	-1
 };
 
@@ -327,8 +327,8 @@ static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *d
 		input_report_key(dev, BTN_TL2, data[10]);
 		input_report_key(dev, BTN_TR2, data[11]);
 	} else {
-		input_report_abs(dev, ABS_Z, data[10]);
-		input_report_abs(dev, ABS_RZ, data[11]);
+		input_report_abs(dev, ABS_BRAKE, data[10]);
+		input_report_abs(dev, ABS_THROTTLE, data[11]);
 	}
 
 	/* digital pad */
@@ -429,8 +429,8 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
 		input_report_key(dev, BTN_TL2, data[4]);
 		input_report_key(dev, BTN_TR2, data[5]);
 	} else {
-		input_report_abs(dev, ABS_Z, data[4]);
-		input_report_abs(dev, ABS_RZ, data[5]);
+		input_report_abs(dev, ABS_BRAKE, data[4]);
+		input_report_abs(dev, ABS_THROTTLE, data[5]);
 	}
 
 	input_sync(dev);
@@ -824,8 +824,8 @@ static void xpad_set_up_abs(struct input_dev *input_dev, signed short abs)
 	case ABS_RY:	/* the two sticks */
 		input_set_abs_params(input_dev, abs, -32768, 32767, 16, 128);
 		break;
-	case ABS_Z:
-	case ABS_RZ:	/* the triggers (if mapped to axes) */
+	case ABS_BRAKE:
+	case ABS_THROTTLE: /* the triggers (if mapped to axes) */
 		input_set_abs_params(input_dev, abs, 0, 255, 0, 0);
 		break;
 	case ABS_HAT0X:
-- 
1.8.1.5

Re: [PATCH 1/2] joydev: Map ABS_{THROTTLE,GAS,BREAK} to positive values.

[Ken Phillis Jr]

On Wed, Dec 11, 2013 at 2:15 AM, Benjamin Franzke
<benjaminfranzke@googlemail.com> wrote:
> Gamepads like XInput devices have triggers with a range from
> e.g. 0 to 255.
> When the conventional joydev AXES correction/mapping method is used,
> the default state 0 (i.e. when the user does not press a button or trigger)
> is mapped to -32767 (and 255 to 32767).
> You'll get 0 only when pressing the trigger half-way down.
> This has several drawbacks:
>  - A trigger is not usable at all when configured to have 2 directions
>    when there is physically only one.
>  - Applications that let the user configure joystick input assignments
>    may register the mapped non-zero default state as a press,
>    preventing the user from configuring any sensible value.
>
> Traditonal calibration e.g. with jscal does not help to get a usable
> experience either, it'll map the default state to -32767 as well.
> Only manually editing the calibration data does work currently.
>
> This patch tries to fix this issue by calculating a positive-only range for
> the throttle break and gas ABS bits.
> Maybe other ABS types need to be added to this scheme as well(?)
>
> Note:
> Although joydev is considered obsolete and people are encouraged to just
> use evdev, joydev should still be fixed, because
>  1. many applications still use it.
>  2. userspace already copies the joydev correction code to evdev (e.g. libSDL).
>
> This bug should be fixed here in joydev first, and when we have a good
> sensible state, this should be propagated to the application/libraries
> that copied the joydev correction code.
>
> Gamepad drivers will need to advertise triggers as THROTTLE,GAS,BREAK instead
> of e.g. ABS_{RZ,Z} where phyisically appropriate, to make use of this.
>
> I've considered changing the current joydev correction calculation for all ABS
> types, to have only a scaling functionality by default, no translation
> (all positive values remain positive, negatives negative and zero remains zero),
> so that drivers would need to expose zero-centered ABS values.
> It turned out to many drivers would need to be adjusted, which i cant
> test.
>
> Signed-off-by: Benjamin Franzke <benjaminfranzke@googlemail.com>


I have serious concerns that this Patch and the Next patch will the
input detection of the triggers on various games powered by SDL2. Have
you tested this against games listed on steam that have full
controller support?

Re: [PATCH 1/2] joydev: Map ABS_{THROTTLE,GAS,BREAK} to positive values.

[Benjamin Franzke]

I didnt test with games on steam.
I've tested with supertuxkart 0.8.1 and jstest.

Could you give me a hint, why you think that it breaks(?) input detection?

I think SDL2 used evdev, so how should a changed joydev break the
correction? (or do you mean in joydev fallback mode?)
Searching through the SDL code I guess you refer to this:
 http://hg.libsdl.org/SDL/file/a5f8137ccf01/src/joystick/SDL_gamecontroller.c#l133
This really looks like a workaround for this brokeness of the joydev
correction for triggers,
thats why this should be fixed in kernel.

It cant really be inteded to map triggers to -32767-32767, and
consider this API to not brake application that have workarounds for
this.
Or is it?

2013/12/14 Ken Phillis Jr <kphillisjr@gmail.com>:
> On Wed, Dec 11, 2013 at 2:15 AM, Benjamin Franzke
> <benjaminfranzke@googlemail.com> wrote:
>> Gamepads like XInput devices have triggers with a range from
>> e.g. 0 to 255.
>> When the conventional joydev AXES correction/mapping method is used,
>> the default state 0 (i.e. when the user does not press a button or trigger)
>> is mapped to -32767 (and 255 to 32767).
>> You'll get 0 only when pressing the trigger half-way down.
>> This has several drawbacks:
>>  - A trigger is not usable at all when configured to have 2 directions
>>    when there is physically only one.
>>  - Applications that let the user configure joystick input assignments
>>    may register the mapped non-zero default state as a press,
>>    preventing the user from configuring any sensible value.
>>
>> Traditonal calibration e.g. with jscal does not help to get a usable
>> experience either, it'll map the default state to -32767 as well.
>> Only manually editing the calibration data does work currently.
>>
>> This patch tries to fix this issue by calculating a positive-only range for
>> the throttle break and gas ABS bits.
>> Maybe other ABS types need to be added to this scheme as well(?)
>>
>> Note:
>> Although joydev is considered obsolete and people are encouraged to just
>> use evdev, joydev should still be fixed, because
>>  1. many applications still use it.
>>  2. userspace already copies the joydev correction code to evdev (e.g. libSDL).
>>
>> This bug should be fixed here in joydev first, and when we have a good
>> sensible state, this should be propagated to the application/libraries
>> that copied the joydev correction code.
>>
>> Gamepad drivers will need to advertise triggers as THROTTLE,GAS,BREAK instead
>> of e.g. ABS_{RZ,Z} where phyisically appropriate, to make use of this.
>>
>> I've considered changing the current joydev correction calculation for all ABS
>> types, to have only a scaling functionality by default, no translation
>> (all positive values remain positive, negatives negative and zero remains zero),
>> so that drivers would need to expose zero-centered ABS values.
>> It turned out to many drivers would need to be adjusted, which i cant
>> test.
>>
>> Signed-off-by: Benjamin Franzke <benjaminfranzke@googlemail.com>
>
>
> I have serious concerns that this Patch and the Next patch will the
> input detection of the triggers on various games powered by SDL2. Have
> you tested this against games listed on steam that have full
> controller support?

Re: [PATCH 1/2] joydev: Map ABS_{THROTTLE,GAS,BREAK} to positive values.

[Ken Phillis Jr]

On Sat, Dec 14, 2013 at 12:22 AM, Benjamin Franzke
<benjaminfranzke@googlemail.com> wrote:
>
> I didnt test with games on steam.
> I've tested with supertuxkart 0.8.1 and jstest.
>
> Could you give me a hint, why you think that it breaks(?) input detection?
>
> I think SDL2 used evdev, so how should a changed joydev break the
> correction? (or do you mean in joydev fallback mode?)
> Searching through the SDL code I guess you refer to this:
>  http://hg.libsdl.org/SDL/file/a5f8137ccf01/src/joystick/SDL_gamecontroller.c#l133
> This really looks like a workaround for this brokeness of the joydev
> correction for triggers,
> thats why this should be fixed in kernel.
>
> It cant really be inteded to map triggers to -32767-32767, and
> consider this API to not brake application that have workarounds for
> this.
> Or is it?
>
>



I believe the more appropriate work around would be to have the
appropriate mappings setting that are dependent on the device type.
This is to help improve overall interoperability. I know that most
xbox 360 devices can directly define the device type with the
descriptors. That said, Two of the mappings that can work are
MAP_FLIGHT_STICK and  MAP_RACING_WHEEL.

In the case of the MAP_RACING_WHEEL the following look reasonable enough...
ABS_WHEEL ( X axis on Left stick )
ABS_GAS  ( Left trigger )
ABS_BRAKE ( Right Trigger )

in the case of MAP_FLIGHT_STICK, the following is needed...
PITCH/ROLL - Axis of Left stick
POV Hat - Right stick
Rudder - Left Trigger
Throttle - Right Trigger.

Re: [PATCH 1/2] joydev: Map ABS_{THROTTLE,GAS,BREAK} to positive values.

[Benjamin Franzke]

2013/12/14 Ken Phillis Jr <kphillisjr@gmail.com>:
> On Sat, Dec 14, 2013 at 12:22 AM, Benjamin Franzke
> <benjaminfranzke@googlemail.com> wrote:
>>
>> I didnt test with games on steam.
>> I've tested with supertuxkart 0.8.1 and jstest.
>>
>> Could you give me a hint, why you think that it breaks(?) input detection?
>>
>> I think SDL2 used evdev, so how should a changed joydev break the
>> correction? (or do you mean in joydev fallback mode?)
>> Searching through the SDL code I guess you refer to this:
>>  http://hg.libsdl.org/SDL/file/a5f8137ccf01/src/joystick/SDL_gamecontroller.c#l133
>> This really looks like a workaround for this brokeness of the joydev
>> correction for triggers,
>> thats why this should be fixed in kernel.
>>
>> It cant really be inteded to map triggers to -32767-32767, and
>> consider this API to not brake application that have workarounds for
>> this.
>> Or is it?
>>
>>
>
>
>
> I believe the more appropriate work around would be to have the
> appropriate mappings setting that are dependent on the device type.
> This is to help improve overall interoperability. I know that most
> xbox 360 devices can directly define the device type with the
> descriptors. That said, Two of the mappings that can work are
> MAP_FLIGHT_STICK and  MAP_RACING_WHEEL.
>
> In the case of the MAP_RACING_WHEEL the following look reasonable enough...
> ABS_WHEEL ( X axis on Left stick )
> ABS_GAS  ( Left trigger )
> ABS_BRAKE ( Right Trigger )

I guess you mean left->brake and right->gas, thats how it is usually used.
I agree that GAS is better suited in combination with BRAKE.
>
> in the case of MAP_FLIGHT_STICK, the following is needed...
> PITCH/ROLL - Axis of Left stick
> POV Hat - Right stick
> Rudder - Left Trigger
> Throttle - Right Trigger.

I think this profiles can be another optimization, but they'll still
need an adjusted default-correction in joydev.
An alternative to these map types would probably be just generic bits
like ABS_TRIGGER_*?

> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] joydev: Map ABS_{THROTTLE,GAS,BREAK} to positive values.

[Benjamin Franzke]

Hi List,

may I ask for some (other) comments that actually help fix this bug?
Some vague concerns dont really help.

Thanks, Ben



2013/12/11 Benjamin Franzke <benjaminfranzke@googlemail.com>:
> Gamepads like XInput devices have triggers with a range from
> e.g. 0 to 255.
> When the conventional joydev AXES correction/mapping method is used,
> the default state 0 (i.e. when the user does not press a button or trigger)
> is mapped to -32767 (and 255 to 32767).
> You'll get 0 only when pressing the trigger half-way down.
> This has several drawbacks:
>  - A trigger is not usable at all when configured to have 2 directions
>    when there is physically only one.
>  - Applications that let the user configure joystick input assignments
>    may register the mapped non-zero default state as a press,
>    preventing the user from configuring any sensible value.
>
> Traditonal calibration e.g. with jscal does not help to get a usable
> experience either, it'll map the default state to -32767 as well.
> Only manually editing the calibration data does work currently.
>
> This patch tries to fix this issue by calculating a positive-only range for
> the throttle break and gas ABS bits.
> Maybe other ABS types need to be added to this scheme as well(?)
>
> Note:
> Although joydev is considered obsolete and people are encouraged to just
> use evdev, joydev should still be fixed, because
>  1. many applications still use it.
>  2. userspace already copies the joydev correction code to evdev (e.g. libSDL).
>
> This bug should be fixed here in joydev first, and when we have a good
> sensible state, this should be propagated to the application/libraries
> that copied the joydev correction code.
>
> Gamepad drivers will need to advertise triggers as THROTTLE,GAS,BREAK instead
> of e.g. ABS_{RZ,Z} where phyisically appropriate, to make use of this.
>
> I've considered changing the current joydev correction calculation for all ABS
> types, to have only a scaling functionality by default, no translation
> (all positive values remain positive, negatives negative and zero remains zero),
> so that drivers would need to expose zero-centered ABS values.
> It turned out to many drivers would need to be adjusted, which i cant
> test.
>
> Signed-off-by: Benjamin Franzke <benjaminfranzke@googlemail.com>
> ---
>  drivers/input/joydev.c | 59 +++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 44 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
> index f362883..8352bee 100644
> --- a/drivers/input/joydev.c
> +++ b/drivers/input/joydev.c
> @@ -761,11 +761,50 @@ static bool joydev_match(struct input_handler *handler, struct input_dev *dev)
>         return true;
>  }
>
> +static void joydev_calculate_correction(struct input_dev *dev, __u8 abs,
> +                                       struct js_corr *corr)
> +{
> +       int t;
> +
> +       corr->type = JS_CORR_BROKEN;
> +       corr->prec = input_abs_get_fuzz(dev, abs);
> +
> +       switch (abs) {
> +       /* Map [min - max] => [0 - 32767]. */
> +       case ABS_THROTTLE:
> +       case ABS_GAS:
> +       case ABS_BRAKE:
> +               corr->coef[0] = input_abs_get_min(dev, abs);
> +               corr->coef[1] = corr->coef[0] + input_abs_get_flat(dev, abs);
> +               corr->coef[2] = 0;
> +
> +               t = input_abs_get_max(dev, abs) - input_abs_get_min(dev, abs)
> +                       - input_abs_get_flat(dev, abs);
> +               if (t)
> +                       corr->coef[3] = (1 << 29) / t;
> +               break;
> +       /* Map [min - max] => [-32768 - 32767]. */
> +       default:
> +               t = (input_abs_get_max(dev, abs) +
> +                    input_abs_get_min(dev, abs)) / 2;
> +               corr->coef[0] = t - input_abs_get_flat(dev, abs);
> +               corr->coef[1] = t + input_abs_get_flat(dev, abs);
> +               t = (input_abs_get_max(dev, abs) -
> +                    input_abs_get_min(dev, abs)) / 2
> +                       - 2 * input_abs_get_flat(dev, abs);
> +               if (t) {
> +                       corr->coef[2] = (1 << 29) / t;
> +                       corr->coef[3] = (1 << 29) / t;
> +               }
> +               break;
> +       }
> +}
> +
>  static int joydev_connect(struct input_handler *handler, struct input_dev *dev,
>                           const struct input_device_id *id)
>  {
>         struct joydev *joydev;
> -       int i, j, t, minor, dev_no;
> +       int i, j, minor, dev_no;
>         int error;
>
>         minor = input_get_new_minor(JOYDEV_MINOR_BASE, JOYDEV_MINORS, true);
> @@ -826,23 +865,13 @@ static int joydev_connect(struct input_handler *handler, struct input_dev *dev,
>                         joydev->abs[i] = input_abs_get_val(dev, j);
>                         continue;
>                 }
> -               joydev->corr[i].type = JS_CORR_BROKEN;
> -               joydev->corr[i].prec = input_abs_get_fuzz(dev, j);
>
> -               t = (input_abs_get_max(dev, j) + input_abs_get_min(dev, j)) / 2;
> -               joydev->corr[i].coef[0] = t - input_abs_get_flat(dev, j);
> -               joydev->corr[i].coef[1] = t + input_abs_get_flat(dev, j);
> +               joydev_calculate_correction(dev, j, &joydev->corr[i]);
>
> -               t = (input_abs_get_max(dev, j) - input_abs_get_min(dev, j)) / 2
> -                       - 2 * input_abs_get_flat(dev, j);
> -               if (t) {
> -                       joydev->corr[i].coef[2] = (1 << 29) / t;
> -                       joydev->corr[i].coef[3] = (1 << 29) / t;
> +               joydev->abs[i] =
> +                       joydev_correct(input_abs_get_val(dev, j),
> +                                      joydev->corr + i);
>
> -                       joydev->abs[i] =
> -                               joydev_correct(input_abs_get_val(dev, j),
> -                                              joydev->corr + i);
> -               }
>         }
>
>         joydev->dev.devt = MKDEV(INPUT_MAJOR, minor);
> --
> 1.8.1.5
>