| Message ID | 20180722165039.5042-1-timschumi@gmx.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] input/iforce: Remove the BTN_DEAD assignment and usage | expand |
On Sun, Jul 22, 2018 at 06:50:39PM +0200, Tim Schumacher wrote: > The nonexistent button assigned as BTN_DEAD is constantly > creating false inputs (as it gets triggered on every joystick > status update), making the task of (as an example) > assigning controls unnecessarily hard. IIRC BTN_DEAD is a dead-man or safety switch, and is supposed to be reported when user stops interacting with the device. I do not think we should be removing it unconditionally... Let's add Vojtech if he remembers anything more... > > Signed-off-by: Tim Schumacher <timschumi@gmx.de> > --- > drivers/input/joystick/iforce/iforce-main.c | 1 - > drivers/input/joystick/iforce/iforce-packets.c | 1 - > 2 files changed, 2 deletions(-) > > diff --git a/drivers/input/joystick/iforce/iforce-main.c b/drivers/input/joystick/iforce/iforce-main.c > index 054abed0fdc1..a4ab58633136 100644 > --- a/drivers/input/joystick/iforce/iforce-main.c > +++ b/drivers/input/joystick/iforce/iforce-main.c > @@ -388,7 +388,6 @@ int iforce_init_device(struct iforce *iforce) > > for (i = 0; iforce->type->btn[i] >= 0; i++) > set_bit(iforce->type->btn[i], input_dev->keybit); > - set_bit(BTN_DEAD, input_dev->keybit); > > for (i = 0; iforce->type->abs[i] >= 0; i++) { > > diff --git a/drivers/input/joystick/iforce/iforce-packets.c b/drivers/input/joystick/iforce/iforce-packets.c > index 08f98f2eaf88..d929c6f05275 100644 > --- a/drivers/input/joystick/iforce/iforce-packets.c > +++ b/drivers/input/joystick/iforce/iforce-packets.c > @@ -220,7 +220,6 @@ void iforce_process_packet(struct iforce *iforce, u16 cmd, unsigned char *data) > break; > > case 0x02: /* status report */ > - input_report_key(dev, BTN_DEAD, data[0] & 0x02); > input_sync(dev); > > /* Check if an effect was just started or stopped */ > -- > 2.18.0 >
On 23.07.2018 19:36, Dmitry Torokhov wrote: > On Sun, Jul 22, 2018 at 06:50:39PM +0200, Tim Schumacher wrote: >> The nonexistent button assigned as BTN_DEAD is constantly >> creating false inputs (as it gets triggered on every joystick >> status update), making the task of (as an example) >> assigning controls unnecessarily hard. > > IIRC BTN_DEAD is a dead-man or safety switch, and is supposed to be > reported when user stops interacting with the device. I do not think we > should be removing it unconditionally... I can only speak for the WingMan Formula Force wheel that I own, but in my case BTN_DEAD always sent rapid on/off inputs instead of only being triggered when I stopped interacting. I didn't notice any different behaviour after I removed BTN_DEAD, but that may have been my device only. -- 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
On Mon, Jul 23, 2018 at 10:36:05AM -0700, Dmitry Torokhov wrote: > On Sun, Jul 22, 2018 at 06:50:39PM +0200, Tim Schumacher wrote: > > The nonexistent button assigned as BTN_DEAD is constantly > > creating false inputs (as it gets triggered on every joystick > > status update), making the task of (as an example) > > assigning controls unnecessarily hard. > > IIRC BTN_DEAD is a dead-man or safety switch, and is supposed to be > reported when user stops interacting with the device. I do not think we > should be removing it unconditionally... > > Let's add Vojtech if he remembers anything more... That is correct. It's expected to be "pressed" when nobody is holding the stick and released otherwise. The stick usually has a reflective optical sensor on the palm side. It certainly worked well on the devices I tested the driver with back then. > > Signed-off-by: Tim Schumacher <timschumi@gmx.de> > > --- > > drivers/input/joystick/iforce/iforce-main.c | 1 - > > drivers/input/joystick/iforce/iforce-packets.c | 1 - > > 2 files changed, 2 deletions(-) > > > > diff --git a/drivers/input/joystick/iforce/iforce-main.c b/drivers/input/joystick/iforce/iforce-main.c > > index 054abed0fdc1..a4ab58633136 100644 > > --- a/drivers/input/joystick/iforce/iforce-main.c > > +++ b/drivers/input/joystick/iforce/iforce-main.c > > @@ -388,7 +388,6 @@ int iforce_init_device(struct iforce *iforce) > > > > for (i = 0; iforce->type->btn[i] >= 0; i++) > > set_bit(iforce->type->btn[i], input_dev->keybit); > > - set_bit(BTN_DEAD, input_dev->keybit); > > > > for (i = 0; iforce->type->abs[i] >= 0; i++) { > > > > diff --git a/drivers/input/joystick/iforce/iforce-packets.c b/drivers/input/joystick/iforce/iforce-packets.c > > index 08f98f2eaf88..d929c6f05275 100644 > > --- a/drivers/input/joystick/iforce/iforce-packets.c > > +++ b/drivers/input/joystick/iforce/iforce-packets.c > > @@ -220,7 +220,6 @@ void iforce_process_packet(struct iforce *iforce, u16 cmd, unsigned char *data) > > break; > > > > case 0x02: /* status report */ > > - input_report_key(dev, BTN_DEAD, data[0] & 0x02); > > input_sync(dev); > > > > /* Check if an effect was just started or stopped */ > > -- > > 2.18.0 > > > > -- > Dmitry >
On 26.07.2018 10:44, Vojtech Pavlik wrote: > On Mon, Jul 23, 2018 at 10:36:05AM -0700, Dmitry Torokhov wrote: > >> On Sun, Jul 22, 2018 at 06:50:39PM +0200, Tim Schumacher wrote: >>> The nonexistent button assigned as BTN_DEAD is constantly >>> creating false inputs (as it gets triggered on every joystick >>> status update), making the task of (as an example) >>> assigning controls unnecessarily hard. >> >> IIRC BTN_DEAD is a dead-man or safety switch, and is supposed to be >> reported when user stops interacting with the device. I do not think we >> should be removing it unconditionally... >> >> Let's add Vojtech if he remembers anything more... > > That is correct. It's expected to be "pressed" when nobody is holding > the stick and released otherwise. > > The stick usually has a reflective optical sensor on the palm side. > > It certainly worked well on the devices I tested the driver with back > then. If BTN_DEAD does fulfill a purpose on some devices in this series it indeed should not be removed unconditionally. Do you know if that optical sensor is a feature of only the joystick-like devices? The Formula Force wheel (at least the red version) doesn't seem to have such a feature, which is probably the root cause for the issues. During testing of the previous version of this patch I noticed that the call to input_report_key() does nothing if the button hasn't been assigned with set_bit() before. Would moving BTN_DEAD into the array of buttons at the top of iforce-main.c (only for the devices that need it) be a viable solution? The current set_bit() call for BTN_DEAD does not differ from the automated call that reads from the selected arrays. That way BTN_DEAD only gets assigned for devices that need it (resolving the issue for some devices while not breaking other ones) and the input_report_key() function call later becomes a no-op if it isn't needed. > >>> Signed-off-by: Tim Schumacher <timschumi@gmx.de> >>> --- >>> drivers/input/joystick/iforce/iforce-main.c | 1 - >>> drivers/input/joystick/iforce/iforce-packets.c | 1 - >>> 2 files changed, 2 deletions(-) >>> >>> diff --git a/drivers/input/joystick/iforce/iforce-main.c b/drivers/input/joystick/iforce/iforce-main.c >>> index 054abed0fdc1..a4ab58633136 100644 >>> --- a/drivers/input/joystick/iforce/iforce-main.c >>> +++ b/drivers/input/joystick/iforce/iforce-main.c >>> @@ -388,7 +388,6 @@ int iforce_init_device(struct iforce *iforce) >>> >>> for (i = 0; iforce->type->btn[i] >= 0; i++) >>> set_bit(iforce->type->btn[i], input_dev->keybit); >>> - set_bit(BTN_DEAD, input_dev->keybit); >>> >>> for (i = 0; iforce->type->abs[i] >= 0; i++) { >>> >>> diff --git a/drivers/input/joystick/iforce/iforce-packets.c b/drivers/input/joystick/iforce/iforce-packets.c >>> index 08f98f2eaf88..d929c6f05275 100644 >>> --- a/drivers/input/joystick/iforce/iforce-packets.c >>> +++ b/drivers/input/joystick/iforce/iforce-packets.c >>> @@ -220,7 +220,6 @@ void iforce_process_packet(struct iforce *iforce, u16 cmd, unsigned char *data) >>> break; >>> >>> case 0x02: /* status report */ >>> - input_report_key(dev, BTN_DEAD, data[0] & 0x02); >>> input_sync(dev); >>> >>> /* Check if an effect was just started or stopped */ >>> -- >>> 2.18.0 >>> >> >> -- >> Dmitry >> > -- 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
On Thu, Jul 26, 2018 at 04:09:18PM +0200, Tim Schumacher wrote: > If BTN_DEAD does fulfill a purpose on some devices in this series it > indeed should not be removed unconditionally. > > Do you know if that optical sensor is a feature of only the > joystick-like devices? The Formula Force wheel (at least the red > version) doesn't seem to have such a feature, which is probably > the root cause for the issues. > > During testing of the previous version of this patch I noticed > that the call to input_report_key() does nothing if the button > hasn't been assigned with set_bit() before. > > Would moving BTN_DEAD into the array of buttons at the top of > iforce-main.c (only for the devices that need it) be a viable > solution? The current set_bit() call for BTN_DEAD does not differ > from the automated call that reads from the selected arrays. > > That way BTN_DEAD only gets assigned for devices that need it > (resolving the issue for some devices while not breaking other ones) > and the input_report_key() function call later becomes a no-op > if it isn't needed. Indeed, the BTN_DEAD present bit should not be set for devices that don't have it. So it does make sense to move it into the per-device array and away from the common code. > > > > Signed-off-by: Tim Schumacher <timschumi@gmx.de> > > > > --- > > > > drivers/input/joystick/iforce/iforce-main.c | 1 - > > > > drivers/input/joystick/iforce/iforce-packets.c | 1 - > > > > 2 files changed, 2 deletions(-) > > > > > > > > diff --git a/drivers/input/joystick/iforce/iforce-main.c b/drivers/input/joystick/iforce/iforce-main.c > > > > index 054abed0fdc1..a4ab58633136 100644 > > > > --- a/drivers/input/joystick/iforce/iforce-main.c > > > > +++ b/drivers/input/joystick/iforce/iforce-main.c > > > > @@ -388,7 +388,6 @@ int iforce_init_device(struct iforce *iforce) > > > > for (i = 0; iforce->type->btn[i] >= 0; i++) > > > > set_bit(iforce->type->btn[i], input_dev->keybit); > > > > - set_bit(BTN_DEAD, input_dev->keybit); > > > > for (i = 0; iforce->type->abs[i] >= 0; i++) { > > > > diff --git a/drivers/input/joystick/iforce/iforce-packets.c b/drivers/input/joystick/iforce/iforce-packets.c > > > > index 08f98f2eaf88..d929c6f05275 100644 > > > > --- a/drivers/input/joystick/iforce/iforce-packets.c > > > > +++ b/drivers/input/joystick/iforce/iforce-packets.c > > > > @@ -220,7 +220,6 @@ void iforce_process_packet(struct iforce *iforce, u16 cmd, unsigned char *data) > > > > break; > > > > case 0x02: /* status report */ > > > > - input_report_key(dev, BTN_DEAD, data[0] & 0x02); > > > > input_sync(dev); > > > > /* Check if an effect was just started or stopped */
diff --git a/drivers/input/joystick/iforce/iforce-main.c b/drivers/input/joystick/iforce/iforce-main.c index 054abed0fdc1..a4ab58633136 100644 --- a/drivers/input/joystick/iforce/iforce-main.c +++ b/drivers/input/joystick/iforce/iforce-main.c @@ -388,7 +388,6 @@ int iforce_init_device(struct iforce *iforce) for (i = 0; iforce->type->btn[i] >= 0; i++) set_bit(iforce->type->btn[i], input_dev->keybit); - set_bit(BTN_DEAD, input_dev->keybit); for (i = 0; iforce->type->abs[i] >= 0; i++) { diff --git a/drivers/input/joystick/iforce/iforce-packets.c b/drivers/input/joystick/iforce/iforce-packets.c index 08f98f2eaf88..d929c6f05275 100644 --- a/drivers/input/joystick/iforce/iforce-packets.c +++ b/drivers/input/joystick/iforce/iforce-packets.c @@ -220,7 +220,6 @@ void iforce_process_packet(struct iforce *iforce, u16 cmd, unsigned char *data) break; case 0x02: /* status report */ - input_report_key(dev, BTN_DEAD, data[0] & 0x02); input_sync(dev); /* Check if an effect was just started or stopped */
The nonexistent button assigned as BTN_DEAD is constantly creating false inputs (as it gets triggered on every joystick status update), making the task of (as an example) assigning controls unnecessarily hard. Signed-off-by: Tim Schumacher <timschumi@gmx.de> --- drivers/input/joystick/iforce/iforce-main.c | 1 - drivers/input/joystick/iforce/iforce-packets.c | 1 - 2 files changed, 2 deletions(-)