[PATCH 1/2] hid-ntrig: sysfs nodes for modes

[PATCH 1/2] hid-ntrig: sysfs nodes for modes

[Rafi Rubin]

Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>

---

If the code is there, might as well expose a friendly interface to the
user.
---
 drivers/hid/hid-ntrig.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 61 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index fa862c5..24ab6a5 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -24,6 +24,8 @@
 
 #define NTRIG_DUPLICATE_USAGES	0x001
 
+static const char *ntrig_modes[4] = { "pen", "touch", "auto", "dual" };
+
 static unsigned int min_width;
 module_param(min_width, uint, 0644);
 MODULE_PARM_DESC(min_width, "Minimum touch contact width to accept.");
@@ -430,6 +432,64 @@ static ssize_t set_deactivate_slack(struct device *dev,
 static DEVICE_ATTR(deactivate_slack, S_IWUSR | S_IRUGO, show_deactivate_slack,
 		   set_deactivate_slack);
 
+
+static ssize_t show_mode(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	int mode = ntrig_get_mode(container_of(dev, struct hid_device, dev));
+	int i, ret;
+	char *s = buf;
+
+	if (mode < 0)
+		return mode;
+
+
+	for (i = 0; i < 4; i++)
+		s += sprintf(s, "%s%s%s ", (i == mode) ? "[" : "",
+			     ntrig_modes[i], (i == mode) ? "]" : "");
+
+	if (mode >= 4)
+		s += sprintf(s, "[%d]\n", mode);
+	else
+		*(s - 1) = '\n';
+
+	ret = s - buf;
+
+	return ret;
+}
+
+static ssize_t store_mode(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	int len;
+	int i;
+	int mode = -EINVAL;
+	char *p;
+
+	p = memchr(buf, '\n', count);
+	len = p ? p - buf : count;
+
+	if (len == 1 && buf[0] >= '0' && buf[0] <= '4') {
+		mode = buf[0] - '0';
+	} else
+		for (i = 0; i < 4; i++)
+			if (len == strlen(ntrig_modes[i]) &&
+			    !strncmp(buf, ntrig_modes[i], len)) {
+				mode = i;
+				break;
+			}
+
+	if (mode < 0)
+		return mode;
+
+	ntrig_set_mode(container_of(dev, struct hid_device, dev), mode);
+
+	return count;
+}
+static DEVICE_ATTR(mode, S_IWUSR | S_IRUGO, show_mode, store_mode);
+
 static struct attribute *sysfs_attrs[] = {
 	&dev_attr_sensor_physical_width.attr,
 	&dev_attr_sensor_physical_height.attr,
@@ -441,6 +501,7 @@ static struct attribute *sysfs_attrs[] = {
 	&dev_attr_activation_width.attr,
 	&dev_attr_activation_height.attr,
 	&dev_attr_deactivate_slack.attr,
+	&dev_attr_mode.attr,
 	NULL
 };
 
-- 
1.7.2.3

[PATCH 2/2] hid-ntrig: calibration

[Rafi Rubin]

Adding a function to tell the device to run its calibration routine.
A number written to the sysfs specifies the duration of the calibration
in milliseconds

Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>
---

The output needs to stay disconnected for the duration for this to work,
otherwise I would have just used submit_report.  Please check to see
that I handled that correctly, I can say it works for me.

Is there a style problem with having wait and sleep in a sysfs call like
this?

Micki: would you care to comment on this?  The windows driver seems to
send extra messages to the device, but in testing they don't seem to
have an effect.  If they do matter, it would be no trouble to put them
back in.  Also can you comment on the difference between "trouble
shooting" and the calibration that your windows tool offers?  As far as
I can tell the actions look very similar.  Is it just a matter of
interpretting the feedback?
---
 drivers/hid/hid-ntrig.c |   48 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 24ab6a5..ddf2c76 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -490,6 +490,53 @@ static ssize_t store_mode(struct device *dev,
 }
 static DEVICE_ATTR(mode, S_IWUSR | S_IRUGO, show_mode, store_mode);
 
+static ssize_t ntrig_calibrate(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
+	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
+	struct usbhid_device *usbhid = hdev->driver_data;
+	int ret;
+	unsigned long t;
+	unsigned char *data;
+
+	if (strict_strtoul(buf, 0, &t))
+		return -EINVAL;
+
+	data = kmalloc(4, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	spin_lock(&usbhid->lock);
+	set_bit(HID_DISCONNECTED, &usbhid->iofl);
+	spin_unlock(&usbhid->lock);
+
+	ret = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+			      USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
+			      | USB_RECIP_INTERFACE | USB_DIR_IN,
+			      0x30b, 1, data, 4, USB_CTRL_GET_TIMEOUT);
+	if (ret < 0)
+		goto fail;
+
+	msleep(t);
+
+	ret = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+			      USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
+			      | USB_RECIP_INTERFACE | USB_DIR_IN,
+			      0x311, 1, data, 4, USB_CTRL_GET_TIMEOUT);
+
+fail:
+	kfree(data);
+	spin_lock(&usbhid->lock);
+	clear_bit(HID_DISCONNECTED, &usbhid->iofl);
+	spin_unlock(&usbhid->lock);
+	schedule_work(&usbhid->reset_work);
+
+	return (ret < 0) ? ret : count;
+}
+static DEVICE_ATTR(calibrate, S_IWUSR, NULL, ntrig_calibrate);
+
 static struct attribute *sysfs_attrs[] = {
 	&dev_attr_sensor_physical_width.attr,
 	&dev_attr_sensor_physical_height.attr,
@@ -502,6 +549,7 @@ static struct attribute *sysfs_attrs[] = {
 	&dev_attr_activation_height.attr,
 	&dev_attr_deactivate_slack.attr,
 	&dev_attr_mode.attr,
+	&dev_attr_calibrate.attr,
 	NULL
 };
 
-- 
1.7.2.3

Re: [PATCH 2/2] hid-ntrig: calibration

[Rafi Rubin]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On a related note, I looked into switching to usbhid_submit_report for firmware
version polling but it looks like the report offset (or something like that) is
wrong.

I get:
af082500

usbmon shows the devices sends:
0c002508 afe10000

If I change the report offset in the field to 8, I get the desired bits:
e1af0825


I don't really know much about parsing the hid descriptors, so I don't know why
its off by a byte.  Perhaps its worth testing a different firmware to see if
this is specific to my current firmware.

Rafi

On 03/11/11 02:37, Rafi Rubin wrote:
> Adding a function to tell the device to run its calibration routine.
> A number written to the sysfs specifies the duration of the calibration
> in milliseconds
> 
> Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>
> ---
> 
> The output needs to stay disconnected for the duration for this to work,
> otherwise I would have just used submit_report.  Please check to see
> that I handled that correctly, I can say it works for me.
> 
> Is there a style problem with having wait and sleep in a sysfs call like
> this?
> 
> Micki: would you care to comment on this?  The windows driver seems to
> send extra messages to the device, but in testing they don't seem to
> have an effect.  If they do matter, it would be no trouble to put them
> back in.  Also can you comment on the difference between "trouble
> shooting" and the calibration that your windows tool offers?  As far as
> I can tell the actions look very similar.  Is it just a matter of
> interpretting the feedback?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJNedO2AAoJEPILXytRLnK2bysP/R3GJGkXrq8EH2qxBmcBXaxy
xD8VRCfN17kPTNf2FQj5A/dpG4NPscwG8wOMxPLa69gal5trJQnm6ekJvinajt1Y
aDEFm/6TripZLRJC8xEWnFN5VK7HOmiXe5CiEJhmfg+KFEjitNekMleNC/9wQEbN
XSH/8NwdU8f82HRgKBL0odpy8Qzjtdg4uRHuNmA9Ec9lp9G8wcYoKGXrDNBMmyf6
lgaaptqw+Tiv2Ad9VOjf9hBQS1Mq7EQ5oKbC2RJeDLRmyJ+nZ1AtZ0rIIYa7/nHC
k78rhUS5+m2WW+1r6ahbyTZWOtNZaZNUyKgG03MXwRcx6GLOu+5WZGuew+Dg/qA4
ws8ZMIbca1sZHa+pwRi/m5hPuU3dSB3J7U/SwSbD5+EGcV2OjN8/w1QbPsnMW9xp
kmt2HjkNh4wMzO7s4vyXOQ1QbQRvBP32NMqbsmW1TjNmYXiC9PgsfsxdKhVox9rQ
ZY8CioS2+zDrL5nmTaVbUnziIEwP1/j2z4rrIE9R7lsnrJDW+aQ0NqxWce/9FJRn
qoRIvYFEHXq6GA1lbUMhgk5dy3NIUHl1dSYEWw5Tcd/YrYfcTQdix8me490OkEds
AD0db+27ifK/xBy8J0LnPYlQLpqgtKHjGcNP/Px2XJTJl8r2+0B8J/hWf9cNQZ8L
Q3ev12Gp5psWNwftUzQe
=X3PD
-----END PGP SIGNATURE-----

Re: [PATCH 2/2] hid-ntrig: calibration

[Jiri Kosina]

On Fri, 11 Mar 2011, Rafi Rubin wrote:

> On a related note, I looked into switching to usbhid_submit_report for firmware
> version polling but it looks like the report offset (or something like that) is
> wrong.
> 
> I get:
> af082500
> 
> usbmon shows the devices sends:
> 0c002508 afe10000
> 
> If I change the report offset in the field to 8, I get the desired bits:
> e1af0825
> 
> 
> I don't really know much about parsing the hid descriptors, so I don't know why
> its off by a byte.  Perhaps its worth testing a different firmware to see if
> this is specific to my current firmware.

I am not really sure I fully understand what you are saying here. Just for 
clarification:

- isn't the byte the 'report id' number here?
- does your message mean that the patch doesn't work? Or is this quite the 
contrary -- explanation why it is written the way it is (i.e. using 
usb_control_msg() instead of usbhid_submit_report())?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 2/2] hid-ntrig: calibration

[Rafi Rubin]

----- Original message -----
> On Fri, 11 Mar 2011, Rafi Rubin wrote:
> 
> > On a related note, I looked into switching to usbhid_submit_report for
> > firmware version polling but it looks like the report offset (or
> > something like that) is wrong.
> > 
> > I get:
> > af082500
> > 
> > usbmon shows the devices sends:
> > 0c002508 afe10000
> > 
> > If I change the report offset in the field to 8, I get the desired
> > bits: e1af0825
> > 
> > 
> > I don't really know much about parsing the hid descriptors, so I don't
> > know why its off by a byte.   Perhaps its worth testing a different
> > firmware to see if this is specific to my current firmware.
> 
> I am not really sure I fully understand what you are saying here. Just
> for   clarification:
> 
> - isn't the byte the 'report id' number here?
> - does your message mean that the patch doesn't work? Or is this quite
> the   contrary -- explanation why it is written the way it is (i.e. using 
> usb_control_msg() instead of usbhid_submit_report())?

usbhid_submit_report does not work for calibration and firmware version polling.

Sorry for the confusion.

Rafi
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] hid-ntrig: calibration

[Henrik Rydberg]

Hi Rafi,

On Fri, Mar 11, 2011 at 02:37:52AM -0500, Rafi Rubin wrote:
> Adding a function to tell the device to run its calibration routine.
> A number written to the sysfs specifies the duration of the calibration
> in milliseconds
> 
> Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>
> ---

This is great functionality, and from what it seems, it
works. However, the poking at the usb layer makes me wonder if there
is another way... Jiri? Dmitry? Awaiting answers from someone more
knowledgeable, please find some comments inline.

> diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
> index 24ab6a5..ddf2c76 100644
> --- a/drivers/hid/hid-ntrig.c
> +++ b/drivers/hid/hid-ntrig.c
> @@ -490,6 +490,53 @@ static ssize_t store_mode(struct device *dev,
>  }
>  static DEVICE_ATTR(mode, S_IWUSR | S_IRUGO, show_mode, store_mode);
>  
> +static ssize_t ntrig_calibrate(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> +	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
> +	struct usbhid_device *usbhid = hdev->driver_data;
> +	int ret;
> +	unsigned long t;
> +	unsigned char *data;
> +
> +	if (strict_strtoul(buf, 0, &t))
> +		return -EINVAL;
> +
> +	data = kmalloc(4, GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	spin_lock(&usbhid->lock);
> +	set_bit(HID_DISCONNECTED, &usbhid->iofl);
> +	spin_unlock(&usbhid->lock);

Perhaps an addition to the internal HID api instead of the above?

> +
> +	ret = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
> +			      USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
> +			      | USB_RECIP_INTERFACE | USB_DIR_IN,
> +			      0x30b, 1, data, 4, USB_CTRL_GET_TIMEOUT);
> +	if (ret < 0)
> +		goto fail;

How about launching restoration work here, instead of waiting?

> +
> +	msleep(t);
> +
> +	ret = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
> +			      USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
> +			      | USB_RECIP_INTERFACE | USB_DIR_IN,
> +			      0x311, 1, data, 4, USB_CTRL_GET_TIMEOUT);
> +
> +fail:
> +	kfree(data);
> +	spin_lock(&usbhid->lock);
> +	clear_bit(HID_DISCONNECTED, &usbhid->iofl);
> +	spin_unlock(&usbhid->lock);
> +	schedule_work(&usbhid->reset_work);
> +
> +	return (ret < 0) ? ret : count;
> +}
> +static DEVICE_ATTR(calibrate, S_IWUSR, NULL, ntrig_calibrate);
> +
>  static struct attribute *sysfs_attrs[] = {
>  	&dev_attr_sensor_physical_width.attr,
>  	&dev_attr_sensor_physical_height.attr,
> @@ -502,6 +549,7 @@ static struct attribute *sysfs_attrs[] = {
>  	&dev_attr_activation_height.attr,
>  	&dev_attr_deactivate_slack.attr,
>  	&dev_attr_mode.attr,
> +	&dev_attr_calibrate.attr,

Maybe one could actually run the calibration at module load, or even
at start/stop? A few seconds of unresponsiveness might be ok at that
time.

>  	NULL
>  };

Thanks,
Henrik

Re: [PATCH 2/2] hid-ntrig: calibration

[Rafi Rubin]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 03/21/11 13:15, Henrik Rydberg wrote:
> Hi Rafi,
> 
> On Fri, Mar 11, 2011 at 02:37:52AM -0500, Rafi Rubin wrote:
>> Adding a function to tell the device to run its calibration routine.
>> A number written to the sysfs specifies the duration of the calibration
>> in milliseconds
>>
>> Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>
>> ---
> 
> This is great functionality, and from what it seems, it
> works. However, the poking at the usb layer makes me wonder if there
> is another way... Jiri? Dmitry? Awaiting answers from someone more
> knowledgeable, please find some comments inline.
> 
>> diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
>> index 24ab6a5..ddf2c76 100644
>> --- a/drivers/hid/hid-ntrig.c
>> +++ b/drivers/hid/hid-ntrig.c
>> @@ -490,6 +490,53 @@ static ssize_t store_mode(struct device *dev,
>>  }
>>  static DEVICE_ATTR(mode, S_IWUSR | S_IRUGO, show_mode, store_mode);
>>  
>> +static ssize_t ntrig_calibrate(struct device *dev,
>> +			       struct device_attribute *attr,
>> +			       const char *buf, size_t count)
>> +{
>> +	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> +	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
>> +	struct usbhid_device *usbhid = hdev->driver_data;
>> +	int ret;
>> +	unsigned long t;
>> +	unsigned char *data;
>> +
>> +	if (strict_strtoul(buf, 0, &t))
>> +		return -EINVAL;
>> +
>> +	data = kmalloc(4, GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	spin_lock(&usbhid->lock);
>> +	set_bit(HID_DISCONNECTED, &usbhid->iofl);
>> +	spin_unlock(&usbhid->lock);
> 
> Perhaps an addition to the internal HID api instead of the above?

I don't have a personal preference.  I figured this is probably unusual enough
that there might not be a point in putting the support in the core, but that's
part of the point of review by other people who know if this would be useful
elsewhere.

>> +
>> +	ret = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
>> +			      USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
>> +			      | USB_RECIP_INTERFACE | USB_DIR_IN,
>> +			      0x30b, 1, data, 4, USB_CTRL_GET_TIMEOUT);
>> +	if (ret < 0)
>> +		goto fail;
> 
> How about launching restoration work here, instead of waiting?

The user needs some feedback bracketing the calibration routine.  If one touches
the screen during calibration alters the result and typically results in dead
zones (a useful check to verify it works).

Perhaps during the loading of the driver that would make more sense.  I hadn't
really thought about it.


I would like some pointers of how to use work queues.  That's the last item
holding me back from sending in my new track and filter code for review.

>> +
>> +	msleep(t);
>> +
>> +	ret = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
>> +			      USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
>> +			      | USB_RECIP_INTERFACE | USB_DIR_IN,
>> +			      0x311, 1, data, 4, USB_CTRL_GET_TIMEOUT);
>> +
>> +fail:
>> +	kfree(data);
>> +	spin_lock(&usbhid->lock);
>> +	clear_bit(HID_DISCONNECTED, &usbhid->iofl);
>> +	spin_unlock(&usbhid->lock);
>> +	schedule_work(&usbhid->reset_work);
>> +
>> +	return (ret < 0) ? ret : count;
>> +}
>> +static DEVICE_ATTR(calibrate, S_IWUSR, NULL, ntrig_calibrate);
>> +
>>  static struct attribute *sysfs_attrs[] = {
>>  	&dev_attr_sensor_physical_width.attr,
>>  	&dev_attr_sensor_physical_height.attr,
>> @@ -502,6 +549,7 @@ static struct attribute *sysfs_attrs[] = {
>>  	&dev_attr_activation_height.attr,
>>  	&dev_attr_deactivate_slack.attr,
>>  	&dev_attr_mode.attr,
>> +	&dev_attr_calibrate.attr,
> 
> Maybe one could actually run the calibration at module load, or even
> at start/stop? A few seconds of unresponsiveness might be ok at that
> time.

I still have mixed feelings about that.  I like the idea, but I'm concerned
about keeping hands off the screen during calibration.  This seems like a
Hippocratic issue, first do no harm.  For the devices that are stable, I
wouldn't be surprised if we actually make things worse more often than better.

For the devices that are really screwed up (I've heard reports of machines that
are stable for a few hours at best), we'll need something more active than load
time calibration.

I've been thinking for a while about using feedback from the filters to decide
when its time to recalibrate (a problem that's closer to my day job than most of
this).  In those cases it would be nice to either notify that it needs
calibration, or in the extreme that calibration is already running and to keep
hands off.

I need to play more with short calibrations, I've played a bit, but not
carefully.  If we can do some good with a few milliseconds of calibrations that
would make auto-calibration more tempting.

Also I don't know how frequent calibrations will affect the device.  If, for
example, calibration is stored in flash that's only intended to be written a few
hundred times we could wear it out pretty quickly.  If only we had some feedback
from the folks that build these things....

>>  	NULL
>>  };
> 
> Thanks,
> Henrik
> 

Thanks for the feedback,
Rafi
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJNh5gQAAoJEPILXytRLnK2Kl0P+we4k9fhKOPx/VP7gW1l0KZT
61PNdKXhMdeg4T4ZmOa2n6pz2xzfDozP+vuRJgkilS0eEz3TZqKvTdiiw0ykjZlK
+Ko1a+T7gAN+ZnSP4AFFglF82K4mbW/Vd0vVwbbC3tUbyzwqxvPrLRxWKhgDJ6FY
CP4i0jW4AJawpB3UoIVwli66X1DzD9RaPNqCJrQiSEDxcay6H1oPB3rwDKGdrZ51
RMKsMaGF7kfeQ1SfrnDhrhEM/8tvs86FQTkHkZ2m2bHdjDEh3uMwfavCe7z4hWUL
4Ln1RMc4fjBoudSMU20WkT38PyqnC5FMfhtxcP50SNjxzsDIQttXU6WE6xjBGAG1
Jt0AKbM/nr3QjhlD7AbbC8NjvgP6JY4ZfneBm5e2tFATluzeRoTfY2e9m76PRr1t
QUhdF26qKZf6Vzt7OnOSxHWH5SyL92C+3q0ScV0c7F8l0MFCnALKCxs7RXIPE1kT
DCmN5fEp6SvtYdkWOtddvR3qex6gCipaUu4lq7mf/snHn4yw2iuMsisns0XGv7AU
MRF0gwzYhWAkbeFvHPuhwE0/R10w7pZXXiPGyARH92nlpGJxOiHb6gZ32TdMUlm/
h5DOhggXGI0wSLSAAP6BTz2f0+pMihTGNvlgKgu5AnvTbYS5coYkpufGQ+qKltHV
oQHICuniy4uI6s88PrZ/
=Fx2M
-----END PGP SIGNATURE-----

Re: [PATCH 2/2] hid-ntrig: calibration

[Jiri Kosina]

On Mon, 21 Mar 2011, Henrik Rydberg wrote:

> > Adding a function to tell the device to run its calibration routine.
> > A number written to the sysfs specifies the duration of the calibration
> > in milliseconds
> > 
> > Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>
> > ---
> 
> This is great functionality, and from what it seems, it works. However, 
> the poking at the usb layer makes me wonder if there is another way... 
> Jiri? Dmitry? Awaiting answers from someone more knowledgeable, please 
> find some comments inline.

Actually, let me put it another way -- is there any reason not to have 
this as a functionality provided by HID core code (with driver registering 
callbacks if they want to provide calibration functionality)?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 2/2] hid-ntrig: calibration

[Rafi Rubin]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/24/11 10:32, Jiri Kosina wrote:
> On Mon, 21 Mar 2011, Henrik Rydberg wrote:
> 
>>> Adding a function to tell the device to run its calibration routine.
>>> A number written to the sysfs specifies the duration of the calibration
>>> in milliseconds
>>>
>>> Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>
>>> ---
>>
>> This is great functionality, and from what it seems, it works. However, 
>> the poking at the usb layer makes me wonder if there is another way... 
>> Jiri? Dmitry? Awaiting answers from someone more knowledgeable, please 
>> find some comments inline.
> 
> Actually, let me put it another way -- is there any reason not to have 
> this as a functionality provided by HID core code (with driver registering 
> callbacks if they want to provide calibration functionality)?
> 
> Thanks,
> 

I thought Henrik was asking about an alternative to the raw usb calls, something
like usbhid_submit_report that would satisfy the needs of this calibration routine.


As for a standard hid calibration interface, I like the idea.

Rafi
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJNi3bIAAoJEPILXytRLnK2HO4QAJQtrlG2PB4mERWQ5gGkoJTT
qYoQiek1RJylFb/wJlzVajMtjQ+kFc98N3faSmIvbrsXwYZfEYZQxlnS/463EYTw
vnYgSFTqj7+mSwrQMX4civJTZJPfbLGJD1wvzBBq8cAhPonedn2z3a8gU30AdPc3
Dc/Ht+dHgOomzdkfjn77GTGYyiGDZwl0mYPuh6nhOBjhOmUDErRg+4yZQ1/wfaJr
2jEty0hEXwwkD1iqgDPmIqiEIZ7r5RkGCOWUsi+0rMeIiobaUxk281c0aZ295BFT
PhGbiqzkASp+a7TpVpdnHFCofmCSinb00gpbVbeqtuUsauNuUgVVij/lPe0vxTNB
ivW5V3MYgceDCDkAR+p3nsu3lvYv6cWqbxFQ5pzUIkxsnEfUowpjWLoSQl3Fmym0
IbKYs4ueAgI3G7gw+xQfjnJX53DmAbKoXoQAesSEwzAXCAz1BSZih2ORPTGM68Pd
eDLk729/6RAFZVmnwgNlEfx68vaxmhrHK0GDod2k/V/TRsSBG42I15G8p7Xu359m
cQyNUraMDFfHvC71Y6ZIL3tK0h2LXjsvd1Z17HzAROFWm2tNSSjg/xaVJ0ftDot3
cIM86Na/ysGwdKRB1xdElPBf8rb+vjVPDcv4v3PJTd9GbgN6MoRl1RWtv+XwnW/A
VzPpDqxYbAW9hewHbzSW
=KW05
-----END PGP SIGNATURE-----

Re: [PATCH 1/2] hid-ntrig: sysfs nodes for modes

[Dmitry Torokhov]

On Fri, Mar 11, 2011 at 02:37:51AM -0500, Rafi Rubin wrote:
> Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>
> 
> ---
> 
> If the code is there, might as well expose a friendly interface to the
> user.

The bigger question is should the user care? If we can make the device
work well without user intervention then we should not really expose
extra attributes.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] hid-ntrig: sysfs nodes for modes

[Rafi Rubin]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

User desires aren't exactly my specialty.  I just know the ntrig windows control
panel gives the user that control with cute little radio buttons.

Would it be cleaner to expose the control with an ioctrl or some other mechanism?

I do have user space tools for mode and calibration, but that requires unbinding
the device, and just seems sloppier.  But if this really is inappropriate to add
the sysfs nodes, at least there's still some solution.

Since you brought it up, I am thinking of removing some of the other nodes soon.
 I've learned the physical and logical ranges are already exposed both through
the event nodes and debugfs.  Also, I have better filtering that so far seems
not to need as many parameters so I should be able to remove those nodes as well.

Still learning,
Rafi

On 03/11/11 03:18, Dmitry Torokhov wrote:
> On Fri, Mar 11, 2011 at 02:37:51AM -0500, Rafi Rubin wrote:
>> Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>
>>
>> ---
>>
>> If the code is there, might as well expose a friendly interface to the
>> user.
> 
> The bigger question is should the user care? If we can make the device
> work well without user intervention then we should not really expose
> extra attributes.
> 
> Thanks.
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJNefHZAAoJEPILXytRLnK2ud4QAJxzSX+tIrkZawpqvJM69mf3
1P4+E8vCXqsX9usaayGQoa/+CLe2so6frQo3B5nH8ExTOs8+a/Zx1Y9JbNxwqDBJ
aMeWBhO9stqdGjg2hllFeJNV/vuvlU7y6oOW6qme/sKHp0zfisco2nJNvG3yHAQl
tD0TVgiCeEIWkN/AAqoUxGSMtv9pOmkcVfShDVktmOH6pqAsacP29UENZBAKuqQ6
jHtJ2hfQV5NBrKjlHPhcvTiBRrls5SZ+RfRq4hy8MWC8Vj5rjjNLCxmgBeI5Ktqf
ZTRNVvvHClgIbuwHx5LdzPBFNjYv35A87vW8zMHb6/qU57KkeXL1VX6RcONTOS+w
6fWX2AYVMxs/6rU6nXfa/DxgRlPZqyVGP1E8VZ+bl09fJ41WR7xpsOIko93KemUq
DC+kxp9nyfH7bZEpWsr3chEQkbj2aT+1Rjc6312Vsio2Gbzgth8t0WWxEIX69Sp+
hZCzrFPFKiUrlWOdKQG1TkJNh6Z9a1HQLYb67ga2/liyJt9rW6uUfylQDJqN9adj
iGE9xVX6D4+da0CW+hl+0U+EhGX+ZSmvI5IgVdBGsxZnFa/ReP5nJKGFcJmkTtPO
WcZe0zqxORua6xPqA9p2cScbcvCXiHLQtjpLxBQMLlMgghKunSVX9Hl/3OrtOHRZ
r36BGVFmHWBvddvosBqK
=vHok
-----END PGP SIGNATURE-----

Re: [PATCH 1/2] hid-ntrig: sysfs nodes for modes

[Dmitry Torokhov]

On Fri, Mar 11, 2011 at 04:56:45AM -0500, Rafi Rubin wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> User desires aren't exactly my specialty.  I just know the ntrig windows control
> panel gives the user that control with cute little radio buttons.
> 
> Would it be cleaner to expose the control with an ioctrl or some other mechanism?
> 
> I do have user space tools for mode and calibration, but that requires unbinding
> the device, and just seems sloppier.  But if this really is inappropriate to add
> the sysfs nodes, at least there's still some solution.

No, this is not inappropriate, I was just musing how useful they are. Do
we foresee users really using them or if is it more "we export because we can".
I.e. in which cases the default mode is not suitable?

> 
> Since you brought it up, I am thinking of removing some of the other nodes soon.
>  I've learned the physical and logical ranges are already exposed both through
> the event nodes and debugfs.  Also, I have better filtering that so far seems
> not to need as many parameters so I should be able to remove those nodes as well.
>

Well, that's the issue with sysfs - it really forms kernel ABI so
removing something that was once added is hard.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] hid-ntrig: sysfs nodes for modes

[Rafi Rubin]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/13/11 01:27, Dmitry Torokhov wrote:
> On Fri, Mar 11, 2011 at 04:56:45AM -0500, Rafi Rubin wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> User desires aren't exactly my specialty.  I just know the ntrig windows control
>> panel gives the user that control with cute little radio buttons.
>>
>> Would it be cleaner to expose the control with an ioctrl or some other mechanism?
>>
>> I do have user space tools for mode and calibration, but that requires unbinding
>> the device, and just seems sloppier.  But if this really is inappropriate to add
>> the sysfs nodes, at least there's still some solution.
> 
> No, this is not inappropriate, I was just musing how useful they are. Do
> we foresee users really using them or if is it more "we export because we can".
> I.e. in which cases the default mode is not suitable?

Calibration is handy, I haven't counted how many users have asked for help and
calibrating fixed their problems.  It can be run with libusb from userspace, but
that requires unbinding the device and running more stuff with elevated
privileges.  X hotswap of input devices has improved, but transient settings are
still lost.  And of course we're still talking pain for any application that
uses the device directly but doesn't handle hotswap.


As for mode setting, I know I have used that feature through userspace and my
sysfs nodes.  But that doesn't mean other people care about that feature.
N-trig put it in the firmware and their windows control panel, perhaps Micki or
someone else from the company would care to weigh in on that.

>> Since you brought it up, I am thinking of removing some of the other nodes soon.
>>  I've learned the physical and logical ranges are already exposed both through
>> the event nodes and debugfs.  Also, I have better filtering that so far seems
>> not to need as many parameters so I should be able to remove those nodes as well.
>>
> 
> Well, that's the issue with sysfs - it really forms kernel ABI so
> removing something that was once added is hard.

Hm, sorry I was careless about that.  I was hoping for some feedback on the
tuning parameters and hoped to be able to remove some of those nodes once we
established good values.  Of course almost nobody actually used them and let me
know :(

Rafi
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJNfuthAAoJEPILXytRLnK2brkP/1bj6+i+/umuuyJq58PBe8P1
+bagO7Rz2NUee7o++wdJSUW+GaL7lGLOySKi+gJ5N7vYHn7AekWT4lIUqjv0PinE
mnI2nhFqnWTyTGjbOFxazR4o6DG7YcdSmHDvLbqUOh7mJbfxZI9XqFLV7E+mCDpG
7VjtZMANq2Ju2/qr5B1h53WzvufEoXGhNIRyMpJbEaFAFjR0iG4UmTKNtsTPvNVt
knzZfkKk4VX+h72WgvvQ85/ViTLtBQXIU/AFNlgn3hysM+orA/OF1Rp0OX5aAz/p
rDQeJCYtP6GK1pzIbir2SS1tvnFfm+G2gxIDkBeh4ahL/x6z0D8EHl6rdZiodVeK
bxaXRQJh8cFTWMUDrySlzVf88ZQRvNVK8+bRcNXlqWnCDoY2xopPrVGBninZwvIs
laiWN64di1HVn7hgoy/iW/L2SA8NC9wQjd8X5TL9+EH0lToUvHxfqXooJzq31iwI
UuHPpVdyIeBTVl81zj+/cHWi4mEXnnjoANbjWOnTfFwDzDwaxop4/OwKhl4yebRM
t8ZJGPAJAzlse+FeuEd20P6kRvN7AUzBaG6Zwvyi9IRPfOPbsTF86jllNxJaEv4Y
ExuR6QZQ+HFRveojqhoHgD79nvgtQZlpVvIIggnsm0ImQ0pmSMMuhMjSb+Mna8c+
RfiTBxl9o6vVutQ4AkTk
=QQ0T
-----END PGP SIGNATURE-----