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

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://alioth-lists.debian.net/pipermail/nut-upsdev/attachments/20231105/007d08dd/attachment.htm>

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/20231106/cae739ab/attachment.htm>