serial: extend stream detect for variable length packet checkers

The previous implementation of the packet detection in a serial stream
assumed that all packets are of equal length which is known in advance.
Extend the packet validity check interface such that caller provided
callbacks can either decide that the input is valid or invalid (terminal
decision), or request more receive data before a decision can be made
(deferral, coverring variable length packets, with a minimum size to
cover the header before a length becomes available and the total packet
length is known).

This commit extends the API, and adjusts the call sites to not break the
compilation. Actual variable length checkers are yet to be done. Improve
readability while we are here: Better reflect the purpose and units of
variables in their identifiers. Tweak diagnostics messages, update
inline and doxygen comments.

diff --git a/include/libsigrok/libsigrok.h b/include/libsigrok/libsigrok.h
index 02d1841aa1833f1537fbd54815ffc927d3c148e7..5d3d23c9963f102bf0aac1b7127a412bf784829a 100644 (file)
--- a/include/libsigrok/libsigrok.h
+++ b/include/libsigrok/libsigrok.h
@@ -80,6 +80,13 @@ enum sr_error_code {
        /* Update sr_strerror()/sr_strerror_name() (error.c) upon changes! */
 };
 
+/** Ternary return type for DMM/LCR/etc packet parser validity checks. */
+enum sr_valid_code {
+       SR_PACKET_INVALID = -1, /**< Certainly invalid. */
+       SR_PACKET_VALID = 0,    /**< Certainly valid. */
+       SR_PACKET_NEED_RX = +1, /**< Need more RX data. */
+};
+
 #define SR_MAX_CHANNELNAME_LEN 32
 
 /* Handy little macros */

diff --git a/src/hardware/appa-55ii/api.c b/src/hardware/appa-55ii/api.c
index 50bc37338cc3635d7f28b6d2fe086499ffb8552c..14211c6a92df90eb4f546d561d9517a60a93d5bd 100644 (file)
--- a/src/hardware/appa-55ii/api.c
+++ b/src/hardware/appa-55ii/api.c
@@ -81,7 +81,7 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
 
        /* Let's get a bit of data and see if we can find a packet. */
        if (serial_stream_detect(serial, buf, &len, 25,
-                       appa_55ii_packet_valid, 500) != SR_OK)
+                       appa_55ii_packet_valid, NULL, NULL, 500) != SR_OK)
                goto scan_cleanup;
 
        sr_info("Found device on port %s.", conn);

diff --git a/src/hardware/kern-scale/api.c b/src/hardware/kern-scale/api.c
index fae1dfe18b58ea56d30d3f555dda314a095f73a3..f94eeda5e9322f1812bd52e8b79497af8674058c 100644 (file)
--- a/src/hardware/kern-scale/api.c
+++ b/src/hardware/kern-scale/api.c
@@ -88,7 +88,7 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
        /* Let's get a bit of data and see if we can find a packet. */
        len = sizeof(buf);
        ret = serial_stream_detect(serial, buf, &len, scale->packet_size,
-                                  scale->packet_valid, 3000);
+               scale->packet_valid, NULL, NULL, 3000);
        if (ret != SR_OK)
                goto scan_cleanup;
 

diff --git a/src/hardware/mastech-ms6514/api.c b/src/hardware/mastech-ms6514/api.c
index 6e22dd6c344f5f3a646668b525289ea62786fb7a..62d923c460e8fff8574318297bf20bbf2d54b599 100644 (file)
--- a/src/hardware/mastech-ms6514/api.c
+++ b/src/hardware/mastech-ms6514/api.c
@@ -82,8 +82,8 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
        sr_info("Probing serial port %s.", conn);
 
        /* Let's get a bit of data and see if we can find a packet. */
-       if (serial_stream_detect(serial, buf, &len, (2 * MASTECH_MS6514_FRAME_SIZE),
-                       mastech_ms6514_packet_valid, 500) != SR_OK)
+       if (serial_stream_detect(serial, buf, &len, 2 * MASTECH_MS6514_FRAME_SIZE,
+                       mastech_ms6514_packet_valid, NULL, NULL, 500) != SR_OK)
                goto scan_cleanup;
 
        sr_info("Found device on port %s.", conn);

diff --git a/src/hardware/serial-dmm/api.c b/src/hardware/serial-dmm/api.c
index 9b0f3b23fb0b1a8616de5ab50a40938d9c267562..6651e80231b408fe22fcbb5a41062958af981793 100644 (file)
--- a/src/hardware/serial-dmm/api.c
+++ b/src/hardware/serial-dmm/api.c
@@ -103,7 +103,7 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
        /* Let's get a bit of data and see if we can find a packet. */
        len = sizeof(buf);
        ret = serial_stream_detect(serial, buf, &len, dmm->packet_size,
-                                  dmm->packet_valid, 3000);
+                                  dmm->packet_valid, NULL, NULL, 3000);
        if (ret != SR_OK)
                goto scan_cleanup;
 

diff --git a/src/hardware/serial-lcr/api.c b/src/hardware/serial-lcr/api.c
index 25b4e47ffa0c69540182ce322c9274ec27ca50c4..7f3fc5362ae1b980991857be4b88d7d66d2be0ad 100644 (file)
--- a/src/hardware/serial-lcr/api.c
+++ b/src/hardware/serial-lcr/api.c
@@ -114,7 +114,7 @@ static int scan_lcr_port(const struct lcr_info *lcr,
        }
        len = sizeof(buf);
        ret = serial_stream_detect(serial, buf, &len,
-               lcr->packet_size, lcr->packet_valid, 3000);
+               lcr->packet_size, lcr->packet_valid, NULL, NULL, 3000);
        if (ret != SR_OK)
                goto scan_port_cleanup;
 
@@ -198,7 +198,7 @@ static int read_lcr_port(struct sr_dev_inst *sdi,
        len = sizeof(buf);
        scan_packet_check_setup(sdi);
        ret = serial_stream_detect(serial, buf, &len,
-               lcr->packet_size, scan_packet_check_func, 1500);
+               lcr->packet_size, scan_packet_check_func, NULL, NULL, 1500);
        scan_packet_check_setup(NULL);
 
        return ret;

diff --git a/src/hardware/teleinfo/api.c b/src/hardware/teleinfo/api.c
index 92bc27a9e32d8919c784eef76735d90dd2fd73d7..e7b4e2342f36e7c71da3846998ce9e3476b1af4c 100644 (file)
--- a/src/hardware/teleinfo/api.c
+++ b/src/hardware/teleinfo/api.c
@@ -77,7 +77,7 @@ static GSList *scan(struct sr_dev_driver *di, GSList *options)
 
        /* Let's get a bit of data and see if we can find a packet. */
        if (serial_stream_detect(serial, buf, &len, len,
-                                teleinfo_packet_valid, 3000) != SR_OK)
+                       teleinfo_packet_valid, NULL, NULL, 3000) != SR_OK)
                goto scan_cleanup;
 
        sr_info("Found device on port %s.", conn);

diff --git a/src/libsigrok-internal.h b/src/libsigrok-internal.h
index b614c93d0bdc039407a345cdc0db0d844103abcf..1ff594bf58b729422734bdb3923ba9bf1b1a0e0d 100644 (file)
--- a/src/libsigrok-internal.h
+++ b/src/libsigrok-internal.h
@@ -1823,6 +1823,8 @@ enum {
 };
 
 typedef gboolean (*packet_valid_callback)(const uint8_t *buf);
+typedef int (*packet_valid_len_callback)(void *st,
+       const uint8_t *p, size_t l, size_t *pl);
 
 typedef GSList *(*sr_ser_list_append_t)(GSList *devs, const char *name,
                const char *desc);
@@ -1852,10 +1854,10 @@ SR_PRIV int serial_set_paramstr(struct sr_serial_dev_inst *serial,
 SR_PRIV int serial_readline(struct sr_serial_dev_inst *serial, char **buf,
                int *buflen, gint64 timeout_ms);
 SR_PRIV int serial_stream_detect(struct sr_serial_dev_inst *serial,
-                                uint8_t *buf, size_t *buflen,
-                                size_t packet_size,
-                                packet_valid_callback is_valid,
-                                uint64_t timeout_ms);
+               uint8_t *buf, size_t *buflen,
+               size_t packet_size, packet_valid_callback is_valid,
+               packet_valid_len_callback is_valid_len, size_t *return_size,
+               uint64_t timeout_ms);
 SR_PRIV int sr_serial_extract_options(GSList *options, const char **serial_device,
                                      const char **serial_options);
 SR_PRIV int serial_source_add(struct sr_session *session,

diff --git a/src/serial.c b/src/serial.c
index 1b76c935e3a0e1b73874b875185a18105f1f7f37..c87c3a3347fc3a45ef4c2824a7e797ff692bf41e 100644 (file)
--- a/src/serial.c
+++ b/src/serial.c
@@ -787,14 +787,24 @@ SR_PRIV int serial_readline(struct sr_serial_dev_inst *serial,
 /**
  * Try to find a valid packet in a serial data stream.
  *
- * @param serial Previously initialized serial port structure.
- * @param buf Buffer containing the bytes to write.
- * @param buflen Size of the buffer.
+ * @param[in] serial Previously initialized serial port structure.
+ * @param[in] buf Buffer containing the bytes to write.
+ * @param[in] buflen Size of the buffer.
  * @param[in] packet_size Size, in bytes, of a valid packet.
- * @param is_valid Callback that assesses whether the packet is valid or not.
+ * @param[in] is_valid Callback that assesses whether the packet is valid or not.
+ * @param[in] is_valid_len Callback which checks a variable length packet.
+ * @param[out] return_size Detected packet size in case of successful match.
  * @param[in] timeout_ms The timeout after which, if no packet is detected, to
  *                       abort scanning.
  *
+ * Data is received from the serial port and into the caller provided
+ * buffer, until the buffer is exhausted, or the timeout has expired,
+ * or a valid packet was found. Receive data is passed to the caller
+ * provided validity check routine, assuming either fixed size packets
+ * (#is_valid parameter, exact match to the #packet_size length) or
+ * packets of variable length (#is_valid_len parameter, minimum length
+ * #packet_size required for first invocation).
+ *
  * @retval SR_OK Valid packet was found within the given timeout.
  * @retval SR_ERR Failure.
  *
@@ -802,72 +812,115 @@ SR_PRIV int serial_readline(struct sr_serial_dev_inst *serial,
  */
 SR_PRIV int serial_stream_detect(struct sr_serial_dev_inst *serial,
        uint8_t *buf, size_t *buflen,
-       size_t packet_size,
-       packet_valid_callback is_valid,
+       size_t packet_size, packet_valid_callback is_valid,
+       packet_valid_len_callback is_valid_len, size_t *return_size,
        uint64_t timeout_ms)
 {
-       uint64_t start, time, byte_delay_us;
-       size_t ibuf, i, maxlen;
-       ssize_t len;
-
-       maxlen = *buflen;
+       uint64_t start_us, elapsed_ms, byte_delay_us;
+       size_t fill_idx, check_idx, max_fill_idx;
+       ssize_t recv_len;
+       const uint8_t *check_ptr;
+       size_t check_len, pkt_len;
+       gboolean do_dump;
+       int ret;
 
        sr_dbg("Detecting packets on %s (timeout = %" PRIu64 "ms).",
                serial->port, timeout_ms);
 
-       if (maxlen < (packet_size * 2) ) {
-               sr_err("Buffer size must be at least twice the packet size.");
-               return SR_ERR;
+       max_fill_idx = *buflen;
+       if (max_fill_idx < 2 * packet_size) {
+               /*
+                * The heuristics in this check is only useful for fixed
+                * packet length scenarios, but for variable length setups
+                * we don't know the packets' sizes up front.
+                */
+               sr_err("Small stream detect RX buffer, want 2x packet size.");
+               return SR_ERR_ARG;
        }
 
-       /* Assume 8n1 transmission. That is 10 bits for every byte. */
        byte_delay_us = serial_timeout(serial, 1) * 1000;
-       start = g_get_monotonic_time();
+       start_us = g_get_monotonic_time();
+
+       check_idx = fill_idx = 0;
+       while (fill_idx < max_fill_idx) {
+               /*
+                * Read bytes individually. Lets callers continue to
+                * successfully process next RX data after first match.
+                * Run full loop bodies for empty or failed reception
+                * in an iteration, to have timeouts checked.
+                */
+               recv_len = serial_read_nonblocking(serial, &buf[fill_idx], 1);
+               if (recv_len > 0)
+                       fill_idx += recv_len;
+
+               /* Dump receive data when (a minimum) size is reached. */
+               check_ptr = &buf[check_idx];
+               check_len = fill_idx - check_idx;
+               do_dump = check_len >= packet_size;
+               do_dump &= sr_log_loglevel_get() >= SR_LOG_SPEW;
+               if (do_dump) {
+                       GString *text;
 
-       i = ibuf = len = 0;
-       while (ibuf < maxlen) {
-               len = serial_read_nonblocking(serial, &buf[ibuf], 1);
-               if (len > 0) {
-                       ibuf += len;
-               } else if (len == 0) {
-                       /* No logging, already done in serial_read(). */
-               } else {
-                       /* Error reading byte, but continuing anyway. */
+                       text = sr_hexdump_new(check_ptr, check_len);
+                       sr_spew("Trying packet: len %zu, bytes %s",
+                               check_len, text->str);
+                       sr_hexdump_free(text);
                }
 
-               time = g_get_monotonic_time() - start;
-               time /= 1000;
-
-               if ((ibuf - i) >= packet_size) {
-                       GString *text;
-                       /* We have at least a packet's worth of data. */
-                       text = sr_hexdump_new(&buf[i], packet_size);
-                       sr_spew("Trying packet: %s", text->str);
-                       sr_hexdump_free(text);
-                       if (is_valid(&buf[i])) {
-                               sr_spew("Found valid %zu-byte packet after "
-                                       "%" PRIu64 "ms.", (ibuf - i), time);
-                               *buflen = ibuf;
+               /* A packet's (minimum) length was received, check its data. */
+               elapsed_ms = g_get_monotonic_time() - start_us;
+               elapsed_ms /= 1000;
+               if (is_valid_len && check_len >= packet_size) {
+                       pkt_len = packet_size;
+                       ret = is_valid_len(NULL, check_ptr, check_len, &pkt_len);
+                       if (ret == SR_PACKET_VALID) {
+                               /* Exact match. Terminate with success. */
+                               sr_spew("Valid packet after %" PRIu64 "ms.",
+                                       elapsed_ms);
+                               sr_spew("RX count %zu, packet len %zu.",
+                                       fill_idx, pkt_len);
+                               *buflen = fill_idx;
+                               if (return_size)
+                                       *return_size = pkt_len;
                                return SR_OK;
+                       }
+                       if (ret == SR_PACKET_NEED_RX) {
+                               /* Incomplete, keep accumulating RX data. */
+                               sr_spew("Checker needs more RX data.");
                        } else {
-                               sr_spew("Got %zu bytes, but not a valid "
-                                       "packet.", (ibuf - i));
+                               /* Not a valid packet. Continue searching. */
+                               sr_spew("Invalid packet, advancing read pos.");
+                               check_idx++;
+                       }
+               }
+               if (is_valid && check_len >= packet_size) {
+                       if (is_valid(check_ptr)) {
+                               /* Exact match. Terminate with success. */
+                               sr_spew("Valid packet after %" PRIu64 "ms.",
+                                       elapsed_ms);
+                               sr_spew("RX count %zu, packet len %zu.",
+                                       fill_idx, packet_size);
+                               *buflen = fill_idx;
+                               if (return_size)
+                                       *return_size = packet_size;
+                               return SR_OK;
                        }
                        /* Not a valid packet. Continue searching. */
-                       i++;
+                       sr_spew("Invalid packet, advancing read pointer.");
+                       check_idx++;
                }
-               if (time >= timeout_ms) {
-                       /* Timeout */
-                       sr_dbg("Detection timed out after %" PRIu64 "ms.", time);
+
+               /* Check for packet search timeout. */
+               if (elapsed_ms >= timeout_ms) {
+                       sr_dbg("Detection timed out after %" PRIu64 "ms.",
+                               elapsed_ms);
                        break;
                }
-               if (len < 1)
+               if (recv_len < 1)
                        g_usleep(byte_delay_us);
        }
-
-       *buflen = ibuf;
-
-       sr_info("Didn't find a valid packet (read %zu bytes).", *buflen);
+       sr_info("Didn't find a valid packet (read %zu bytes).", fill_idx);
+       *buflen = fill_idx;
 
        return SR_ERR;
 }