All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kent Gibson <warthog618@gmail.com>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	linux-gpio@vger.kernel.org
Subject: Re: [libgpiod v2][PATCH v5] bindings: cxx: implement C++ bindings for libgpiod v2.0
Date: Sun, 27 Mar 2022 20:21:53 +0800	[thread overview]
Message-ID: <20220327122153.GA24870@sol> (raw)
In-Reply-To: <20220323142236.670890-1-brgl@bgdev.pl>

On Wed, Mar 23, 2022 at 03:22:36PM +0100, Bartosz Golaszewski wrote:
> This rewrites the C++ bindings for libgpiod in order to work with v2.0
> version of the C API. The C++ standard use is C++17 which is well
> supported in GCC. The documentation covers the entire API so for details
> please refer to it, the tests and example programs.
> 

So C++17 for cxx bindings, but still C89 for core?
Maybe time to switch that to C99?

> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>

Would've been nice to split this into several patches to make it more
manageable.  As it is I'm reluctant to suggest any changes as I really
don't want to see this go to a v6 :-(.

Maybe a patch each for headers, impl, tests and examples?
If nothing else that would be a better order than what we find below.
My comments below may be misordered for the same reason.

Alternatively, I don't have any major issues with this patch, so I would
also be ok with applying it as is and refining it from there.

Btw, I no longer actively use C++, so I'm not a good judge of what is
idomatic, I'm just pointing out what looks odd to me.

Anyway, enough waffle, let's dive right in...

> ---
> v4 -> v5:
> 
> While this is technically the fifth iteration of C++ bindings I'm posting, the
> change are so many, it doesn't make sense to list them here - especially since
> the C API changed in the meantime too. This time the tests have been rewritten
> as well so the bindings can actually be tested using gpio-sim.
> 
>  Doxyfile.in                                 |   4 +-
>  bindings/cxx/Makefile.am                    |  23 +-
>  bindings/cxx/chip-info.cpp                  |  74 ++
>  bindings/cxx/chip.cpp                       | 213 +++--
>  bindings/cxx/edge-event-buffer.cpp          | 115 +++
>  bindings/cxx/edge-event.cpp                 | 135 +++
>  bindings/cxx/examples/Makefile.am           |  12 +-
>  bindings/cxx/examples/gpiodetectcxx.cpp     |  10 +-
>  bindings/cxx/examples/gpiofindcxx.cpp       |   2 +-
>  bindings/cxx/examples/gpiogetcxx.cpp        |  19 +-
>  bindings/cxx/examples/gpioinfocxx.cpp       |  64 +-
>  bindings/cxx/examples/gpiomoncxx.cpp        |  53 +-
>  bindings/cxx/examples/gpiosetcxx.cpp        |  33 +-
>  bindings/cxx/exception.cpp                  | 119 +++
>  bindings/cxx/gpiod.hpp                      | 944 +-------------------
>  bindings/cxx/gpiodcxx/Makefile.am           |  18 +
>  bindings/cxx/gpiodcxx/chip-info.hpp         | 105 +++
>  bindings/cxx/gpiodcxx/chip.hpp              | 179 ++++
>  bindings/cxx/gpiodcxx/edge-event-buffer.hpp | 129 +++
>  bindings/cxx/gpiodcxx/edge-event.hpp        | 137 +++
>  bindings/cxx/gpiodcxx/exception.hpp         | 158 ++++
>  bindings/cxx/gpiodcxx/info-event.hpp        | 123 +++
>  bindings/cxx/gpiodcxx/line-config.hpp       | 564 ++++++++++++
>  bindings/cxx/gpiodcxx/line-info.hpp         | 176 ++++
>  bindings/cxx/gpiodcxx/line-request.hpp      | 221 +++++
>  bindings/cxx/gpiodcxx/line.hpp              | 274 ++++++
>  bindings/cxx/gpiodcxx/misc.hpp              |  44 +
>  bindings/cxx/gpiodcxx/request-config.hpp    | 163 ++++
>  bindings/cxx/gpiodcxx/timestamp.hpp         | 122 +++
>  bindings/cxx/info-event.cpp                 | 102 +++
>  bindings/cxx/internal.cpp                   |  28 +
>  bindings/cxx/internal.hpp                   | 208 ++++-
>  bindings/cxx/iter.cpp                       |  60 --
>  bindings/cxx/line-config.cpp                | 685 ++++++++++++++
>  bindings/cxx/line-info.cpp                  | 189 ++++
>  bindings/cxx/line-request.cpp               | 224 +++++
>  bindings/cxx/line.cpp                       | 331 ++-----
>  bindings/cxx/line_bulk.cpp                  | 366 --------
>  bindings/cxx/misc.cpp                       |  20 +
>  bindings/cxx/request-config.cpp             | 174 ++++
>  bindings/cxx/tests/Makefile.am              |  27 +-
>  bindings/cxx/tests/check-kernel.cpp         |  48 +
>  bindings/cxx/tests/gpio-mockup.cpp          | 153 ----
>  bindings/cxx/tests/gpio-mockup.hpp          |  94 --
>  bindings/cxx/tests/gpiod-cxx-test.cpp       |  55 --
>  bindings/cxx/tests/gpiosim.cpp              | 264 ++++++
>  bindings/cxx/tests/gpiosim.hpp              |  69 ++
>  bindings/cxx/tests/helpers.cpp              |  37 +
>  bindings/cxx/tests/helpers.hpp              |  36 +
>  bindings/cxx/tests/tests-chip-info.cpp      |  91 ++
>  bindings/cxx/tests/tests-chip.cpp           | 219 +++--
>  bindings/cxx/tests/tests-edge-event.cpp     | 417 +++++++++
>  bindings/cxx/tests/tests-event.cpp          | 280 ------
>  bindings/cxx/tests/tests-info-event.cpp     | 198 ++++
>  bindings/cxx/tests/tests-iter.cpp           |  21 -
>  bindings/cxx/tests/tests-line-config.cpp    | 270 ++++++
>  bindings/cxx/tests/tests-line-info.cpp      | 140 +++
>  bindings/cxx/tests/tests-line-request.cpp   | 494 ++++++++++
>  bindings/cxx/tests/tests-line.cpp           | 467 ----------
>  bindings/cxx/tests/tests-misc.cpp           |  78 ++
>  bindings/cxx/tests/tests-request-config.cpp | 155 ++++
>  configure.ac                                |   1 +
>  62 files changed, 7270 insertions(+), 2964 deletions(-)
>  create mode 100644 bindings/cxx/chip-info.cpp
>  create mode 100644 bindings/cxx/edge-event-buffer.cpp
>  create mode 100644 bindings/cxx/edge-event.cpp
>  create mode 100644 bindings/cxx/exception.cpp
>  create mode 100644 bindings/cxx/gpiodcxx/Makefile.am
>  create mode 100644 bindings/cxx/gpiodcxx/chip-info.hpp
>  create mode 100644 bindings/cxx/gpiodcxx/chip.hpp
>  create mode 100644 bindings/cxx/gpiodcxx/edge-event-buffer.hpp
>  create mode 100644 bindings/cxx/gpiodcxx/edge-event.hpp
>  create mode 100644 bindings/cxx/gpiodcxx/exception.hpp
>  create mode 100644 bindings/cxx/gpiodcxx/info-event.hpp
>  create mode 100644 bindings/cxx/gpiodcxx/line-config.hpp
>  create mode 100644 bindings/cxx/gpiodcxx/line-info.hpp
>  create mode 100644 bindings/cxx/gpiodcxx/line-request.hpp
>  create mode 100644 bindings/cxx/gpiodcxx/line.hpp
>  create mode 100644 bindings/cxx/gpiodcxx/misc.hpp
>  create mode 100644 bindings/cxx/gpiodcxx/request-config.hpp
>  create mode 100644 bindings/cxx/gpiodcxx/timestamp.hpp
>  create mode 100644 bindings/cxx/info-event.cpp
>  create mode 100644 bindings/cxx/internal.cpp
>  delete mode 100644 bindings/cxx/iter.cpp
>  create mode 100644 bindings/cxx/line-config.cpp
>  create mode 100644 bindings/cxx/line-info.cpp
>  create mode 100644 bindings/cxx/line-request.cpp
>  delete mode 100644 bindings/cxx/line_bulk.cpp
>  create mode 100644 bindings/cxx/misc.cpp
>  create mode 100644 bindings/cxx/request-config.cpp
>  create mode 100644 bindings/cxx/tests/check-kernel.cpp
>  delete mode 100644 bindings/cxx/tests/gpio-mockup.cpp
>  delete mode 100644 bindings/cxx/tests/gpio-mockup.hpp
>  delete mode 100644 bindings/cxx/tests/gpiod-cxx-test.cpp
>  create mode 100644 bindings/cxx/tests/gpiosim.cpp
>  create mode 100644 bindings/cxx/tests/gpiosim.hpp
>  create mode 100644 bindings/cxx/tests/helpers.cpp
>  create mode 100644 bindings/cxx/tests/helpers.hpp
>  create mode 100644 bindings/cxx/tests/tests-chip-info.cpp
>  create mode 100644 bindings/cxx/tests/tests-edge-event.cpp
>  delete mode 100644 bindings/cxx/tests/tests-event.cpp
>  create mode 100644 bindings/cxx/tests/tests-info-event.cpp
>  delete mode 100644 bindings/cxx/tests/tests-iter.cpp
>  create mode 100644 bindings/cxx/tests/tests-line-config.cpp
>  create mode 100644 bindings/cxx/tests/tests-line-info.cpp
>  create mode 100644 bindings/cxx/tests/tests-line-request.cpp
>  delete mode 100644 bindings/cxx/tests/tests-line.cpp

No tests for line.cpp?
Should still be some tests for the streaming operators, even if it is
just to provide an example of what to expect.

>  create mode 100644 bindings/cxx/tests/tests-misc.cpp
>  create mode 100644 bindings/cxx/tests/tests-request-config.cpp
> 

[snip]

> diff --git a/bindings/cxx/examples/gpiomoncxx.cpp b/bindings/cxx/examples/gpiomoncxx.cpp
> index 4d6ac6e..db053dd 100644
> --- a/bindings/cxx/examples/gpiomoncxx.cpp
> +++ b/bindings/cxx/examples/gpiomoncxx.cpp
> @@ -3,29 +3,27 @@
>  
>  /* Simplified C++ reimplementation of the gpiomon tool. */
>  
> -#include <gpiod.hpp>
> -
> +#include <chrono>
>  #include <cstdlib>
> +#include <gpiod.hpp>
>  #include <iostream>
>  
>  namespace {
>  
> -void print_event(const ::gpiod::line_event& event)
> +void print_event(const ::gpiod::edge_event& event)
>  {
> -	if (event.event_type == ::gpiod::line_event::RISING_EDGE)
> +	if (event.type() == ::gpiod::edge_event::event_type::RISING_EDGE)
>  		::std::cout << " RISING EDGE";
> -	else if (event.event_type == ::gpiod::line_event::FALLING_EDGE)
> -		::std::cout << "FALLING EDGE";
>  	else
> -		throw ::std::logic_error("invalid event type");
> +		::std::cout << "FALLING EDGE";
>  
>  	::std::cout << " ";
>  
> -	::std::cout << ::std::chrono::duration_cast<::std::chrono::seconds>(event.timestamp).count();
> +	::std::cout << event.timestamp_ns() / 1000000000;
>  	::std::cout << ".";
> -	::std::cout << event.timestamp.count() % 1000000000;
> +	::std::cout << event.timestamp_ns() % 1000000000;
>  
> -	::std::cout << " line: " << event.source.offset();
> +	::std::cout << " line: " << event.line_offset();
>  
>  	::std::cout << ::std::endl;
>  }
> @@ -39,25 +37,36 @@ int main(int argc, char **argv)
>  		return EXIT_FAILURE;
>  	}
>  
> -	::std::vector<unsigned int> offsets;
> +	::gpiod::line::offsets offsets;
>  	offsets.reserve(argc);
>  	for (int i = 2; i < argc; i++)
>  		offsets.push_back(::std::stoul(argv[i]));
>  
>  	::gpiod::chip chip(argv[1]);
> -	auto lines = chip.get_lines(offsets);
> -
> -	lines.request({
> -		argv[0],
> -		::gpiod::line_request::EVENT_BOTH_EDGES,
> -		0,
> -	});
> +	auto request = chip.request_lines(
> +			::gpiod::request_config({
> +				{ ::gpiod::request_config::property::OFFSETS, offsets},
> +				{ ::gpiod::request_config::property::CONSUMER, "gpiomoncxx"},
> +			}),
> +			::gpiod::line_config({
> +				{
> +					::gpiod::line_config::property::DIRECTION,
> +					::gpiod::line::direction::INPUT
> +				},
> +				{
> +					::gpiod::line_config::property::EDGE,
> +					::gpiod::line::edge::BOTH
> +				}
> +			}));
> +

Would be good to close the chip to highlight the fact that the chip is not
required once the lines are requested.

Or use ::gpiod::request_lines(path,request_config,line_config) to
request the lines.

> +	::gpiod::edge_event_buffer buffer;
>  
>  	for (;;) {
> -		auto events = lines.event_wait(::std::chrono::seconds(1));
> -		if (events) {
> -			for (auto& it: events)
> -				print_event(it.event_read());
> +		if (request.wait_edge_event(::std::chrono::seconds(5))) {
> +			request.read_edge_event(buffer);
> +
> +			for (const auto& event: buffer)
> +				print_event(event);
>  		}
>  	}
>  

What is the purpose of the wait_edge_event() here?
Wouldn't read_edge_event() block until the next event?

This example should be minimal and demonstrate how the code should
normally be used. e.g.

        for (const auto& event: request.events_iter())
                  print_event(event);

(assuming the addition of an iterator wrapping request and event buffer)

If you want to showcase more complex examples then provide them separately.

[snip]

> diff --git a/bindings/cxx/gpiodcxx/chip-info.hpp b/bindings/cxx/gpiodcxx/chip-info.hpp
> new file mode 100644
> index 0000000..9313e88

[snip]

> +
> +/**
> + * @brief Stream insertion operator for GPIO chip objects.

chip info objects

> + * @param out Output stream to write to.
> + * @param chip GPIO chip to insert into the output stream.
> + * @return Reference to out.
> + */
> +::std::ostream& operator<<(::std::ostream& out, const chip_info& chip);
> +
> +/**
> + * @}
> + */
> +
> +} /* namespace gpiod */
> +
> +#endif /* __LIBGPIOD_CXX_CHIP_INFO_HPP__ */
> diff --git a/bindings/cxx/gpiodcxx/chip.hpp b/bindings/cxx/gpiodcxx/chip.hpp
> new file mode 100644
> index 0000000..7cc2156
> --- /dev/null
> +++ b/bindings/cxx/gpiodcxx/chip.hpp
> @@ -0,0 +1,179 @@
> +/* SPDX-License-Identifier: LGPL-3.0-or-later */
> +/* SPDX-FileCopyrightText: 2021-2022 Bartosz Golaszewski <brgl@bgdev.pl> */
> +
> +/**
> + * @file chip.hpp
> + */
> +
> +#ifndef __LIBGPIOD_CXX_CHIP_HPP__
> +#define __LIBGPIOD_CXX_CHIP_HPP__
> +
> +#if !defined(__LIBGPIOD_GPIOD_CXX_INSIDE__)
> +#error "Only gpiod.hpp can be included directly."
> +#endif
> +
> +#include <chrono>
> +#include <cstddef>
> +#include <iostream>
> +#include <filesystem>
> +#include <memory>
> +
> +#include "line.hpp"
> +
> +namespace gpiod {
> +
> +class chip_info;
> +class info_event;
> +class line_config;
> +class line_info;
> +class line_request;
> +class request_config;
> +
> +/**
> + * @ingroup gpiod_cxx
> + * @{
> + */
> +
> +/**
> + * @brief Represents a GPIO chip.
> + */
> +class chip
> +{
> +public:
> +
> +	/**
> +	 * @brief Instantiates a new chip object by opening the device file
> +	 *        indicated by the path argument.
> +	 * @param path Path to the device file to open.
> +	 */
> +	explicit chip(const ::std::filesystem::path& path);
> +
> +	chip(const chip& other) = delete;
> +
> +	/**
> +	 * @brief Move constructor.
> +	 * @param other Object to move.
> +	 */
> +	chip(chip&& other) noexcept;
> +
> +	~chip(void);
> +
> +	chip& operator=(const chip& other) = delete;
> +
> +	/**
> +	 * @brief Move assignment operator.
> +	 * @param other Object to move.
> +	 * @return Reference to self.
> +	 */
> +	chip& operator=(chip&& other) noexcept;
> +
> +	/**
> +	 * @brief Check if this object is valid.
> +	 * @return True if this object's methods can be used, false otherwise.
> +	 *         False usually means the chip was closed. If the user calls
> +	 *         any of the methods of this class on an object for which this
> +	 *         operator returned false, a logic_error will be thrown.
> +	 */
> +	explicit operator bool(void) const noexcept;
> +
> +	/**
> +	 * @brief Close the GPIO chip device file and free associated resources.
> +	 * @note The chip object can live after calling this method but any of
> +	 *       the chip's mutators will throw a logic_error exception.
> +	 */
> +	void close(void);
> +
> +	/**
> +	 * @brief Get the filesystem path that was used to open this GPIO chip.
> +	 * @return Path to the underlying character device file.
> +	 */
> +	::std::filesystem::path path(void) const;
> +
> +	/**
> +	 * @brief Get information about the chip.
> +	 * @return New chip_info object.
> +	 */
> +	chip_info get_info(void) const;
> +
> +	/**
> +	 * @brief Retrieve the current snapshot of line information for a
> +	 *        single line.
> +	 * @param offset Offset of the line to get the info for.
> +	 * @return New ::gpiod::line_info object.
> +	 */
> +	line_info get_line_info(line::offset offset) const;
> +
> +	/**
> +	 * @brief Wrapper around ::gpiod::chip::get_line_info that retrieves
> +	 *        the line info and starts watching the line for changes.
> +	 * @param offset Offset of the line to get the info for.
> +	 * @return New ::gpiod::line_info object.
> +	 */
> +	line_info watch_line_info(line::offset offset) const;
> +
> +	/**
> +	 * @brief Stop watching the line at given offset for info events.
> +	 * @param offset Offset of the line to get the info for.
> +	 */
> +	void unwatch_line_info(line::offset offset) const;
> +
> +	/**
> +	 * @brief Get the file descriptor associated with this chip.
> +	 * @return File descriptor number.
> +	 */
> +	int fd(void) const;
> +
> +	/**
> +	 * @brief Wait for line status events on any of the watched lines
> +	 *        exposed by this chip.
> +	 * @param timeout Wait time limit in nanoseconds.
> +	 * @return True if at least one event is ready to be read. False if the
> +	 *         wait timed out.
> +	 */
> +	bool wait_info_event(const ::std::chrono::nanoseconds& timeout) const;
> +
> +	/**
> +	 * @brief Read a single line status change event from this chip.
> +	 * @return New info_event object.
> +	 */
> +	info_event read_info_event(void) const;
> +
> +	/**
> +	 * @brief Map a GPIO line's name to its offset within the chip.
> +	 * @param name Name of the GPIO line to map.
> +	 * @return Offset of the line within the chip or -1 if the line with
> +	 *         given name is not exposed by this chip.
> +	 */
> +	int find_line(const ::std::string& name) const;
> +
> +	/**
> +	 * @brief Request a set of lines for exclusive usage.
> +	 * @param req_cfg Request config object.
> +	 * @param line_cfg Line config object.
> +	 * @return New line_request object.
> +	 */
> +	line_request request_lines(const request_config& req_cfg,
> +				   const line_config& line_cfg);
> +

A common use case is to just want to request a line, so a top-level
version of this that hides the chip object from the user might be nice.
i.e. 

    line_request request_lines(const ::std::filesystem::path& path,
                               const request_config& req_cfg,
                               const line_config& line_cfg);

Also applies to C API.

> +private:
> +
> +	struct impl;
> +
> +	::std::unique_ptr<impl> _m_priv;
> +};
> +
> +/**
> + * @brief Stream insertion operator for GPIO chip objects.
> + * @param out Output stream to write to.
> + * @param chip GPIO chip to insert into the output stream.
> + * @return Reference to out.
> + */
> +::std::ostream& operator<<(::std::ostream& out, const chip& chip);
> +
> +/**
> + * @}
> + */
> +
> +} /* namespace gpiod */
> +
> +#endif /* __LIBGPIOD_CXX_CHIP_HPP__ */

[snip]

> +++ b/bindings/cxx/gpiodcxx/line-request.hpp
> @@ -0,0 +1,221 @@
> +/* SPDX-License-Identifier: LGPL-3.0-or-later */
> +/* SPDX-FileCopyrightText: 2021-2022 Bartosz Golaszewski <brgl@bgdev.pl> */
> +
> +/**
> + * @file line-request.hpp
> + */
> +
> +#ifndef __LIBGPIOD_CXX_LINE_REQUEST_HPP__
> +#define __LIBGPIOD_CXX_LINE_REQUEST_HPP__
> +
> +#if !defined(__LIBGPIOD_GPIOD_CXX_INSIDE__)
> +#error "Only gpiod.hpp can be included directly."
> +#endif
> +
> +#include <chrono>
> +#include <cstddef>
> +#include <iostream>
> +#include <memory>
> +
> +#include "misc.hpp"
> +
> +namespace gpiod {
> +
> +class chip;
> +class edge_event;
> +class edge_event_buffer;
> +class line_config;
> +
> +/**
> + * @ingroup gpiod_cxx
> + * @{
> + */
> +
> +/**
> + * @brief Stores the context of a set of requested GPIO lines.
> + */
> +class line_request
> +{
> +public:
> +
> +	line_request(const line_request& other) = delete;
> +
> +	/**
> +	 * @brief Move constructor.
> +	 * @param other Object to move.
> +	 */
> +	line_request(line_request&& other) noexcept;
> +
> +	~line_request(void);
> +
> +	line_request& operator=(const line_request& other) = delete;
> +
> +	/**
> +	 * @brief Move assignment operator.
> +	 * @param other Object to move.
> +	 */
> +	line_request& operator=(line_request&& other) noexcept;
> +
> +	/**
> +	 * @brief Check if this object is valid.
> +	 * @return True if this object's methods can be used, false otherwise.
> +	 *         False usually means the request was released. If the user
> +	 *         calls any of the methods of this class on an object for
> +	 *         which this operator returned false, a logic_error will be
> +	 *         thrown.
> +	 */
> +	explicit operator bool(void) const noexcept;
> +
> +	/**
> +	 * @brief Release the GPIO chip and free all associated resources.
> +	 * @note The object can still be used after this method is called but
> +	 *       using any of the mutators will result in throwing
> +	 *       a logic_error exception.
> +	 */
> +	void release(void);
> +
> +	/**
> +	 * @brief Get the number of requested lines.
> +	 * @return Number of lines in this request.
> +	 */
> +	::std::size_t num_lines(void) const;
> +
> +	/**
> +	 * @brief Get the list of offsets of requested lines.
> +	 * @return List of hardware offsets of the lines in this request.
> +	 */
> +	line::offsets offsets(void) const;
> +
> +	/**
> +	 * @brief Get the value of a single requested line.
> +	 * @param offset Offset of the line to read within the chip.
> +	 * @return Current line value.
> +	 */
> +	line::value get_value(line::offset offset);
> +
> +	/**
> +	 * @brief Get the values of a subset of requested lines.
> +	 * @param offsets Vector of line offsets
> +	 * @return Vector of lines values with indexes of values corresponding
> +	 *         to those of the offsets.
> +	 */
> +	line::values get_values(const line::offsets& offsets);
> +
> +	/**
> +	 * @brief Get the values of all requested lines.
> +	 * @return List of read values.
> +	 */
> +	line::values get_values(void);
> +
> +	/**
> +	 * @brief Get the values of a subset of requested lines into a vector
> +	 *        supplied by the caller.
> +	 * @param offsets Vector of line offsets.
> +	 * @param values Vector for storing the values. Its size must be at
> +	 *               least that of the offsets vector. The indexes of read
> +	 *               values will correspond with those in the offsets
> +	 *               vector.
> +	 */
> +	void get_values(const line::offsets& offsets, line::values& values);
> +
> +	/**
> +	 * @brief Get the values of all requested lines.
> +	 * @param values Array in which the values will be stored. Must hold
> +	 *               at least the number of elements returned by
> +	 *               line_request::num_lines.
> +	 */
> +	void get_values(line::values& values);
> +
> +	/**
> +	 * @brief Set the value of a single requested line.
> +	 * @param offset Offset of the line to set within the chip.
> +	 * @param value New line value.
> +	 */
> +	void set_value(line::offset offset, line::value value);
> +
> +	/**
> +	 * @brief Set the values of a subset of requested lines.
> +	 * @param values Vector containing a set of offset->value mappings.
> +	 */
> +	void set_values(const line::value_mappings& values);
> +
> +	/**
> +	 * @brief Set the values of a subset of requested lines.
> +	 * @param offsets Vector containing the offsets of lines to set.
> +	 * @param values Vector containing new values with indexes
> +	 *               corresponding with those in the offsets vector.
> +	 */
> +	void set_values(const line::offsets& offsets, const line::values& values);
> +
> +	/**
> +	 * @brief Set the values of all requested lines.
> +	 * @param values Array of new line values. The size must be equal to
> +	 *               the value returned by line_request::num_lines.
> +	 */
> +	void set_values(const line::values& values);
> +
> +	/**
> +	 * @brief Apply new config options to requested lines.
> +	 * @param config New configuration.
> +	 */
> +	void reconfigure_lines(const line_config& config);
> +
> +	/**
> +	 * @brief Get the file descriptor number associated with this line
> +	 *        request.
> +	 * @return File descriptor number.
> +	 */
> +	int fd(void) const;
> +
> +	/**
> +	 * @brief Wait for edge events on any of the lines requested with edge
> +	 *        detection enabled.
> +	 * @param timeout Wait time limit in nanoseconds.
> +	 * @return True if at least one event is ready to be read. False if the
> +	 *         wait timed out.
> +	 */
> +	bool wait_edge_event(const ::std::chrono::nanoseconds& timeout) const;
> +
> +	/**
> +	 * @brief Read a number of edge events from this request up to the
> +	 *        maximum capacity of the buffer.
> +	 * @param buffer Edge event buffer to read events into.
> +	 * @return Number of events read.
> +	 */
> +	::std::size_t read_edge_event(edge_event_buffer& buffer);
> +
> +	/**
> +	 * @brief Read a number of edge events from this request.
> +	 * @param buffer Edge event buffer to read events into.
> +	 * @param max_events Maximum number of events to read. Limited by the
> +	 *                   capacity of the buffer.
> +	 * @return Number of events read.
> +	 */
> +	::std::size_t read_edge_event(edge_event_buffer& buffer, ::std::size_t max_events);
> +
> +private:
> +
> +	line_request(void);
> +
> +	struct impl;
> +
> +	::std::unique_ptr<impl> _m_priv;
> +
> +	friend chip;
> +};

Just wanting to get events from a request is a common use case, and
having to use an edge_event_buffer is tedious (refer to the gpiomoncxx
example)
How about an iterator that wraps the request and an edge_event_buffer
and provides events?
I refer to this elsewhere as request.events_iter().

> +
> +/**
> + * @brief Stream insertion operator for line requests.
> + * @param out Output stream to write to.
> + * @param request Line request object to insert into the output stream.
> + * @return Reference to out.
> + */
> +::std::ostream& operator<<(::std::ostream& out, const line_request& request);
> +
> +/**
> + * @}
> + */
> +
> +} /* namespace gpiod */
> +
> +#endif /* __LIBGPIOD_CXX_LINE_REQUEST_HPP__ */
> diff --git a/bindings/cxx/gpiodcxx/line.hpp b/bindings/cxx/gpiodcxx/line.hpp
> new file mode 100644
> index 0000000..8e8a984
> --- /dev/null
> +++ b/bindings/cxx/gpiodcxx/line.hpp
> @@ -0,0 +1,274 @@
> +/* SPDX-License-Identifier: LGPL-3.0-or-later */
> +/* SPDX-FileCopyrightText: 2021-2022 Bartosz Golaszewski <brgl@bgdev.pl> */
> +
> +/**
> + * @file line.hpp
> + */
> +
> +#ifndef __LIBGPIOD_CXX_LINE_HPP__
> +#define __LIBGPIOD_CXX_LINE_HPP__
> +
> +#if !defined(__LIBGPIOD_GPIOD_CXX_INSIDE__)
> +#error "Only gpiod.hpp can be included directly."
> +#endif
> +
> +#include <ostream>
> +#include <utility>
> +#include <vector>
> +
> +namespace gpiod {
> +
> +/**
> + * @brief Namespace containing various type definitions for GPIO lines.
> + */
> +namespace line {
> +
> +/**
> + * @ingroup gpiod_cxx
> + * @{
> + */
> +
> +/**
> + * @brief Wrapper around unsigned int for representing line offsets.
> + */
> +class offset
> +{
> +public:
> +	/**
> +	 * @brief Constructor with implicit conversion from unsigned int.
> +	 * @param off Line offset.
> +	 */
> +	offset(unsigned int off = 0) : _m_offset(off) {	}
> +
> +	/**
> +	 * @brief Copy constructor.
> +	 * @param other Object to copy.
> +	 */
> +	offset(const offset& other) = default;
> +
> +	/**
> +	 * @brief Move constructor.
> +	 * @param other Object to move.
> +	 */
> +	offset(offset&& other) = default;
> +
> +	~offset(void) = default;
> +
> +	/**
> +	 * @brief Assignment operator.
> +	 * @param other Object to copy.
> +	 * @return Reference to self.
> +	 */
> +	offset& operator=(const offset& other) = default;
> +
> +	/**
> +	 * @brief Move assignment operator.
> +	 * @param other Object to move.
> +	 * @return Reference to self.
> +	 */
> +	offset& operator=(offset&& other) noexcept = default;
> +
> +	/**
> +	 * @brief Conversion operator to `unsigned int`.
> +	 */
> +	operator unsigned int(void) const noexcept
> +	{
> +		return this->_m_offset;
> +	}
> +
> +private:
> +	unsigned int _m_offset;
> +};
> +

Wrapping unsigned int in a class seems like overkill.
Is this just to get the streaming operators for offsets to work?

[snip]

> +GPIOD_CXX_API line::offsets line_request::offsets(void) const
> +{
> +	this->_m_priv->throw_if_released();

redundant as this->num_lines() also does it.

> +
> +	::std::vector<unsigned int> buf(this->num_lines());
> +	line::offsets offsets(this->num_lines());
> +
> +	::gpiod_line_request_get_offsets(this->_m_priv->request.get(), buf.data());
> +
> +	for (unsigned int i = 0; i < this->num_lines(); i++)
> +		offsets[i] = buf[i];
> +

Cache num_lines locally rather than calling num_lines() several times.


[snip]

> +template<typename T>
> +::std::ostream& insert_vector(::std::ostream& out,
> +			      const ::std::string& name, const ::std::vector<T>& vec)
>  {
> -	this->_m_line = nullptr;
> -	this->_m_owner.reset();
> -}
> +	out << name << "([";
> +	::std::copy(vec.begin(), ::std::prev(vec.end()),
> +		    ::std::ostream_iterator<T>(out, ", "));
> +	out << vec.back();
> +	out << "])";
>  

What formatting are you after for vectors?
Is the double bracketing necessary?

[snip]

> +TEST_CASE("line_request stream insertion operator works", "[line-request]")
> +{
> +	::gpiosim::chip sim({{ simprop::NUM_LINES, 4 }});
> +	::gpiod::chip chip(sim.dev_path());
> +
> +	auto request = chip.request_lines(
> +		::gpiod::request_config({
> +			{ reqprop::OFFSETS, offsets({ 3, 1, 0, 2 }) }
> +		}),
> +		::gpiod::line_config()
> +	);
> +
> +	::std::stringstream buf, expected;
> +
> +	expected << "line_request(num_lines=4, line_offsets=[offsets([3, 1, 0, 2])], fd=" <<
> +		    request.fd() << ")";
> +

Still not sure what formatting you are going for with the vectors.
And the line_offsets=[] is not consistent with the offsets=() used in
request_config.

[snip]

> +TEST_CASE("request_config stream insertion operator works", "[request-config]")
> +{
> +	::gpiod::request_config cfg({
> +		{ property::CONSUMER, "foobar" },
> +		{ property::OFFSETS, offsets({ 0, 1, 2, 3 })},
> +		{ property::EVENT_BUFFER_SIZE, 32 }
> +	});
> +
> +	::std::stringstream buf;
> +
> +	buf << cfg;
> +
> +	::std::string expected("request_config(consumer='foobar', num_offsets=4, "
> +			       "offsets=(offsets([0, 1, 2, 3])), event_buffer_size=32)");
> +

The streaming output stutters.
"offsets=(offsets([0, 1, 2, 3]))" should be "offsets=([0, 1, 2, 3])"?
And even then one of the sets of brackets is redundant.

> +	REQUIRE_THAT(buf.str(), Catch::Equals(expected));
> +}
> +
> +} /* namespace */
> diff --git a/configure.ac b/configure.ac
> index f8d34ed..ab03673 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -239,6 +239,7 @@ AC_CONFIG_FILES([Makefile
>  		 bindings/cxx/libgpiodcxx.pc
>  		 bindings/Makefile
>  		 bindings/cxx/Makefile
> +		 bindings/cxx/gpiodcxx/Makefile
>  		 bindings/cxx/examples/Makefile
>  		 bindings/cxx/tests/Makefile
>  		 bindings/python/Makefile
> -- 
> 2.30.1
> 

Phew.  So nothing major.

Cheers,
Kent.

  reply	other threads:[~2022-03-27 12:22 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-23 14:22 [libgpiod v2][PATCH v5] bindings: cxx: implement C++ bindings for libgpiod v2.0 Bartosz Golaszewski
2022-03-27 12:21 ` Kent Gibson [this message]
2022-04-02 15:13   ` Bartosz Golaszewski
2022-04-04  9:54     ` Kent Gibson
2022-04-25 14:48   ` Bartosz Golaszewski
2022-04-26  4:24     ` Kent Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220327122153.GA24870@sol \
    --to=warthog618@gmail.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=brgl@bgdev.pl \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.