From: Gerhard Sittig Date: Fri, 29 Dec 2023 11:32:45 +0000 (+0100) Subject: output/srzip: accept arbitrary input and output unit sizes X-Git-Url: https://sigrok.org/gitaction?a=commitdiff_plain;h=5c52d96a3bd14aa7870e9d506ed40b3e4e8e73a6;p=libsigrok.git output/srzip: accept arbitrary input and output unit sizes Commit c03aaf342c3f introduced a check and refused to store sample data when the feed would not exactly match an expected width that was derived from the device's or input's total count of logic channels. Earlier versions assumed the match but never checked (immediately forwarded session feed content to the ZIP archive). Existing applications may not be prepared to process the resulting archive where meta data and samples disagree on essential properties. The fatal condition aborted execution, which was perceived as a regression. A message was added later to communicate that the condition was hit, but its WARN severity was misleading, and its meaning still was obscure to users. This commit extends the reception of sample data in the session feed and its accumulation in the local buffer before ZIP archive appends. Any combination of input and output unit sizes are accepted. It's perfectly legal for sources to not communicate data for disabled channels, as well as to communicate wider data images than strictly necessary to simplify their support for a variety of input formats or device models. Details are available to users at higher log levels (INFO). Default levels only communicate fatal conditions (which should be implementation flaws now exclusively). The issue reproduces especially well with input formats that are rather flexible, or device drivers which support a range of devices with many configurations or models of differing capabilities. The issue was most recently reported for the OLS driver and an Arduino SUMP firmware. Given how many input modules and device drivers can feed into a few output modules, it's assumed that addressing the general issue in a common location is preferrable over local adjustment of individual input modules or device drivers. Adjusting both places doesn't harm either, increases overall robustness. The implementation results in a negligable runtime overhead for the regular case of matching unit sizes (a few integer checks). And an acceptable overhead when the session feed is wider than the srzip archive's unit size (multiple memcpy() calls where previously was only one which would have been incorrect without the consistency check). The code path which needs to apply padding to the output is most expensive, but the implementation is only as expensive as it needs to be. The added cost only occurs in the case of mismatches, which were not handled at all before this change. The combination of extensive diagnostics and internal consistency checks shall increase robustness and help during future maintenance. Reported-By: Pavel Fedin --- diff --git a/src/output/srzip.c b/src/output/srzip.c index 76a2d5f2..c1e2e438 100644 --- a/src/output/srzip.c +++ b/src/output/srzip.c @@ -396,21 +396,61 @@ static int zip_append(const struct sr_output *o, * @returns SR_OK et al error codes. */ static int zip_append_queue(const struct sr_output *o, - const uint8_t *buf, size_t unitsize, size_t length, + const uint8_t *buf, size_t feed_unitsize, size_t length, gboolean flush) { + static gboolean sizes_seen; + struct out_context *outc; struct logic_buff *buff; + size_t sample_copy_size, sample_skip_size, sample_pad_size; size_t send_count, remain, copy_count; const uint8_t *rdptr; uint8_t *wrptr; int ret; + /* + * Check input parameters. Prepare to either grab data as is, + * or to adjust between differing input and output unit sizes. + * Diagnostics is rate limited for improved usability, assumes + * that session feeds are consistent across calls. Processing + * would cope with inconsistent calls though when required. + */ outc = o->priv; buff = &outc->logic_buff; - if (length && unitsize != buff->zip_unit_size) { - sr_warn("Unexpected unit size, discarding logic data."); - return SR_ERR_ARG; + if (length) { + if (!sizes_seen) { + sr_info("output unit size %zu, feed unit size %zu.", + buff->zip_unit_size, feed_unitsize); + } + if (feed_unitsize > buff->zip_unit_size) { + if (!sizes_seen) + sr_info("Large unit size, discarding excess logic data."); + sample_copy_size = buff->zip_unit_size; + sample_skip_size = feed_unitsize - buff->zip_unit_size; + sample_pad_size = 0; + } else if (feed_unitsize < buff->zip_unit_size) { + if (!sizes_seen) + sr_info("Small unit size, padding logic data."); + sample_copy_size = feed_unitsize; + sample_skip_size = 0; + sample_pad_size = buff->zip_unit_size - feed_unitsize; + } else { + if (!sizes_seen) + sr_dbg("Matching unit size, passing logic data as is."); + sample_copy_size = buff->zip_unit_size; + sample_skip_size = 0; + sample_pad_size = 0; + } + if (sample_copy_size + sample_skip_size != feed_unitsize) { + sr_err("Inconsistent input unit size. Implementation flaw?"); + return SR_ERR_BUG; + } + if (sample_copy_size + sample_pad_size != buff->zip_unit_size) { + sr_err("Inconsistent output unit size. Implementation flaw?"); + return SR_ERR_BUG; + } + sizes_seen = TRUE; } /* @@ -418,16 +458,24 @@ static int zip_append_queue(const struct sr_output *o, * Flush to the ZIP archive when the buffer space is exhausted. */ rdptr = buf; - send_count = buff->zip_unit_size ? length / buff->zip_unit_size : 0; + send_count = feed_unitsize ? length / feed_unitsize : 0; while (send_count) { remain = buff->alloc_size - buff->fill_size; if (remain) { wrptr = &buff->samples[buff->fill_size * buff->zip_unit_size]; copy_count = MIN(send_count, remain); + if (sample_skip_size || sample_pad_size) + copy_count = 1; send_count -= copy_count; buff->fill_size += copy_count; - memcpy(wrptr, rdptr, copy_count * buff->zip_unit_size); - rdptr += copy_count * buff->zip_unit_size; + memcpy(wrptr, rdptr, copy_count * sample_copy_size); + if (sample_pad_size) { + wrptr += sample_copy_size; + memset(wrptr, 0, sample_pad_size); + } + rdptr += copy_count * sample_copy_size; + if (sample_skip_size) + rdptr += sample_skip_size; remain -= copy_count; } if (send_count && !remain) {