From 5e6a040273d5b6aadb77a2a8736bc32b08d74649 Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Sun, 6 Feb 2022 21:32:28 +0100 Subject: [PATCH 01/16] show: use getter result to determine "current" presence (voltage range) Improve the condition which presents the "(current)" decoration when voltage ranges are listed in --show output. Use an explicit boolean which is derived from the config getter's return code. Avoid "forcing" a comparison against a zero value, which can be a valid item in the list of supported voltages. This is similar to commit 0171a4a7a49f. See more context (-U10) for the motivation of the change. Was observed with the kingst-la2016 driver. --- show.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/show.c b/show.c index 5243e53..85b4409 100644 --- a/show.c +++ b/show.c @@ -418,7 +418,7 @@ void show_dev_detail(void) char *tmp_str, *s, c; const char **stropts; double tmp_flt; - gboolean have_tmp_flt; + gboolean have_tmp_flt, have_curr; const double *fltopts; if (parse_driver(opt_drv, &driver_from_opt, NULL)) { @@ -718,12 +718,11 @@ void show_dev_detail(void) continue; } + have_curr = FALSE; if (maybe_config_get(driver, sdi, channel_group, key, &gvar) == SR_OK) { g_variant_get(gvar, "(dd)", &dcur_low, &dcur_high); g_variant_unref(gvar); - } else { - dcur_low = 0; - dcur_high = 0; + have_curr = TRUE; } num_elements = g_variant_n_children(gvar_list); @@ -734,6 +733,8 @@ void show_dev_detail(void) if (i) printf(", "); printf("%.1f-%.1f", dlow, dhigh); + if (!have_curr) + continue; if (dlow == dcur_low && dhigh == dcur_high) printf(" (current)"); } -- 2.30.2 From 525f48143fc004d104b2e3fb91c6e5bb2a1064df Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Tue, 22 Feb 2022 23:06:36 +0100 Subject: [PATCH 02/16] doc: update uart decoder options example in sigrok-cli(1) manpage Catch up with renamed UART decoder options. Reported-By: via IRC --- doc/sigrok-cli.1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/sigrok-cli.1 b/doc/sigrok-cli.1 index 4258009..b333bee 100644 --- a/doc/sigrok-cli.1 +++ b/doc/sigrok-cli.1 @@ -292,7 +292,7 @@ Example: $ .B "sigrok\-cli \-i " .br -.B " \-P uart:baudrate=115200:parity_type=odd" +.B " \-P uart:baudrate=115200:parity=odd" .sp The list of supported options depends entirely on the protocol decoder. Every protocol decoder has different options it supports. -- 2.30.2 From 2b29fb3954da8ede79ade2e4940f95b3ef836d8f Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Thu, 7 Apr 2022 23:12:34 +0200 Subject: [PATCH 03/16] input: unbreak automatic format detection when reading from stdin The combination of reading from stdin and automatic file format match could result in the execution of an input module's .end() callback before its .receive() ever executed. Which then suffers from an sdi which is not yet ready, and no data was sent to the sigrok session. Fix that. Unfortunately the input module's accumulator is hidden behind the libsigrok API. That's why applications cannot defer the forwarding of the first data chunk from the input file. Insert a .receive() call with a zero length instead before more file content is consumed. All existing input modules are prepared to handle this call sequence. --- input.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/input.c b/input.c index 6c14972..eb496ee 100644 --- a/input.c +++ b/input.c @@ -44,6 +44,7 @@ static void load_input_file_module(struct df_arg_desc *df_arg) ssize_t len; char *mod_id; gboolean is_stdin; + gboolean push_scan_data; if (!sr_input_list()) g_critical("No supported input formats available."); @@ -56,6 +57,7 @@ static void load_input_file_module(struct df_arg_desc *df_arg) } is_stdin = strcmp(opt_input_file, "-") == 0; + push_scan_data = FALSE; fd = 0; buf = g_string_sized_new(CHUNK_SIZE); if (mod_id) { @@ -108,6 +110,7 @@ static void load_input_file_module(struct df_arg_desc *df_arg) g_strerror(errno)); buf->len = len; sr_input_scan_buffer(buf, &in); + push_scan_data = TRUE; } if (!in) g_critical("Error: no input module found for this file."); @@ -116,15 +119,33 @@ static void load_input_file_module(struct df_arg_desc *df_arg) df_arg->session = session; sr_session_datafeed_callback_add(session, datafeed_in, df_arg); + /* + * Implementation detail: The combination of reading from stdin + * and automatic file format detection may have pushed the first + * chunk of input data into the input module's data accumulator, + * _bypassing_ the .receive() callback. It is essential to call + * .receive() before calling .end() for files of size smaller than + * CHUNK_SIZE (which is a typical case). So that sdi becomes ready. + * Fortunately all input modules accept .receive() calls with + * a zero length, and inspect whatever was accumulated so far. + * + * After that optional initial push of data which was queued + * above during format detection, continue reading remaining + * chunks from the input file until EOF is seen. + */ got_sdi = FALSE; while (TRUE) { g_string_truncate(buf, 0); - len = read(fd, buf->str, CHUNK_SIZE); + if (push_scan_data) + len = 0; + else + len = read(fd, buf->str, CHUNK_SIZE); if (len < 0) g_critical("Read failed: %s", g_strerror(errno)); - if (len == 0) + if (len == 0 && !push_scan_data) /* End of file or stream. */ break; + push_scan_data = FALSE; buf->len = len; if (sr_input_send(in, buf) != SR_OK) break; @@ -148,7 +169,6 @@ static void load_input_file_module(struct df_arg_desc *df_arg) df_arg->session = NULL; sr_session_destroy(session); - } void load_input_file(gboolean do_props) -- 2.30.2 From f3e44829a2de542c5da923db51d4f4377e401369 Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Sat, 23 Apr 2022 21:25:35 +0200 Subject: [PATCH 04/16] input: minor robustness improvement, close fd leak Eliminate redundant CHUNK_SIZE references, just fill the GString buffer as far as it was allocated no matter how. Call an input related free routine for options, not the output routine (which happened to work by coincidence, thus went unnoticed). Close the input file descriptor after the read loop, the caller has got no reference to it. --- input.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/input.c b/input.c index eb496ee..eafbd6f 100644 --- a/input.c +++ b/input.c @@ -68,7 +68,7 @@ static void load_input_file_module(struct df_arg_desc *df_arg) if ((options = sr_input_options_get(imod))) { mod_opts = generic_arg_to_opt(options, mod_args); (void)warn_unknown_keys(options, mod_args, NULL); - sr_output_options_free(options); + sr_input_options_free(options); } else { mod_opts = NULL; } @@ -105,7 +105,7 @@ static void load_input_file_module(struct df_arg_desc *df_arg) g_critical("Failed to load %s: %s.", opt_input_file, g_strerror(errno)); } - if ((len = read(fd, buf->str, CHUNK_SIZE)) < 1) + if ((len = read(fd, buf->str, buf->allocated_len)) < 1) g_critical("Failed to read %s: %s.", opt_input_file, g_strerror(errno)); buf->len = len; @@ -139,7 +139,7 @@ static void load_input_file_module(struct df_arg_desc *df_arg) if (push_scan_data) len = 0; else - len = read(fd, buf->str, CHUNK_SIZE); + len = read(fd, buf->str, buf->allocated_len); if (len < 0) g_critical("Read failed: %s", g_strerror(errno)); if (len == 0 && !push_scan_data) @@ -166,6 +166,7 @@ static void load_input_file_module(struct df_arg_desc *df_arg) sr_input_end(in); sr_input_free(in); g_string_free(buf, TRUE); + close(fd); df_arg->session = NULL; sr_session_destroy(session); -- 2.30.2 From a9b64d89d2509c095f7ed35fcfc78e146edde5d5 Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Sat, 23 Apr 2022 21:28:23 +0200 Subject: [PATCH 05/16] input: emit diagnostics when file import fails fatally The previous implementation did terminate the file import upon fatal errors, but did so silently. Add diagnostics to the code paths where input module's .send() calls or channel selection fail. This raises awareness when an individual input module would fail and not warn. --- input.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/input.c b/input.c index eafbd6f..cce0d9f 100644 --- a/input.c +++ b/input.c @@ -147,14 +147,18 @@ static void load_input_file_module(struct df_arg_desc *df_arg) break; push_scan_data = FALSE; buf->len = len; - if (sr_input_send(in, buf) != SR_OK) + if (sr_input_send(in, buf) != SR_OK) { + g_critical("File import failed (read)"); break; + } sdi = sr_input_dev_inst_get(in); if (!got_sdi && sdi) { /* First time we got a valid sdi. */ - if (select_channels(sdi) != SR_OK) + if (select_channels(sdi) != SR_OK) { + g_critical("File import failed (channels)"); return; + } if (sr_session_dev_add(session, sdi) != SR_OK) { g_critical("Failed to use device."); sr_session_destroy(session); -- 2.30.2 From e8e8b5e1a09563b66e9ef6d475b3c735c8fdc910 Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Sat, 23 Apr 2022 21:31:06 +0200 Subject: [PATCH 06/16] parsers: use proper conversion routine for PD option data types The generic_arg_to_opt() routine converted text input to protocol decoder option values, but might have lost significant information. Use a signed conversion routine for the int32 data, use a long long routine for uint64 data. This shall unbreak negative values (like UART packet separators), and improve compatibility with 32bit targets (like for large sample numbers or high frequency values). --- parsers.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/parsers.c b/parsers.c index d92218f..c1ab9bd 100644 --- a/parsers.c +++ b/parsers.c @@ -444,11 +444,11 @@ GHashTable *generic_arg_to_opt(const struct sr_option **opts, GHashTable *genarg g_hash_table_insert(hash, g_strdup(opts[i]->id), g_variant_ref_sink(gvar)); } else if (g_variant_is_of_type(opts[i]->def, G_VARIANT_TYPE_INT32)) { - gvar = g_variant_new_int32(strtoul(s, NULL, 10)); + gvar = g_variant_new_int32(strtol(s, NULL, 10)); g_hash_table_insert(hash, g_strdup(opts[i]->id), g_variant_ref_sink(gvar)); } else if (g_variant_is_of_type(opts[i]->def, G_VARIANT_TYPE_UINT64)) { - gvar = g_variant_new_uint64(strtoul(s, NULL, 10)); + gvar = g_variant_new_uint64(strtoull(s, NULL, 10)); g_hash_table_insert(hash, g_strdup(opts[i]->id), g_variant_ref_sink(gvar)); } else if (g_variant_is_of_type(opts[i]->def, G_VARIANT_TYPE_DOUBLE)) { -- 2.30.2 From 99595de1452934951c0d8311cf5dceef897f9860 Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Tue, 16 Aug 2022 22:14:23 +0200 Subject: [PATCH 07/16] parsers: use common library code to rename sigrok channels Drop an open coded g_free() and g_strdup() sequence which accesses internal details of a sigrok channel. Use the sr_dev_channel_name_set() library routine instead. --- parsers.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/parsers.c b/parsers.c index c1ab9bd..b74d74f 100644 --- a/parsers.c +++ b/parsers.c @@ -139,8 +139,7 @@ range_fail: } if (names[1]) { /* Rename channel. */ - g_free(ch->name); - ch->name = g_strdup(names[1]); + sr_dev_channel_name_set(ch, names[1]); } channellist = g_slist_append(channellist, ch); -- 2.30.2 From af9fa8c5ea9925cb8fe076ae7d4e996836a6db13 Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Wed, 31 Aug 2022 00:40:37 +0200 Subject: [PATCH 08/16] parsers: avoid NULL dereference when option strings are empty When colon separated options are scanned (like "-P ps2: --show") then the empty list item which results from the trailing colon resulted in key/value pairs becoming NULL. Handle that situation, skip empty list elements. Reported-By: Peter Mortensen via IRC --- parsers.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/parsers.c b/parsers.c index b74d74f..59a7a91 100644 --- a/parsers.c +++ b/parsers.c @@ -364,6 +364,8 @@ GHashTable *parse_generic_arg(const char *arg, i++; } for (; elements[i]; i++) { + if (!elements[i][0]) + continue; split_key_value(elements[i], &k, &v); k = g_strdup(k); v = v ? g_strdup(v) : NULL; -- 2.30.2 From 625606db65d118c9a246057297633d4c5f0c1571 Mon Sep 17 00:00:00 2001 From: Sergey Spivak Date: Fri, 12 Aug 2022 15:53:43 +0400 Subject: [PATCH 09/16] decode: Optionally print annotation class with PD annotations Introduce the "--protocol-decoder-ann-class" command line option (no short form available), which emits annotation class names with textual output from protocol decoder annotations, regardless of a log level value. Based on commit 08e9378bf68a by Gerhard Sittig --- decode.c | 2 ++ doc/sigrok-cli.1 | 3 +++ options.c | 3 +++ sigrok-cli.h | 1 + 4 files changed, 9 insertions(+) diff --git a/decode.c b/decode.c index 4ddf34a..ac0e3e1 100644 --- a/decode.c +++ b/decode.c @@ -684,6 +684,8 @@ void show_pd_annotations(struct srd_proto_data *pdata, void *cb_data) show_class = TRUE; show_abbrev = TRUE; } + if (opt_pd_ann_class) + show_class = TRUE; /* * Display the annotation's fields after the layout was diff --git a/doc/sigrok-cli.1 b/doc/sigrok-cli.1 index b333bee..b489928 100644 --- a/doc/sigrok-cli.1 +++ b/doc/sigrok-cli.1 @@ -426,6 +426,9 @@ Not every decoder generates binary output. When given, decoder annotations will include sample numbers, too. This allows consumers to receive machine readable timing information. .TP +.BR "\-\-protocol\-decoder\-ann\-class +When given, decoder annotations will include annotation class names. +.TP .BR "\-l, \-\-loglevel " Set the libsigrok and libsigrokdecode loglevel. At the moment \fBsigrok\-cli\fP doesn't support setting the two loglevels independently. The higher the diff --git a/options.c b/options.c index 2573bd4..e47b680 100644 --- a/options.c +++ b/options.c @@ -40,6 +40,7 @@ gchar **opt_pds = NULL; gchar *opt_pd_annotations = NULL; gchar *opt_pd_meta = NULL; gchar *opt_pd_binary = NULL; +gboolean opt_pd_ann_class = FALSE; gboolean opt_pd_samplenum = FALSE; gboolean opt_pd_jsontrace = FALSE; #endif @@ -139,6 +140,8 @@ static const GOptionEntry optargs[] = { "Protocol decoder meta output to show", NULL}, {"protocol-decoder-binary", 'B', 0, G_OPTION_ARG_CALLBACK, &check_opt_pd_binary, "Protocol decoder binary output to show", NULL}, + {"protocol-decoder-ann-class", 0, 0, G_OPTION_ARG_NONE, &opt_pd_ann_class, + "Show annotation class in decoder output", NULL}, {"protocol-decoder-samplenum", 0, 0, G_OPTION_ARG_NONE, &opt_pd_samplenum, "Show sample numbers in decoder output", NULL}, {"protocol-decoder-jsontrace", 0, 0, G_OPTION_ARG_NONE, &opt_pd_jsontrace, diff --git a/sigrok-cli.h b/sigrok-cli.h index 2c0fbdc..684aed9 100644 --- a/sigrok-cli.h +++ b/sigrok-cli.h @@ -140,6 +140,7 @@ extern gchar **opt_pds; extern gchar *opt_pd_annotations; extern gchar *opt_pd_meta; extern gchar *opt_pd_binary; +extern gboolean opt_pd_ann_class; extern gboolean opt_pd_samplenum; extern gboolean opt_pd_jsontrace; #endif -- 2.30.2 From 9a7a9200195ad0c7bab387a1f953119bece40823 Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Wed, 14 Dec 2022 17:52:42 +0100 Subject: [PATCH 10/16] manpage: add --protocol-decoder-jsontrace command line option Support for JSON formatted decoder output was introduced in commit 54614916a903 ("decode: add support for Google Trace Event output (JSON)") in 2020-07. Update the manpage. --- doc/sigrok-cli.1 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/sigrok-cli.1 b/doc/sigrok-cli.1 index b489928..47ec579 100644 --- a/doc/sigrok-cli.1 +++ b/doc/sigrok-cli.1 @@ -429,6 +429,10 @@ This allows consumers to receive machine readable timing information. .BR "\-\-protocol\-decoder\-ann\-class When given, decoder annotations will include annotation class names. .TP +.BR "\-\-protocol\-decoder\-jsontrace +When given, decoder output uses the Google Trace Event format (JSON). +Which can be inspected in web browsers or other viewers. +.TP .BR "\-l, \-\-loglevel " Set the libsigrok and libsigrokdecode loglevel. At the moment \fBsigrok\-cli\fP doesn't support setting the two loglevels independently. The higher the -- 2.30.2 From 394fd9b7a456f16c7ac15f41b0e29081f1d951f8 Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Wed, 14 Dec 2022 17:57:19 +0100 Subject: [PATCH 11/16] manpage: add environment variables section (firmware, decoders path) Add the SIGROK_FIRMWARE_DIR, SIGROK_FIRMWARE_PATH, SIGROKDECODE_DIR, and SIGROKDECODE_PATH environment variables to the sigrok-cli(1) manpage. Strictly speaking these are the libraries' environment variables, when libsigrok looks up device firmware, and libsigrokdecode searches for Python scripts. But the libraries don't have individual manpages. So let's put them here for better perception, and to have them _somewhere_ and not lose this information. --- doc/sigrok-cli.1 | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/doc/sigrok-cli.1 b/doc/sigrok-cli.1 index 47ec579..9c847a7 100644 --- a/doc/sigrok-cli.1 +++ b/doc/sigrok-cli.1 @@ -588,6 +588,25 @@ To turn on internal logging on a Lascar EL-USB series device: .SH "EXIT STATUS" .B sigrok\-cli exits with 0 on success, 1 on most failures. +.SH "ENVIRONMENT" +.TP +.B SIGROK_FIRMWARE_DIR +A single path where to search for firmware images, in addition to a +builtin list of locations. +.TP +.B SIGROK_FIRMWARE_PATH +Multiple path entries where to search for firmware images, in addition +to builtin locations. +.TP +When decoder support was enabled in the application's configuration: +.TP +.B SIGROKDECODE_DIR +A single path where to search for protocol decoders, in addition to +a builtin list of locations. +.TP +.B SIGROKDECODE_PATH +Multiple path entries where to search for protocol decoders, in addition +to builtin locations. .SH "SEE ALSO" \fBpulseview\fP(1) .SH "BUGS" -- 2.30.2 From 527dd7262e49dd051ff554401f9cb21c2c9006dd Mon Sep 17 00:00:00 2001 From: fenugrec Date: Fri, 2 Dec 2022 21:52:00 -0500 Subject: [PATCH 12/16] HACKING: catch up with libsigrok docs (mem alloc, var decl) Bring the "Random notes" section closer to the libsigrok documentation. Adjust the discussion of memory allocation. Prefer variable declaration at the start of routines, and separate them from value assignments. --- HACKING | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/HACKING b/HACKING index c9b876b..36c6756 100644 --- a/HACKING +++ b/HACKING @@ -29,18 +29,27 @@ Contributions Random notes ------------ - - Consistently use g_try_malloc() / g_try_malloc0(). Do not use standard + - Don't do variable declarations in compound statements, only at the + beginning of a function. + + - Generally avoid assigning values to variables at declaration time, + especially so for complex and/or run-time dependent values. + + - Consistently use g_*malloc() / g_*malloc0(). Do not use standard malloc()/calloc() if it can be avoided (sometimes other libs such as libftdi can return malloc()'d memory, for example). - Always properly match allocations with the proper *free() functions. If - glib's g_try_malloc()/g_try_malloc0() was used, use g_free() to free the + glib's g_*malloc()/g_*malloc0() was used, use g_free() to free the memory. Otherwise use standard free(). Never use the wrong function! - - Never use g_malloc() or g_malloc0(). These functions do not return NULL - if not enough memory is available but rather lead to an exit() or segfault - instead. This behaviour is not acceptable. - Use g_try_malloc()/g_try_malloc0() instead and check the return value. + - We assume that "small" memory allocations (< 1MB) will always succeed. + Thus, it's fine to use g_malloc() or g_malloc0() for allocations of + simple/small structs and such (instead of using g_try_malloc()), and + there's no need to check the return value. + + Do use g_try_malloc() or g_try_malloc0() for large (>= 1MB) allocations + and check the return value. - You should never print any messages (neither to stdout nor stderr nor elsewhere) "manually" via e.g. printf() or g_log() or similar functions. -- 2.30.2 From 51cf9b2cbdb58001253e9921aff1a09acfa539e1 Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Fri, 7 Apr 2023 18:31:53 +0200 Subject: [PATCH 13/16] decode: add 'const' decorator in hash item move helper routine Decorate the lookup key in move_hash_element() as const. --- decode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/decode.c b/decode.c index ac0e3e1..1a0747c 100644 --- a/decode.c +++ b/decode.c @@ -89,7 +89,7 @@ static int opts_to_gvar(struct srd_decoder *dec, GHashTable *hash, return ret; } -static int move_hash_element(GHashTable *src, GHashTable *dest, void *key) +static int move_hash_element(GHashTable *src, GHashTable *dest, const void *key) { void *orig_key, *value; -- 2.30.2 From a2dbd8488c1d18c39d55ebbf6e4c3c83db0296ba Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Fri, 7 Apr 2023 18:34:11 +0200 Subject: [PATCH 14/16] parsers: optional case insensitive channel name lookup Prepare the parsers.c:find_channel() routine to optionally lookup channels by case insensitive name matches. Stick with case sensitive comparison in all existing call sites. --- decode.c | 2 +- parsers.c | 16 +++++++++++----- sigrok-cli.h | 3 ++- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/decode.c b/decode.c index 1a0747c..fe5b968 100644 --- a/decode.c +++ b/decode.c @@ -278,7 +278,7 @@ static void map_pd_inst_channels(void *key, void *value, void *user_data) (char *)channel_id); continue; } - ch = find_channel(channel_list, channel_target); + ch = find_channel(channel_list, channel_target, TRUE); if (!ch) { g_printerr("cli: No channel with name \"%s\" found.\n", (char *)channel_target); diff --git a/parsers.c b/parsers.c index 59a7a91..2aacefb 100644 --- a/parsers.c +++ b/parsers.c @@ -25,7 +25,8 @@ #include #include "sigrok-cli.h" -struct sr_channel *find_channel(GSList *channellist, const char *channelname) +struct sr_channel *find_channel(GSList *channellist, const char *channelname, + gboolean exact_case) { struct sr_channel *ch; GSList *l; @@ -33,8 +34,13 @@ struct sr_channel *find_channel(GSList *channellist, const char *channelname) ch = NULL; for (l = channellist; l; l = l->next) { ch = l->data; - if (!strcmp(ch->name, channelname)) - break; + if (exact_case) { + if (strcmp(ch->name, channelname) == 0) + break; + } else { + if (g_ascii_strcasecmp(ch->name, channelname) == 0) + break; + } } ch = l ? l->data : NULL; @@ -105,7 +111,7 @@ GSList *parse_channelstring(struct sr_dev_inst *sdi, const char *channelstring) ret = SR_ERR; break; } - ch = find_channel(channels, str); + ch = find_channel(channels, str, TRUE); if (!ch) { g_critical("unknown channel '%d'.", b); ret = SR_ERR; @@ -130,7 +136,7 @@ range_fail: break; } - ch = find_channel(channels, names[0]); + ch = find_channel(channels, names[0], TRUE); if (!ch) { g_critical("unknown channel '%s'.", names[0]); g_strfreev(names); diff --git a/sigrok-cli.h b/sigrok-cli.h index 684aed9..b33b076 100644 --- a/sigrok-cli.h +++ b/sigrok-cli.h @@ -103,7 +103,8 @@ void map_pd_channels(struct sr_dev_inst *sdi); #endif /* parsers.c */ -struct sr_channel *find_channel(GSList *channellist, const char *channelname); +struct sr_channel *find_channel(GSList *channellist, const char *channelname, + gboolean exact_case); GSList *parse_channelstring(struct sr_dev_inst *sdi, const char *channelstring); int parse_triggerstring(const struct sr_dev_inst *sdi, const char *s, struct sr_trigger **trigger); -- 2.30.2 From 1f90599fa522afb9cfb9f603e2a27f89a6cb222f Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Fri, 7 Apr 2023 18:48:46 +0200 Subject: [PATCH 15/16] decode: assign logic channels to PD inputs (by name or by index) Add support for the -P :assign_channels=auto_index or =auto_names keywords. Which assigns sigrok logic channels to decoder input signals, but requires explicit initiation by users. The default behaviour remains backwards compatible, exclusively assigns individual channels as mapped by user specs, assigns nothing by default. Name matching is implemented as a simple case insensitive comparison. Neither weaker name checks (optional separators) nor advanced heuristics are supported as seen in the Pulseview GUI application. Future versions could implement these once the set of transformation rules has settled. --- decode.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/decode.c b/decode.c index fe5b968..12083a1 100644 --- a/decode.c +++ b/decode.c @@ -33,6 +33,10 @@ uint64_t pd_samplerate = 0; extern struct srd_session *srd_sess; +static const char *keyword_assign = "assign_channels"; +static const char *assign_by_index = "auto_index"; +static const char *assign_by_name = "auto_names"; + static int opts_to_gvar(struct srd_decoder *dec, GHashTable *hash, GHashTable **options) { @@ -111,6 +115,7 @@ static GHashTable *extract_channel_map(struct srd_decoder *dec, GHashTable *hash channel_map = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, g_free); + move_hash_element(hash, channel_map, keyword_assign); for (l = dec->channels; l; l = l->next) { pdch = l->data; move_hash_element(hash, channel_map, pdch->id); @@ -253,11 +258,21 @@ static void map_pd_inst_channels(void *key, void *value, void *user_data) GHashTable *channel_indices; GSList *channel_list; struct srd_decoder_inst *di; + struct srd_decoder *pd; GVariant *var; void *channel_id; void *channel_target; struct sr_channel *ch; GHashTableIter iter; + enum assign_t { + ASSIGN_UNKNOWN, + ASSIGN_USER_SPEC, + ASSIGN_BY_INDEX, + ASSIGN_BY_NAMES, + } assign; + const char *keyword; + GSList *l_pd, *l_pdo, *l_ch; + struct srd_channel *pdch; channel_map = value; channel_list = user_data; @@ -271,8 +286,102 @@ static void map_pd_inst_channels(void *key, void *value, void *user_data) channel_indices = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, (GDestroyNotify)g_variant_unref); + /* + * The typical mode of operation is to apply a user specified + * mapping of sigrok channels to decoder inputs. Lack of mapping + * specs will assign no signals (compatible behaviour to earlier + * implementations). + * + * Alternatively we can assign sigrok logic channels to decoder + * inputs in the strict order of channel indices, or when their + * names match. Users need to request this automatic assignment + * though, because this behaviour differs from earlier versions. + * + * Even more sophisticated heuristics of mapping sigrok channels + * to decoder inputs are not implemented here. Later versions + * could translate the Pulseview approach to the C language, but + * it's better to stabilize that logic first before porting it. + */ + assign = ASSIGN_USER_SPEC; + keyword = g_hash_table_lookup(channel_map, keyword_assign); + if (g_hash_table_size(channel_map) != 1) { + assign = ASSIGN_USER_SPEC; + } else if (!keyword) { + assign = ASSIGN_USER_SPEC; + } else if (strcmp(keyword, assign_by_index) == 0) { + assign = ASSIGN_BY_INDEX; + } else if (strcmp(keyword, assign_by_name) == 0) { + assign = ASSIGN_BY_NAMES; + } else { + g_critical("Unknown type of decoder channel assignment: %s.", + keyword); + return; + } + + pd = di->decoder; + if (assign == ASSIGN_BY_INDEX) { + /* + * Iterate the protocol decoder's list of input signals + * (mandatory and optional, never more than the decoder's + * total channels count). Assign sigrok logic channels + * until either is exhausted. Use sigrok channels in the + * very order of their declaration in the input stream. + */ + l_ch = channel_list; + l_pd = pd->channels; + while (l_ch && l_pd) { + ch = l_ch->data; + l_ch = l_ch->next; + if (ch->type != SR_CHANNEL_LOGIC) + break; + if (ch->index >= di->dec_num_channels) + break; + pdch = l_pd->data; + l_pd = l_pd->next; + if (!l_pd) + l_pd = pd->opt_channels; + /* TODO Emit an INFO message. */ + g_hash_table_insert(channel_map, + g_strdup(pdch->id), g_strdup(ch->name)); + } + } else if (assign == ASSIGN_BY_NAMES) { + /* + * Iterate the protocol decoder's list of input signals. + * Search for sigrok logic channels that have matching + * names (case insensitive comparison). Not finding a + * sigrok channel of a given name is non-fatal (could be + * an optional channel, the decoder will check later for + * all mandatory channels to be assigned). + */ + l_pd = pd->channels; + l_pdo = pd->opt_channels; + while (l_pd) { + pdch = l_pd->data; + l_pd = l_pd->next; + if (!l_pd) { + l_pd = l_pdo; + l_pdo = NULL; + } + ch = find_channel(channel_list, pdch->id, FALSE); + if (!ch) { + /* TODO Emit a WARN message. */ + /* if (l_pdo) ...; */ + continue; + } + if (ch->type != SR_CHANNEL_LOGIC) { + /* TODO Emit a WARN message. */ + continue; + } + /* TODO Emit an INFO message. */ + g_hash_table_insert(channel_map, + g_strdup(pdch->id), g_strdup(ch->name)); + } + } + g_hash_table_iter_init(&iter, channel_map); while (g_hash_table_iter_next(&iter, &channel_id, &channel_target)) { + if (strcmp(channel_id, keyword_assign) == 0) + continue; if (!channel_target) { g_printerr("cli: Channel name for \"%s\" missing.\n", (char *)channel_id); -- 2.30.2 From 9d9f7b82008e3b3665bda12a63a3339e9f7aabc3 Mon Sep 17 00:00:00 2001 From: Gerhard Sittig Date: Fri, 7 Apr 2023 18:54:14 +0200 Subject: [PATCH 16/16] doc: update sigrok-cli(1) for channel assignment to decoder inputs Extend the sigrok-cli manpage section which discusses the -P option. Mention the assignment of logic channels to decoder inputs by index or by name. Automatic assignment by position in the absence of user specs seems to not have been recent implementations' default behaviour. Remove that outdated or incorrect comment. --- doc/sigrok-cli.1 | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/doc/sigrok-cli.1 b/doc/sigrok-cli.1 index 9c847a7..33174cd 100644 --- a/doc/sigrok-cli.1 +++ b/doc/sigrok-cli.1 @@ -1,4 +1,4 @@ -.TH SIGROK\-CLI 1 "March 28, 2019" +.TH SIGROK\-CLI 1 "April 07, 2023" .SH "NAME" sigrok\-cli \- Command-line client for the sigrok software .SH "SYNOPSIS" @@ -311,15 +311,27 @@ In this example, .B wordsize is an option supported by the .B spi -protocol decoder. Additionally, the user tells sigrok to decode the SPI +protocol decoder. Additionally, the user requests sigrok to decode the SPI protocol using channel 1 as MISO signal for SPI, channel 5 as MOSI, channel 3 as CLK, and channel 0 as CS# signal. .sp -Notice that the +The .B sigrok\-cli -application does not support "name matching". Instead it's assumed that the -traces in the input stream match the order of the decoder's input signals, -or that users explicitly specify the input channel to decoder signal mapping. +application can automatically assign logic channels to decoder inputs +in their strict channel index order (by means of the +.B auto_index +keyword), or can automatically assign logic channels to decoder inputs +when their names match (case insensitive match, +.B auto_names +keyword). Users need to explicitly request this assignment, the default +behaviour is to not automatically assign any signals. +.sp +Example: +.sp + $ +.B "sigrok\-cli \-i " +.br +.B " \-P ieee488:assign_channels=auto_names" .br .sp When multiple decoders are specified in the same -- 2.30.2