[libgpiod][PATCH 0/2] fix potential glitches in bindings example gpiosets

[libgpiod][PATCH 0/2] fix potential glitches in bindings example gpiosets

[Kent Gibson]

A couple of minor fixes to the bindings examples.
Noticed the python one in passing, and fixed that, then checked the
other bindings to see if they had the same problem.

The rust example is ok, but the cxx example has the same issue, so
fixed that as well.

The checkin comments are virtually identical, as they fix the same thing
for each of the bindings, but the alternative of combining them in one
patch seemed weird.

Cheers,
Kent.

Kent Gibson (2):
  bindings: python: examples: fix potential glitch in gpioset.py
  bindings: cxx: examples: fix potential glitch in gpiosetcxx

 bindings/cxx/examples/gpiosetcxx.cpp | 25 ++++++++++---------------
 bindings/python/examples/gpioset.py  | 10 +++++-----
 2 files changed, 15 insertions(+), 20 deletions(-)

-- 
2.40.1

[libgpiod][PATCH 1/2] bindings: python: examples: fix potential glitch in gpioset.py

[Kent Gibson]

gpioset.py requests lines without setting their output value, and so
sets them all inactive, and subsequently sets them to their requested
value.  This can result in glitches on lines which were active and
are set active.

As this is example code, it is also important to demonstrate that the
output value can be set by the request itself.

Request the lines with the correct output values set in the request
itself.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 bindings/python/examples/gpioset.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/bindings/python/examples/gpioset.py b/bindings/python/examples/gpioset.py
index 372a9a8..b36b376 100755
--- a/bindings/python/examples/gpioset.py
+++ b/bindings/python/examples/gpioset.py
@@ -21,16 +21,16 @@ if __name__ == "__main__":
         x, y = arg.split("=")
         return (x, Value(int(y)))
 
+    def settings(v):
+        return gpiod.LineSettings(direction=Direction.OUTPUT, output_value=v)
+
     lvs = [parse_value(arg) for arg in sys.argv[2:]]
-    lines = [x[0] for x in lvs]
-    values = dict(lvs)
+    config = dict([(l, settings(v)) for (l, v) in lvs])
 
     request = gpiod.request_lines(
         path,
         consumer="gpioset.py",
-        config={tuple(lines): gpiod.LineSettings(direction=Direction.OUTPUT)},
+        config=config,
     )
 
-    vals = request.set_values(values)
-
     input()
-- 
2.40.1

[libgpiod][PATCH 2/2] bindings: cxx: examples: fix potential glitch in gpiosetcxx

[Kent Gibson]

gpiosetcxx requests lines without setting their output value, and so
sets them all inactive, and subsequently sets them to their requested
value.  This can result in glitches on lines which were active and
are set active.

As this is example code, it is also important to demonstrate that the
output value can be set by the request itself.

Request the lines with the correct output values set in the request
itself.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 bindings/cxx/examples/gpiosetcxx.cpp | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/bindings/cxx/examples/gpiosetcxx.cpp b/bindings/cxx/examples/gpiosetcxx.cpp
index dde5379..ae35038 100644
--- a/bindings/cxx/examples/gpiosetcxx.cpp
+++ b/bindings/cxx/examples/gpiosetcxx.cpp
@@ -16,8 +16,8 @@ int main(int argc, char **argv)
 		return EXIT_FAILURE;
 	}
 
-	::gpiod::line::offsets offsets;
-	::gpiod::line::values values;
+	auto builder = ::gpiod::chip(argv[1]).prepare_request();
+	builder.set_consumer("gpiosetcxx");
 
 	for (int i = 2; i < argc; i++) {
 		::std::string arg(argv[i]);
@@ -31,22 +31,17 @@ int main(int argc, char **argv)
 			throw ::std::invalid_argument("invalid offset=value mapping: " +
 						      ::std::string(argv[i]));
 
-		offsets.push_back(::std::stoul(offset));
-		values.push_back(::std::stoul(value) ? ::gpiod::line::value::ACTIVE :
-						       ::gpiod::line::value::INACTIVE);
-	}
-
-	auto request = ::gpiod::chip(argv[1])
-		.prepare_request()
-		.set_consumer("gpiosetcxx")
-		.add_line_settings(
-			offsets,
+		::gpiod::line::value v = ::std::stoul(value) ?
+						 ::gpiod::line::value::ACTIVE :
+						 ::gpiod::line::value::INACTIVE;
+		builder.add_line_settings(
+			::std::stoul(offset),
 			::gpiod::line_settings()
 				.set_direction(::gpiod::line::direction::OUTPUT)
-		)
-		.do_request();
+				.set_output_value(v));
+	}
 
-	request.set_values(values);
+	auto request = builder.do_request();
 
 	::std::cin.get();
 
-- 
2.40.1

Re: [libgpiod][PATCH 1/2] bindings: python: examples: fix potential glitch in gpioset.py

[andy.shevchenko]

Fri, Jun 09, 2023 at 11:36:06PM +0800, Kent Gibson kirjoitti:
> gpioset.py requests lines without setting their output value, and so
> sets them all inactive, and subsequently sets them to their requested
> value.  This can result in glitches on lines which were active and
> are set active.
> 
> As this is example code, it is also important to demonstrate that the
> output value can be set by the request itself.
> 
> Request the lines with the correct output values set in the request
> itself.

Do we need a comment in the code to specify this?

...

> +    config = dict([(l, settings(v)) for (l, v) in lvs])

Aren't [] not needed?

-- 
With Best Regards,
Andy Shevchenko

Re: [libgpiod][PATCH 2/2] bindings: cxx: examples: fix potential glitch in gpiosetcxx

[andy.shevchenko]

Fri, Jun 09, 2023 at 11:36:07PM +0800, Kent Gibson kirjoitti:
> gpiosetcxx requests lines without setting their output value, and so
> sets them all inactive, and subsequently sets them to their requested
> value.  This can result in glitches on lines which were active and
> are set active.
> 
> As this is example code, it is also important to demonstrate that the
> output value can be set by the request itself.
> 
> Request the lines with the correct output values set in the request
> itself.

Same question about commenting in the code.

-- 
With Best Regards,
Andy Shevchenko

Re: [libgpiod][PATCH 1/2] bindings: python: examples: fix potential glitch in gpioset.py

[Bartosz Golaszewski]

On Fri, Jun 9, 2023 at 10:19 PM <andy.shevchenko@gmail.com> wrote:
>
> Fri, Jun 09, 2023 at 11:36:06PM +0800, Kent Gibson kirjoitti:
> > gpioset.py requests lines without setting their output value, and so
> > sets them all inactive, and subsequently sets them to their requested
> > value.  This can result in glitches on lines which were active and
> > are set active.
> >
> > As this is example code, it is also important to demonstrate that the
> > output value can be set by the request itself.
> >
> > Request the lines with the correct output values set in the request
> > itself.
>
> Do we need a comment in the code to specify this?
>
> ...
>
> > +    config = dict([(l, settings(v)) for (l, v) in lvs])
>
> Aren't [] not needed?
>

This is a list comprehension used to create the dictionary. Think:

>>> config = dict([(1, 2), (3, 4)])
>>> config
{1: 2, 3: 4}

Bart

> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [libgpiod][PATCH 1/2] bindings: python: examples: fix potential glitch in gpioset.py

[Bartosz Golaszewski]

On Fri, Jun 9, 2023 at 5:36 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> gpioset.py requests lines without setting their output value, and so
> sets them all inactive, and subsequently sets them to their requested
> value.  This can result in glitches on lines which were active and
> are set active.
>
> As this is example code, it is also important to demonstrate that the
> output value can be set by the request itself.
>
> Request the lines with the correct output values set in the request
> itself.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>  bindings/python/examples/gpioset.py | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/bindings/python/examples/gpioset.py b/bindings/python/examples/gpioset.py
> index 372a9a8..b36b376 100755
> --- a/bindings/python/examples/gpioset.py
> +++ b/bindings/python/examples/gpioset.py
> @@ -21,16 +21,16 @@ if __name__ == "__main__":
>          x, y = arg.split("=")
>          return (x, Value(int(y)))
>
> +    def settings(v):

I renamed this to make_settings() when applying. Thanks.

Bart

> +        return gpiod.LineSettings(direction=Direction.OUTPUT, output_value=v)
> +
>      lvs = [parse_value(arg) for arg in sys.argv[2:]]
> -    lines = [x[0] for x in lvs]
> -    values = dict(lvs)
> +    config = dict([(l, settings(v)) for (l, v) in lvs])
>
>      request = gpiod.request_lines(
>          path,
>          consumer="gpioset.py",
> -        config={tuple(lines): gpiod.LineSettings(direction=Direction.OUTPUT)},
> +        config=config,
>      )
>
> -    vals = request.set_values(values)
> -
>      input()
> --
> 2.40.1
>

Re: [libgpiod][PATCH 1/2] bindings: python: examples: fix potential glitch in gpioset.py

[Andy Shevchenko]

On Mon, Jun 12, 2023 at 5:26 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Fri, Jun 9, 2023 at 10:19 PM <andy.shevchenko@gmail.com> wrote:
> > Fri, Jun 09, 2023 at 11:36:06PM +0800, Kent Gibson kirjoitti:

...

> > > +    config = dict([(l, settings(v)) for (l, v) in lvs])
> >
> > Aren't [] not needed?
>
> This is a list comprehension used to create the dictionary. Think:
>
> >>> config = dict([(1, 2), (3, 4)])
> >>> config
> {1: 2, 3: 4}

Think about it in dynamic:

In [1]: x= [(1,2),(2,4)]
In [2]: dict((a,b)for a,b in x)
Out[2]: {1: 2, 2: 4}

[] are redundant, so I remembered that correctly :-)


-- 
With Best Regards,
Andy Shevchenko

Re: [libgpiod][PATCH 1/2] bindings: python: examples: fix potential glitch in gpioset.py

[Kent Gibson]

On Mon, Jun 12, 2023 at 04:26:46PM +0200, Bartosz Golaszewski wrote:
> On Fri, Jun 9, 2023 at 10:19 PM <andy.shevchenko@gmail.com> wrote:
> >
> > Fri, Jun 09, 2023 at 11:36:06PM +0800, Kent Gibson kirjoitti:
> > > gpioset.py requests lines without setting their output value, and so
> > > sets them all inactive, and subsequently sets them to their requested
> > > value.  This can result in glitches on lines which were active and
> > > are set active.
> > >
> > > As this is example code, it is also important to demonstrate that the
> > > output value can be set by the request itself.
> > >
> > > Request the lines with the correct output values set in the request
> > > itself.
> >
> > Do we need a comment in the code to specify this?
> >

Andy, I'm not ignoring you - I'm still not getting mail from you, and I
hadn't looked on the list for replies.  Weird.

In answer to your point - yes and no.  The code is not doing anything
unusual, so no.  OTOH it does serve as example code, so a bit of
commentary wouldn't hurt.

> > ...
> >
> > > +    config = dict([(l, settings(v)) for (l, v) in lvs])
> >
> > Aren't [] not needed?
> >

Ok, but now I did get this one:

> Think about it in dynamic:

> In [1]: x= [(1,2),(2,4)]
> In [2]: dict((a,b)for a,b in x)
> Out[2]: {1: 2, 2: 4}

> [] are redundant, so I remembered that correctly 😄

Terrible example - which 2 is which?
1,2,3,4 would've been better.

True - dict() accepts an iterable, so the [] are redundant in thise case.

Cheers,
Kent.

[libgpiod][PATCH 2/2] bindings: cxx: examples: fix potential glitch in gpiosetcxx

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

gpiosetcxx requests lines without setting their output value, and so
sets them all inactive, and subsequently sets them to their requested
value. This can result in glitches on lines which were active and
are set active.

As this is example code, it is also important to demonstrate that the
output value can be set by the request itself.

Request the lines with the correct output values set in the request
itself.

Suggested-by: Kent Gibson <warthog618@gmail.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 bindings/cxx/examples/gpiosetcxx.cpp | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/bindings/cxx/examples/gpiosetcxx.cpp b/bindings/cxx/examples/gpiosetcxx.cpp
index dde5379..f46cb85 100644
--- a/bindings/cxx/examples/gpiosetcxx.cpp
+++ b/bindings/cxx/examples/gpiosetcxx.cpp
@@ -44,10 +44,9 @@ int main(int argc, char **argv)
 			::gpiod::line_settings()
 				.set_direction(::gpiod::line::direction::OUTPUT)
 		)
+		.set_output_values(values)
 		.do_request();
 
-	request.set_values(values);
-
 	::std::cin.get();
 
 	return EXIT_SUCCESS;
-- 
2.39.2

Re: [libgpiod][PATCH 1/2] bindings: python: examples: fix potential glitch in gpioset.py

[Bartosz Golaszewski]

On Mon, Jun 12, 2023 at 4:54 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, Jun 12, 2023 at 04:26:46PM +0200, Bartosz Golaszewski wrote:
> > On Fri, Jun 9, 2023 at 10:19 PM <andy.shevchenko@gmail.com> wrote:
> > >
> > > Fri, Jun 09, 2023 at 11:36:06PM +0800, Kent Gibson kirjoitti:
> > > > gpioset.py requests lines without setting their output value, and so
> > > > sets them all inactive, and subsequently sets them to their requested
> > > > value.  This can result in glitches on lines which were active and
> > > > are set active.
> > > >
> > > > As this is example code, it is also important to demonstrate that the
> > > > output value can be set by the request itself.
> > > >
> > > > Request the lines with the correct output values set in the request
> > > > itself.
> > >
> > > Do we need a comment in the code to specify this?
> > >
>
> Andy, I'm not ignoring you - I'm still not getting mail from you, and I
> hadn't looked on the list for replies.  Weird.
>
> In answer to your point - yes and no.  The code is not doing anything
> unusual, so no.  OTOH it does serve as example code, so a bit of
> commentary wouldn't hurt.
>
> > > ...
> > >
> > > > +    config = dict([(l, settings(v)) for (l, v) in lvs])
> > >
> > > Aren't [] not needed?
> > >
>
> Ok, but now I did get this one:
>
> > Think about it in dynamic:
>
> > In [1]: x= [(1,2),(2,4)]
> > In [2]: dict((a,b)for a,b in x)
> > Out[2]: {1: 2, 2: 4}
>
> > [] are redundant, so I remembered that correctly 😄
>
> Terrible example - which 2 is which?
> 1,2,3,4 would've been better.
>
> True - dict() accepts an iterable, so the [] are redundant in thise case.
>
> Cheers,
> Kent.

Ok, I will add a follow-up commit.

Bart