tools: improvements for v2

Rework the tool suite to support identifying lines by name and to support operating on the GPIO lines available to the user at once, rather than on one particular GPIO chip.

All tools, other than gpiodetect, now provide the name to (chip,offset) mapping that was previously only performed by gpiofind. As names are not guaranteed to be unique, a --strict option is provided for all tools to either abort the operation or report all lines with the matching name, as appropriate.
By default the tools operate on the first line found with a matching name.

Selection of line by (chip,offset) is still supported with a --chip option, though it restricts the scope of the operation to an individual chip. When the --chip option is specified, the lines are assumed to be identified by offset where they parse as an integer, else by name. To cater for the unusual case where a line name parses as an integer, but is different from the offset, the --by-name option forces the lines to be identified by name.

The updated tools are intentionally NOT backwardly compatible with the previous tools. Using old command lines with the updated tools will almost certainly fail, though migrating old command lines is generally as simple as adding a '-c' before the chip.

In addition the individual tools are modified as follows:

gpiodetect:

Add the option to select individual chips.

gpioinfo:

Change the focus from chips to lines, so the scope can be an individual line, a subset of lines, all lines on a particular chip, or all the lines available to the user. For line scope a single line summary is output for each line. For chip scope the existing format displaying a summary of the chip and each of its lines is retained.

Line attributes are consolidated into a list format, and are extended to cover all attributes supported by uAPI v2.

gpioget:

The default output format is becomes line=value, as per the input for gpioset, and the value is reported as active or inactive, rather than 0 or 1.
The previous format is available using the --numeric option.

Add an optional hold period between requesting a line and reading the value to allow the line to settle once the requested configuration has been applied (e.g. bias).

gpiomon:

Consolidate the edge options into a single option.

Add a debounce period option.

Add options to report event times as UTC or localtime.

Add format specifiers for GPIO chip path, line name, stringified event type, and event time as a datetime.

Rearrange default output format to place fields with more predicable widths to the left, and to separate major field groups with tabs. Lines are identified consistent with the command line.

gpioset:

Add a hold period option that specifies the minimum period the line value must be held for. This applies to all set options.

Support line values specified as active/inactive, on/off and true/false, as well as 1/0.

Add a toggle option that specifies a time sequence over which the requested lines should be toggled. If the sequence is 0 terminated then gpioset exits when the sequence completes, else it repeats the sequence. This allows for anything from simple blinkers to bit bashing from the command line. e.g. gpioset -t 500ms LED=on

Add an interactive option to provide a shell-like interface to allow manual or scripted manipulation of requested lines. A basic command set allows lines to be get, set, or toggled, and to insert sleeps between operations.

Remove the --mode, --sec, and --usec options.
The combination of hold period and interactive mode provide functionality equivalent to the old --mode options.

Signed-off-by Kent Gibson <[email protected]>

---
 configure.ac         |   8 +-
 tools/Makefile.am    |   2 +-
 tools/gpiodetect.c   | 113 +++++-
 tools/gpioget.c      | 204 ++++++++++-
 tools/gpioinfo.c     | 223 +++++++++++-
 tools/gpiomon.c      | 450 +++++++++++++++++++++++-
 tools/gpioset.c      | 815 ++++++++++++++++++++++++++++++++++++++++++-
 tools/tools-common.c | 713 ++++++++++++++++++++++++++++++++++++-
 tools/tools-common.h |  99 +++++-
 9 files changed, 2585 insertions(+), 42 deletions(-)

configure.ac +4 -4

index 6ac1d8e..c8033c5 100644
@@ -106,14 +106,14 @@ AC_DEFUN([FUNC_NOT_FOUND_TOOLS],
 AC_DEFUN([HEADER_NOT_FOUND_TOOLS],
        [ERR_NOT_FOUND([$1 header], [tools])])
 
+AC_DEFUN([LIB_NOT_FOUND_TOOLS],
+       [ERR_NOT_FOUND([lib$1], [tools])])
+
 if test "x$with_tools" = xtrue
 then
        # These are only needed to build tools
-       AC_CHECK_FUNC([basename], [], [FUNC_NOT_FOUND_TOOLS([basename])])
        AC_CHECK_FUNC([daemon], [], [FUNC_NOT_FOUND_TOOLS([daemon])])
-       AC_CHECK_FUNC([signalfd], [], [FUNC_NOT_FOUND_TOOLS([signalfd])])
-       AC_CHECK_FUNC([setlinebuf], [], [FUNC_NOT_FOUND_TOOLS([setlinebuf])])
-       AC_CHECK_HEADERS([sys/signalfd.h], [], [HEADER_NOT_FOUND_TOOLS([sys/signalfd.h])])
+       PKG_CHECK_MODULES(LIBEDIT, libedit)
 fi
 
 AC_ARG_ENABLE([tests],

tools/Makefile.am +1 -1

index fc074b9..c956314 100644
@@ -7,7 +7,7 @@ AM_CFLAGS += -Wall -Wextra -g -std=gnu89
 noinst_LTLIBRARIES = libtools-common.la
 libtools_common_la_SOURCES = tools-common.c tools-common.h
 
-LDADD = libtools-common.la $(top_builddir)/lib/libgpiod.la
+LDADD = libtools-common.la $(top_builddir)/lib/libgpiod.la $(LIBEDIT_LIBS)
 
 bin_PROGRAMS = gpiodetect gpioinfo gpioget gpioset gpiomon
 

tools/gpiodetect.c +16 -2

index 30bde32..910fe9e 100644
@@ -1,8 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 // SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]>
+// SPDX-FileCopyrightText: 2022 Kent Gibson <[email protected]>
 
-#include <dirent.h>
-#include <errno.h>
 #include <getopt.h>
 #include <gpiod.h>
 #include <stdio.h>

@@ -11,3 +10,113 @@
 
 #include "tools-common.h"
 
+static void print_help(void)
+{
+       printf("Usage: %s [OPTIONS] [chip]...\n", get_progname());
+       printf("\n");
+       printf("List GPIO chips, print their labels and number of GPIO lines.\n");
+       printf("\n");
+       printf("Chips may be identified by number, name, or path.\n");
+       printf("e.g. '0', 'gpiochip0', and '/dev/gpiochip0' all refer to the same chip.\n");
+       printf("\n");
+       printf("If no chips are specified then all chips are listed.\n");
+       printf("\n");
+       printf("Options:\n");
+       printf("  -h, --help\t\tdisplay this help and exit\n");
+       printf("  -v, --version\t\toutput version information and exit\n");
+}

tools/gpiodetect.c +85 -0

+
+       for (;;) {
+               optc = getopt_long(argc, argv, shortopts, longopts, &opti);
+               if (optc < 0)
+                       break;
+
+               switch (optc) {
+               case 'h':
+                       print_help();
+                       exit(EXIT_SUCCESS);
+               case 'v':
+                       print_version();
+                       exit(EXIT_SUCCESS);
+               case '?':
+                       die("try %s --help", get_progname());
+               default:
+                       abort();
+               }
+       }
+       return optind;
+}
+
+int print_chip_info(const char *path)
+{
+       struct gpiod_chip *chip;
+       struct gpiod_chip_info *info;
+
+       chip = gpiod_chip_open(path);
+       if (!chip) {
+               print_perror("unable to open chip '%s'", path);
+               return 1;
+       }
+
+       info = gpiod_chip_get_info(chip);
+       if (!info)
+               die_perror("unable to read info for '%s'", path);
+
+       printf("%s [%s] (%zu lines)\n",
+              gpiod_chip_info_get_name(info),
+              gpiod_chip_info_get_label(info),
+              gpiod_chip_info_get_num_lines(info));
+
+       gpiod_chip_info_free(info);
+       gpiod_chip_close(chip);
+       return 0;
+}
+
+int main(int argc, char **argv)
+{
+       int num_chips, i;
+       char **paths;
+       char *path;
+       int ret = EXIT_SUCCESS;
+
+       i = parse_config(argc, argv);
+       argc -= i;
+       argv += i;
+
+       if (argc == 0) {
+               num_chips = all_chip_paths(&paths);
+               for (i = 0; i < num_chips; i++) {
+                       if (print_chip_info(paths[i]))
+                               ret = EXIT_FAILURE;
+
+                       free(paths[i]);
+               }
+               free(paths);
+       }
+
+       for (i = 0; i < argc; i++) {
+               if (chip_path_lookup(argv[i], &path)) {
+                       if (print_chip_info(path))
+                               ret = EXIT_FAILURE;
+
+                       free(path);
+               } else {
+                       print_error(
+                               "cannot find GPIO chip character device '%s'",
+                               argv[i]);
+                       ret = EXIT_FAILURE;
+               }
+       }
+
+       return ret;
+}

tools/gpioget.c +203 -1

index 1b3e666..7a26066 100644
@@ -1,12 +1,214 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 // SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]>
+// SPDX-FileCopyrightText: 2022 Kent Gibson <[email protected]>
 
 #include <getopt.h>
 #include <gpiod.h>
-#include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <unistd.h>
 
 #include "tools-common.h"
 
+static void print_help(void)
+{
+       printf("Usage: %s [OPTIONS] <line>...\n", get_progname());
+       printf("\n");
+       printf("Read values of GPIO lines.\n");
+       printf("\n");
+       printf("Lines are specified by name, or optionally by offset if the chip option\n");
+       printf("is provided.\n");
+       printf("\n");
+       printf("Options:\n");
+       printf("  -a, --as-is\t\tleave the line direction unchanged, not forced to input\n");
+       print_bias_help();
+       printf("      --by-name\t\ttreat lines as names even if they would parse as an offset\n");
+       printf("  -c, --chip <chip>\trestrict scope to a particular chip\n");
+       printf("  -h, --help\t\tdisplay this help and exit\n");
+       printf("  -l, --active-low\ttreat the line as active low\n");
+       printf("  -p, --hold-period <period>\n");
+       printf("\t\t\twait between requesting the lines and reading the values\n");
+       printf("      --numeric\t\tdisplay line values as '0' (inactive) or '1' (active)\n");
+       printf("  -s, --strict\t\tabort if requested line names are not unique\n");
+       printf("  -v, --version\t\toutput version information and exit\n");
+       print_chip_help();
+       print_period_help();
+}
+struct config {
+       bool active_low;
+       bool strict;
+       int bias;
+       int direction;
+       unsigned int hold_period_us;
+       const char *chip_id;
+       int by_name;
+       int numeric;
+};
+
+int parse_config(int argc, char **argv, struct config *cfg)
+{
+       int opti, optc;
+       const char *const shortopts = "+ab:c:hlp:sv";
+       const struct option longopts[] = {
+               { "active-low", no_argument,            NULL,   'l' },
+               { "as-is",      no_argument,            NULL,   'a' },
+               { "bias",       required_argument,      NULL,   'b' },
+               { "by-name",    no_argument,            &cfg->by_name,  1 },
+               { "chip",       required_argument,      NULL,   'c' },
+               { "help",       no_argument,            NULL,   'h' },
+               { "hold-period", required_argument,     NULL,   'p' },
+               { "numeric",    no_argument,            &cfg->numeric,  1 },
+               { "strict",     no_argument,            NULL,   's' },
+               { "version",    no_argument,            NULL,   'v' },
+               { GETOPT_NULL_LONGOPT },
+       };
+
+       memset(cfg, 0, sizeof(*cfg));
+       cfg->direction = GPIOD_LINE_DIRECTION_INPUT;
+
+       for (;;) {
+               optc = getopt_long(argc, argv, shortopts, longopts, &opti);
+               if (optc < 0)
+                       break;
+
+               switch (optc) {
+               case 'a':
+                       cfg->direction = GPIOD_LINE_DIRECTION_AS_IS;
+                       break;
+               case 'b':
+                       cfg->bias = parse_bias_or_die(optarg);
+                       break;
+               case 'c':
+                       cfg->chip_id = optarg;
+                       break;
+               case 'l':
+                       cfg->active_low = true;
+                       break;
+               case 'p':
+                       cfg->hold_period_us = parse_period_or_die(optarg);
+                       break;
+               case 's':
+                       cfg->strict = true;
+                       break;
+               case 'h':
+                       print_help();
+                       exit(EXIT_SUCCESS);
+               case 'v':
+                       print_version();
+                       exit(EXIT_SUCCESS);
+               case '?':
+                       die("try %s --help", get_progname());
+               case 0:
+                       break;
+               default:
+                       abort();
+               }
+       }
+
+       return optind;
+}
+
+int main(int argc, char **argv)
+{
+       int i, num_lines, ret, *values;
+       struct gpiod_line_settings *settings;
+       struct gpiod_request_config *req_cfg;
+       struct gpiod_line_request *request;
+       struct gpiod_line_config *line_cfg;
+       struct gpiod_chip *chip;
+       unsigned int *offsets;
+       struct line_resolver *resolver;
+       struct resolved_line *line;
+       struct config cfg;
+
+       i = parse_config(argc, argv, &cfg);
+       argc -= i;
+       argv += i;
+
+       if (argc < 1)
+               die("at least one GPIO line must be specified");
+
+       resolver = resolve_lines(argc, argv, cfg.chip_id, cfg.strict,
+                                cfg.by_name);
+       validate_resolution(resolver, cfg.chip_id);
+
+       offsets = calloc(resolver->num_lines, sizeof(*offsets));
+       values = calloc(resolver->num_lines, sizeof(*values));
+       if (!offsets || !values)
+               die("out of memory");
+
+       settings = gpiod_line_settings_new();
+       if (!settings)
+               die_perror("unable to allocate line settings");
+
+       gpiod_line_settings_set_direction(settings, cfg.direction);
+
+       if (cfg.bias)
+               gpiod_line_settings_set_bias(settings, cfg.bias);
+
+       if (cfg.active_low)
+               gpiod_line_settings_set_active_low(settings, true);
+
+       req_cfg = gpiod_request_config_new();
+       if (!req_cfg)
+               die_perror("unable to allocate the request config structure");
+
+       line_cfg = gpiod_line_config_new();
+       if (!line_cfg)
+               die_perror("unable to allocate the line config structure");
+
+       gpiod_request_config_set_consumer(req_cfg, "gpioget");
+       for (i = 0; i < resolver->num_chips; i++) {
+               chip = gpiod_chip_open(resolver->chips[i].path);
+               if (!chip)
+                       die_perror("unable to open chip '%s'",
+                                  resolver->chips[i].path);
+
+               num_lines = get_line_offsets_and_values(resolver, i, offsets,
+                                                       NULL);
+
+               gpiod_line_config_reset(line_cfg);
+               ret = gpiod_line_config_add_line_settings(line_cfg, offsets,
+                                                         num_lines, settings);
+               if (ret)
+                       die_perror("unable to add line settings");
+
+               request = gpiod_chip_request_lines(chip, req_cfg, line_cfg);
+               if (!request)
+                       die_perror("unable to request lines");
+
+               if (cfg.hold_period_us)
+                       usleep(cfg.hold_period_us);
+
+               ret = gpiod_line_request_get_values(request, values);
+               if (ret)
+                       die_perror("unable to read GPIO line values");
+
+               set_line_values(resolver, i, values);
+
+               gpiod_line_request_release(request);
+               gpiod_chip_close(chip);
+       }
+       for (i = 0; i < resolver->num_lines; i++) {
+               line = &resolver->lines[i];
+               if (cfg.numeric)
+                       printf("%d", line->value);
+               else
+                       printf("%s=%s", line->id,
+                              line->value ? "active" : "inactive");
+
+               if (i != resolver->num_lines - 1)
+                       printf(" ");
+       }
+       printf("\n");
+
+       free_line_resolver(resolver);
+       gpiod_request_config_free(req_cfg);
+       gpiod_line_config_free(line_cfg);
+       gpiod_line_settings_free(settings);
+       free(offsets);
+       free(values);
+
+       return EXIT_SUCCESS;
+}

tools/gpioinfo.c +66 -2

index ae368fa..5dc28f8 100644
@@ -1,8 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 // SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]>
+// SPDX-FileCopyrightText: 2022 Kent Gibson <[email protected]>
 
-#include <dirent.h>
-#include <errno.h>
 #include <getopt.h>
 #include <gpiod.h>
 #include <stdarg.h>

@@ -12,3 +11,223 @@
 
 #include "tools-common.h"
 
+static void print_help(void)
+{
+       printf("Usage: %s [OPTIONS] [line]...\n", get_progname());
+       printf("\n");
+       printf("Print information about GPIO lines.\n");
+       printf("\n");
+       printf("Lines are specified by name, or optionally by offset if the chip option\n");
+       printf("is provided.\n");
+       printf("\n");
+       printf("If no lines are specified than all lines are displayed.\n");
+       printf("\n");
+       printf("Options:\n");
+       printf("      --by-name\t\ttreat lines as names even if they would parse as an offset\n");
+       printf("  -c, --chip <chip>\trestrict scope to a particular chip\n");
+       printf("  -h, --help\t\tdisplay this help and exit\n");
+       printf("  -s, --strict\t\tcheck all lines - don't assume line names are unique\n");
+       printf("  -v, --version\t\toutput version information and exit\n");
+       print_chip_help();
+}
+
+struct config {
+       bool strict;
+       const char *chip_id;
+       int by_name;
+};
+
+int parse_config(int argc, char **argv, struct config *cfg)
+{
+       int opti, optc;
+       const char *const shortopts = "+c:hsv";
+       const struct option longopts[] = {
+               { "by-name",    no_argument,    &cfg->by_name,  1 },
+               { "chip",       required_argument,      NULL,   'c' },
+               { "help",       no_argument,    NULL,           'h' },
+               { "strict",     no_argument,    NULL,           's' },
+               { "version",    no_argument,    NULL,           'v' },
+               { GETOPT_NULL_LONGOPT },
+       };
+
+       memset(cfg, 0, sizeof(*cfg));
+
+       for (;;) {
+               optc = getopt_long(argc, argv, shortopts, longopts, &opti);
+               if (optc < 0)
+                       break;
+
+               switch (optc) {
+               case 'c':
+                       cfg->chip_id = optarg;
+                       break;
+               case 's':
+                       cfg->strict = true;
+                       break;
+               case 'h':
+                       print_help();
+                       exit(EXIT_SUCCESS);
+               case 'v':
+                       print_version();
+                       exit(EXIT_SUCCESS);
+               case '?':
+                       die("try %s --help", get_progname());
+               case 0:
+                       break;
+               default:
+                       abort();

Quoted 20 lines · Kent Gibson

Rework the tool suite to support identifying lines by name and to support operating on the GPIO lines available to the user at once, rather than on one particular GPIO chip.

All tools, other than gpiodetect, now provide the name to (chip,offset) mapping that was previously only performed by gpiofind. As names are not guaranteed to be unique, a --strict option is provided for all tools to either abort the operation or report all lines with the matching name, as appropriate.
By default the tools operate on the first line found with a matching name.

Selection of line by (chip,offset) is still supported with a --chip option, though it restricts the scope of the operation to an individual chip. When the --chip option is specified, the lines are assumed to be identified by offset where they parse as an integer, else by name. To cater for the unusual case where a line name parses as an integer, but is different from the offset, the --by-name option forces the lines to be identified by name.

Quoted 5 lines

The updated tools are intentionally NOT backwardly compatible with the previous tools. Using old command lines with the updated tools will almost certainly fail, though migrating old command lines is generally as simple as adding a '-c' before the chip.

While at it: how about adding the --consumer/-C switch to specify a consumer string other than the name of the program?

Quoted 8 lines

In addition the individual tools are modified as follows:

gpiodetect:

Add the option to select individual chips.

gpioinfo:

Quoted 9 lines

Change the focus from chips to lines, so the scope can be an individual line, a subset of lines, all lines on a particular chip, or all the lines available to the user. For line scope a single line summary is output for each line. For chip scope the existing format displaying a summary of the chip and each of its lines is retained.

Line attributes are consolidated into a list format, and are extended to cover all attributes supported by uAPI v2.

One change in the output that bothers me is the removal of quotation marks around the line name and consumer. I did that in v1 to visually distinguish between unnamed/unused lines and those that are named. I know it's highly unlikely that a line would be named "unnamed" (sic!) but still:

line 0: "foo"
line 1: unnamed

looks more intuitive to me.

Same for the consumer as with your current
version, if the consumer string has spaces in it, it will look like
this: consumer=foo bar. I think consumer="foo bar" would be easier to parse.

Quoted 41 lines

gpioget:

The default output format is becomes line=value, as per the input for gpioset, and the value is reported as active or inactive, rather than 0 or 1.
The previous format is available using the --numeric option.

Add an optional hold period between requesting a line and reading the value to allow the line to settle once the requested configuration has been applied (e.g. bias).

gpiomon:

Consolidate the edge options into a single option.

Add a debounce period option.

Add options to report event times as UTC or localtime.

Add format specifiers for GPIO chip path, line name, stringified event type, and event time as a datetime.

Rearrange default output format to place fields with more predicable widths to the left, and to separate major field groups with tabs. Lines are identified consistent with the command line.

gpioset:

Add a hold period option that specifies the minimum period the line value must be held for. This applies to all set options.

Support line values specified as active/inactive, on/off and true/false, as well as 1/0.

Add a toggle option that specifies a time sequence over which the requested lines should be toggled. If the sequence is 0 terminated then gpioset exits when the sequence completes, else it repeats the sequence. This allows for anything from simple blinkers to bit bashing from the command line. e.g. gpioset -t 500ms LED=on

Quoted 5 lines

Add an interactive option to provide a shell-like interface to allow manual or scripted manipulation of requested lines. A basic command set allows lines to be get, set, or toggled, and to insert sleeps between operations.

As discussed elsewhere - it would be great if this part was optional and configurable at build-time so that the dependency on libedit could be dropped if unavailable.

Quoted 4 lines

Remove the --mode, --sec, and --usec options.
The combination of hold period and interactive mode provide functionality equivalent to the old --mode options.

I have one problem with that - I think the basic functionality of:
"take a line, set its value and wait for a signal" would still be useful. As it is now, I'm not sure how to make gpioset just hold a line without calling the GPIO_V2_LINE_SET_VALUES_IOCTL ioctl periodically.

Quoted 86 lines

Signed-off-by Kent Gibson <[email protected]>

---
 configure.ac         |   8 +-
 tools/Makefile.am    |   2 +-
 tools/gpiodetect.c   | 113 +++++-
 tools/gpioget.c      | 204 ++++++++++-
 tools/gpioinfo.c     | 223 +++++++++++-
 tools/gpiomon.c      | 450 +++++++++++++++++++++++-
 tools/gpioset.c      | 815 ++++++++++++++++++++++++++++++++++++++++++-
 tools/tools-common.c | 713 ++++++++++++++++++++++++++++++++++++-
 tools/tools-common.h |  99 +++++-
 9 files changed, 2585 insertions(+), 42 deletions(-)

configure.ac +4 -4

index 6ac1d8e..c8033c5 100644
@@ -106,14 +106,14 @@ AC_DEFUN([FUNC_NOT_FOUND_TOOLS],
 AC_DEFUN([HEADER_NOT_FOUND_TOOLS],
        [ERR_NOT_FOUND([$1 header], [tools])])
 
+AC_DEFUN([LIB_NOT_FOUND_TOOLS],
+       [ERR_NOT_FOUND([lib$1], [tools])])
+
 if test "x$with_tools" = xtrue
 then
        # These are only needed to build tools
-       AC_CHECK_FUNC([basename], [], [FUNC_NOT_FOUND_TOOLS([basename])])
        AC_CHECK_FUNC([daemon], [], [FUNC_NOT_FOUND_TOOLS([daemon])])
-       AC_CHECK_FUNC([signalfd], [], [FUNC_NOT_FOUND_TOOLS([signalfd])])
-       AC_CHECK_FUNC([setlinebuf], [], [FUNC_NOT_FOUND_TOOLS([setlinebuf])])
-       AC_CHECK_HEADERS([sys/signalfd.h], [], [HEADER_NOT_FOUND_TOOLS([sys/signalfd.h])])
+       PKG_CHECK_MODULES(LIBEDIT, libedit)
 fi
 
 AC_ARG_ENABLE([tests],

tools/Makefile.am +1 -1

index fc074b9..c956314 100644
@@ -7,7 +7,7 @@ AM_CFLAGS += -Wall -Wextra -g -std=gnu89
 noinst_LTLIBRARIES = libtools-common.la
 libtools_common_la_SOURCES = tools-common.c tools-common.h
 
-LDADD = libtools-common.la $(top_builddir)/lib/libgpiod.la
+LDADD = libtools-common.la $(top_builddir)/lib/libgpiod.la $(LIBEDIT_LIBS)
 
 bin_PROGRAMS = gpiodetect gpioinfo gpioget gpioset gpiomon
 

tools/gpiodetect.c +20 -2

index 30bde32..910fe9e 100644
@@ -1,8 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 // SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]>
+// SPDX-FileCopyrightText: 2022 Kent Gibson <[email protected]>
 
-#include <dirent.h>
-#include <errno.h>
 #include <getopt.h>
 #include <gpiod.h>
 #include <stdio.h>

@@ -11,3 +10,113 @@
 
 #include "tools-common.h"
 
+static void print_help(void)
+{
+       printf("Usage: %s [OPTIONS] [chip]...\n", get_progname());
+       printf("\n");
+       printf("List GPIO chips, print their labels and number of GPIO lines.\n");
+       printf("\n");
+       printf("Chips may be identified by number, name, or path.\n");
+       printf("e.g. '0', 'gpiochip0', and '/dev/gpiochip0' all refer to the same chip.\n");
+       printf("\n");
+       printf("If no chips are specified then all chips are listed.\n");
+       printf("\n");
+       printf("Options:\n");
+       printf("  -h, --help\t\tdisplay this help and exit\n");
+       printf("  -v, --version\t\toutput version information and exit\n");
+}
+
+int parse_config(int argc, char **argv)
+{
+       int optc, opti;

tools/gpiodetect.c line 37

Quoted 6 lines

tools/gpiodetect.c +6 -0

+       const char *const shortopts = "+hv";
+       const struct option longopts[] = {
+               { "help",       no_argument,    NULL,   'h' },
+               { "version",    no_argument,    NULL,   'v' },
+               { GETOPT_NULL_LONGOPT },
+       };

This can be static as there are no addresses of flag variables assigned.

Quoted 392 lines

tools/gpiodetect.c +85 -0

+
+       for (;;) {
+               optc = getopt_long(argc, argv, shortopts, longopts, &opti);
+               if (optc < 0)
+                       break;
+
+               switch (optc) {
+               case 'h':
+                       print_help();
+                       exit(EXIT_SUCCESS);
+               case 'v':
+                       print_version();
+                       exit(EXIT_SUCCESS);
+               case '?':
+                       die("try %s --help", get_progname());
+               default:
+                       abort();
+               }
+       }
+       return optind;
+}
+
+int print_chip_info(const char *path)
+{
+       struct gpiod_chip *chip;
+       struct gpiod_chip_info *info;
+
+       chip = gpiod_chip_open(path);
+       if (!chip) {
+               print_perror("unable to open chip '%s'", path);
+               return 1;
+       }
+
+       info = gpiod_chip_get_info(chip);
+       if (!info)
+               die_perror("unable to read info for '%s'", path);
+
+       printf("%s [%s] (%zu lines)\n",
+              gpiod_chip_info_get_name(info),
+              gpiod_chip_info_get_label(info),
+              gpiod_chip_info_get_num_lines(info));
+
+       gpiod_chip_info_free(info);
+       gpiod_chip_close(chip);
+       return 0;
+}
+
+int main(int argc, char **argv)
+{
+       int num_chips, i;
+       char **paths;
+       char *path;
+       int ret = EXIT_SUCCESS;
+
+       i = parse_config(argc, argv);
+       argc -= i;
+       argv += i;
+
+       if (argc == 0) {
+               num_chips = all_chip_paths(&paths);
+               for (i = 0; i < num_chips; i++) {
+                       if (print_chip_info(paths[i]))
+                               ret = EXIT_FAILURE;
+
+                       free(paths[i]);
+               }
+               free(paths);
+       }
+
+       for (i = 0; i < argc; i++) {
+               if (chip_path_lookup(argv[i], &path)) {
+                       if (print_chip_info(path))
+                               ret = EXIT_FAILURE;
+
+                       free(path);
+               } else {
+                       print_error(
+                               "cannot find GPIO chip character device '%s'",
+                               argv[i]);
+                       ret = EXIT_FAILURE;
+               }
+       }
+
+       return ret;
+}

tools/gpioget.c +203 -1

index 1b3e666..7a26066 100644
@@ -1,12 +1,214 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 // SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]>
+// SPDX-FileCopyrightText: 2022 Kent Gibson <[email protected]>
 
 #include <getopt.h>
 #include <gpiod.h>
-#include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <unistd.h>
 
 #include "tools-common.h"
 
+static void print_help(void)
+{
+       printf("Usage: %s [OPTIONS] <line>...\n", get_progname());
+       printf("\n");
+       printf("Read values of GPIO lines.\n");
+       printf("\n");
+       printf("Lines are specified by name, or optionally by offset if the chip option\n");
+       printf("is provided.\n");
+       printf("\n");
+       printf("Options:\n");
+       printf("  -a, --as-is\t\tleave the line direction unchanged, not forced to input\n");
+       print_bias_help();
+       printf("      --by-name\t\ttreat lines as names even if they would parse as an offset\n");
+       printf("  -c, --chip <chip>\trestrict scope to a particular chip\n");
+       printf("  -h, --help\t\tdisplay this help and exit\n");
+       printf("  -l, --active-low\ttreat the line as active low\n");
+       printf("  -p, --hold-period <period>\n");
+       printf("\t\t\twait between requesting the lines and reading the values\n");
+       printf("      --numeric\t\tdisplay line values as '0' (inactive) or '1' (active)\n");
+       printf("  -s, --strict\t\tabort if requested line names are not unique\n");
+       printf("  -v, --version\t\toutput version information and exit\n");
+       print_chip_help();
+       print_period_help();
+}
+struct config {
+       bool active_low;
+       bool strict;
+       int bias;
+       int direction;
+       unsigned int hold_period_us;
+       const char *chip_id;
+       int by_name;
+       int numeric;
+};
+
+int parse_config(int argc, char **argv, struct config *cfg)
+{
+       int opti, optc;
+       const char *const shortopts = "+ab:c:hlp:sv";
+       const struct option longopts[] = {
+               { "active-low", no_argument,            NULL,   'l' },
+               { "as-is",      no_argument,            NULL,   'a' },
+               { "bias",       required_argument,      NULL,   'b' },
+               { "by-name",    no_argument,            &cfg->by_name,  1 },
+               { "chip",       required_argument,      NULL,   'c' },
+               { "help",       no_argument,            NULL,   'h' },
+               { "hold-period", required_argument,     NULL,   'p' },
+               { "numeric",    no_argument,            &cfg->numeric,  1 },
+               { "strict",     no_argument,            NULL,   's' },
+               { "version",    no_argument,            NULL,   'v' },
+               { GETOPT_NULL_LONGOPT },
+       };
+
+       memset(cfg, 0, sizeof(*cfg));
+       cfg->direction = GPIOD_LINE_DIRECTION_INPUT;
+
+       for (;;) {
+               optc = getopt_long(argc, argv, shortopts, longopts, &opti);
+               if (optc < 0)
+                       break;
+
+               switch (optc) {
+               case 'a':
+                       cfg->direction = GPIOD_LINE_DIRECTION_AS_IS;
+                       break;
+               case 'b':
+                       cfg->bias = parse_bias_or_die(optarg);
+                       break;
+               case 'c':
+                       cfg->chip_id = optarg;
+                       break;
+               case 'l':
+                       cfg->active_low = true;
+                       break;
+               case 'p':
+                       cfg->hold_period_us = parse_period_or_die(optarg);
+                       break;
+               case 's':
+                       cfg->strict = true;
+                       break;
+               case 'h':
+                       print_help();
+                       exit(EXIT_SUCCESS);
+               case 'v':
+                       print_version();
+                       exit(EXIT_SUCCESS);
+               case '?':
+                       die("try %s --help", get_progname());
+               case 0:
+                       break;
+               default:
+                       abort();
+               }
+       }
+
+       return optind;
+}
+
+int main(int argc, char **argv)
+{
+       int i, num_lines, ret, *values;
+       struct gpiod_line_settings *settings;
+       struct gpiod_request_config *req_cfg;
+       struct gpiod_line_request *request;
+       struct gpiod_line_config *line_cfg;
+       struct gpiod_chip *chip;
+       unsigned int *offsets;
+       struct line_resolver *resolver;
+       struct resolved_line *line;
+       struct config cfg;
+
+       i = parse_config(argc, argv, &cfg);
+       argc -= i;
+       argv += i;
+
+       if (argc < 1)
+               die("at least one GPIO line must be specified");
+
+       resolver = resolve_lines(argc, argv, cfg.chip_id, cfg.strict,
+                                cfg.by_name);
+       validate_resolution(resolver, cfg.chip_id);
+
+       offsets = calloc(resolver->num_lines, sizeof(*offsets));
+       values = calloc(resolver->num_lines, sizeof(*values));
+       if (!offsets || !values)
+               die("out of memory");
+
+       settings = gpiod_line_settings_new();
+       if (!settings)
+               die_perror("unable to allocate line settings");
+
+       gpiod_line_settings_set_direction(settings, cfg.direction);
+
+       if (cfg.bias)
+               gpiod_line_settings_set_bias(settings, cfg.bias);
+
+       if (cfg.active_low)
+               gpiod_line_settings_set_active_low(settings, true);
+
+       req_cfg = gpiod_request_config_new();
+       if (!req_cfg)
+               die_perror("unable to allocate the request config structure");
+
+       line_cfg = gpiod_line_config_new();
+       if (!line_cfg)
+               die_perror("unable to allocate the line config structure");
+
+       gpiod_request_config_set_consumer(req_cfg, "gpioget");
+       for (i = 0; i < resolver->num_chips; i++) {
+               chip = gpiod_chip_open(resolver->chips[i].path);
+               if (!chip)
+                       die_perror("unable to open chip '%s'",
+                                  resolver->chips[i].path);
+
+               num_lines = get_line_offsets_and_values(resolver, i, offsets,
+                                                       NULL);
+
+               gpiod_line_config_reset(line_cfg);
+               ret = gpiod_line_config_add_line_settings(line_cfg, offsets,
+                                                         num_lines, settings);
+               if (ret)
+                       die_perror("unable to add line settings");
+
+               request = gpiod_chip_request_lines(chip, req_cfg, line_cfg);
+               if (!request)
+                       die_perror("unable to request lines");
+
+               if (cfg.hold_period_us)
+                       usleep(cfg.hold_period_us);
+
+               ret = gpiod_line_request_get_values(request, values);
+               if (ret)
+                       die_perror("unable to read GPIO line values");
+
+               set_line_values(resolver, i, values);
+
+               gpiod_line_request_release(request);
+               gpiod_chip_close(chip);
+       }
+       for (i = 0; i < resolver->num_lines; i++) {
+               line = &resolver->lines[i];
+               if (cfg.numeric)
+                       printf("%d", line->value);
+               else
+                       printf("%s=%s", line->id,
+                              line->value ? "active" : "inactive");
+
+               if (i != resolver->num_lines - 1)
+                       printf(" ");
+       }
+       printf("\n");
+
+       free_line_resolver(resolver);
+       gpiod_request_config_free(req_cfg);
+       gpiod_line_config_free(line_cfg);
+       gpiod_line_settings_free(settings);
+       free(offsets);
+       free(values);
+
+       return EXIT_SUCCESS;
+}

tools/gpioinfo.c +70 -2

index ae368fa..5dc28f8 100644
@@ -1,8 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 // SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]>
+// SPDX-FileCopyrightText: 2022 Kent Gibson <[email protected]>
 
-#include <dirent.h>
-#include <errno.h>
 #include <getopt.h>
 #include <gpiod.h>
 #include <stdarg.h>

@@ -12,3 +11,223 @@
 
 #include "tools-common.h"
 
+static void print_help(void)
+{
+       printf("Usage: %s [OPTIONS] [line]...\n", get_progname());
+       printf("\n");
+       printf("Print information about GPIO lines.\n");
+       printf("\n");
+       printf("Lines are specified by name, or optionally by offset if the chip option\n");
+       printf("is provided.\n");
+       printf("\n");
+       printf("If no lines are specified than all lines are displayed.\n");
+       printf("\n");
+       printf("Options:\n");
+       printf("      --by-name\t\ttreat lines as names even if they would parse as an offset\n");
+       printf("  -c, --chip <chip>\trestrict scope to a particular chip\n");
+       printf("  -h, --help\t\tdisplay this help and exit\n");
+       printf("  -s, --strict\t\tcheck all lines - don't assume line names are unique\n");
+       printf("  -v, --version\t\toutput version information and exit\n");
+       print_chip_help();
+}
+
+struct config {
+       bool strict;
+       const char *chip_id;
+       int by_name;
+};
+
+int parse_config(int argc, char **argv, struct config *cfg)
+{
+       int opti, optc;
+       const char *const shortopts = "+c:hsv";
+       const struct option longopts[] = {
+               { "by-name",    no_argument,    &cfg->by_name,  1 },
+               { "chip",       required_argument,      NULL,   'c' },
+               { "help",       no_argument,    NULL,           'h' },
+               { "strict",     no_argument,    NULL,           's' },
+               { "version",    no_argument,    NULL,           'v' },
+               { GETOPT_NULL_LONGOPT },
+       };
+
+       memset(cfg, 0, sizeof(*cfg));
+
+       for (;;) {
+               optc = getopt_long(argc, argv, shortopts, longopts, &opti);
+               if (optc < 0)
+                       break;
+
+               switch (optc) {
+               case 'c':
+                       cfg->chip_id = optarg;
+                       break;
+               case 's':
+                       cfg->strict = true;
+                       break;
+               case 'h':
+                       print_help();
+                       exit(EXIT_SUCCESS);
+               case 'v':
+                       print_version();
+                       exit(EXIT_SUCCESS);
+               case '?':
+                       die("try %s --help", get_progname());
+               case 0:
+                       break;
+               default:
+                       abort();
+               }
+       }
+
+       return optind;

tools/gpioinfo.c line 88

Quoted 6 lines

tools/gpioinfo.c +6 -0

+}
+
+
+// minimal version similar to tools-common that indicates if a line should be
+// printed rather than storing details into the resolver.
+// Does not die on non-unique lines.

C-style comments only please. Same elsewhere.

<snip>

I like the new tools in general. I don't have many issues with the code - you are a much better coder than I am.

I would change the
coding style here and there but I will probably just spend some time on a good clang-format config and use it indiscriminately like I did with black for Python.

Quoted 21 lines · Bartosz Golaszewski

Quoted 20 lines · Kent Gibson

Rework the tool suite to support identifying lines by name and to support operating on the GPIO lines available to the user at once, rather than on one particular GPIO chip.

All tools, other than gpiodetect, now provide the name to (chip,offset) mapping that was previously only performed by gpiofind. As names are not guaranteed to be unique, a --strict option is provided for all tools to either abort the operation or report all lines with the matching name, as appropriate.
By default the tools operate on the first line found with a matching name.

Selection of line by (chip,offset) is still supported with a --chip option, though it restricts the scope of the operation to an individual chip. When the --chip option is specified, the lines are assumed to be identified by offset where they parse as an integer, else by name. To cater for the unusual case where a line name parses as an integer, but is different from the offset, the --by-name option forces the lines to be identified by name.

Quoted 9 lines

Quoted 5 lines

The updated tools are intentionally NOT backwardly compatible with the previous tools. Using old command lines with the updated tools will almost certainly fail, though migrating old command lines is generally as simple as adding a '-c' before the chip.

While at it: how about adding the --consumer/-C switch to specify a consumer string other than the name of the program?

Ironically I added that to the Rust version, for the long lived commands anyway, so it could better emulate the C version for testing purposes.
But could be generally useful, so ok.

I only used the long form there to avoid confusion with -c (as they are visually very similar) and following the principle that rarely used options only get a long form, so I will omit the short -C option - unless you insist.

Quoted 24 lines

Quoted 8 lines

In addition the individual tools are modified as follows:

gpiodetect:

Add the option to select individual chips.

gpioinfo:

Quoted 9 lines

Change the focus from chips to lines, so the scope can be an individual line, a subset of lines, all lines on a particular chip, or all the lines available to the user. For line scope a single line summary is output for each line. For chip scope the existing format displaying a summary of the chip and each of its lines is retained.

Line attributes are consolidated into a list format, and are extended to cover all attributes supported by uAPI v2.

One change in the output that bothers me is the removal of quotation marks around the line name and consumer. I did that in v1 to visually distinguish between unnamed/unused lines and those that are named. I know it's highly unlikely that a line would be named "unnamed" (sic!) but still:

Quoted 4 lines

line 0: "foo"
line 1: unnamed

looks more intuitive to me.

I disagree on this one. In the longer term all lines should be named and then the quotes just become pointless noise, and require more work to parse.

Quoted 4 lines

Same for the consumer as with your current
version, if the consumer string has spaces in it, it will look like
this: consumer=foo bar. I think consumer="foo bar" would be easier to parse.

For this very reason, the consumer is explicitly listed last, so the consumer name matches everything between the "consumer=" and end of line.

Unless consumer names with spaces are very common in the wild then quotes only add more clutter.

Quoted 47 lines

Quoted 41 lines

gpioget:

The default output format is becomes line=value, as per the input for gpioset, and the value is reported as active or inactive, rather than 0 or 1.
The previous format is available using the --numeric option.

Add an optional hold period between requesting a line and reading the value to allow the line to settle once the requested configuration has been applied (e.g. bias).

gpiomon:

Consolidate the edge options into a single option.

Add a debounce period option.

Add options to report event times as UTC or localtime.

Add format specifiers for GPIO chip path, line name, stringified event type, and event time as a datetime.

Rearrange default output format to place fields with more predicable widths to the left, and to separate major field groups with tabs. Lines are identified consistent with the command line.

gpioset:

Add a hold period option that specifies the minimum period the line value must be held for. This applies to all set options.

Support line values specified as active/inactive, on/off and true/false, as well as 1/0.

Add a toggle option that specifies a time sequence over which the requested lines should be toggled. If the sequence is 0 terminated then gpioset exits when the sequence completes, else it repeats the sequence. This allows for anything from simple blinkers to bit bashing from the command line. e.g. gpioset -t 500ms LED=on

Quoted 5 lines

Add an interactive option to provide a shell-like interface to allow manual or scripted manipulation of requested lines. A basic command set allows lines to be get, set, or toggled, and to insert sleeps between operations.

Quoted 4 lines

As discussed elsewhere - it would be great if this part was optional and configurable at build-time so that the dependency on libedit could be dropped if unavailable.

Agreed.

Quoted 11 lines

Quoted 4 lines

Remove the --mode, --sec, and --usec options.
The combination of hold period and interactive mode provide functionality equivalent to the old --mode options.

I have one problem with that - I think the basic functionality of:
"take a line, set its value and wait for a signal" would still be useful. As it is now, I'm not sure how to make gpioset just hold a line without calling the GPIO_V2_LINE_SET_VALUES_IOCTL ioctl periodically.

I forgot to mention the daemonize option here, so

gpioset -z myline=1

will do that.

(or

gpioset -i myline=1

if you want to keep the process in the foreground.)

I'll add something to the daemonize help to highlight that it will hold the line until killed. Is that sufficient?

Quoted 86 lines

Quoted 86 lines

Signed-off-by Kent Gibson <[email protected]>

---
 configure.ac         |   8 +-
 tools/Makefile.am    |   2 +-
 tools/gpiodetect.c   | 113 +++++-
 tools/gpioget.c      | 204 ++++++++++-
 tools/gpioinfo.c     | 223 +++++++++++-
 tools/gpiomon.c      | 450 +++++++++++++++++++++++-
 tools/gpioset.c      | 815 ++++++++++++++++++++++++++++++++++++++++++-
 tools/tools-common.c | 713 ++++++++++++++++++++++++++++++++++++-
 tools/tools-common.h |  99 +++++-
 9 files changed, 2585 insertions(+), 42 deletions(-)

configure.ac +4 -4

index 6ac1d8e..c8033c5 100644
@@ -106,14 +106,14 @@ AC_DEFUN([FUNC_NOT_FOUND_TOOLS],
 AC_DEFUN([HEADER_NOT_FOUND_TOOLS],
        [ERR_NOT_FOUND([$1 header], [tools])])
 
+AC_DEFUN([LIB_NOT_FOUND_TOOLS],
+       [ERR_NOT_FOUND([lib$1], [tools])])
+
 if test "x$with_tools" = xtrue
 then
        # These are only needed to build tools
-       AC_CHECK_FUNC([basename], [], [FUNC_NOT_FOUND_TOOLS([basename])])
        AC_CHECK_FUNC([daemon], [], [FUNC_NOT_FOUND_TOOLS([daemon])])
-       AC_CHECK_FUNC([signalfd], [], [FUNC_NOT_FOUND_TOOLS([signalfd])])
-       AC_CHECK_FUNC([setlinebuf], [], [FUNC_NOT_FOUND_TOOLS([setlinebuf])])
-       AC_CHECK_HEADERS([sys/signalfd.h], [], [HEADER_NOT_FOUND_TOOLS([sys/signalfd.h])])
+       PKG_CHECK_MODULES(LIBEDIT, libedit)
 fi
 
 AC_ARG_ENABLE([tests],

tools/Makefile.am +1 -1

index fc074b9..c956314 100644
@@ -7,7 +7,7 @@ AM_CFLAGS += -Wall -Wextra -g -std=gnu89
 noinst_LTLIBRARIES = libtools-common.la
 libtools_common_la_SOURCES = tools-common.c tools-common.h
 
-LDADD = libtools-common.la $(top_builddir)/lib/libgpiod.la
+LDADD = libtools-common.la $(top_builddir)/lib/libgpiod.la $(LIBEDIT_LIBS)
 
 bin_PROGRAMS = gpiodetect gpioinfo gpioget gpioset gpiomon
 

tools/gpiodetect.c +20 -2

index 30bde32..910fe9e 100644
@@ -1,8 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 // SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]>
+// SPDX-FileCopyrightText: 2022 Kent Gibson <[email protected]>
 
-#include <dirent.h>
-#include <errno.h>
 #include <getopt.h>
 #include <gpiod.h>
 #include <stdio.h>

@@ -11,3 +10,113 @@
 
 #include "tools-common.h"
 
+static void print_help(void)
+{
+       printf("Usage: %s [OPTIONS] [chip]...\n", get_progname());
+       printf("\n");
+       printf("List GPIO chips, print their labels and number of GPIO lines.\n");
+       printf("\n");
+       printf("Chips may be identified by number, name, or path.\n");
+       printf("e.g. '0', 'gpiochip0', and '/dev/gpiochip0' all refer to the same chip.\n");
+       printf("\n");
+       printf("If no chips are specified then all chips are listed.\n");
+       printf("\n");
+       printf("Options:\n");
+       printf("  -h, --help\t\tdisplay this help and exit\n");
+       printf("  -v, --version\t\toutput version information and exit\n");
+}
+
+int parse_config(int argc, char **argv)
+{
+       int optc, opti;

Quoted 9 lines

tools/gpiodetect.c line 37

Quoted 6 lines

tools/gpiodetect.c +6 -0

+       const char *const shortopts = "+hv";
+       const struct option longopts[] = {
+               { "help",       no_argument,    NULL,   'h' },
+               { "version",    no_argument,    NULL,   'v' },
+               { GETOPT_NULL_LONGOPT },
+       };

This can be static as there are no addresses of flag variables assigned.

Indeed - and now you mention it I notice a few other things that should be static too.

Quoted 7 lines

tools/gpiodetect.c

Quoted 4 lines

tools/gpiodetect.c +4 -0

+
+// minimal version similar to tools-common that indicates if a line should be
+// printed rather than storing details into the resolver.
+// Does not die on non-unique lines.

C-style comments only please. Same elsewhere.

Yeah - sorry again - I'm so used to that style that I don't even notice I'm doing it.

Quoted 2 lines

<snip>

Quoted 2 lines

I like the new tools in general. I don't have many issues with the code - you are a much better coder than I am.

That's being a bit harsh.

One thing I was considering was reworking the resolver so it would be more suitable for general use, and move it to core libgpiod so apps could more readily perform line name discovery.

Quoted 5 lines

I would change the
coding style here and there but I will probably just spend some time on a good clang-format config and use it indiscriminately like I did with black for Python.

That could be useful. Why doesn't the kernel do that?

And that reminds me - I still need to circle back and take another look through the Python bindings.

Quoted 38 lines · Kent Gibson

Quoted 21 lines · Bartosz Golaszewski

Quoted 20 lines · Kent Gibson

Rework the tool suite to support identifying lines by name and to support operating on the GPIO lines available to the user at once, rather than on one particular GPIO chip.

All tools, other than gpiodetect, now provide the name to (chip,offset) mapping that was previously only performed by gpiofind. As names are not guaranteed to be unique, a --strict option is provided for all tools to either abort the operation or report all lines with the matching name, as appropriate.
By default the tools operate on the first line found with a matching name.

Selection of line by (chip,offset) is still supported with a --chip option, though it restricts the scope of the operation to an individual chip. When the --chip option is specified, the lines are assumed to be identified by offset where they parse as an integer, else by name. To cater for the unusual case where a line name parses as an integer, but is different from the offset, the --by-name option forces the lines to be identified by name.

Quoted 9 lines

Quoted 5 lines

The updated tools are intentionally NOT backwardly compatible with the previous tools. Using old command lines with the updated tools will almost certainly fail, though migrating old command lines is generally as simple as adding a '-c' before the chip.

While at it: how about adding the --consumer/-C switch to specify a consumer string other than the name of the program?

Ironically I added that to the Rust version, for the long lived commands anyway, so it could better emulate the C version for testing purposes.
But could be generally useful, so ok.

Quoted 5 lines

I only used the long form there to avoid confusion with -c (as they are visually very similar) and following the principle that rarely used options only get a long form, so I will omit the short -C option - unless you insist.

I don't see why it would hurt but I'm fine either way.

Quoted 24 lines

Quoted 24 lines

Quoted 8 lines

In addition the individual tools are modified as follows:

gpiodetect:

Add the option to select individual chips.

gpioinfo:

Quoted 9 lines

Change the focus from chips to lines, so the scope can be an individual line, a subset of lines, all lines on a particular chip, or all the lines available to the user. For line scope a single line summary is output for each line. For chip scope the existing format displaying a summary of the chip and each of its lines is retained.

Line attributes are consolidated into a list format, and are extended to cover all attributes supported by uAPI v2.

One change in the output that bothers me is the removal of quotation marks around the line name and consumer. I did that in v1 to visually distinguish between unnamed/unused lines and those that are named. I know it's highly unlikely that a line would be named "unnamed" (sic!) but still:

Quoted 9 lines

Quoted 4 lines

line 0: "foo"
line 1: unnamed

looks more intuitive to me.

I disagree on this one. In the longer term all lines should be named and then the quotes just become pointless noise, and require more work to parse.

I insist on this one as just a quick glance at the current values of gpio-line-names DT properties in the kernel show all kinds of funky names - not only including spaces but also various other characters like [, ], #, /, - and that not. I think it makes a lot of sense to delimit them visually with quotes.

Quoted 9 lines

Quoted 4 lines

Same for the consumer as with your current
version, if the consumer string has spaces in it, it will look like
this: consumer=foo bar. I think consumer="foo bar" would be easier to parse.

For this very reason, the consumer is explicitly listed last, so the consumer name matches everything between the "consumer=" and end of line.

Quoted 3 lines

Unless consumer names with spaces are very common in the wild then quotes only add more clutter.

We can't know, but instead of putting it last, I'd just treat it like every other flag and instead delimit the name with "".

The tool is mostly aimed at humans anyway and if someone's brave enough to parse the output with a script then a cut-based one-liner is all they need, no?

Quoted 25 lines

Quoted 6 lines

Quoted 5 lines

Remove the --mode, --sec, and --usec options.
The combination of hold period and interactive mode provide functionality equivalent to the old --mode options.

Quoted 6 lines

I have one problem with that - I think the basic functionality of:
"take a line, set its value and wait for a signal" would still be useful. As it is now, I'm not sure how to make gpioset just hold a line without calling the GPIO_V2_LINE_SET_VALUES_IOCTL ioctl periodically.

I forgot to mention the daemonize option here, so

gpioset -z myline=1

will do that.

(or

gpioset -i myline=1

if you want to keep the process in the foreground.)

Quoted 3 lines

I'll add something to the daemonize help to highlight that it will hold the line until killed. Is that sufficient?

What if I don't want to daemonize the program nor open the interactive mode? Why not just make the default behavior of gpioset foo=active
be: stay alive until interrupted? The current immediate exiting is mostly useless anyway.

Quoted 14 lines

Quoted 8 lines

tools/gpioinfo.c line 88

Quoted 5 lines

tools/gpioinfo.c +4 -0

+
+// minimal version similar to tools-common that indicates if a line should be
+// printed rather than storing details into the resolver.
+// Does not die on non-unique lines.

C-style comments only please. Same elsewhere.

Yeah - sorry again - I'm so used to that style that I don't even notice I'm doing it.

Quoted 2 lines

<snip>

Quoted 9 lines

Quoted 2 lines

I like the new tools in general. I don't have many issues with the code - you are a much better coder than I am.

That's being a bit harsh.

One thing I was considering was reworking the resolver so it would be more suitable for general use, and move it to core libgpiod so apps could more readily perform line name discovery.

Hmm, I think it's unnecessary clutter in the library. I was thinking about whether to put the upcoming fdinfo parsing into the library and figured that it's more of a suitable candidate for a new command-line tool as the library should focus on the character device exclusively IMO.

Quoted 7 lines

I insist on this one as just a quick glance at the current values of gpio-line-names DT properties in the kernel show all kinds of funky names - not only including spaces but also various other characters like [, ], #, /, - and that not. I think it makes a lot of sense to delimit them visually with quotes.

Good grief, what a mess. Would've been nice to have some naming conventions in place, but too late now...

Which reminds me - any guarantees the name, particularly the consumer name, is valid UTF-8?
If not then need to double check how the Rust bindings deal with that. Throwing a utf8_error would not be appropriate if it isn't guaranteed to be UTF-8.

Quoted 9 lines

Quoted 9 lines

Quoted 4 lines

Same for the consumer as with your current
version, if the consumer string has spaces in it, it will look like
this: consumer=foo bar. I think consumer="foo bar" would be easier to parse.

For this very reason, the consumer is explicitly listed last, so the consumer name matches everything between the "consumer=" and end of line.

Quoted 6 lines

Quoted 3 lines

Unless consumer names with spaces are very common in the wild then quotes only add more clutter.

We can't know, but instead of putting it last, I'd just treat it like every other flag and instead delimit the name with "".

It is also last based on length predicatability - more predicatable first, and the other attributes are more predictable.

Quoted 5 lines

The tool is mostly aimed at humans anyway and if someone's brave enough to parse the output with a script then a cut-based one-liner is all they need, no?

Sure, but don't underestimate the ability of users to complain when a tool isn't quite as easy to use as they would like.

Hmmm, how about an option to quote all string attributes? Say --quoted?
Or if quoted is the default then --unquoted to remove the clutter?

<snip>

Quoted 25 lines

Quoted 25 lines

Quoted 6 lines

Quoted 5 lines

Remove the --mode, --sec, and --usec options.
The combination of hold period and interactive mode provide functionality equivalent to the old --mode options.

Quoted 6 lines

I have one problem with that - I think the basic functionality of:
"take a line, set its value and wait for a signal" would still be useful. As it is now, I'm not sure how to make gpioset just hold a line without calling the GPIO_V2_LINE_SET_VALUES_IOCTL ioctl periodically.

I forgot to mention the daemonize option here, so

gpioset -z myline=1

will do that.

(or

gpioset -i myline=1

if you want to keep the process in the foreground.)

Quoted 9 lines

Quoted 3 lines

I'll add something to the daemonize help to highlight that it will hold the line until killed. Is that sufficient?

What if I don't want to daemonize the program nor open the interactive mode? Why not just make the default behavior of gpioset foo=active
be: stay alive until interrupted? The current immediate exiting is mostly useless anyway.

Hmmm, not when used with the --hold-period, so a script could call a series of gpiosets to generate a waveform. Though that doesn't work well, as the line is released between sets, and the whole point of the toggle option and interactive modes is to provide better alternatives for doing that.

So yeah, true - I guess I just blindly followed the v1 behaviour on that.
- the standard behaviour should be to not exit.
The only exception being a zero-terminated toggle sequence.

That should also reduce the chance of users complaining that gpioset doesn't work where they assume the set persists after gpioset exists. OTOH I'm willing to bet we get at least one complaint that gpioset hangs... :|.

<snip>

Quoted 24 lines

Quoted 14 lines

Quoted 8 lines

tools/gpioinfo.c line 88

Quoted 5 lines

tools/gpioinfo.c +4 -0

+
+// minimal version similar to tools-common that indicates if a line should be
+// printed rather than storing details into the resolver.
+// Does not die on non-unique lines.

C-style comments only please. Same elsewhere.

Yeah - sorry again - I'm so used to that style that I don't even notice I'm doing it.

Quoted 2 lines

<snip>

Quoted 9 lines

Quoted 2 lines

I like the new tools in general. I don't have many issues with the code - you are a much better coder than I am.

That's being a bit harsh.

One thing I was considering was reworking the resolver so it would be more suitable for general use, and move it to core libgpiod so apps could more readily perform line name discovery.

Quoted 6 lines

Hmm, I think it's unnecessary clutter in the library. I was thinking about whether to put the upcoming fdinfo parsing into the library and figured that it's more of a suitable candidate for a new command-line tool as the library should focus on the character device exclusively IMO.

I think you were right the first time. The fdinfo is another aspect of the cdev GPIO uAPI - it isn't just the ioctls.
If the GPIO uAPI were extended to use AIO, would that not go in libgpiod as well?

And agreed that fdinfo warrants another tool.

It makes sense to me to add higher level abstractions or functionality where that greatly simplifies apps.
Speaking from experience, looking up up the line chip/offset based on name is at best tedious and at worst a serious PITA.
If it isn't available in libgpiod then apps are more likely to do it out-of-band, such as by using gpioinfo to generate app configuration, or just stick to specifying lines by chip/offset, which is a bit sad.

But, as per the unified tool binary, this can always be added later.

Quoted 1 line · Kent Gibson

[snip]

Quoted 8 lines

Quoted 7 lines

I insist on this one as just a quick glance at the current values of gpio-line-names DT properties in the kernel show all kinds of funky names - not only including spaces but also various other characters like [, ], #, /, - and that not. I think it makes a lot of sense to delimit them visually with quotes.

Quoted 9 lines

Good grief, what a mess. Would've been nice to have some naming conventions in place, but too late now...

Which reminds me - any guarantees the name, particularly the consumer name, is valid UTF-8?
If not then need to double check how the Rust bindings deal with that. Throwing a utf8_error would not be appropriate if it isn't guaranteed to be UTF-8.

There's nothing that technically prohibits anyone from using non-UTF8 characters and also current libgpiod seems to be handling such chars just fine (just tested to make sure).

Quoted 28 lines

Quoted 9 lines

Quoted 9 lines

Quoted 4 lines

Same for the consumer as with your current
version, if the consumer string has spaces in it, it will look like
this: consumer=foo bar. I think consumer="foo bar" would be easier to parse.

For this very reason, the consumer is explicitly listed last, so the consumer name matches everything between the "consumer=" and end of line.

Quoted 6 lines

Quoted 3 lines

Unless consumer names with spaces are very common in the wild then quotes only add more clutter.

We can't know, but instead of putting it last, I'd just treat it like every other flag and instead delimit the name with "".

It is also last based on length predicatability - more predicatable first, and the other attributes are more predictable.

Quoted 5 lines

The tool is mostly aimed at humans anyway and if someone's brave enough to parse the output with a script then a cut-based one-liner is all they need, no?

Sure, but don't underestimate the ability of users to complain when a tool isn't quite as easy to use as they would like.

Quoted 4 lines

Hmmm, how about an option to quote all string attributes? Say --quoted?
Or if quoted is the default then --unquoted to remove the clutter?

I don't feel like it's necessary. How would you handle line names with spaces? It would add an additional column? That would be actually harder to parse than quoted names IMO.

Quoted 12 lines

Quoted 11 lines

Quoted 4 lines

One thing I was considering was reworking the resolver so it would be more suitable for general use, and move it to core libgpiod so apps could more readily perform line name discovery.

Hmm, I think it's unnecessary clutter in the library. I was thinking about whether to put the upcoming fdinfo parsing into the library and figured that it's more of a suitable candidate for a new command-line tool as the library should focus on the character device exclusively IMO.

Quoted 7 lines

I think you were right the first time. The fdinfo is another aspect of the cdev GPIO uAPI - it isn't just the ioctls.
If the GPIO uAPI were extended to use AIO, would that not go in libgpiod as well?

And agreed that fdinfo warrants another tool.

I was thinking that both gpioinfo could use that (e.g. consumer="foobar",consumer_pid=12345) and we could have something like:

# gpioowner foobar
12345

Quoted 8 lines

It makes sense to me to add higher level abstractions or functionality where that greatly simplifies apps.
Speaking from experience, looking up up the line chip/offset based on name is at best tedious and at worst a serious PITA.
If it isn't available in libgpiod then apps are more likely to do it out-of-band, such as by using gpioinfo to generate app configuration, or just stick to specifying lines by chip/offset, which is a bit sad.

Quoted 2 lines

But, as per the unified tool binary, this can always be added later.

Ok, I'm buying this. Let's think about adding this to the core library.

Add a gpiowatch tool, based on gpiomon, to report line info change events read from chip file descriptors.

Inspired by the gpio-watch tool in the linux kernel, but with gpiomon features such as custom formatted output, filtering events of interest and exiting after a number of events, so more useful for scripting.

Default output is minimalist, so just time, event type and line id. Full event details are available using the custom formatted output.

Signed-off-by Kent Gibson <[email protected]>

---

Changes v2 -> v3:
- Minimise the default output to more closely match gpiomon.
- Add --format option for when more detail is required.
- Add --num-events option to exit after a number of events.
- Add --event option to report only specific event types.
- Add --quiet option to not print events.
- fix monotonic to realtime conversion on 32 bit platforms.

Quoted 15 lines · Kent Gibson

Add a gpiowatch tool, based on gpiomon, to report line info change events read from chip file descriptors.

Inspired by the gpio-watch tool in the linux kernel, but with gpiomon features such as custom formatted output, filtering events of interest and exiting after a number of events, so more useful for scripting.

Default output is minimalist, so just time, event type and line id. Full event details are available using the custom formatted output.

Signed-off-by Kent Gibson <[email protected]>

---

Quoted 8 lines

Changes v2 -> v3:
- Minimise the default output to more closely match gpiomon.
- Add --format option for when more detail is required.
- Add --num-events option to exit after a number of events.
- Add --event option to report only specific event types.
- Add --quiet option to not print events.
- fix monotonic to realtime conversion on 32 bit platforms.

Nice and clean, I don't have any issues other than the regular coding-style bikeshedding.

What happened to the idea we've been floating about creating a single, busyboxy executable with links rather than separate executables? Have we ever agreed on it?

Quoted 25 lines · Bartosz Golaszewski

Quoted 23 lines · Kent Gibson

Add a gpiowatch tool, based on gpiomon, to report line info change events read from chip file descriptors.

Inspired by the gpio-watch tool in the linux kernel, but with gpiomon features such as custom formatted output, filtering events of interest and exiting after a number of events, so more useful for scripting.

Default output is minimalist, so just time, event type and line id. Full event details are available using the custom formatted output.

Signed-off-by Kent Gibson <[email protected]>

---

Changes v2 -> v3:
- Minimise the default output to more closely match gpiomon.
- Add --format option for when more detail is required.
- Add --num-events option to exit after a number of events.
- Add --event option to report only specific event types.
- Add --quiet option to not print events.
- fix monotonic to realtime conversion on 32 bit platforms.

Quoted 3 lines

Nice and clean, I don't have any issues other than the regular coding-style bikeshedding.

Will be renamed to gpionotify for v5, ok?

Quoted 5 lines

What happened to the idea we've been floating about creating a single, busyboxy executable with links rather than separate executables? Have we ever agreed on it?

Yeah, last we spoke on it we agreed it was of dubious value and a low priority, so I didn't go anywhere with it. You've reconsidered?

Quoted 27 lines · Kent Gibson

Quoted 25 lines · Bartosz Golaszewski

Quoted 23 lines · Kent Gibson

Add a gpiowatch tool, based on gpiomon, to report line info change events read from chip file descriptors.

Inspired by the gpio-watch tool in the linux kernel, but with gpiomon features such as custom formatted output, filtering events of interest and exiting after a number of events, so more useful for scripting.

Default output is minimalist, so just time, event type and line id. Full event details are available using the custom formatted output.

Signed-off-by Kent Gibson <[email protected]>

---

Changes v2 -> v3:
- Minimise the default output to more closely match gpiomon.
- Add --format option for when more detail is required.
- Add --num-events option to exit after a number of events.
- Add --event option to report only specific event types.
- Add --quiet option to not print events.
- fix monotonic to realtime conversion on 32 bit platforms.

Quoted 5 lines

Quoted 3 lines

Nice and clean, I don't have any issues other than the regular coding-style bikeshedding.

Will be renamed to gpionotify for v5, ok?

Yes, sure, just like discussed.

Quoted 9 lines

Quoted 5 lines

What happened to the idea we've been floating about creating a single, busyboxy executable with links rather than separate executables? Have we ever agreed on it?

Yeah, last we spoke on it we agreed it was of dubious value and a low priority, so I didn't go anywhere with it. You've reconsidered?

I'm seeing that the tools-common.c file grew quite a bit after your rework (which is good, a lot of stuff has been generalized) which makes me think it wouldn't be a bad idea to not include it 6 times separately. It's either a libgpio-tools or putting this stuff into the same executable?

Anyway, we can do it later.