Revert the dropReportIdZero hack.

[jhoenicke]

The hack was never necessary, but just worked around a bug in the Trezor/Keepkey implementation of the hardware wallet. It caused other problems later, see #13 and #36.

The real problem was that Keepkey and the old versions of Trezor required a report_id of 63. But the code used a report_id of 0 and put the 63 as first byte into the data. The documentation of write clearly states that the report_id should not be included in the data.

After this pull request, the keepkey/trezor implementation require a fix to work again, see jhoenicke/multibit-hardware@877f25d.

[jhoenicke]

See also keepkey/multibit-hardware#32

[gary-rowe]

Thanks for providing this PR.

Can I just verify that the findings of #13 and #36 are fully invalidated? While I'd love to drop this zero offset problem, others have noted problems with non-Trezor/KeepKey devices and I'm wondering if it might be a bug in a common USB library used by other devices. If so then it may make sense to keep the optional dropReportIdZero to allow more flexibility in handling this situation.

As an aside, I was under the impression that the Trezor used reportId 0x00 and a max packet size of 64 bytes, of which the first was the length 0x3f which is what lead me down this offset path in the first place. When I tried using other reportIds I kept getting failures.

[jhoenicke]

Trezor used to have a ReportDescriptor from a usb to uart bridge (cp2110), that basically used 63 different report ids, one for each package length. So the report_id coincided with the length (note that every report in HID must have a constant length and the length is never explicitly encoded in the packet). Trezor always checked that the first byte is 63 (it checked the first byte to be '?', which has ascii code 63), so a package using a different report id would just have been ignored.

In trezor/trezor-mcu@a1ba431 I simplified the descriptor and removed the report_ids for the other (unsupported) lengths. However, later in trezor/trezor-mcu@e119656, stick changed the descriptor to match with U2F which doesn't use a report_id but 64 byte payload. This is why the new Trezor 1.3.6 needs report_id 0 and 64 bytes.

I noticed that Linux is lenient with respect to wrong usage, which may explain why people don't set the report_id correctly and then experience problems later under Windows. Also the different API for java and the hidapi C library (which expects the report_id to be the first byte of the array) may have confused people.

Maybe one can also provide a function write(HidDeviceStructure device, byte[] data), that expects the report_id to be in the data and takes no extra parameters; this would also make it easier for people to upgrade if they rely on this bug.

[jhoenicke]

Under Linux if the device has a non-zero report_id, a leading 0 is just ignored. Also if the device has no report_id then a report_id of 0 is implicitly assumed. This means that it will almost always work under Linux, even if your driver is buggy.

From the bug reports, it seems like under Windows 8/10 you can omit the report_id 0 if the device has no report id, while under Windows 7 this causes an error (see #36). But you are not allowed to add a leading zero if the packet starts with a report_id (which caused #13).

[gary-rowe]

@jhoenicke Can we work together to resolve this for Trezor devices in the coming weeks for the 0.6.0 release?

[qoire]

Hi, just wondering if theres any resolution on this? I'm using hid4java (in conjunction with ledger) and I've applied the changes here to success.

[gary-rowe]

I've accepted the changes indicated this PR and added them to the develop branch.