Fix USB report framing and guard configuration writes

src/lib/HIDInterface.cpp

@@ -19,6 +19,7 @@
 
 HIDInterface::HIDInterface(USBHID *hidDevice) {
 	d = hidDevice;
+	m_bConfigReadOk = true;
 	memset(m_chPackages, 0, SETTINGS_PACKS * 16);
 }
 

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];
 

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() 
@@ -227,11 +217,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;
 	}
 }

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;
 	}
 }

src/lib/usbhid.cpp

@@ -59,14 +59,26 @@ 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());
 
     connected = true;
     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;

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();
 };
 

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");