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 <redacted>

diff --git a/src/output/srzip.c b/src/output/srzip.c
index 76a2d5f2485ef7e3bc5f23df8309ebc33ecfd466..c1e2e438dac078e32c9d7c3e7155bffcdabed03c 100644 (file)
--- 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) {