From: Gerhard Sittig Date: Thu, 24 Sep 2020 14:29:06 +0000 (+0200) Subject: serial: extend stream detect for variable length packet checkers X-Git-Url: https://sigrok.org/gitweb/?p=libsigrok.git;a=commitdiff_plain;h=1a7adeac29d6331b53a2c78fc9c70429b32da0bd 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 02d1841a..5d3d23c9 100644 --- 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 50bc3733..14211c6a 100644 --- 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 fae1dfe1..f94eeda5 100644 --- 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 6e22dd6c..62d923c4 100644 --- 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 9b0f3b23..6651e802 100644 --- 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 25b4e47f..7f3fc536 100644 --- 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 92bc27a9..e7b4e234 100644 --- 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 b614c93d..1ff594bf 100644 --- 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 1b76c935..c87c3a33 100644 --- 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; }