Revise magic numbers in USB codebase

[jimklimov]

Reflecting a helpful answering comment, originally posted by @marcan in #300 (comment)

Question to people who dealt with non-zero USB interface numbers (CC: @abratchik, @zykh, @clepple, @aquette OTOH) and LibUSB in general: we have several drivers and libusbN.c code which follows the same pattern, to deal with USB in master branch or these libusb-1.0 related branches, and part of that pattern had a number of operations dealing with either a literal 0x81 argument, or some macro (with version-dependent name) resolving to 0x80 and either adding or bitwise-OR'ing a 1 as a number.

If you see 0x80, that's an IN endpoint number, not an interface number.

What the right way to deal with endpoint numbers is depends on the devices involved. Often you can get away with hardcoding endpoint numbers if they never change between device variants. Sometimes it's better to look at the descriptors and identify the endpoints by their position inside an interface, which itself can be identified by index or by its class or in some other way.

usb_clear_halt(dev_h, 0x81)

That's IN endpoint 1.

ret = usb_interrupt_read(udev, 0x81, (usb_ctrl_char)tmp, 8, 1000);

Same, IN endpoint 1 (which must be of interrupt type in this case). The 8 later is the buffer length and 1000 is a timeout.

ep = 0x81 | USB_ENDPOINT_IN

This is actually wrong; USB_ENDPOINT_IN should be equal to 0x80. It should be 1 | USB_ENDPOINT_IN to denote IN endpoint 1.

(note that IN 1 (0x81) and OUT 1 (0x01) are distinct, separate endpoints)

ret = usb_interrupt_read(udev, USB_ENDPOINT_IN | 1, (usb_ctrl_char)&buf[i], 8, 1000);

Same story again, IN endpoint 1, length 8, timeout 1000.

ret = libusb_interrupt_transfer(udev,
	LIBUSB_ENDPOINT_IN + usb_subdriver.hid_ep_in,
	(unsigned char *)buf, bufsize, &bufsize, timeout);

In this case the HID input endpoint is, presumably, dynamically determined based on descriptors or some other way, so it comes from a structure, but it presumably lacks the IN bit so that gets added here (ORed would be more appropriate though).

res = usb_control_msg(udev,
                                USB_ENDPOINT_IN + 1, // <<< this
                                USB_REQ_GET_DESCRIPTOR,
                                (USB_DT_HID << 8) + usb_subdriver.hid_desc_index,
                                usb_subdriver.hid_rep_index,
                                buf, 0x9, USB_TIMEOUT);

               /* in libusb0 was: USB_ENDPOINT_IN + 1 */
               res = libusb_control_transfer(udev,
                       LIBUSB_ENDPOINT_IN|LIBUSB_REQUEST_TYPE_STANDARD|LIBUSB_RECIPIENT_INTERFACE,
                       LIBUSB_REQUEST_GET_DESCRIPTOR,
                       (LIBUSB_DT_HID << 8) + usb_subdriver.hid_desc_index,
                       usb_subdriver.hid_rep_index,
                       buf, 0x9, USB_TIMEOUT);

I'm pretty sure the libusb0 code is written incorrectly (but works by accident). The prototype for that function is:

int usb_control_msg(usb_dev_handle *dev, int requesttype, int request,
	int value, int index, char *bytes, int size, int timeout);

So the first parameter is the request type. But USB_ENDPOINT_IN + 1 reads like it's trying to specify that the control request should be sent to IN endpoint 1, which doesn't make much sense (sending control requests to non-zero endpoints is exceedingly rare and not even supported by this API), and isn't what that parameter does. However, if you evaluate LIBUSB_ENDPOINT_IN|LIBUSB_REQUEST_TYPE_STANDARD|LIBUSB_RECIPIENT_INTERFACE, you'll see it is 0x81 indeed (which does not mean IN 1 here, rather it's a bitfield describing the control request type). So in the end both versions do the same thing, but the libusb0 one is written in a confusing way and the author may not have understood it properly.

and examples above also show a magical 0x9 that is sometimes 0x8 in other calls.

That's a length.

My question to libusb experts is: can (and should) these magical numbers (and mathematical additions) be replaced with "proper" named macros, bit-flag operations, etc. to reduce confusion and simplify maintenance? If the integer values are multi-purpose (e.g. flags in high bits, interface or endpoint number in lower bits), the variable "hid_ep_in" should be range-checked, and/or some union type used?

The 0x81 stuff can be replaced with LIBUSB_ENDPOINT_IN|1 in most cases. That said, endpoint numbers are such a fundamental part of USB that I think anyone working on this stuff probably understands that 0x81 means IN 1 and 0x01 means OUT 1. It would be more useful to have a define specific to each driver, e.g. something like:

#define FOODEV_EP_CMD (LIBUSB_ENDPOINT_IN | 1)
#define FOODEV_EP_REPLY (LIBUSB_ENDPOINT_OUT | 1)

Which makes it clear that these are hardcoded endpoint numbers for that given device.

Addition should almost always be a logical OR instead; it doesn't make a difference for combining bitfields, but addition will break if e.g. one component already had the IN bit set, and OR is more idiomatic.

Otherwise it is quite annoying to not know if e.g. two branches of same or similar-looking logic do same things or not, and whether that is intentional or just historic/other discrepancy (in short: should I overwrite one with another? Looking at code, I have no idea nor clues to nudge me in the right direction so far).

And also, since there were several efforts in these libusb-1.x branches and otherwise (e.g. in PR #1044) to avoid the hardcoded USB interface number zero in favor of configurable use of other values, I can't say now if all of NUT codebase magically benefits from existence of this configuration, or if some code continues to interact with "number zero". Likewise, I do not really know if the "number zero" is somehow magical for USB (e.g. maybe some mgmt operations must be done through it, and data operations may be at other numbers).

Endpoint zero is magical and used for control transfers almost always (and nothing else). For simple devices with only two data endpoints, those are often going to be either IN 1 (0x81) and OUT 1 (0x01), or something like IN 1 (0x81) and OUT 2 (0x02) (some devices prefer to use separate endpoint numbers for IN/OUT; this could be a USB device controller hardware limitation, or just caused by confused embedded engineers thinking they can't do that even though it's fully allowed). For a device with one interface, they would be part of interface 0, and you can usually get away with hardcoding them in that case. Devices with multiple interfaces though could end up having endpoint numbers change between device revisions / hardware; in that case it's a better idea to get the endpoint numbers from the interface descriptor.

HID is a particularly special case, because it's a standard and often implemented alongside other things. Since many devices implement HID, you can't hardcode interface or endpoint numbers. The correct way to do this is to look through all interface descriptors and find the one that claims to have the HID class; then use that interface number and get the endpoint descriptors from that interface descriptor. That way you don't have to hardcode anything for specific devices.

UPDATE: Partially related to #2149 subject area and USB non-zero interface numbers Most UPSes serve USB interactions on interface 0 which is default. Recent "composite devices" differ labelled issues.