[PATCH RESEND] Initialize axis values in joydev_open_device()

Postpone axis initialization to the first open instead of doing it once
in joydev_connect. This is to make sure the generated startup events
are representative of the current joystick state rather than what
it was when joydev_connect() was called, potentially much earlier.

This solves issues with joystick driven menus that start scrolling
up each time they are started, until the user moves the joystick to
generate events. In emulator menu setups where the menu program is
restarted every time the game exits, the repeated need to move the
joystick to stop the unintended scrolling gets old rather quickly...

Unless I misunderstood the intent of JS_EVENT_INIT, I think the startup
events should reflect the current state of the joystick. Please consider
applying if it makes sense.

Signed-off-by: Raphael Assenat <redacted>

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 5d11fea..5b42e5a 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c

@@ -156,6 +156,38 @@ static void joydev_event(struct input_handle *handle,
 	wake_up_interruptible(&joydev->wait);
 }
 
+static void joydev_refresh_state(struct joydev *joydev)
+{
+	struct input_dev *dev = joydev->handle.dev;
+	int i, j, t;
+
+	for (i = 0; i < joydev->nabs; i++) {
+		j = joydev->abspam[i];
+		if (input_abs_get_max(dev, j) == input_abs_get_min(dev, j)) {
+			joydev->corr[i].type = JS_CORR_NONE;
+			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);
+
+		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);
+		}
+	}
+}
+
 static int joydev_fasync(int fd, struct file *file, int on)
 {
 	struct joydev_client *client = file->private_data;

@@ -202,6 +234,8 @@ static int joydev_open_device(struct joydev *joydev)
 		retval = input_open_device(&joydev->handle);
 		if (retval)
 			joydev->open--;
+		else
+			joydev_refresh_state(joydev);
 	}
 
 	mutex_unlock(&joydev->mutex);

@@ -816,7 +850,7 @@ 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, minor, dev_no;
 	int error;
 
 	minor = input_get_new_minor(JOYDEV_MINOR_BASE, JOYDEV_MINORS, true);

@@ -869,31 +903,7 @@ static int joydev_connect(struct input_handler *handler, struct input_dev *dev,
 			joydev->nkey++;
 		}
 
-	for (i = 0; i < joydev->nabs; i++) {
-		j = joydev->abspam[i];
-		if (input_abs_get_max(dev, j) == input_abs_get_min(dev, j)) {
-			joydev->corr[i].type = JS_CORR_NONE;
-			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);
-
-		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_refresh_state(joydev);
 
 	joydev->dev.devt = MKDEV(INPUT_MAJOR, minor);
 	joydev->dev.class = &input_class;

Hi Raphael,

On Sun, Dec 18, 2016 at 03:20:50PM -0500, Raphael Assenat wrote:

Postpone axis initialization to the first open instead of doing it once
in joydev_connect. This is to make sure the generated startup events
are representative of the current joystick state rather than what
it was when joydev_connect() was called, potentially much earlier.

This solves issues with joystick driven menus that start scrolling
up each time they are started, until the user moves the joystick to
generate events. In emulator menu setups where the menu program is
restarted every time the game exits, the repeated need to move the
joystick to stop the unintended scrolling gets old rather quickly...

Unless I misunderstood the intent of JS_EVENT_INIT, I think the startup
events should reflect the current state of the joystick. Please consider
applying if it makes sense.

Sorry for the delay and what you are saying certainly makes sense.
Unfortunately with the patch as is we end up re-initializing calibration
coefficients every time user opens device (assuming that there is only
one user of joystick device at a time), whereas before one could have a
small utility calibrating the joystick, and then go on to using it with
some other application (game). How about we keep most of the
initialization in connect() and only populate axis data in open(), like
below?

-- 
Dmitry


Input: joydev - do not report stale values on first open

From: Raphael Assenat <redacted>

Postpone axis initialization to the first open instead of doing it
in joydev_connect. This is to make sure the generated startup events
are representative of the current joystick state rather than what
it was when joydev_connect() was called, potentially much earlier.
Once the first user is connected to joydev node we'll be updating
joydev->abs[] values and subsequent clients will be getting correct
initial states as well.

This solves issues with joystick driven menus that start scrolling
up each time they are started, until the user moves the joystick to
generate events. In emulator menu setups where the menu program is
restarted every time the game exits, the repeated need to move the
joystick to stop the unintended scrolling gets old rather quickly...

Signed-off-by: Raphael Assenat <redacted>
Patchwork-Id: 9479395
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/joydev.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index abd18f31b24f..db5e290e897e 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c

@@ -187,6 +187,17 @@ static void joydev_detach_client(struct joydev *joydev,
 	synchronize_rcu();
 }
 
+static void joydev_refresh_state(struct joydev *joydev)
+{
+	struct input_dev *dev = joydev->handle.dev;
+	int i, val;
+
+	for (i = 0; i < joydev->nabs; i++) {
+		val = input_abs_get_val(dev, joydev->abspam[i]);
+		joydev->abs[i] = joydev_correct(val, &joydev->corr[i]);
+	}
+}
+
 static int joydev_open_device(struct joydev *joydev)
 {
 	int retval;

@@ -201,6 +212,8 @@ static int joydev_open_device(struct joydev *joydev)
 		retval = input_open_device(&joydev->handle);
 		if (retval)
 			joydev->open--;
+		else
+			joydev_refresh_state(joydev);
 	}
 
 	mutex_unlock(&joydev->mutex);

@@ -872,7 +885,6 @@ static int joydev_connect(struct input_handler *handler, struct input_dev *dev,
 		j = joydev->abspam[i];
 		if (input_abs_get_max(dev, j) == input_abs_get_min(dev, j)) {
 			joydev->corr[i].type = JS_CORR_NONE;
-			joydev->abs[i] = input_abs_get_val(dev, j);
 			continue;
 		}
 		joydev->corr[i].type = JS_CORR_BROKEN;

@@ -887,10 +899,6 @@ static int joydev_connect(struct input_handler *handler, struct input_dev *dev,
 		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);
 		}
 	}
 

On Sat, Jan 28, 2017 at 11:01:00AM -0800, Dmitry Torokhov wrote:

Hi Raphael,

On Sun, Dec 18, 2016 at 03:20:50PM -0500, Raphael Assenat wrote:

quoted

Postpone axis initialization to the first open instead of doing it once
in joydev_connect. This is to make sure the generated startup events
are representative of the current joystick state rather than what
it was when joydev_connect() was called, potentially much earlier.

This solves issues with joystick driven menus that start scrolling
up each time they are started, until the user moves the joystick to
generate events. In emulator menu setups where the menu program is
restarted every time the game exits, the repeated need to move the
joystick to stop the unintended scrolling gets old rather quickly...

Unless I misunderstood the intent of JS_EVENT_INIT, I think the startup
events should reflect the current state of the joystick. Please consider
applying if it makes sense.

Sorry for the delay and what you are saying certainly makes sense.
Unfortunately with the patch as is we end up re-initializing calibration
coefficients every time user opens device (assuming that there is only
one user of joystick device at a time), whereas before one could have a
small utility calibrating the joystick, and then go on to using it with
some other application (game). How about we keep most of the
initialization in connect() and only populate axis data in open(), like
below?

Thinking about it some more, do we really want to fetch current state of
joystick and not start with "rest" state?

If joystick is actively generating events then it doe snot matter, but
if it is resting and has not generated any events yet, then axis values
will be at 0, which, if axis range is [0-1024], will translate to
-32767.

Maybe we should start all clients with JS_CORR_BROKEN as:

	val = (input_abs_get_max(dev, i) + input_abs_get_min(dev, i)) / 2;
	joydev->abs[i] = joydev_correct(val, &joydev->corr[i]);

What do you think?

Thanks.

-- 
Dmitry

Hello Dmitry,

On Tue, Jan 31, 2017 at 05:44:49PM -0800, Dmitry Torokhov wrote:

On Sat, Jan 28, 2017 at 11:01:00AM -0800, Dmitry Torokhov wrote:

quoted

Hi Raphael,

On Sun, Dec 18, 2016 at 03:20:50PM -0500, Raphael Assenat wrote:

quoted

Postpone axis initialization to the first open instead of doing it once
in joydev_connect. This is to make sure the generated startup events
are representative of the current joystick state rather than what
it was when joydev_connect() was called, potentially much earlier.

This solves issues with joystick driven menus that start scrolling
up each time they are started, until the user moves the joystick to
generate events. In emulator menu setups where the menu program is
restarted every time the game exits, the repeated need to move the
joystick to stop the unintended scrolling gets old rather quickly...

Unless I misunderstood the intent of JS_EVENT_INIT, I think the startup
events should reflect the current state of the joystick. Please consider
applying if it makes sense.

Sorry for the delay and what you are saying certainly makes sense.
Unfortunately with the patch as is we end up re-initializing calibration
coefficients every time user opens device (assuming that there is only
one user of joystick device at a time), whereas before one could have a
small utility calibrating the joystick, and then go on to using it with
some other application (game). How about we keep most of the
initialization in connect() and only populate axis data in open(), like
below?

Thinking about it some more, do we really want to fetch current state of
joystick and not start with "rest" state?

Both approaches have merits, and in the context of my original
issue, they both work perfectly.

But I think that returning the current value might be more correct as there
could be situations where it is needed. I'm thinking about unusual/custom/homemade
controls with potentiometers on a panel where the initial value should be
effective right from the start. But one could argue that this is not a joystick
anymore and should be using evdev (or even hidraw) instead.

(There is a similar issue with evdev by the way. For HID joysticks, the current
value of abs axes is zero until events are generated, but for a different
reason. More on this below.)

If joystick is actively generating events then it doe snot matter, but
if it is resting and has not generated any events yet, then axis values
will be at 0, which, if axis range is [0-1024], will translate to
-32767.

Indeed, it won't return a centered value initially, but HID joysticks
should generate events at startup (Through the use of HID Get_Report requests).

However this feature seems broken at the moment. While the get_report requests
are properly sent to the HID device, the answers arrive at a time when they
cannot be processed and get dropped.

I created two patches last year to workaround this issue, but I'm not sure if they
are correct (I re-order things and am not sure if there are implications I don't
see). I planned to rebase them (if necessary) and post them here soon, but for now
here are the originals:
http://www.raphnet-tech.com/support/retropie/usbhid_iostart.diff
http://www.raphnet-tech.com/support/retropie/usbhid_start_before_connect.diff

Maybe we should start all clients with JS_CORR_BROKEN as:

	val = (input_abs_get_max(dev, i) + input_abs_get_min(dev, i)) / 2;
	joydev->abs[i] = joydev_correct(val, &joydev->corr[i]);

What do you think?

I'm comfortable with both suggestions, however I feel that returning the current
value is better, even though it would not be perfect until a solution to the other
issue I mentioned is implemented.

Best regards,
Raphaël Assénat

On Thu, Feb 02, 2017 at 11:22:29PM -0500, Raphael Assenat wrote:

Hello Dmitry,

On Tue, Jan 31, 2017 at 05:44:49PM -0800, Dmitry Torokhov wrote:

quoted

On Sat, Jan 28, 2017 at 11:01:00AM -0800, Dmitry Torokhov wrote:

quoted

Hi Raphael,

On Sun, Dec 18, 2016 at 03:20:50PM -0500, Raphael Assenat wrote:

quoted

Postpone axis initialization to the first open instead of doing it once
in joydev_connect. This is to make sure the generated startup events
are representative of the current joystick state rather than what
it was when joydev_connect() was called, potentially much earlier.

This solves issues with joystick driven menus that start scrolling
up each time they are started, until the user moves the joystick to
generate events. In emulator menu setups where the menu program is
restarted every time the game exits, the repeated need to move the
joystick to stop the unintended scrolling gets old rather quickly...

Unless I misunderstood the intent of JS_EVENT_INIT, I think the startup
events should reflect the current state of the joystick. Please consider
applying if it makes sense.

Sorry for the delay and what you are saying certainly makes sense.
Unfortunately with the patch as is we end up re-initializing calibration
coefficients every time user opens device (assuming that there is only
one user of joystick device at a time), whereas before one could have a
small utility calibrating the joystick, and then go on to using it with
some other application (game). How about we keep most of the
initialization in connect() and only populate axis data in open(), like
below?

Thinking about it some more, do we really want to fetch current state of
joystick and not start with "rest" state?

Both approaches have merits, and in the context of my original
issue, they both work perfectly.

But I think that returning the current value might be more correct as there
could be situations where it is needed. I'm thinking about unusual/custom/homemade
controls with potentiometers on a panel where the initial value should be
effective right from the start. But one could argue that this is not a joystick
anymore and should be using evdev (or even hidraw) instead.

(There is a similar issue with evdev by the way. For HID joysticks, the current
value of abs axes is zero until events are generated, but for a different
reason. More on this below.)

quoted

If joystick is actively generating events then it doe snot matter, but
if it is resting and has not generated any events yet, then axis values
will be at 0, which, if axis range is [0-1024], will translate to
-32767.

Indeed, it won't return a centered value initially, but HID joysticks
should generate events at startup (Through the use of HID Get_Report requests).

However this feature seems broken at the moment. While the get_report requests
are properly sent to the HID device, the answers arrive at a time when they
cannot be processed and get dropped.

I created two patches last year to workaround this issue, but I'm not sure if they
are correct (I re-order things and am not sure if there are implications I don't
see). I planned to rebase them (if necessary) and post them here soon, but for now
here are the originals:
http://www.raphnet-tech.com/support/retropie/usbhid_iostart.diff
http://www.raphnet-tech.com/support/retropie/usbhid_start_before_connect.diff

quoted

Maybe we should start all clients with JS_CORR_BROKEN as:

	val = (input_abs_get_max(dev, i) + input_abs_get_min(dev, i)) / 2;
	joydev->abs[i] = joydev_correct(val, &joydev->corr[i]);

What do you think?

I'm comfortable with both suggestions, however I feel that returning the current
value is better, even though it would not be perfect until a solution to the other
issue I mentioned is implemented.

OK, let's start with the current one and we may revisit it later.

Thanks.

-- 
Dmitry