Re: [libgpiod v2][PATCH 1/4] tools: line name focussed rework

[Kent Gibson <warthog618@xxxxxxxxx>]

On Wed, Jul 06, 2022 at 10:20:00PM +0200, Bartosz Golaszewski wrote:
> On Mon, Jun 27, 2022 at 3:46 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > 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.
> >
> 
> We could potentially extend it by allowing multiple --chip arguments
> for multiple chips but let's not go there unless requested.
> 

We could but then we'd have to custom parse the command line.
Or repeatedly apply getopt()?
So yeah, keep it simple for now.

> > 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.
> >
> 
> We discussed at some point that gpiodetect should also check if any of
> the files it iterates over is a symbolic link to a GPIO device and
> print some info accordingly (e.g. foobar [link to /dev/gpiochip2])-
> are you thinking about adding this too?
> 

Did we?  My bad then - I clearly forgot and instead filtered the symlinks
out so it only reports actual chips (btw the v1 tools report the symlinks
by repeating the actual, which is the worst of both worlds).

> > gpiofind:
> >
> > Add the option to display the info for found lines.
> >
> > 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 <warthog618@xxxxxxxxx>
> > ---
> >  configure.ac         |   9 +-
> >  tools/gpiodetect.c   | 108 ++++--
> >  tools/gpiofind.c     | 126 +++++--
> >  tools/gpioget.c      | 200 ++++++----
> >  tools/gpioinfo.c     | 356 ++++++++----------
> >  tools/gpiomon.c      | 493 +++++++++++++++----------
> >  tools/gpioset.c      | 861 +++++++++++++++++++++++++++++++------------
> >  tools/tools-common.c | 640 +++++++++++++++++++++++++++++---
> >  tools/tools-common.h |  59 ++-
> >  9 files changed, 2011 insertions(+), 841 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index ab03673..c7e353c 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -105,14 +105,15 @@ 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])])
> > +       AC_CHECK_HEADERS([readline/readline.h], [], [HEADER_NOT_FOUND_TOOLS([readline/readline.h])])
> > +       AC_CHECK_LIB([readline], readline, [], [LIB_NOT_FOUND_TOOLS([readline])])
> 
> readline is licensed under GPLv3 - this bleeds into gpioset and will
> make a lot of companies bounce off of it. Any chance you could use
> libedit instead? It's supposed to be a drop-in replacement for
> readline but I have never used it first hand.
> 

Hey, you mentioned readline before I implemented it.
Though I don't recall "avoid" being mentioned :-(.

Ok, I'll take a look.  Hopefully it is just a drop-in.

Out of curiosity which aspect of GPLv3 is the problem, for a tool
which is publicly available and they aren't going to modify?
Just having a GPLv3 library on their system?

> >  fi
> >
> >  AC_ARG_ENABLE([tests],
> > diff --git a/tools/gpiodetect.c b/tools/gpiodetect.c
> > index 8f6e8b3..18b6e95 100644
> > --- a/tools/gpiodetect.c
> > +++ b/tools/gpiodetect.c
> > @@ -1,7 +1,6 @@
> >  // SPDX-License-Identifier: GPL-2.0-or-later
> > -// SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@xxxxxxxxx>
> > +// SPDX-FileCopyrightText: 2017-2022 Bartosz Golaszewski <bartekgola@xxxxxxxxx>
> 
> This should have your copyright, not mine. Same elsewhere.
> 

Your argument would be stronger for gpiowatch, though even that was
based on your gpiomon.

These files are originally yours, I just modified them, so it wouldn't
be right to replace your copyright.

And as you mention below, you will be maintaining these and to keep it
simple I grant you the copyrights to the content of this series.

> [snip]
> 
> I will try to get back to this one later but it's so big I just kept
> losing focus. Do you think you could submit it the way I did for the
> bindings - remove the existing code and then apply the new
> implementations?
> 

Hey, this is nothing compared to your bindings ;-).

I'm not a huge fan of the remove and replace approach as it loses history,
and while the changes here are substantial they aren't a complete
replacement like the bindings changes were.

If the diff is too confusing I usually refer to the final code for
clarity.

I had originally split it per-tool plus common code, but that didn't
help much.  But I'll take another look at making the patches more
easily digestible.

> Just a general note on coding style: I would prefer that you use /* */
> style comments for the sake of consistency with the rest of the code
> and if you tried to keep the lines limited to 80 characters wherever
> it doesn't significantly impact readability? A sprinkle of newlines
> here and there separating logical blocks of code would be nice too.
> 

Keeping to 80 characters might be tough, I find 100 limiting, but I'll
try.  The kernel has switched to 100, right?

I guess I can break out a green screen terminal.

> We may not agree on the above but after all I will maintain that code
> and would prefer to keep it so that it stays easy for my brain to
> parse it.
> 

Indeed.

Cheers,
Kent.