[3/3] drivers: hid: G940 FF structure & autocenter fixes
diff mbox series

Message ID 20190311212041.GA25630@nova.chris.boyle.name
State New
Delegated to: Jiri Kosina
Headers show
Series
  • [1/3] drivers: hid: fix G940 axis/button mappings
Related show

Commit Message

Chris Boyle March 11, 2019, 9:20 p.m. UTC
Disable default auto-centering of the Logitech G940, which otherwise
makes the stick difficult to use. Also when autocenter is requested, obey
the magnitude.

The auto-centering effect will only unlock (or become the requested
level) when an application starts using the joystick and the hand sensor
on the stick is covered. That appears to be a hardware limitation.

To make the above and future work on spring/damper support simpler and
more type-safe, add a struct of the HID report. Many thanks to fred41 from
forums.x-plane.org for useful information towards that.

It seems byte 0 does not need to be a "command byte" of 0x51; it can be
anything and is the least-significant byte of the constant force.

Signed-off-by: Chris Boyle <chris@boyle.name>
---
 drivers/hid/hid-lg3ff.c | 148 +++++++++++++++++++++-------------------
 1 file changed, 78 insertions(+), 70 deletions(-)

Comments

Jiri Kosina March 19, 2019, 1:48 p.m. UTC | #1
[ CCing Gary Stein ]

On Mon, 11 Mar 2019, Chris Boyle wrote:

> Disable default auto-centering of the Logitech G940, which otherwise
> makes the stick difficult to use. Also when autocenter is requested, obey
> the magnitude.
> 
> The auto-centering effect will only unlock (or become the requested
> level) when an application starts using the joystick and the hand sensor
> on the stick is covered. That appears to be a hardware limitation.
> 
> To make the above and future work on spring/damper support simpler and
> more type-safe, add a struct of the HID report. Many thanks to fred41 from
> forums.x-plane.org for useful information towards that.
> 
> It seems byte 0 does not need to be a "command byte" of 0x51; it can be
> anything and is the least-significant byte of the constant force.
> 
> Signed-off-by: Chris Boyle <chris@boyle.name>
> ---
>  drivers/hid/hid-lg3ff.c | 148 +++++++++++++++++++++-------------------
>  1 file changed, 78 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/hid/hid-lg3ff.c b/drivers/hid/hid-lg3ff.c
> index 8c2da183d3bc..6fcd449d5b46 100644
> --- a/drivers/hid/hid-lg3ff.c
> +++ b/drivers/hid/hid-lg3ff.c
> @@ -2,6 +2,7 @@
>   *  Force feedback support for Logitech Flight System G940
>   *
>   *  Copyright (c) 2009 Gary Stein <LordCnidarian@gmail.com>
> + *  Copyright (c) 2019 Chris Boyle
>   */
>  
>  /*
> @@ -26,51 +27,68 @@
>  
>  #include "hid-lg.h"
>  
> -/*
> - * G940 Theory of Operation (from experimentation)
> - *
> - * There are 63 fields (only 3 of them currently used)
> - * 0 - seems to be command field
> - * 1 - 30 deal with the x axis
> - * 31 -60 deal with the y axis
> - *
> - * Field 1 is x axis constant force
> - * Field 31 is y axis constant force
> - *
> - * other interesting fields 1,2,3,4 on x axis
> - * (same for 31,32,33,34 on y axis)
> - *
> - * 0 0 127 127 makes the joystick autocenter hard
> - *
> - * 127 0 127 127 makes the joystick loose on the right,
> - * but stops all movemnt left
> - *
> - * -127 0 -127 -127 makes the joystick loose on the left,
> - * but stops all movement right
> - *
> - * 0 0 -127 -127 makes the joystick rattle very hard
> - *
> - * I'm sure these are effects that I don't know enough about them
> - */
> +/* Ensure we remember to swap bytes (there's no sle16) */
> +typedef __s16 __bitwise lg3_s16;
>  
> -struct lg3ff_device {
> -	struct hid_report *report;
> -};
> +static inline lg3_s16 lg3ff_cpu_to_sle16(s16 val)
> +{
> +	return (__force lg3_s16)cpu_to_le16(val);
> +}
> +
> +struct hid_lg3ff_axis {
> +	lg3_s16	constant_force;  /* can cancel autocenter on relevant side */
> +	u8	_padding0;  /* extra byte of strength? no apparent effect */
> +	/* how far towards center does the effect keep pushing:
> +	 * 0   = no autocenter, up to:
> +	 * 127 = push immediately on any deflection
> +	 * <0  = repel center
> +	 */
> +	s8	autocenter_strength;
> +	/* how hard does autocenter push */
> +	s8	autocenter_force;
> +	/* damping with force of autocenter_force (see also damper_*) */
> +	s8	autocenter_damping;
> +	lg3_s16	spring_deadzone_neg;  /* for offset center, set these equal */
> +	lg3_s16	spring_deadzone_pos;
> +	s8	spring_coeff_neg;  /* <0 repels center */
> +	s8	spring_coeff_pos;
> +	lg3_s16	spring_saturation;
> +	u8	_padding1[8];  /* [4-8]: a different way of autocentering? */
> +	s8	damper_coeff_neg;
> +	s8	damper_coeff_pos;
> +	lg3_s16	damper_saturation;
> +	u8	_padding2[4];  /* seems to do the same as damper*? */
> +} __packed;
> +
> +struct hid_lg3ff_report {
> +	struct hid_lg3ff_axis x;
> +	struct hid_lg3ff_axis y;
> +	u8	_padding[3];
> +} __packed;
> +
> +#define FF_REPORT_ID 2
> +
> +static void hig_lg3ff_send(struct input_dev *idev,
> +			   struct hid_lg3ff_report *raw_rep)
> +{
> +	struct hid_device *hid = input_get_drvdata(idev);
> +	struct hid_report *hid_rep = hid->report_enum[HID_OUTPUT_REPORT]
> +					 .report_id_hash[FF_REPORT_ID];
> +	int i;
> +
> +	/* We can be called while atomic (via hid_lg3ff_play) and must queue;
> +	 * there's nowhere to enqueue a raw report, so populate a hid_report.
> +	 */
> +	for (i = 0; i < sizeof(*raw_rep); i++)
> +		hid_rep->field[0]->value[i] = ((u8 *)raw_rep)[i];
> +	hid_hw_request(hid, hid_rep, HID_REQ_SET_REPORT);
> +}
>  
>  static int hid_lg3ff_play(struct input_dev *dev, void *data,
>  			 struct ff_effect *effect)
>  {
> -	struct hid_device *hid = input_get_drvdata(dev);
> -	struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
> -	struct hid_report *report = list_entry(report_list->next, struct hid_report, list);
> -	int x, y;
> -
> -/*
> - * Available values in the field should always be 63, but we only use up to
> - * 35. Instead, clear the entire area, however big it is.
> - */
> -	memset(report->field[0]->value, 0,
> -	       sizeof(__s32) * report->field[0]->report_count);
> +	struct hid_lg3ff_report report = {0};
> +	s16 x, y;
>  
>  	switch (effect->type) {
>  	case FF_CONSTANT:
> @@ -78,46 +96,32 @@ static int hid_lg3ff_play(struct input_dev *dev, void *data,
>   * Already clamped in ff_memless
>   * 0 is center (different then other logitech)
>   */
> -		x = effect->u.ramp.start_level;
> -		y = effect->u.ramp.end_level;
> -
> -		/* send command byte */
> -		report->field[0]->value[0] = 0x51;
> +		x = -effect->u.ramp.start_level << 8;
> +		y = -effect->u.ramp.end_level << 8;
>  
>  /*
>   * Sign backwards from other Force3d pro
>   * which get recast here in two's complement 8 bits
>   */
> -		report->field[0]->value[1] = (unsigned char)(-x);
> -		report->field[0]->value[31] = (unsigned char)(-y);
> -
> -		hid_hw_request(hid, report, HID_REQ_SET_REPORT);
> +		report.x.constant_force = lg3ff_cpu_to_sle16(x);
> +		report.y.constant_force = lg3ff_cpu_to_sle16(y);
> +		hig_lg3ff_send(dev, &report);
>  		break;
>  	}
>  	return 0;
>  }
>  static void hid_lg3ff_set_autocenter(struct input_dev *dev, u16 magnitude)
>  {
> -	struct hid_device *hid = input_get_drvdata(dev);
> -	struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
> -	struct hid_report *report = list_entry(report_list->next, struct hid_report, list);
> +	struct hid_lg3ff_report report = {0};
>  
> -/*
> - * Auto Centering probed from device
> - * NOTE: deadman's switch on G940 must be covered
> - * for effects to work
> - */
> -	report->field[0]->value[0] = 0x51;
> -	report->field[0]->value[1] = 0x00;
> -	report->field[0]->value[2] = 0x00;
> -	report->field[0]->value[3] = 0x7F;
> -	report->field[0]->value[4] = 0x7F;
> -	report->field[0]->value[31] = 0x00;
> -	report->field[0]->value[32] = 0x00;
> -	report->field[0]->value[33] = 0x7F;
> -	report->field[0]->value[34] = 0x7F;
> -
> -	hid_hw_request(hid, report, HID_REQ_SET_REPORT);
> +	/* negative means repel from center, so scale to 0-127 */
> +	s8 mag_scaled = magnitude >> 9;
> +
> +	report.x.autocenter_strength = 127;
> +	report.x.autocenter_force = mag_scaled;
> +	report.y.autocenter_strength = 127;
> +	report.y.autocenter_force = mag_scaled;
> +	hig_lg3ff_send(dev, &report);
>  }
>  
>  
> @@ -136,7 +140,9 @@ int lg3ff_init(struct hid_device *hid)
>  	int i;
>  
>  	/* Check that the report looks ok */
> -	if (!hid_validate_values(hid, HID_OUTPUT_REPORT, 0, 0, 35))
> +	BUILD_BUG_ON(sizeof(struct hid_lg3ff_report) != 63);  /* excl. id */
> +	if (!hid_validate_values(hid, HID_OUTPUT_REPORT, FF_REPORT_ID, 0,
> +				 sizeof(struct hid_lg3ff_report)))
>  		return -ENODEV;
>  
>  	/* Assume single fixed device G940 */
> @@ -147,8 +153,10 @@ int lg3ff_init(struct hid_device *hid)
>  	if (error)
>  		return error;
>  
> -	if (test_bit(FF_AUTOCENTER, dev->ffbit))
> +	if (test_bit(FF_AUTOCENTER, dev->ffbit)) {
>  		dev->ff->set_autocenter = hid_lg3ff_set_autocenter;
> +		hid_lg3ff_set_autocenter(dev, 0);
> +	}
>  
>  	hid_info(hid, "Force feedback for Logitech Flight System G940 by Gary Stein <LordCnidarian@gmail.com>\n");
>  	return 0;
> -- 
> 2.17.1
> 
> 
> -- 
> Chris Boyle
> https://chris.boyle.name/
>

Patch
diff mbox series

diff --git a/drivers/hid/hid-lg3ff.c b/drivers/hid/hid-lg3ff.c
index 8c2da183d3bc..6fcd449d5b46 100644
--- a/drivers/hid/hid-lg3ff.c
+++ b/drivers/hid/hid-lg3ff.c
@@ -2,6 +2,7 @@ 
  *  Force feedback support for Logitech Flight System G940
  *
  *  Copyright (c) 2009 Gary Stein <LordCnidarian@gmail.com>
+ *  Copyright (c) 2019 Chris Boyle
  */
 
 /*
@@ -26,51 +27,68 @@ 
 
 #include "hid-lg.h"
 
-/*
- * G940 Theory of Operation (from experimentation)
- *
- * There are 63 fields (only 3 of them currently used)
- * 0 - seems to be command field
- * 1 - 30 deal with the x axis
- * 31 -60 deal with the y axis
- *
- * Field 1 is x axis constant force
- * Field 31 is y axis constant force
- *
- * other interesting fields 1,2,3,4 on x axis
- * (same for 31,32,33,34 on y axis)
- *
- * 0 0 127 127 makes the joystick autocenter hard
- *
- * 127 0 127 127 makes the joystick loose on the right,
- * but stops all movemnt left
- *
- * -127 0 -127 -127 makes the joystick loose on the left,
- * but stops all movement right
- *
- * 0 0 -127 -127 makes the joystick rattle very hard
- *
- * I'm sure these are effects that I don't know enough about them
- */
+/* Ensure we remember to swap bytes (there's no sle16) */
+typedef __s16 __bitwise lg3_s16;
 
-struct lg3ff_device {
-	struct hid_report *report;
-};
+static inline lg3_s16 lg3ff_cpu_to_sle16(s16 val)
+{
+	return (__force lg3_s16)cpu_to_le16(val);
+}
+
+struct hid_lg3ff_axis {
+	lg3_s16	constant_force;  /* can cancel autocenter on relevant side */
+	u8	_padding0;  /* extra byte of strength? no apparent effect */
+	/* how far towards center does the effect keep pushing:
+	 * 0   = no autocenter, up to:
+	 * 127 = push immediately on any deflection
+	 * <0  = repel center
+	 */
+	s8	autocenter_strength;
+	/* how hard does autocenter push */
+	s8	autocenter_force;
+	/* damping with force of autocenter_force (see also damper_*) */
+	s8	autocenter_damping;
+	lg3_s16	spring_deadzone_neg;  /* for offset center, set these equal */
+	lg3_s16	spring_deadzone_pos;
+	s8	spring_coeff_neg;  /* <0 repels center */
+	s8	spring_coeff_pos;
+	lg3_s16	spring_saturation;
+	u8	_padding1[8];  /* [4-8]: a different way of autocentering? */
+	s8	damper_coeff_neg;
+	s8	damper_coeff_pos;
+	lg3_s16	damper_saturation;
+	u8	_padding2[4];  /* seems to do the same as damper*? */
+} __packed;
+
+struct hid_lg3ff_report {
+	struct hid_lg3ff_axis x;
+	struct hid_lg3ff_axis y;
+	u8	_padding[3];
+} __packed;
+
+#define FF_REPORT_ID 2
+
+static void hig_lg3ff_send(struct input_dev *idev,
+			   struct hid_lg3ff_report *raw_rep)
+{
+	struct hid_device *hid = input_get_drvdata(idev);
+	struct hid_report *hid_rep = hid->report_enum[HID_OUTPUT_REPORT]
+					 .report_id_hash[FF_REPORT_ID];
+	int i;
+
+	/* We can be called while atomic (via hid_lg3ff_play) and must queue;
+	 * there's nowhere to enqueue a raw report, so populate a hid_report.
+	 */
+	for (i = 0; i < sizeof(*raw_rep); i++)
+		hid_rep->field[0]->value[i] = ((u8 *)raw_rep)[i];
+	hid_hw_request(hid, hid_rep, HID_REQ_SET_REPORT);
+}
 
 static int hid_lg3ff_play(struct input_dev *dev, void *data,
 			 struct ff_effect *effect)
 {
-	struct hid_device *hid = input_get_drvdata(dev);
-	struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
-	struct hid_report *report = list_entry(report_list->next, struct hid_report, list);
-	int x, y;
-
-/*
- * Available values in the field should always be 63, but we only use up to
- * 35. Instead, clear the entire area, however big it is.
- */
-	memset(report->field[0]->value, 0,
-	       sizeof(__s32) * report->field[0]->report_count);
+	struct hid_lg3ff_report report = {0};
+	s16 x, y;
 
 	switch (effect->type) {
 	case FF_CONSTANT:
@@ -78,46 +96,32 @@  static int hid_lg3ff_play(struct input_dev *dev, void *data,
  * Already clamped in ff_memless
  * 0 is center (different then other logitech)
  */
-		x = effect->u.ramp.start_level;
-		y = effect->u.ramp.end_level;
-
-		/* send command byte */
-		report->field[0]->value[0] = 0x51;
+		x = -effect->u.ramp.start_level << 8;
+		y = -effect->u.ramp.end_level << 8;
 
 /*
  * Sign backwards from other Force3d pro
  * which get recast here in two's complement 8 bits
  */
-		report->field[0]->value[1] = (unsigned char)(-x);
-		report->field[0]->value[31] = (unsigned char)(-y);
-
-		hid_hw_request(hid, report, HID_REQ_SET_REPORT);
+		report.x.constant_force = lg3ff_cpu_to_sle16(x);
+		report.y.constant_force = lg3ff_cpu_to_sle16(y);
+		hig_lg3ff_send(dev, &report);
 		break;
 	}
 	return 0;
 }
 static void hid_lg3ff_set_autocenter(struct input_dev *dev, u16 magnitude)
 {
-	struct hid_device *hid = input_get_drvdata(dev);
-	struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
-	struct hid_report *report = list_entry(report_list->next, struct hid_report, list);
+	struct hid_lg3ff_report report = {0};
 
-/*
- * Auto Centering probed from device
- * NOTE: deadman's switch on G940 must be covered
- * for effects to work
- */
-	report->field[0]->value[0] = 0x51;
-	report->field[0]->value[1] = 0x00;
-	report->field[0]->value[2] = 0x00;
-	report->field[0]->value[3] = 0x7F;
-	report->field[0]->value[4] = 0x7F;
-	report->field[0]->value[31] = 0x00;
-	report->field[0]->value[32] = 0x00;
-	report->field[0]->value[33] = 0x7F;
-	report->field[0]->value[34] = 0x7F;
-
-	hid_hw_request(hid, report, HID_REQ_SET_REPORT);
+	/* negative means repel from center, so scale to 0-127 */
+	s8 mag_scaled = magnitude >> 9;
+
+	report.x.autocenter_strength = 127;
+	report.x.autocenter_force = mag_scaled;
+	report.y.autocenter_strength = 127;
+	report.y.autocenter_force = mag_scaled;
+	hig_lg3ff_send(dev, &report);
 }
 
 
@@ -136,7 +140,9 @@  int lg3ff_init(struct hid_device *hid)
 	int i;
 
 	/* Check that the report looks ok */
-	if (!hid_validate_values(hid, HID_OUTPUT_REPORT, 0, 0, 35))
+	BUILD_BUG_ON(sizeof(struct hid_lg3ff_report) != 63);  /* excl. id */
+	if (!hid_validate_values(hid, HID_OUTPUT_REPORT, FF_REPORT_ID, 0,
+				 sizeof(struct hid_lg3ff_report)))
 		return -ENODEV;
 
 	/* Assume single fixed device G940 */
@@ -147,8 +153,10 @@  int lg3ff_init(struct hid_device *hid)
 	if (error)
 		return error;
 
-	if (test_bit(FF_AUTOCENTER, dev->ffbit))
+	if (test_bit(FF_AUTOCENTER, dev->ffbit)) {
 		dev->ff->set_autocenter = hid_lg3ff_set_autocenter;
+		hid_lg3ff_set_autocenter(dev, 0);
+	}
 
 	hid_info(hid, "Force feedback for Logitech Flight System G940 by Gary Stein <LordCnidarian@gmail.com>\n");
 	return 0;