Re: [libgpiod v2][PATCH v5] bindings: cxx: implement C++ bindings for libgpiod v2.0

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

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@xxxxxxxx>

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@xxxxxxxx> */
> +
> +/**
> + * @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@xxxxxxxx> */
> +
> +/**
> + * @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@xxxxxxxx> */
> +
> +/**
> + * @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.
[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux