mbox series

[0/2] xpad, hid-sony: Report analog buttons

Message ID 20230826152111.13525-1-max@enpas.org
Headers show
Series xpad, hid-sony: Report analog buttons | expand

Message

Max Staudt Aug. 26, 2023, 3:21 p.m. UTC 
Dear input maintainers,

I would like to add support for pressure sensitive buttons on the
original Xbox gamepad, as well as the PlayStation 3 controllers.


In an attempt to maximise backwards compatibility, the attached patches
add the corresponding analog values for BTN_A..BTN_Z as ABS_MISC+0..+5,
L1/R1 as ABS_HAT1Y/HAT1X, and the D-Pad as ABS_HAT0X/Y.

All of these axes have higher indices than any axes previously exposed,
so gamepad mappings in SDL keep working. Also, where possible, I have
stuck to the Linux gamepad specification (for HAT0/HAT1).


Now, I am wondering what best to do with the action buttons, since the
Linux gamepad specification does not foresee them being analog. In the
patches, they are reported as ABS_MISC+0..+5 - do you think this is
reasonable, or would a new ABS_* range at 0x40.. be better suited to
this task?


I'd appreciate your thoughts on the patches and on how to best add
analog buttons to the drivers!


Thanks,

Max


Patches included:
  [PATCH 1/2] xpad: XTYPE_XBOX: Report analog buttons
  [PATCH 2/2] hid-sony: DS3: Report analog buttons for Sixaxis

Comments

Rahul Rameshbabu Aug. 26, 2023, 4:52 p.m. UTC | #1 
Hi,

You will want to update the commit message subject to use the prefix
"Input: xpad -" instead of "xpad:".

  https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/drivers/input/joystick/xpad.c

On Sun, 27 Aug, 2023 00:21:10 +0900 "Max Staudt" <max@enpas.org> wrote:
> The original Xbox controllers (XTYPE_XBOX) report 8 buttons in an analog
> fashion, in addition to the digital on/off state:
>
>  - Action buttons A/B/X/Y/black/white
>  - Triggers L/R
>
> Up until now, only the triggers L/R are reported as values 0-255. The
> other pressure sensitive buttons are reported as digital buttons, as
> found on other controllers.
>
> This change exposes these buttons as axes in a way that is as backwards
> compatible as possible.
> The new axes are merely added, and numbered after any existing axes.
> This way, libraries like SDL which renumber axes in enumeration order,
> can keep their button/axis mapping as-is. Userspace can keep working as
> before, and can optionally use the new values when handling this type of
> gamepad.

FWIW, I like the way you handled adding support for the range of the
analog buttons.

>
>  - BTN_A..BTN_Z mapped to ABS_MISC+0..ABS_MISC+5, 0 to 255
>
> Signed-off-by: Max Staudt <max@enpas.org>
> ---
>  drivers/input/joystick/xpad.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index cdb193317c3b..609c06f795de 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -420,6 +420,14 @@ static const signed short xpad_abs_triggers[] = {
>  	-1
>  };
>
> +/* used for analog face buttons mapped to axes */
> +static const signed short xpad_abs_analog_face_buttons[] = {
> +	ABS_MISC + 0, ABS_MISC + 1, /* A, B */
> +	ABS_MISC + 3, ABS_MISC + 4, /* X, Y */
> +	ABS_MISC + 2, ABS_MISC + 5, /* C, Z */
> +	-1
> +};

Would it make more sense to use an enum for this?
Something like the below enum.

  enum xpad_abs_analog_face_btn {
       XPAD_ABS_ANALOG_FACE_BTN_A = ABS_MISC,
       XPAD_ABS_ANALOG_FACE_BTN_B,
       XPAD_ABS_ANALOG_FACE_BTN_C,
       XPAD_ABS_ANALOG_FACE_BTN_X,
       XPAD_ABS_ANALOG_FACE_BTN_Y,
       XPAD_ABS_ANALOG_FACE_BTN_Z,
       XPAD_ABS_ANALOG_FACE_BTN_END, /* Must remain as the last element */
  };

This would clean up both xpad_process_packet and xpad_set_up_abs a bit
in my opinion. Your loop for xpad_set_up_abs would look like the
following.

  enum xpad_abs_analog_face_btn btn;

  ...

  for (btn = XPAD_ABS_ANALOG_FACE_BTN_A; btn != XPAD_ABS_ANALOG_FACE_BTN_END; ++btn)
          xpad_set_up_abs(input_dev, btn);

> +
>  /* used when the controller has extra paddle buttons */
>  static const signed short xpad_btn_paddles[] = {
>  	BTN_TRIGGER_HAPPY5, BTN_TRIGGER_HAPPY6, /* paddle upper right, lower right */
> @@ -784,6 +792,15 @@ static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *d
>  	input_report_key(dev, BTN_C, data[8]);
>  	input_report_key(dev, BTN_Z, data[9]);
>
> +	/* analog buttons A, B, X, Y as axes */
> +	input_report_abs(dev, xpad_abs_analog_face_buttons[0], data[4]); /* A */
> +	input_report_abs(dev, xpad_abs_analog_face_buttons[1], data[5]); /* B */
> +	input_report_abs(dev, xpad_abs_analog_face_buttons[2], data[6]); /* X */
> +	input_report_abs(dev, xpad_abs_analog_face_buttons[3], data[7]); /* Y */
> +
> +	/* analog buttons black, white (C, Z) as axes */
> +	input_report_abs(dev, xpad_abs_analog_face_buttons[4], data[8]); /* C */
> +	input_report_abs(dev, xpad_abs_analog_face_buttons[5], data[9]); /* Z */
>
>  	input_sync(dev);
>  }
> @@ -1827,6 +1844,14 @@ static void xpad_set_up_abs(struct input_dev *input_dev, signed short abs)
>  	case ABS_HAT0Y:	/* the d-pad (only if dpad is mapped to axes */
>  		input_set_abs_params(input_dev, abs, -1, 1, 0, 0);
>  		break;
> +	case ABS_MISC + 0:
> +	case ABS_MISC + 1:
> +	case ABS_MISC + 2:
> +	case ABS_MISC + 3:
> +	case ABS_MISC + 4:
> +	case ABS_MISC + 5:
> +		input_set_abs_params(input_dev, abs, 0, 255, 0, 0);
> +		break;
>  	case ABS_PROFILE: /* 4 value profile button (such as on XAC) */
>  		input_set_abs_params(input_dev, abs, 0, 4, 0, 0);
>  		break;
> @@ -1928,6 +1953,10 @@ static int xpad_init_input(struct usb_xpad *xpad)
>  			xpad_set_up_abs(input_dev, xpad_abs_triggers[i]);
>  	}
>
> +	if (xpad->xtype == XTYPE_XBOX)
> +		for (i = 0; xpad_abs_analog_face_buttons[i] >= 0; i++)
> +			xpad_set_up_abs(input_dev, xpad_abs_analog_face_buttons[i]);
> +
>  	/* setup profile button as an axis with 4 possible values */
>  	if (xpad->mapping & MAP_PROFILE_BUTTON)
>  		xpad_set_up_abs(input_dev, ABS_PROFILE);

--
Thanks,

Rahul Rameshbabu
Max Staudt Aug. 27, 2023, 3:07 p.m. UTC | #2 
On 8/27/23 01:52, Rahul Rameshbabu wrote:
> You will want to update the commit message subject to use the prefix
> "Input: xpad -" instead of "xpad:".

Thanks, will do!


>> +/* used for analog face buttons mapped to axes */
>> +static const signed short xpad_abs_analog_face_buttons[] = {
>> +	ABS_MISC + 0, ABS_MISC + 1, /* A, B */
>> +	ABS_MISC + 3, ABS_MISC + 4, /* X, Y */
>> +	ABS_MISC + 2, ABS_MISC + 5, /* C, Z */
>> +	-1
>> +};
> 
> Would it make more sense to use an enum for this?
> Something like the below enum.
> 
>    enum xpad_abs_analog_face_btn {
>         XPAD_ABS_ANALOG_FACE_BTN_A = ABS_MISC,
>         XPAD_ABS_ANALOG_FACE_BTN_B,
>         XPAD_ABS_ANALOG_FACE_BTN_C,
>         XPAD_ABS_ANALOG_FACE_BTN_X,
>         XPAD_ABS_ANALOG_FACE_BTN_Y,
>         XPAD_ABS_ANALOG_FACE_BTN_Z,
>         XPAD_ABS_ANALOG_FACE_BTN_END, /* Must remain as the last element */
>    };
> 
> This would clean up both xpad_process_packet and xpad_set_up_abs a bit
> in my opinion. Your loop for xpad_set_up_abs would look like the
> following.
> 
>    enum xpad_abs_analog_face_btn btn;
> 
>    ...
> 
>    for (btn = XPAD_ABS_ANALOG_FACE_BTN_A; btn != XPAD_ABS_ANALOG_FACE_BTN_END; ++btn)
>            xpad_set_up_abs(input_dev, btn);

I agree, that looks cleaner.

Since it's a step closer to standardising a mapping for those analog buttons, I'd like to wait and see whether there is a consensus across drivers and maintainers. Maybe we can include something like this enum in input-event-codes.h and have a really clean solution.



Thanks!

Max