[PATCH 1/1] Revert "HID: fix unit exponent parsing"

[PATCH 1/1] Revert "HID: fix unit exponent parsing"

[Nikolai Kondrashov]

From: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>

Revert "HID: fix unit exponent parsing" as it is based on incorrect
understanding of the HID specification and breaks resolution
calculation at least on graphics tablets.

There are two "unit exponents" in HID specification and it is important
not to mix them. One is the global "Unit Exponent" item and another is
nibble values in the global "Unit" item. See 6.2.2.7 Global Items.

The "Unit Exponent" value is just a signed integer and is used to scale
the integer resolution unit values, so fractions can be expressed.

The nibbles of "Unit" value are used to select the unit system (nibble
0), and presence of a particular basic unit type in the unit formula and
its *exponent* (or power, nibbles 1-6). And yes, the latter is in two
complement and zero means absence of the unit type.

Taking the representation example of (integer) joules from the
specification:

    [mass(grams)][length(centimeters)^2][time(seconds)^-2] * 10^-7

the "Unit Exponent" would be -7 (or 0xF9, if stored as a byte) and the
"Unit" value would be 0xE121, signifying:

    Nibble  Part        Value   Meaning
    -----   ----        -----   -------
    0       System      1       SI Linear
    1       Length      2       Centimeters^2
    2       Mass        1       Grams
    3       Time        -2      Seconds^-2

To give the resolution in e.g. hundredth of joules the "Unit Exponent"
item value should have been -9.

See also the examples of "Unit" values for some common units in the same
chapter.

This reverts commit 774638386826621c984ab6994439f474709cac5e.
---

Jiri, I've had the HID specification thoroughly read WRT report descriptors
when implementing hidrd [1] and would be happy to help reviewing any patches
touching that area. Please CC me if there's any need.

[1] https://sf.net/apps/mediawiki/digimend?title=Hidrd

 drivers/hid/hid-core.c  | 16 +---------------
 drivers/hid/hid-input.c | 13 ++++---------
 include/linux/hid.h     |  1 -
 3 files changed, 5 insertions(+), 25 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index b8470b1..94350b7 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -319,7 +319,6 @@ static s32 item_sdata(struct hid_item *item)
 
 static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
 {
-	__u32 raw_value;
 	switch (item->tag) {
 	case HID_GLOBAL_ITEM_TAG_PUSH:
 
@@ -370,14 +369,7 @@ static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_UNIT_EXPONENT:
-		/* Units exponent negative numbers are given through a
-		 * two's complement.
-		 * See "6.2.2.7 Global Items" for more information. */
-		raw_value = item_udata(item);
-		if (!(raw_value & 0xfffffff0))
-			parser->global.unit_exponent = hid_snto32(raw_value, 4);
-		else
-			parser->global.unit_exponent = raw_value;
+		parser->global.unit_exponent = item_sdata(item);
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_UNIT:
@@ -983,12 +975,6 @@ static s32 snto32(__u32 value, unsigned n)
 	return value & (1 << (n - 1)) ? value | (-1 << n) : value;
 }
 
-s32 hid_snto32(__u32 value, unsigned n)
-{
-	return snto32(value, n);
-}
-EXPORT_SYMBOL_GPL(hid_snto32);
-
 /*
  * Convert a signed 32-bit integer to a signed n-bit integer.
  */
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 8741d95..d97f232 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -192,6 +192,7 @@ static int hidinput_setkeycode(struct input_dev *dev,
 	return -EINVAL;
 }
 
+
 /**
  * hidinput_calc_abs_res - calculate an absolute axis resolution
  * @field: the HID report field to calculate resolution for
@@ -234,23 +235,17 @@ __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
 	case ABS_MT_TOOL_Y:
 	case ABS_MT_TOUCH_MAJOR:
 	case ABS_MT_TOUCH_MINOR:
-		if (field->unit & 0xffffff00)		/* Not a length */
-			return 0;
-		unit_exponent += hid_snto32(field->unit >> 4, 4) - 1;
-		switch (field->unit & 0xf) {
-		case 0x1:				/* If centimeters */
+		if (field->unit == 0x11) {		/* If centimeters */
 			/* Convert to millimeters */
 			unit_exponent += 1;
-			break;
-		case 0x3:				/* If inches */
+		} else if (field->unit == 0x13) {	/* If inches */
 			/* Convert to millimeters */
 			prev = physical_extents;
 			physical_extents *= 254;
 			if (physical_extents < prev)
 				return 0;
 			unit_exponent -= 1;
-			break;
-		default:
+		} else {
 			return 0;
 		}
 		break;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 31b9d29..96f3432 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -766,7 +766,6 @@ int hid_connect(struct hid_device *hid, unsigned int connect_mask);
 void hid_disconnect(struct hid_device *hid);
 const struct hid_device_id *hid_match_id(struct hid_device *hdev,
 					 const struct hid_device_id *id);
-s32 hid_snto32(__u32 value, unsigned n);
 
 /**
  * hid_device_io_start - enable HID input during probe, remove
-- 
1.8.4.rc3

[PATCH 1/1 FROM FIXED] Revert "HID: fix unit exponent parsing"

[Nikolai Kondrashov]

Revert "HID: fix unit exponent parsing" as it is based on incorrect
understanding of the HID specification and breaks resolution
calculation at least on graphics tablets.

There are two "unit exponents" in HID specification and it is important
not to mix them. One is the global "Unit Exponent" item and another is
nibble values in the global "Unit" item. See 6.2.2.7 Global Items.

The "Unit Exponent" value is just a signed integer and is used to scale
the integer resolution unit values, so fractions can be expressed.

The nibbles of "Unit" value are used to select the unit system (nibble
0), and presence of a particular basic unit type in the unit formula and
its *exponent* (or power, nibbles 1-6). And yes, the latter is in two
complement and zero means absence of the unit type.

Taking the representation example of (integer) joules from the
specification:

    [mass(grams)][length(centimeters)^2][time(seconds)^-2] * 10^-7

the "Unit Exponent" would be -7 (or 0xF9, if stored as a byte) and the
"Unit" value would be 0xE121, signifying:

    Nibble  Part        Value   Meaning
    -----   ----        -----   -------
    0       System      1       SI Linear
    1       Length      2       Centimeters^2
    2       Mass        1       Grams
    3       Time        -2      Seconds^-2

To give the resolution in e.g. hundredth of joules the "Unit Exponent"
item value should have been -9.

See also the examples of "Unit" values for some common units in the same
chapter.

This reverts commit 774638386826621c984ab6994439f474709cac5e.
---
This patch has correct e-mail in "From".

 drivers/hid/hid-core.c  | 16 +---------------
 drivers/hid/hid-input.c | 13 ++++---------
 include/linux/hid.h     |  1 -
 3 files changed, 5 insertions(+), 25 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index b8470b1..94350b7 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -319,7 +319,6 @@ static s32 item_sdata(struct hid_item *item)
 
 static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
 {
-	__u32 raw_value;
 	switch (item->tag) {
 	case HID_GLOBAL_ITEM_TAG_PUSH:
 
@@ -370,14 +369,7 @@ static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_UNIT_EXPONENT:
-		/* Units exponent negative numbers are given through a
-		 * two's complement.
-		 * See "6.2.2.7 Global Items" for more information. */
-		raw_value = item_udata(item);
-		if (!(raw_value & 0xfffffff0))
-			parser->global.unit_exponent = hid_snto32(raw_value, 4);
-		else
-			parser->global.unit_exponent = raw_value;
+		parser->global.unit_exponent = item_sdata(item);
 		return 0;
 
 	case HID_GLOBAL_ITEM_TAG_UNIT:
@@ -983,12 +975,6 @@ static s32 snto32(__u32 value, unsigned n)
 	return value & (1 << (n - 1)) ? value | (-1 << n) : value;
 }
 
-s32 hid_snto32(__u32 value, unsigned n)
-{
-	return snto32(value, n);
-}
-EXPORT_SYMBOL_GPL(hid_snto32);
-
 /*
  * Convert a signed 32-bit integer to a signed n-bit integer.
  */
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 8741d95..d97f232 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -192,6 +192,7 @@ static int hidinput_setkeycode(struct input_dev *dev,
 	return -EINVAL;
 }
 
+
 /**
  * hidinput_calc_abs_res - calculate an absolute axis resolution
  * @field: the HID report field to calculate resolution for
@@ -234,23 +235,17 @@ __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
 	case ABS_MT_TOOL_Y:
 	case ABS_MT_TOUCH_MAJOR:
 	case ABS_MT_TOUCH_MINOR:
-		if (field->unit & 0xffffff00)		/* Not a length */
-			return 0;
-		unit_exponent += hid_snto32(field->unit >> 4, 4) - 1;
-		switch (field->unit & 0xf) {
-		case 0x1:				/* If centimeters */
+		if (field->unit == 0x11) {		/* If centimeters */
 			/* Convert to millimeters */
 			unit_exponent += 1;
-			break;
-		case 0x3:				/* If inches */
+		} else if (field->unit == 0x13) {	/* If inches */
 			/* Convert to millimeters */
 			prev = physical_extents;
 			physical_extents *= 254;
 			if (physical_extents < prev)
 				return 0;
 			unit_exponent -= 1;
-			break;
-		default:
+		} else {
 			return 0;
 		}
 		break;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 31b9d29..96f3432 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -766,7 +766,6 @@ int hid_connect(struct hid_device *hid, unsigned int connect_mask);
 void hid_disconnect(struct hid_device *hid);
 const struct hid_device_id *hid_match_id(struct hid_device *hdev,
 					 const struct hid_device_id *id);
-s32 hid_snto32(__u32 value, unsigned n);
 
 /**
  * hid_device_io_start - enable HID input during probe, remove
-- 
1.8.4.rc3

Re: [PATCH 1/1 FROM FIXED] Revert "HID: fix unit exponent parsing"

[Nikolai Kondrashov]

On 10/08/2013 10:05 PM, Nikolai Kondrashov wrote:
> Revert "HID: fix unit exponent parsing" as it is based on incorrect
> understanding of the HID specification and breaks resolution
> calculation at least on graphics tablets.
>
> There are two "unit exponents" in HID specification and it is important
> not to mix them. One is the global "Unit Exponent" item and another is
> nibble values in the global "Unit" item. See 6.2.2.7 Global Items.
>
> The "Unit Exponent" value is just a signed integer and is used to scale
> the integer resolution unit values, so fractions can be expressed.

I see now that the official "HID Descriptor Tool" writes the "Unit Exponent"
as a two's complement nibble and Microsoft cites binary representations as
such (although they seem to be made with that tool).

This likely means most report descriptors in the wild store it as such. Which
probably means that it is more practical to expect them to be so, but I don't
have any statistically-significant data.

However, I still think that this interpretation of the standard is incorrect
(the standard is vague in this part). First, the specification says about
"Unit Exponent":

     Value of the unit exponent in base 10. See the
     table later in this section for more information.

I.e. nothing about nibbles. It refers to a table, but doesn't say which one.
However, the first table appearing later is a mouse example mentioning an
exponent.

The specification also mentions the one-byte prefix for the item to be
"0101 01 nn", where "nn" is the size value, which would be pointless to have
anything different from 1 byte, if it was limited to a nibble.

And lastly and most importantly there is no point in limiting the "Unit
Exponent" to a nibble where you have a full 32-bit signed integer available,
thus complicating encoding/decoding and limiting the possible reported unit
scale.

 From what data I have on various non-Wacom graphics tablets, most of the older
ones provide incorrect "unit" item value, so "unit exponent" doesn't apply.
However, some recent ones, such as made by Huion, do have correct units,
and nibble-stored "unit exponent". And I was wondering where that strange data
came from. Sigh...

I'll try to reach usb.org for comments on this.

Meanwhile, can I suggest a hybrid approach? As positive unit exponent beyond 7
is unlikely to appear for axes which have their resolution calculated
currently, can we assume anything higher than 7 to be nibble-stored negative
exponents and anything else normal signed integers?

Sincerely,
Nick

Re: [PATCH 1/1 FROM FIXED] Revert "HID: fix unit exponent parsing"

[Benjamin Tissoires]

Hi Nikolai,

first, I wouldn't revert the patch as this, because I think the patch
also fixes the nibbles parsing in unit:
- the part regarding the unit exponent seems blurry
- the part regarding the unit exponent seems correct to me.

On Wed, Oct 9, 2013 at 9:37 AM, Nikolai Kondrashov <spbnick@gmail.com> wrote:
> On 10/08/2013 10:05 PM, Nikolai Kondrashov wrote:
>>
>> Revert "HID: fix unit exponent parsing" as it is based on incorrect
>> understanding of the HID specification and breaks resolution
>> calculation at least on graphics tablets.

I have re-read the HID spec, and indeed, it is not clear at all.
However, as you mentioned later in this mail, I based this assumption
by looking at the report descriptors I have in hand for hid-multitouch
(which is still quite a lot), and it looked correct this way.

>>
>> There are two "unit exponents" in HID specification and it is important
>> not to mix them. One is the global "Unit Exponent" item and another is
>> nibble values in the global "Unit" item. See 6.2.2.7 Global Items.
>>
>> The "Unit Exponent" value is just a signed integer and is used to scale
>> the integer resolution unit values, so fractions can be expressed.
>
>
> I see now that the official "HID Descriptor Tool" writes the "Unit Exponent"
> as a two's complement nibble and Microsoft cites binary representations as
> such (although they seem to be made with that tool).
>
> This likely means most report descriptors in the wild store it as such.
> Which
> probably means that it is more practical to expect them to be so, but I
> don't
> have any statistically-significant data.
>
> However, I still think that this interpretation of the standard is incorrect
> (the standard is vague in this part). First, the specification says about
> "Unit Exponent":
>
>     Value of the unit exponent in base 10. See the
>     table later in this section for more information.
>
> I.e. nothing about nibbles. It refers to a table, but doesn't say which one.
> However, the first table appearing later is a mouse example mentioning an
> exponent.

>From what I read, the unit exponent has nothing to do with nibbles.
Only "Unit" has them.
However, my experience says that the unit exponent is often used as a
two's complement :(

>
> The specification also mentions the one-byte prefix for the item to be
> "0101 01 nn", where "nn" is the size value, which would be pointless to have
> anything different from 1 byte, if it was limited to a nibble.

It could be, as it allows the firmware maker to avoid to add padding
bits... Which seems a convenient way to do it.

>
> And lastly and most importantly there is no point in limiting the "Unit
> Exponent" to a nibble where you have a full 32-bit signed integer available,
> thus complicating encoding/decoding and limiting the possible reported unit
> scale.

sure.

>
> From what data I have on various non-Wacom graphics tablets, most of the
> older
> ones provide incorrect "unit" item value, so "unit exponent" doesn't apply.

Yes. The "correct" specification of unit and unit exponent has only be
a requirements since Windows 8 [1]. (Note, there used to be a pdf [2],
which I found more convenient to read, but still...).

So definitely, Microsoft considers that the unit exponent is a 4 bits
two's complement, otherwise, the firmware will not get a Windows 8
certification.

> However, some recent ones, such as made by Huion, do have correct units,
> and nibble-stored "unit exponent". And I was wondering where that strange
> data
> came from. Sigh...

See above... Microsoft certification.

>
> I'll try to reach usb.org for comments on this.
>
> Meanwhile, can I suggest a hybrid approach? As positive unit exponent beyond
> 7
> is unlikely to appear for axes which have their resolution calculated
> currently, can we assume anything higher than 7 to be nibble-stored negative
> exponents and anything else normal signed integers?

I would say that the current approach (without the revert) is exactly this:
- if the data is stored on only 1 byte ( if (!(raw_value &
0xfffffff0))), do the two's complement -> any value less than 7 will
be the same, above are considered as negative.
- if not, then use the raw value.

Could you please share some of the report descriptors which you found
problematic, so that I can have a better understanding of the problem?
If you want to have a look at some multitouch descriptors (and few
other devices), I started to build a database of hid devices[3].

Cheers,
Benjamin

[1] http://msdn.microsoft.com/en-us/library/windows/hardware/dn383621.aspx
[2] http://feishare.com/attachments/article/299/windows-pointer-device-protocol.pdf
[3] https://github.com/bentiss/hid-devices

PS: I'll be mostly offline until the 1st of November when I'll finish
my transfer to the RH Westford Office.
PPS: my nick on the RH internal IRC is bentiss :)

Re: [PATCH 1/1 FROM FIXED] Revert "HID: fix unit exponent parsing"

[Nikolai Kondrashov]

Hi Benjamin,

On 10/09/2013 12:02 PM, Benjamin Tissoires wrote:
> first, I wouldn't revert the patch as this, because I think the patch
> also fixes the nibbles parsing in unit:
> - the part regarding the unit exponent seems blurry
> - the part regarding the unit exponent seems correct to me.

This statement is sure confusing :)

> On Wed, Oct 9, 2013 at 9:37 AM, Nikolai Kondrashov<spbnick@gmail.com>  wrote:
>> On 10/08/2013 10:05 PM, Nikolai Kondrashov wrote:
>>  From what data I have on various non-Wacom graphics tablets, most of the
>>  older ones provide incorrect "unit" item value, so "unit exponent" doesn't
>>  apply.
>
> Yes. The "correct" specification of unit and unit exponent has only be
> a requirements since Windows 8 [1]. (Note, there used to be a pdf [2],
> which I found more convenient to read, but still...).

I've stumbled on the very same document yesterday.

> So definitely, Microsoft considers that the unit exponent is a 4 bits
> two's complement, otherwise, the firmware will not get a Windows 8
> certification.

I think this is due to their use of the "HID Descriptor Tool". Maybe
assuming it is a reference implementation, which it is not, but more likely
just not noticing the discrepancy.

I'll try to reach Microsoft on this and maybe make them correct their
specification.

>> Meanwhile, can I suggest a hybrid approach? As positive unit exponent
>> beyond 7 is unlikely to appear for axes which have their resolution
>> calculated currently, can we assume anything higher than 7 to be
>> nibble-stored negative exponents and anything else normal signed integers?
>
> I would say that the current approach (without the revert) is exactly this:
> - if the data is stored on only 1 byte ( if (!(raw_value&
> 0xfffffff0))), do the two's complement ->  any value less than 7 will
> be the same, above are considered as negative.
> - if not, then use the raw value.

It is not exactly what I suggested. It also considers anything above 15 to be
a normal integer. However, it might be a cleaner way.

All-in-all, I'd say that the relevant hid-core.c code should have its comment
fixed, and the hid-input.c (hidinput_calc_abs_res) change needs to be reverted
as it (incorrectly) takes the component unit power into account for resolution
calculation and makes the "unit" item value handling harder to comprehend.

I'll prepare and test a patch and we can carry on from there.

> Could you please share some of the report descriptors which you found
> problematic, so that I can have a better understanding of the problem?
> If you want to have a look at some multitouch descriptors (and few
> other devices), I started to build a database of hid devices[3].

I have my repository of tablet diagnostics published at
https://github.com/DIGImend/devices

The original report descriptors are in */rd/original*.txt files and their XML
representation is in */rd/original*.xml files. Most (if not all) of them have
the "unit exponent" as nibble.

The XML representation was created by my "hidrd-convert" tool [1], which can
also output binary, specification example format and code from either binary
or XML input.

I use it to generate fixed report descriptors for graphics tablets. Maybe you
can find it useful as well. However, it interprets the "unit exponent" per
specification, i.e. just as an integer.

> [1] http://msdn.microsoft.com/en-us/library/windows/hardware/dn383621.aspx
> [2] http://feishare.com/attachments/article/299/windows-pointer-device-protocol.pdf
> [3] https://github.com/bentiss/hid-devices
>
> PS: I'll be mostly offline until the 1st of November when I'll finish
> my transfer to the RH Westford Office.
> PPS: my nick on the RH internal IRC is bentiss :)

My nick there is "nkondrashov". However, I would prefer to keep this
discussion on the mailing list.

Good luck in the US!

Sincerely,
Nick

[1] https://sf.net/apps/mediawiki/digimend?title=Hidrd

Re: [PATCH 1/1 FROM FIXED] Revert "HID: fix unit exponent parsing"

[Nikolai Kondrashov]

On 10/09/2013 09:42 PM, Nikolai Kondrashov wrote:
>> I would say that the current approach (without the revert) is exactly this:
>> - if the data is stored on only 1 byte ( if (!(raw_value&
>> 0xfffffff0))), do the two's complement -> any value less than 7 will
>> be the same, above are considered as negative.
>> - if not, then use the raw value.
>
> It is not exactly what I suggested. It also considers anything above 15 to be
> a normal integer. However, it might be a cleaner way.
>
> All-in-all, I'd say that the relevant hid-core.c code should have its comment
> fixed, and the hid-input.c (hidinput_calc_abs_res) change needs to be reverted
> as it (incorrectly) takes the component unit power into account for resolution
> calculation and makes the "unit" item value handling harder to comprehend.

On a second glance at the test data, hid-core.c needs to be fixed as well, as
for the non-nibble case it loses the sign by reading the item value as
unsigned. So a perfectly valid 1 byte 0xFD value becomes 253, instead of -3.

I'll include that into the patch.

Sincerely,
Nick

Re: [PATCH 1/1 FROM FIXED] Revert "HID: fix unit exponent parsing"

[Nikolai Kondrashov]

On 10/09/2013 09:42 PM, Nikolai Kondrashov wrote:
>> So definitely, Microsoft considers that the unit exponent is a 4 bits
>> two's complement, otherwise, the firmware will not get a Windows 8
>> certification.
>
> I think this is due to their use of the "HID Descriptor Tool". Maybe
> assuming it is a reference implementation, which it is not, but more likely
> just not noticing the discrepancy.
>
> I'll try to reach Microsoft on this and maybe make them correct their
> specification.

I've started my attempts with a post on Microsoft's official hardware
development forums:

http://social.msdn.microsoft.com/Forums/windowshardware/en-US/e87d0db1-486e-42ae-bf95-d1ac5ffc0b02/unit-exponent-item-value-encoding-in-hid-report-descriptors?forum=whck

Let's see how it goes.

Sincerely,
Nick