| Message ID | 20220627134447.81927-4-warthog618@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | tools: line name focussed rework | expand |
On Mon, Jun 27, 2022 at 3:46 PM Kent Gibson <warthog618@gmail.com> wrote: > > Add a gpiowatch tool, based on gpiomon, but reporting line info change > events similar to the gpio-watch tool in the linux kernel. > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > --- > man/Makefile.am | 2 +- > tools/.gitignore | 1 + > tools/Makefile.am | 4 +- > tools/gpiowatch.c | 214 ++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 219 insertions(+), 2 deletions(-) > create mode 100644 tools/gpiowatch.c > > diff --git a/man/Makefile.am b/man/Makefile.am > index 4d2c29b..3badd3b 100644 > --- a/man/Makefile.am > +++ b/man/Makefile.am > @@ -3,7 +3,7 @@ > > if WITH_MANPAGES > > -dist_man1_MANS = gpiodetect.man gpioinfo.man gpioget.man gpioset.man gpiofind.man gpiomon.man > +dist_man1_MANS = gpiodetect.man gpioinfo.man gpioget.man gpioset.man gpiofind.man gpiomon.man gpiowatch.man > > %.man: $(top_builddir)/tools/$(*F) > help2man $(top_builddir)/tools/$(*F) --include=$(srcdir)/template --output=$(builddir)/$@ --no-info > diff --git a/tools/.gitignore b/tools/.gitignore > index 0d53de9..6175e26 100644 > --- a/tools/.gitignore > +++ b/tools/.gitignore > @@ -7,3 +7,4 @@ gpioget > gpioset > gpiomon > gpiofind > +gpiowatch > diff --git a/tools/Makefile.am b/tools/Makefile.am > index 4a13266..8bb2cac 100644 > --- a/tools/Makefile.am > +++ b/tools/Makefile.am > @@ -9,7 +9,7 @@ libtools_common_la_SOURCES = tools-common.c tools-common.h > > LDADD = libtools-common.la $(top_builddir)/lib/libgpiod.la > > -bin_PROGRAMS = gpiodetect gpioinfo gpioget gpioset gpiomon gpiofind > +bin_PROGRAMS = gpiodetect gpioinfo gpioget gpioset gpiomon gpiofind gpiowatch > > gpiodetect_SOURCES = gpiodetect.c > > @@ -23,6 +23,8 @@ gpiomon_SOURCES = gpiomon.c > > gpiofind_SOURCES = gpiofind.c > > +gpiowatch_SOURCES = gpiowatch.c > + > EXTRA_DIST = gpio-tools-test gpio-tools-test.bats > > if WITH_TESTS > diff --git a/tools/gpiowatch.c b/tools/gpiowatch.c > new file mode 100644 > index 0000000..e6bfeb6 > --- /dev/null > +++ b/tools/gpiowatch.c > @@ -0,0 +1,214 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +// SPDX-FileCopyrightText: 2017-2022 Bartosz Golaszewski <bartekgola@gmail.com> > + > +#include <getopt.h> > +#include <gpiod.h> > +#include <inttypes.h> > +#include <poll.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > +#include <time.h> > + > +#include "tools-common.h" > + > +static int by_name; > +static int event_clock_mode; > +static int banner; > + > +static const struct option longopts[] = { > + { "banner", no_argument, &banner, 1 }, > + { "by-name", no_argument, &by_name, 1 }, > + { "chip", required_argument, NULL, 'c' }, > + { "help", no_argument, NULL, 'h' }, > + { "localtime", no_argument, &event_clock_mode, 2 }, > + { "strict", no_argument, NULL, 's' }, > + { "utc", no_argument, &event_clock_mode, 1 }, > + { "version", no_argument, NULL, 'v' }, > + { GETOPT_NULL_LONGOPT }, > +}; > + > +static const char *const shortopts = "+c:shv"; > + > +static void print_help(void) > +{ > + printf("Usage: %s [OPTIONS] <line> ...\n", get_progname()); > + printf("\n"); > + printf("Wait for changes to info on GPIO lines and print them to standard output.\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(" --banner\t\tdisplay a banner on successful startup\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(" --localtime\treport event time as a local time (default is monotonic)\n"); > + printf(" -s, --strict\t\tabort if requested line names are not unique\n"); > + printf(" --utc\t\treport event time as UTC (default is monotonic)\n"); > + printf(" -v, --version\t\toutput version information and exit\n"); > + print_chip_help(); > +} > + > +struct config { > + bool strict; > + const char *chip_id; > +}; Let's either have all options in a local config struct (preferred) or all of them as global variables, otherwise it's confusing. If you want to use flags in long opts you can always define that structure within the function calling getopt(). > + > +int parse_config(int argc, char **argv, struct config *cfg) > +{ > + int opti, optc; > + > + 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; > +} > + > +static void print_banner(int num_lines, char **lines) > +{ > + int i; > + > + if (num_lines > 1) { > + printf("Watching lines "); > + for (i = 0; i < num_lines - 1; i++) > + printf("%s, ", lines[i]); > + printf("and %s...\n", lines[i]); > + } else { > + printf("Watching line %s ...\n", lines[0]); > + } > +} > + > +static void event_print(struct gpiod_info_event *event, const char *chip_id) > +{ > + struct gpiod_line_info *info; > + uint64_t evtime, before, after, mono; > + char *evname; > + int evtype; > + struct timespec ts; > + > + info = gpiod_info_event_get_line_info(event); > + evtime = gpiod_info_event_get_timestamp_ns(event); > + evtype = gpiod_info_event_get_event_type(event); > + > + switch (evtype) { > + case GPIOD_INFO_EVENT_LINE_REQUESTED: > + evname = "REQUESTED"; > + break; > + case GPIOD_INFO_EVENT_LINE_RELEASED: > + evname = "RELEASED "; > + break; > + case GPIOD_INFO_EVENT_LINE_CONFIG_CHANGED: > + evname = "RECONFIG "; > + break; > + default: > + evname = "UNKNOWN "; > + } Newline for readability, please. > + if (event_clock_mode) { C-style comments only except for SPDX headers please. > + // map clock monotonic to realtime, as uAPI only supports CLOCK_MONOTONIC > + clock_gettime(CLOCK_REALTIME, &ts); > + before = ts.tv_nsec + ts.tv_sec * 1000000000; > + clock_gettime(CLOCK_MONOTONIC, &ts); > + mono = ts.tv_nsec + ts.tv_sec * 1000000000; > + clock_gettime(CLOCK_REALTIME, &ts); > + after = ts.tv_nsec + ts.tv_sec * 1000000000; > + evtime += (after/2 - mono + before/2); > + } Moar newlines, I really like between blocks of code, it really helps me, thanks. > + print_event_time(evtime, event_clock_mode); > + printf(" %s", evname); > + if (chip_id) > + printf(" %s %d", chip_id, gpiod_line_info_get_offset(info)); > + print_line_info(info); > + printf("\n"); > +} > + > +int main(int argc, char **argv) > +{ > + int i, j; > + struct gpiod_chip **chips; > + struct pollfd *pollfds; > + struct gpiod_chip *chip; > + struct line_resolver *resolver; > + struct gpiod_info_event *event; > + struct config cfg; > + > + i = parse_config(argc, argv, &cfg); > + argc -= optind; > + argv += optind; > + > + if (argc < 1) > + die("at least one GPIO line must be specified"); > + > + if (argc > 64) > + die("too many lines given"); > + > + resolver = resolve_lines(argc, argv, cfg.chip_id, cfg.strict, by_name); > + chips = calloc(resolver->num_chips, sizeof(*chips)); > + pollfds = calloc(resolver->num_chips, sizeof(*pollfds)); > + if (!pollfds) > + die("out of memory"); > + for (i = 0; i < resolver->num_chips; i++) { > + chip = gpiod_chip_open(resolver->chip_paths[i]); > + if (!chip) > + die_perror("unable to open chip %s", resolver->chip_paths[i]); > + Don't other tools do the same thing basically (resolving and opening chips)? Can't we fold it into tools-common.c so that we get a list of open chips? > + for (j = 0; j < resolver->num_lines; j++) > + if (resolver->lines[j].chip_path == resolver->chip_paths[i]) > + if (!gpiod_chip_watch_line_info(chip, resolver->lines[j].offset)) > + die_perror("unable to watch line on chip %s", > + resolver->chip_paths[i]); > + > + chips[i] = chip; > + pollfds[i].fd = gpiod_chip_get_fd(chip); > + pollfds[i].events = POLLIN; > + } > + > + if (banner) > + print_banner(argc, argv); > + > + for (;;) { > + if (poll(pollfds, resolver->num_chips, -1) < 0) > + die_perror("error polling for events"); > + > + for (i = 0; i < resolver->num_chips; i++) { > + if (pollfds[i].revents == 0) > + continue; > + > + event = gpiod_chip_read_info_event(chips[i]); > + event_print(event, cfg.chip_id); > + } > + } > + for (i = 0; i < resolver->num_chips; i++) > + gpiod_chip_close(chips[i]); > + free(chips); > + free_line_resolver(resolver); > + > + return EXIT_SUCCESS; > +} > -- > 2.36.1 > Looks good to me, I would have probably added a machine-readable output formatting like gpiomon but we can always extend it later. Bart
On Wed, Jul 06, 2022 at 10:46:28PM +0200, Bartosz Golaszewski wrote: > On Mon, Jun 27, 2022 at 3:46 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > Add a gpiowatch tool, based on gpiomon, but reporting line info change > > events similar to the gpio-watch tool in the linux kernel. > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > --- > > man/Makefile.am | 2 +- > > tools/.gitignore | 1 + > > tools/Makefile.am | 4 +- > > tools/gpiowatch.c | 214 ++++++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 219 insertions(+), 2 deletions(-) > > create mode 100644 tools/gpiowatch.c > > > > diff --git a/man/Makefile.am b/man/Makefile.am > > index 4d2c29b..3badd3b 100644 > > --- a/man/Makefile.am > > +++ b/man/Makefile.am > > @@ -3,7 +3,7 @@ > > > > if WITH_MANPAGES > > > > -dist_man1_MANS = gpiodetect.man gpioinfo.man gpioget.man gpioset.man gpiofind.man gpiomon.man > > +dist_man1_MANS = gpiodetect.man gpioinfo.man gpioget.man gpioset.man gpiofind.man gpiomon.man gpiowatch.man > > > > %.man: $(top_builddir)/tools/$(*F) > > help2man $(top_builddir)/tools/$(*F) --include=$(srcdir)/template --output=$(builddir)/$@ --no-info > > diff --git a/tools/.gitignore b/tools/.gitignore > > index 0d53de9..6175e26 100644 > > --- a/tools/.gitignore > > +++ b/tools/.gitignore > > @@ -7,3 +7,4 @@ gpioget > > gpioset > > gpiomon > > gpiofind > > +gpiowatch > > diff --git a/tools/Makefile.am b/tools/Makefile.am > > index 4a13266..8bb2cac 100644 > > --- a/tools/Makefile.am > > +++ b/tools/Makefile.am > > @@ -9,7 +9,7 @@ libtools_common_la_SOURCES = tools-common.c tools-common.h > > > > LDADD = libtools-common.la $(top_builddir)/lib/libgpiod.la > > > > -bin_PROGRAMS = gpiodetect gpioinfo gpioget gpioset gpiomon gpiofind > > +bin_PROGRAMS = gpiodetect gpioinfo gpioget gpioset gpiomon gpiofind gpiowatch > > > > gpiodetect_SOURCES = gpiodetect.c > > > > @@ -23,6 +23,8 @@ gpiomon_SOURCES = gpiomon.c > > > > gpiofind_SOURCES = gpiofind.c > > > > +gpiowatch_SOURCES = gpiowatch.c > > + > > EXTRA_DIST = gpio-tools-test gpio-tools-test.bats > > > > if WITH_TESTS > > diff --git a/tools/gpiowatch.c b/tools/gpiowatch.c > > new file mode 100644 > > index 0000000..e6bfeb6 > > --- /dev/null > > +++ b/tools/gpiowatch.c > > @@ -0,0 +1,214 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +// SPDX-FileCopyrightText: 2017-2022 Bartosz Golaszewski <bartekgola@gmail.com> > > + > > +#include <getopt.h> > > +#include <gpiod.h> > > +#include <inttypes.h> > > +#include <poll.h> > > +#include <stdio.h> > > +#include <stdlib.h> > > +#include <string.h> > > +#include <time.h> > > + > > +#include "tools-common.h" > > + > > +static int by_name; > > +static int event_clock_mode; > > +static int banner; > > + > > +static const struct option longopts[] = { > > + { "banner", no_argument, &banner, 1 }, > > + { "by-name", no_argument, &by_name, 1 }, > > + { "chip", required_argument, NULL, 'c' }, > > + { "help", no_argument, NULL, 'h' }, > > + { "localtime", no_argument, &event_clock_mode, 2 }, > > + { "strict", no_argument, NULL, 's' }, > > + { "utc", no_argument, &event_clock_mode, 1 }, > > + { "version", no_argument, NULL, 'v' }, > > + { GETOPT_NULL_LONGOPT }, > > +}; > > + > > +static const char *const shortopts = "+c:shv"; > > + > > +static void print_help(void) > > +{ > > + printf("Usage: %s [OPTIONS] <line> ...\n", get_progname()); > > + printf("\n"); > > + printf("Wait for changes to info on GPIO lines and print them to standard output.\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(" --banner\t\tdisplay a banner on successful startup\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(" --localtime\treport event time as a local time (default is monotonic)\n"); > > + printf(" -s, --strict\t\tabort if requested line names are not unique\n"); > > + printf(" --utc\t\treport event time as UTC (default is monotonic)\n"); > > + printf(" -v, --version\t\toutput version information and exit\n"); > > + print_chip_help(); > > +} > > + > > +struct config { > > + bool strict; > > + const char *chip_id; > > +}; > > Let's either have all options in a local config struct (preferred) or > all of them as global variables, otherwise it's confusing. If you want > to use flags in long opts you can always define that structure within > the function calling getopt(). > Agreed. I wasn't keen on moving the struct into the function, but will do. Btw I prefer the long only options for the corner case options to avoid poluting the short space. The common options get a short form as well. > > + > > +int parse_config(int argc, char **argv, struct config *cfg) > > +{ > > + int opti, optc; > > + > > + 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; > > +} > > + > > +static void print_banner(int num_lines, char **lines) > > +{ > > + int i; > > + > > + if (num_lines > 1) { > > + printf("Watching lines "); > > + for (i = 0; i < num_lines - 1; i++) > > + printf("%s, ", lines[i]); > > + printf("and %s...\n", lines[i]); > > + } else { > > + printf("Watching line %s ...\n", lines[0]); > > + } > > +} > > + > > +static void event_print(struct gpiod_info_event *event, const char *chip_id) > > +{ > > + struct gpiod_line_info *info; > > + uint64_t evtime, before, after, mono; > > + char *evname; > > + int evtype; > > + struct timespec ts; > > + > > + info = gpiod_info_event_get_line_info(event); > > + evtime = gpiod_info_event_get_timestamp_ns(event); > > + evtype = gpiod_info_event_get_event_type(event); > > + > > + switch (evtype) { > > + case GPIOD_INFO_EVENT_LINE_REQUESTED: > > + evname = "REQUESTED"; > > + break; > > + case GPIOD_INFO_EVENT_LINE_RELEASED: > > + evname = "RELEASED "; > > + break; > > + case GPIOD_INFO_EVENT_LINE_CONFIG_CHANGED: > > + evname = "RECONFIG "; > > + break; > > + default: > > + evname = "UNKNOWN "; > > + } > > Newline for readability, please. > > > + if (event_clock_mode) { > > C-style comments only except for SPDX headers please. > Yeah, sorry - lazy habit. I don't even notice I'm doing it. > > + // map clock monotonic to realtime, as uAPI only supports CLOCK_MONOTONIC > > + clock_gettime(CLOCK_REALTIME, &ts); > > + before = ts.tv_nsec + ts.tv_sec * 1000000000; > > + clock_gettime(CLOCK_MONOTONIC, &ts); > > + mono = ts.tv_nsec + ts.tv_sec * 1000000000; > > + clock_gettime(CLOCK_REALTIME, &ts); > > + after = ts.tv_nsec + ts.tv_sec * 1000000000; > > + evtime += (after/2 - mono + before/2); > > + } > > Moar newlines, I really like between blocks of code, it really helps me, thanks. > Oh, ok, I see this as being one block. Where would you like the splits? > > + print_event_time(evtime, event_clock_mode); > > + printf(" %s", evname); > > + if (chip_id) > > + printf(" %s %d", chip_id, gpiod_line_info_get_offset(info)); > > + print_line_info(info); > > + printf("\n"); > > +} > > + > > +int main(int argc, char **argv) > > +{ > > + int i, j; > > + struct gpiod_chip **chips; > > + struct pollfd *pollfds; > > + struct gpiod_chip *chip; > > + struct line_resolver *resolver; > > + struct gpiod_info_event *event; > > + struct config cfg; > > + > > + i = parse_config(argc, argv, &cfg); > > + argc -= optind; > > + argv += optind; > > + > > + if (argc < 1) > > + die("at least one GPIO line must be specified"); > > + > > + if (argc > 64) > > + die("too many lines given"); > > + > > + resolver = resolve_lines(argc, argv, cfg.chip_id, cfg.strict, by_name); > > + chips = calloc(resolver->num_chips, sizeof(*chips)); > > + pollfds = calloc(resolver->num_chips, sizeof(*pollfds)); > > + if (!pollfds) > > + die("out of memory"); > > + for (i = 0; i < resolver->num_chips; i++) { > > + chip = gpiod_chip_open(resolver->chip_paths[i]); > > + if (!chip) > > + die_perror("unable to open chip %s", resolver->chip_paths[i]); > > + > > Don't other tools do the same thing basically (resolving and opening > chips)? Can't we fold it into tools-common.c so that we get a list of > open chips? > Yes and no. I wasn't keen on opening all the chips at once in the common code as in general the chips are immediately closed once the lines are requested. And the request is tool specific. Watch is a bit of an aberation in that regard - it holds the chips open indefinitely. > > + for (j = 0; j < resolver->num_lines; j++) > > + if (resolver->lines[j].chip_path == resolver->chip_paths[i]) > > + if (!gpiod_chip_watch_line_info(chip, resolver->lines[j].offset)) > > + die_perror("unable to watch line on chip %s", > > + resolver->chip_paths[i]); > > + > > + chips[i] = chip; > > + pollfds[i].fd = gpiod_chip_get_fd(chip); > > + pollfds[i].events = POLLIN; > > + } > > + > > + if (banner) > > + print_banner(argc, argv); > > + > > + for (;;) { > > + if (poll(pollfds, resolver->num_chips, -1) < 0) > > + die_perror("error polling for events"); > > + > > + for (i = 0; i < resolver->num_chips; i++) { > > + if (pollfds[i].revents == 0) > > + continue; > > + > > + event = gpiod_chip_read_info_event(chips[i]); > > + event_print(event, cfg.chip_id); > > + } > > + } > > + for (i = 0; i < resolver->num_chips; i++) > > + gpiod_chip_close(chips[i]); > > + free(chips); > > + free_line_resolver(resolver); > > + > > + return EXIT_SUCCESS; > > +} > > -- > > 2.36.1 > > > > Looks good to me, I would have probably added a machine-readable > output formatting like gpiomon but we can always extend it later. > You mean the format option? Cheers, Kent.
On Thu, Jul 7, 2022 at 4:27 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Jul 06, 2022 at 10:46:28PM +0200, Bartosz Golaszewski wrote: > > On Mon, Jun 27, 2022 at 3:46 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > Add a gpiowatch tool, based on gpiomon, but reporting line info change > > > events similar to the gpio-watch tool in the linux kernel. > > > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > > --- > > > man/Makefile.am | 2 +- > > > tools/.gitignore | 1 + > > > tools/Makefile.am | 4 +- > > > tools/gpiowatch.c | 214 ++++++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 219 insertions(+), 2 deletions(-) > > > create mode 100644 tools/gpiowatch.c > > > > > > diff --git a/man/Makefile.am b/man/Makefile.am > > > index 4d2c29b..3badd3b 100644 > > > --- a/man/Makefile.am > > > +++ b/man/Makefile.am > > > @@ -3,7 +3,7 @@ > > > > > > if WITH_MANPAGES > > > > > > -dist_man1_MANS = gpiodetect.man gpioinfo.man gpioget.man gpioset.man gpiofind.man gpiomon.man > > > +dist_man1_MANS = gpiodetect.man gpioinfo.man gpioget.man gpioset.man gpiofind.man gpiomon.man gpiowatch.man > > > > > > %.man: $(top_builddir)/tools/$(*F) > > > help2man $(top_builddir)/tools/$(*F) --include=$(srcdir)/template --output=$(builddir)/$@ --no-info > > > diff --git a/tools/.gitignore b/tools/.gitignore > > > index 0d53de9..6175e26 100644 > > > --- a/tools/.gitignore > > > +++ b/tools/.gitignore > > > @@ -7,3 +7,4 @@ gpioget > > > gpioset > > > gpiomon > > > gpiofind > > > +gpiowatch > > > diff --git a/tools/Makefile.am b/tools/Makefile.am > > > index 4a13266..8bb2cac 100644 > > > --- a/tools/Makefile.am > > > +++ b/tools/Makefile.am > > > @@ -9,7 +9,7 @@ libtools_common_la_SOURCES = tools-common.c tools-common.h > > > > > > LDADD = libtools-common.la $(top_builddir)/lib/libgpiod.la > > > > > > -bin_PROGRAMS = gpiodetect gpioinfo gpioget gpioset gpiomon gpiofind > > > +bin_PROGRAMS = gpiodetect gpioinfo gpioget gpioset gpiomon gpiofind gpiowatch > > > > > > gpiodetect_SOURCES = gpiodetect.c > > > > > > @@ -23,6 +23,8 @@ gpiomon_SOURCES = gpiomon.c > > > > > > gpiofind_SOURCES = gpiofind.c > > > > > > +gpiowatch_SOURCES = gpiowatch.c > > > + > > > EXTRA_DIST = gpio-tools-test gpio-tools-test.bats > > > > > > if WITH_TESTS > > > diff --git a/tools/gpiowatch.c b/tools/gpiowatch.c > > > new file mode 100644 > > > index 0000000..e6bfeb6 > > > --- /dev/null > > > +++ b/tools/gpiowatch.c > > > @@ -0,0 +1,214 @@ > > > +// SPDX-License-Identifier: GPL-2.0-or-later > > > +// SPDX-FileCopyrightText: 2017-2022 Bartosz Golaszewski <bartekgola@gmail.com> > > > + > > > +#include <getopt.h> > > > +#include <gpiod.h> > > > +#include <inttypes.h> > > > +#include <poll.h> > > > +#include <stdio.h> > > > +#include <stdlib.h> > > > +#include <string.h> > > > +#include <time.h> > > > + > > > +#include "tools-common.h" > > > + > > > +static int by_name; > > > +static int event_clock_mode; > > > +static int banner; > > > + > > > +static const struct option longopts[] = { > > > + { "banner", no_argument, &banner, 1 }, > > > + { "by-name", no_argument, &by_name, 1 }, > > > + { "chip", required_argument, NULL, 'c' }, > > > + { "help", no_argument, NULL, 'h' }, > > > + { "localtime", no_argument, &event_clock_mode, 2 }, > > > + { "strict", no_argument, NULL, 's' }, > > > + { "utc", no_argument, &event_clock_mode, 1 }, > > > + { "version", no_argument, NULL, 'v' }, > > > + { GETOPT_NULL_LONGOPT }, > > > +}; > > > + > > > +static const char *const shortopts = "+c:shv"; > > > + > > > +static void print_help(void) > > > +{ > > > + printf("Usage: %s [OPTIONS] <line> ...\n", get_progname()); > > > + printf("\n"); > > > + printf("Wait for changes to info on GPIO lines and print them to standard output.\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(" --banner\t\tdisplay a banner on successful startup\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(" --localtime\treport event time as a local time (default is monotonic)\n"); > > > + printf(" -s, --strict\t\tabort if requested line names are not unique\n"); > > > + printf(" --utc\t\treport event time as UTC (default is monotonic)\n"); > > > + printf(" -v, --version\t\toutput version information and exit\n"); > > > + print_chip_help(); > > > +} > > > + > > > +struct config { > > > + bool strict; > > > + const char *chip_id; > > > +}; > > > > Let's either have all options in a local config struct (preferred) or > > all of them as global variables, otherwise it's confusing. If you want > > to use flags in long opts you can always define that structure within > > the function calling getopt(). > > > > Agreed. I wasn't keen on moving the struct into the function, but will > do. > > Btw I prefer the long only options for the corner case options to avoid > poluting the short space. The common options get a short form as well. > > > > + > > > +int parse_config(int argc, char **argv, struct config *cfg) > > > +{ > > > + int opti, optc; > > > + > > > + 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; > > > +} > > > + > > > +static void print_banner(int num_lines, char **lines) > > > +{ > > > + int i; > > > + > > > + if (num_lines > 1) { > > > + printf("Watching lines "); > > > + for (i = 0; i < num_lines - 1; i++) > > > + printf("%s, ", lines[i]); > > > + printf("and %s...\n", lines[i]); > > > + } else { > > > + printf("Watching line %s ...\n", lines[0]); > > > + } > > > +} > > > + > > > +static void event_print(struct gpiod_info_event *event, const char *chip_id) > > > +{ > > > + struct gpiod_line_info *info; > > > + uint64_t evtime, before, after, mono; > > > + char *evname; > > > + int evtype; > > > + struct timespec ts; > > > + > > > + info = gpiod_info_event_get_line_info(event); > > > + evtime = gpiod_info_event_get_timestamp_ns(event); > > > + evtype = gpiod_info_event_get_event_type(event); > > > + > > > + switch (evtype) { > > > + case GPIOD_INFO_EVENT_LINE_REQUESTED: > > > + evname = "REQUESTED"; > > > + break; > > > + case GPIOD_INFO_EVENT_LINE_RELEASED: > > > + evname = "RELEASED "; > > > + break; > > > + case GPIOD_INFO_EVENT_LINE_CONFIG_CHANGED: > > > + evname = "RECONFIG "; > > > + break; > > > + default: > > > + evname = "UNKNOWN "; > > > + } > > > > Newline for readability, please. > > > > > + if (event_clock_mode) { > > > > C-style comments only except for SPDX headers please. > > > > Yeah, sorry - lazy habit. I don't even notice I'm doing it. > > > > + // map clock monotonic to realtime, as uAPI only supports CLOCK_MONOTONIC > > > + clock_gettime(CLOCK_REALTIME, &ts); > > > + before = ts.tv_nsec + ts.tv_sec * 1000000000; > > > + clock_gettime(CLOCK_MONOTONIC, &ts); > > > + mono = ts.tv_nsec + ts.tv_sec * 1000000000; > > > + clock_gettime(CLOCK_REALTIME, &ts); > > > + after = ts.tv_nsec + ts.tv_sec * 1000000000; > > > + evtime += (after/2 - mono + before/2); > > > + } > > > > Moar newlines, I really like between blocks of code, it really helps me, thanks. > > > > Oh, ok, I see this as being one block. Where would you like the splits? > I will add them myself later, don't worry about it. > > > + print_event_time(evtime, event_clock_mode); > > > + printf(" %s", evname); > > > + if (chip_id) > > > + printf(" %s %d", chip_id, gpiod_line_info_get_offset(info)); > > > + print_line_info(info); > > > + printf("\n"); > > > +} > > > + > > > +int main(int argc, char **argv) > > > +{ > > > + int i, j; > > > + struct gpiod_chip **chips; > > > + struct pollfd *pollfds; > > > + struct gpiod_chip *chip; > > > + struct line_resolver *resolver; > > > + struct gpiod_info_event *event; > > > + struct config cfg; > > > + > > > + i = parse_config(argc, argv, &cfg); > > > + argc -= optind; > > > + argv += optind; > > > + > > > + if (argc < 1) > > > + die("at least one GPIO line must be specified"); > > > + > > > + if (argc > 64) > > > + die("too many lines given"); > > > + > > > + resolver = resolve_lines(argc, argv, cfg.chip_id, cfg.strict, by_name); > > > + chips = calloc(resolver->num_chips, sizeof(*chips)); > > > + pollfds = calloc(resolver->num_chips, sizeof(*pollfds)); > > > + if (!pollfds) > > > + die("out of memory"); > > > + for (i = 0; i < resolver->num_chips; i++) { > > > + chip = gpiod_chip_open(resolver->chip_paths[i]); > > > + if (!chip) > > > + die_perror("unable to open chip %s", resolver->chip_paths[i]); > > > + > > > > Don't other tools do the same thing basically (resolving and opening > > chips)? Can't we fold it into tools-common.c so that we get a list of > > open chips? > > > > Yes and no. I wasn't keen on opening all the chips at once in the > common code as in general the chips are immediately closed once the lines > are requested. And the request is tool specific. > Watch is a bit of an aberation in that regard - it holds the chips open > indefinitely. > Right, makes sense. > > > + for (j = 0; j < resolver->num_lines; j++) > > > + if (resolver->lines[j].chip_path == resolver->chip_paths[i]) > > > + if (!gpiod_chip_watch_line_info(chip, resolver->lines[j].offset)) > > > + die_perror("unable to watch line on chip %s", > > > + resolver->chip_paths[i]); > > > + > > > + chips[i] = chip; > > > + pollfds[i].fd = gpiod_chip_get_fd(chip); > > > + pollfds[i].events = POLLIN; > > > + } > > > + > > > + if (banner) > > > + print_banner(argc, argv); > > > + > > > + for (;;) { > > > + if (poll(pollfds, resolver->num_chips, -1) < 0) > > > + die_perror("error polling for events"); > > > + > > > + for (i = 0; i < resolver->num_chips; i++) { > > > + if (pollfds[i].revents == 0) > > > + continue; > > > + > > > + event = gpiod_chip_read_info_event(chips[i]); > > > + event_print(event, cfg.chip_id); > > > + } > > > + } > > > + for (i = 0; i < resolver->num_chips; i++) > > > + gpiod_chip_close(chips[i]); > > > + free(chips); > > > + free_line_resolver(resolver); > > > + > > > + return EXIT_SUCCESS; > > > +} > > > -- > > > 2.36.1 > > > > > > > Looks good to me, I would have probably added a machine-readable > > output formatting like gpiomon but we can always extend it later. > > > > You mean the format option? > Yes. But that can be easily added later. Bart > Cheers, > Kent.
On Thu, Jul 07, 2022 at 10:41:21AM +0200, Bartosz Golaszewski wrote: > On Thu, Jul 7, 2022 at 4:27 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Wed, Jul 06, 2022 at 10:46:28PM +0200, Bartosz Golaszewski wrote: > > > On Mon, Jun 27, 2022 at 3:46 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > Add a gpiowatch tool, based on gpiomon, but reporting line info change > > > > events similar to the gpio-watch tool in the linux kernel. > > > > > > > > + } ... > > > > > > Moar newlines, I really like between blocks of code, it really helps me, thanks. > > > > > > > Oh, ok, I see this as being one block. Where would you like the splits? > > > > I will add them myself later, don't worry about it. > I think I know what you mean. Anyway, I'll add a tonne of whitespace... > > > > + print_event_time(evtime, event_clock_mode); > > > > + printf(" %s", evname); Like here > > > > + if (chip_id) > > > > + printf(" %s %d", chip_id, gpiod_line_info_get_offset(info)); and here. > > > > + print_line_info(info); > > > > + printf("\n"); > > > > +} > > > > + ... > > > > > > > > > > Looks good to me, I would have probably added a machine-readable > > > output formatting like gpiomon but we can always extend it later. > > > > > > > You mean the format option? > > > > Yes. But that can be easily added later. > I was thinking the Python bindings would be the way to go if you want to extract specific bits. Turns out when I created a new libgpiod workspace in my editor it defaulted tabs to 4 spaces, which might've been the cause of some unfortunate formatting. Sorted for the next version. Cheers, Kent.
diff --git a/man/Makefile.am b/man/Makefile.am index 4d2c29b..3badd3b 100644 --- a/man/Makefile.am +++ b/man/Makefile.am @@ -3,7 +3,7 @@ if WITH_MANPAGES -dist_man1_MANS = gpiodetect.man gpioinfo.man gpioget.man gpioset.man gpiofind.man gpiomon.man +dist_man1_MANS = gpiodetect.man gpioinfo.man gpioget.man gpioset.man gpiofind.man gpiomon.man gpiowatch.man %.man: $(top_builddir)/tools/$(*F) help2man $(top_builddir)/tools/$(*F) --include=$(srcdir)/template --output=$(builddir)/$@ --no-info diff --git a/tools/.gitignore b/tools/.gitignore index 0d53de9..6175e26 100644 --- a/tools/.gitignore +++ b/tools/.gitignore @@ -7,3 +7,4 @@ gpioget gpioset gpiomon gpiofind +gpiowatch diff --git a/tools/Makefile.am b/tools/Makefile.am index 4a13266..8bb2cac 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -9,7 +9,7 @@ libtools_common_la_SOURCES = tools-common.c tools-common.h LDADD = libtools-common.la $(top_builddir)/lib/libgpiod.la -bin_PROGRAMS = gpiodetect gpioinfo gpioget gpioset gpiomon gpiofind +bin_PROGRAMS = gpiodetect gpioinfo gpioget gpioset gpiomon gpiofind gpiowatch gpiodetect_SOURCES = gpiodetect.c @@ -23,6 +23,8 @@ gpiomon_SOURCES = gpiomon.c gpiofind_SOURCES = gpiofind.c +gpiowatch_SOURCES = gpiowatch.c + EXTRA_DIST = gpio-tools-test gpio-tools-test.bats if WITH_TESTS diff --git a/tools/gpiowatch.c b/tools/gpiowatch.c new file mode 100644 index 0000000..e6bfeb6 --- /dev/null +++ b/tools/gpiowatch.c @@ -0,0 +1,214 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// SPDX-FileCopyrightText: 2017-2022 Bartosz Golaszewski <bartekgola@gmail.com> + +#include <getopt.h> +#include <gpiod.h> +#include <inttypes.h> +#include <poll.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <time.h> + +#include "tools-common.h" + +static int by_name; +static int event_clock_mode; +static int banner; + +static const struct option longopts[] = { + { "banner", no_argument, &banner, 1 }, + { "by-name", no_argument, &by_name, 1 }, + { "chip", required_argument, NULL, 'c' }, + { "help", no_argument, NULL, 'h' }, + { "localtime", no_argument, &event_clock_mode, 2 }, + { "strict", no_argument, NULL, 's' }, + { "utc", no_argument, &event_clock_mode, 1 }, + { "version", no_argument, NULL, 'v' }, + { GETOPT_NULL_LONGOPT }, +}; + +static const char *const shortopts = "+c:shv"; + +static void print_help(void) +{ + printf("Usage: %s [OPTIONS] <line> ...\n", get_progname()); + printf("\n"); + printf("Wait for changes to info on GPIO lines and print them to standard output.\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(" --banner\t\tdisplay a banner on successful startup\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(" --localtime\treport event time as a local time (default is monotonic)\n"); + printf(" -s, --strict\t\tabort if requested line names are not unique\n"); + printf(" --utc\t\treport event time as UTC (default is monotonic)\n"); + printf(" -v, --version\t\toutput version information and exit\n"); + print_chip_help(); +} + +struct config { + bool strict; + const char *chip_id; +}; + +int parse_config(int argc, char **argv, struct config *cfg) +{ + int opti, optc; + + 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; +} + +static void print_banner(int num_lines, char **lines) +{ + int i; + + if (num_lines > 1) { + printf("Watching lines "); + for (i = 0; i < num_lines - 1; i++) + printf("%s, ", lines[i]); + printf("and %s...\n", lines[i]); + } else { + printf("Watching line %s ...\n", lines[0]); + } +} + +static void event_print(struct gpiod_info_event *event, const char *chip_id) +{ + struct gpiod_line_info *info; + uint64_t evtime, before, after, mono; + char *evname; + int evtype; + struct timespec ts; + + info = gpiod_info_event_get_line_info(event); + evtime = gpiod_info_event_get_timestamp_ns(event); + evtype = gpiod_info_event_get_event_type(event); + + switch (evtype) { + case GPIOD_INFO_EVENT_LINE_REQUESTED: + evname = "REQUESTED"; + break; + case GPIOD_INFO_EVENT_LINE_RELEASED: + evname = "RELEASED "; + break; + case GPIOD_INFO_EVENT_LINE_CONFIG_CHANGED: + evname = "RECONFIG "; + break; + default: + evname = "UNKNOWN "; + } + if (event_clock_mode) { + // map clock monotonic to realtime, as uAPI only supports CLOCK_MONOTONIC + clock_gettime(CLOCK_REALTIME, &ts); + before = ts.tv_nsec + ts.tv_sec * 1000000000; + clock_gettime(CLOCK_MONOTONIC, &ts); + mono = ts.tv_nsec + ts.tv_sec * 1000000000; + clock_gettime(CLOCK_REALTIME, &ts); + after = ts.tv_nsec + ts.tv_sec * 1000000000; + evtime += (after/2 - mono + before/2); + } + print_event_time(evtime, event_clock_mode); + printf(" %s", evname); + if (chip_id) + printf(" %s %d", chip_id, gpiod_line_info_get_offset(info)); + print_line_info(info); + printf("\n"); +} + +int main(int argc, char **argv) +{ + int i, j; + struct gpiod_chip **chips; + struct pollfd *pollfds; + struct gpiod_chip *chip; + struct line_resolver *resolver; + struct gpiod_info_event *event; + struct config cfg; + + i = parse_config(argc, argv, &cfg); + argc -= optind; + argv += optind; + + if (argc < 1) + die("at least one GPIO line must be specified"); + + if (argc > 64) + die("too many lines given"); + + resolver = resolve_lines(argc, argv, cfg.chip_id, cfg.strict, by_name); + chips = calloc(resolver->num_chips, sizeof(*chips)); + pollfds = calloc(resolver->num_chips, sizeof(*pollfds)); + if (!pollfds) + die("out of memory"); + for (i = 0; i < resolver->num_chips; i++) { + chip = gpiod_chip_open(resolver->chip_paths[i]); + if (!chip) + die_perror("unable to open chip %s", resolver->chip_paths[i]); + + for (j = 0; j < resolver->num_lines; j++) + if (resolver->lines[j].chip_path == resolver->chip_paths[i]) + if (!gpiod_chip_watch_line_info(chip, resolver->lines[j].offset)) + die_perror("unable to watch line on chip %s", + resolver->chip_paths[i]); + + chips[i] = chip; + pollfds[i].fd = gpiod_chip_get_fd(chip); + pollfds[i].events = POLLIN; + } + + if (banner) + print_banner(argc, argv); + + for (;;) { + if (poll(pollfds, resolver->num_chips, -1) < 0) + die_perror("error polling for events"); + + for (i = 0; i < resolver->num_chips; i++) { + if (pollfds[i].revents == 0) + continue; + + event = gpiod_chip_read_info_event(chips[i]); + event_print(event, cfg.chip_id); + } + } + for (i = 0; i < resolver->num_chips; i++) + gpiod_chip_close(chips[i]); + free(chips); + free_line_resolver(resolver); + + return EXIT_SUCCESS; +}
Add a gpiowatch tool, based on gpiomon, but reporting line info change events similar to the gpio-watch tool in the linux kernel. Signed-off-by: Kent Gibson <warthog618@gmail.com> --- man/Makefile.am | 2 +- tools/.gitignore | 1 + tools/Makefile.am | 4 +- tools/gpiowatch.c | 214 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 219 insertions(+), 2 deletions(-) create mode 100644 tools/gpiowatch.c