tools: line name focussed rework
v2 cover tools: line name focussed rework
This patch series is an optimistic reimagining of the tools intended to simplify usage for well configured systems, i.e. for systems where lines can be uniquely identified by name. In such systems the chip and offset location of the line is no longer of relevance to the user, so the tools should be able to operate without mentioning them.
e.g.
gpioget GPIO17
gpioset GPIO17=active
gpiomon --localtime GPIO17 GPIO18
It is accepted that the kernel does not guarantee line name uniqueness within the system, or even within a chip, and not all systems are well configured, so the tools retain the option to identify lines by chip and offset. The hope and expectation is that over time systems will become more well configured, not less, and identification of GPIO lines by name will become the norm.
The core of the series is patch 1 which is a reworking of the tools to support identifying lines by name, and to operate across multiple GPIO chips if named lines are located on different chips.
The gpioset tool is extended to support toggling lines and interactive control of line values, so some common use cases can be trivially implemented from the command line.
e.g.
gpioset --toggle 500ms LED=on
will blink the LED line at 1Hz, indefinitely.
More complex outputs can be generated by adding more entries to the toggle sequence:
gpioset --toggle 1s,2s,1s,300ms LED=on
Even more complex outputs can be generated by driving gpioset in interactive mode from another script.
Those are the major changes. A more complete list of the changes can be found in the patch description.
Patch 2 updates and extends the tool tests to cover the reworked tools, including demonstrating gpioset being driven interactively via a script.
The final two patches add a gpiowatch tool that monitors changes to the state line information, similar to the gpio-watch tool in the kernel, and extend the test suite to cover it.
Cheers,
Kent.
Kent Gibson (4):
tools: line name focussed rework
tools: tests for line name focussed rework
tools: add gpiowatch
tools: gpiowatch tests
configure.ac | 9 +- man/Makefile.am | 2 +- tools/.gitignore | 1 + tools/Makefile.am | 4 +- tools/gpio-tools-test | 3 - tools/gpio-tools-test.bats | 2189 ++++++++++++++++++++++++++++-------- 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/gpiowatch.c | 214 ++++ tools/tools-common.c | 640 ++++++++++- tools/tools-common.h | 59 +- 15 files changed, 3927 insertions(+), 1338 deletions(-) create mode 100644 tools/gpiowatch.c
Signature · 1 line
2.36.1
PATCH v2 1/4 tools: line name focussed rework
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.
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.
--- 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(-)
configure.ac +5 -4
@@ -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])]) fi AC_ARG_ENABLE([tests],
tools/gpiodetect.c +69 -39
145 lines not on this page · raw message
tools/gpiofind.c +92 -34
179 lines not on this page · raw message
tools/gpioget.c +124 -76
272 lines not on this page · raw message
tools/gpioinfo.c +162 -194
411 lines not on this page · raw message
tools/gpiomon.c +294 -199
594 lines not on this page · raw message
tools/gpioset.c +631 -230
960 lines not on this page · raw message
tools/tools-common.c +581 -59
420 lines not on this page · raw message
tools/tools-common.h +53 -6
59 lines not on this page · raw message
The rest of this diff is not shown. View the raw message.
Signature · 1 line
2.36.1
Part of the diff is not shown here. View the raw message.
Quoted 12 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.
Quoted 8 lines
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.
Quoted 11 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.
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?
Quoted 88 lines
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 <[email protected]>--- 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(-)configure.ac +3
-0@@ -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])]) +
configure.ac AC_DEFUN([FUNC_NOT_FOUND_TOOLS], · line 116
Quoted 10 lines
configure.ac +2
-4if 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.
tools/gpiodetect.c line 2
Quoted 11 lines
configure.ac +0
-0fi AC_ARG_ENABLE([tests],tools/gpiodetect.c +1
-1@@ -1,7 +1,6 @@// SPDX-License-Identifier: GPL-2.0-or-later -// SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]> +// SPDX-FileCopyrightText: 2017-2022 Bartosz Golaszewski <[email protected]>
This should have your copyright, not mine. Same elsewhere.
[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?
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.
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.
Bart
Quoted 22 lines · Bartosz Golaszewski
Quoted 12 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.Quoted 8 lines
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 3 lines
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.
Quoted 9 lines
Quoted 9 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.
In addition the individual tools are modified as follows:
gpiodetect:
Quoted 8 lines
Quoted 2 lines
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).
Quoted 95 lines
Quoted 95 lines
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 <[email protected]>--- 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(-)configure.ac +3
-3@@ -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])])
Quoted 9 lines
configure.ac AC_DEFUN([FUNC_NOT_FOUND_TOOLS], · line 116
Quoted 3 lines
configure.ac +2
-1- 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?
Quoted 14 lines
tools/gpiodetect.c line 2
Quoted 11 lines
configure.ac +0
-0fi AC_ARG_ENABLE([tests],tools/gpiodetect.c +1
-1@@ -1,7 +1,6 @@// SPDX-License-Identifier: GPL-2.0-or-later -// SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]> +// SPDX-FileCopyrightText: 2017-2022 Bartosz Golaszewski <[email protected]> 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.
Quoted 7 lines
[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.
Quoted 6 lines
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.
Quoted 4 lines
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.
Quoted 24 lines · Kent Gibson
Quoted 22 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 7 lines
Quoted 3 lines
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.
getopt() will go to the relevant switch case everytime it encounters the flag, then you can process it and add it to the list.
Quoted 19 lines
Quoted 10 lines
Quoted 10 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.
In addition the individual tools are modified as follows:
gpiodetect:
Quoted 8 lines
Quoted 2 lines
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?
Quoted 4 lines
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).
I think so. In any case I think this would be useful.
Quoted 105 lines
Quoted 95 lines
Quoted 95 lines
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 <[email protected]>--- 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(-)configure.ac +3
-3@@ -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])])Quoted 9 lines
configure.ac AC_DEFUN([FUNC_NOT_FOUND_TOOLS], · line 116
Quoted 3 lines
configure.ac +2
-1- 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.
Quoted 3 lines
Hey, you mentioned readline before I implemented it. Though I don't recall "avoid" being mentioned :-(.
No, you're right, I mentioned it but then realized it's GPLv3.
Quoted 6 lines
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?
You'd be surprised how allergic companies are to GPLv3. Anything "tainted" with GPLv3 is often banned.
Quoted 15 lines
Quoted 14 lines
tools/gpiodetect.c line 2
Quoted 11 lines
configure.ac +0
-0fi AC_ARG_ENABLE([tests],tools/gpiodetect.c +1
-1@@ -1,7 +1,6 @@// SPDX-License-Identifier: GPL-2.0-or-later -// SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]> +// SPDX-FileCopyrightText: 2017-2022 Bartosz Golaszewski <[email protected]> This should have your copyright, not mine. Same elsewhere.
Quoted 9 lines
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.
You're introducing very significant changes to the code, I think it's only fair to include your name here together with git history. It should be:
// SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]> // SPDX-FileCopyrightText: 2022 Kent Gibson <[email protected]>
All open-source projects do it this way. And yeah - especially for gpiowatch as it's your file entirely.
Quoted 10 lines
Quoted 7 lines
[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 know, it was just late :)
Quoted 4 lines
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.
Oh, sure, we'd squash it back for the master branch, it's just for an easier review.
Quoted 14 lines
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.
Quoted 6 lines
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.
Quoted 3 lines
Keeping to 80 characters might be tough, I find 100 limiting, but I'll try. The kernel has switched to 100, right?
The limit was technically lifted but let's see what the docs say:
---
The preferred limit on the length of a single line is 80 columns.
Statements longer than 80 columns should be broken into sensible chunks, unless exceeding 80 columns significantly increases readability and does not hide information
---
So 80 columns are still preferred.
Quoted 2 lines
I guess I can break out a green screen terminal.
I feel personally attacked. :)
I personally use one big screen for work (and my laptop below) divided into multiple columns. Keeping the line length limited to 80 characters allows for more columns. :)
Bart
Quoted 9 lines
Quoted 4 lines
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.
Quoted 29 lines · Bartosz Golaszewski
Quoted 28 lines · Kent Gibson
Quoted 25 lines · Bartosz Golaszewski
Quoted 12 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.Quoted 8 lines
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.
Quoted 7 lines
Quoted 3 lines
We could but then we'd have to custom parse the command line. Or repeatedly apply
getopt()?
So yeah, keep it simple for now.
getopt()will go to the relevant switch case everytime it encounters the flag, then you can process it and add it to the list.
Yeah, but the lines are positional parameters between the --chip options.
i.e.
--chip gpiochip1 1 2 4 --chip gpiochip3 6 7 8
I thought getopt() bails when it hits positional args? Or am I missing something?
Quoted 19 lines
Quoted 19 lines
Quoted 18 lines
Quoted 6 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.
Quoted 6 lines
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?
Quoted 7 lines
Quoted 4 lines
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).
I think so. In any case I think this would be useful.
gpiodetect on a symlink will report for the chip that link points to. Similarly gpioinfo.
Isn't that sufficient?
i.e. the bare form of gpiodetect and gpioinfo report all the actual chipts, while the specific form reports whatever the provided path, resolves to, including following symlinks.
...
Quoted 15 lines
Quoted 10 lines
Quoted 6 lines
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 :-(.
No, you're right, I mentioned it but then realized it's GPLv3.
Quoted 2 lines
Ok, I'll take a look. Hopefully it is just a drop-in.
Quoted 8 lines
Quoted 4 lines
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?
You'd be surprised how allergic companies are to GPLv3. Anything "tainted" with GPLv3 is often banned.
Ok, news to me. I might quarantine them to prevent building against them, but banning them outright is a bit extreme.
OTOH I've worked at places that banned Linux (or tried to anyway).
Anyway, I'll look into the alternatives.
Cheers,
Kent.
PATCH v2 2/4 tools: tests for line name focussed rework
In reply to Kent GibsonRework the tools tests and expand to cover new functionality.
--- tools/gpio-tools-test | 3 - tools/gpio-tools-test.bats | 2052 ++++++++++++++++++++++++++---------- 2 files changed, 1504 insertions(+), 551 deletions(-)
tools/gpio-tools-test +0 -3
@@ -37,9 +37,6 @@ check_prog() { # Check all required non-coreutils tools check_prog bats check_prog modprobe -check_prog rmmod -check_prog udevadm -check_prog timeout # Check if we're running a kernel at the required version or later check_kernel $MIN_KERNEL_VERSION
tools/gpio-tools-test.bats +1504 -548
2,556 lines not on this page · raw message
Signature · 1 line
2.36.1
Re: [libgpiod v2][PATCH 2/4] tools: tests for line name focussed rework
Quoted 2 lines · Kent Gibson
Rework the tools tests and expand to cover new functionality.
Just reaslised the tests don't cover the symlink behaviour.
The expected behaviour is:
For bare gpiodetect and gpioinfo, symlinks in /dev are filtered out - only actual devices are listed.
For all commands the --chip option will accept a path which is a symlink that resolves to an actual device. The symlink itself need not be in /dev.
I'll extend the tests to cover that behaviour for the next patch version.
Cheers,
Kent.
PATCH v2 3/4 tools: add gpiowatch
In reply to Kent GibsonAdd a gpiowatch tool, based on gpiomon, but reporting line info change events similar to the gpio-watch tool in the linux kernel.
--- 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
man/Makefile.am +1 -1
@@ -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
tools/.gitignore +1 -0
@@ -7,3 +7,4 @@ gpioget gpioset gpiomon gpiofind +gpiowatch
tools/Makefile.am +3 -1
@@ -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
tools/gpiowatch.c new file +214 -0
214 lines not on this page · raw message
Signature · 1 line
2.36.1
Re: [libgpiod v2][PATCH 3/4] tools: add gpiowatch
Quoted 110 lines · Kent Gibson
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 <[email protected]>--- 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.cman/Makefile.am +1
-1@@ -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-infotools/.gitignore +1
-0@@ -7,3 +7,4 @@ gpioget gpioset gpiomon gpiofind +gpiowatchtools/Makefile.am +3
-1@@ -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_TESTStools/gpiowatch.c new file +47
-0@@ -0,0 +1,214 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// SPDX-FileCopyrightText: 2017-2022 Bartosz Golaszewski <[email protected]> + +#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" );
tools/gpiowatch.c line 57
Quoted 10 lines
tools/gpiowatch.c new file +10
-0+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().
Quoted 66 lines
tools/gpiowatch.c new file +66
-0+ +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" ;
tools/gpiowatch.c line 133
Quoted 10 lines
tools/gpiowatch.c new file +10
-0+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.
tools/gpiowatch.c line 134
Quoted 1 line
tools/gpiowatch.c new file +1
-0+if (event_clock_mode) {
C-style comments only except for SPDX headers please.
tools/gpiowatch.c line 143
Quoted 9 lines
tools/gpiowatch.c new file +9
-0+// 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.
Quoted 28 lines
tools/gpiowatch.c new file +28
-0+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" ); +
tools/gpiowatch.c line 181
Quoted 10 lines
tools/gpiowatch.c new file +10
-0+ 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?
Quoted 34 lines
tools/gpiowatch.c new file +33
-1+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 ; +} --
Quoted 2 lines
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
Re: [libgpiod v2][PATCH 3/4] tools: add gpiowatch
Quoted 118 lines · Bartosz Golaszewski
Quoted 117 lines · Kent Gibson
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 <[email protected]>--- 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.cman/Makefile.am +1
-1@@ -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-infotools/.gitignore +1
-0@@ -7,3 +7,4 @@ gpioget gpioset gpiomon gpiofind +gpiowatchtools/Makefile.am +3
-1@@ -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_TESTStools/gpiowatch.c new file +54
-0@@ -0,0 +1,214 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// SPDX-FileCopyrightText: 2017-2022 Bartosz Golaszewski <[email protected]> + +#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 {
Quoted 9 lines
tools/gpiowatch.c line 57
Quoted 3 lines
tools/gpiowatch.c new file +3
-0+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.
Quoted 79 lines
Quoted 66 lines
tools/gpiowatch.c new file +66
-0+ +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" ;tools/gpiowatch.c line 133
Quoted 10 lines
tools/gpiowatch.c new file +10
-0+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.
Quoted 4 lines
tools/gpiowatch.c line 134
Quoted 1 line
tools/gpiowatch.c new file +1
-0+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.
Quoted 10 lines
tools/gpiowatch.c line 143
Quoted 9 lines
tools/gpiowatch.c new file +9
-0+// 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 ); + }
Quoted 2 lines
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?
Quoted 34 lines
Quoted 34 lines
tools/gpiowatch.c new file +34
-0+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++) {
Quoted 9 lines
tools/gpiowatch.c line 181
Quoted 4 lines
tools/gpiowatch.c new file +4
-0+ 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.cso 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.
Quoted 31 lines
Quoted 31 lines
tools/gpiowatch.c new file +31
-0+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); +
Quoted 9 lines
Quoted 5 lines
tools/gpiowatch.c new file +2
-1+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.
Re: [libgpiod v2][PATCH 3/4] tools: add gpiowatch
Quoted 232 lines · Kent Gibson
Quoted 118 lines · Bartosz Golaszewski
Quoted 117 lines · Kent Gibson
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 <[email protected]>--- 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.cman/Makefile.am +1
-1@@ -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-infotools/.gitignore +1
-0@@ -7,3 +7,4 @@ gpioget gpioset gpiomon gpiofind +gpiowatchtools/Makefile.am +3
-1@@ -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_TESTStools/gpiowatch.c new file +54
-0@@ -0,0 +1,214 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// SPDX-FileCopyrightText: 2017-2022 Bartosz Golaszewski <[email protected]> + +#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 {Quoted 9 lines
tools/gpiowatch.c line 57
Quoted 3 lines
tools/gpiowatch.c new file +3
-0+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.
Quoted 79 lines
Quoted 66 lines
tools/gpiowatch.c new file +66
-0+ +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" ;tools/gpiowatch.c line 133
Quoted 10 lines
tools/gpiowatch.c new file +10
-0+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.
Quoted 4 lines
tools/gpiowatch.c line 134
Quoted 1 line
tools/gpiowatch.c new file +1
-0+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.
Quoted 10 lines
Quoted 9 lines
tools/gpiowatch.c new file +9
-0+// 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 ); + }
Quoted 5 lines
Quoted 2 lines
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.
Quoted 44 lines
Quoted 34 lines
Quoted 34 lines
tools/gpiowatch.c new file +34
-0+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++) {Quoted 9 lines
tools/gpiowatch.c line 181
Quoted 4 lines
tools/gpiowatch.c new file +4
-0+ 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.cso that we get a list of open chips?
Quoted 6 lines
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.
Quoted 41 lines
Quoted 31 lines
Quoted 31 lines
tools/gpiowatch.c new file +31
-0+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); +Quoted 9 lines
Quoted 5 lines
tools/gpiowatch.c new file +2
-1+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.
Quoted 2 lines
You mean the format option?
Yes. But that can be easily added later.
Bart
Quoted 2 lines
Cheers,
Kent.
Re: [libgpiod v2][PATCH 3/4] tools: add gpiowatch
Quoted 9 lines · Bartosz Golaszewski
Quoted 8 lines · Kent Gibson
Quoted 6 lines · Bartosz Golaszewski
Quoted 5 lines · Kent Gibson
Add a gpiowatch tool, based on gpiomon, but reporting line info change events similar to the gpio-watch tool in the linux kernel.
+ }
...
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
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...
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
tools/gpiowatch.c line 145
Quoted 2 lines
tools/gpiowatch.c new file +2
-0+print_event_time (evtime, event_clock_mode); +printf (" %s" , evname);
Like here
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
tools/gpiowatch.c line 147
Quoted 2 lines
tools/gpiowatch.c new file +2
-0+if (chip_id) +printf (" %s %d" , chip_id,gpiod_line_info_get_offset (info));
and here.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
tools/gpiowatch.c new file +4
-0+print_line_info (info); +printf ("\n" ); +} +
...
Quoted 11 lines
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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.
PATCH v2 4/4 tools: gpiowatch tests
In reply to Kent GibsonExtend the tool test suite to cover the gpiowatch tool.
--- tools/gpio-tools-test.bats | 249 +++++++++++++++++++++++++++++++++++++ 1 file changed, 249 insertions(+)
tools/gpio-tools-test.bats +249 -0
@@ -2069,3 +2069,252 @@ request_release_line() { dut_read output_is "%x" } + +# +# gpiowatch test cases +# + +@test "gpiowatch: by name" { + gpiosim_chip sim0 num_lines=8 line_name=4:foo + local sim0=$(gpiosim_chip_name sim0) + + dut_run gpiowatch --banner foo + dut_regex_match "Watching line .*" + + request_release_line $sim0 4 + + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+foo\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+foo\\s+.*" + # tools currently have no way to generate a RECONFIG event +} + +@test "gpiowatch: by offset" { + gpiosim_chip sim0 num_lines=8 + local sim0=$(gpiosim_chip_name sim0) + + dut_run gpiowatch --banner --chip $sim0 4 + dut_regex_match "Watching line .*" + + request_release_line $sim0 4 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+$sim0\\s+4\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+$sim0\\s+4\\s+.*" + + assert_fail dut_readable +} + +@test "gpiowatch: by chip and name" { + gpiosim_chip sim0 num_lines=8 line_name=4:foo + gpiosim_chip sim1 num_lines=8 line_name=2:foo + local sim1=$(gpiosim_chip_name sim1) + + dut_run gpiowatch --banner --chip $sim1 foo + dut_regex_match "Watching line .*" + + request_release_line $sim1 2 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+$sim1\\s+2\\s+foo\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+$sim1\\s+2\\s+foo\\s+.*" + + assert_fail dut_readable +} + +@test "gpiowatch: first matching named line" { + gpiosim_chip sim0 num_lines=4 line_name=1:foo line_name=2:bar line_name=3:foobar + gpiosim_chip sim1 num_lines=8 line_name=0:baz line_name=2:foobar \ + line_name=4:xyz line_name=7:foobar + gpiosim_chip sim2 num_lines=16 + + dut_run gpiowatch --banner foobar + dut_regex_match "Watching line .*" + + request_release_line $(gpiosim_chip_name sim0) 3 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+foobar\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+foobar\\s+.*" + + assert_fail dut_readable +} + +@test "gpiowatch: multiple lines" { + gpiosim_chip sim0 num_lines=8 + local sim0=$(gpiosim_chip_name sim0) + + dut_run gpiowatch --banner --chip $sim0 1 2 3 4 5 + dut_regex_match "Watching lines .*" + + request_release_line $sim0 2 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+$sim0\\s+2\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+$sim0\\s+2\\s+.*" + + request_release_line $sim0 3 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+$sim0\\s+3\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+$sim0\\s+3\\s+.*" + + request_release_line $sim0 4 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+$sim0\\s+4\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+$sim0\\s+4\\s+.*" + + assert_fail dut_readable +} + +@test "gpiowatch: multiple lines by name and offset" { + gpiosim_chip sim0 num_lines=4 line_name=1:foo line_name=2:bar + local sim0=$(gpiosim_chip_name sim0) + + dut_run gpiowatch --banner --chip $sim0 bar foo 3 + dut_regex_match "Watching lines .*" + + request_release_line $sim0 2 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+$sim0\\s+2\\s+bar\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+$sim0\\s+2\\s+bar\\s+.*" + + request_release_line $sim0 1 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+$sim0\\s+1\\s+foo\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+$sim0\\s+1\\s+foo\\s+.*" + + request_release_line $sim0 3 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+$sim0\\s+3\\s+unnamed\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+$sim0\\s+3\\s+unnamed\\s+.*" + + assert_fail dut_readable +} + +@test "gpiowatch: multiple lines across multiple chips" { + gpiosim_chip sim0 num_lines=4 line_name=1:foo line_name=2:bar + gpiosim_chip sim1 num_lines=8 line_name=0:baz line_name=4:xyz + local sim0=$(gpiosim_chip_name sim0) + local sim1=$(gpiosim_chip_name sim1) + + dut_run gpiowatch --banner baz bar foo xyz + dut_regex_match "Watching lines .*" + + request_release_line $sim0 2 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+bar\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+bar\\s+.*" + + request_release_line $sim0 1 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+foo\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+foo\\s+.*" + + request_release_line $sim1 4 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+xyz\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+xyz\\s+.*" + + request_release_line $sim1 0 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+baz\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+baz\\s+.*" + + assert_fail dut_readable +} + +@test "gpiowatch: exit after SIGINT" { + gpiosim_chip sim0 num_lines=8 + + dut_run gpiowatch --banner --chip $(gpiosim_chip_name sim0) 4 + dut_regex_match "Watching line .*" + + dut_kill -SIGINT + dut_wait + + status_is 130 +} + +@test "gpiowatch: exit after SIGTERM" { + gpiosim_chip sim0 num_lines=8 + + dut_run gpiowatch --banner --chip $(gpiosim_chip_name sim0) 4 + dut_regex_match "Watching line .*" + + dut_kill -SIGTERM + dut_wait + + status_is 143 +} + +@test "gpiowatch: with nonexistent line" { + run_tool gpiowatch nonexistent-line + + status_is 1 + output_regex_match ".*cannot find line nonexistent-line" +} + +@test "gpiowatch: with same line twice" { + gpiosim_chip sim0 num_lines=8 line_name=1:foo + local sim0=$(gpiosim_chip_name sim0) + + # by offset + run_tool gpiowatch --chip $sim0 0 0 + + status_is 1 + num_lines_is 1 + output_regex_match ".*lines 0 and 0 are the same" + + # by name + run_tool gpiowatch --chip $sim0 foo foo + + status_is 1 + num_lines_is 1 + output_regex_match ".*lines foo and foo are the same" + + # by name and offset + run_tool gpiowatch --chip $sim0 1 foo + + status_is 1 + num_lines_is 1 + output_regex_match ".*lines 1 and foo are the same" +} + +@test "gpiowatch: with strict named line check" { + gpiosim_chip sim0 num_lines=4 line_name=1:foo line_name=2:bar line_name=3:foobar + gpiosim_chip sim1 num_lines=8 line_name=0:baz line_name=2:foobar \ + line_name=4:xyz line_name=7:foobar + gpiosim_chip sim2 num_lines=16 + + run_tool gpiowatch --strict foobar + + status_is 1 + output_regex_match ".*line foobar is not unique" +} + +@test "gpiowatch: with lines strictly by name" { + # not suggesting this setup makes sense - just test that we can deal with it + gpiosim_chip sim0 num_lines=8 line_name=1:42 line_name=6:13 + local sim0=$(gpiosim_chip_name sim0) + + dut_run gpiowatch --banner --by-name --chip $sim0 42 13 + dut_flush + + request_release_line $sim0 1 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+$sim0\\s+1\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+$sim0\\s+1\\s+.*" + + request_release_line $sim0 6 + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+REQUESTED\\s+$sim0\\s+6\\s+.*" + dut_regex_match "\\s*[0-9]+\.[0-9]+\\s+RELEASED\\s+$sim0\\s+6\\s+.*" + + assert_fail dut_readable +} + +@test "gpiowatch: with no arguments" { + run_tool gpiowatch + + status_is 1 + output_regex_match ".*at least one GPIO line must be specified" +} + +@test "gpiowatch: with no line specified" { + gpiosim_chip sim0 num_lines=8 + + run_tool gpiowatch --chip $(gpiosim_chip_name sim0) + + status_is 1 + output_regex_match ".*at least one GPIO line must be specified" +} + +@test "gpiowatch: with offset out of range" { + gpiosim_chip sim0 num_lines=4 + local sim0=$(gpiosim_chip_name sim0) + + run_tool gpiowatch --chip $sim0 5 + + status_is 1 + output_regex_match ".*offset 5 is out of range on chip $sim0" +}
Signature · 1 line
2.36.1
Quoted 39 lines · Kent Gibson
This patch series is an optimistic reimagining of the tools intended to simplify usage for well configured systems, i.e. for systems where lines can be uniquely identified by name. In such systems the chip and offset location of the line is no longer of relevance to the user, so the tools should be able to operate without mentioning them.
e.g.
gpioget GPIO17gpioset GPIO17=active
gpiomon --localtime GPIO17 GPIO18It is accepted that the kernel does not guarantee line name uniqueness within the system, or even within a chip, and not all systems are well configured, so the tools retain the option to identify lines by chip and offset. The hope and expectation is that over time systems will become more well configured, not less, and identification of GPIO lines by name will become the norm.
The core of the series is patch 1 which is a reworking of the tools to support identifying lines by name, and to operate across multiple GPIO chips if named lines are located on different chips.
The gpioset tool is extended to support toggling lines and interactive control of line values, so some common use cases can be trivially implemented from the command line.
e.g.
gpioset --toggle 500ms LED=onwill blink the LED line at 1Hz, indefinitely.
More complex outputs can be generated by adding more entries to the toggle sequence:
gpioset --toggle 1s,2s,1s,300ms LED=onEven more complex outputs can be generated by driving gpioset in interactive mode from another script.
Those are the major changes. A more complete list of the changes can be found in the patch description.
Quoted 7 lines
Patch 2 updates and extends the tool tests to cover the reworked tools, including demonstrating gpioset being driven interactively via a script.
The final two patches add a gpiowatch tool that monitors changes to the state line information, similar to the gpio-watch tool in the kernel, and extend the test suite to cover it.
I forgot to mention that gpiofind is effectively redundant now its functionality has been absorbed into the other tools.
I kept it in the patch for completeness but would be happy to remove it.
I am also reconsidering gpioset's interactive get command. The get currently returns the requested set values - it does not query the kernel. It may be useful to be able to do both for open-drain outputs.
How about the get does a GET_VALUES, while a bare set displays the requested set values?
Or would a flag on the get be clearer?
Cheers,
Kent.