@jimklimov: Looking at the code that uses hid_rep_index, hid_desc_index, and hid_ep_in|out in usb_subdriver (all in nut_libusb.h) I don't see any use of the addvars mechanism. If I follow the examples that set and use those struct members, it looks like everything relies on them being initialized to 0 by default, then only set in subdrivers or situations that need something different.
If I have this right, I'll just follow that example, test, and submit a PR. If not, let me know what I'm missing. On Sun, Nov 5, 2023 at 3:26 PM Kelly Byrd <[email protected]> wrote: > Thanks for the quick reply! Ya, I'm new to all this so I didn't know this > either, I was vaguely aware of USB composite devices from other projects, > but never how it was all enumerated. > > I dug a bit more on this and I'm convinced the relationship is this: > Device > └── Configuration 1 > ├── Interface 1 > │ ├── Endpoint 1 > │ ├── Endpoint 2 > │ └── ... > ├── Interface 2 > │ └── ... > └── ... > └── Configuration 2 > ├── Interface 1 > │ ├── Endpoint 1 > │ └── ... > └── ... > > I got this from pages like this > https://wiki.osdev.org/Universal_Serial_Bus, which says: > All USB devices, or functions, have at least one configuration, and every > configuration has at least one interface. An interface may define zero or > more endpoints. > > Reading about multiple configurations, they appear to be intended for > things like a USB device operating in high-speed or low-speed mode, or a > debug vs normal mode, or maybe high vs low power mode. Given this, > hid_rep_index (which libusb1.h defines as "number of the interface we use > in a composite USB device") is absolutely not the right thing to pass to > libusb_get_config_descriptor(), because the number of configurations are > not related to the index of the interface being used. Passing 0 would be > better than the current code, because 0 is what happens with not-composite > devices (most UPS devices) and I bet even among the composite ones, there's > only one config descriptor on the device > > I'll look at doing changes you mentioned. I've looked only briefly at the > "addvars" code, so I don't know if it's possible to have a default without > requiring subdriver authors to worry about this. Most won't care and we > shouldn't make them. It should just be there as an option in case someone > needs to support a really unusual device. Looking at the codebase and list > archives, you all seem to get a lot of those on this project :-) > > On Sun, Nov 5, 2023 at 3:01 PM Jim Klimov <[email protected]> wrote: > >> Thank you for the investigation! >> >> I've thought about this in vague terms, i.e. that numbers like this >> should be configurable, but did not have time to read into USB-related >> libraries and specs to make any more specific sense of it (and the libusb >> code is not mine except the recent layers of cleanup). I guess I did not >> even realize that there is more than the regularly mentioned "interface >> number" to juggle here. >> >> So if you manage to reliably pinpoint which interface concept applies >> where, and add configurability to that with `addvar()` in >> `drivers/libusb1.c` (and `drivers/libusb0.c` if applicable/possible there) >> and `docs/man/nut_usb_addvars.txt` -- so this change applies consistently >> to all USB-aware drivers, and if it all works - that would be a welcome >> improvement :D >> >> Jim >> >> >> On Sun, Nov 5, 2023 at 11:29 PM Kelly Byrd <[email protected]> wrote: >> >>> Hi all, >>> I posted originally here last week: >>> >>> https://alioth-lists.debian.net/pipermail/nut-upsuser/2023-October/013461.html >>> >>> Since then, I've been building and testing from source following the >>> instructions a helpful reply in the above thread pointed out. The original >>> problem is I've got one of these arduino HID power devices, as a project >>> for a DIY UPS at home. This arduino is a composite USB device, CDC is >>> on interface one, HID Power on Interface 2. According to the NUT source >>> tree, an arduino HID subdriver came in a couple of years ago (e07bec1), so >>> this should "just work" ;-) But it doesn't for me. >>> >>> I traced what I think the problem is to this section in libusb1.c >>> diff --git a/drivers/libusb1.c b/drivers/libusb1.c >>> index 17f4b8f74..f49cc78aa 100644 >>> --- a/drivers/libusb1.c >>> +++ b/drivers/libusb1.c >>> @@ -420,7 +420,7 @@ static int nut_libusb_open(libusb_device_handle >>> **udevp, >>> >>> upsdebugx(2, "Reading first configuration descriptor"); >>> ret = libusb_get_config_descriptor(device, >>> - (uint8_t)usb_subdriver.hid_rep_index, >>> + 0, //(uint8_t)usb_subdriver.hid_rep_index, >>> &conf_desc); >>> /*ret = libusb_get_active_config_descriptor(device, >>> &conf_desc);*/ >>> >>> When running from commit ab55bc0 on master, I end up with >>> usb_subdriver.hid_rep_index = 2 and libusb_get_config_descriptor >>> returns is -5 (Entity not found), conf_desc is not filled in and then it >>> ends up in this code section: >>> if (!conf_desc) { /* ?? this should never happen */ >>> upsdebugx(2, " Couldn't retrieve descriptors"); >>> goto next_device; >>> } >>> >>> With my hacked in test code to force 0 as the config_index the >>> usbhid-ups driver (and the arduino subdriver) appear happy, I see reports, >>> I see state changes for my AC Present, etc. >>> >>> From my brief reading about USB it appears to me that the "tree" of >>> descriptors is like this: >>> - A device descriptor has one or more configuration descriptors >>> (bNumConfigurations in libusb_device_descriptor) >>> - Each configuration descriptor has one or more interface descriptors >>> (bNumInterfaces in libusb_config_descriptor) >>> - Each interface descriptor has one or more endpoint descriptors >>> >>> I've done just enough reading to think that while it is rare to have >>> more than one interface, it is even more rare to have more than one >>> configuration. A multi-interface composite device will often still have one >>> configuration (index 0), and which configuration is correct has nothing to >>> do with using an alternate interface on these composite USB devices. I've >>> seen reference to multiple configurations being rare and for things like a >>> high power vs low power mode, but this is out of my depth. In any case, it >>> appears to me that calling libusb_get_config_descriptor(device, 0, >>> &conf_desc) should work for nearly everyone. >>> >>> I'm happy to put a PR together for this simple change (looking at the >>> code, it feels like it should always be calling the commented out >>> libusb_get_active_config_descriptor, but I'm not sure why that is commented >>> out). I don't know enough about USB to know how NUT would know which config >>> to choose if there were several, just that it appears to me that >>> configurations own interfaces, not the other way around. >>> >>> It looks like the most flexible thing would be to create a new member in >>> usb_communication_subdriver_t, maybe called usb_config_index (I don't know >>> if config descriptors are HID specific?) This member would be a lot >>> like hid_desc_index. It would default to zero but be available for those >>> USB subdrivers that needed something other than the default. >>> >>> Please let me know if I'm on the right track! In the meantime, I can get >>> my DIY project running for my specific device. >>> >>> >>> _______________________________________________ >>> Nut-upsdev mailing list >>> [email protected] >>> https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/nut-upsdev >>> >>
_______________________________________________ Nut-upsdev mailing list [email protected] https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/nut-upsdev
