korad-kaxxxxp: speed up scan process, rephrase response read routine

The Korad protocol relies on unterminated request and response strings,
which works well enough for fixed length acquisition and status queries.
But the variable length replies to identification requests suffered from
an implementation detail in the receive routine. A large timeout must be
used because supported devices reportedly are slow to respond. There is
no simple yet robust condition to detect the response's completion. The
scan code must prepare for the maximum response length across the set of
supported devices. Unfortunately the maximum amount of time was spent
waiting for the response to occupy the provided response buffer, before
a long total timeout expired.

Rework the korad driver's helper routine which gets a variable length
non-terminated text string. Keep the long initial timeout, and keep
iterating in that initial phase to quickly detect when response data
became available. But terminate the read sequence after a shorter period
without receive data after some initial receive data was seen. Assume
that identification responses get transferred at wire speed and without
additional delays beyond bitrate expectations. Acquisition and status
responses shall not be affected by this change.

This speeds up the scan for devices from roughly 5s to some 0.1s on
newer devices (KA3005P v5.5) and 0.5s on older devices (KA3005P V2.0).

This commit also addresses an issue in the response text termination,
where partial responses contained undefined data. The previous version's
return value was unspecific: Negative for fatal errors, but either zero
or non-zero for successful reads, with no way for callers to learn about
the received amount of data. The rephrased version always returns the
amount of received data, and adds internal documentation which discusses
the implementation's constraints and the motivation for the approach.

This is a modified version of the initial implementation which was
Submitted-By: Karl Palsson <redacted>

diff --git a/src/hardware/korad-kaxxxxp/protocol.c b/src/hardware/korad-kaxxxxp/protocol.c
index 848279d7c941311ff1bd946adf736d640f230900..2d0cb9b55e97b620d5cf9f25427c50b88278df38 100644 (file)
--- a/src/hardware/korad-kaxxxxp/protocol.c
+++ b/src/hardware/korad-kaxxxxp/protocol.c
@@ -38,31 +38,118 @@ SR_PRIV int korad_kaxxxxp_send_cmd(struct sr_serial_dev_inst *serial,
        return ret;
 }
 
+/**
+ * Read a variable length non-terminated string (caller specified maximum size).
+ *
+ * @param[in] serial The serial port to read from.
+ * @param[in] count The maximum amount of data to read.
+ * @param[out] buf The buffer to read data into. Must be larger than @a count.
+ *
+ * @return The amount of received data, or negative in case of error.
+ *     See @ref SR_ERR and other error codes.
+ *
+ * @internal
+ *
+ * The protocol has no concept of request/response termination. The only
+ * terminating conditions are either the caller's expected maxmimum byte
+ * count, or a period of time without receive data. It's essential to
+ * accept a longer initial period of time before the first receive data
+ * is seen. The supported devices can be very slow to respond.
+ *
+ * The protocol is text based. That's why the 'count' parameter specifies
+ * the expected number of text characters, and does not include the NUL
+ * termination which is not part of the wire protocol but gets added by
+ * the receive routine. The caller provided buffer is expected to have
+ * enough space for the text data and the NUL termination.
+ *
+ * Implementation detail: It's assumed that once receive data was seen,
+ * remaining response data will follow at wire speed. No further delays
+ * are expected beyond bitrate expectations. All normal commands in the
+ * acquisition phase are of fixed length which is known to the caller.
+ * Identification during device scan needs to deal with variable length
+ * data. Quick termination after reception is important there, as is the
+ * larger initial timeout period before receive data is seen.
+ */
 SR_PRIV int korad_kaxxxxp_read_chars(struct sr_serial_dev_inst *serial,
-                               int count, char *buf)
+       size_t count, char *buf)
 {
-       int ret, received, turns;
+       int timeout_first, timeout_later, timeout;
+       size_t retries_first, retries_later, retries;
+       size_t received;
+       int ret;
 
+       /* Clear the buffer early, to simplify the receive code path. */
+       memset(buf, 0, count + 1);
+
+       /*
+        * This calculation is aiming for backwards compatibility with
+        * an earlier implementation. An initial timeout is used which
+        * depends on the expected response byte count, and a maximum
+        * iteration count is used for read attempts.
+        *
+        * TODO Consider an absolute initial timeout instead, to reduce
+        * accumulated rounding errors for serial timeout results. The
+        * iteration with a short period is still required for variable
+        * length responses, because otherwise the serial communication
+        * layer would spend the total amount of time waiting for the
+        * remaining bytes, while the device probe code path by design
+        * passes a larger acceptable count than the typical and legal
+        * response would occupy.
+        *
+        * After initial receive data was seen, a shorter timeout is
+        * used which corresponds to a few bytes at wire speed. Idle
+        * periods without receive data longer than this threshold are
+        * taken as the end of the response. This is not compatible to
+        * the previous implementation, but was found to work as well.
+        * And severely reduces the time spent scanning for devices.
+        */
+       timeout_first = serial_timeout(serial, count);
+       retries_first = 100;
+       timeout_later = serial_timeout(serial, 3);
+       retries_later = 1;
+
+       sr_spew("want %zu bytes, timeout/retry: init %d/%zu, later %d/%zu.",
+               count, timeout_first, retries_first,
+               timeout_later, retries_later);
+
+       /*
+        * Run a sequence of read attempts. Try with the larger timeout
+        * and a high retry count until the first receive data became
+        * available. Then continue with a short timeout and small retry
+        * count.
+        *
+        * Failed read is fatal, immediately terminates the read sequence.
+        * A timeout in the initial phase just keeps repeating. A timeout
+        * after receive data was seen regularly terminates the sequence.
+        * Successful reads of non-empty responses keep extending the
+        * read sequence until no more receive data is available.
+        */
        received = 0;
-       turns = 0;
-
-       do {
-               if ((ret = serial_read_blocking(serial, buf + received,
-                               count - received,
-                               serial_timeout(serial, count))) < 0) {
-                       sr_err("Error %d reading %d bytes from device.",
+       timeout = timeout_first;
+       retries = retries_first;
+       while (received < count && retries--) {
+               ret = serial_read_blocking(serial,
+                       &buf[received], count - received, timeout);
+               if (ret < 0) {
+                       sr_err("Error %d reading %zu bytes from device.",
                               ret, count);
                        return ret;
                }
+               if (ret == 0 && !received)
+                       continue;
+               if (ret == 0 && received) {
+                       sr_spew("receive timed out, want %zu, received %zu.",
+                               count, received);
+                       break;
+               }
                received += ret;
-               turns++;
-       } while ((received < count) && (turns < 100));
-
-       buf[count] = 0;
-
-       sr_spew("Received: '%s'.", buf);
+               timeout = timeout_later;
+               retries = retries_later;
+       }
+       /* TODO Escape non-printables? Seen those with status queries. */
+       sr_dbg("got %zu bytes, received: '%s'.", received, buf);
 
-       return ret;
+       return received;
 }
 
 static void give_device_time_to_process(struct dev_context *devc)

diff --git a/src/hardware/korad-kaxxxxp/protocol.h b/src/hardware/korad-kaxxxxp/protocol.h
index 8f8ce5f229fc344d4a43265e8447725ef4f21a09..428f67bb2cc9375fb848d9410305ddbd18e5296b 100644 (file)
--- a/src/hardware/korad-kaxxxxp/protocol.h
+++ b/src/hardware/korad-kaxxxxp/protocol.h
@@ -118,7 +118,7 @@ struct dev_context {
 SR_PRIV int korad_kaxxxxp_send_cmd(struct sr_serial_dev_inst *serial,
                                        const char *cmd);
 SR_PRIV int korad_kaxxxxp_read_chars(struct sr_serial_dev_inst *serial,
-                                       int count, char *buf);
+                                       size_t count, char *buf);
 SR_PRIV int korad_kaxxxxp_set_value(struct sr_serial_dev_inst *serial,
                                        int target, struct dev_context *devc);
 SR_PRIV int korad_kaxxxxp_get_value(struct sr_serial_dev_inst *serial,