Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 20 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
f5ab583
input: add import module for tektronix isf file format
filipkosecek Jun 14, 2022
741a37b
input: fix errors in ISF import module based on pull request reviews
filipkosecek May 30, 2023
166b486
input: make WFID an optional parameter in ISF input module
filipkosecek Jun 8, 2023
e36ebb0
input: add support for unsigned integer data format in ISF module
filipkosecek Jun 8, 2023
4f87b65
input: isf.c: make find_string_value more readable
filipkosecek Jun 11, 2023
2825e37
input: isf.c add input data bounds checking
filipkosecek Jun 11, 2023
0f1e957
input: isf.c add bounds checking for float values
filipkosecek Jun 11, 2023
60b2ac0
input: isf.c Don't require minimum header size
filipkosecek Jun 11, 2023
2407599
input: isf.c don't set confidence when the format does not match
filipkosecek Jun 11, 2023
d8b8f9c
input: isf.c add a brief ISF format description
filipkosecek Jun 11, 2023
332fafd
input/isf.c: perform bounds cheking on bytes per sample value
filipkosecek Jun 13, 2023
482955d
input/isf.c: add error handling when parsing numeric header items
filipkosecek Jun 13, 2023
94e475a
input/isf.c: improve readability
filipkosecek Jun 14, 2023
3704fc1
input/isf.c: wrap the long condition line
filipkosecek Jun 25, 2023
8b9d1e1
input/isf.c: modify the comment
filipkosecek Jun 25, 2023
2081200
input/isf.c: perform bounds checking on curve metadata length
filipkosecek Jun 25, 2023
228a5ee
input/isf.c: reimplement format_match function
filipkosecek Jun 25, 2023
17b954f
input/isf.c: perform bounds checking when reading data samples
filipkosecek Jun 25, 2023
e9f509b
input/isf.c: remove assignment in conditional expressions
filipkosecek Jun 26, 2023
af06f48
input/isf.c: make obtaining curve metadata more obvious
filipkosecek Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 22 additions & 17 deletions src/input/isf.c
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@
#define CHUNK_SIZE (4 * 1024 * 1024)

/* Number of bytes required to process the header. */
#define MIN_INITIAL_OFFSET 512
#define MIN_INITIAL_OFFSET 1024

/* Number of items in the header. */
#define HEADER_ITEMS_PARAMETERS 10
Expand Down Expand Up @@ -71,8 +71,7 @@ union floating_point {

struct context {
gboolean started;
gboolean found_header;
gboolean create_channel;
gboolean found_data_section;
float yoff;
float yzero;
float ymult;
Expand All @@ -84,7 +83,7 @@ struct context {
char channel_name[MAX_CHANNEL_NAME_SIZE];
};

/* Header items required to process the input file. */
/* Header items used to process the input file. */
enum header_items_enum {
YOFF = 0,
YZERO = 1,
Expand Down Expand Up @@ -187,7 +186,7 @@ static int find_encoding(const char *beg)
find_string_value(beg, value, MAX_ENCODING_STRING_SIZE - 1);

/* TODO: Use strncmp instead. */
/* TODO: "BIN" vs "BINARY" as suggested in the github comment. */
/* "BIN" and "BINARY" are accepted as suggested in a pull request comment. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (strcmp(value, "BINARY") != 0 && strcmp(value, "BIN") != 0) {
sr_err("Only binary encoding supported.");
return SR_ERR_NA;
Expand Down Expand Up @@ -287,12 +286,16 @@ static int parse_isf_header(GString *buf, struct context *inc)

if (inc == NULL)
return SR_ERR_ARG;


/* Search for all header items. */
for (i = 0; i < HEADER_ITEMS_PARAMETERS; ++i) {
pattern = find_item(buf, header_items[i]);
if (pattern == NULL)
/* Ignore unknown command/header. */
continue;
if (pattern == NULL) {
/* WFID is not required. */
if (i == WFID)
continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return SR_ERR_DATA;
}

ret = process_header_item(pattern + strlen(header_items[i]), inc, i);
if (ret != SR_OK)
Expand Down Expand Up @@ -350,8 +353,7 @@ static int init(struct sr_input *in, GHashTable *options)
in->priv = g_malloc0(sizeof(struct context));

inc = in->priv;
inc->found_header = FALSE;
inc->create_channel = TRUE;
inc->found_data_section = FALSE;
memset(inc->channel_name, 0, MAX_CHANNEL_NAME_SIZE);

return SR_OK;
Expand All @@ -372,8 +374,8 @@ static int64_t read_int_value(struct sr_input *in, size_t offset)

inc = in->priv;
bytnr = inc->bytnr;
/* TODO: Perform proper bound checking. */
g_assert(bytnr <= 8);
/* TODO: Perform proper bounds checking. */
g_assert(bytnr <= 8 && bytnr <= sizeof(data));
memcpy(data, in->buf->str + offset, bytnr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bounds checking on bytnr is now performed.

value = 0;
if (inc->byte_order == MSB) {
Expand Down Expand Up @@ -486,14 +488,15 @@ static int process_buffer(struct sr_input *in)
inc->started = TRUE;
}

if (!inc->found_header) {
/* Set offset to the data section beginning. */
if (!inc->found_data_section) {
data = find_data_section(in->buf);
if (data == NULL) {
sr_err("Couldn't find data section.");
return SR_ERR;
}
offset = data - in->buf->str;
inc->found_header = TRUE;
inc->found_data_section = TRUE;
} else {
offset = 0;
}
Expand Down Expand Up @@ -546,8 +549,10 @@ static int receive(struct sr_input *in, GString *buf)
return SR_ERR_NA;
}

if (inc->create_channel)
sr_channel_new(in->sdi, 0, SR_CHANNEL_ANALOG, TRUE, inc->channel_name);
/* Set default channel name. */
if (strlen(inc->channel_name) == 0)
snprintf(inc->channel_name, MAX_CHANNEL_NAME_SIZE, "CH");
sr_channel_new(in->sdi, 0, SR_CHANNEL_ANALOG, TRUE, inc->channel_name);

in->sdi_ready = TRUE;
return SR_OK;
Expand Down