Conversation
usb_set_altinterface() resets the host's data toggle, but the UPS firmware keeps its own, so the first OUT report after opening the device was silently dropped. A reply that arrives after its read timed out also stays queued and makes every later reply answer the previous request, which corrupts a configuration read. Drop the altinterface call and drain the endpoint on open.
A page reply that answers an earlier request was stored at the address that was asked for, so one late reply shifted the whole configuration. Check the address the device echoes, retry the page, and record whether every page was read.
-i overlays the file on the settings read from the device, so with -s (which skips the read) it wrote zeros over every setting the file does not list, including the calibration block. Refuse the write unless the configuration was read completely.
The clock report carries the fuel-gauge capacity. It was commented out as breaking further communication, which was the lost first report of the session; with that fixed it reads fine. Skip parsing a failed read rather than parsing a stale buffer.
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
2 participants
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.
Four fixes found while bringing up an OpenUPS (v1, firmware 1.5) on Linux with libusb-0.1. The first two are USB framing bugs that affect every model; the others prevent a corrupted configuration being written to flash.
usb_set_altinterface() loses the first report of every session
open()callsusb_set_altinterface(), which resets the host's data toggle. The UPS firmware keeps its own, so the first OUT report after opening is silently dropped and its read times out. On the OpenUPS this reproduced on nearly every open (0 of 8 clean opens before, 15 of 15 after).This is also why
GetStatus()had the clock report commented out as "breaks further communication": the lost report was the cause, not the clock request. With the toggle left alone it reads fine, so the fuel-gauge capacity is available again.A late reply leaves every later reply one request behind
If a read times out, the reply still arrives and stays queued, including across a reopen. From then on every reply answers the previous request. A status read still appears to work because replies are dispatched by type, but a configuration read is silently shifted.
drain()discards anything queued when the device is opened.A shifted configuration read could be written back to flash
Each page reply was stored at the address that was requested rather than the one the device echoed, so one late reply shifted the whole configuration.
-ithen wrote that shifted copy back, calibration block included. The reply's echoed address is now checked and the page retried.Only the offset within the settings block is compared, because on the OpenUPS the echoed page alternates between
0x74xxand0x78xxfrom one configuration write to the next.-iwith-swrote zeros over every unlisted setting-ioverlays the file on the settings read from the device, but-sskips that read, so everything the file does not list was written as zero, including calibration.-inow refuses unless the configuration was read completely.Testing
0xd004, firmware 1.5): all four fixes, including repeated open/close cycles, configuration read/write with verified readback, and a stale-reply injection test that previously corrupted the readback.0xd005): detection, status polling, repeated opens, configuration read, and the same stale-reply test. Read-only, nothing written to it.usbhidchange but no model-specific code was touched.