treewide: rework line configuration
Subsystems (2)
RFC PATCH v2 1/1 treewide: rework line configuration
This tries to get rid of the concept of defaults and overrides for line properties from the library (or rather hide them from the users). While this makes the C API a bit more complex, it allows for a more elegant high-level interface.
This patch is pretty big but I'll just give an overview here. I don't expect a detailed review of every line.
Low-level data structure model (as seen in the C API):
We're adding a new structure - line_settings. It's a basic data class that stores a set of line properties. The line_config object is simplified and becomes a storage for the mappings between offsets and line_settings.
We no longer require the user to store the offsets array in the request_config. The offsets to request are simply those for which the user explicitly added settings to the line_config. Subsequently, the request_config structure becomes optional for the request.
In C++ bindings this allows us to implement an elegant interface with rust-like chained mutators. To that end, we're also introducing a new intermediate class called request_builder that's returned by the chip's prepare_request() method which exposes routines for storing the line and request configs for the request we're making. For examples of usage - see C++ tests and samples.
--- bindings/cxx/Makefile.am | 2 + bindings/cxx/chip-info.cpp | 3 + bindings/cxx/chip.cpp | 54 +- bindings/cxx/edge-event-buffer.cpp | 2 + bindings/cxx/edge-event.cpp | 2 + bindings/cxx/examples/gpiogetcxx.cpp | 22 +- bindings/cxx/examples/gpiomoncxx.cpp | 35 +- bindings/cxx/examples/gpiosetcxx.cpp | 27 +- bindings/cxx/gpiod.hpp | 2 + bindings/cxx/gpiodcxx/Makefile.am | 2 + bindings/cxx/gpiodcxx/chip.hpp | 18 +- bindings/cxx/gpiodcxx/line-config.hpp | 497 +------- bindings/cxx/gpiodcxx/line-request.hpp | 18 +- bindings/cxx/gpiodcxx/line-settings.hpp | 193 +++ bindings/cxx/gpiodcxx/request-builder.hpp | 92 ++ bindings/cxx/gpiodcxx/request-config.hpp | 69 +- bindings/cxx/info-event.cpp | 1 + bindings/cxx/internal.hpp | 32 +- bindings/cxx/line-config.cpp | 661 ++-------- bindings/cxx/line-info.cpp | 3 +- bindings/cxx/line-request.cpp | 22 +- bindings/cxx/line-settings.cpp | 303 +++++ bindings/cxx/line.cpp | 3 + bindings/cxx/request-builder.cpp | 83 ++ bindings/cxx/request-config.cpp | 99 +- bindings/cxx/tests/Makefile.am | 5 +- bindings/cxx/tests/tests-edge-event.cpp | 194 +-- bindings/cxx/tests/tests-info-event.cpp | 50 +- bindings/cxx/tests/tests-line-config.cpp | 288 +---- bindings/cxx/tests/tests-line-request.cpp | 259 ++-- bindings/cxx/tests/tests-line-settings.cpp | 143 +++ bindings/cxx/tests/tests-request-config.cpp | 82 +- include/gpiod.h | 678 +++------- lib/Makefile.am | 1 + lib/chip.c | 8 +- lib/internal.h | 11 +- lib/line-config.c | 1241 ++++--------------- lib/line-request.c | 29 +- lib/line-settings.c | 237 ++++ lib/request-config.c | 47 +- tests/Makefile.am | 3 +- tests/gpiod-test-helpers.h | 24 + tests/tests-edge-event.c | 168 +-- tests/tests-info-event.c | 50 +- tests/tests-line-config.c | 547 +++----- tests/tests-line-info.c | 227 ++-- tests/tests-line-request.c | 357 +++--- tests/tests-line-settings.c | 314 +++++ tests/tests-request-config.c | 51 +- tools/gpio-tools-test.bats | 6 +- tools/gpioget.c | 27 +- tools/gpiomon.c | 27 +- tools/gpioset.c | 37 +- tools/tools-common.c | 13 + tools/tools-common.h | 1 + 55 files changed, 3103 insertions(+), 4267 deletions(-) create mode 100644 bindings/cxx/gpiodcxx/line-settings.hpp create mode 100644 bindings/cxx/gpiodcxx/request-builder.hpp create mode 100644 bindings/cxx/line-settings.cpp create mode 100644 bindings/cxx/request-builder.cpp create mode 100644 bindings/cxx/tests/tests-line-settings.cpp create mode 100644 lib/line-settings.c create mode 100644 tests/tests-line-settings.c
bindings/cxx/Makefile.am +2 -0
@@ -15,7 +15,9 @@ libgpiodcxx_la_SOURCES = \ line-config.cpp \ line-info.cpp \ line-request.cpp \ + line-settings.cpp \ misc.cpp \ + request-builder.cpp \ request-config.cpp libgpiodcxx_la_CXXFLAGS = -Wall -Wextra -g -std=gnu++17
bindings/cxx/chip-info.cpp +3 -0
@@ -1,6 +1,9 @@ // SPDX-License-Identifier: LGPL-3.0-or-later // SPDX-FileCopyrightText: 2021-2022 Bartosz Golaszewski <[email protected]> +#include <ostream> +#include <utility> + #include "internal.hpp" namespace gpiod {
bindings/cxx/chip.cpp +19 -35
88 lines not on this page · raw message
bindings/cxx/edge-event-buffer.cpp +2 -0
@@ -2,6 +2,8 @@ // SPDX-FileCopyrightText: 2021-2022 Bartosz Golaszewski <[email protected]> #include <iterator> +#include <ostream> +#include <utility> #include "internal.hpp"
bindings/cxx/edge-event.cpp +2 -0
@@ -2,6 +2,8 @@ // SPDX-FileCopyrightText: 2021-2022 Bartosz Golaszewski <[email protected]> #include <map> +#include <ostream> +#include <utility> #include "internal.hpp"
bindings/cxx/examples/gpiogetcxx.cpp +15 -7
@@ -20,13 +20,21 @@ int main(int argc, char **argv) for (int i = 2; i < argc; i++) offsets.push_back(::std::stoul(argv[i])); - ::gpiod::chip chip(argv[1]); - auto request = chip.request_lines( - ::gpiod::request_config({ - { ::gpiod::request_config::property::OFFSETS, offsets }, - { ::gpiod::request_config::property::CONSUMER, "gpiogetcxx" } - }), - ::gpiod::line_config()); + auto request = ::gpiod::chip(argv[1]) + .prepare_request() + .set_request_config( + ::gpiod::request_config() + .set_consumer("gpiogetcxx") + ) + .set_line_config( + ::gpiod::line_config() + .add_line_settings( + offsets, + ::gpiod::line_settings() + .set_direction(::gpiod::line::direction::INPUT) + ) + ) + .do_request(); auto vals = request.get_values();
bindings/cxx/examples/gpiomoncxx.cpp +15 -20
@@ -42,27 +42,22 @@ int main(int argc, char **argv) for (int i = 2; i < argc; i++) offsets.push_back(::std::stoul(argv[i])); - ::gpiod::chip chip(argv[1]); - 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_DETECTION, - ::gpiod::line::edge::BOTH - } - } + auto request = ::gpiod::chip(argv[1]) + .prepare_request() + .set_request_config( + ::gpiod::request_config() + .set_consumer("gpiomoncxx") ) - ); + .set_line_config( + ::gpiod::line_config() + .add_line_settings( + offsets, + ::gpiod::line_settings() + .set_direction(::gpiod::line::direction::INPUT) + .set_edge_detection(::gpiod::line::edge::BOTH) + ) + ) + .do_request(); ::gpiod::edge_event_buffer buffer;
bindings/cxx/examples/gpiosetcxx.cpp +15 -12
@@ -36,18 +36,21 @@ int main(int argc, char **argv) ::gpiod::line::value::INACTIVE); } - ::gpiod::chip chip(argv[1]); - auto request = chip.request_lines( - ::gpiod::request_config({ - { ::gpiod::request_config::property::OFFSETS, offsets }, - { ::gpiod::request_config::property::CONSUMER, "gpiogetcxx" } - }), - ::gpiod::line_config({ - { - ::gpiod::line_config::property::DIRECTION, - ::gpiod::line::direction::OUTPUT - } - })); + auto request = ::gpiod::chip(argv[1]) + .prepare_request() + .set_request_config( + ::gpiod::request_config() + .set_consumer("gpiosetcxx") + ) + .set_line_config( + ::gpiod::line_config() + .add_line_settings( + offsets, + ::gpiod::line_settings() + .set_direction(::gpiod::line::direction::OUTPUT) + ) + ) + .do_request(); request.set_values(values);
bindings/cxx/gpiod.hpp +2 -0
@@ -40,6 +40,8 @@ #include "gpiodcxx/line-config.hpp" #include "gpiodcxx/line-info.hpp" #include "gpiodcxx/line-request.hpp" +#include "gpiodcxx/line-settings.hpp" +#include "gpiodcxx/request-builder.hpp" #include "gpiodcxx/request-config.hpp" #undef __LIBGPIOD_GPIOD_CXX_INSIDE__
bindings/cxx/gpiodcxx/Makefile.am +2 -0
@@ -13,6 +13,8 @@ otherinclude_HEADERS = \ line-config.hpp \ line-info.hpp \ line-request.hpp \ + line-settings.hpp \ misc.hpp \ + request-builder.hpp \ request-config.hpp \ timestamp.hpp
bindings/cxx/gpiodcxx/chip.hpp +9 -9
@@ -27,6 +27,7 @@ class info_event; class line_config; class line_info; class line_request; +class request_builder; class request_config; /**
@@ -48,8 +49,6 @@ public: */ explicit chip(const ::std::filesystem::path& path); - chip(const chip& other) = delete; - /** * @brief Move constructor. * @param other Object to move.
@@ -147,19 +146,20 @@ public: int get_line_offset_from_name(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. + * @brief Create a request_builder associated with this chip. + * @return New request_builder object. */ - line_request request_lines(const request_config& req_cfg, - const line_config& line_cfg); + request_builder prepare_request(); private: struct impl; - ::std::unique_ptr<impl> _m_priv; + ::std::shared_ptr<impl> _m_priv; + + chip(const chip& other); + + friend request_builder; }; /**
bindings/cxx/gpiodcxx/line-config.hpp +23 -474
546 lines not on this page · raw message
bindings/cxx/gpiodcxx/line-request.hpp +12 -6
@@ -53,6 +53,7 @@ public: /** * @brief Move assignment operator. * @param other Object to move. + * @return Reference to self. */ line_request& operator=(line_request&& other) noexcept;
@@ -130,35 +131,40 @@ public: * @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. + * @return Reference to self. */ - void set_value(line::offset offset, line::value value); + line_request& 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. + * @return Reference to self. */ - void set_values(const line::value_mappings& values); + line_request& 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. + * @return Reference to self. */ - void set_values(const line::offsets& offsets, const line::values& values); + line_request& 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. + * @return Reference to self. */ - void set_values(const line::values& values); + line_request& set_values(const line::values& values); /** * @brief Apply new config options to requested lines. * @param config New configuration. + * @return Reference to self. */ - void reconfigure_lines(const line_config& config); + line_request& reconfigure_lines(const line_config& config); /** * @brief Get the file descriptor number associated with this line
@@ -201,7 +207,7 @@ private: ::std::unique_ptr<impl> _m_priv; - friend chip; + friend request_builder; }; /**
bindings/cxx/gpiodcxx/line-settings.hpp new file +193 -0
193 lines not on this page · raw message
bindings/cxx/gpiodcxx/request-builder.hpp new file +92 -0
92 lines not on this page · raw message
bindings/cxx/gpiodcxx/request-config.hpp +10 -59
125 lines not on this page · raw message
bindings/cxx/info-event.cpp +1 -0
@@ -2,6 +2,7 @@ // SPDX-FileCopyrightText: 2021-2022 Bartosz Golaszewski <[email protected]> #include <map> +#include <ostream> #include "internal.hpp"
bindings/cxx/internal.hpp +30 -2
@@ -5,8 +5,7 @@ #define __LIBGPIOD_CXX_INTERNAL_HPP__ #include <gpiod.h> -#include <iostream> -#include <iterator> +#include <map> #include <memory> #include <string> #include <utility>
@@ -25,6 +24,7 @@ map_int_to_enum(int value, const ::std::map<int, enum_type>& mapping) try { return mapping.at(value); } catch (const ::std::out_of_range& err) { + /* FIXME Demangle the name. */ throw bad_mapping(::std::string("invalid value for ") + typeid(enum_type).name()); }
@@ -40,9 +40,11 @@ template<class T, void F(T*)> struct deleter } }; +using chip_deleter = deleter<::gpiod_chip, ::gpiod_chip_close>; using chip_info_deleter = deleter<::gpiod_chip_info, ::gpiod_chip_info_free>; using line_info_deleter = deleter<::gpiod_line_info, ::gpiod_line_info_free>; using info_event_deleter = deleter<::gpiod_info_event, ::gpiod_info_event_free>; +using line_settings_deleter = deleter<::gpiod_line_settings, ::gpiod_line_settings_free>; using line_config_deleter = deleter<::gpiod_line_config, ::gpiod_line_config_free>; using request_config_deleter = deleter<::gpiod_request_config, ::gpiod_request_config_free>; using line_request_deleter = deleter<::gpiod_line_request, ::gpiod_line_request_release>;
@@ -50,9 +52,11 @@ using edge_event_deleter = deleter<::gpiod_edge_event, ::gpiod_edge_event_free>; using edge_event_buffer_deleter = deleter<::gpiod_edge_event_buffer, ::gpiod_edge_event_buffer_free>; +using chip_ptr = ::std::unique_ptr<::gpiod_chip, chip_deleter>; using chip_info_ptr = ::std::unique_ptr<::gpiod_chip_info, chip_info_deleter>; using line_info_ptr = ::std::unique_ptr<::gpiod_line_info, line_info_deleter>; using info_event_ptr = ::std::unique_ptr<::gpiod_info_event, info_event_deleter>; +using line_settings_ptr = ::std::unique_ptr<::gpiod_line_settings, line_settings_deleter>; using line_config_ptr = ::std::unique_ptr<::gpiod_line_config, line_config_deleter>; using request_config_ptr = ::std::unique_ptr<::gpiod_request_config, request_config_deleter>; using line_request_ptr = ::std::unique_ptr<::gpiod_line_request, line_request_deleter>;
@@ -60,6 +64,19 @@ using edge_event_ptr = ::std::unique_ptr<::gpiod_edge_event, edge_event_deleter> using edge_event_buffer_ptr = ::std::unique_ptr<::gpiod_edge_event_buffer, edge_event_buffer_deleter>; +struct chip::impl +{ + impl(const ::std::filesystem::path& path); + impl(const impl& other) = delete; + impl(impl&& other) = delete; + impl& operator=(const impl& other) = delete; + impl& operator=(impl&& other) = delete; + + void throw_if_closed() const; + + chip_ptr chip; +}; + struct chip_info::impl { impl() = default;
@@ -100,6 +117,17 @@ struct info_event::impl line_info info; }; +struct line_settings::impl +{ + impl(); + impl(const impl& other) = delete; + impl(impl&& other) = delete; + impl& operator=(const impl& other) = delete; + impl& operator=(impl&& other) = delete; + + line_settings_ptr settings; +}; + struct line_config::impl { impl();
bindings/cxx/line-config.cpp +70 -591
735 lines not on this page · raw message
bindings/cxx/line-info.cpp +2 -1
@@ -2,7 +2,8 @@ // SPDX-FileCopyrightText: 2021-2022 Bartosz Golaszewski <[email protected]> #include <map> -#include <iostream> +#include <ostream> +#include <utility> #include "internal.hpp"
bindings/cxx/line-request.cpp +14 -8
@@ -2,6 +2,7 @@ // SPDX-FileCopyrightText: 2021-2022 Bartosz Golaszewski <[email protected]> #include <iterator> +#include <ostream> #include <utility> #include "internal.hpp"
@@ -126,12 +127,13 @@ GPIOD_CXX_API void line_request::get_values(line::values& values) this->get_values(this->offsets(), values); } -GPIOD_CXX_API void line_request::line_request::set_value(line::offset offset, line::value value) +GPIOD_CXX_API line_request& +line_request::line_request::set_value(line::offset offset, line::value value) { - this->set_values({ offset }, { value }); + return this->set_values({ offset }, { value }); } -GPIOD_CXX_API void +GPIOD_CXX_API line_request& line_request::set_values(const line::value_mappings& values) { line::offsets offsets(values.size());
@@ -142,10 +144,10 @@ line_request::set_values(const line::value_mappings& values) vals[i] = values[i].second; } - this->set_values(offsets, vals); + return this->set_values(offsets, vals); } -GPIOD_CXX_API void line_request::set_values(const line::offsets& offsets, +GPIOD_CXX_API line_request& line_request::set_values(const line::offsets& offsets, const line::values& values) { this->_m_priv->throw_if_released();
@@ -160,14 +162,16 @@ GPIOD_CXX_API void line_request::set_values(const line::offsets& offsets, reinterpret_cast<const int*>(values.data())); if (ret) throw_from_errno("unable to set line values"); + + return *this; } -GPIOD_CXX_API void line_request::set_values(const line::values& values) +GPIOD_CXX_API line_request& line_request::set_values(const line::values& values) { - this->set_values(this->offsets(), values); + return this->set_values(this->offsets(), values); } -GPIOD_CXX_API void line_request::reconfigure_lines(const line_config& config) +GPIOD_CXX_API line_request& line_request::reconfigure_lines(const line_config& config) { this->_m_priv->throw_if_released();
@@ -175,6 +179,8 @@ GPIOD_CXX_API void line_request::reconfigure_lines(const line_config& config) config._m_priv->config.get()); if (ret) throw_from_errno("unable to reconfigure GPIO lines"); + + return *this; } GPIOD_CXX_API int line_request::fd() const
bindings/cxx/line-settings.cpp new file +303 -0
303 lines not on this page · raw message
bindings/cxx/line.cpp +3 -0
@@ -1,6 +1,9 @@ // SPDX-License-Identifier: LGPL-3.0-or-later // SPDX-FileCopyrightText: 2021-2022 Bartosz Golaszewski <[email protected]> +#include <iterator> +#include <ostream> + #include "internal.hpp" namespace gpiod {
bindings/cxx/request-builder.cpp new file +83 -0
83 lines not on this page · raw message
bindings/cxx/request-config.cpp +14 -85
159 lines not on this page · raw message
bindings/cxx/tests/Makefile.am +3 -2
@@ -22,10 +22,11 @@ gpiod_cxx_test_SOURCES = \ tests-chip.cpp \ tests-chip-info.cpp \ tests-edge-event.cpp \ + tests-info-event.cpp \ tests-line.cpp \ tests-line-config.cpp \ tests-line-info.cpp \ tests-line-request.cpp \ - tests-info-event.cpp \ + tests-line-settings.cpp \ tests-misc.cpp \ - tests-request-config.cpp + tests-request-config.cpp
bindings/cxx/tests/tests-edge-event.cpp +110 -84
246 lines not on this page · raw message
bindings/cxx/tests/tests-info-event.cpp +27 -23
50 lines not on this page · raw message
bindings/cxx/tests/tests-line-config.cpp +61 -227
288 lines not on this page · raw message
bindings/cxx/tests/tests-line-request.cpp +148 -111
259 lines not on this page · raw message
bindings/cxx/tests/tests-line-settings.cpp new file +143 -0
143 lines not on this page · raw message
bindings/cxx/tests/tests-request-config.cpp +5 -77
82 lines not on this page · raw message
include/gpiod.h +161 -517
678 lines not on this page · raw message
lib/line-config.c +240 -1001
1,241 lines not on this page · raw message
lib/line-settings.c new file +237 -0
237 lines not on this page · raw message
lib/request-config.c +2 -45
47 lines not on this page · raw message
tests/tests-edge-event.c +90 -78
168 lines not on this page · raw message
tests/tests-info-event.c +26 -24
50 lines not on this page · raw message
tests/tests-line-config.c +153 -394
547 lines not on this page · raw message
tests/tests-line-info.c +149 -78
227 lines not on this page · raw message
tests/tests-line-request.c +155 -202
357 lines not on this page · raw message
tests/tests-line-settings.c new file +314 -0
314 lines not on this page · raw message
tests/tests-request-config.c +2 -49
51 lines not on this page · raw message
The rest of this diff is not shown. View the raw message.
Signature · 1 line
2.34.1
Part of the diff is not shown here. View the raw message.