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

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

[Bartosz Golaszewski]

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

Kent,

This is an alternative approach to the problem you raised with your gpioset
patches. It adds a new helper method that allows to set all output values at
request-time using the line_config's set_output_values() method.

Please take a look and let me know what you think.

Bart

Bartosz Golaszewski (2):
  bindings: cxx: provide request_builder::set_output_values()
  bindings: cxx: examples: fix potential glitch in gpiosetcxx

 bindings/cxx/examples/gpiosetcxx.cpp      | 3 +--
 bindings/cxx/gpiodcxx/request-builder.hpp | 8 ++++++++
 bindings/cxx/request-builder.cpp          | 8 ++++++++
 3 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.39.2

[libgpiod][PATCH 1/2] bindings: cxx: provide request_builder::set_output_values()

[Bartosz Golaszewski]

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

Implement a new method in the request_builder class that allows to set
output values at request-time by calling the set_output_values() method
of the internal line_config object.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 bindings/cxx/gpiodcxx/request-builder.hpp | 8 ++++++++
 bindings/cxx/request-builder.cpp          | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/bindings/cxx/gpiodcxx/request-builder.hpp b/bindings/cxx/gpiodcxx/request-builder.hpp
index 3216216..90cc8f0 100644
--- a/bindings/cxx/gpiodcxx/request-builder.hpp
+++ b/bindings/cxx/gpiodcxx/request-builder.hpp
@@ -114,6 +114,14 @@ public:
 	 */
 	request_builder& add_line_settings(const line::offsets& offsets, const line_settings& settings);
 
+	/**
+	 * @brief Set output values for a number of lines in the line config
+	 *        stored by this object.
+	 * @param values Buffer containing the output values.
+	 * @return Reference to self.
+	 */
+	request_builder& set_output_values(const line::values& values);
+
 	/**
 	 * @brief Make the line request.
 	 * @return New line_request object.
diff --git a/bindings/cxx/request-builder.cpp b/bindings/cxx/request-builder.cpp
index 6a1a487..d11e47b 100644
--- a/bindings/cxx/request-builder.cpp
+++ b/bindings/cxx/request-builder.cpp
@@ -106,6 +106,14 @@ request_builder::add_line_settings(const line::offsets& offsets, const line_sett
 	return *this;
 }
 
+GPIOD_CXX_API request_builder&
+request_builder::set_output_values(const line::values& values)
+{
+	this->_m_priv->line_cfg.set_output_values(values);
+
+	return *this;
+}
+
 GPIOD_CXX_API line_request request_builder::do_request()
 {
 	line_request_ptr request(::gpiod_chip_request_lines(
-- 
2.39.2

[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 0/2] bindings: cxx: fix potential glitch in gpiosetcxx

[Kent Gibson]

On Mon, Jun 12, 2023 at 04:56:51PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Kent,
> 
> This is an alternative approach to the problem you raised with your gpioset
> patches. It adds a new helper method that allows to set all output values at
> request-time using the line_config's set_output_values() method.
> 
> Please take a look and let me know what you think.
> 

That works for me - it is waaaay less painful than the per-line approach.

Reviewed-by: Kent Gibson <warthog618@gmail.com>

for the series.

Though come to think of it, it could use tests as well.
But you can always add some subsequently.

Btw, this is also an example of the outcome I expected to see from
expanding the examples - identify common pain points and extend the API
to address them.

Cheers,
Kent.

[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 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 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