input: add import module for tektronix isf file format #186

filipkosecek · 2022-06-25T11:08:47Z

No description provided.

acassis · 2022-06-28T14:23:33Z

src/input/isf.c

+    size_t i, channel_ix;
+
+    channel_ix = 0;
+    /* Isf wfid looks something like WFID "Ch1, ..."; thus we must skip character '"' */


Suggested change

/* Isf wfid looks something like WFID "Ch1, ..."; thus we must skip character '"' */

/* If wfid looks something like WFID "Ch1, ..."; thus we must skip character '"' */

Sorry for being inactive for a long time. This comment was supposed to mean that WFID follows this pattern in ISF format. So Isf was not a typing error, in case I understand your review correctly.

suggest writing acronyms in all-caps ?, "ISF WFID" looks a lot less accidental

acassis · 2022-06-30T12:39:31Z

ping @filipkosecek

fenugrec

very sparsely commented, so that without a prior knowledge of the format details, it's hard to properly review the implementation.

Instead of merging master back into this branch, you should probably be rebasing on top of master.

disclaimer : I am neither maintainer nor committer in this project.

fenugrec · 2022-12-17T13:31:24Z

src/input/input.c

@@ -89,6 +90,7 @@ static const struct sr_input_module *input_module_list[] = {
 	&input_raw_analog,
 	&input_logicport,
 	&input_saleae,
+    &input_isf,


indentation doesn't seem to fit with other lines here

fenugrec · 2022-12-17T13:33:40Z

src/input/isf.c

+    float ymult;
+    float xincr;
+    int bytnr;
+    int byte_order;


why not "enum byteorder byte_order " ? you defined nice enums but are not using them here...

fenugrec · 2022-12-17T13:36:21Z

src/input/isf.c

+{
+    char value[10];
+
+    find_string_value(beg, value, 9);


not a fan of hardcoding an array length like this. Either a macro for the buffer size, or using sizeof, would be an improvement IMO

fenugrec · 2022-12-17T13:37:09Z

src/input/isf.c

+        inc->channel_name[channel_ix++] = beg[i++];
+}
+
+static void find_string_value(const char *beg, char *value, size_t value_len)


for these helper functions, even though they're simple enough to understand, would benefit from a short comment above to describe the purpose, args, etc

fenugrec · 2022-12-17T13:40:08Z

src/input/isf.c

+    return SR_OK;
+}
+
+static int format_match(GHashTable *metadata, unsigned int *confidence)


would help to clarify what values "confidence" can return. Higher values are better ? what is the expected range of returned values?

fenugrec · 2022-12-17T13:51:04Z

src/input/isf.c

+        }
+    }
+
+    if(value & (1 << (8*inc->bytnr-1))){


unclear what this is supposed to do

fenugrec · 2022-12-17T13:51:57Z

src/input/isf.c

+
+    inc = in->priv;
+    bytnr = inc->bytnr;
+    memcpy(data, in->buf->str + offset, inc->bytnr);


no bounds checking on inc->bytnr vs sizeof data[]

fenugrec · 2022-12-17T13:52:14Z

src/input/isf.c

+    bytnr = inc->bytnr;
+    g_assert(bytnr == 4);
+    fp.i = 0;
+    memcpy(data, in->buf->str + offset, 4);


hardcoded size arg

fenugrec · 2022-12-17T13:54:43Z

src/input/isf.c

+    fdata = g_malloc0(sizeof(float) * num_samples);
+    for(i = 0; i < num_samples; ++i){
+        if(inc->bn_fmt == RI){
+            fdata[i] = ((float) read_int_value(in, offset) - inc->yoff) * inc->ymult + inc->yzero;


why are you calling read_int_value and casting it to float ?

Function read_int_value returns an integer and a float value inc->yoff is then subtracted from the result. In my opinion, it should be cast to float but I may be wrong.

fenugrec · 2022-12-17T13:56:05Z

src/input/isf.c

+    for(i = 0; i < num_samples; ++i){
+        if(inc->bn_fmt == RI){
+            fdata[i] = ((float) read_int_value(in, offset) - inc->yoff) * inc->ymult + inc->yzero;
+        }else if(inc->bn_fmt == FP)


whenever you use braces in the first if {} clause, recommend having braces for all others. Can't remember what the kernel coding style guide says about this though

marcows · 2023-01-22T00:23:40Z

I'm not quite convinced of the ISF file format as it seems possible to differ dependent on the device. However, the import of my .isf example was more than twice as fast as the same data from .csv file and you don't need to configure the import.

There are a few coding style violations which happen throughout the isf.c file, affected rules are:

use tabs for indentation, not spaces
add a space between statement and opening parenthesis
add a space before opening brace in struct declaration
put the case statement to the same indentation level as its switch counterpart

As was already mentioned, the merge commit doesn't belong here.

marcows

I didn't make a full review, just focussed on file format specific stuff.

marcows · 2023-01-22T00:48:23Z

src/input/isf.c

+    for(i = 0; i < HEADER_ITEMS_PARAMETERS; ++i){
+        pattern = find_item(buf, header_items[i]);
+        if(pattern == NULL)
+            return SR_ERR_DATA;


Suggested change

return SR_ERR_DATA;

/* Ignore unknown command/header. */

continue;

With this change, the .isf from Tektronix DPO 4034 can be imported properly, before it aborted.

The .isf file begins as follows:

:WFMPRE:NR_PT 1000000;:WFMPRE:BYT_NR 2;BIT_NR 16;ENCDG BINARY;BN_FMT RI;BYT_OR MSB;WFID "Ch1, DC coupling, 1.000V/div, 400.0ms/div, 1000000 points, Sample mode";NR_PT 1000000;PT_FMT Y;XUNIT "s";XINCR 4.0000E-6;XZERO -2.0000;PT_OFF 0;YUNIT "V";YMULT 156.2500E-6;YOFF -25.6000E+3;YZERO 0.0E+0;VSCALE 1.0000;HSCALE 400.0000E-3;VPOS -4.0000;VOFFSET 0.0E+0;HDELAY 0.0E+0;:CURVE #72000000

Formatted for improved readability:

:WFMPRE:NR_PT 1000000; :WFMPRE:BYT_NR 2; BIT_NR 16; ENCDG BINARY; BN_FMT RI; BYT_OR MSB; WFID "Ch1, DC coupling, 1.000V/div, 400.0ms/div, 1000000 points, Sample mode"; NR_PT 1000000; PT_FMT Y; XUNIT "s"; XINCR 4.0000E-6; XZERO -2.0000; PT_OFF 0; YUNIT "V"; YMULT 156.2500E-6; YOFF -25.6000E+3; YZERO 0.0E+0; VSCALE 1.0000; HSCALE 400.0000E-3; VPOS -4.0000; VOFFSET 0.0E+0; HDELAY 0.0E+0; :CURVE #72000000

In principle sigrok-cli generates the same output as with .csv, but there are only two instead of three digits after decimal point (but the third digit is always 0 in my case). I'm not sure if that matters:

.csv:

META samplerate: 250000 CH1: 4.880 CH1: 4.840 ...

.isf:

META samplerate: 250000 Ch1: 4.88 Ch1: 4.84 ...

marcows · 2023-01-22T00:58:42Z

src/input/isf.c

+
+    find_string_value(beg, value, 9);
+
+    if(strcmp(value, "BINARY") != 0){


In https://www.tek.com/en/support/faqs/what-format-isf-file there is the following preamble (formatted for readability):

:WFMPRE:BYT_NR 2; BIT_NR 16; ENCDG BIN; BN_FMT RI; BYT_OR MSB; NR_PT 10000; WFID "Ch1, DC coupling, 2.0E0 V/div, 1.0E-5 s/div, 10000 points, Sample mode"; PT_FMT Y; XINCR 1.0E-8; PT_OFF 0; XZERO 3.5E-4; XUNIT "s"; YMULT 3.125E-4; YZERO 0.0E0; YOFF 0.0E0; YUNIT "V"; :CURVE #520000

Here the argument for ENCDG is BIN. I read a bit in MSO4000 and DPO4000 Series Digital Phosphor Oscilloscopes Programmer Manual and noticed some commands or keywords can be shortened. E.g. BIN/BINARY is specified as BINary, so the ary part is optional (I'm not sure if BINa or BINar would/should work).

Conclusion: It might be possible that some oscilloscope models don't use the full name.

marcows · 2023-01-22T01:08:11Z

src/input/isf.c

+        case BN_FMT:
+            if(strncmp(beg, "RI", 2) == 0)
+                inc->bn_fmt = RI;
+            else if(strncmp(beg, "FP", 2) == 0)


In MSO4000 and DPO4000 Series Digital Phosphor Oscilloscopes Programmer Manual there are RI (signed integer) and RP (positive integer), see WFMOutpre:BN_Fmt.

Where did you get FP (floating point)?

@marcows I used Tektronix TDS5000B Series Online Programmer Manual which specifies FP (floating point).

Add comments, conform to the kernel coding style and add bounds checking in ISF import module.

Don't require WFID in ISF header and increase minimum amount of bytes required to process the header. Rename members of struct context to be more descriptive.

Don't perform header parsing until "CURVE#" (indicating data section start) is found. This approach doesn't require minimum header size. If a large amount of bytes has been loaded and "CURVE#" still has not been found, an error code is returned.

Rename variables and make comments more descriptive.

fenugrec · 2023-06-23T18:17:10Z

src/input/isf.c

+
+	find_string_value(buf, buflen, value, MAX_ENCODING_STRING_SIZE);
+
+	/* "BIN" and "BINARY" are accepted as suggested in a pull request comment. */


probably not necessary to specify that the idea was from a PR comment

fenugrec · 2023-06-23T18:19:15Z

src/input/isf.c

+	channel_ix = 0;
+	/* ISF WFID looks something like WFID "Ch1, ..."; hence we must skip character '"' */
+	i = 1;
+	while (i < buflen && buf[i] != ',' && buf[i] != '"' && channel_ix < MAX_CHANNEL_NAME_SIZE - 1)


not a fan of this : very long line, with many conditions, and the following instruction block isn't braced. Makes this hard to process if editor wraps the line due to limited width

I braced the instruction block and wrapped the condition line.

fenugrec · 2023-06-23T18:20:55Z

src/input/isf.c

+		return NULL;
+
+	/* Curve metadata length is an ASCII byte, hence -48. */
+	metadata_length = (size_t) *data_ptr - 48;


do you mean the length is encoded as an ascii digit '0' to '9' ? or that they simply mapped lengths 0 to (255-48) to chars 0x30 to 0xff ?

Suggest bounds checking before subtracting, in case of malformed packet.

The length is encoded as an ascii digit '0' to '9'. Fixed the comment and bounds checking is performed.

Instead of using 48 you should use '0' to make this obvious.

Instead of using 48 you should use '0' to make this obvious.

Agreed. This is something often written if (val > '9' || val < '0') return bad; metadata_length = val - '0'

Replaced numeric values with character values to make it obvious.

fenugrec · 2023-06-23T18:27:02Z

src/input/isf.c

+ */
+static int format_match(GHashTable *metadata, unsigned int *confidence)
+{
+	const char default_extension[] = ".isf";


suggest to do extension matching with non-case-sensitive functions (some files may end up with uppercase .ISF and then fail on linux/mac)

The extension matching is now done using a non-case-sensitive function.

fenugrec · 2023-06-23T18:28:15Z

src/input/isf.c

+	bytnr = inc->bytnr;
+
+	/* Value bytnr is checked in function "receive". */
+	memcpy(data, in->buf->str + offset, bytnr);


bounds check on bytnr before memcpy ? not sure if that's what you mean in the comment on the previous line
[EDIT] ok, yes I see now, it is already checked. But I would probably just replace that comment with the bounds check, even if it's redundant - very inexpensive precaution and 1% easier to review

Bounds checking on bytnr is now performed.

First try to find "NR_PT" header item and then increase the confidence if the file extension matches. The extension comparison is performed ignoring the case of the characters.

fenugrec · 2023-06-26T03:24:15Z

src/input/isf.c

+
+	/* Increase the confidence if the extension is '.isf'. */
+	fn = g_hash_table_lookup(metadata, GINT_TO_POINTER(SR_INPUT_META_FILENAME));
+	if (fn != NULL && (fn_len = strlen(fn)) >= strlen(default_extension)) {


Also, I didn't catch this last time but you have an assignment inside a conditional, which should be split (I think that is now mentioned in the HACKING doc)

Use character values instead of numeric values to obtain curve metadata.

marcows · 2024-02-06T22:21:05Z

src/input/isf.c

+		if (pattern == NULL) {
+			/* WFID is not required. */
+			if (i == WFID)
+				continue;


For me (.isf created by Tektronix DPO 4034) "WFMTYPE" was the problem, not "WFID", as could be determined from the .isf header I had posted:
#186 (comment)

The change in your previous commit (the removed lines above had been added there), which ignored all unknown header items (proposal from my review), worked for me. After this change it didn't work anymore.

The thing is that it is pretty hard to identify which header items should be mandatory as the header slightly varies depending on the device used.

Do you think it would be better to initialize header items to default values and make them all optional?

I'm not sure because I didn't had a deep look at some specifications. But I don't think there are sensible default values possible in most cases.

I couldn't find WFMTYPE in the manual linked in the comment at the beginning of isf.c file.

The default option for WFMTYPE could be ANALOG value. The other options are probably rarely used since they are used for radio frequency data. The options are specified in this manual on page 2-586.

input: add import module for tektronix isf file format

f5ab583

acassis reviewed Jun 28, 2022

View reviewed changes

fenugrec reviewed Dec 17, 2022

View reviewed changes

marcows reviewed Jan 22, 2023

View reviewed changes

input: fix errors in ISF import module based on pull request reviews

741a37b

Add comments, conform to the kernel coding style and add bounds checking in ISF import module.

filipkosecek force-pushed the master branch from 1f81da0 to 741a37b Compare June 7, 2023 20:34

filipkosecek added 7 commits June 8, 2023 14:24

input: make WFID an optional parameter in ISF input module

166b486

Don't require WFID in ISF header and increase minimum amount of bytes required to process the header. Rename members of struct context to be more descriptive.

input: add support for unsigned integer data format in ISF module

e36ebb0

input: isf.c: make find_string_value more readable

4f87b65

input: isf.c add input data bounds checking

2825e37

input: isf.c add bounds checking for float values

0f1e957

input: isf.c don't set confidence when the format does not match

2407599

filipkosecek force-pushed the master branch from 3d05edd to 10fe75e Compare June 11, 2023 19:52

input: isf.c add a brief ISF format description

d8b8f9c

filipkosecek force-pushed the master branch from 10fe75e to d8b8f9c Compare June 13, 2023 09:01

filipkosecek added 3 commits June 13, 2023 12:16

input/isf.c: perform bounds cheking on bytes per sample value

332fafd

input/isf.c: add error handling when parsing numeric header items

482955d

input/isf.c: improve readability

94e475a

Rename variables and make comments more descriptive.

filipkosecek requested review from acassis, marcows and fenugrec June 14, 2023 12:04

fenugrec reviewed Jun 23, 2023

View reviewed changes

filipkosecek added 5 commits June 25, 2023 20:42

input/isf.c: wrap the long condition line

3704fc1

input/isf.c: modify the comment

8b9d1e1

input/isf.c: perform bounds checking on curve metadata length

2081200

input/isf.c: reimplement format_match function

228a5ee

First try to find "NR_PT" header item and then increase the confidence if the file extension matches. The extension comparison is performed ignoring the case of the characters.

input/isf.c: perform bounds checking when reading data samples

17b954f

fenugrec reviewed Jun 26, 2023

View reviewed changes

filipkosecek added 2 commits June 26, 2023 22:41

input/isf.c: remove assignment in conditional expressions

e9f509b

input/isf.c: make obtaining curve metadata more obvious

af06f48

Use character values instead of numeric values to obtain curve metadata.

filipkosecek closed this Oct 4, 2023

marcows mentioned this pull request Feb 6, 2024

input: add import module for Tektronix ISF file format #238

Open

marcows reviewed Feb 6, 2024

View reviewed changes

	/* Isf wfid looks something like WFID "Ch1, ..."; thus we must skip character '"' */
	/* If wfid looks something like WFID "Ch1, ..."; thus we must skip character '"' */

	return SR_ERR_DATA;
	/* Ignore unknown command/header. */
	continue;


		find_string_value(beg, value, 9);

		if(strcmp(value, "BINARY") != 0){


		find_string_value(buf, buflen, value, MAX_ENCODING_STRING_SIZE);

		/* "BIN" and "BINARY" are accepted as suggested in a pull request comment. */

input: add import module for tektronix isf file format #186

input: add import module for tektronix isf file format #186

Conversation

filipkosecek commented Jun 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acassis commented Jun 30, 2022

fenugrec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fenugrec Dec 17, 2022 • edited

Choose a reason for hiding this comment

fenugrec Dec 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcows commented Jan 22, 2023

marcows left a comment

Choose a reason for hiding this comment

marcows Jan 22, 2023 • edited

Choose a reason for hiding this comment

marcows Jan 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipkosecek May 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipkosecek Jun 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipkosecek Jun 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fenugrec Dec 17, 2022 •

edited

fenugrec Dec 17, 2022 •

edited

marcows Jan 22, 2023 •

edited

marcows Jan 22, 2023 •

edited

filipkosecek May 31, 2023 •

edited

filipkosecek Jun 28, 2023 •

edited

filipkosecek Jun 25, 2023 •

edited