Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add usb_config_index to usb_communication_subdriver_s, default to 0. #2148

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

kellybyrd
Copy link
Contributor

@kellybyrd kellybyrd commented Nov 6, 2023

The tactical goal of this change is to change
ret = libusb_get_config_descriptor(device,
(uint8_t)usb_subdriver.hid_rep_index,
&conf_desc);
to
ret = libusb_get_config_descriptor(device,
(uint8_t)usb_subdriver.usb_config_index,
&conf_desc);

Before this change, libusb1.c did libusb_get_config_descriptor() with a config index equal to the interface number. For composite devices using an interface index > 0, this is usally the wrong choice. Concretely, I'm using an Arduino for a DIY UPS project and these are composite devices with multiple interfaces under the first (and only) config descriptor.

In the USB descriptor heirarchy, deivce descriptors have config descriptors which have interface descriptors. Also, nearly all USB devices have a single configuration (index 0).

In order to do this, I added a new member alongside the existing hid_rep_idex and hid_desc_index. I chose to do this instead of using the add_var method because this member is used in places in very similar ways to how hid_rep_index and hid_desc_index is used. This new member defaults to 0 which covers the majority of USB devices. Any future subdriver is able to use this if a device requires it. For existing subdrives, we'll just use an index of 0.

I also changed some debug logging to print out the config index where the code was already printing the interface index.

General points

  • Described the changes in the PR submission or a separate issue, e.g.
    known published or discovered protocols, applicable hardware (expected
    compatible and actually tested/developed against), limitations, etc.

  • There may be multiple commits in the PR, aligned and commented with
    a functional change. Notably, coding style changes better belong in a
    separate PR, but certainly in a dedicated commit to simplify reviews
    of "real" changes in the other commits. Similarly for typo fixes in
    comments or text documents.

Frequent "underwater rocks" for driver addition/update PRs

  • Revised existing driver families and added a sub-driver if applicable
    (nutdrv_qx, usbhid-ups...) or added a brand new driver in the other
    case.

  • Did not extend obsoleted drivers with new hardware support features
    (notably blazer and other single-device family drivers for Qx protocols,
    except the new nutdrv_qx which should cover them all).

  • For updated existing device drivers, bumped the DRIVER_VERSION macro
    or its equivalent.

  • For USB devices (HID or not), revised that the driver uses unique
    VID/PID combinations, or raised discussions when this is not the case
    (several vendors do use same interface chips for unrelated protocols).

  • For new USB devices, built and committed the changes for the
    scripts/upower/95-upower-hid.hwdb file

  • Proposed NUT data mapping is aligned with existing docs/nut-names.txt
    file. If the device exposes useful data points not listed in the file, the
    experimental.* namespace can be used as documented there, and discussion
    should be raised on the NUT Developers mailing list to standardize the new
    concept.

  • Updated data/driver.list.in if applicable (new tested device info)

Frequent "underwater rocks" for general C code PRs

  • Did not "blindly assume" default integer type sizes and value ranges,
    structure layout and alignment in memory, endianness (layout of bytes and
    bits in memory for multi-byte numeric types), or use of generic int where
    language or libraries dictate the use of size_t (or ssize_t sometimes).
  • Progress and errors are handled with upsdebugx(), upslogx(),
    fatalx() and related methods, not with direct printf() or exit().
    Similarly, NUT helpers are used for error-checked memory allocation and
    string operations (except where customized error handling is needed,
    such as unlocking device ports, etc.)

  • Coding style (including whitespace for indentations) follows precedent
    in the code of the file, and examples/guide in docs/developers.txt file.

  • For newly added files, the Makefile.am recipes were updated and the
    make distcheck target passes.

General documentation updates

  • Updated docs/acknowledgements.txt (for vendor-backed device support)

  • Added or updated manual page information in docs/man/*.txt files
    and corresponding recipe lists in docs/man/Makefile.am for new pages

  • Passed make spellcheck, updated spell-checking dictionary in the
    docs/nut.dict file if needed (did not remove any words -- the make
    rule printout in case of changes suggests how to maintain it).

Additional work may be needed after posting this PR

  • Propose a PR for NUT DDL with detailed device data dumps from tests
    against real hardware (the more models, the better).

  • Address NUT CI farm build failures for the PR: testing on numerous
    platforms and toolkits can expose issues not seen on just one system.

  • Revise suggestions from LGTM.COM analysis about "new issues" with
    the changed codebase.

@AppVeyorBot
Copy link

@jimklimov
Copy link
Member

jimklimov commented Nov 6, 2023

For clarification, the addvar() idea (raised in the mailing list) regards the way for end-users to set a specific number value via configuration variables their ups.conf sections, if for some reason they don't want to default to 0 but to something else.

Such user-friendly integration can be addressed separately (more so if this bites someone); so far it is great to have separate (potentially-)manageable values with recognized semantic difference :)

The tactical goal of this change is to change
                ret = libusb_get_config_descriptor(device,
                        (uint8_t)usb_subdriver.hid_rep_index,
                        &conf_desc);
to
                ret = libusb_get_config_descriptor(device,
                        (uint8_t)usb_subdriver.usb_config_index,
                        &conf_desc);

Before this change, libusb1.c did libusb_get_config_descriptor()
with a config index equal to the interface number. For composite
devices using an interface index > 0, this is usally the wrong
choice. Concretely, I'm using an Arduino for a DIY UPS project and
these are composite devices with multiple interfaces under the
first (and only) config descriptor.

In the USB descriptor heirarchy, deivce descriptors have config
descriptors which have interface descriptors. Also, nearly all
USB devices have a single configuration (index 0).

In order to do this, I added a new member alongside the existing
hid_rep_idex and hid_desc_index. I chose to do this instead of
using the add_var method because this member is used in places
in very similar ways to how hid_rep_index and hid_desc_index is
used. This new member defaults to 0 which covers the majority of
USB devices. Any future subdriver is able to use this if a device
requires it. For existing subdrives, we'll just use an index of 0.

I also changed some debug logging to print out the config index
where the code was already printing the interface index.

Signed-off-by: Kelly Byrd <[email protected]>
@kellybyrd
Copy link
Contributor Author

For clarification, the addvar() idea (raised in the mailing list) regards the way for end-users to set a specific number value via configuration variables their ups.conf sections, if for some reason they don't want to default to 0 but to something else.

Ahh, gotcha! IMO, it's really only useful to have this one be a config var, if the others are too. So that would be
usb_config_index
usb_ctrl_repindex
hid_desc_index
hid_ep_in
hid_ep_out

Such user-friendly integration can be addressed separately (more so if this bites someone); so far it is great to have separate (potentially-)manageable values with recognized semantic difference :)

Cool, I'll leave this PR without that then. Doing the above set of value might let someone handle odd hardware without a code change. I think lsusb can be made to output enough information for a user to set those values in a config.

@AppVeyorBot
Copy link

@jimklimov
Copy link
Member

So, that fault for shellcheck was irrelevant (a github connection issue). Otherwise, CI seems mostly satisfied (not all scenarios finished yet, others green).

@jimklimov jimklimov merged commit e451a97 into networkupstools:master Nov 6, 2023
12 of 13 checks passed
@jimklimov jimklimov added USB USB non-zero interface numbers Most UPSes serve USB interactions on interface 0 which is default. Recent "composite devices" differ labels Nov 6, 2023
@jimklimov jimklimov added this to the 2.8.2 milestone Nov 6, 2023
@jimklimov
Copy link
Member

Thanks! Tested with my USB UPS, still works. Not all zeroes were created equal ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
USB non-zero interface numbers Most UPSes serve USB interactions on interface 0 which is default. Recent "composite devices" differ USB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants