From: Gerhard Sittig <redacted>
Date: Sat, 22 Aug 2020 15:57:55 +0000 (+0200)
Subject: output/csv: check unsupported/untested input signal conditions
X-Git-Url: https://sigrok.org/gitweb/?p=libsigrok.git;a=commitdiff_plain;h=ce384e074f41685bad83c3cef10ea1d635a2834d

output/csv: check unsupported/untested input signal conditions

The current implementation of the CSV output module makes assumptions
which don't hold. Which results in incorrect or incomplete output for
some combinations of logic and analog signals.

Check for some of the known problematic conditions, and warn the user
about potentially unexpected results. This is a workaround until the
issues properly get addressed in the implementation.

This is motivated by but does not resolve bug #1026.
---

diff --git a/src/output/csv.c b/src/output/csv.c
index 296d9a06..49d6eb6e 100644
--- a/src/output/csv.c
+++ b/src/output/csv.c
@@ -101,6 +101,11 @@ struct context {
 	uint8_t *logic_samples;
 	const char *xlabel;	/* Don't free: will point to a static string. */
 	const char *title;	/* Don't free: will point into the driver struct. */
+
+	/* Input data constraints check. */
+	gboolean have_checked;
+	gboolean have_frames;
+	uint64_t pkt_snums;
 };
 
 /*
@@ -569,10 +574,91 @@ static void save_gnuplot(struct context *ctx)
 	g_string_free(script, TRUE);
 }
 
+static void check_input_constraints(struct context *ctx)
+{
+	size_t snum_count, snum_warn_limit;
+	size_t logic, analog;
+	gboolean has_frames, is_short, is_mixed, is_multi_analog;
+	gboolean do_warn;
+
+	/*
+	 * Check and conditionally warn exactly once during execution
+	 * of the output module on a set of input data.
+	 */
+	if (ctx->have_checked)
+		return;
+	ctx->have_checked = TRUE;
+
+	/*
+	 * This implementation of the CSV output module assumes some
+	 * constraints which need not be met in reality. Emit warnings
+	 * until a better version becomes available. Letting users know
+	 * that their request may not get processed correctly is the
+	 * only thing we can do for now except for complete refusal to
+	 * process the input data.
+	 *
+	 * What the implementation appears to assume (unverified, this
+	 * interpretation may be incorrect and/or incomplete):
+	 * - Multi-channel analog data, or mixed signal input, always
+	 *   is enclosed in frame markers.
+	 * - Data which gets received across several packets spans a
+	 *   consistent sample number range. All samples of one frame
+	 *   and channel number or data type fit into a single packet.
+	 *   Arbitrary chunking seems to not be supported.
+	 * - A specific order of analog data packets is assumed.
+	 *
+	 * With these assumptions encoded in the implementation, and
+	 * not being met at runtime, incorrect and unexpected results
+	 * were seen for these configurations:
+	 * - More than one analog channel.
+	 * - The combination of logic and analog channel types.
+	 *
+	 * The condition of frames with large sample counts is a wild
+	 * guess, the limit is a totally arbitrary choice. It assumes
+	 * typical scope frames with at most a few thousand samples per
+	 * frame, and assumes that a channel's data gets sent in large
+	 * enough packets. The absence of a warning message does not
+	 * necessarily translate to correct output, it's more of a rate
+	 * limiting approach to not scare users too much.
+	 */
+	snum_count = ctx->pkt_snums;
+	snum_warn_limit = 1 * 1000 * 1000;
+	logic = ctx->num_logic_channels;
+	analog = ctx->num_analog_channels;
+	has_frames = ctx->have_frames;
+	is_short = snum_count < snum_warn_limit;
+	is_mixed = logic && analog;
+	is_multi_analog = analog > 1;
+
+	if (has_frames && is_short) {
+		sr_info("Assuming consistent framed input data.");
+		return;
+	}
+
+	do_warn = FALSE;
+	if (has_frames) {
+		sr_warn("Untested configuration: large frame content.");
+		do_warn = TRUE;
+	}
+	if (is_mixed) {
+		sr_warn("Untested configuration: mixed signal input data.");
+		do_warn = TRUE;
+	}
+	if (is_multi_analog) {
+		sr_warn("Untested configuration: multi-channel analog data.");
+		do_warn = TRUE;
+	}
+	if (!do_warn)
+		return;
+	sr_warn("Resulting CSV output data may be incomplete or incorrect.");
+}
+
 static int receive(const struct sr_output *o,
 		   const struct sr_datafeed_packet *packet, GString **out)
 {
 	struct context *ctx;
+	const struct sr_datafeed_logic *logic;
+	const struct sr_datafeed_analog *analog;
 
 	*out = NULL;
 	if (!o || !o->sdi)
@@ -583,18 +669,30 @@ static int receive(const struct sr_output *o,
 	sr_dbg("Got packet of type %d", packet->type);
 	switch (packet->type) {
 	case SR_DF_HEADER:
+		ctx->have_checked = FALSE;
+		ctx->have_frames = FALSE;
+		ctx->pkt_snums = FALSE;
 		*out = gen_header(o, packet->payload);
 		break;
 	case SR_DF_TRIGGER:
 		ctx->trigger = TRUE;
 		break;
 	case SR_DF_LOGIC:
-		process_logic(ctx, packet->payload);
+		logic = packet->payload;
+		ctx->pkt_snums = logic->length;
+		ctx->pkt_snums /= logic->length;
+		check_input_constraints(ctx);
+		process_logic(ctx, logic);
 		break;
 	case SR_DF_ANALOG:
-		process_analog(ctx, packet->payload);
+		analog = packet->payload;
+		ctx->pkt_snums = analog->num_samples;
+		ctx->pkt_snums /= g_slist_length(analog->meaning->channels);
+		check_input_constraints(ctx);
+		process_analog(ctx, analog);
 		break;
 	case SR_DF_FRAME_BEGIN:
+		ctx->have_frames = TRUE;
 		*out = g_string_new(ctx->frame);
 		/* Fallthrough */
 	case SR_DF_END: