| Message ID | 1381259117-25256-1-git-send-email-spbnick@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
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 -- 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
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 :) -- 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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- 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
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
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(-)