rtu-usb: claim the HID interface, send full-length reports, allow disabling reset - #12
Open
d01 wants to merge 1 commit into
Open
rtu-usb: claim the HID interface, send full-length reports, allow disabling reset#12d01 wants to merge 1 commit into
d01 wants to merge 1 commit into
Conversation
…abling reset Found while getting NUT's apc_modbus working with an APC Smart-UPS X1500 (051d:0003, firmware "UPS 16.0"). Two bugs, plus an option needed because the existing device reset is fatal on this hardware. 1. Claim the HID interface before doing I/O on it. _usb_get_hid_descriptor() claims the interface only long enough to read the report descriptor and then releases it, so the interrupt transfers that follow run against an interface still owned by the kernel HID driver. The OUT transfer is not delivered and the device's input reports go to usbhid rather than to us. The interface is now held for the life of the connection and released in _modbus_rtu_usb_close(). 2. Send full-length, zero-padded OUT reports. The report descriptor declares the output report as 63 bytes. A short transfer (report id plus only the Modbus bytes -- 7 bytes for a 6-byte request) is accepted by the host controller but ignored by this firmware, and leaves the device's Modbus engine unresponsive until the USB cable is physically reseated. The interrupt OUT transfer also used timeout 0, i.e. wait forever, so an unresponsive endpoint blocked the caller indefinitely instead of returning an error. Now a finite timeout. 3. Make the reset on open optional (modbus_rtu_usb_set_reset_on_open). Default is unchanged -- the reset still happens -- because it is there for a reason: it recovers devices whose framing has desynchronised and which return stale data from earlier requests. On the Smart-UPS X1500 it is fatal. After the reset the device never services its interrupt OUT endpoint again, every register read times out, and only physically reseating the USB cable restores it. A sysfs-level re-enumeration reproduces the same state; neither idle time nor draining the endpoint recovers it. Worth noting the reset runs for every device on the bus during enumeration, before the match callback is consulted. So rather than removing it, this adds a way to turn it off. Whether the default should change, or whether it would be better driven by a quirk or by detecting the desync it is meant to fix, is a judgement call for someone with visibility across more hardware than I have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d01
force-pushed
the
upstream-submission
branch
from
August 9, 2026 21:15
fedd0ad to
4553446
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updated after reading networkupstools/nut#2609 properly — see the note at the end. The original version of this PR removed the device reset outright, which was wrong of me.
Two bugs in the
rtu_usbbackend, plus an option needed because the existingreset on open is fatal on one device. Found getting NUT's
apc_modbusworkingwith an APC Smart-UPS X1500 (051d:0003, firmware "UPS 16.0").
1. The HID interface is never claimed for I/O
_usb_get_hid_descriptor()claims the interface just long enough to read thereport descriptor and then releases it. On a match,
_modbus_rtu_usb_connect()stores the handle and breaks without claiming anything, so every subsequent
interrupt transfer runs against an interface still owned by the kernel HID
driver: the OUT transfer is not delivered, and the device's input reports go to
usbhidrather than to us.Now claimed for the life of the connection and released in
_modbus_rtu_usb_close().2. Short interrupt OUT transfers are ignored
The report descriptor declares output report
0x90as 63 bytes, but the sendpath transfers
payload_chunk_len + 1— 7 bytes for a 6-byte request. Thisfirmware accepts the short transfer at the host-controller level and then
ignores it: no reply arrives, and the Modbus engine is left unresponsive until
the USB cable is reseated. Sending the full declared length, zero padded, makes
the identical request work.
The OUT transfer also passed timeout
0(wait forever), so an unresponsiveendpoint blocked the caller indefinitely rather than returning an error — which
is how this bug presented, and made it much harder to find. Now finite.
3.
modbus_rtu_usb_set_reset_on_open()— new, default unchangedThe reset in
_modbus_rtu_usb_connect()is fatal on the Smart-UPS X1500. Afterit, the device never services its interrupt OUT endpoint again, every register
read times out, and only physically reseating the USB cable recovers it. A
sysfs-level re-enumeration reproduces the same state; neither idle time nor
draining the endpoint helps. It also runs for every device on the bus before
the match callback is consulted.
I originally submitted this as "remove the reset". That was wrong: @EchterAgo
added it deliberately in #11, and explained why in
networkupstools/nut#2609 — it recovers devices whose framing has
desynchronised and which return stale data from earlier requests. I should have
read that thread before proposing to revert it, and I apologise for the noise.
So this keeps the reset on by default and adds a way to switch it off, rather
than changing behaviour for anyone else. Whether the default should change, or
whether this would be better as a device quirk or driven by detecting the
desync it targets, is a judgement call for someone with visibility across more
hardware than I have — happy to rework it whichever way you prefer.
Testing
Tested with NUT 2.8.4's
apc_modbus, statically linked against this branch,against an APC Smart-UPS X1500. With fixes 1 and 2 and the reset disabled, the
driver starts, reads the inventory block and polls cleanly — 5 minutes, zero
read failures, at the driver's stock 35 ms interframe delay. With either fix
missing, the inventory read at 516:636 fails immediately and the driver exits
with "Can't read inventory information from the UPS".
I have only this one model, so fixes 1 and 2 would benefit from review by
anyone with other
rtu_usbhardware. The reset option should be behaviour-neutral by construction, since the default path is unchanged.
Note on authorship
Developed with assistance from Claude (Anthropic); the commit carries a
Co-Authored-Bytrailer. All findings were verified on real hardware.