Re: [Patch-V2 1/6] INPUT: xpad: Add minimal support for Logitech G920 Wheel
On Thu, Dec 10, 2015 at 6:08 PM, Benjamin Tissoires wrote: > On Dec 09 2015 or thereabouts, Dmitry Torokhov wrote: >> On Wed, Dec 9, 2015 at 5:23 PM, Dmitry Torokhov >> wrote: >> > On Thu, Nov 19, 2015 at 10:31 AM, Dmitry Torokhov >> > wrote: >> >> On Thu, Nov 19, 2015 at 02:50:51PM +0100, Jiri Kosina wrote: >> >>> On Thu, 12 Nov 2015, Simon Wood wrote: >> >>> >> >>> > When plugged in the Logitech G920 wheel starts with USBID 046d:c261 >> >>> > and behaviors as a vendor specific class. If a 'magic' byte sequence >> >>> > is sent the wheel will detach and reconnect as a HID device with the >> >>> > USBID 046d:c262. >> >>> > >> >>> > Signed-off-by: Simon Wood >> >>> >> >>> Adding Dmitry to CC. >> >>> >> >>> Dmitry, I am planning to take this through my tree together with the rest >> >>> of the actual HID support for that device if you Ack this. >> >> >> >> Hmm, I have an incoming series for xbox that night clash with this... If >> >> you'll put it in a clean branch off 4.3 I'd pull it and then get more >> >> changes on top. >> >> >> >> Can we also change the subject as it is not about adding a minimal >> >> support. Something like "Input: xpad - switch Logitech G920 Wheel into >> >> HID mode" >> >> >> >> Otherwise: >> >> >> >> Acked-by: Dmitry Torokhov >> > >> > Hmm, looking sat this some more why are we waiting to switch device >> > mode until after userspace opens input device instead of when we are >> > executing driver probe()? >> > >> >> Actually, thinking about it even more, why do we want to have this in >> xpad.c? Have HID module handle both IDs and switch to HID mode if we >> want HID to handle this device. I think we should revert/drop this >> patch. >> > > Hi Dmitry, > > IIRC, last time I saw an XBox-like controller, it doesn't register as a > HID device at all. SO I think It will be hard to switch it into the HID > mode from HID directly. > Simon, can you confirm that the device does not contains any references > to HID while in the XBox mode (lsusb -v should give enough information). This is probably not relevant anymore, but here is the "sudo lsusb -v" output of my G920 in XBox mode: Bus 001 Device 121: ID 046d:c261 Logitech, Inc. Device Descriptor: bLength18 bDescriptorType 1 bcdUSB 2.00 bDeviceClass 255 Vendor Specific Class bDeviceSubClass 255 Vendor Specific Subclass bDeviceProtocol 255 Vendor Specific Protocol bMaxPacketSize064 idVendor 0x046d Logitech, Inc. idProduct 0xc261 bcdDevice 96.01 iManufacturer 1 Logitech iProduct2 G920 Driving Force Racing Wheel for Xbox One iSerial 3 72fb6b3b9f58 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 32 bNumInterfaces 1 bConfigurationValue 1 iConfiguration 0 bmAttributes 0xa0 (Bus Powered) Remote Wakeup MaxPower 100mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber0 bAlternateSetting 0 bNumEndpoints 2 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass 71 bInterfaceProtocol208 iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x01 EP 1 OUT bmAttributes3 Transfer TypeInterrupt Synch Type None Usage Type Data wMaxPacketSize 0x0040 1x 64 bytes bInterval 4 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes3 Transfer TypeInterrupt Synch Type None Usage Type Data wMaxPacketSize 0x0040 1x 64 bytes bInterval 4 Device Status: 0x0002 (Bus Powered) Remote Wakeup Enabled -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] HID: Force feedback support for the Logitech G920 Wheel
On Sat, Nov 21, 2015 at 1:37 PM, Edwin Velds wrote: > This patch implements force feedback support for the Logitech G920 Driving > Force > Racing Wheel. > > This patch is based on the basic G920 support patch by Simon Wood: > http://www.spinics.net/lists/linux-input/msg42174.html > > The first patch splits and renames the HID++ driver (hid-logitech-hidpp.c) > into > a basic header and implementation in order to allow building additional > features > from multiple sources. Since this only satisfies the needs for the FF driver, > any comments or suggestions are welcome. > > The second patch implements all supported force feedback effects. The force > feedback implementation was not made optional because the G920 (like all > Logitech wheels) can't function without it due to very stiff to default > springs. > > Edwin Velds (1): > HID: Create header for Logitech HID++ driver > HID: Force feedback support for the Logitech G920 > > drivers/hid/Makefile |2 + > drivers/hid/hid-logitech-hidpp-base.c | 1725 ++ > drivers/hid/hid-logitech-hidpp-base.h | 81 ++ > drivers/hid/hid-logitech-hidpp-ff.c | 540 ++ > drivers/hid/hid-logitech-hidpp-ff.h |9 + > drivers/hid/hid-logitech-hidpp.c | 1900 > - > 6 files changed, 2357 insertions(+), 1900 deletions(-) > create mode 100644 drivers/hid/hid-logitech-hidpp-base.c > create mode 100644 drivers/hid/hid-logitech-hidpp-base.h > create mode 100644 drivers/hid/hid-logitech-hidpp-ff.c > create mode 100644 drivers/hid/hid-logitech-hidpp-ff.h > delete mode 100644 drivers/hid/hid-logitech-hidpp.c > > -- > 2.5.0 > You have my: Tested-by: Elias Vanderstuyft Note however that at least a V2 can be expected: I did a review off-list just before this patchset made it to this mailing list, and not all changes are applied yet. Thanks, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl
On Mon, Nov 9, 2015 at 8:50 AM, Benjamin Tissoires wrote:
>
> - Original Message -
>> From: "Elias Vanderstuyft"
>> To: "Benjamin Tissoires"
>> Cc: "Dmitry Torokhov" , "David Herrmann"
>> , "Peter Hutterer"
>> , "open list:HID CORE LAYER"
>> , linux-ker...@vger.kernel.org
>> Sent: Sunday, November 8, 2015 11:55:04 AM
>> Subject: Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and
>> UI_ABS_SETUP ioctl
>>
>> I.e., why not:
>>
>> struct uinput_setup {
>> char name[UINPUT_MAX_NAME_SIZE];
>> struct input_id id;
>> __u32 ff_effects_max;
>> };
>>
>> In case you would change this, also make sure to change the order in
>> the documentation of UI_DEV_SETUP.
>>
>
> Works for me.
> Dmitry, how do you want to handle this change? Me re-sending the whole series
> or you applying the change directly on your tree?
If this is too much of a hassle, please leave it as it was, I was just
being pedantic ;-)
Cheers,
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] HID: Add vendor specific usage pages for Logitech G920
On Mon, Nov 9, 2015 at 9:20 AM, Benjamin Tissoires wrote: > On Sat, Nov 7, 2015 at 5:10 PM, Simon Wood wrote: >> The Logitech G920 uses a couple of vendor specific usage pages, >> which results in incorrect number of axis/buttons being detected. >> >> This patch adds these pages to the 'ignore' list. >> >> Reported-by: Elias Vanderstuyft >> Signed-off-by: Simon Wood >> --- >> drivers/hid/hid-input.c | 2 +- >> include/linux/hid.h | 2 ++ >> 2 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c >> index 53aeaf6..c120be5 100644 >> --- a/drivers/hid/hid-input.c >> +++ b/drivers/hid/hid-input.c >> @@ -959,7 +959,7 @@ static void hidinput_configure_usage(struct hid_input >> *hidinput, struct hid_fiel >> set_bit(EV_REP, input->evbit); >> goto ignore; >> >> - case HID_UP_LOGIVENDOR: >> + case HID_UP_LOGIVENDOR: case HID_UP_LOGIVENDOR2: case >> HID_UP_LOGIVENDOR3: > > One line per case, please. This is my fault, I followed the convention used some lines above: "case HID_GD_SLIDER: case HID_GD_DIAL: case HID_GD_WHEEL:" but we expected this would get comments ;-) Cheers, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF
Currently the user can set ff_effects_max to zero with the EV_FF bit
(and the FF_GAIN and/or FF_AUTOCENTER bits) set,
in this case the uninitialized methods
ff->set_gain and/or ff->set_autocenter can be dereferenced,
resulting in a kernel oops.
Check in uinput_create_device() and
print a helpful message and return -EINVAL in case the check fails.
Signed-off-by: Elias Vanderstuyft
---
Changes in v2:
- Rebase on pending patches from David Herrmann and Benjamin Tissoires:
- v3 Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl
- Input: uinput - rework ABS validation
- Don't require EV_FF bit to be set when ff_effects_max is non-zero
- Move check from uinput_setup_device() to uinput_create_device()
- Update commit description
At the same time, the new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctls were
tested as well (in both orders).
The legacy write() (instead of UINPUT_DEV_SETUP) was also tested.
drivers/input/misc/uinput.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 1d93037..b9d0713 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -272,6 +272,13 @@ static int uinput_create_device(struct uinput_device *udev)
input_set_events_per_packet(dev, 60);
}
+ if (test_bit(EV_FF, dev->evbit) && !udev->ff_effects_max) {
+ printk(KERN_DEBUG "%s: ff_effects_max should be non-zero when
FF_BIT is set\n",
+ UINPUT_NAME);
+ error = -EINVAL;
+ goto fail1;
+ }
+
if (udev->ff_effects_max) {
error = input_ff_create(dev, udev->ff_effects_max);
if (error)
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl
Hi,
On Tue, Aug 25, 2015 at 5:12 PM, Benjamin Tissoires
wrote:
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index 013c9d8..ef6c9f5 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -20,6 +20,11 @@
> * Author: Aristeu Sergio Rozanski Filho
> *
> * Changes/Revisions:
> + * 0.5 08/13/2015 (David Herrmann &
> + * Benjamin Tissoires
> )
> + * - add UI_DEV_SETUP ioctl
> + * - add UI_ABS_SETUP ioctl
> + * - add UI_GET_VERSION ioctl
> * 0.4 01/09/2014 (Benjamin Tissoires
> )
> * - add UI_GET_SYSNAME ioctl
> * 0.3 24/05/2006 (Anssi Hannula )
> @@ -37,8 +42,8 @@
> #include
> #include
>
> -#define UINPUT_VERSION 4
> -
> +#define UINPUT_VERSION 5
> +#define UINPUT_MAX_NAME_SIZE 80
>
> struct uinput_ff_upload {
> __u32 request_id;
> @@ -58,6 +63,79 @@ struct uinput_ff_erase {
> #define UI_DEV_CREATE _IO(UINPUT_IOCTL_BASE, 1)
> #define UI_DEV_DESTROY _IO(UINPUT_IOCTL_BASE, 2)
>
> +struct uinput_setup {
> + struct input_id id;
> + char name[UINPUT_MAX_NAME_SIZE];
> + __u32 ff_effects_max;
> +};
Is there a reason to not follow the same field order as in struct
uinput_user_dev?
I.e., why not:
struct uinput_setup {
char name[UINPUT_MAX_NAME_SIZE];
struct input_id id;
__u32 ff_effects_max;
};
In case you would change this, also make sure to change the order in
the documentation of UI_DEV_SETUP.
Cheers,
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF
Hi Dmitry,
Excuse me for the long delay.
On Thu, Oct 15, 2015 at 2:52 AM, Dmitry Torokhov
wrote:
> On Thu, Sep 17, 2015 at 07:29:48PM +0200, Elias Vanderstuyft wrote:
>> Currently the user can specify a non-zero value for ff_effects_max,
>> without setting the EV_FF bit.
>> Inversely,
>> the user can also set ff_effects_max to zero with the EV_FF bit set,
>> in this case the uninitialized method ff->upload can be dereferenced,
>> resulting in a kernel oops.
>>
>> Instead of adding a check in uinput_create_device() and
>> omitting setup of ff-core infrastructure silently in case the check fails,
>> perform the check early in uinput_setup_device(),
>> and print a helpful message and return -EINVAL in case the check fails.
>>
>> Signed-off-by: Elias Vanderstuyft
>> ---
>> drivers/input/misc/uinput.c | 15 +++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
>> index 345df9b..3a90a16 100644
>> --- a/drivers/input/misc/uinput.c
>> +++ b/drivers/input/misc/uinput.c
>> @@ -393,6 +393,21 @@ static int uinput_setup_device(struct uinput_device
>> *udev,
>> if (IS_ERR(user_dev))
>> return PTR_ERR(user_dev);
>>
>> + if (!!user_dev->ff_effects_max ^ test_bit(EV_FF, dev->evbit)) {
>> + if (user_dev->ff_effects_max)
>> + printk(KERN_DEBUG
>> + "%s: ff_effects_max (%u) should be zero "
>> + "when FF_BIT is not set\n",
>> + UINPUT_NAME, user_dev->ff_effects_max);
>> + else
>> + printk(KERN_DEBUG
>> + "%s: ff_effects_max should be non-zero "
>> + "when FF_BIT is set\n",
>> + UINPUT_NAME);
>
> I do not think this is the right place for this check: userspace is
> allowed to write device structure before calling any ioctls to set
> various bits. Also, userspace doe snot have to explicitly set EV_FF bit
> as input_ff_create() does it for us.
OK, I put it here to be consistent with the uinput_validate_absbits() function,
which checks absbit in case the EV_ABS bit is set,
but I incorrectly assumed the EV_ABS bit was required to be set.
> I think the check should be in uinput_create_device() and we should only
> check case when udev->ff_effects_max is 0 but EV_FF is set.
This made me think about the whole idea whether or not
allowing ff_effects_max to be zero is possible for a FF device.
I think it is perfectly possible to have a FF device with no support
for uploading effects,
but with an adjustable AUTOCENTER-force axis.
Or, more exotically, a device with a trigger-button which on press
automatically emits rumble,
with adjustable GAIN.
The only places where we'd need to change code for allowing this,
is in:
- http://lxr.free-electrons.com/source/drivers/input/ff-core.c?v=4.3#L316
: remove if-then-block
- http://lxr.free-electrons.com/source/drivers/input/misc/uinput.c?v=4.3#L266
: change if-test to "udev->ff_effects_max || test_bit(EV_FF, dev->evbit)"
Of course, the latter change may conflict with your initial reply:
userspace does not have to explicitly set the EV_FF bit in advance;
however it does make sense to set the bit if e.g. only FF_AUTOCENTER
support is available,
but no uploading of effects (ff->upload and friends will still be set,
but not used, thanks to check_effect_access()).
What do you think about this: should I go with "forbid ff_effects_max
to be zero, and check on it" or "allow ff_effects_max to be zero"?
My previous patches wouldn't conflict with either options.
Thanks,
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: input.h: Fix EVIOCSFF macro inconsistency by using _IOW()
On Thu, Sep 17, 2015 at 7:26 PM, Elias Vanderstuyft wrote:
> Just like the EVIOCSABS(abs) macro, use the more compact
> _IOW(..., type) instead of _IOC(_IOC_WRITE, ..., sizeof(type))
> for the EVIOCSFF macro.
>
> Signed-off-by: Elias Vanderstuyft
> ---
> include/uapi/linux/input.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index 731417c..63915a7 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -147,7 +147,7 @@ struct input_keymap_entry {
> #define EVIOCGABS(abs) _IOR('E', 0x40 + (abs), struct input_absinfo)
> /* get abs value/limits */
> #define EVIOCSABS(abs) _IOW('E', 0xc0 + (abs), struct input_absinfo)
> /* set abs value/limits */
>
> -#define EVIOCSFF _IOC(_IOC_WRITE, 'E', 0x80, sizeof(struct
> ff_effect)) /* send a force effect to a force feedback device */
> +#define EVIOCSFF _IOW('E', 0x80, struct ff_effect) /*
> send a force effect to a force feedback device */
> #define EVIOCRMFF _IOW('E', 0x81, int)/*
> Erase a force effect */
> #define EVIOCGEFFECTS _IOR('E', 0x84, int)/*
> Report number of effects playable at the same time */
>
> --
> 1.9.3
>
Hi Dmitry,
Is there anything that prevents this patch to be applied?
About a year ago, we discussed this by message:
https://www.marc.info/?l=linux-input&m=141020112415578&w=1
Thanks,
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Input: Document and check on implicitly defined FF_MAX_EFFECTS
There is an undocumented upper bound for the total number of ff effects:
FF_GAIN (= 96).
This can be found as follows:
- user: write(EV_FF, effect_id, iterations)
calls kernel: ff->playback(effect_id, ...): starts effect "effect_id"
- user: write(EV_FF, FF_GAIN, gain)
calls kernel: ff->set_gain(gain, ...): sets gain
A collision occurs when effect_id equals FF_GAIN.
According to input_ff_event(),
FF_GAIN is the smallest value where a collision occurs.
Therefore the greatest safe value for effect_id is FF_GAIN - 1,
and thus the total number of effects should never exceed FF_GAIN.
Define FF_MAX_EFFECTS as FF_GAIN and check on this limit in ff-core.
Signed-off-by: Elias Vanderstuyft
---
drivers/input/ff-core.c| 5 +
include/uapi/linux/input.h | 8
2 files changed, 13 insertions(+)
diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
index c642082..0bf7008 100644
--- a/drivers/input/ff-core.c
+++ b/drivers/input/ff-core.c
@@ -318,6 +318,11 @@ int input_ff_create(struct input_dev *dev, unsigned int
max_effects)
return -EINVAL;
}
+ if (max_effects > FF_MAX_EFFECTS) {
+ dev_err(&dev->dev, "cannot allocate more than FF_MAX_EFFECTS
effects\n");
+ return -EINVAL;
+ }
+
ff_dev_size = sizeof(struct ff_device) +
max_effects * sizeof(struct file *);
if (ff_dev_size < max_effects) /* overflow */
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 63915a7..42d7933 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -1200,6 +1200,14 @@ struct ff_effect {
#define FF_GAIN0x60
#define FF_AUTOCENTER 0x61
+/*
+ * ff->playback(effect_id = FF_GAIN) is the first effect_id to
+ * cause a collision with another ff method, in this case ff->set_gain().
+ * Therefore the greatest safe value for effect_id is FF_GAIN - 1,
+ * and thus the total number of effects should never exceed FF_GAIN.
+ */
+#define FF_MAX_EFFECTS FF_GAIN
+
#define FF_MAX 0x7f
#define FF_CNT (FF_MAX+1)
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] Input: Improve handling of ff max_effects
This is a patch-set to improve the handling of max_effects in ff-core and uinput. Elias Vanderstuyft (2): Input: Document and check on implicitly defined FF_MAX_EFFECTS Input: uinput: Sanity check on ff_effects_max and EV_FF drivers/input/ff-core.c | 5 + drivers/input/misc/uinput.c | 15 +++ include/uapi/linux/input.h | 8 3 files changed, 28 insertions(+) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] Input: uinput: Sanity check on ff_effects_max and EV_FF
Currently the user can specify a non-zero value for ff_effects_max,
without setting the EV_FF bit.
Inversely,
the user can also set ff_effects_max to zero with the EV_FF bit set,
in this case the uninitialized method ff->upload can be dereferenced,
resulting in a kernel oops.
Instead of adding a check in uinput_create_device() and
omitting setup of ff-core infrastructure silently in case the check fails,
perform the check early in uinput_setup_device(),
and print a helpful message and return -EINVAL in case the check fails.
Signed-off-by: Elias Vanderstuyft
---
drivers/input/misc/uinput.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 345df9b..3a90a16 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -393,6 +393,21 @@ static int uinput_setup_device(struct uinput_device *udev,
if (IS_ERR(user_dev))
return PTR_ERR(user_dev);
+ if (!!user_dev->ff_effects_max ^ test_bit(EV_FF, dev->evbit)) {
+ if (user_dev->ff_effects_max)
+ printk(KERN_DEBUG
+ "%s: ff_effects_max (%u) should be zero "
+ "when FF_BIT is not set\n",
+ UINPUT_NAME, user_dev->ff_effects_max);
+ else
+ printk(KERN_DEBUG
+ "%s: ff_effects_max should be non-zero "
+ "when FF_BIT is set\n",
+ UINPUT_NAME);
+ retval = -EINVAL;
+ goto exit;
+ }
+
udev->ff_effects_max = user_dev->ff_effects_max;
/* Ensure name is filled in */
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Input: input.h: Fix EVIOCSFF macro inconsistency by using _IOW()
Just like the EVIOCSABS(abs) macro, use the more compact
_IOW(..., type) instead of _IOC(_IOC_WRITE, ..., sizeof(type))
for the EVIOCSFF macro.
Signed-off-by: Elias Vanderstuyft
---
include/uapi/linux/input.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 731417c..63915a7 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -147,7 +147,7 @@ struct input_keymap_entry {
#define EVIOCGABS(abs) _IOR('E', 0x40 + (abs), struct input_absinfo)
/* get abs value/limits */
#define EVIOCSABS(abs) _IOW('E', 0xc0 + (abs), struct input_absinfo)
/* set abs value/limits */
-#define EVIOCSFF _IOC(_IOC_WRITE, 'E', 0x80, sizeof(struct
ff_effect)) /* send a force effect to a force feedback device */
+#define EVIOCSFF _IOW('E', 0x80, struct ff_effect) /* send
a force effect to a force feedback device */
#define EVIOCRMFF _IOW('E', 0x81, int)/*
Erase a force effect */
#define EVIOCGEFFECTS _IOR('E', 0x84, int)/*
Report number of effects playable at the same time */
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Conversion between unsigned and signed for "ff_effects_max" in uinput and input.
Hi everyone, Making observations based on the following headers: uinput.h: "(struct uinput_device).ff_effects_max" is defined as "unsigned int". uapi/uinput.h: "(struct uinput_user_dev).ff_effects_max" is defined as "__u32". vs input.h: "(struct ff_device).max_effects" is defined as "int", however, signature of input_ff_create() in input.h is: int input_ff_create(struct input_dev *dev, unsigned int max_effects) Why is "(struct ff_device).max_effects" defined as a signed integer, instead of an unsigned integer, as defined in the majority of the headers? If that question is cleared out, I would like to point to a potential integer overflow in the assignment in ff-core.c::input_ff_create(): ff->max_effects = max_effects; (http://lxr.free-electrons.com/source/drivers/input/ff-core.c#L337) Although physically unlikely that max_effects would exceed 0x7FFF, shouldn't there be a check to prevent "ff->max_effects" becoming negative? Note that using UInput, the user can define its own value of max_effects, so adding a check might be justified. Feedback would be nice. Thanks, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Uinput: Deadlock upon UI_DEV_DESTROY ioctl while a non-zero number of ff_effects is still active (uploaded)
Hi everyone, In the process of extending the libsuinput library (https://github.com/tuomasjjrasanen/python-uinput/tree/master/libsuinput/src) to support force feedback, I encountered some bugs in the kernel. For some bugs, I'm not sure for the right fix, so it's safer to first present the problem and ask for feedback. For clarity, I divided the bugs between different mail subjects. This subject handles about a deadlock encountered when userspace issues a UI_DEV_DESTROY ioctl on a uinput device, while a non-zero number of ff_effects is still active (i.e. the application that issued the ioctl has not yet erased all uploaded effects). The mutex causing the deadlock is "(struct uinput_device).mutex". The chain of the deadlock is as follows: - uinput.c::uinput_ioctl_handler() LOCKS using mutex_lock_interruptible() - uinput.c::uinput_ioctl_handler() calls uinput.c::uinput_destroy_device() - uinput.c::uinput_destroy_device() calls input.c::input_unregister_device() - input.c::input_unregister_device() calls input.c::__input_unregister_device() - input.c::__input_unregister_device() calls handle->handler->disconnect() - evdev.c::evdev_disconnect calls evdev.c::evdev_cleanup() - evdev.c::evdev_cleanup() calls input.c::input_flush_device() - input.c::input_flush_device() calls dev->flush() - ff-core.c::flush_effects() calls ff-core.c::erase_effect() - ff-core.c::erase_effect() calls ff->erase() - uinput.c::uinput_dev_erase_effect() calls uinput.c::uinput_request_submit() - uinput.c::uinput_request_submit() calls uinput.c::uinput_request_send() - uinput.c::uinput_request_send() LOCKS using mutex_lock_interruptible() Hints for solving this problem are much appreciated. Maybe by setting a flag when UI_DEV_DESTROY ioctl is issued, and avoiding to acquire the mutex in uinput_request_send() when this flag is found to be set? Thanks, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/12] HID: hid-lg, hid-lg4ff: Mostly cleanup patches
On Sat, Mar 21, 2015 at 12:47 PM, Michal Malý wrote: > Hello everyone, > > this is a mostly boring series that deals with a few inconsistencies in the > code that have accumulated over the years. Besides that it patches up a > handful > of problems such a return values not being checked etc. > > The only significant change comes in patch 0007 which introduces a spinlock to > handle concurrent access to the HID report that is used by the driver to send > data to the wheel. I would appreciate some comments on this one, particularly > on the way it handles deinitialization. > > Michal Malý (12): > HID: hid-lg4ff: (Cleanup) Remove double underscore prefix from numeric > types. > HID: hid-lg4ff: (Cleanup) Remove "hid_" prefix from some functions > names. > HID: hid-lg4ff: (Cleanup) Replace DEVICE_ATTR_RW with DEVICE_ATTR to > have all internal functions prefixed with "lg4ff_" > HID: hid-lg4ff: (Cleanup) Remove unused variable from the > "lg4ff_device_entry" struct. > HID: hid-lg4ff: Update a warning message for a case where device is > incorrectly flagged to be handled by hid-lg4ff in hid-lg. > HID: hid-lg: Check return values from lg[N]ff_init() > HID: hid-lg4ff: Protect concurrent access to the output HID report > values with a spinlock. > HID: hid-lg4ff: Store pointer to the output HID report struct in the > device entry struct. > HID: hid-lg4ff: Constify those members of lg4ff_device_entry struct > whose value is not supposed to change. > HID: hid-lg4ff: Allow the driver to continue without sysfs interface. > HID: hid-lg4ff: Update respective sysfs interface documentation > HID: hid-lg: Only one of LG_FF flags can be set for a given device. > > .../ABI/testing/sysfs-driver-hid-logitech-lg4ff| 8 +- > drivers/hid/hid-lg.c | 21 +- > drivers/hid/hid-lg4ff.c| 459 > ++--- > drivers/hid/hid-lg4ff.h| 4 +- > 4 files changed, 319 insertions(+), 173 deletions(-) > > -- > 2.3.3 > Tested on kernel 3.18.6 (I had to apply some uptream patches first). I tried to crash the kernel by simultaneously running many instances of FF applications (which made use of effects with nonzero attack times, thus the ff-memless timer's timeout got modified often), and then unplugging the device while in use. Instead of crashing, the kernel behaved like it should, no panics or lockups were found. Tested with to following devices: - Logitech MOMO (Black) : PID 0xCA03 - Logitech Formula Vibration : PID 0xCA04 Tested-by: Elias Vanderstuyft Regards, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/4] HID: hid-lg4ff: Improve handling of Logitech multimode gaming wheels
On Sun, Feb 15, 2015 at 3:03 AM, Michal Malý wrote: > This patch series improves handling of various Logitech gaming wheels and > allows switching between various compatibility modes which might be useful > to improve compatibility with very old games and testing purposes. > > Signed-off-by: Michal Malý > > v2: - Rebased against latest linux-next > - Document the newly introduced sysfs interfaces in appropriate commits > - Assume that we are targeting kernel version 4.1 > - Fix a misleading error message regarding unsupported wheel mode > > Michal Malý (4): > Identify Logitech gaming wheels in compatibility modes accordingly to > Logitech specifications > Display the real wheel model and supported alternate modes through > sysfs. This applies only to multimode wheels. > Introduce a module parameter to disable automatic switch of Logitech > gaming wheels from compatibility to native mode. This only applies > to multimode wheels. > Allow switching of Logitech gaming wheels between available > compatibility modes through sysfs. This only applies to multimode > wheels. > > .../ABI/testing/sysfs-driver-hid-logitech-lg4ff| 45 ++ > drivers/hid/hid-lg.c | 6 + > drivers/hid/hid-lg4ff.c| 605 > ++--- > 3 files changed, 580 insertions(+), 76 deletions(-) > > -- > 2.3.0 > Applied cleanly on kernel 3.18.6, and retested with: - Logitech Formula Vibration Wheel - Logitech MOMO (Black) Wheel Everything keeps working like it was before, which is correct because these wheels are no multimode wheels. Tested-by: Elias Vanderstuyft Thanks, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] HID: Improve handling of multimode Logitech handling wheels
On Fri, Feb 6, 2015 at 5:34 PM, Michal Malý wrote: > This patch series improves handling of various Logitech gaming wheels and > allows switching between various compatibility modes which might be useful > to improve compatibility with very old games and testing purposes. > > Signed-off-by: Michal Malý > > Michal Malý (4): > Identify Logitech gaming wheels in compatibility modes accordingly to >Logitech specifications > Display the real wheel model and supported alternate modes through > sysfs. This applies only to multimode wheels. > Introduce a module parameter to disable automatic switch of Logitech > gaming wheels from compatibility to native mode. This only applies > to multimode wheels. > Allow switching of Logitech gaming wheels between available > compatibility modes through sysfs. This only applies to multimode > wheels. > > .../ABI/testing/sysfs-driver-hid-logitech-lg4ff| 45 ++ > drivers/hid/hid-lg.c | 6 + > drivers/hid/hid-lg4ff.c| 608 > ++--- > 3 files changed, 583 insertions(+), 76 deletions(-) > > -- > 2.2.2 > Applied cleanly on kernel 3.17.8, and tested with: - Logitech Formula Vibration Wheel - Logitech MOMO (Black) Wheel Everything keeps working like it was before, which is correct because these wheels are no multimode wheels. Tested-by: Elias Vanderstuyft Thanks, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: EVIOCSFF macro inconsistency
On Mon, Sep 8, 2014 at 10:24 PM, Dmitry Torokhov
wrote:
> On Monday, September 08, 2014 09:03:11 PM Elias Vanderstuyft wrote:
>> On Mon, Sep 8, 2014 at 8:31 PM, Dmitry Torokhov
>>
>> wrote:
>> > Hi Elias,
>> >
>> > On Mon, Sep 08, 2014 at 08:14:13PM +0200, Elias Vanderstuyft wrote:
>> >> Hi everyone,
>> >>
>> >> After inspecting the header file, I found that there
>> >> is one single ioctl value macro that is inconsistent w.r.t. the other
>> >>
>> >> macros that do an IOC_WRITE :
>> >> EVIOCSFF _IOC(_IOC_WRITE, 'E', 0x80, sizeof(struct ff_effect))
>> >>
>> >> Why not define it as follows? :
>> >> EVIOCSFF _IOW('E', 0x80, struct ff_effect)
>> >>
>> >> Apart from having a more readable definition, it also explicitly
>> >> reveals type info ("struct ff_effect").
>> >
>> > I think it is just historical.
>> >
>> >> Is it worth to create a patch to fix it?
>> >
>> > Sure, why not.
>>
>> Cool, thanks!
>>
>> So probably the same applies to the IOC_READ counter parts? :
>> EVIOCGBIT(ev,len)_IOC(_IOC_READ, 'E', 0x20 + (ev), len)
>> EVIOCGKEY(len)_IOC(_IOC_READ, 'E', 0x18, len)
>> EVIOCGLED(len)_IOC(_IOC_READ, 'E', 0x19, len)
>> EVIOCGMTSLOTS(len)_IOC(_IOC_READ, 'E', 0x0a, len)
>> EVIOCGNAME(len)_IOC(_IOC_READ, 'E', 0x06, len)
>> EVIOCGPHYS(len)_IOC(_IOC_READ, 'E', 0x07, len)
>> EVIOCGPROP(len)_IOC(_IOC_READ, 'E', 0x09, len)
>> EVIOCGSND(len)_IOC(_IOC_READ, 'E', 0x1a, len)
>> EVIOCGSW(len)_IOC(_IOC_READ, 'E', 0x1b, len)
>> EVIOCGUNIQ(len)_IOC(_IOC_READ, 'E', 0x08, len)
>> to be converted to:
>> EVIOCGBIT(ev,len)_IOR('E', 0x20 + (ev), len)
>> EVIOCGKEY(len)_IOR('E', 0x18, len)
>> EVIOCGLED(len)_IOR('E', 0x19, len)
>> EVIOCGMTSLOTS(len)_IOR('E', 0x0a, len)
>> EVIOCGNAME(len)_IOR('E', 0x06, len)
>> EVIOCGPHYS(len)_IOR('E', 0x07, len)
>> EVIOCGPROP(len)_IOR('E', 0x09, len)
>> EVIOCGSND(len)_IOR('E', 0x1a, len)
>> EVIOCGSW(len)_IOR('E', 0x1b, len)
>> EVIOCGUNIQ(len)_IOR('E', 0x08, len)
>
> No, because 'len' is not a type.
Indeed, just found out by myself, thank you for the confirmation.
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: EVIOCSFF macro inconsistency
On Mon, Sep 8, 2014 at 8:31 PM, Dmitry Torokhov
wrote:
> Hi Elias,
>
> On Mon, Sep 08, 2014 at 08:14:13PM +0200, Elias Vanderstuyft wrote:
>> Hi everyone,
>>
>> After inspecting the header file, I found that there
>> is one single ioctl value macro that is inconsistent w.r.t. the other
>> macros that do an IOC_WRITE :
>> EVIOCSFF _IOC(_IOC_WRITE, 'E', 0x80, sizeof(struct ff_effect))
>> Why not define it as follows? :
>> EVIOCSFF _IOW('E', 0x80, struct ff_effect)
>> Apart from having a more readable definition, it also explicitly
>> reveals type info ("struct ff_effect").
>
> I think it is just historical.
>
>> Is it worth to create a patch to fix it?
>
> Sure, why not.
Cool, thanks!
So probably the same applies to the IOC_READ counter parts? :
EVIOCGBIT(ev,len)_IOC(_IOC_READ, 'E', 0x20 + (ev), len)
EVIOCGKEY(len)_IOC(_IOC_READ, 'E', 0x18, len)
EVIOCGLED(len)_IOC(_IOC_READ, 'E', 0x19, len)
EVIOCGMTSLOTS(len)_IOC(_IOC_READ, 'E', 0x0a, len)
EVIOCGNAME(len)_IOC(_IOC_READ, 'E', 0x06, len)
EVIOCGPHYS(len)_IOC(_IOC_READ, 'E', 0x07, len)
EVIOCGPROP(len)_IOC(_IOC_READ, 'E', 0x09, len)
EVIOCGSND(len)_IOC(_IOC_READ, 'E', 0x1a, len)
EVIOCGSW(len)_IOC(_IOC_READ, 'E', 0x1b, len)
EVIOCGUNIQ(len)_IOC(_IOC_READ, 'E', 0x08, len)
to be converted to:
EVIOCGBIT(ev,len)_IOR('E', 0x20 + (ev), len)
EVIOCGKEY(len)_IOR('E', 0x18, len)
EVIOCGLED(len)_IOR('E', 0x19, len)
EVIOCGMTSLOTS(len)_IOR('E', 0x0a, len)
EVIOCGNAME(len)_IOR('E', 0x06, len)
EVIOCGPHYS(len)_IOR('E', 0x07, len)
EVIOCGPROP(len)_IOR('E', 0x09, len)
EVIOCGSND(len)_IOR('E', 0x1a, len)
EVIOCGSW(len)_IOR('E', 0x1b, len)
EVIOCGUNIQ(len)_IOR('E', 0x08, len)
Thanks,
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
EVIOCSFF macro inconsistency
Hi everyone,
After inspecting the header file, I found that there
is one single ioctl value macro that is inconsistent w.r.t. the other
macros that do an IOC_WRITE :
EVIOCSFF _IOC(_IOC_WRITE, 'E', 0x80, sizeof(struct ff_effect))
Why not define it as follows? :
EVIOCSFF _IOW('E', 0x80, struct ff_effect)
Apart from having a more readable definition, it also explicitly
reveals type info ("struct ff_effect").
Is it worth to create a patch to fix it?
Thanks,
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PATCH hid: Implement mode switching on Logitech gaming wheels accordingly to the documentation
On Wed, Jul 30, 2014 at 12:10 PM, Michal Malý wrote: > Implement mode switching on Logitech gaming wheels accordingly to the > documentation > > Signed-off-by: Michal Malý Tested all lg4ff_switch_force_mode modes (0, 1, 2, 3, default), on both: - MOMO (Black) : PID 0xCA03 - Formula Vibration : PID 0xCA04 Behaviour didn't deviate from normal, which is as expected because these devices don't use the native versus compatibility mode, according to the documentation. So I consider this patch successful for these devices: Tested-by: Elias Vanderstuyft Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] lib: int_sqrt: add int64_sqrt routine
On Mon, Jun 23, 2014 at 11:43 AM, Barry Song <21cn...@gmail.com> wrote:
> From: Yibo Cai
>
> Get square root of a 64-bit digit on 32bit platforms. CSR SiRFSoC
> touchscreen driver needs it.
>
> Signed-off-by: Yibo Cai
> Signed-off-by: Barry Song
> ---
> include/linux/kernel.h | 1 +
> lib/int_sqrt.c | 27 +++
> 2 files changed, 28 insertions(+)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 4c52907..6e63081 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -418,6 +418,7 @@ struct pid;
> extern struct pid *session_of_pgrp(struct pid *pgrp);
>
> unsigned long int_sqrt(unsigned long);
> +unsigned long long int64_sqrt(unsigned long long);
>
> extern void bust_spinlocks(int yes);
> extern int oops_in_progress; /* If set, an oops, panic(), BUG() or
> die() is in progress */
> diff --git a/lib/int_sqrt.c b/lib/int_sqrt.c
> index 1ef4cc3..2aa9fcc 100644
> --- a/lib/int_sqrt.c
> +++ b/lib/int_sqrt.c
> @@ -36,3 +36,30 @@ unsigned long int_sqrt(unsigned long x)
> return y;
> }
> EXPORT_SYMBOL(int_sqrt);
> +
> +/*
> + * Square root of a 64-bit digit.
> + * Same as int_sqrt on 64-bit platforms where "long" equals "long long"
> + */
> +unsigned long long int64_sqrt(unsigned long long x)
> +{
> + unsigned long long m = 0, y = 0, b = 0;
> +
> + if (x <= 1)
> + return x;
> +
> + m = 1ULL << (BITS_PER_LONG_LONG - 2);
> + while (m != 0) {
> + b = y + m;
> + y >>= 1;
> +
> + if (x >= b) {
> + x -= b;
> + y += m;
> + }
> + m >>= 2;
> + }
> +
> + return y;
> +}
> +EXPORT_SYMBOL(int64_sqrt);
>
Maybe it's a stupid question (I'm new to kernel programming),
but why don't you use a for-loop instead of a while-loop?
(apart from following the already-existing code of int_sqrt.c)
E.g.:
for (m = 1ULL << (BITS_PER_LONG_LONG - 2); m != 0; m >>= 2) {
b = y + m;
y >>= 1;
if (x >= b) {
x -= b;
y += m;
}
}
Instead of:
m = 1ULL << (BITS_PER_LONG_LONG - 2);
while (m != 0) {
b = y + m;
y >>= 1;
if (x >= b) {
x -= b;
y += m;
}
m >>= 2;
}
Thanks,
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/24] input: Add ff-memless-next module
On Tue, May 20, 2014 at 11:26 PM, Elias Vanderstuyft wrote: > On Tue, May 20, 2014 at 10:58 PM, Michal Malý > wrote: >> On Tuesday 20 of May 2014 16:16:12 si...@mungewell.org wrote: >>> Regarding the question of emulated vs. real effects, can we extend the API >>> so that applications can know which effects are really supported, and >>> enable/disable emulation somehow? >> >> I suppose that a few extra flags (FF_PERIODIC_EMULATED etc.) defined in >> "uapi/linux/input.h" should suffice. > > @Dmitry: > Now that we're talking about API changes, I would like to propose some > additional things in the Linux FF API: > > - Possibility to *get* FF state variables, such as the value of the > current GAIN, and the current AUTOCENTER: > At the moment, only *setting* is possible. > This poses a problem when implementing proper AUTOCENTER state > recovery (upon closing a Windows program) in Wine, because the initial > AUTOCENTER state was unknown. > The same applies for GAIN. > > - Introduce multiple types of GAIN, apart from the current global > GAIN, e.g. GAIN_CONSTANT and GAIN_DAMPER: > This is mainly on request of some users, especially Logitech users: on > Windows Logitech additionally supports to set the global Damper and > Spring gain. > Maybe it would be nicer to create a global gain for every supported > Linux FF effect? And Roland Bosa (from Logitech) also agrees with the following addition/modification to the Linux FF API: - Infinite iterations, or in Linux FF terms: playback of an effect with an event count of infinity. Right now, an event count of zero stops the effect, while the maximum count is INT_MAX. An event count value of '-1' can be used to indicate infinity. (However note that it is somewhat inconsistent with the infinite effect "duration" we already support: in that case '0' is used) Is this feasible? (in terms of backward compatibility) Thanks, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/24] input: Add ff-memless-next module
On Tue, May 20, 2014 at 11:38 PM, Roland Bosa wrote: > On 05/20/2014 12:00 PM, Michal Malı wrote: > There's a healthy amount of > code in the Windows driver that you would call 'quirks' which deals with > deciding how to allocate multiple springs and dampers to a single slot. But 2 Spring effects with different offsets and non-zero effective force can't be combined into a single slot (without streaming them with Constant force), right? > Sometimes, even the springs and dampers are being streamed in via the > constants force, but that requires (as you pointed out earlier) a fast > update rate and some "smartness" OK, is this method used when all force-slots are already full? Or is it only used for some specific (restricted) devices, such as devices with only 2 force slots? And in this case, is the position of the device monitored at a speed higher than 8ms, in order to keep the feedback-loop stable? Thanks, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/24] input: Add ff-memless-next module
On Wed, May 21, 2014 at 3:35 PM, Elias Vanderstuyft wrote: > On Wed, May 21, 2014 at 4:13 AM, Michal Malý > wrote: >> Proper decoupling of the userspace and driver is the only important thing >> that >> is missing from the current code. > > Also dynamic updating of periodic effects is not yet supported in > "ff-memless-next". > This can also be important in simulation games (=second type), e.g. > the "rFactor" game. Oh, and I forgot some other things that need to be mentioned: - Trigger-button is not yet implemented, as far as I know - Custom force effects, in Linux it is called FF_CUSTOM. (however I think we can neglect this effect for now) - Infinite iterations in the Linux FF API is not supported. @Roland: Now I understand why Logitech Windows drivers have bugs in the periodic implementation of the DInput specs: Logitech seems to have based their drivers on the Immersion Studio visualisation of the to-be-generated periodic effects, and these visualisations are not conform with the DInput specs of MSDN. Take a quick look at the MSDN specs for e.g. SawtoothUp: http://msdn.microsoft.com/en-us/library/windows/desktop/ee418719%28v=vs.85%29.aspx Now open the Immersion Studio (demo) and create a standard SawtoothUp effect, and look at the visualisation... It's wrong, right? Same applies for SawtoothDown and Triangle. Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/24] input: Add ff-memless-next module
On Wed, May 21, 2014 at 12:17 PM, Nestor Lopez Casado wrote: > Elias, Simon, Michal, > > It is unfortunate that we didn't get back to you in 2013 when you > asked for help in reverse engineering, oftentimes we have conflicting > priorities and a particular request may be left laying around for some > time before being addressed. But please, don't hesitate to re-kick us > if you feel we are not being reactive. > > Let me clearly say that our intentions are to disclose as much > information as possible and to help the community in having great > support to all our hardware across all platforms. We have zero problem > with being asked for information. OK, thanks for your reply. Don't worry, I understand ;) Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/24] input: Add ff-memless-next module
On Wed, May 21, 2014 at 4:13 AM, Michal Malý wrote: > On Tuesday 20 of May 2014 18:17:51 Roland Bosa wrote: >> >> The file format of an IFR is probably easily deducible. There's a lot of >> textual clues to parameters and the values are also written out in >> string form. >> >> I don't have a FEdit file at hand, but I suppose it will be similar. > > I believe that Elias successfully reverse engineered the effect file format > produced by FEdit. There is no support for this kind of prefabricated effects > in the Linux FF API. Of course I did, Simon, what did you expect? :P Here you are: http://www.winehq.org/pipermail/wine-devel/2014-February/103108.html It contains a description of the DInput effect file format (.ffe), a working Python script to load these files and dump their contents, as well as a bunch of example files and tests/results. The FFE file format is binary RIFF, if you're interested. But I prefer not to include such things in the Linux kernel, I believe this is the task of the userspace application. I'm planning to implement this function into Wine, but maybe SDL would benefit from similar functionality too (using an open file-format instead, of course)? Anyway, this is off-topic. Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/24] input: Add ff-memless-next module
On Wed, May 21, 2014 at 4:13 AM, Michal Malý wrote: > On Tuesday 20 of May 2014 18:17:51 Roland Bosa wrote: >> In any case, the USB traffic should be decoupled from the app. Any force >> updates should only change state in the ff-memless[-next] driver. Any >> change there should trickle down to a 'slot' representation of the >> device. If there's any change in the slots, the device is marked as >> 'dirty' and USB transfers are scheduled to send the latest state to the >> physical device. >> >> The scheduling should keep track of how many requests are in-flight and >> delay writing the next output, until the previous one has completed. > > The approach I had in mind would keep track of the last effect that made it to > the device and the last effect that arrived from userspace. This would be > stored for each effect slot. An update would be scheduled at the desired > update > rate. The updating routine would figure out the state change between last > update and "now", send the required data to the device and reschedule itself. > The routine could check if there are any USB transfers still running and > reschedule itself immediately. I've got another/(a more detailed?) idea about this: We can use round-robin scheduling for the slots, and use a priority queue for e.g. LED data or data that doesn't fit in the slot-methodology and doesn't permit in-between packets to be lost. And I believe that ff-memless-next should generate a periodic force with a constant/defined sample-period/update-rate, while the layer between ff-memless-next and USB transfers, i.e. the round-robin and priority queue scheduler, should send the dirty slots as soon as possible, i.e. when the USB pipe is clear or when some custom #maxAllowedPacketsInUsbBuffer (optimized for somewhere between latency and throughput) is not exceeded by the kernel USB queue. Now the question is, can we query the state of the kernel USB queue? Optionally, this method of packet scheduling could be used by other kernel drivers (even non-FF drivers) as well, and can further increase flexibility of ff-memless-next and the HW-specific drivers, and it will reduce code duplication. >> Question back to the community: are there APIs in the USB layer to check >> for presence of in-progress requests? Can one add a 'completion' >> callback to a request, that gets invoked on completion/cancellation? > > For instance "usb_submit_urb()" can have a completion handler that is called > once the transfer is done. The current code uses "hid_hw_request()" which is > asynchronous and doesn't report anything back. > > Proper decoupling of the userspace and driver is the only important thing that > is missing from the current code. Also dynamic updating of periodic effects is not yet supported in "ff-memless-next". This can also be important in simulation games (=second type), e.g. the "rFactor" game. Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/24] input: Add ff-memless-next module
On Tue, May 20, 2014 at 10:58 PM, Michal Malý wrote: > On Tuesday 20 of May 2014 16:16:12 si...@mungewell.org wrote: >> Regarding the question of emulated vs. real effects, can we extend the API >> so that applications can know which effects are really supported, and >> enable/disable emulation somehow? > > I suppose that a few extra flags (FF_PERIODIC_EMULATED etc.) defined in > "uapi/linux/input.h" should suffice. @Dmitry: Now that we're talking about API changes, I would like to propose some additional things in the Linux FF API: - Possibility to *get* FF state variables, such as the value of the current GAIN, and the current AUTOCENTER: At the moment, only *setting* is possible. This poses a problem when implementing proper AUTOCENTER state recovery (upon closing a Windows program) in Wine, because the initial AUTOCENTER state was unknown. The same applies for GAIN. - Introduce multiple types of GAIN, apart from the current global GAIN, e.g. GAIN_CONSTANT and GAIN_DAMPER: This is mainly on request of some users, especially Logitech users: on Windows Logitech additionally supports to set the global Damper and Spring gain. Maybe it would be nicer to create a global gain for every supported Linux FF effect? Thanks, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/24] input: Add ff-memless-next module
On Wed, May 14, 2014 at 10:35 AM, Michal Malý wrote: > Hi Dmitry, > > thank you for reviewing this. > > On Tuesday 13 of May 2014 23:38:06 Dmitry Torokhov wrote: >> On Sat, Apr 26, 2014 at 05:02:00PM +0200, Michal Malý wrote: >> > + >> > +/** DEFINITION OF TERMS >> > + * >> > + * Combined effect - An effect whose force is a superposition of forces >> > + * generated by all effects that can be added together. >> > + * Only one combined effect can be playing at a time. >> > + * Effects that can be added together to create a >> > combined + * effect are FF_CONSTANT, FF_PERIODIC and >> > FF_RAMP. + * Uncombinable effect - An effect that cannot be combined with >> > another effect. + * All conditional effects - >> > FF_DAMPER, FF_FRICTION, + * FF_INERTIA and >> > FF_SPRING are uncombinable. + * Number of >> > uncombinable effects playing simultaneously + * >> > depends on the capabilities of the hardware. + * Rumble effect - An >> > effect generated by device's rumble motors instead of + * >> > force feedback actuators. >> > + * >> > + * >> > + * HANDLING OF UNCOMBINABLE EFFECTS >> > + * >> > + * Uncombinable effects cannot be combined together into just one effect, >> > at + * least not in a clear and obvious manner. Therefore these effects >> > have to + * be handled individually by ff-memless-next. Handling of these >> > effects is + * left entirely to the hardware-specific driver, >> > ff-memless-next merely + * passes these effects to the hardware-specific >> > driver at appropriate time. + * ff-memless-next provides the UPLOAD >> > command to notify the hardware-specific + * driver that the userspace is >> > about to request playback of an uncombinable + * effect. The >> > hardware-specific driver shall take all steps needed to make + * the >> > device ready to play the effect when it receives the UPLOAD command. + * >> > The actual playback shall commence when START_UNCOMB command is received. >> > + * Opposite to the UPLOAD command is the ERASE command which tells + * >> > the hardware-specific driver that the playback has finished and that + * >> > the effect will not be restarted. STOP_UNCOMB command tells >> > + * the hardware-specific driver that the playback shall stop but the >> > device + * shall still be ready to resume the playback immediately. >> > + * >> > + * In case it is not possible to make the device ready to play an >> > uncombinable + * effect (all hardware effect slots are occupied), the >> > hardware-specific + * driver may return an error when it receives an >> > UPLOAD command. If the >> This part concerns me. It seems to me that devices supporting >> "uncombinable" effects are in fact not memoryless devices and we should >> not be introducing this term here. If the goal is to work around limited >> number of effect slots in the devices by combining certain effects then >> it needs to be done at ff-core level as it will be potentially useful >> for all devices. > > Force generated by a conditional effect (referred to as "uncombinable" within > ff-memless-next to make the distinction clear) depends on a position of the > device. For instance the more a device is deflected from a neutral position > the > greater force FF_SPRING generates. A truly memoryless device would have to > report its position to the driver, have it calculate the appropriate force and > send it back to the device. IMHO such a loop would require a very high USB > polling rate to play conditional effects with acceptable quality. > > We know for a fact that at least many (all?) Logitech devices that support > conditional effects use this "semi-memoryless" approach where FF_CONSTANT and > FF_PERIODIC are handled in the memoryless fashion and conditional effects are > uploaded to the device (in a somewhat simplified form). The amount of effects > that can be uploaded to a device is limited which is why ff-memless-next uses > two steps (UPLOAD/ERASE and START/STOP) to handle these effects. > > Conditional effects - even if they are of the same type - cannot be > effectively > combined into one because superposition doesn't seem to work here so they have > to be processed one by one. > > If we ever come across a really memoryless device it should not be > particularly difficult to add another callback to ff-memless-next which would > emulate conditional effects with constant force. And I should add that even the conditional effects themselves are handled semi-memless by the Logitech devices: the effects' time parameters are handled in a memless way: the scheduling has to be done by the driver, and is not uploaded to the device. Compare this with a fully non-memless device, an example of a driver that handles such devices: "hid-pidff.c" Here, all effect data is sent directly to the device, also the time-related parameters. Elias -- To unsubscribe from this list: send the line "
Re: [PATCH v3 23/24] hid: Port hid-lg4ff to ff-memless-next
st(hid, report, HID_REQ_SET_REPORT);
> + switch (command->cmd) {
> + case MLNX_START_COMBINED:
> + return hid_lg4ff_start_combined(hid, report,
> &command->u.simple_force);
> + break;
> + case MLNX_STOP_COMBINED:
> + return hid_lg4ff_stop_combined(hid, report);
> break;
> + default:
> + dbg_hid("Unsupported effect command");
> + return -EINVAL;
> }
> - return 0;
> }
>
> /* Sends default autocentering command compatible with
> @@ -610,7 +633,7 @@ int lg4ff_init(struct hid_device *hid)
> for (j = 0; lg4ff_devices[i].ff_effects[j] >= 0; j++)
> set_bit(lg4ff_devices[i].ff_effects[j], dev->ffbit);
>
> - error = input_ff_create_memless(dev, NULL, hid_lg4ff_play);
> + error = input_ff_create_mlnx(dev, (void *)NULL, hid_lg4ff_control,
> FF_UPDATE_RATE);
>
> if (error)
> return error;
> --
> 1.9.2
>
You can add Tested-by: Elias Vanderstuyft
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/24] Introduce ff-memless-next as an improved replacement for ff-memless
On Mon, Apr 21, 2014 at 12:29 AM, wrote: > >> Did the WiiWheel have working FF_CONSTANT before this patchset? >> It would be weird if yes, because lg4ff is also used for MOMO-Black >> which works fine. > > Yes, I think that the force requested by fftest was just too weak to > actually move the wheel. ffcfstress (or whatever it is called) works OK. Ah, nice to hear. I had some additional questions that might benefit to the porting of the lgff and sony driver, concerning the optimal value for FF_UPDATE_RATE (at the moment it defaults to a safe 50ms): - Could you try to sniff FF command USB traffic with the Windows driver of the WingMan Force, just to learn what the update interval is when playing a ConstantForce effect with an envelope (I'm only interested in the envelope part)? - If official/decent Windows drivers are present for one of the Sony controllers you have, can you please do the same, but now when playing a vibration effect (using XInput)? Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/24] Port hid-lg3ff to ff-memless-next
On Wed, Apr 9, 2014 at 1:24 PM, Michal Malý
wrote:
> Port hid-lg3ff to ff-memless-next
>
> Signed-off-by: Michal Malý
> ---
> drivers/hid/Kconfig | 2 +-
> drivers/hid/hid-lg3ff.c | 56
> +++--
> 2 files changed, 37 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index c7794ae..6f2941a 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -382,7 +382,7 @@ config LOGIRUMBLEPAD2_FF
> config LOGIG940_FF
> bool "Logitech Flight System G940 force feedback support"
> depends on HID_LOGITECH
> - select INPUT_FF_MEMLESS
> + select INPUT_FF_MEMLESS_NEXT
> help
> Say Y here if you want to enable force feedback support for Logitech
> Flight System G940 devices.
> diff --git a/drivers/hid/hid-lg3ff.c b/drivers/hid/hid-lg3ff.c
> index 8c2da18..a85f8e5 100644
> --- a/drivers/hid/hid-lg3ff.c
> +++ b/drivers/hid/hid-lg3ff.c
> @@ -23,9 +23,12 @@
>
> #include
> #include
> +#include
>
> #include "hid-lg.h"
>
> +#define FF_UPDATE_RATE 50
> +
> /*
> * G940 Theory of Operation (from experimentation)
> *
> @@ -58,12 +61,11 @@ struct lg3ff_device {
> };
>
> static int hid_lg3ff_play(struct input_dev *dev, void *data,
> -struct ff_effect *effect)
> +const struct mlnx_effect_command *command)
> {
> 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
> @@ -72,30 +74,37 @@ static int hid_lg3ff_play(struct input_dev *dev, void
> *data,
> memset(report->field[0]->value, 0,
>sizeof(__s32) * report->field[0]->report_count);
>
> - switch (effect->type) {
> - case FF_CONSTANT:
> -/*
> - * 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;
> + /* send command byte */
> + report->field[0]->value[0] = 0x51;
>
> -/*
> - * Sign backwards from other Force3d pro
> - * which get recast here in two's complement 8 bits
> - */
> + switch (command->cmd) {
> + case MLNX_START_COMBINED: {
> + const struct mlnx_simple_force *simple_force =
> &command->u.simple_force;
> + /* Scale down from MLNX range */
> + const int x = 0x80 - (simple_force->x * 0xff / 0x);
> + const int y = simple_force->y * 0xff / 0x;
Similarly as in the lgff port, change previous two lines by:
const int x = -(simple_force->x * 0xff / 0x);
const int y = -(simple_force->y * 0xff / 0x);
> +
> + /*
> +* 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);
> break;
> + }
> + case MLNX_STOP_COMBINED:
> + report->field[0]->value[1] = 0;
> + report->field[0]->value[31] = 0;
> + break;
> + default:
> + return -EINVAL;
> }
> +
> + hid_hw_request(hid, report, HID_REQ_SET_REPORT);
> +
> return 0;
> }
> +
> static void hid_lg3ff_set_autocenter(struct input_dev *dev, u16 magnitude)
> {
> struct hid_device *hid = input_get_drvdata(dev);
> @@ -123,6 +132,13 @@ static void hid_lg3ff_set_autocenter(struct input_dev
> *dev, u16 magnitude)
>
> static const signed short ff3_joystick_ac[] = {
> FF_CONSTANT,
> + FF_RAMP,
> + FF_PERIODIC,
> + FF_SQUARE,
> + FF_TRIANGLE,
> + FF_SINE,
> + FF_SAW_UP,
> + FF_SAW_DOWN,
> FF_AUTOCENTER,
> -1
> };
> @@ -143,7 +159,7 @@ int lg3ff_init(struct hid_device *hid)
> for (i = 0; ff_bits[i] >= 0; i++)
> set_bit(ff_bits[i], dev->ffbit);
>
> - error = input_ff_create_memless(dev, NULL, hid_lg3ff_play);
> + error = input_ff_create_mlnx(dev, NULL, hid_lg3ff_play,
> FF_UPDATE_RATE);
> if (error)
> return error;
>
> --
> 1.9.1
>
>
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/24] Introduce ff-memless-next as an improved replacement for ff-memless
On Sun, Apr 20, 2014 at 7:27 PM, wrote: > Hi all, > I got a chance to build this series of patches and test with the > controllers I have (*). Without specific instructions I wasn't sure > exactly what to test, but it seems to be OK and the devices > rumbled/wobbled appropriately, > Simon > > tested-by: Simon Wood > > * controllers: > hid-sony: DS4, DS3SA, Intec > hid-lg: WiiWheel, MomoRed, MomoBlack, DFP, WingMan Force Did the WiiWheel have working FF_CONSTANT before this patchset? It would be weird if yes, because lg4ff is also used for MOMO-Black which works fine. Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/24] Port hid-lgff to ff-memless-next
On Wed, Apr 9, 2014 at 1:24 PM, Michal Malý
wrote:
> Port hid-lgff to ff-memless-next
>
> Signed-off-by: Michal Malý
> ---
> drivers/hid/Kconfig| 2 +-
> drivers/hid/hid-lgff.c | 63
> ++
> 2 files changed, 44 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index d1d4e77..c7794ae 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -356,7 +356,7 @@ config HID_LOGITECH_DJ
> config LOGITECH_FF
> bool "Logitech force feedback support"
> depends on HID_LOGITECH
> - select INPUT_FF_MEMLESS
> + select INPUT_FF_MEMLESS_NEXT
> help
> Say Y here if you have one of these devices:
> - Logitech WingMan Cordless RumblePad
> diff --git a/drivers/hid/hid-lgff.c b/drivers/hid/hid-lgff.c
> index e1394af..b929a3a 100644
> --- a/drivers/hid/hid-lgff.c
> +++ b/drivers/hid/hid-lgff.c
> @@ -31,9 +31,12 @@
>
> #include
> #include
> +#include
>
> #include "hid-lg.h"
>
> +#define FF_UPDATE_RATE 50
> +
> struct dev_type {
> u16 idVendor;
> u16 idProduct;
> @@ -47,6 +50,13 @@ static const signed short ff_rumble[] = {
>
> static const signed short ff_joystick[] = {
> FF_CONSTANT,
> + FF_RAMP,
> + FF_PERIODIC,
> + FF_SQUARE,
> + FF_TRIANGLE,
> + FF_SINE,
> + FF_SAW_UP,
> + FF_SAW_DOWN,
> -1
> };
Also do the same for the ff_joystick_ac[] array, as it also contains
the FF_CONSTANT capability.
>
> @@ -66,45 +76,58 @@ static const struct dev_type devices[] = {
> { 0x046d, 0xc295, ff_joystick },
> };
>
> -static int hid_lgff_play(struct input_dev *dev, void *data, struct ff_effect
> *effect)
> +static int hid_lgff_play(struct input_dev *dev, void *data,
> +const struct mlnx_effect_command *command)
> {
> 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;
> - unsigned int left, right;
>
> -#define CLAMP(x) if (x < 0) x = 0; if (x > 0xff) x = 0xff
> + switch (command->cmd) {
> + case MLNX_START_COMBINED: {
> + const struct mlnx_simple_force *simple_force =
> &command->u.simple_force;
> + /* Scale down from MLNX range */
> + const int x = 0x80 - (simple_force->x * 0xff / 0x);
> + const int y = simple_force->y * 0xff / 0x;
This should be concentrated about the center, which is 0x7f (see * in
next comment.)
Something like this:
const int x = 0x7f - (simple_force->x * 0xff / 0x);
const int y = 0x7f - (simple_force->y * 0xff / 0x);
>
> - switch (effect->type) {
> - case FF_CONSTANT:
> - x = effect->u.ramp.start_level + 0x7f; /* 0x7f is center */
* Here.
> - y = effect->u.ramp.end_level + 0x7f;
> - CLAMP(x);
> - CLAMP(y);
> report->field[0]->value[0] = 0x51;
> report->field[0]->value[1] = 0x08;
> report->field[0]->value[2] = x;
> report->field[0]->value[3] = y;
> dbg_hid("(x, y)=(%04x, %04x)\n", x, y);
> - hid_hw_request(hid, report, HID_REQ_SET_REPORT);
> break;
> + }
> + case MLNX_STOP_COMBINED:
> + report->field[0]->value[0] = 0x51;
> + report->field[0]->value[1] = 0x08;
> + report->field[0]->value[2] = 0;
> + report->field[0]->value[3] = 0;
Then this should become the center-position:
report->field[0]->value[2] = 0x7f;
report->field[0]->value[3] = 0x7f;
> + break;
> + case MLNX_START_RUMBLE: {
> + const struct mlnx_rumble_force *rumble_force =
> &command->u.rumble_force;
> + /* Scale down from MLNX range */
> + const unsigned int right = rumble_force->weak * 0xff / 0x;
> + const unsigned int left = rumble_force->strong * 0xff /
> 0x;
>
> - case FF_RUMBLE:
> - right = effect->u.rumble.strong_magnitude;
> - left = effect->u.rumble.weak_magnitude;
> - right = right * 0xff / 0x;
> - left = left * 0xff / 0x;
> - CLAMP(left);
> - CLAMP(right);
> report->field[0]->value[0] = 0x42;
> report->field[0]->value[1] = 0x00;
> report->field[0]->value[2] = left;
> report->field[0]->value[3] = right;
> dbg_hid("(left, right)=(%04x, %04x)\n", left, right);
> - hid_hw_request(hid, report, HID_REQ_SET_REPORT);
> break;
> }
> + case MLNX_STOP_RUMBLE:
> + report->field[0]->value[
Re: [PATCH] HID: lg2ff: add rumble magnitude clamping quirk
Since this patch haven't been applied yet, I withdraw this patch for the following reason: It will get in as a part of the move to ff-memless-next. Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] HID: lg2ff: add rumble magnitude clamping quirk
If a magnitude in the output report is lower than 2, i.e. 1 or 0,
the corresponding rumble motor shakes irregularly,
instead of being turned (almost) off like when magnitude 2 is used.
On the other hand, if a magnitude is higher than 0xfd, i.e 0xfe or 0xff,
the corresponding rumble motor shakes irregularly with a rotation speed
lower than when magnitude 0xfd is used.
>From 0x02 to 0xfd, the device behaves well:
a monotonic increase of rotation speed.
This applies to both weak and strong rumble motor types.
This patch fixes this issue by clamping magnitudes from 0x02 to 0xfd.
This behaviour is observed on the Formula Vibration wheel (ca04).
However it's not present on the Wingman Rumblepad (c20a) device,
yet the clamping has no effect on the haptic side,
so that special case handling is not needed.
Discussion on http://www.spinics.net/lists/linux-input/msg30711.html
Note: The same thing appears to happen in the Windows Logitech driver,
except the max clamping bound is not 0xfd, but 0xfe.
Experimentally, I proved this to be wrong.
Tested-by: Hendrik Iben
Signed-off-by: Elias Vanderstuyft
Cc: Edgar Simo-Serra
Cc: Michal Malý
Cc: linux-input@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
---
drivers/hid/hid-lg2ff.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/hid/hid-lg2ff.c b/drivers/hid/hid-lg2ff.c
index 0e3fb1a..e180e1e 100644
--- a/drivers/hid/hid-lg2ff.c
+++ b/drivers/hid/hid-lg2ff.c
@@ -38,12 +38,17 @@ static int play_effect(struct input_dev *dev, void *data,
struct lg2ff_device *lg2ff = data;
int weak, strong;
+#define CLAMP_QUIRK(x) do { if (x < 2) x = 2; else if (x > 0xfd) x = 0xfd; } \
+ while (0)
+
strong = effect->u.rumble.strong_magnitude;
weak = effect->u.rumble.weak_magnitude;
if (weak || strong) {
weak = weak * 0xff / 0x;
strong = strong * 0xff / 0x;
+ CLAMP_QUIRK(weak);
+ CLAMP_QUIRK(strong);
lg2ff->report->field[0]->value[0] = 0x51;
lg2ff->report->field[0]->value[2] = weak;
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: hid-lg2ff handling of zero/low magnitude rumble effects
On Sat, Apr 5, 2014 at 1:02 AM, Hendrik Iben wrote: > Hi Elias, > > First of all - thanks for caring about compatibility with old devices. > It would be really annoying if something that worked once stopped > working on an update. > > I just dug out my old Wingman Rumblepad (VID 046d PID c20a) and did the > test cases (thanks for the tool by the way - might come in handy for > other things). The first motor for this device controls a weak/smooth > rumble effect and the second one a strong/coarse one. For both motors, > values from 00 to 03 have no effect on the haptic side. You can hear the > motors spinning a bit higher if you hold the pad to your ear... :-) - > but it sounds like a linear increase. It similar for the high values. > FC-FF produce the same high amount of rumble. There might be a small > increase but not really any difference. > From these observations I would say that clamping is fine for the > Rumblepad although not needed. But a special case handling would have no > positive effect. OK, thanks for testing! I agree that a special case handling would have no positive effect. I tried to contact Edgar earlier past month, but until now I can't seem to get in touch with him. If there is no reply in a reasonable amount of time, I'll release the patch, it won't hurt anyone ;) Again thanks, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/1] Input: don't modify the id of ioctl-provided ff effect on upload failure
If a new (id == -1) ff effect was uploaded from userspace,
ff-core.c::input_ff_upload() will have assigned
a positive number to the new effect id.
Currently, evdev.c::evdev_do_ioctl() will save this new id to userspace,
regardless of whether the upload succeeded or not.
On upload failure, this can be confusing because the dev->ff->effects[] array
will not contain an element at the index of that new effect id.
This patch fixes this by leaving the id unchanged after upload fails.
Note: Unfortunately applications should still expect changed effect id for
quite some time.
This has been discussed on:
http://www.mail-archive.com/linux-input@vger.kernel.org/msg08513.html
("ff-core effect id handling in case of a failed effect upload")
Suggested-by: Dmitry Torokhov
Signed-off-by: Elias Vanderstuyft
Cc: Anssi Hannula
Cc: Michal Malý
Cc: linux-input@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
---
v2:
Only added one line to the commit message to say
what this patch actually does,
instead of only stating the reason why it's submitted.
drivers/input/evdev.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index a06e125..ce953d8 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -954,11 +954,13 @@ static long evdev_do_ioctl(struct file *file, unsigned
int cmd,
return -EFAULT;
error = input_ff_upload(dev, &effect, file);
+ if (error)
+ return error;
if (put_user(effect.id, &(((struct ff_effect __user *)p)->id)))
return -EFAULT;
- return error;
+ return 0;
}
/* Multi-number variable-length handlers */
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
hid-lg2ff handling of zero/low magnitude rumble effects
Hi, I noticed that my rumble wheel (Logitech Formula Vibration Feedback) reacts in a strange way when sending low rumble magnitudes to the device: Assume the following USB command to be send to emit rumble: report->field[0]->value[0] = 0x51; report->field[0]->value[2] = weak; report->field[0]->value[4] = strong; When 'weak' or 'strong' is lower than 0x02 (i.e. 0x01 or 0x00), then the corresponding rumble motor begins to rumble intermittently, this resembles a bit to forcing a 2-state light-switch to be in the middle position. Now my question is whether all other devices (e.g. "Logitech RumblePad", "Rumblepad 2") experience this behaviour? (If you own such a device, please verify this.) Best regards, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] Input: don't modify the id of ioctl-provided ff effect on upload failure
On Sun, Feb 23, 2014 at 3:18 PM, Elias Vanderstuyft wrote:
> From: Elias Vanderstuyft
>
> If a new (id == -1) ff effect was uploaded from userspace,
> ff-core.c::input_ff_upload() will have assigned
> a positive number to the new effect id.
> Currently, evdev.c::evdev_do_ioctl() will save this new id to userspace,
> regardless of whether the upload succeeded or not.
>
> On upload failure, this can be confusing because the dev->ff->effects[] array
> will not contain an element at the index of that new effect id.
>
> Note: Unfortunately applications should still expect changed effect id for
> quite some time.
>
> This has been discussed on:
> http://www.mail-archive.com/linux-input@vger.kernel.org/msg08513.html
> ("ff-core effect id handling in case of a failed effect upload")
>
> Suggested-by: Dmitry Torokhov
> Signed-off-by: Elias Vanderstuyft
> ---
> drivers/input/evdev.c |4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> --- a/drivers/input/evdev.c 2014-02-23 14:21:19.980606615 +0100
> +++ b/drivers/input/evdev.c 2014-02-23 14:25:04.417118859 +0100
> @@ -954,11 +954,13 @@ static long evdev_do_ioctl(struct file *
> return -EFAULT;
>
> error = input_ff_upload(dev, &effect, file);
> + if (error)
> + return error;
>
> if (put_user(effect.id, &(((struct ff_effect __user
> *)p)->id)))
> return -EFAULT;
>
> - return error;
> + return 0;
> }
>
> /* Multi-number variable-length handlers */
This is a reminder that there have been no replies on this subject for
the last 3 weeks.
If there is something wrong with the format of this patch, please tell
me, I tried to follow the instructions on
http://www.kernel.org/doc/Documentation/SubmittingPatches , but it's
the first time I send a patch by myself, so I might have forgotten
something.
Did I forget to CC some people?
The reason I'm insisting on this, is because this change will be
perceivable to userspace developers, and the sooner this gets applied,
the less userland applications will have to circumvent this bug.
Best regards,
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Fwd: [PATCH] input: ff-memless: don't schedule already playing effect to play again
-- Forwarded message --
From: Michal Malý
Date: Sun, Mar 2, 2014 at 2:29 PM
Subject: Re: [PATCH] input: ff-memless: don't schedule already playing
effect to play again
To: Elias Vanderstuyft
On Sunday 02 of March 2014 14:17:58 you wrote:
> On Sun, Mar 2, 2014 at 12:35 PM, Felix Rueegg
wrote:
> > When an effect with zero replay length, zero replay delay
> > and zero envelope attack length is uploaded, it is played and then
> > scheduled to play again one timer tick later. This triggers a
warning
> > (URB submitted while active) in combination with the xpad driver.
> >
> > Skipping the rescheduling of this effect fixes the issue.
> >
> > Signed-off-by: Felix Rueegg
> > ---
> >
> > drivers/input/ff-memless.c | 7 +--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
> > index 74c0d8c..2e06948 100644
> > --- a/drivers/input/ff-memless.c
> > +++ b/drivers/input/ff-memless.c
> > @@ -139,10 +139,13 @@ static void ml_schedule_timer(struct
ml_device *ml)
> >
> > if (!test_bit(FF_EFFECT_STARTED, &state->flags))
> >
> > continue;
> >
> > - if (test_bit(FF_EFFECT_PLAYING, &state->flags))
> > + if (test_bit(FF_EFFECT_PLAYING, &state->flags)) {
> >
> > next_at = calculate_next_time(state);
> >
> > - else
> > + if (next_at == now)
> > + continue;
> > + } else {
> >
> > next_at = state->play_at;
> >
> > + }
> >
> > if (time_before_eq(now, next_at) &&
> >
> > (++events == 1 || time_before(next_at, earliest)))
> >
> > --
> > 1.9.0
> >
> > --
>
> @Michal: Is ff-memless-next also affected by this problem?
>
> Elias
I hope it's not, see mlnx_get_envelope_update_time(), this part in
particular:
/* Prevent the effect from being started twice */
if (mlnxeff->begin_at == now && mlnx_is_playing(mlnxeff))
return now - 1;
return mlnxeff->begin_at;
Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] input: ff-memless: don't schedule already playing effect to play again
On Sun, Mar 2, 2014 at 12:35 PM, Felix Rueegg wrote:
> When an effect with zero replay length, zero replay delay
> and zero envelope attack length is uploaded, it is played and then scheduled
> to play
> again one timer tick later. This triggers a warning (URB submitted while
> active) in combination with the xpad driver.
>
> Skipping the rescheduling of this effect fixes the issue.
>
> Signed-off-by: Felix Rueegg
> ---
> drivers/input/ff-memless.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
> index 74c0d8c..2e06948 100644
> --- a/drivers/input/ff-memless.c
> +++ b/drivers/input/ff-memless.c
> @@ -139,10 +139,13 @@ static void ml_schedule_timer(struct ml_device *ml)
> if (!test_bit(FF_EFFECT_STARTED, &state->flags))
> continue;
>
> - if (test_bit(FF_EFFECT_PLAYING, &state->flags))
> + if (test_bit(FF_EFFECT_PLAYING, &state->flags)) {
> next_at = calculate_next_time(state);
> - else
> + if (next_at == now)
> + continue;
> + } else {
> next_at = state->play_at;
> + }
>
> if (time_before_eq(now, next_at) &&
> (++events == 1 || time_before(next_at, earliest)))
> --
> 1.9.0
>
> --
@Michal: Is ff-memless-next also affected by this problem?
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Questions about the documentation/specification of Linux ForceFeedback input.h
On Wed, Feb 19, 2014 at 7:54 PM, Elias Vanderstuyft wrote: > Today, I noticed something strange: an explicit saturation of zero in > DInput (or at least using the most recent driver for my Logitech MOMO > wheel), appears to be transformed to max saturation! (This is why I > chose to quote your "sat_right = 0x" part, however that's not the > point I want to make in this mail) > > E.g: > pos_sat = 1->Max saturation send to device > pos_sat = 5000->Half saturation send to device > ... > pos_sat = 1->Zero saturation send to device (because > it appears to be rounded to zero) > pos_sat = 0->Max saturation send to device (?!) > > Also interesting is that the FEdit tool (from MS DirectX SDK) sets all > 'saturation' values to zero by default. This is not the case for the > 'coefficient' values: they are set to maximum. The resulting default > conditional force generates a maxed out conditional effect on my MOMO > wheel. > The weird thing is, that MSDN does not explicitly say anything about > this: > http://msdn.microsoft.com/en-us/library/windows/desktop/microsoft.directx_sdk.reference.dicondition%28v=vs.85%29.aspx > "dwPositiveSaturation: > Maximum force output on the positive side of the offset, in the > range from 0 through 10,000. > If the device does not support force saturation, the value of this > member is ignored. > " > > So, my question is: > - Should Wine's translation layer convert a 0 dinput saturation to > max linux saturation? > - or Should the Linux FF drivers send a max saturation command > when the linux saturation equals zero? > - or Should we forget about this? (but that will result Wine's > behaviour to be different from Windows', at least for my MOMO wheel (I > don't have another capable (non-Logitech?) wheel to test it with)) > On Thu, Feb 20, 2014 at 9:52 AM, Elias Vanderstuyft wrote: > Another question: > There is written on /include/uapi/linux/input.h:1058 : > "@phase: 'horizontal' shift" > where 'horizontal' most likely means time dimension, not phase of the > waveform, right? > Hi Anssi, Any thoughts about my previous 2 mails' questions? (I just need be sure of these things to finish my Wine FF patchset, however, if you don't have time, no problem, it can wait for some time) Best regards, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/4] Add ff-memless-next and make hid-lg4ff use it
On Mon, Feb 24, 2014 at 1:58 AM, Michal Malý wrote: > On Monday 24 of February 2014 02:32:27 Anssi Hannula wrote: >> >> I think we should extend the current ff-memless instead of duplicating >> its functionality (even on a "for now" basis). >> >> Having looked at ff-memless-next briefly, it seems very similar to >> ff-memless on its basic working principle, and therefore I don't really >> see why extending ff-memless would be too cumbersome. Unless I'm missing >> something - in that case, feel free to point it out to me :) > > Deciding whether to patch ff-memless or write a new driver from scratch was a > perfect example of being caught between the rock and a hard place. I am not > particularly fond of the fact that we would have two modules doing pretty much > the same thing. My reasons for writing a separate module were: > - Periodic effects. ff-memless doesn't do "real" periodic effects, it simply > emulates them through rumble effect. Devices without rumble effect support > require emulation through constant force effect. Just this was not something > one could write in one afternoon:) > - Conditional effects. These effects cannot be by nature combined into one > overall force (at least not easily) so they have to be handled one by one - > this is a concept ff-memless does not seem to consider. FFB devices have > limits as to how many conditional (referred to as "uncombinable" in MLNX) > effects can be active simultaneously, etc. > All in all it seemed less error prone to write a new driver based on the ff- > memless logic, test and deploy it on devices I have access to and once we are > sure there are no nasty regressions port the rest of the drivers to the new > API. Given the scope of the changes I am afraid that a "patch" to ff-memless > would be pretty close to a rewrite anyway. And add the fact that we already heavily tested the ff-memless-next driver. Unless you do a diff between the original ff-memless.c and the current ff-memless-next.c (which will result in a rather unintuitive patch), it would be a huge waste of time to retest the modified (when doing efforts to create an intuitive patch) ff-memless-next.c, considered my total time spend on testing (and not to speak of the time that Michal spent to fix the corresponding bugs.) I know that might not be much of an argument, but on the other side, my motivation to test again from scratch will be much lower (I can't change much on that, I'm afraid), which would eventually lead to lower reliability of the final product. Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ff-core effect id handling in case of a failed effect upload
I sent a patch, see:
http://www.mail-archive.com/linux-input@vger.kernel.org/msg08572.html
("[PATCH 1/1] Input: don't modify the id of ioctl-provided ff effect
on upload failure")
Elias
On Sun, Feb 23, 2014 at 7:30 AM, Dmitry Torokhov
wrote:
> On Wed, Feb 19, 2014 at 11:14:18PM +0100, Elias Vanderstuyft wrote:
>> On Wed, Feb 19, 2014 at 10:05 PM, Dmitry Torokhov
>> wrote:
>> > On Wed, Feb 19, 2014 at 06:49:36PM +0200, Anssi Hannula wrote:
>> >> (added Dmitry to CC)
>> >>
>> >> 19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
>> >> > Hi,
>> >> >
>> >>
>> >> Hi,
>> >>
>> >> > In the process of reviewing the Wine DInput translation layer, I
>> >> > noticed an inconvenience (in the ff-core implementation?) that can
>> >> > possibly lead to confusing problems to application developers (not
>> >> > only for Wine), in short:
>> >> > If a new (id==-1) effect was uploaded (look at
>> >> > ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
>> >> > ff-core will have assigned a positive number to the effect id. This
>> >> > can be confusing because the dev->ff->effects[] array will not contain
>> >> > an element at the index of that new effect id.
>> >>
>> >> I agree that this is a bit confusing, and the kernel code should
>> >> probably be modified to not clobber the ioctl-provided effect on failure
>> >> (effect->id is set to an "undefined" value, i.e. next free effect slot).
>> >>
>> >> Dmitry, WDYT?
>> >
>> > Yeah, it looks like we need to change evdev.c to read:
>> >
>> > error = input_ff_upload(dev, &effect, file);
>> > if (error)
>> > return error;
>> >
>> > if (put_user(effect.id, &(((struct ff_effect __user
>> > *)p)->id)))
>> > return -EFAULT;
>> >
>> > return 0;
>>
>> Alright, who will create the patch?
>> Do I may / have to do it?
>
> If you could create the patch that would be appreciated.
>
> Thanks.
>
> --
> Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] Input: don't modify the id of ioctl-provided ff effect on upload failure
From: Elias Vanderstuyft
If a new (id == -1) ff effect was uploaded from userspace,
ff-core.c::input_ff_upload() will have assigned
a positive number to the new effect id.
Currently, evdev.c::evdev_do_ioctl() will save this new id to userspace,
regardless of whether the upload succeeded or not.
On upload failure, this can be confusing because the dev->ff->effects[] array
will not contain an element at the index of that new effect id.
Note: Unfortunately applications should still expect changed effect id for
quite some time.
This has been discussed on:
http://www.mail-archive.com/linux-input@vger.kernel.org/msg08513.html
("ff-core effect id handling in case of a failed effect upload")
Suggested-by: Dmitry Torokhov
Signed-off-by: Elias Vanderstuyft
---
drivers/input/evdev.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
--- a/drivers/input/evdev.c 2014-02-23 14:21:19.980606615 +0100
+++ b/drivers/input/evdev.c 2014-02-23 14:25:04.417118859 +0100
@@ -954,11 +954,13 @@ static long evdev_do_ioctl(struct file *
return -EFAULT;
error = input_ff_upload(dev, &effect, file);
+ if (error)
+ return error;
if (put_user(effect.id, &(((struct ff_effect __user *)p)->id)))
return -EFAULT;
- return error;
+ return 0;
}
/* Multi-number variable-length handlers */
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Questions about the documentation/specification of Linux ForceFeedback input.h
Another question: There is written on /include/uapi/linux/input.h:1058 : "@phase: 'horizontal' shift" where 'horizontal' most likely means time dimension, not phase of the waveform, right? Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ff-core effect id handling in case of a failed effect upload
On Wed, Feb 19, 2014 at 10:05 PM, Dmitry Torokhov wrote: > On Wed, Feb 19, 2014 at 06:49:36PM +0200, Anssi Hannula wrote: >> (added Dmitry to CC) >> >> 19.02.2014 13:42, Elias Vanderstuyft kirjoitti: >> > Hi, >> > >> >> Hi, >> >> > In the process of reviewing the Wine DInput translation layer, I >> > noticed an inconvenience (in the ff-core implementation?) that can >> > possibly lead to confusing problems to application developers (not >> > only for Wine), in short: >> > If a new (id==-1) effect was uploaded (look at >> > ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL), >> > ff-core will have assigned a positive number to the effect id. This >> > can be confusing because the dev->ff->effects[] array will not contain >> > an element at the index of that new effect id. >> >> I agree that this is a bit confusing, and the kernel code should >> probably be modified to not clobber the ioctl-provided effect on failure >> (effect->id is set to an "undefined" value, i.e. next free effect slot). >> >> Dmitry, WDYT? > > Yeah, it looks like we need to change evdev.c to read: > > error = input_ff_upload(dev, &effect, file); > if (error) > return error; > > if (put_user(effect.id, &(((struct ff_effect __user > *)p)->id))) > return -EFAULT; > > return 0; Alright, who will create the patch? Do I may / have to do it? > > Unfortunately applications should still expect changed effect ID for > quite some time. > > Thanks. > > -- > Dmitry Best regards, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ff-core effect id handling in case of a failed effect upload
On Wed, Feb 19, 2014 at 8:32 PM, Andrew Eikum wrote: > On Wed, Feb 19, 2014 at 06:32:54PM +0100, Elias Vanderstuyft wrote: >> I'll include the code (with effectId_old variable) and this discussion >> when I submit my whole patchset for Wine's DInput libs to Wine-devel >> mailing list. Right now, I don't know who I should personally mail to. >> > > Patches ready to be included in Wine should go to > wine-patc...@winehq.org. I've done a bit of work in dinput myself, so > you can feel free to send them to me and/or wine-devel first if you'd > like a review. Alright, thanks Andrew! I've also done some work on reverse engineering the FFE file format, which is needed to complete some missing functionality (http://wiki.winehq.org/ForceFeedbackSummerOfCode2005Summary) of the following functions: IDirectInputDevice8::WriteEffectToFile and IDirectInputDevice8::EnumEffectsInFile. Can I send it to you (in a new mail subject, of course) and cc wine-devel? > > Andrew Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Questions about the documentation/specification of Linux ForceFeedback input.h
On Sun, Feb 16, 2014 at 12:04 AM, Anssi Hannula wrote: > 15.02.2014 22:32, Elias Vanderstuyft kirjoitti: >> On Sat, Feb 15, 2014 at 3:04 PM, Anssi Hannula wrote: >>> >>> However, from what I can see, you can achieve the exact same effect with >>> these parameters (DInput deadband definition): >>> sat_left = 0x >>> sat_right = 0x >>> coeff_left = 0x4000 >>> coeff_right = 0x >>> center = 0x (50% point) >>> deadband (DInput/ours) = 0x8000 (50% total area, or 50% of >>> center-to-end, so it reaches 25%..75%) >>> >>> On the left side, the effect is exactly the same as before with same >>> parameters (sat 0x, coeff 0x4000, starts at 25% point). >>> >>> On the right side the parameters differ, but the end-result is the same, >>> there is no effect at all: >>> - In the first example, the right side is fully in deadband area, >>> causing the effect to have zero effect. >>> - In my variant with our definition, the right side has zero saturation, >>> causing the effect to have zero effect. >>> >>> >>> So the present definitions (and DInput definitions) can achieve the same >>> effects as your proposed definitions, unless I'm missing something, >>> making the change unneeded. Today, I noticed something strange: an explicit saturation of zero in DInput (or at least using the most recent driver for my Logitech MOMO wheel), appears to be transformed to max saturation! (This is why I chose to quote your "sat_right = 0x" part, however that's not the point I want to make in this mail) E.g: pos_sat = 1->Max saturation send to device pos_sat = 5000->Half saturation send to device ... pos_sat = 1->Zero saturation send to device (because it appears to be rounded to zero) pos_sat = 0->Max saturation send to device (?!) Also interesting is that the FEdit tool (from MS DirectX SDK) sets all 'saturation' values to zero by default. This is not the case for the 'coefficient' values: they are set to maximum. The resulting default conditional force generates a maxed out conditional effect on my MOMO wheel. The weird thing is, that MSDN does not explicitly say anything about this: http://msdn.microsoft.com/en-us/library/windows/desktop/microsoft.directx_sdk.reference.dicondition%28v=vs.85%29.aspx "dwPositiveSaturation: Maximum force output on the positive side of the offset, in the range from 0 through 10,000. If the device does not support force saturation, the value of this member is ignored. " So, my question is: - Should Wine's translation layer convert a 0 dinput saturation to max linux saturation? - or Should the Linux FF drivers send a max saturation command when the linux saturation equals zero? - or Should we forget about this? (but that will result Wine's behaviour to be different from Windows', at least for my MOMO wheel (I don't have another capable (non-Logitech?) wheel to test it with)) Best regards, Elias -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ff-core effect id handling in case of a failed effect upload
On Wed, Feb 19, 2014 at 5:49 PM, Anssi Hannula wrote:
> (added Dmitry to CC)
>
> 19.02.2014 13:42, Elias Vanderstuyft kirjoitti:
>> Hi,
>>
>
> Hi,
>
>> In the process of reviewing the Wine DInput translation layer, I
>> noticed an inconvenience (in the ff-core implementation?) that can
>> possibly lead to confusing problems to application developers (not
>> only for Wine), in short:
>> If a new (id==-1) effect was uploaded (look at
>> ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
>> ff-core will have assigned a positive number to the effect id. This
>> can be confusing because the dev->ff->effects[] array will not contain
>> an element at the index of that new effect id.
>
> I agree that this is a bit confusing, and the kernel code should
> probably be modified to not clobber the ioctl-provided effect on failure
> (effect->id is set to an "undefined" value, i.e. next free effect slot).
>
> Dmitry, WDYT?
>
> The downside is that if changed, any new userspace code may unknowingly
> depend on the fixed non-clobbering behavior (though apparently they
> already do, as evidenced by Wine DInput, so might not be much of an
> argument...).
I don't think that's a problem.
Something that can be more problematic, is that a newer application
assumes the fixed non-clobbering behaviour, but runs on a kernel with
the old clobbering behaviour, could experience problems.
>
>> Here is a more elaborated description/discussion:
>> - This is a bug that is either the responsibility of the Linux kernel,
>> or of Wine (and possibly other applications that do the same thing as
>> described below):
>> It is caused by the following situation:
>> When uploading an effect, the specific kernel device driver
>> may return an error,
>> e.g. EINVAL for example when a periodic's effect period is set to
>> zero.
>> This error will then be returned by "ioctl(*(This->fd),
>> EVIOCSFF, &This->effect)".
>> With or without error, one can find out that
>> /drivers/input/ff-core.c:input_ff_upload(...) is called,
>> which will set effect->id >= 0, if it was set to -1 (=> new
>> effect created) by the user.
>>
>> But in case of an error:
>> - Assume effect->id was set to -1 by the user:
>> The error is reported by ff->upload(...) at
>> /drivers/input/ff-core.c:167,
>> the effect->id will also be set >= 0 (*).
>> The offending effect will not be saved in the
>> ff->effects[] array (***).
>> - Assume effect->id was set >= 0 by the user (and
>> ff->effects[effect->id] is a valid existing effect):
>> The error is reported by ff->upload(...) at
>> /drivers/input/ff-core.c:167,
>> the effect->id will remain unchanged (**).
>> The offending effect will not overwrite the
>> ff->effects[effect->id] element ().
>>
>> Is this (see *, **, *** and ) desired behaviour?
>> - If yes:
>> Change the following in Wine's dinput/effect_linuxinput.c:90 :
>> if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
>> to :
>> int effectId_old = This->effect.id;
>> if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
>> This->effect.id = effectId_old;
>> - If no for *:
>> Kernel code /drivers/input/ff-core.c:input_ff_upload(...)
>> has to be patched to revert "effect->id" back to its original value
>> set by the user,
>> which is only needed when the initial (by user) value of
>> "effect->id" was equal to -1.
>> - If no for (or maybe also ***):
>> ff->effects[effect->id] could be replaced by an 'empty'
>> effect (however this can get complex because the effect's type has to
>> remain unchanged)
>> This would be a change in the kernel code
>> /drivers/input/ff-core.c:input_ff_upload(...).
>> - If no for **:
>> I don't really know. Discussion is needed.
>>
>> - In my opinion, **, *** and are desired behaviour, while
>> * should leave the effect->id at -1.
>
> Right.
>
>> In that case, Wine's dinput implementation does not have
>> to be patched, and the kernel code only should apply a minor patch.
>
> Well, maybe better to change dinput anyway so that it can work properly
> with current kernel versions (assuming this gets changed in the future)?
> Not my call, though.
Yes, agreed.
I'll include the code (with effectId_old variable) and this discussion
when I submit my whole patchset for Wine's DInput libs to Wine-devel
mailing list. Right now, I don't know who I should personally mail to.
>
> --
> Anssi Hannula
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
ff-core effect id handling in case of a failed effect upload
Hi,
In the process of reviewing the Wine DInput translation layer, I
noticed an inconvenience (in the ff-core implementation?) that can
possibly lead to confusing problems to application developers (not
only for Wine), in short:
If a new (id==-1) effect was uploaded (look at
ff-core.c::input_ff_upload(...)) that failed (e.g. returning EINVAL),
ff-core will have assigned a positive number to the effect id. This
can be confusing because the dev->ff->effects[] array will not contain
an element at the index of that new effect id.
Here is a more elaborated description/discussion:
- This is a bug that is either the responsibility of the Linux kernel,
or of Wine (and possibly other applications that do the same thing as
described below):
It is caused by the following situation:
When uploading an effect, the specific kernel device driver
may return an error,
e.g. EINVAL for example when a periodic's effect period is set to zero.
This error will then be returned by "ioctl(*(This->fd),
EVIOCSFF, &This->effect)".
With or without error, one can find out that
/drivers/input/ff-core.c:input_ff_upload(...) is called,
which will set effect->id >= 0, if it was set to -1 (=> new
effect created) by the user.
But in case of an error:
- Assume effect->id was set to -1 by the user:
The error is reported by ff->upload(...) at
/drivers/input/ff-core.c:167,
the effect->id will also be set >= 0 (*).
The offending effect will not be saved in the
ff->effects[] array (***).
- Assume effect->id was set >= 0 by the user (and
ff->effects[effect->id] is a valid existing effect):
The error is reported by ff->upload(...) at
/drivers/input/ff-core.c:167,
the effect->id will remain unchanged (**).
The offending effect will not overwrite the
ff->effects[effect->id] element ().
Is this (see *, **, *** and ) desired behaviour?
- If yes:
Change the following in Wine's dinput/effect_linuxinput.c:90 :
if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
to :
int effectId_old = This->effect.id;
if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) {
This->effect.id = effectId_old;
- If no for *:
Kernel code /drivers/input/ff-core.c:input_ff_upload(...)
has to be patched to revert "effect->id" back to its original value
set by the user,
which is only needed when the initial (by user) value of
"effect->id" was equal to -1.
- If no for (or maybe also ***):
ff->effects[effect->id] could be replaced by an 'empty'
effect (however this can get complex because the effect's type has to
remain unchanged)
This would be a change in the kernel code
/drivers/input/ff-core.c:input_ff_upload(...).
- If no for **:
I don't really know. Discussion is needed.
- In my opinion, **, *** and are desired behaviour, while
* should leave the effect->id at -1.
In that case, Wine's dinput implementation does not have
to be patched, and the kernel code only should apply a minor patch.
Best regards,
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Questions about the documentation/specification of Linux ForceFeedback input.h
On Sun, Feb 16, 2014 at 12:04 AM, Anssi Hannula wrote: > 15.02.2014 22:32, Elias Vanderstuyft kirjoitti: >> On Sat, Feb 15, 2014 at 3:04 PM, Anssi Hannula wrote: >>> 15.02.2014 14:14, Elias Vanderstuyft kirjoitti: >>>> On Sat, Feb 15, 2014 at 3:05 AM, Anssi Hannula >>>> wrote: >>>>> 15.02.2014 01:28, Elias Vanderstuyft kirjoitti: > [...] >>>>>> 1) >>>>>> The real meaning of 'directions', declared in >>>>>> http://lxr.free-electrons.com/source/include/uapi/linux/input.h#L1113 >>>>>> : >>>>>> "Direction of the effect" is encoded as follows: ... >>>>>> But it is not clear whether 'direction of the effect' means either: >>>>>> - positive direction of the force the user should apply to counteract >>>>>> the force that the joystick applies; or >>>>>> - positive direction of the force applied by joystick >>>>>> From my intuition, I think the latter is (silently?) meant by input.h >>>>>> If you're interested why this is so important, I attached a document >>>>>> "DInputVsLinuxDirections.txt" that tries to explain the dilemma if a >>>>>> translation layer between DInput and Linux input is to be written. >>>>> >>>>> From input.h: >>>>> * Direction of the effect is encoded as follows: >>>>> * 0 deg -> 0x (down) >>>>> * 90 deg -> 0x4000 (left) >>>>> * 180 deg -> 0x8000 (up) >>>>> * 270 deg -> 0xC000 (right) >>>>> >>>>> The directions in parantheses are the direction of applied force. >>>> >>>> Alright, thanks! That is invaluable information, maybe it should be >>>> added to input.h . >>>> It will avoid a lot of confusion for former DInput devs. >>>> >>>>> >>>>> However, there is actually a 1:1 mapping between DInput polar >>>>> coordinates and our direction parameter; DInput polar coordinates have 0 >>>>> deg = up, 90 deg = right, etc, so they are exactly flipped and therefore >>>>> match our values due to the reverse definition. >>>>> >>>>> Looking at your DInputVsLinuxDirections.txt, you seem to have mixed >>>>> different definitions of Carts: For DInput you use -1 = north, but for >>>>> Linux +1 = up, while you use -1 = west/left for both. >>>> >>>> So I assume you agree that I got the DInput part right? ((0, -1) = north) >>> >>> I guess so. >>> >>>> Then my mistake lies in the assumption that (0, +1) = up, so I should >>>> flip the y-axis to correct the Linux part. >>>> I'll explain how I originally derived the Linux part: >>>> Michal documented (in "ff-memless-next.txt") the Linux directions in >>>> the following way: >>>> " >>>> Direction of the effect's force translates to Cartesian coordinates system >>>> as follows: >>>> Direction = 0: Down >>>> Direction (0; 16384): 3rd quadrant >>>> Direction = 16384: Left >>>> Direction (16385; 32768): 2nd quadrant >>>> Direction = 32768: Up >>>> Direction (32769; 49152): 1st quadrant >>>> Direction = 49152: Right >>>> Direction (49153; 65535) :4th quadrant >>>> Direction = 65565: Down >>>> " >>> >>> The above is correct. >>> >>>> For a Cartesian coordinates system: >>>> - The (-1, 0)-axis (=-x) is the intersection of 3rd quadrant and 2nd >>>> quadrant => Left >>>> - The (0, +1)-axis (=+y) is the intersection of 2nd quadrant and 1st >>>> quadrant => Up >>>> - The (+1, 0)-axis (=+x) is the intersection of 1st quadrant and 4th >>>> quadrant => Right >>>> - The (0, -1)-axis (=-y) is the intersection of 4th quadrant and 3rd >>>> quadrant => Down >>> >>> Not sure why you've arbitrarily chosen reverse definition of Y axis >>> here, >> >> I really tried to derive the above in a non-arbitrary manner, here you >> can verify why I 'chose' the +y axis as the intersection of 2nd >> quadrant and 1st quadrant: >> http://en.wikipedia.org/wiki/File:Cartesian_coordinates_2D.svg >> And because you said that the part about directions in >> "f
Re: Questions about the documentation/specification of Linux ForceFeedback input.h
On Sat, Feb 15, 2014 at 3:04 PM, Anssi Hannula wrote: > 15.02.2014 14:14, Elias Vanderstuyft kirjoitti: >> On Sat, Feb 15, 2014 at 3:05 AM, Anssi Hannula wrote: >>> 15.02.2014 01:28, Elias Vanderstuyft kirjoitti: >>>> Hi everyone, >>> >>> Hi! >>> >>>> >>>> If you receive my mail, it is either because you: >>>> - are listed under the MAINTAINERS for "/include/uapi/linux/input.h": >>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS#n4410 >>>> - are listed as an author of "/Documentation/input/ff.txt" or of >>>> "/Documentation/input/interactive.fig" >>>> >>>> In the process of testing the new driver (ff-memless-next) Michal >>>> (CC-ed) is working on, this allowed me to gain more insight in the >>>> internal workings of Linux FF. That's why I decided to review the >>>> userspace code that currently uses most of Linux FF functionality: >>>> Wine's DInput translation layer (and eventually SDL2.) >>>> Trying to get everything correct, I noticed both >>>> "/include/uapi/linux/input.h" and "/Documentation/input/ff.txt" >>>> provide not enough information for the following things: >>>> >>>> 1) >>>> The real meaning of 'directions', declared in >>>> http://lxr.free-electrons.com/source/include/uapi/linux/input.h#L1113 >>>> : >>>> "Direction of the effect" is encoded as follows: ... >>>> But it is not clear whether 'direction of the effect' means either: >>>> - positive direction of the force the user should apply to counteract >>>> the force that the joystick applies; or >>>> - positive direction of the force applied by joystick >>>> From my intuition, I think the latter is (silently?) meant by input.h >>>> If you're interested why this is so important, I attached a document >>>> "DInputVsLinuxDirections.txt" that tries to explain the dilemma if a >>>> translation layer between DInput and Linux input is to be written. >>> >>> From input.h: >>> * Direction of the effect is encoded as follows: >>> * 0 deg -> 0x (down) >>> * 90 deg -> 0x4000 (left) >>> * 180 deg -> 0x8000 (up) >>> * 270 deg -> 0xC000 (right) >>> >>> The directions in parantheses are the direction of applied force. >> >> Alright, thanks! That is invaluable information, maybe it should be >> added to input.h . >> It will avoid a lot of confusion for former DInput devs. >> >>> >>> However, there is actually a 1:1 mapping between DInput polar >>> coordinates and our direction parameter; DInput polar coordinates have 0 >>> deg = up, 90 deg = right, etc, so they are exactly flipped and therefore >>> match our values due to the reverse definition. >>> >>> Looking at your DInputVsLinuxDirections.txt, you seem to have mixed >>> different definitions of Carts: For DInput you use -1 = north, but for >>> Linux +1 = up, while you use -1 = west/left for both. >> >> So I assume you agree that I got the DInput part right? ((0, -1) = north) > > I guess so. > >> Then my mistake lies in the assumption that (0, +1) = up, so I should >> flip the y-axis to correct the Linux part. >> I'll explain how I originally derived the Linux part: >> Michal documented (in "ff-memless-next.txt") the Linux directions in >> the following way: >> " >> Direction of the effect's force translates to Cartesian coordinates system >> as follows: >> Direction = 0: Down >> Direction (0; 16384): 3rd quadrant >> Direction = 16384: Left >> Direction (16385; 32768): 2nd quadrant >> Direction = 32768: Up >> Direction (32769; 49152): 1st quadrant >> Direction = 49152: Right >> Direction (49153; 65535) :4th quadrant >> Direction = 65565: Down >> " > > The above is correct. > >> For a Cartesian coordinates system: >> - The (-1, 0)-axis (=-x) is the intersection of 3rd quadrant and 2nd >> quadrant => Left >> - The (0, +1)-axis (=+y) is the intersection of 2nd quadrant and 1st >> quadrant => Up >> - The (+1, 0)-axis (=+x) is the intersection of 1st quadrant and 4th >> quadrant => Right >> - The (0, -1)-axis (=-y) is the intersection of 4th quadrant and 3rd >> quadrant => Down > > Not s
Re: Questions about the documentation/specification of Linux ForceFeedback input.h
On Sat, Feb 15, 2014 at 3:05 AM, Anssi Hannula wrote: > 15.02.2014 01:28, Elias Vanderstuyft kirjoitti: >> Hi everyone, > > Hi! > >> >> If you receive my mail, it is either because you: >> - are listed under the MAINTAINERS for "/include/uapi/linux/input.h": >> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS#n4410 >> - are listed as an author of "/Documentation/input/ff.txt" or of >> "/Documentation/input/interactive.fig" >> >> In the process of testing the new driver (ff-memless-next) Michal >> (CC-ed) is working on, this allowed me to gain more insight in the >> internal workings of Linux FF. That's why I decided to review the >> userspace code that currently uses most of Linux FF functionality: >> Wine's DInput translation layer (and eventually SDL2.) >> Trying to get everything correct, I noticed both >> "/include/uapi/linux/input.h" and "/Documentation/input/ff.txt" >> provide not enough information for the following things: >> >> 1) >> The real meaning of 'directions', declared in >> http://lxr.free-electrons.com/source/include/uapi/linux/input.h#L1113 >> : >> "Direction of the effect" is encoded as follows: ... >> But it is not clear whether 'direction of the effect' means either: >> - positive direction of the force the user should apply to counteract >> the force that the joystick applies; or >> - positive direction of the force applied by joystick >> From my intuition, I think the latter is (silently?) meant by input.h >> If you're interested why this is so important, I attached a document >> "DInputVsLinuxDirections.txt" that tries to explain the dilemma if a >> translation layer between DInput and Linux input is to be written. > > From input.h: > * Direction of the effect is encoded as follows: > * 0 deg -> 0x (down) > * 90 deg -> 0x4000 (left) > * 180 deg -> 0x8000 (up) > * 270 deg -> 0xC000 (right) > > The directions in parantheses are the direction of applied force. Alright, thanks! That is invaluable information, maybe it should be added to input.h . It will avoid a lot of confusion for former DInput devs. > > However, there is actually a 1:1 mapping between DInput polar > coordinates and our direction parameter; DInput polar coordinates have 0 > deg = up, 90 deg = right, etc, so they are exactly flipped and therefore > match our values due to the reverse definition. > > Looking at your DInputVsLinuxDirections.txt, you seem to have mixed > different definitions of Carts: For DInput you use -1 = north, but for > Linux +1 = up, while you use -1 = west/left for both. So I assume you agree that I got the DInput part right? ((0, -1) = north) Then my mistake lies in the assumption that (0, +1) = up, so I should flip the y-axis to correct the Linux part. I'll explain how I originally derived the Linux part: Michal documented (in "ff-memless-next.txt") the Linux directions in the following way: " Direction of the effect's force translates to Cartesian coordinates system as follows: Direction = 0: Down Direction (0; 16384): 3rd quadrant Direction = 16384: Left Direction (16385; 32768): 2nd quadrant Direction = 32768: Up Direction (32769; 49152): 1st quadrant Direction = 49152: Right Direction (49153; 65535) :4th quadrant Direction = 65565: Down " For a Cartesian coordinates system: - The (-1, 0)-axis (=-x) is the intersection of 3rd quadrant and 2nd quadrant => Left - The (0, +1)-axis (=+y) is the intersection of 2nd quadrant and 1st quadrant => Up - The (+1, 0)-axis (=+x) is the intersection of 1st quadrant and 4th quadrant => Right - The (0, -1)-axis (=-y) is the intersection of 4th quadrant and 3rd quadrant => Down Michal's approach seems logical to me, if he made a mistake, it's caused by the lack of documentation of input.h : it should mention what axes (-x, +x, -y or +y) the words (left, right, down and up) correspond with. So, which interpretation is the right one? (I did not find anything in the Linux documentation that states "there is actually a 1:1 mapping between DInput polar coordinates and our direction parameter") > This causes the > 1st and 3rd entries on both of the Mapping tables to be reversed. When > that is fixed, the table #2 shows the correct result. > >> 2) >> It is not clear how to create a FF effect with infinite duration >> (replay.length): >> http://lxr.free-electrons.com/source/include/uapi/linux/input.h#L966 >> Only the following is written about duration values in general: >> "All duration values are express
