[Nut-upsdev] Passing hid_rep_index to libusb_get_config_descriptor is wrong?
Kelly Byrd
kbyrd at memcpy.com
Sun Nov 5 23:59:52 GMT 2023
@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 <kbyrd at memcpy.com> 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 <jimklimov+nut at gmail.com> 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 <kbyrd at memcpy.com> 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
>>> Nut-upsdev at alioth-lists.debian.net
>>> https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/nut-upsdev
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://alioth-lists.debian.net/pipermail/nut-upsdev/attachments/20231105/a5d09686/attachment.htm>
More information about the Nut-upsdev
mailing list