From 58b8348b1cc3ccfc75134c5889558f2806b445ec Mon Sep 17 00:00:00 2001 From: Jack Elston Date: Wed, 16 Sep 2026 14:02:46 -0600 Subject: [PATCH 1/4] usbhid: keep the data toggle and discard stale reports on open 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. --- src/lib/usbhid.cpp | 14 +++++++++++++- src/lib/usbhid.h | 1 + 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/lib/usbhid.cpp b/src/lib/usbhid.cpp index 0548493..ee5513e 100644 --- a/src/lib/usbhid.cpp +++ b/src/lib/usbhid.cpp @@ -59,7 +59,9 @@ struct usb_dev_handle *USBHID::open(void) return NULL; } - ret = usb_set_altinterface(this->handle, 0); + // no usb_set_altinterface(): it resets the host's data toggle but the device + // firmware keeps its own, so the first OUT report of the session is dropped + drain(); fprintf(stdout, "Product: %s, Manufacturer: %s, Firmware Version: %s\n", this->getProduct(), this->getManufacturer(), this->getSerial()); @@ -67,6 +69,16 @@ struct usb_dev_handle *USBHID::open(void) return this->handle; } +// Discard reports left queued by an earlier session (a reply that arrived after its +// read timed out), otherwise every reply lands one request late +void USBHID::drain(void) +{ + char buf[64]; + for (int i = 0; i < 8; i++) + if (usb_interrupt_read(this->handle, USB_ENDPOINT_IN + 1, buf, sizeof(buf), 20) <= 0) + break; +} + struct usb_device *USBHID::find(void) { struct usb_bus *bus; diff --git a/src/lib/usbhid.h b/src/lib/usbhid.h index 59d7335..c442852 100644 --- a/src/lib/usbhid.h +++ b/src/lib/usbhid.h @@ -32,6 +32,7 @@ class USBHID { struct usb_dev_handle *handle; struct usb_device *find(void); + void drain(void); int release(); }; From 8f64da985c74320e8f9ec6ad7c4420a80c14cdc1 Mon Sep 17 00:00:00 2001 From: Jack Elston Date: Wed, 16 Sep 2026 14:02:46 -0600 Subject: [PATCH 2/4] config: verify the echoed address when reading configuration memory 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. --- src/lib/HIDInterface.cpp | 1 + src/lib/HIDInterface.h | 2 ++ src/lib/HIDOpenUPS.cpp | 22 +++++++++++++++++++--- src/lib/HIDOpenUPS2.cpp | 22 +++++++++++++++++++--- 4 files changed, 41 insertions(+), 6 deletions(-) diff --git a/src/lib/HIDInterface.cpp b/src/lib/HIDInterface.cpp index 8675215..0773e8a 100644 --- a/src/lib/HIDInterface.cpp +++ b/src/lib/HIDInterface.cpp @@ -19,6 +19,7 @@ HIDInterface::HIDInterface(USBHID *hidDevice) { d = hidDevice; + m_bConfigReadOk = true; memset(m_chPackages, 0, SETTINGS_PACKS * 16); } diff --git a/src/lib/HIDInterface.h b/src/lib/HIDInterface.h index 7ab2fab..9748aa6 100644 --- a/src/lib/HIDInterface.h +++ b/src/lib/HIDInterface.h @@ -78,6 +78,8 @@ class HIDInterface { virtual void restartUPS() = 0; virtual void restartUPSInBootloaderMode() = 0; + bool m_bConfigReadOk; // false if ReadConfigurationMemory() could not read every page + unsigned long m_ulSettingsAddr; unsigned char m_chPackages[SETTINGS_PACKS * 16]; diff --git a/src/lib/HIDOpenUPS.cpp b/src/lib/HIDOpenUPS.cpp index e818467..28c4bc9 100644 --- a/src/lib/HIDOpenUPS.cpp +++ b/src/lib/HIDOpenUPS.cpp @@ -227,11 +227,27 @@ void HIDOpenUPS::ReadConfigurationMemory() m_ulSettingsAddr = OPENUPS_SETTINGS_ADDR_START; memset(m_chPackages, 0, SETTINGS_PACKS * 16); + m_bConfigReadOk = true; + while (m_ulSettingsAddr < OPENUPS_SETTINGS_ADDR_END) { - sendMessage(OPENUPS_MEM_READ_OUT, 4, m_ulSettingsAddr & 0xFF, (m_ulSettingsAddr >> 8) & 0xFF, 0x00, 0x10); - recvMessage(recv); - parseMessage(recv); + unsigned char lo = m_ulSettingsAddr & 0xFF, hi = (m_ulSettingsAddr >> 8) & 0xFF; + bool got = false; + // the reply echoes the address; a mismatch is a late reply to an earlier request. + // Only the offset within the settings block is compared: the echoed page can + // alternate between banks from one configuration write to the next + for (int tries = 0; tries < 3 && !got; tries++) { + sendMessage(OPENUPS_MEM_READ_OUT, 4, lo, hi, 0x00, 0x10); + for (int reads = 0; reads < 2 && !got; reads++) { + if (recvMessage(recv) <= 0) break; + got = recv[0] == OPENUPS_MEM_READ_IN && recv[1] == lo && (recv[2] & 0x03) == (hi & 0x03); + } + } + if (got) parseMessage(recv); + else { + fprintf(stderr, "Failed to read configuration page 0x%lx\n", m_ulSettingsAddr); + m_bConfigReadOk = false; + } m_ulSettingsAddr += 16; } } diff --git a/src/lib/HIDOpenUPS2.cpp b/src/lib/HIDOpenUPS2.cpp index b221c19..70c81f9 100644 --- a/src/lib/HIDOpenUPS2.cpp +++ b/src/lib/HIDOpenUPS2.cpp @@ -950,11 +950,27 @@ void HIDOpenUPS2::ReadConfigurationMemory() m_ulSettingsAddr = OPENUPS2_SETTINGS_ADDR_START; memset(m_chPackages, 0, SETTINGS_PACKS * 16); + m_bConfigReadOk = true; + while (m_ulSettingsAddr < OPENUPS2_SETTINGS_ADDR_END) { - sendMessage(OPENUPS2_MEM_READ_OUT, 4, m_ulSettingsAddr & 0xFF, (m_ulSettingsAddr >> 8) & 0xFF, 0x00, 0x10); - recvMessage(recv); - parseMessage(recv); + unsigned char lo = m_ulSettingsAddr & 0xFF, hi = (m_ulSettingsAddr >> 8) & 0xFF; + bool got = false; + // the reply echoes the address; a mismatch is a late reply to an earlier request. + // Only the offset within the settings block is compared: the echoed page can + // alternate between banks from one configuration write to the next + for (int tries = 0; tries < 3 && !got; tries++) { + sendMessage(OPENUPS2_MEM_READ_OUT, 4, lo, hi, 0x00, 0x10); + for (int reads = 0; reads < 2 && !got; reads++) { + if (recvMessage(recv) <= 0) break; + got = recv[0] == OPENUPS2_MEM_READ_IN && recv[1] == lo && (recv[2] & 0x03) == (hi & 0x03); + } + } + if (got) parseMessage(recv); + else { + fprintf(stderr, "Failed to read configuration page 0x%lx\n", m_ulSettingsAddr); + m_bConfigReadOk = false; + } m_ulSettingsAddr += 16; } } From 487f57c62c43293d3b6ba6801b4ec3063a28dc2d Mon Sep 17 00:00:00 2001 From: Jack Elston Date: Wed, 16 Sep 2026 14:02:46 -0600 Subject: [PATCH 3/4] main: refuse to write a configuration that was not fully 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. --- src/main.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/main.cpp b/src/main.cpp index 6b42be2..7e61694 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -222,6 +222,14 @@ int main(int argc, char **argv) } if (infile) { + // the file is overlaid on the settings just read; writing an unread or partial + // buffer would zero or corrupt everything else, calibration included + if (!withConfiguration || !ups->m_bConfigReadOk) { + fprintf(stderr, "Not writing %s: device configuration was not read completely%s\n", + infile, withConfiguration ? "" : " (-s given)"); + d->close(); + return 4; + } ups->EraseConfigurationMemory(); fprintf(stdout, "Erased configuration\n"); From 331359cea4473e36ac82abeb9319475369a31a52 Mon Sep 17 00:00:00 2001 From: Jack Elston Date: Wed, 16 Sep 2026 14:02:46 -0600 Subject: [PATCH 4/4] openups: check status reads and read the clock report 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. --- src/lib/HIDOpenUPS.cpp | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/src/lib/HIDOpenUPS.cpp b/src/lib/HIDOpenUPS.cpp index 28c4bc9..e6c90ad 100644 --- a/src/lib/HIDOpenUPS.cpp +++ b/src/lib/HIDOpenUPS.cpp @@ -197,27 +197,17 @@ void HIDOpenUPS::printValues() void HIDOpenUPS::GetStatus() { unsigned char recv[32]; - int ret; sendMessage(OPENUPS_GET_ALL_VALUES, 0); - usleep(1000); - recvMessage(recv); - parseMessage(recv); - usleep(1000); - - ret = sendMessage(OPENUPS_GET_ALL_VALUES_2, 0); - usleep(1000); - recvMessage(recv); - parseMessage(recv); - usleep(1000); - - //TODO This breaks further communication with device. Needs more investigation. - /* - ret = sendMessage(OPENUPS_CLOCK_OUT, 0); - usleep(1000); - recvMessage(recv); - parseMessage(recv); - */ + if (recvMessage(recv) > 0) parseMessage(recv); + + sendMessage(OPENUPS_GET_ALL_VALUES_2, 0); + if (recvMessage(recv) > 0) parseMessage(recv); + + // carries the fuel-gauge capacity; this used to break further communication + // because the first report of the session was lost to the data toggle reset + sendMessage(OPENUPS_CLOCK_OUT, 0); + if (recvMessage(recv) > 0) parseMessage(recv); } void HIDOpenUPS::ReadConfigurationMemory()