From: Gerhard Sittig <redacted>
Date: Sat, 22 Jan 2022 07:38:53 +0000 (+0100)
Subject: kingst-la2016: rephrase USB renum code path after firmware upload
X-Git-Url: https://sigrok.org/gitaction?a=commitdiff_plain;h=22cb067ab034ab95111f5bc388413dd244f64492;p=libsigrok.git

kingst-la2016: rephrase USB renum code path after firmware upload

The dev_open() routine postprocesses USB renumeration after a previous
FX2 MCU firmware upload. Concentrate all tunables in the driver's header
file, and discuss their meaning in relation to each other. Rephrase the
logic to start checking earlier, and keep checking for longer, adjust
diagnostics in the process. Void the "firmware uploaded" flag when the
upload took effect. Consistently use u64 for timestamps. Rename a few
variables to improve readabilty.
---

diff --git a/src/hardware/kingst-la2016/api.c b/src/hardware/kingst-la2016/api.c
index c5a66108..4d501450 100644
--- a/src/hardware/kingst-la2016/api.c
+++ b/src/hardware/kingst-la2016/api.c
@@ -142,7 +142,7 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
 	unsigned int i, j;
 	const char *conn;
 	char connection_id[64];
-	int64_t fw_updated;
+	uint64_t fw_uploaded;
 	unsigned int dev_addr;
 
 	drvc = di->context;
@@ -196,7 +196,7 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
 		sdi->status = SR_ST_INITIALIZING;
 		sdi->connection_id = g_strdup(connection_id);
 
-		fw_updated = 0;
+		fw_uploaded = 0;
 		dev_addr = libusb_get_device_address(devlist[i]);
 		if (des.iProduct != 2) {
 			sr_info("Device at '%s' has no firmware loaded.", connection_id);
@@ -207,7 +207,7 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
 				g_free(sdi);
 				continue;
 			}
-			fw_updated = g_get_monotonic_time();
+			fw_uploaded = g_get_monotonic_time();
 			/* Will re-enumerate. Mark as "unknown address yet". */
 			dev_addr = 0xff;
 		}
@@ -222,7 +222,7 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
 
 		devc = g_malloc0(sizeof(struct dev_context));
 		sdi->priv = devc;
-		devc->fw_updated = fw_updated;
+		devc->fw_uploaded = fw_uploaded;
 		devc->threshold_voltage_idx = 0;
 		devc->threshold_voltage = logic_threshold_value[devc->threshold_voltage_idx];
 
@@ -336,43 +336,42 @@ static int la2016_dev_open(struct sr_dev_inst *sdi)
 static int dev_open(struct sr_dev_inst *sdi)
 {
 	struct dev_context *devc;
-	int64_t timediff_us, timediff_ms;
-	uint64_t reset_done;
-	uint64_t now;
+	uint64_t reset_done, now, elapsed_ms;
 	int ret;
 
 	devc = sdi->priv;
 
 	/*
-	 * When the sigrok driver recently uploaded firmware, wait for
-	 * the FX2 to re-enumerate. Deal with the condition that the
-	 * MCU can take some 2000ms to be gone from the bus before it
-	 * re-appears executing the recently uploaded firmware.
+	 * When the sigrok driver recently has uploaded MCU firmware,
+	 * then wait for the FX2 to re-enumerate. Allow the USB device
+	 * to vanish before it reappears. Timeouts are rough estimates
+	 * after all, the imprecise time of the last check (potentially
+	 * executes after the total check period) simplifies code paths
+	 * with optional diagnostics. And increases the probability of
+	 * successfully detecting "late/slow" devices.
 	 */
-	ret = SR_ERR;
-	if (devc->fw_updated > 0) {
+	if (devc->fw_uploaded) {
 		sr_info("Waiting for device to reset after firmware upload.");
-		reset_done = devc->fw_updated;
-		reset_done += 1800 * 1000; /* 1.8 seconds */
 		now = g_get_monotonic_time();
-		if (reset_done > now)
+		reset_done = devc->fw_uploaded + RENUM_GONE_DELAY_MS * 1000;
+		if (now < reset_done)
 			g_usleep(reset_done - now);
-		timediff_ms = 0;
-		while (timediff_ms < MAX_RENUM_DELAY_MS) {
-			g_usleep(200 * 1000);
-
-			timediff_us = g_get_monotonic_time() - devc->fw_updated;
-			timediff_ms = timediff_us / 1000;
-
-			if ((ret = la2016_dev_open(sdi)) == SR_OK)
+		do {
+			now = g_get_monotonic_time();
+			elapsed_ms = (now - devc->fw_uploaded) / 1000;
+			sr_spew("Waited %" PRIu64 "ms.", elapsed_ms);
+			ret = la2016_dev_open(sdi);
+			if (ret == SR_OK) {
+				devc->fw_uploaded = 0;
 				break;
-			sr_spew("Waited %" PRIi64 "ms.", timediff_ms);
-		}
+			}
+			g_usleep(RENUM_POLL_INTERVAL_MS * 1000);
+		} while (elapsed_ms < RENUM_CHECK_PERIOD_MS);
 		if (ret != SR_OK) {
 			sr_err("Device failed to re-enumerate.");
 			return SR_ERR;
 		}
-		sr_info("Device came back after %" PRIi64 "ms.", timediff_ms);
+		sr_info("Device came back after %" PRIi64 "ms.", elapsed_ms);
 	} else {
 		ret = la2016_dev_open(sdi);
 	}
diff --git a/src/hardware/kingst-la2016/protocol.h b/src/hardware/kingst-la2016/protocol.h
index 4a35c9ee..8ea01809 100644
--- a/src/hardware/kingst-la2016/protocol.h
+++ b/src/hardware/kingst-la2016/protocol.h
@@ -43,9 +43,20 @@
 #define LA2016_EP6_PKTSZ	512 /* Max packet size of USB endpoint 6. */
 #define LA2016_USB_BUFSZ	(256 * 2 * LA2016_EP6_PKTSZ) /* 256KiB buffer. */
 
-#define MAX_RENUM_DELAY_MS	3000
+/* USB communication timeout during regular operation. */
 #define DEFAULT_TIMEOUT_MS	200
 
+/*
+ * Check for MCU firmware to take effect after upload. Check the device
+ * presence for a maximum period of time, delay between checks in that
+ * phase. Allow for the device to vanish after upload and before checks,
+ * to not mistake its earlier incarnation for the successful operation
+ * of the most recently loaded firmware.
+ */
+#define RENUM_CHECK_PERIOD_MS	3000
+#define RENUM_GONE_DELAY_MS	1800
+#define RENUM_POLL_INTERVAL_MS	200
+
 #define LA2016_THR_VOLTAGE_MIN	0.40
 #define LA2016_THR_VOLTAGE_MAX	4.00
 
@@ -81,8 +92,7 @@ typedef struct pwm_setting {
 
 struct dev_context {
 	struct sr_context *ctx;
-
-	int64_t fw_updated;
+	uint64_t fw_uploaded;
 
 	/* User specified parameters. */
 	pwm_setting_t pwm_setting[2];