Nut upsdev - Nov 2023 - Passing hid_rep_index to libusb_get_config_descriptor is wrong?

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/dce7d66f/attachment-0001.htm>

@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>