Re: [libgpiod v2][PATCH 3/5] bindings: python: add examples for v2 API

[Kent Gibson <warthog618@gmail.com>]

On Wed, May 25, 2022 at 04:07:02PM +0200, Bartosz Golaszewski wrote:
> This adds the usual set of reimplementations of gpio-tools using the new
> python module.
> 
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> ---
>  bindings/python/examples/Makefile.am   | 10 ++++++++
>  bindings/python/examples/gpiodetect.py | 17 +++++++++++++
>  bindings/python/examples/gpiofind.py   | 20 +++++++++++++++
>  bindings/python/examples/gpioget.py    | 31 +++++++++++++++++++++++
>  bindings/python/examples/gpioinfo.py   | 35 ++++++++++++++++++++++++++
>  bindings/python/examples/gpiomon.py    | 31 +++++++++++++++++++++++
>  bindings/python/examples/gpioset.py    | 35 ++++++++++++++++++++++++++
>  7 files changed, 179 insertions(+)
>  create mode 100644 bindings/python/examples/Makefile.am
>  create mode 100755 bindings/python/examples/gpiodetect.py
>  create mode 100755 bindings/python/examples/gpiofind.py
>  create mode 100755 bindings/python/examples/gpioget.py
>  create mode 100755 bindings/python/examples/gpioinfo.py
>  create mode 100755 bindings/python/examples/gpiomon.py
>  create mode 100755 bindings/python/examples/gpioset.py
> 
> diff --git a/bindings/python/examples/Makefile.am b/bindings/python/examples/Makefile.am
> new file mode 100644
> index 0000000..4169469
> --- /dev/null
> +++ b/bindings/python/examples/Makefile.am
> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@gmail.com>
> +
> +EXTRA_DIST =				\
> +		gpiodetect.py		\
> +		gpiofind.py		\
> +		gpioget.py		\
> +		gpioinfo.py		\
> +		gpiomon.py		\
> +		gpioset.py
> diff --git a/bindings/python/examples/gpiodetect.py b/bindings/python/examples/gpiodetect.py
> new file mode 100755
> index 0000000..08e586b
> --- /dev/null
> +++ b/bindings/python/examples/gpiodetect.py
> @@ -0,0 +1,17 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@gmail.com>
> +
> +"""Reimplementation of the gpiodetect tool in Python."""
> +
> +import gpiod
> +import os
> +
> +if __name__ == "__main__":
> +    for entry in os.scandir("/dev/"):
> +        if gpiod.is_gpiochip_device(entry.path):
> +            with gpiod.Chip(entry.path) as chip:
> +                info = chip.get_info()
> +                print(
> +                    "{} [{}] ({} lines)".format(info.name, info.label, info.num_lines)
> +                )
> diff --git a/bindings/python/examples/gpiofind.py b/bindings/python/examples/gpiofind.py
> new file mode 100755
> index 0000000..e488a49
> --- /dev/null
> +++ b/bindings/python/examples/gpiofind.py
> @@ -0,0 +1,20 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@gmail.com>
> +
> +"""Reimplementation of the gpiofind tool in Python."""
> +
> +import gpiod
> +import os
> +import sys
> +
> +if __name__ == "__main__":
> +    for entry in os.scandir("/dev/"):
> +        if gpiod.is_gpiochip_device(entry.path):
> +            with gpiod.Chip(entry.path) as chip:
> +                offset = chip.get_line_offset_from_name(sys.argv[1])
> +                if offset is not None:
> +                    print("{} {}".format(chip.get_info().name, offset))
> +                    sys.exit(0)
> +
> +    sys.exit(1)
> diff --git a/bindings/python/examples/gpioget.py b/bindings/python/examples/gpioget.py
> new file mode 100755
> index 0000000..c509f38
> --- /dev/null
> +++ b/bindings/python/examples/gpioget.py
> @@ -0,0 +1,31 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@gmail.com>
> +
> +"""Simplified reimplementation of the gpioget tool in Python."""
> +
> +import gpiod
> +import sys
> +
> +Direction = gpiod.Line.Direction
> +Value = gpiod.Line.Value
> +
> +if __name__ == "__main__":
> +    if len(sys.argv) < 3:
> +        raise TypeError("usage: gpioget.py <gpiochip> <offset1> <offset2> ...")
> +
> +    path = sys.argv[1]
> +    offsets = []
> +    for off in sys.argv[2:]:
> +        offsets.append(int(off))
> +
> +    with gpiod.request_lines(
> +        path,
> +        gpiod.RequestConfig(offsets=offsets, consumer="gpioget.py"),
> +        gpiod.LineConfig(direction=Direction.INPUT),
> +    ) as request:

With request_lines() being the primary entry point to the gpiod module,
consider extending it to allow the RequestConfig and LineConfig kwargs to
be passed directly to request_lines(), and for those transient objects to
be constructed within request_lines().
That way the average user does not need to concern themselves with those
objects and the code is easier to read.
i.e.
    with gpiod.request_lines(
        path,
        offsets=offsets,
        consumer="gpioget.py",
        direction=Direction.INPUT,
    ) as request:

You can still support passing in complete RequestConfig and LineConfig
as kwargs for cases where the user requires complex configuration.
i.e.
    with gpiod.request_lines(
        path,
        req_cfg=gpiod.RequestConfig(offsets=offsets, consumer="gpioget.py"),
        line_cfg=gpiod.LineConfig(direction=Direction.INPUT),
    ) as request:

Or for those use cases the user could use the Chip.request_lines() (which
wouldn't have the kwarg sugar) instead.

Would be good to have some examples with complex configuration as well,
not just the tool reimplementations.

> +        vals = request.get_values()
> +
> +        for val in vals:
> +            print("0" if val == Value.INACTIVE else "1", end=" ")
> +        print()
> diff --git a/bindings/python/examples/gpioinfo.py b/bindings/python/examples/gpioinfo.py
> new file mode 100755
> index 0000000..3097d10
> --- /dev/null
> +++ b/bindings/python/examples/gpioinfo.py
> @@ -0,0 +1,35 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@gmail.com>
> +
> +"""Simplified reimplementation of the gpioinfo tool in Python."""
> +
> +import gpiod
> +import os
> +
> +if __name__ == "__main__":
> +    for entry in os.scandir("/dev/"):
> +        if gpiod.is_gpiochip_device(entry.path):
> +            with gpiod.Chip(entry.path) as chip:
> +                cinfo = chip.get_info()
> +                print("{} - {} lines:".format(cinfo.name, cinfo.num_lines))
> +
> +                for offset in range(0, cinfo.num_lines):
> +                    linfo = chip.get_line_info(offset)
> +                    offset = linfo.offset
> +                    name = linfo.name
> +                    consumer = linfo.consumer
> +                    direction = linfo.direction
> +                    active_low = linfo.active_low
> +
> +                    print(
> +                        "\tline {:>3}: {:>18} {:>12} {:>8} {:>10}".format(
> +                            offset,
> +                            "unnamed" if name is None else name,
> +                            "unused" if consumer is None else consumer,
> +                            "input"
> +                            if direction == gpiod.Line.Direction.INPUT
> +                            else "output",
> +                            "active-low" if active_low else "active-high",
> +                        )
> +                    )
> diff --git a/bindings/python/examples/gpiomon.py b/bindings/python/examples/gpiomon.py
> new file mode 100755
> index 0000000..b0f4b88
> --- /dev/null
> +++ b/bindings/python/examples/gpiomon.py
> @@ -0,0 +1,31 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@gmail.com>
> +
> +"""Simplified reimplementation of the gpiomon tool in Python."""
> +
> +import gpiod
> +import sys
> +
> +Edge = gpiod.Line.Edge
> +
> +if __name__ == "__main__":
> +    if len(sys.argv) < 3:
> +        raise TypeError("usage: gpiomon.py <gpiochip> <offset1> <offset2> ...")
> +
> +    path = sys.argv[1]
> +    offsets = []
> +    for off in sys.argv[2:]:
> +        offsets.append(int(off))
> +
> +    buf = gpiod.EdgeEventBuffer()
> +
> +    with gpiod.request_lines(
> +        path,
> +        gpiod.RequestConfig(offsets=offsets, consumer="gpiomon.py"),
> +        gpiod.LineConfig(edge_detection=Edge.BOTH),
> +    ) as request:
> +        while True:
> +            request.read_edge_event(buf)
> +            for event in buf:
> +                print(event)

Can you hide the buffer here to simplify the common case?
How about having the request manage the buffer, and add a generator
method that returns the next event, say edge_events()?

For the uncommon case, add kwargs to allow the user to provide the buffer,
or to specify the buffer size.  If neither provided then the request
constructs a default sized buffer.

Then the code becomes:

    with gpiod.request_lines(
        path,
        offsets=offsets,
        consumer="gpiomon.py",
        edge_detection=Edge.BOTH,
    ) as request:
        for event in request.edge_events():
            print(event)

> diff --git a/bindings/python/examples/gpioset.py b/bindings/python/examples/gpioset.py
> new file mode 100755
> index 0000000..3a8f8cc
> --- /dev/null
> +++ b/bindings/python/examples/gpioset.py
> @@ -0,0 +1,35 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@gmail.com>
> +
> +"""Simplified reimplementation of the gpioset tool in Python."""
> +
> +import gpiod
> +import sys
> +
> +Value = gpiod.Line.Value
> +
> +if __name__ == "__main__":
> +    if len(sys.argv) < 3:
> +        raise TypeError("usage: gpioset.py <gpiochip> <offset1>=<value1> ...")
> +
> +    path = sys.argv[1]
> +    values = dict()
> +    for arg in sys.argv[2:]:
> +        arg = arg.split("=")
> +        key = int(arg[0])
> +        val = int(arg[1])
> +
> +        if val == 1:
> +            values[key] = Value.ACTIVE
> +        elif val == 0:
> +            values[key] = Value.INACTIVE
> +        else:
> +            raise ValueError("{} is an invalid value for GPIO lines".format(val))
> +
> +    with gpiod.request_lines(
> +        path,
> +        gpiod.RequestConfig(offsets=values.keys(), consumer="gpioset.py"),
> +        gpiod.LineConfig(direction=gpiod.Line.Direction.OUTPUT, output_values=values),
> +    ) as request:
> +        input()
> -- 
> 2.34.1
> 

The focus of my comments above is to simplify the API for the most common
case, and to make it a little more Pythonic rather than mirroring the C
API, in both cases by hiding implementation details that the casual user
doesn't need to know about.

I'm pretty sure other minor things that I'm not 100% comfortable with are
the same as with the C++ bindings, and the Python is in keeping with that,
so I wont recover the same ground.

I'm ok with the series overall.  As per my C++ comments, it would be
great to get some feedback from Python developers.

Cheers,
Kent.