treewide: continue beating libgpiod v2 into shape for an upcoming release
Subsystems (2)
cover treewide: continue beating libgpiod v2 into shape for an upcoming release
From: Bartosz Golaszewski <[email protected]>
This series contains an assortment of smaller and bigger changes. Some are just simple updates to docs, some fix bugs and we have two more changes to the API: unifying the get_offsets functions for line config and line request as well as providing an interface for setting multiple output values at once in line_config before requesting lines.
Rust bindings have also been updated slightly so Viresh please make sure to take a look and review.
I really hope this is the last set of bigger changes and that I'll be able to tag an RC and release v2 in the next couple of weeks.
Bartosz Golaszewski (16):
README: update for libgpiod v2
tests: avoid shadowing local variables with common names in macros
build: unify the coding style of source files lists in Makefiles
treewide: unify gpiod_line_config/request_get_offsets() functions
doc: update docs for libgpiod v2
bindings: cxx: prepend all C symbols with the scope resolution
operator
bindings: cxx: allow to copy line_settings
tests: fix the line config reset test case
tests: add a helper for reading back line settings from line config
core: provide gpiod_line_config_set_output_values()
gpioset: use gpiod_line_config_set_output_values()
bindings: cxx: add line_config.set_output_values()
bindings: python: provide line_config.set_output_values()
bindings: rust: make request_config optional in Chip.request_lines()
bindings: rust: make mutators return &mut self
bindings: rust: provide line_config.set_output_values()
README | 32 ++--- bindings/cxx/Makefile.am | 32 ++--- bindings/cxx/examples/Makefile.am | 12 +- bindings/cxx/gpiodcxx/line-config.hpp | 7 ++ bindings/cxx/gpiodcxx/line-settings.hpp | 13 +- bindings/cxx/internal.hpp | 3 +- bindings/cxx/line-config.cpp | 33 +++-- bindings/cxx/line-request.cpp | 10 +- bindings/cxx/line-settings.cpp | 85 +++++++++---- bindings/cxx/tests/Makefile.am | 36 +++--- bindings/cxx/tests/tests-line-config.cpp | 51 ++++++++ bindings/cxx/tests/tests-line-settings.cpp | 43 +++++++ bindings/python/gpiod/chip.py | 6 + bindings/python/gpiod/ext/line-config.c | 64 ++++++++++ bindings/python/gpiod/ext/request.c | 8 +- bindings/python/tests/tests_line_request.py | 14 +++ .../rust/libgpiod/examples/gpio_events.rs | 4 +- .../examples/gpio_threaded_info_events.rs | 8 +- bindings/rust/libgpiod/examples/gpioget.rs | 6 +- bindings/rust/libgpiod/examples/gpiomon.rs | 4 +- bindings/rust/libgpiod/examples/gpioset.rs | 6 +- bindings/rust/libgpiod/src/chip.rs | 10 +- bindings/rust/libgpiod/src/lib.rs | 1 + bindings/rust/libgpiod/src/line_config.rs | 83 +++++++------ bindings/rust/libgpiod/src/line_request.rs | 22 ++-- bindings/rust/libgpiod/src/request_config.rs | 8 +- bindings/rust/libgpiod/tests/common/config.rs | 10 +- bindings/rust/libgpiod/tests/info_event.rs | 8 +- bindings/rust/libgpiod/tests/line_config.rs | 76 +++++++++--- bindings/rust/libgpiod/tests/line_request.rs | 99 +++++++-------- .../rust/libgpiod/tests/request_config.rs | 2 +- configure.ac | 1 + include/gpiod.h | 104 ++++++++++++---- lib/Makefile.am | 27 ++-- lib/line-config.c | 98 +++++++++++---- lib/line-request.c | 23 ++-- man/Makefile.am | 8 +- tests/Makefile.am | 34 ++--- tests/gpiod-test-helpers.h | 36 ++++-- tests/tests-line-config.c | 116 +++++++++++++----- tests/tests-line-request.c | 10 +- tools/gpioset.c | 21 ++-- 42 files changed, 879 insertions(+), 395 deletions(-)
Signature · 1 line
2.37.2
PATCH 1/16 README: update for libgpiod v2
From: Bartosz Golaszewski <[email protected]>
Certain parts of the README file still refer to concepts removed from libgpiod v2. Update whatever needs updating.
--- README | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
README +16 -16
@@ -1,5 +1,5 @@ # SPDX-License-Identifier: CC-BY-SA-4.0 -# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]> +# SPDX-FileCopyrightText: 2017-2023 Bartosz Golaszewski <[email protected]> libgpiod ========
@@ -30,14 +30,10 @@ allow an easy conversion of user scripts to using the character device. BUILDING -------- -This is a pretty standard autotools project. It does not depend on any -libraries other than the standard C library with GNU extensions. +This is a pretty standard autotools project. The core C library does not have +any external dependencies other than the standard C library with GNU extensions. -The autoconf version needed to compile the project is 2.61. - -Recent kernel headers are also required for the GPIO user API definitions. For -the exact version of kernel headers required, please refer to the configure.ac -contents. +The command-line tools optionally depend on libedit for the interactive feature. To build the project (including command-line utilities) run:
@@ -51,6 +47,8 @@ arguments to it. If building from release tarballs, the configure script is already provided and there's no need to invoke autogen.sh. +For all configure features, see: ./configure --help. + TOOLS -----
@@ -231,10 +229,10 @@ interface. The minimum kernel version required to run the tests can be checked in the tests/gpiod-test.c source file (it's subject to change if new features are -added to the kernel). The tests work together with the gpio-mockup kernel -module which must be enabled. NOTE: the module must not be built-in. A helper -library - libgpiomockup - is included to enable straightforward interaction -with the module. +added to the kernel). The tests work together with the gpio-sim kernel which +must either be built-in or available for loading using kmod. A helper +library - libgpiosim - is included to enable straightforward interaction with +the module. To build the testing executable add the '--enable-tests' option when running the configure script. If enabled, the tests will be installed next to
@@ -251,12 +249,12 @@ The gpio-tools programs can be tested separately using the gpio-tools-test.bats script. It requires bats[1] to run and assumes that the tested executables are in the same directory as the script. -Both C++ and Python bindings also include their own test-suites. Both reuse the -libgpiomockup library to avoid code duplication when interacting with -gpio-mockup. +C++, Rust and Python bindings also include their own test-suites. Both reuse the +libgpiosim library to avoid code duplication when interacting with gpio-sim. Python test-suite uses the standard unittest package. C++ tests use an external -testing framework - Catch2 - which must be installed in the system. +testing framework - Catch2 - which must be installed in the system. Rust +bindings use the standard tests module layout and the #[test] attribute. DOCUMENTATION -------------
@@ -268,6 +266,8 @@ doxygen markup blocks. Doxygen documentation can be generated by executing Python bindings contain help strings that can be accessed with the help builtin. +Rust bindings use rustdoc. + Man pages for command-line programs are generated automatically if gpio-tools were selected and help2man is available in the system.
Signature · 1 line
2.37.2
Re: [libgpiod][PATCH 01/16] README: update for libgpiod v2
Quoted 4 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Certain parts of the README file still refer to concepts removed from libgpiod v2. Update whatever needs updating.
...
README interface. · line 232
Quoted 7 lines
The minimum kernel version required to run the tests can be checked in the
tests/gpiod-test.csource file (it's subject to change if new features areREADME +1
-4-added to the kernel). The tests work together with the gpio-mockup kernel -module which must be enabled. NOTE: the module must not be built-in. A helper -library - libgpiomockup - is included to enable straightforward interaction -with the module. +added to the kernel). The tests work together with the gpio-sim kernel which
Mistakenly removed word 'module' ?
Quoted 3 lines
README +3
-0+must either be built-in or available for loading using kmod. A helper +library - libgpiosim - is included to enable straightforward interaction with +the module.
...
README The gpio-tools programs can be tested separately using the gpio-tools-test.bats · line 252
Quoted 4 lines
README +1
-3-Both C++ and Python bindings also include their own test-suites. Both reuse the -libgpiomockup library to avoid code duplication when interacting with -gpio-mockup. +C++, Rust and Python bindings also include their own test-suites. Both reuse the
"Both" out of three?
Quoted 1 line
+libgpiosim library to avoid code duplication when interacting with gpio-sim.
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH 2/16 tests: avoid shadowing local variables with common names in macros
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
The name 'ret' if very common for local variables so change it to _ret in test helper macros to avoid potential shadowing.
--- tests/gpiod-test-helpers.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
tests/gpiod-test-helpers.h +8 -8
@@ -118,11 +118,11 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(struct_gpiod_edge_event_buffer,#define gpiod_test_line_config_add_line_settings_or_fail (_line_cfg, _offsets, \ _num_offsets, _settings) \ do { \ - gint ret = gpiod_line_config_add_line_settings(_line_cfg, \ - _offsets, \ - _num_offsets, \ - _settings); \ - g_assert_cmpint(ret, ==, 0); \ + gint _ret = gpiod_line_config_add_line_settings(_line_cfg, \ + _offsets, \ + _num_offsets, \ + _settings); \ + g_assert_cmpint(_ret, ==, 0); \ gpiod_test_return_if_failed(); \ } while (0)
@@ -147,9 +147,9 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(struct_gpiod_edge_event_buffer,#define gpiod_test_reconfigure_lines_or_fail (_request, _line_cfg) \ do { \ - gint ret = gpiod_line_request_reconfigure_lines(_request, \ - _line_cfg); \ - g_assert_cmpint(ret, ==, 0); \ + gint _ret = gpiod_line_request_reconfigure_lines(_request, \ + _line_cfg); \ + g_assert_cmpint(_ret, ==, 0); \ gpiod_test_return_if_failed(); \ } while (0)
Signature · 1 line
2.37.2
Re: [libgpiod][PATCH 02/16] tests: avoid shadowing local variables with common names in macros
Quoted 4 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
The name 'ret' if very common for local variables so change it to
_retin test helper macros to avoid potential shadowing.
Makes sense!
Quoted 42 lines
Signed-off-by Bartosz Golaszewski <[email protected]>--- tests/gpiod-test-helpers.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)tests/gpiod-test-helpers.h +8
-8@@ -118,11 +118,11 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(struct_gpiod_edge_event_buffer,#define gpiod_test_line_config_add_line_settings_or_fail (_line_cfg, _offsets, \ _num_offsets, _settings) \ do { \ - gint ret = gpiod_line_config_add_line_settings(_line_cfg, \ - _offsets, \ - _num_offsets, \ - _settings); \ - g_assert_cmpint(ret, ==, 0); \ + gint _ret = gpiod_line_config_add_line_settings(_line_cfg, \ + _offsets, \ + _num_offsets, \ + _settings); \ + g_assert_cmpint(_ret, ==, 0); \ gpiod_test_return_if_failed(); \ } while (0)@@ -147,9 +147,9 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(struct_gpiod_edge_event_buffer,#define gpiod_test_reconfigure_lines_or_fail (_request, _line_cfg) \ do { \ - gint ret = gpiod_line_request_reconfigure_lines(_request, \ - _line_cfg); \ - g_assert_cmpint(ret, ==, 0); \ + gint _ret = gpiod_line_request_reconfigure_lines(_request, \ + _line_cfg); \ + g_assert_cmpint(_ret, ==, 0); \ gpiod_test_return_if_failed(); \ } while (0)Signature · 1 line
2.37.2
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH 3/16 build: unify the coding style of source files lists in Makefiles
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Use the most common and readable convention for listing source files in Makefiles wherever it's not consistent yet.
--- bindings/cxx/Makefile.am | 32 +++++++++++++-------------- bindings/cxx/examples/Makefile.am | 12 +++++------ bindings/cxx/tests/Makefile.am | 36 +++++++++++++++---------------- lib/Makefile.am | 27 ++++++++++++----------- man/Makefile.am | 8 ++++++- tests/Makefile.am | 34 ++++++++++++++--------------- 6 files changed, 78 insertions(+), 71 deletions(-)
bindings/cxx/Makefile.am +16 -16
@@ -2,22 +2,22 @@ # SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]> lib_LTLIBRARIES = libgpiodcxx.la -libgpiodcxx_la_SOURCES = \ - chip.cpp \ - chip-info.cpp \ - edge-event-buffer.cpp \ - edge-event.cpp \ - exception.cpp \ - info-event.cpp \ - internal.cpp \ - internal.hpp \ - line.cpp \ - line-config.cpp \ - line-info.cpp \ - line-request.cpp \ - line-settings.cpp \ - misc.cpp \ - request-builder.cpp \ +libgpiodcxx_la_SOURCES = \ + chip.cpp \ + chip-info.cpp \ + edge-event-buffer.cpp \ + edge-event.cpp \ + exception.cpp \ + info-event.cpp \ + internal.cpp \ + internal.hpp \ + line.cpp \ + 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/examples/Makefile.am +6 -6
@@ -6,12 +6,12 @@ AM_CXXFLAGS += -Wall -Wextra -g -std=gnu++17 AM_LDFLAGS = -lgpiodcxx -L$(top_builddir)/bindings/cxx/ noinst_PROGRAMS = \ - gpiodetectcxx \ - gpiofindcxx \ - gpiogetcxx \ - gpioinfocxx \ - gpiomoncxx \ - gpiosetcxx + gpiodetectcxx \ + gpiofindcxx \ + gpiogetcxx \ + gpioinfocxx \ + gpiomoncxx \ + gpiosetcxx gpiodetectcxx_SOURCES = gpiodetectcxx.cpp
bindings/cxx/tests/Makefile.am +18 -18
@@ -12,21 +12,21 @@ AM_LDFLAGS += -pthread bin_PROGRAMS = gpiod-cxx-test -gpiod_cxx_test_SOURCES = \ - check-kernel.cpp \ - gpiod-cxx-test-main.cpp \ - gpiosim.cpp \ - gpiosim.hpp \ - helpers.cpp \ - helpers.hpp \ - 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-line-settings.cpp \ - tests-misc.cpp \ - tests-request-config.cpp +gpiod_cxx_test_SOURCES = \ + check-kernel.cpp \ + gpiod-cxx-test-main.cpp \ + gpiosim.cpp \ + gpiosim.hpp \ + helpers.cpp \ + helpers.hpp \ + 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-line-settings.cpp \ + tests-misc.cpp \ + tests-request-config.cpp
lib/Makefile.am +14 -13
@@ -2,19 +2,20 @@ # SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]> lib_LTLIBRARIES = libgpiod.la -libgpiod_la_SOURCES = chip.c \ - chip-info.c \ - edge-event.c \ - info-event.c \ - internal.h \ - internal.c \ - line-config.c \ - line-info.c \ - line-request.c \ - line-settings.c \ - misc.c \ - request-config.c \ - uapi/gpio.h +libgpiod_la_SOURCES = \ + chip.c \ + chip-info.c \ + edge-event.c \ + info-event.c \ + internal.h \ + internal.c \ + line-config.c \ + line-info.c \ + line-request.c \ + line-settings.c \ + misc.c \ + request-config.c \ + uapi/gpio.h libgpiod_la_CFLAGS = -Wall -Wextra -g -std=gnu89 libgpiod_la_CFLAGS += -fvisibility=hidden -I$(top_srcdir)/include/
man/Makefile.am +7 -1
@@ -3,7 +3,13 @@ if WITH_MANPAGES -dist_man1_MANS = gpiodetect.man gpioinfo.man gpioget.man gpioset.man gpiomon.man gpionotify.man +dist_man1_MANS = \ + gpiodetect.man \ + gpioinfo.man \ + gpioget.man \ + gpioset.man \ + gpiomon.man \ + gpionotify.man %.man: $(top_builddir)/tools/$(*F) help2man $(top_builddir)/tools/$(*F) --include=$(srcdir)/template --output=$(builddir)/$@ --no-info
tests/Makefile.am +17 -17
@@ -14,20 +14,20 @@ LDADD += $(GLIB_LIBS) $(GOBJECT_LIBS) bin_PROGRAMS = gpiod-test -gpiod_test_SOURCES = \ - gpiod-test.c \ - gpiod-test.h \ - gpiod-test-helpers.c \ - gpiod-test-helpers.h \ - gpiod-test-sim.c \ - gpiod-test-sim.h \ - tests-chip.c \ - tests-chip-info.c \ - tests-edge-event.c \ - tests-info-event.c \ - tests-line-config.c \ - tests-line-info.c \ - tests-line-request.c \ - tests-line-settings.c \ - tests-misc.c \ - tests-request-config.c +gpiod_test_SOURCES = \ + gpiod-test.c \ + gpiod-test.h \ + gpiod-test-helpers.c \ + gpiod-test-helpers.h \ + gpiod-test-sim.c \ + gpiod-test-sim.h \ + tests-chip.c \ + tests-chip-info.c \ + tests-edge-event.c \ + tests-info-event.c \ + tests-line-config.c \ + tests-line-info.c \ + tests-line-request.c \ + tests-line-settings.c \ + tests-misc.c \ + tests-request-config.c
Signature · 1 line
2.37.2
PATCH 4/16 treewide: unify gpiod_line_config/request_get_offsets() functions
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
We have two functions in the C API that allow users to retrieve a list of offsets from objects: gpiod_line_request_get_offsets() and gpiod_line_config_get_offsets(). Even though they serve pretty much the same purpose, they have different signatures and one of them also requires the user to free the memory allocated within the libgpiod library with a non-libgpiod free() function.
Unify them: make them take the array in which to store offsets and the size of this array. Make them return the number of offsets actually stored in the array and make them impossible to fail. Change their names to be more descriptive and in the case of line_config: add a new function that allows users to get the number of configured offsets.
Update the entire tree to use the new interfaces.
For rust bindings: also unify the line config interface to return a map of line settings like C++ bindings do instead of having a function to get settings by offset. A map returned from a single call is easier to iterate over with a for loop than using an integer and calling the previous line_settings() method.
--- bindings/cxx/line-config.cpp | 18 ++---- bindings/cxx/line-request.cpp | 6 +- bindings/python/gpiod/ext/request.c | 8 +-- bindings/rust/libgpiod/src/line_config.rs | 71 ++++++++------------- bindings/rust/libgpiod/src/line_request.rs | 6 +- bindings/rust/libgpiod/tests/line_config.rs | 24 ++----- configure.ac | 1 + include/gpiod.h | 54 ++++++++++------ lib/line-config.c | 38 +++++------ lib/line-request.c | 23 ++++--- tests/tests-line-config.c | 32 ++++------ tests/tests-line-request.c | 10 +-- 12 files changed, 139 insertions(+), 152 deletions(-)
bindings/cxx/line-config.cpp +7 -11
@@ -102,31 +102,27 @@ GPIOD_CXX_API line_config& line_config::add_line_settings(const line::offsets& o GPIOD_CXX_API ::std::map<line::offset, line_settings> line_config::get_line_settings() const { + ::std::size_t num_offsets = ::gpiod_line_config_get_num_configured_offsets( + this->_m_priv->config.get()); ::std::map<line::offset, line_settings> settings_map; - ::std::size_t num_offsets; - unsigned int *offsets_ptr; - int ret; - - ret = ::gpiod_line_config_get_offsets(this->_m_priv->config.get(), - &num_offsets, &offsets_ptr); - if (ret) - throw_from_errno("unable to retrieve line offsets"); + ::std::vector<unsigned int> offsets(num_offsets); if (num_offsets == 0) return settings_map; - ::std::unique_ptr<unsigned int, malloc_deleter> offsets(offsets_ptr); + ::gpiod_line_config_get_configured_offsets(this->_m_priv->config.get(), + offsets.data(), num_offsets); for (size_t i = 0; i < num_offsets; i++) { line_settings settings; settings._m_priv->settings.reset(::gpiod_line_config_get_line_settings( this->_m_priv->config.get(), - offsets.get()[i])); + offsets[i])); if (!settings._m_priv->settings) throw_from_errno("unable to retrieve line settings"); - settings_map[offsets.get()[i]] = ::std::move(settings); + settings_map[offsets[i]] = ::std::move(settings); } return settings_map;
bindings/cxx/line-request.cpp +3 -3
@@ -18,7 +18,7 @@ void line_request::impl::throw_if_released() const void line_request::impl::set_request_ptr(line_request_ptr& ptr) { this->request = ::std::move(ptr); - this->offset_buf.resize(::gpiod_line_request_get_num_lines(this->request.get())); + this->offset_buf.resize(::gpiod_line_request_get_num_requested_lines(this->request.get())); } void line_request::impl::fill_offset_buf(const line::offsets& offsets)
@@ -67,7 +67,7 @@ GPIOD_CXX_API ::std::size_t line_request::num_lines() const { this->_m_priv->throw_if_released(); - return ::gpiod_line_request_get_num_lines(this->_m_priv->request.get()); + return ::gpiod_line_request_get_num_requested_lines(this->_m_priv->request.get()); } GPIOD_CXX_API line::offsets line_request::offsets() const
@@ -78,7 +78,7 @@ GPIOD_CXX_API line::offsets line_request::offsets() const ::std::vector<unsigned int> buf(num_lines); line::offsets offsets(num_lines); - ::gpiod_line_request_get_offsets(this->_m_priv->request.get(), buf.data()); + ::gpiod_line_request_get_requested_offsets(this->_m_priv->request.get(), buf.data(), buf.size()); for (unsigned int i = 0; i < num_lines; i++) offsets[i] = buf[i];
bindings/python/gpiod/ext/request.c +4 -4
@@ -41,7 +41,7 @@ static PyObject * request_num_lines(request_object *self, void *Py_UNUSED(ignored)) {return PyLong_FromUnsignedLong ( -gpiod_line_request_get_num_lines (self->request)); +gpiod_line_request_get_num_requested_lines (self->request)); }static PyObject *request_offsets (request_object *self,void *Py_UNUSED (ignored ))
@@ -51,13 +51,13 @@ static PyObject *request_offsets(request_object *self, void *Py_UNUSED(ignored))size_t num_lines, i;int ret; - num_lines =gpiod_line_request_get_num_lines (self->request); + num_lines =gpiod_line_request_get_num_requested_lines (self->request); offsets =PyMem_Calloc (num_lines,sizeof (unsigned int ));if (!offsets)return PyErr_NoMemory (); -gpiod_line_request_get_offsets (self->request, offsets); +gpiod_line_request_get_requested_offsets (self->request, offsets, num_lines); lines =PyList_New (num_lines);if (!lines) {
@@ -365,7 +365,7 @@ PyObject *Py_gpiod_MakeRequestObject(struct gpiod_line_request *request,unsigned int *offsets;size_t num_lines; - num_lines =gpiod_line_request_get_num_lines (request); + num_lines =gpiod_line_request_get_num_requested_lines (request); req_obj =PyObject_New (request_object, &request_type);if (!req_obj)
bindings/rust/libgpiod/src/line_config.rs +26 -45
85 lines not on this page · raw message
bindings/rust/libgpiod/src/line_request.rs +4 -2
@@ -28,7 +28,7 @@ impl Request {/// Get the number of lines in the request. pub fn num_lines (&self) ->usize {// SAFETY: `gpiod_line_request` is guaranteed to be valid here. -unsafe { gpiod::gpiod_line_request_get_num_lines (self.request)as usize } +unsafe { gpiod::gpiod_line_request_get_num_requested_lines (self.request)as usize } }/// Get the offsets of lines in the request.
@@ -36,7 +36,9 @@ impl Request {let mut offsets =vec! [0 ; self.num_lines ()as usize ];// SAFETY: `gpiod_line_request` is guaranteed to be valid here. -unsafe { gpiod::gpiod_line_request_get_offsets (self.request, offsets.as_mut_ptr ()) }; +unsafe { gpiod::gpiod_line_request_get_requested_offsets (self.request, + offsets.as_mut_ptr (), + self.num_lines ()as u64 ) }; offsets }
bindings/rust/libgpiod/tests/line_config.rs +4 -20
@@ -37,8 +37,10 @@ mod line_config { lconfig.add_line_settings (&[0 ,1 ,2 ], lsettings1).unwrap (); lconfig.add_line_settings (&[4 ,5 ], lsettings2).unwrap (); +let settings_map = lconfig.line_settings ().unwrap (); +// Retrieve settings -let lsettings = lconfig.line_settings (1 ).unwrap (); +let lsettings = settings_map.get (&1 ).unwrap ();assert_eq! ( lsettings.prop (SettingKind ::Direction ).unwrap (),SettingVal ::Direction (Direction ::Input )
@@ -56,7 +58,7 @@ mod line_config {SettingVal ::Drive (Drive ::PushPull ) ); -let lsettings = lconfig.line_settings (5 ).unwrap (); +let lsettings = settings_map.get (&5 ).unwrap ();assert_eq! ( lsettings.prop (SettingKind ::Direction ).unwrap (),SettingVal ::Direction (Direction ::Output )
@@ -74,22 +76,4 @@ mod line_config {SettingVal ::OutputValue (Value ::Active ) ); } - -#[test] -fn offsets () { -let mut lsettings1 = line::Settings ::new ().unwrap (); - lsettings1.set_direction (Direction ::Input ).unwrap (); - -let mut lsettings2 = line::Settings ::new ().unwrap (); - lsettings2.set_event_clock (EventClock ::Realtime ).unwrap (); - -// Add settings for multiple lines -let lconfig = line::Config ::new ().unwrap (); - lconfig.add_line_settings (&[0 ,1 ,2 ], lsettings1).unwrap (); - lconfig.add_line_settings (&[4 ,5 ], lsettings2).unwrap (); - -// Verify offsets -let offsets = lconfig.offsets ().unwrap (); -assert_eq! (offsets, [0 ,1 ,2 ,4 ,5 ]); - } }
configure.ac +1 -0
@@ -94,6 +94,7 @@ AC_CHECK_HEADERS([dirent.h], [], [HEADER_NOT_FOUND_LIB([dirent.h])]) AC_CHECK_HEADERS([poll.h], [], [HEADER_NOT_FOUND_LIB([poll.h])]) AC_CHECK_HEADERS([sys/sysmacros.h], [], [HEADER_NOT_FOUND_LIB([sys/sysmacros.h])]) AC_CHECK_HEADERS([sys/ioctl.h], [], [HEADER_NOT_FOUND_LIB([sys/ioctl.h])]) +AC_CHECK_HEADERS([sys/param.h], [], [HEADER_NOT_FOUND_LIB([sys/param.h])]) AC_CHECK_HEADERS([sys/stat.h], [], [HEADER_NOT_FOUND_LIB([sys/stat.h])]) AC_CHECK_HEADERS([sys/types.h], [], [HEADER_NOT_FOUND_LIB([sys/types.h])]) AC_CHECK_HEADERS([linux/const.h], [], [HEADER_NOT_FOUND_LIB([linux/const.h])])
include/gpiod.h +36 -18
91 lines not on this page · raw message
lib/line-config.c +19 -19
@@ -6,6 +6,7 @@#include <gpiod.h> #include <stdlib.h> #include <string.h> +#include <sys/param.h> #include "internal.h"
@@ -152,36 +153,35 @@ gpiod_line_config_get_line_settings(struct gpiod_line_config *config,return NULL ; } -GPIOD_API int gpiod_line_config_get_offsets(struct gpiod_line_config *config, -size_t *num_offsets, -unsigned int **offsets) +GPIOD_API size_t +gpiod_line_config_get_num_configured_offsets (struct gpiod_line_config *config) { -unsigned int *offs; -size_t i; +assert (config); + +return config->num_configs; +} + +GPIOD_API size_t +gpiod_line_config_get_configured_offsets (struct gpiod_line_config *config, +unsigned int *offsets, +size_t max_offsets) +{ +size_t num_offsets, i;assert (config); -if (!num_offsets || !offsets) { - errno =EINVAL ; -return -1 ; - } +if (!offsets || !max_offsets) +return 0 ; - *num_offsets = config->num_configs; - *offsets =NULL ; + num_offsets =MIN (config->num_configs, max_offsets);if (!config->num_configs)return 0 ; - offs =calloc (config->num_configs,sizeof (unsigned int )); -if (!offs) -return -1 ; -for (i =0 ; i < config->num_configs; i++) - offs[i] = config->line_configs[i].offset; - - *offsets = offs; + offsets[i] = config->line_configs[i].offset; -return 0 ; +return num_offsets; } static void set_offsets(struct gpiod_line_config *config,
lib/line-request.c +15 -8
@@ -7,6 +7,7 @@#include <stdlib.h> #include <string.h> #include <sys/ioctl.h> +#include <sys/param.h> #include <unistd.h> #include "internal.h"
@@ -45,24 +46,30 @@ GPIOD_API void gpiod_line_request_release(struct gpiod_line_request *request) } GPIOD_API size_t -gpiod_line_request_get_num_lines (struct gpiod_line_request *request) +gpiod_line_request_get_num_requested_lines (struct gpiod_line_request *request) {assert (request);return request->num_lines; } -GPIOD_API void -gpiod_line_request_get_offsets (struct gpiod_line_request *request, -unsigned int *offsets) +GPIOD_API size_t +gpiod_line_request_get_requested_offsets (struct gpiod_line_request *request, +unsigned int *offsets, +size_t max_offsets) { +size_t num_offsets; +assert (request); -if (!offsets) -return ; +if (!offsets || !max_offsets) +return 0 ; + + num_offsets =MIN (request->num_lines, max_offsets); + +memcpy (offsets, request->offsets,sizeof (*offsets) * num_offsets); -memcpy (offsets, request->offsets, -sizeof (*offsets) * request->num_lines); +return num_offsets; } GPIOD_API enum gpiod_line_value
tests/tests-line-config.c +14 -18
@@ -190,10 +190,8 @@ GPIOD_TEST_CASE(get_offsets) {g_autoptr (struct_gpiod_line_settings) settings =NULL ;g_autoptr (struct_gpiod_line_config) config =NULL ; - g_autofree guint *config_offs = NULL; -guint offsets[8 ]; +guint offsets[8 ], offsets_in[4 ];size_t num_offsets; -gint ret; settings =gpiod_test_create_line_settings_or_fail (); config =gpiod_test_create_line_config_or_fail ();
@@ -211,39 +209,37 @@ GPIOD_TEST_CASE(get_offsets)gpiod_test_line_config_add_line_settings_or_fail (config, offsets,2 , settings); - ret =gpiod_line_config_get_offsets (config, &num_offsets, &config_offs); - g_assert_cmpint(ret, ==, 0); + num_offsets =gpiod_line_config_get_configured_offsets (config, + offsets_in,4 ); g_assert_cmpuint(num_offsets, ==, 4); - g_assert_cmpuint(config_offs[0], ==, 2); - g_assert_cmpuint(config_offs[1], ==, 4); - g_assert_cmpuint(config_offs[2], ==, 6); - g_assert_cmpuint(config_offs[3], ==, 7); + g_assert_cmpuint(offsets_in[0], ==, 2); + g_assert_cmpuint(offsets_in[1], ==, 4); + g_assert_cmpuint(offsets_in[2], ==, 6); + g_assert_cmpuint(offsets_in[3], ==, 7); }GPIOD_TEST_CASE (get_0_offsets) {g_autoptr (struct_gpiod_line_config) config =NULL ; - g_autofree guint *offsets = NULL;size_t num_offsets; -gint ret; +guint offsets[3 ]; config =gpiod_test_create_line_config_or_fail (); - ret =gpiod_line_config_get_offsets (config, &num_offsets, &offsets); - g_assert_cmpint(ret, ==, 0); + num_offsets =gpiod_line_config_get_configured_offsets (config, + offsets,0 ); g_assert_cmpuint(num_offsets, ==, 0); -g_assert_null (offsets); }GPIOD_TEST_CASE (get_null_offsets) {g_autoptr (struct_gpiod_line_config) config =NULL ; g_autofree guint *offsets = NULL; -gint ret; +size_t num_offsets; config =gpiod_test_create_line_config_or_fail (); - ret =gpiod_line_config_get_offsets (config,NULL , &offsets); - g_assert_cmpint(ret, ==, -1); -gpiod_test_expect_errno (EINVAL ); + num_offsets =gpiod_line_config_get_configured_offsets (config, +NULL ,10 ); + g_assert_cmpuint(num_offsets, ==, 0); }
tests/tests-line-request.c +6 -4
@@ -45,9 +45,10 @@ GPIOD_TEST_CASE(request_fails_with_duplicate_offsets) request =gpiod_chip_request_lines (chip,NULL , line_cfg);g_assert_nonnull (request); - num_requested_offsets =gpiod_line_request_get_num_lines (request); + num_requested_offsets = +gpiod_line_request_get_num_requested_lines (request); g_assert_cmpuint(num_requested_offsets, ==, 3); -gpiod_line_request_get_offsets (request, requested_offsets); +gpiod_line_request_get_requested_offsets (request, requested_offsets,4 ); g_assert_cmpuint(requested_offsets[0], ==, 0); g_assert_cmpuint(requested_offsets[1], ==, 2); g_assert_cmpuint(requested_offsets[2], ==, 3);
@@ -401,9 +402,10 @@ GPIOD_TEST_CASE(num_lines_and_offsets) request =gpiod_test_request_lines_or_fail (chip,NULL , line_cfg); - g_assert_cmpuint(gpiod_line_request_get_num_lines(request), ==, 8); +g_assert_cmpuint (gpiod_line_request_get_num_requested_lines (request), + ==, 8);gpiod_test_return_if_failed (); -gpiod_line_request_get_offsets (request, read_back); +gpiod_line_request_get_requested_offsets (request, read_back,8 );for (i =0 ; i <8 ; i++) g_assert_cmpuint(read_back[i], ==, offsets[i]); }
Signature · 1 line
2.37.2
Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
Quoted 9 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
We have two functions in the C API that allow users to retrieve a list of offsets from objects:
gpiod_line_request_get_offsets()andgpiod_line_config_get_offsets(). Even though they serve pretty much the same purpose, they have different signatures and one of them also requires the user to free the memory allocated within the libgpiod library with a non-libgpiodfree()function.
They differ because they operate in different circumstances. Requests are immutable, wrt lines/offsets, while configs are not. More on this below.
Quoted 6 lines
Unify them: make them take the array in which to store offsets and the size of this array. Make them return the number of offsets actually stored in the array and make them impossible to fail. Change their names to be more descriptive and in the case of
line_config: add a new function that allows users to get the number of configured offsets.
Not sure symmetry => beauty in this case.
Quoted 9 lines
Update the entire tree to use the new interfaces.
For rust bindings: also unify the line config interface to return a map of line settings like C++ bindings do instead of having a function to get settings by offset. A map returned from a single call is easier to iterate over with a for loop than using an integer and calling the previous
line_settings()method.Signed-off-by Bartosz Golaszewski <[email protected]>
<snip>
Quoted 4 lines
include/gpiod.h +0
-0@@ -780,19 +780,29 @@ struct gpiod_line_settings * gpiod_line_config_get_line_settings(struct gpiod_line_config *config,
include/gpiod.h struct gpiod_line_settings * · line 790
Quoted 10 lines
include/gpiod.h +8
-0unsigned int offset); +/** +* @brief Get the number of configured line offsets. +* @param config Line config object. +* @return Number of offsets for which line settings have been added. +*/ +size_t +gpiod_line_config_get_num_configured_offsets (struct gpiod_line_config *config); +
^^^^^^ ^^^^^^^^^^
Not a fan of the stuttering.
What other kinds of lines are there in the config? Similarly requested lines in the request.
Quoted 15 lines
include/gpiod.h +6
-6/** * @brief Get configured offsets. * @param config Line config object. -* @param num_offsets Pointer to a variable in which the number of line offsets -* will be stored. -* @param offsets Pointer to a pointer which will be set to point to an array -* containing the configured offsets. The array will be allocated -* using malloc() and must be freed using free(). -* @return 0 on success, -1 on failure. +* @param offsets Array to store offsets. +* @param max_offsets Number of offsets that can be stored in the offsets array. +* @return Number of offsets stored in the offsets array. +* +* If max_offsets is lower than the number of lines actually requested (this +* value can be retrieved using ::gpiod_line_config_get_num_configured_offsets),
include/gpiod.h struct gpiod_line_settings * · line 806
Quoted 10 lines
include/gpiod.h +5
-3+* then only up to max_lines offsets will be stored in offsets. */ -int gpiod_line_config_get_offsets (struct gpiod_line_config *config, -size_t *num_offsets, -unsigned int **offsets); +size_t +gpiod_line_config_get_configured_offsets (struct gpiod_line_config *config, +unsigned int *offsets, +size_t max_offsets);
So, to be sure that they have all the offsets, the user needs to call gpiod_line_config_get_num_configured_offsets() before every call to gpiod_line_config_get_configured_offsets()?
Unless they can be sure no lines have been added subsequently?
Another way of looking at it is that the old API combines those two functions in one, hence the difference from the request equivalent. The old way you got them all, no matter what and no questions asked. That seems simpler to me, so I'm not sure this is a step forward.
Cheers,
Kent.
Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
Quoted 51 lines · Kent Gibson
Quoted 9 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
We have two functions in the C API that allow users to retrieve a list of offsets from objects:
gpiod_line_request_get_offsets()andgpiod_line_config_get_offsets(). Even though they serve pretty much the same purpose, they have different signatures and one of them also requires the user to free the memory allocated within the libgpiod library with a non-libgpiodfree()function.They differ because they operate in different circumstances. Requests are immutable, wrt lines/offsets, while configs are not. More on this below.
Quoted 6 lines
Unify them: make them take the array in which to store offsets and the size of this array. Make them return the number of offsets actually stored in the array and make them impossible to fail. Change their names to be more descriptive and in the case of
line_config: add a new function that allows users to get the number of configured offsets.Not sure symmetry => beauty in this case.
Quoted 9 lines
Update the entire tree to use the new interfaces.
For rust bindings: also unify the line config interface to return a map of line settings like C++ bindings do instead of having a function to get settings by offset. A map returned from a single call is easier to iterate over with a for loop than using an integer and calling the previous
line_settings()method.Signed-off-by Bartosz Golaszewski <[email protected]><snip>
Quoted 4 lines
include/gpiod.h +0
-0@@ -780,19 +780,29 @@ struct gpiod_line_settings * gpiod_line_config_get_line_settings(struct gpiod_line_config *config,include/gpiod.h struct gpiod_line_settings * · line 790
Quoted 10 lines
include/gpiod.h +8
-0unsigned int offset); +/** +* @brief Get the number of configured line offsets. +* @param config Line config object. +* @return Number of offsets for which line settings have been added. +*/ +size_t +gpiod_line_config_get_num_configured_offsets (struct gpiod_line_config *config); +^^^^^^ ^^^^^^^^^^
Quoted 2 lines
Not a fan of the stuttering.
What other kinds of lines are there in the config?
The user may not have an in-depth knowledge of the data model we use and just wants to use the library. I think this name is much more descriptive that way. It's not that long or repetetive, have you seen udev_monitor_filter_add_match_subsystem_devtype() or kmod_module_dependency_symbol_get_symbol()? :)
Quoted 24 lines
Similarly requested lines in the request.
Quoted 22 lines
include/gpiod.h +9
-9/** * @brief Get configured offsets. * @param config Line config object. -* @param num_offsets Pointer to a variable in which the number of line offsets -* will be stored. -* @param offsets Pointer to a pointer which will be set to point to an array -* containing the configured offsets. The array will be allocated -* using malloc() and must be freed using free(). -* @return 0 on success, -1 on failure. +* @param offsets Array to store offsets. +* @param max_offsets Number of offsets that can be stored in the offsets array. +* @return Number of offsets stored in the offsets array. +* +* If max_offsets is lower than the number of lines actually requested (this +* value can be retrieved using ::gpiod_line_config_get_num_configured_offsets), +* then only up to max_lines offsets will be stored in offsets. */ -int gpiod_line_config_get_offsets (struct gpiod_line_config *config, -size_t *num_offsets, -unsigned int **offsets); +size_t +gpiod_line_config_get_configured_offsets(struct gpiod_line_config *config,
Quoted 9 lines
include/gpiod.h struct gpiod_line_settings * · line 806
Quoted 3 lines
include/gpiod.h +2
-0+unsigned int *offsets, + size_t max_offsets);So, to be sure that they have all the offsets, the user needs to call
gpiod_line_config_get_num_configured_offsets()before every call togpiod_line_config_get_configured_offsets()?
Unless they can be sure no lines have been added subsequently?
Yes. Just like when you add config for new lines and need to be sure it hasn't changed between that and the request. Perfectly normal for a library that doesn't provide any thread-safety.
Quoted 5 lines
Another way of looking at it is that the old API combines those two functions in one, hence the difference from the request equivalent. The old way you got them all, no matter what and no questions asked. That seems simpler to me, so I'm not sure this is a step forward.
I'd like to avoid having the library provide resources that are then managed with functions from outside of the library (even if it's a standard library function). That's bad practice and we should provide our own function for freeing the allocated resource which in turn makes the API more complicated than what this patch proposes. We still need that *_get_num_*() variant.
Bart
Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
Quoted 55 lines · Bartosz Golaszewski
Quoted 51 lines · Kent Gibson
Quoted 9 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
We have two functions in the C API that allow users to retrieve a list of offsets from objects:
gpiod_line_request_get_offsets()andgpiod_line_config_get_offsets(). Even though they serve pretty much the same purpose, they have different signatures and one of them also requires the user to free the memory allocated within the libgpiod library with a non-libgpiodfree()function.They differ because they operate in different circumstances. Requests are immutable, wrt lines/offsets, while configs are not. More on this below.
Quoted 6 lines
Unify them: make them take the array in which to store offsets and the size of this array. Make them return the number of offsets actually stored in the array and make them impossible to fail. Change their names to be more descriptive and in the case of
line_config: add a new function that allows users to get the number of configured offsets.Not sure symmetry => beauty in this case.
Quoted 9 lines
Update the entire tree to use the new interfaces.
For rust bindings: also unify the line config interface to return a map of line settings like C++ bindings do instead of having a function to get settings by offset. A map returned from a single call is easier to iterate over with a for loop than using an integer and calling the previous
line_settings()method.Signed-off-by Bartosz Golaszewski <[email protected]><snip>
Quoted 4 lines
include/gpiod.h +0
-0@@ -780,19 +780,29 @@ struct gpiod_line_settings * gpiod_line_config_get_line_settings(struct gpiod_line_config *config,include/gpiod.h struct gpiod_line_settings * · line 790
Quoted 10 lines
include/gpiod.h +8
-0unsigned int offset); +/** +* @brief Get the number of configured line offsets. +* @param config Line config object. +* @return Number of offsets for which line settings have been added. +*/ +size_t +gpiod_line_config_get_num_configured_offsets (struct gpiod_line_config *config); +^^^^^^ ^^^^^^^^^^Quoted 2 lines
Not a fan of the stuttering.
What other kinds of lines are there in the config?
Quoted 6 lines
The user may not have an in-depth knowledge of the data model we use and just wants to use the library. I think this name is much more descriptive that way. It's not that long or repetetive, have you seen
udev_monitor_filter_add_match_subsystem_devtype()orkmod_module_dependency_symbol_get_symbol()? :)
If the data model is unclear to the user then you just made it less clear as the implication from this naming is that there are OTHER types of offsets/lines, but there is not.
I don't have a problem with your counter examples. The first does not stutter, and in the second the "symbol" appears to be the object, not an adjective. Length is not the issue - it is clarity. :|
But whatever - this is verging on bikeshedding.
Cheers,
Kent.
Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
In reply to Bartosz GolaszewskiFor Rust changes, please run these as well to find any formatting issues, warnings:
cargo fmt --all -- --check
cargo clippy --release --workspace --bins --examples --tests --benches --all-features --all-targets -- -D warnings -D clippy::undocumented_unsafe_blocks
Quoted 21 lines · Bartosz Golaszewski
bindings/rust/libgpiod/src/line_config.rs +2
-4@@ -2,8 +2,8 @@// SPDX-FileCopyrightText: 2022 Linaro Ltd. // SPDX-FileCopyrightTest: 2022 Viresh Kumar <[email protected]> -use std::os::raw::{c_ulong, c_void}; -use std::slice; +use std::os::raw::c_ulong; +use std::collections::HashMap ;use super ::{ gpiod,@@ -77,51 +77,32 @@ impl Config { } } -/// Get line settings for offset. -pub fn line_settings (&self, offset:Offset ) ->Result <Settings > {
bindings/rust/libgpiod/src/line_config.rs impl Config { · line 81
Quoted 10 lines
bindings/rust/libgpiod/src/line_config.rs +2
-8-// SAFETY: `gpiod_line_config` is guaranteed to be valid here. -let settings =unsafe { gpiod::gpiod_line_config_get_line_settings (self.config, offset) }; - -if settings.is_null () { -return Err (Error ::OperationFailed ( -OperationType ::LineConfigGetSettings , - errno::errno (), - )); +/// Get a mapping of offsets to line settings stored by this object. +pub fn line_settings (&self) ->Result <HashMap <Offset ,Settings >> {
Just like ValueMap, maybe we can add following in lib.rs for this:
pub type line::SettingsMap = IntMap<line::Settings>;
bindings/rust/libgpiod/src/line_config.rs impl Config { · line 83
Quoted 2 lines
bindings/rust/libgpiod/src/line_config.rs +2
-0+let mut map:HashMap <Offset ,Settings > =HashMap ::new (); +let num_lines =unsafe { gpiod::gpiod_line_config_get_num_configured_offsets (self.config) };
This needs a SAFETY comment. Should we check if this returned 0 ?
bindings/rust/libgpiod/src/line_config.rs impl Config { · line 89
Quoted 6 lines
bindings/rust/libgpiod/src/line_config.rs +6
-0+let mut offsets =vec! [0 ; num_linesas usize ]; + +// SAFETY: gpiod_line_config is guaranteed to be valid here. +unsafe { gpiod::gpiod_line_config_get_configured_offsets (self.config, + offsets.as_mut_ptr (), + num_lines) };
Can the returned value be < num_lines here ?
Quoted 51 lines
bindings/rust/libgpiod/src/line_config.rs +14
-33+ +for offsetin offsets { +// SAFETY: `gpiod_line_config` is guaranteed to be valid here. +let settings =unsafe { gpiod::gpiod_line_config_get_line_settings (self.config, + offset) }; +if settings.is_null () { +return Err (Error ::OperationFailed ( +OperationType ::LineConfigGetSettings , + errno::errno (), + )); + } + + map.insert (offset,Settings ::new_with_settings (settings)); } -Ok (Settings ::new_with_settings (settings)) - } - -/// Get configured offsets. -pub fn offsets (&self) ->Result <Vec <Offset >> { -let mut num:u64 =0 ; -let mut ptr: *mut Offset = std::ptr::null_mut (); - -// SAFETY: The `ptr` array returned by libgpiod is guaranteed to live as long -// as it is not explicitly freed with `free()`. -let ret =unsafe { - gpiod::gpiod_line_config_get_offsets ( - self.config, - &mut numas *mut _ as *mut _ , - &mut ptr, - ) - }; - -if ret == -1 { -return Err (Error ::OperationFailed ( -OperationType ::LineConfigGetOffsets , - errno::errno (), - )); - } - -// SAFETY: The `ptr` array returned by libgpiod is guaranteed to live as long -// as it is not explicitly freed with `free()`. -let offsets =unsafe { slice::from_raw_parts (ptras *const Offset , numas usize ).to_vec () }; - -// SAFETY: The `ptr` array is guaranteed to be valid here. -unsafe { libc::free (ptras *mut c_void ) }; - -Ok (offsets) +Ok (map) } }
Signature · 1 line
viresh
Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
Quoted 49 lines · Viresh Kumar
For Rust changes, please run these as well to find any formatting issues, warnings:
cargo fmt --all -- --check
cargo clippy --release --workspace --bins --examples --tests --benches --all-features --all-targets -- -D warnings -Dclippy::undocumented_unsafe_blocksQuoted 21 lines · Bartosz Golaszewski
bindings/rust/libgpiod/src/line_config.rs +2
-4@@ -2,8 +2,8 @@// SPDX-FileCopyrightText: 2022 Linaro Ltd. // SPDX-FileCopyrightTest: 2022 Viresh Kumar <[email protected]> -use std::os::raw::{c_ulong, c_void}; -use std::slice; +use std::os::raw::c_ulong; +use std::collections::HashMap ;use super ::{ gpiod,@@ -77,51 +77,32 @@ impl Config { } } -/// Get line settings for offset. -pub fn line_settings (&self, offset:Offset ) ->Result <Settings > {bindings/rust/libgpiod/src/line_config.rs impl Config { · line 81
Quoted 10 lines
bindings/rust/libgpiod/src/line_config.rs +2
-8-// SAFETY: `gpiod_line_config` is guaranteed to be valid here. -let settings =unsafe { gpiod::gpiod_line_config_get_line_settings (self.config, offset) }; - -if settings.is_null () { -return Err (Error ::OperationFailed ( -OperationType ::LineConfigGetSettings , - errno::errno (), - )); +/// Get a mapping of offsets to line settings stored by this object. +pub fn line_settings (&self) ->Result <HashMap <Offset ,Settings >> {Just like ValueMap, maybe we can add following in
lib.rsfor this:pub type
line::SettingsMap= IntMap<line::Settings>;bindings/rust/libgpiod/src/line_config.rs impl Config { · line 83
Quoted 2 lines
bindings/rust/libgpiod/src/line_config.rs +2
-0+let mut map:HashMap <Offset ,Settings > =HashMap ::new (); +let num_lines =unsafe { gpiod::gpiod_line_config_get_num_configured_offsets (self.config) };This needs a SAFETY comment. Should we check if this returned 0 ?
Quoted 9 lines
bindings/rust/libgpiod/src/line_config.rs impl Config { · line 89
Quoted 6 lines
bindings/rust/libgpiod/src/line_config.rs +6
-0+let mut offsets =vec! [0 ; num_linesas usize ]; + +// SAFETY: gpiod_line_config is guaranteed to be valid here. +unsafe { gpiod::gpiod_line_config_get_configured_offsets (self.config, + offsets.as_mut_ptr (), + num_lines) };Can the returned value be <
num_lineshere ?
Ah, of course it can. Need to add a test case for that. How do I set the size of offsets to whatever this function returns?
Quoted 54 lines
Quoted 41 lines
bindings/rust/libgpiod/src/line_config.rs +13
-26+ +for offsetin offsets { +// SAFETY: `gpiod_line_config` is guaranteed to be valid here. +let settings =unsafe { gpiod::gpiod_line_config_get_line_settings (self.config, + offset) }; +if settings.is_null () { +return Err (Error ::OperationFailed ( +OperationType ::LineConfigGetSettings , + errno::errno (), + )); + } + + map.insert (offset,Settings ::new_with_settings (settings)); } -Ok (Settings ::new_with_settings (settings)) - } - -/// Get configured offsets. -pub fn offsets (&self) ->Result <Vec <Offset >> { -let mut num:u64 =0 ; -let mut ptr: *mut Offset = std::ptr::null_mut (); - -// SAFETY: The `ptr` array returned by libgpiod is guaranteed to live as long -// as it is not explicitly freed with `free()`. -let ret =unsafe { - gpiod::gpiod_line_config_get_offsets ( - self.config, - &mut numas *mut _ as *mut _ , - &mut ptr, - ) - }; - -if ret == -1 { -return Err (Error ::OperationFailed ( -OperationType ::LineConfigGetOffsets , - errno::errno (), - )); - } - -// SAFETY: The `ptr` array returned by libgpiod is guaranteed to live as long bindings/rust/libgpiod/src/line_config.rs impl Config { · line 107
Quoted 10 lines
bindings/rust/libgpiod/src/line_config.rs +1
-7-// as it is not explicitly freed with `free()`. -let offsets =unsafe { slice::from_raw_parts (ptras *const Offset , numas usize ).to_vec () }; - -// SAFETY: The `ptr` array is guaranteed to be valid here. -unsafe { libc::free (ptras *mut c_void ) }; - -Ok (offsets) +Ok (map) } }--
viresh
Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
Quoted 8 lines · Bartosz Golaszewski
Quoted 7 lines · Viresh Kumar
Quoted 6 lines
bindings/rust/libgpiod/src/line_config.rs +6
-0+let mut offsets =vec! [0 ; num_linesas usize ]; + +// SAFETY: gpiod_line_config is guaranteed to be valid here. +unsafe { gpiod::gpiod_line_config_get_configured_offsets (self.config, + offsets.as_mut_ptr (), + num_lines) };
Quoted 5 lines
Quoted 2 lines
Can the returned value be <
num_lineshere ?Ah, of course it can. Need to add a test case for that. How do I set the size of offsets to whatever this function returns?
Instead of any heavy operation, you can rather do something like this:
let num = unsafe { gpiod::gpiod_line_config_get_configured_offsets(self.config,
offsets.as_mut_ptr(), num_lines) };
for offset in offsets[0..num] {
...
}Signature · 1 line
viresh
Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
Quoted 16 lines · Viresh Kumar
Quoted 8 lines · Bartosz Golaszewski
Quoted 7 lines · Viresh Kumar
Quoted 6 lines
bindings/rust/libgpiod/src/line_config.rs +6
-0+let mut offsets =vec! [0 ; num_linesas usize ]; + +// SAFETY: gpiod_line_config is guaranteed to be valid here. +unsafe { gpiod::gpiod_line_config_get_configured_offsets (self.config, + offsets.as_mut_ptr (), + num_lines) };Quoted 5 lines
Quoted 2 lines
Can the returned value be <
num_lineshere ?Ah, of course it can. Need to add a test case for that. How do I set the size of offsets to whatever this function returns?
Quoted 8 lines
Instead of any heavy operation, you can rather do something like this:
let num = unsafe { gpiod::gpiod_line_config_get_configured_offsets(self.config,
offsets.as_mut_ptr(), num_lines) }; for offset in offsets[0..num] { ... }
It sees 'offset' becomes an instance of std::ops::Range. Is there anything more to add here?
Bart
Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
Quoted 26 lines · Bartosz Golaszewski
Quoted 16 lines · Viresh Kumar
Quoted 8 lines · Bartosz Golaszewski
Quoted 7 lines · Viresh Kumar
Quoted 6 lines
(quoted diff) +6
-0+ let mut offsets = vec![0; num_lines as usize]; + + // SAFETY: gpiod_line_config is guaranteed to be valid here. + unsafe { gpiod::gpiod_line_config_get_configured_offsets(self.config, + offsets.as_mut_ptr(), + num_lines) };Quoted 5 lines
Quoted 2 lines
Can the returned value be <
num_lineshere ?Ah, of course it can. Need to add a test case for that. How do I set the size of offsets to whatever this function returns?
Quoted 8 lines
Instead of any heavy operation, you can rather do something like this:
let num = unsafe { gpiod::gpiod_line_config_get_configured_offsets(self.config,
offsets.as_mut_ptr(), num_lines) }; for offset in offsets[0..num] { ... }
Quoted 2 lines
It sees 'offset' becomes an instance of
std::ops::Range. Is there anything more to add here?
This builds fine.
bindings/rust/libgpiod/src/line_config.rs +4 -4
@@ -106,14 +106,14 @@ impl Config { let mut offsets = vec![0; num_lines as usize]; // SAFETY: gpiod_line_config is guaranteed to be valid here. - unsafe { gpiod::gpiod_line_config_get_configured_offsets(self.config, + let len = unsafe { gpiod::gpiod_line_config_get_configured_offsets(self.config, offsets.as_mut_ptr(), num_lines) }; - for offset in offsets { + for offset in &offsets[0..len as usize] { // SAFETY: `gpiod_line_config` is guaranteed to be valid here. let settings = unsafe { gpiod::gpiod_line_config_get_line_settings(self.config, - offset) }; + *offset) }; if settings.is_null() { return Err(Error::OperationFailed( OperationType::LineConfigGetSettings,
@@ -121,7 +121,7 @@ impl Config { )); } - map.insert (offset,Settings ::new_with_settings (settings)); + map.insert (*offset,Settings ::new_with_settings (settings)); }Ok (map)
Signature · 1 line
viresh
Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
In reply to Viresh KumarHi Bartosz,
I gave few comments here, and it looks like you missed them ?
Quoted 17 lines · Viresh Kumar
For Rust changes, please run these as well to find any formatting issues, warnings:
cargo fmt --all -- --check
cargo clippy --release --workspace --bins --examples --tests --benches --all-features --all-targets -- -D warnings -Dclippy::undocumented_unsafe_blocksOn 13-01-23, 22:51, Bartosz Golaszewski wrote:
Just like ValueMap, maybe we can add following in
lib.rsfor this:pub type
line::SettingsMap= IntMap<line::Settings>;bindings/rust/libgpiod/src/line_config.rs impl Config { · line 83
Quoted 2 lines
bindings/rust/libgpiod/src/line_config.rs +2
-0+let mut map:HashMap <Offset ,Settings > =HashMap ::new (); +let num_lines =unsafe { gpiod::gpiod_line_config_get_num_configured_offsets (self.config) };This needs a SAFETY comment. Should we check if this returned 0 ?
Signature · 1 line
viresh
Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
Quoted 18 lines · Viresh Kumar
Hi Bartosz,
I gave few comments here, and it looks like you missed them ?
Quoted 12 lines · Viresh Kumar
For Rust changes, please run these as well to find any formatting issues, warnings:
cargo fmt --all -- --check
cargo clippy --release --workspace --bins --examples --tests --benches --all-features --all-targets -- -D warnings -Dclippy::undocumented_unsafe_blocksOn 13-01-23, 22:51, Bartosz Golaszewski wrote:
Just like ValueMap, maybe we can add following in
lib.rsfor this:pub type
line::SettingsMap= IntMap<line::Settings>;
Quoted 7 lines
Quoted 5 lines
bindings/rust/libgpiod/src/line_config.rs impl Config { · line 83
Quoted 2 lines
bindings/rust/libgpiod/src/line_config.rs +2
-0+let mut map:HashMap <Offset ,Settings > =HashMap ::new (); +let num_lines =unsafe { gpiod::gpiod_line_config_get_num_configured_offsets (self.config) };This needs a SAFETY comment. Should we check if this returned 0 ?
--
viresh
It seems like I did... :(
I will resend this patch separately for less noise after addressing them.
Bart
Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
In reply to Viresh KumarQuoted 7 lines · Viresh Kumar
For Rust changes, please run these as well to find any formatting issues, warnings:
cargo fmt --all -- --check
cargo clippy --release --workspace --bins --examples --tests --benches --all-features --all-targets -- -D warnings -Dclippy::undocumented_unsafe_blocks
FYI The first command introduces changes to unsafe blocks' coding style that the second one interprets as unsafe block with no SAFETY comments. :)
Bart
Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
Quoted 5 lines · Bartosz Golaszewski
Quoted 4 lines · Viresh Kumar
For Rust changes, please run these as well to find any formatting issues, warnings:
Quoted 7 lines
Quoted 3 lines
cargo fmt --all -- --check
cargo clippy --release --workspace --bins --examples --tests --benches --all-features --all-targets -- -D warnings -Dclippy::undocumented_unsafe_blocksFYI The first command introduces changes to unsafe blocks' coding style that the second one interprets as unsafe block with no SAFETY comments. :)
Heh, I also stumbled upon that (bug) earlier. Basically it doesn't recognize this format somehow:
/// SAFETY: ***
let ret =
unsafe { -block- };Just ignore that for now.
Signature · 1 line
viresh
PATCH 5/16 doc: update docs for libgpiod v2
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Update docs in gpiod.h wherever they're outdated or make incorrect statements. While at it: fix formatting in some places.
--- include/gpiod.h | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
include/gpiod.h +14 -9
@@ -22,8 +22,9 @@ extern "C" {* This is the complete documentation of the public API made available to * users of libgpiod. * -* <p>The API is logically split into several parts such as: GPIO chip & line -* operators, GPIO events handling etc. +* <p>The API is logically split into several sections. For each opaque data +* class, there's a set of functions for manipulating it. Together they can be +* thought of as objects and their methods in OOP parlance. * * <p>General note on error handling: all functions exported by libgpiod that * can fail, set errno to one of the error values defined in errno.h upon
@@ -34,10 +35,13 @@ extern "C" {* codes for every function as they propagate errors from the underlying libc * functions. * -* <p>In general libgpiod functions are not NULL-aware and it's expected that -* users pass valid pointers to objects as arguments. An exception to this rule -* are the functions that free/close/release resources - which work when passed -* a NULL-pointer as argument. Other exceptions are documented. +* <p>In general libgpiod functions are NULL-aware. For functions that are +* logically methods of data classes - ones that take a pointer to the object +* of that class as the first argument - passing a NULL pointer will result in +* the program aborting the execution. For non-methods, init functions and +* methods that take a pointer as any of the subsequent arguments, the handling +* of a NULL-pointer depends on the implementation and may range from gracefully +* handling it, ignoring it or returning an error. */ struct gpiod_chip ;
@@ -260,7 +264,8 @@ enum gpiod_line_direction {GPIOD_LINE_DIRECTION_AS_IS =1 ,/**< Request the line(s), but don't change direction. */ GPIOD_LINE_DIRECTION_INPUT , -/**< Direction is input - for reading the value of an externally driven GPIO line. */ +/**< Direction is input - for reading the value of an externally driven +* GPIO line. */ GPIOD_LINE_DIRECTION_OUTPUT, /**< Direction is output - for driving the GPIO line. */ };
@@ -926,8 +931,8 @@ gpiod_line_request_get_value(struct gpiod_line_request *request,* @param offsets Array of offsets identifying the subset of requested lines * from which to read values. * @param values Array in which the values will be stored. Must be sized -* to hold \p num_values entries. Each value is associated with the -* line identified by the corresponding entry in \p offsets. +* to hold \p num_values entries. Each value is associated with +* the line identified by the corresponding entry in \p offsets. * @return 0 on success, -1 on failure. */ int gpiod_line_request_get_values_subset(struct gpiod_line_request *request,
Signature · 1 line
2.37.2
PATCH 6/16 bindings: cxx: prepend all C symbols with the scope resolution operator
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
We explicitly resolve all global C symbols from libgpiod to the top-level namespace. Fix it wherever its missing (mostly for C enum types).
--- bindings/cxx/line-request.cpp | 4 +-- bindings/cxx/line-settings.cpp | 52 ++++++++++++++++++---------------- 2 files changed, 30 insertions(+), 26 deletions(-)
bindings/cxx/line-request.cpp +2 -2
@@ -118,7 +118,7 @@ GPIOD_CXX_API void line_request::get_values(const line::offsets& offsets, line:: int ret = ::gpiod_line_request_get_values_subset( this->_m_priv->request.get(), offsets.size(), this->_m_priv->offset_buf.data(), - reinterpret_cast<gpiod_line_value*>(values.data())); + reinterpret_cast<::gpiod_line_value*>(values.data())); if (ret) throw_from_errno("unable to retrieve line values"); }
@@ -161,7 +161,7 @@ GPIOD_CXX_API line_request& line_request::set_values(const line::offsets& offset int ret = ::gpiod_line_request_set_values_subset( this->_m_priv->request.get(), offsets.size(), this->_m_priv->offset_buf.data(), - reinterpret_cast<const enum gpiod_line_value*>(values.data())); + reinterpret_cast<const ::gpiod_line_value*>(values.data())); if (ret) throw_from_errno("unable to set line values");
bindings/cxx/line-settings.cpp +28 -24
168 lines not on this page · raw message
Signature · 1 line
2.37.2
PATCH 7/16 bindings: cxx: allow to copy line_settings
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Implement the copy operator for line_settings. We have a copy() method for line settings in C API while in C++ it's useful to copy line_settings returned in an std::map from line_config.get_line_settings().
--- bindings/cxx/gpiodcxx/line-settings.hpp | 13 ++++++- bindings/cxx/internal.hpp | 2 +- bindings/cxx/line-settings.cpp | 28 ++++++++++++++ bindings/cxx/tests/tests-line-settings.cpp | 43 ++++++++++++++++++++++ 4 files changed, 83 insertions(+), 3 deletions(-)
bindings/cxx/gpiodcxx/line-settings.hpp +11 -2
@@ -38,7 +38,11 @@ public: */ line_settings(); - line_settings(const line_settings& other) = delete; + /** + * @brief Copy constructor. + * @param other Object to copy. + */ + line_settings(const line_settings& other); /** * @brief Move constructor.
@@ -48,7 +52,12 @@ public: ~line_settings(); - line_settings& operator=(const line_settings& other) = delete; + /** + * @brief Copy assignment operator. + * @param other Object to copy. + * @return Reference to self. + */ + line_settings& operator=(const line_settings& other); /** * @brief Move assignment operator.
bindings/cxx/internal.hpp +1 -1
@@ -120,7 +120,7 @@ struct info_event::impl struct line_settings::impl { impl(); - impl(const impl& other) = delete; + impl(const impl& other); impl(impl&& other) = delete; impl& operator=(const impl& other) = delete; impl& operator=(impl&& other) = delete;
bindings/cxx/line-settings.cpp +28 -0
@@ -86,6 +86,15 @@ line_settings_ptr make_line_settings() return settings; } +line_settings_ptr copy_line_settings(const line_settings_ptr& ptr) +{ + line_settings_ptr settings(::gpiod_line_settings_copy(ptr.get())); + if (!settings) + throw_from_errno("Unable to copy the line settings object"); + + return settings; +} + template<class key_type, class value_type, class exception_type> value_type map_setting(const key_type& key, const ::std::map<key_type, value_type>& mapping) {
@@ -136,12 +145,24 @@ line_settings::impl::impl() } +line_settings::impl::impl(const impl& other) + : settings(copy_line_settings(other.settings)) +{ + +} + GPIOD_CXX_API line_settings::line_settings() : _m_priv(new impl) { } +GPIOD_CXX_API line_settings::line_settings(const line_settings& other) + : _m_priv(new impl(*other._m_priv)) +{ + +} + GPIOD_CXX_API line_settings::line_settings(line_settings&& other) noexcept : _m_priv(::std::move(other._m_priv)) {
@@ -153,6 +174,13 @@ GPIOD_CXX_API line_settings::~line_settings() } +GPIOD_CXX_API line_settings& line_settings::operator=(const line_settings& other) +{ + this->_m_priv.reset(new impl(*other._m_priv)); + + return *this; +} + GPIOD_CXX_API line_settings& line_settings::operator=(line_settings&& other) { this->_m_priv = ::std::move(other._m_priv);
bindings/cxx/tests/tests-line-settings.cpp +43 -0
49 lines not on this page · raw message
Signature · 1 line
2.37.2
PATCH 8/16 tests: fix the line config reset test case
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
We're using testing wrong variables in the reset_config test case. We should be checking the retrieved0 settings which are read back from the line config object.
--- tests/tests-line-config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
tests/tests-line-config.c +2 -2
@@ -175,9 +175,9 @@ GPIOD_TEST_CASE(reset_config)g_assert_nonnull (retrieved0);gpiod_test_return_if_failed (); - g_assert_cmpint(gpiod_line_settings_get_direction(settings), ==, + g_assert_cmpint(gpiod_line_settings_get_direction(retrieved0), ==,GPIOD_LINE_DIRECTION_INPUT ); - g_assert_cmpint(gpiod_line_settings_get_bias(settings), ==, + g_assert_cmpint(gpiod_line_settings_get_bias(retrieved0), ==,GPIOD_LINE_BIAS_PULL_DOWN );gpiod_line_config_reset (config);
Signature · 1 line
2.37.2
PATCH 9/16 tests: add a helper for reading back line settings from line config
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Add a helper for getting line settings from line config that allows to shrink the test code a bit.
--- tests/gpiod-test-helpers.h | 10 ++++++++++ tests/tests-line-config.c | 13 ++++--------- 2 files changed, 14 insertions(+), 9 deletions(-)
tests/gpiod-test-helpers.h +10 -0
@@ -126,6 +126,16 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(struct_gpiod_edge_event_buffer,gpiod_test_return_if_failed (); \ } while (0) +#define gpiod_test_line_config_get_line_settings_or_fail (_line_cfg, _offset) \ + ({ \ + struct gpiod_line_settings *_settings = \ + gpiod_line_config_get_line_settings(_line_cfg, \ + _offset); \ + g_assert_nonnull(_settings); \ + gpiod_test_return_if_failed(); \ + _settings; \ + }) +#define gpiod_test_create_request_config_or_fail () \ ({ \ struct gpiod_request_config *_config = \
tests/tests-line-config.c +4 -9
@@ -46,9 +46,7 @@ GPIOD_TEST_CASE(get_line_settings)gpiod_test_line_config_add_line_settings_or_fail (config, offsets,4 , settings); - retrieved =gpiod_line_config_get_line_settings (config,2 ); -g_assert_nonnull (retrieved); -gpiod_test_return_if_failed (); + retrieved =gpiod_test_line_config_get_line_settings_or_fail (config,2 ); g_assert_cmpint(gpiod_line_settings_get_direction(settings), ==,GPIOD_LINE_DIRECTION_INPUT );
@@ -146,9 +144,7 @@ GPIOD_TEST_CASE(null_settings)gpiod_test_line_config_add_line_settings_or_fail (config, offsets,4 ,NULL ); - settings =gpiod_line_config_get_line_settings (config,2 ); -g_assert_nonnull (settings); -gpiod_test_return_if_failed (); + settings =gpiod_test_line_config_get_line_settings_or_fail (config,2 ); g_assert_cmpint(gpiod_line_settings_get_drive(settings), ==,GPIOD_LINE_DIRECTION_AS_IS );
@@ -171,9 +167,8 @@ GPIOD_TEST_CASE(reset_config)gpiod_test_line_config_add_line_settings_or_fail (config, offsets,4 , settings); - retrieved0 =gpiod_line_config_get_line_settings (config,2 ); -g_assert_nonnull (retrieved0); -gpiod_test_return_if_failed (); + retrieved0 =gpiod_test_line_config_get_line_settings_or_fail (config, +2 ); g_assert_cmpint(gpiod_line_settings_get_direction(retrieved0), ==,GPIOD_LINE_DIRECTION_INPUT );
Signature · 1 line
2.37.2
PATCH 10/16 core: provide gpiod_line_config_set_output_values()
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Currently if user wants to use the same settings for a set of requested lines with the exception of the output value - they need to go through hoops by updating the line settings object and adding it one by one to the line config. Provide a helper function that allows to set a global list of output values that override the settings. For details on the
interface: see documentation in this commit.
--- include/gpiod.h | 27 +++++++++++++++ lib/line-config.c | 60 +++++++++++++++++++++++++++++++--- tests/gpiod-test-helpers.h | 10 ++++++ tests/tests-line-config.c | 67 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 160 insertions(+), 4 deletions(-)
include/gpiod.h +27 -0
@@ -785,6 +785,33 @@ struct gpiod_line_settings * gpiod_line_config_get_line_settings(struct gpiod_line_config *config, unsigned int offset); +/** +* @brief Set output values for a number of lines. +* @param config Line config object. +* @param values Buffer containing the output values. +* @param num_values Number of values in the buffer. +* @return 0 on success, -1 on error. +* +* This is a helper that allows users to set multiple (potentially different) +* output values at once while using the same line settings object. Instead of +* modifying the output value in the settings object and calling +* ::gpiod_line_config_add_line_settings multiple times, we can specify the +* settings, add them for a set of offsets and then call this function to +* set the output values. +* +* Values set by this function override whatever values were specified in the +* regular line settings. +* +* The order of the output values in the array corresponds with the order in +* which offsets were added by ::gpiod_line_config_add_line_settings. For +* example calling add_settings([1, 3]) and add_settings([2, 4]) and then +* calling this function with the following logicall values : [0, 1, 0, 1] +* will result in the following offset->value mapping: 1->0, 2->0, 3->1, 4->1. +*/ +int gpiod_line_config_set_output_values (struct gpiod_line_config *config, +const enum gpiod_line_value *values, +size_t num_values); +/** * @brief Get the number of configured line offsets. * @param config Line config object.
lib/line-config.c +56 -4
99 lines not on this page · raw message
tests/gpiod-test-helpers.h +10 -0
@@ -136,6 +136,16 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(struct_gpiod_edge_event_buffer, _settings; \ }) +#define gpiod_test_line_config_set_output_values_or_fail (_line_cfg, _values, \ + _num_values) \ + do { \ + gint _ret = gpiod_line_config_set_output_values(_line_cfg, \ + _values, \ + _num_values); \ + g_assert_cmpint(_ret, ==, 0); \ + gpiod_test_return_if_failed(); \ + } while (0) +#define gpiod_test_create_request_config_or_fail () \ ({ \ struct gpiod_request_config *_config = \
tests/tests-line-config.c +67 -0
70 lines not on this page · raw message
Signature · 1 line
2.37.2
Re: [libgpiod][PATCH 10/16] core: provide gpiod_line_config_set_output_values()
Quoted 29 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Currently if user wants to use the same settings for a set of requested lines with the exception of the output value - they need to go through hoops by updating the line settings object and adding it one by one to the line config. Provide a helper function that allows to set a global list of output values that override the settings. For details on the
interface: see documentation in this commit.Signed-off-by Bartosz Golaszewski <[email protected]>--- include/gpiod.h | 27 +++++++++++++++ lib/line-config.c | 60 +++++++++++++++++++++++++++++++--- tests/gpiod-test-helpers.h | 10 ++++++ tests/tests-line-config.c | 67 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 160 insertions(+), 4 deletions(-)include/gpiod.h +4
-0@@ -785,6 +785,33 @@ struct gpiod_line_settings * gpiod_line_config_get_line_settings(struct gpiod_line_config *config, unsigned int offset); +/** +* @brief Set output values for a number of lines. +* @param config Line config object. +* @param values Buffer containing the output values.
include/gpiod.h struct gpiod_line_settings * · line 801
Quoted 10 lines
include/gpiod.h +10
-0+* @param num_values Number of values in the buffer. +* @return 0 on success, -1 on error. +* +* This is a helper that allows users to set multiple (potentially different) +* output values at once while using the same line settings object. Instead of +* modifying the output value in the settings object and calling +* ::gpiod_line_config_add_line_settings multiple times, we can specify the +* settings, add them for a set of offsets and then call this function to +* set the output values. +*
A helper such as this did cross my mind when updating gpioset, but I didn't consider it worth the effort then, and still don't.
So the user has to set ALL output values, or more correctly the first num_values, right? i.e. it is the block form.
This helper is only helpful if the user already has ALL the values in the required array format, as is the case for gpioset, else they need to build the array to pass - in which case they may as well call gpiod_line_config_add_line_settings() for each line.
So is it really that helpful?
The sparse form would be more generally useful, particularly in the bindings. i.e. they should accept a mapping from offsets to values rather than the ordered list. Though, again, those may be best implemented with multiple calls to ::gpiod_line_config_add_line_settings rather than jumping through the hoops of constructing the parameters for another helper like this one.
include/gpiod.h struct gpiod_line_settings * · line 814
Quoted 13 lines
include/gpiod.h +13
-0+* Values set by this function override whatever values were specified in the +* regular line settings. +* +* The order of the output values in the array corresponds with the order in +* which offsets were added by ::gpiod_line_config_add_line_settings. For +* example calling add_settings([1, 3]) and add_settings([2, 4]) and then +* calling this function with the following logicall values : [0, 1, 0, 1] +* will result in the following offset->value mapping: 1->0, 2->0, 3->1, 4->1. +*/ +int gpiod_line_config_set_output_values (struct gpiod_line_config *config, +const enum gpiod_line_value *values, +size_t num_values); +
But if you do keep it...
Use documentation from gpiod_line_request_set_values(), suitably modified, to describe ordering - it is clearer and does not imply that the user has to independently know the order lines were added - it is in the order provided by gpiod_line_config_get_offsets().
Cheers,
Kent.
Re: [libgpiod][PATCH 10/16] core: provide gpiod_line_config_set_output_values()
Quoted 45 lines · Kent Gibson
Quoted 29 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Currently if user wants to use the same settings for a set of requested lines with the exception of the output value - they need to go through hoops by updating the line settings object and adding it one by one to the line config. Provide a helper function that allows to set a global list of output values that override the settings. For details on the
interface: see documentation in this commit.Signed-off-by Bartosz Golaszewski <[email protected]>--- include/gpiod.h | 27 +++++++++++++++ lib/line-config.c | 60 +++++++++++++++++++++++++++++++--- tests/gpiod-test-helpers.h | 10 ++++++ tests/tests-line-config.c | 67 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 160 insertions(+), 4 deletions(-)include/gpiod.h +4
-0@@ -785,6 +785,33 @@ struct gpiod_line_settings * gpiod_line_config_get_line_settings(struct gpiod_line_config *config, unsigned int offset); +/** +* @brief Set output values for a number of lines. +* @param config Line config object. +* @param values Buffer containing the output values. include/gpiod.h struct gpiod_line_settings * · line 801
Quoted 10 lines
include/gpiod.h +10
-0+* @param num_values Number of values in the buffer. +* @return 0 on success, -1 on error. +* +* This is a helper that allows users to set multiple (potentially different) +* output values at once while using the same line settings object. Instead of +* modifying the output value in the settings object and calling +* ::gpiod_line_config_add_line_settings multiple times, we can specify the +* settings, add them for a set of offsets and then call this function to +* set the output values. +* A helper such as this did cross my mind when updating gpioset, but I didn't consider it worth the effort then, and still don't.
Quoted 8 lines
So the user has to set ALL output values, or more correctly the first
num_values, right? i.e. it is the block form.This helper is only helpful if the user already has ALL the values in the required array format, as is the case for gpioset, else they need to build the array to pass - in which case they may as well call
gpiod_line_config_add_line_settings()for each line.
So is it really that helpful?
It seems to me that building/adding a separate settings object for every line is quite cumbersome (I know that C is cumbersome in itself but still...). IMO more so than just storing values in an array. And in many cases the user may already know the right set of values in which case it boils down to static const enum gpiod_line_value vals[] = { ... };
This is also pretty much the equivalent of the gpiod_line_request_set_values() function just run at request-time.
Quoted 8 lines
The sparse form would be more generally useful, particularly in the bindings. i.e. they should accept a mapping from offsets to values rather than the ordered list. Though, again, those may be best implemented with multiple calls to ::gpiod_line_config_add_line_settings rather than jumping through the hoops of constructing the parameters for another helper like this one.
I'd argue that this should be implemented in bindings using the new function.
Quoted 16 lines
include/gpiod.h struct gpiod_line_settings * · line 814
Quoted 13 lines
include/gpiod.h +13
-0+* Values set by this function override whatever values were specified in the +* regular line settings. +* +* The order of the output values in the array corresponds with the order in +* which offsets were added by ::gpiod_line_config_add_line_settings. For +* example calling add_settings([1, 3]) and add_settings([2, 4]) and then +* calling this function with the following logicall values : [0, 1, 0, 1] +* will result in the following offset->value mapping: 1->0, 2->0, 3->1, 4->1. +*/ +int gpiod_line_config_set_output_values (struct gpiod_line_config *config, +const enum gpiod_line_value *values, +size_t num_values); +But if you do keep it...
Quoted 5 lines
Use documentation from
gpiod_line_request_set_values(), suitably modified, to describe ordering - it is clearer and does not imply that the user has to independently know the order lines were added - it is in the order provided bygpiod_line_config_get_offsets().
I'd like to have some version of that. Maybe keep what I proposed for C but have something more elaborate (with mappings?) for bindings? I need to think about it tomorrow.
Bart
PATCH 11/16 gpioset: use gpiod_line_config_set_output_values()
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Use the new line config function to shrink the gpioset code and drop one for loop.
--- tools/gpioset.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
tools/gpioset.c +11 -10
@@ -870,9 +870,9 @@ int main(int argc, char **argv)struct gpiod_line_config *line_cfg;struct line_resolver *resolver;enum gpiod_line_value *values; -int i, j, num_lines, ret;struct gpiod_chip *chip;unsigned int *offsets; +int i, num_lines, ret;struct config cfg;char **lines;
@@ -933,15 +933,16 @@ int main(int argc, char **argv) values);gpiod_line_config_reset (line_cfg); -for (j =0 ; j < num_lines; j++) { -gpiod_line_settings_set_output_value (settings, - values[j]); - - ret =gpiod_line_config_add_line_settings ( - line_cfg, &offsets[j],1 , settings); -if (ret) -die_perror ("unable to add line settings" ); - } + + ret =gpiod_line_config_add_line_settings (line_cfg, offsets, + num_lines, settings); +if (ret) +die_perror ("unable to add line settings" ); + + ret =gpiod_line_config_set_output_values (line_cfg, + values, num_lines); +if (ret) +die_perror ("unable to set output values" ); chip =gpiod_chip_open (resolver->chips[i].path);if (!chip)
Signature · 1 line
2.37.2
PATCH 12/16 bindings: cxx: add line_config.set_output_values()
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Extend line_config to expose a new method - set_output_values() - which wraps the new C function for setting multiple output values at once.
--- bindings/cxx/gpiodcxx/line-config.hpp | 7 ++++ bindings/cxx/internal.hpp | 1 + bindings/cxx/line-config.cpp | 15 +++++++ bindings/cxx/line-settings.cpp | 5 +++ bindings/cxx/tests/tests-line-config.cpp | 51 ++++++++++++++++++++++++ 5 files changed, 79 insertions(+)
bindings/cxx/gpiodcxx/line-config.hpp +7 -0
@@ -76,6 +76,13 @@ public: */ line_config& add_line_settings(const line::offsets& offsets, const line_settings& settings); + /** + * @brief Set output values for a number of lines. + * @param values Buffer containing the output values. + * @return Reference to self. + */ + line_config& set_output_values(const line::values& values); + /** * @brief Get a mapping of offsets to line settings stored by this * object.
bindings/cxx/internal.hpp +1 -0
@@ -31,6 +31,7 @@ map_enum_c_to_cxx(c_enum_type value, const ::std::map<c_enum_type, cxx_enum_type } void throw_from_errno(const ::std::string& what); +::gpiod_line_value map_output_value(line::value value); template<class T, void F(T*)> struct deleter {
bindings/cxx/line-config.cpp +15 -0
@@ -100,6 +100,21 @@ GPIOD_CXX_API line_config& line_config::add_line_settings(const line::offsets& o return *this; } +GPIOD_CXX_API line_config& line_config::set_output_values(const line::values& values) +{ + ::std::vector<::gpiod_line_value> mapped_values(values.size()); + + for (unsigned int i = 0; i < values.size(); i++) + mapped_values[i] = map_output_value(values[i]); + + auto ret = ::gpiod_line_config_set_output_values(this->_m_priv->config.get(), + mapped_values.data(), mapped_values.size()); + if (ret) + throw_from_errno("unable to set output values"); + + return *this; +} + GPIOD_CXX_API ::std::map<line::offset, line_settings> line_config::get_line_settings() const { ::std::size_t num_offsets = ::gpiod_line_config_get_num_configured_offsets(
bindings/cxx/line-settings.cpp +5 -0
@@ -139,6 +139,11 @@ cxx_enum_type get_mapped_value(::gpiod_line_settings* settings, } /* namespace */ +::gpiod_line_value map_output_value(line::value value) +{ + return do_map_value(value, value_mapping); +} + line_settings::impl::impl() : settings(make_line_settings()) {
bindings/cxx/tests/tests-line-config.cpp +51 -0
69 lines not on this page · raw message
Signature · 1 line
2.37.2
Re: [libgpiod][PATCH 12/16] bindings: cxx: add line_config.set_output_values()
Quoted 4 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Extend
line_configto expose a new method -set_output_values()- which wraps the new C function for setting multiple output values at once.
...
Side Q: Does documentation describe the order in which lines are being set? Or is it solely specified by a kernel driver for a hardware?
(I can imagine that this may be not so trivial as long as the input parameters,
i.e. line offsets, are not sorted and hardware supports full bank atomic write, this may have a lot of interesting side effects.)
Signature · 2 lines
With Best Regards, Andy Shevchenko
PATCH 13/16 bindings: python: provide line_config.set_output_values()
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Add a new argument to Chip.request_lines() that allows the user to pass a list of output values for configured lines instead of using several LineSettings objects between which the only difference is the output value.
--- bindings/python/gpiod/chip.py | 6 ++ bindings/python/gpiod/ext/line-config.c | 64 +++++++++++++++++++++ bindings/python/tests/tests_line_request.py | 14 +++++ 3 files changed, 84 insertions(+)
bindings/python/gpiod/chip.py +6 -0
@@ -6,10 +6,12 @@ from .chip_info import ChipInfo from .exception import ChipClosedError from .info_event import InfoEvent from .internal import poll_fd +from .line import Value from .line_info import LineInfo from .line_settings import LineSettings, _line_settings_to_ext from .line_request import LineRequest from collections import Counter +from collections.abc import Iterable from datetime import timedelta from errno import ENOENT from select import select
@@ -221,6 +223,7 @@ class Chip: config: dict[tuple[Union[int, str]], Optional[LineSettings]], consumer: Optional[str] = None, event_buffer_size: Optional[int] = None, + output_values: Optional[Iterable[Value]] = None, ) -> LineRequest: """ Request a set of lines for exclusive usage.
@@ -279,6 +282,9 @@ class Chip: offsets, _line_settings_to_ext(settings or LineSettings()) ) + if output_values: + line_cfg.set_output_values(output_values) + req_internal = self._chip.request_lines(line_cfg, consumer, event_buffer_size) request = LineRequest(req_internal)
bindings/python/gpiod/ext/line-config.c +64 -0
76 lines not on this page · raw message
bindings/python/tests/tests_line_request.py +14 -0
@@ -402,6 +402,20 @@ class LineRequestConsumerString(TestCase): self.assertEqual(info.consumer, "?") +class LineRequestSetOutputValues(TestCase): + def test_request_with_globally_set_output_values(self): + sim = gpiosim.Chip(num_lines=4) + with gpiod.request_lines( + sim.dev_path, + config={(0, 1, 2, 3): gpiod.LineSettings(direction=Direction.OUTPUT)}, + output_values=(Value.ACTIVE, Value.INACTIVE, Value.ACTIVE, Value.INACTIVE), + ) as request: + self.assertEqual(sim.get_value(0), SimVal.ACTIVE) + self.assertEqual(sim.get_value(1), SimVal.INACTIVE) + self.assertEqual(sim.get_value(2), SimVal.ACTIVE) + self.assertEqual(sim.get_value(3), SimVal.INACTIVE) + + class ReconfigureRequestedLines(TestCase): def setUp(self): self.sim = gpiosim.Chip(num_lines=8, line_names={3: "foo", 4: "bar", 6: "baz"})
Signature · 1 line
2.37.2
PATCH 14/16 bindings: rust: make request_config optional in Chip.request_lines()
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Request config is not necessary to request lines. In C API we accept a NULL pointer, in C++ it's not necessary to assign a request_config to the request builder, in Python the consumer and event buffer size arguments are optional. Let's make rust bindings consistent and not require the request config to be always present. Convert the argument in request_lines to Option and update the rest of the code.
--- bindings/rust/libgpiod/examples/gpio_events.rs | 2 +- .../libgpiod/examples/gpio_threaded_info_events.rs | 2 +- bindings/rust/libgpiod/examples/gpioget.rs | 2 +- bindings/rust/libgpiod/examples/gpiomon.rs | 2 +- bindings/rust/libgpiod/examples/gpioset.rs | 2 +- bindings/rust/libgpiod/src/chip.rs | 10 ++++++++-- bindings/rust/libgpiod/tests/common/config.rs | 2 +- bindings/rust/libgpiod/tests/info_event.rs | 2 +- 8 files changed, 15 insertions(+), 9 deletions(-)
bindings/rust/libgpiod/examples/gpio_events.rs +1 -1
@@ -42,7 +42,7 @@ fn main() -> Result<()> {let rconfig = request::Config ::new ()?;let mut buffer = request::Buffer ::new (1 )?; -let request = chip.request_lines (&rconfig, &lconfig)?; +let request = chip.request_lines (Some (&rconfig), &lconfig)?;loop {match request.wait_edge_events (None ) {
bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs +1 -1
@@ -40,7 +40,7 @@ fn request_reconfigure_line(let request = chip .lock () .unwrap () - .request_lines (&rconfig, &lconfig) + .request_lines (Some (&rconfig), &lconfig) .unwrap ();// Signal the parent to continue
bindings/rust/libgpiod/examples/gpioget.rs +1 -1
@@ -37,7 +37,7 @@ fn main() -> Result<()> {let rconfig = request::Config ::new ()?; rconfig.set_consumer (&args[0 ])?; -let request = chip.request_lines (&rconfig, &lconfig)?; +let request = chip.request_lines (Some (&rconfig), &lconfig)?;let map = request.values ()?;println! ("{:?}" , map);
bindings/rust/libgpiod/examples/gpiomon.rs +1 -1
@@ -41,7 +41,7 @@ fn main() -> Result<()> {let rconfig = request::Config ::new ()?;let mut buffer = request::Buffer ::new (1 )?; -let request = chip.request_lines (&rconfig, &lconfig)?; +let request = chip.request_lines (Some (&rconfig), &lconfig)?;loop {match request.wait_edge_events (None ) {
bindings/rust/libgpiod/examples/gpioset.rs +1 -1
@@ -54,7 +54,7 @@ fn main() -> Result<()> {let rconfig = request::Config ::new ()?; rconfig.set_consumer (&args[0 ])?; - chip.request_lines (&rconfig, &lconfig)?; + chip.request_lines (Some (&rconfig), &lconfig)?;// Wait for keypress, let user verify line status. stdin ().read_exact (&mut [0u8 ]).unwrap ();
bindings/rust/libgpiod/src/chip.rs +8 -2
@@ -11,6 +11,7 @@ use std::cmp::Ordering;use std::ffi::{CStr ,CString };use std::os::{raw::c_char, unix::prelude::AsRawFd };use std::path::Path ; +use std::ptr;use std::str;use std::sync::Arc ;use std::time::Duration ;
@@ -195,13 +196,18 @@ impl Chip {/// Request a set of lines for exclusive usage. pub fn request_lines ( &self, - rconfig: &request::Config , + rconfig:Option <&request::Config >, lconfig: &line::Config , ) ->Result <request::Request > { +let req_cfg =match rconfig { +Some (cfg) => cfg.config, + _ => ptr::null (), + }as *mut gpiod::gpiod_request_config ; +// SAFETY: The `gpiod_line_request` returned by libgpiod is guaranteed to live as long // as the `struct Request`. let request =unsafe { - gpiod::gpiod_chip_request_lines (self.ichip.chip, rconfig.config, lconfig.config) + gpiod::gpiod_chip_request_lines (self.ichip.chip, req_cfg, lconfig.config) };if request.is_null () {
bindings/rust/libgpiod/tests/common/config.rs +1 -1
@@ -106,7 +106,7 @@ impl TestConfig {pub (crate )fn request_lines (&mut self) ->Result <()> {let chip =Chip ::open (&self.sim.lock ().unwrap ().dev_path ())?; - self.request =Some (chip.request_lines (&self.rconfig, &self.lconfig)?); + self.request =Some (chip.request_lines (Some (&self.rconfig), &self.lconfig)?); self.chip =Some (chip);Ok (())
bindings/rust/libgpiod/tests/info_event.rs +1 -1
@@ -32,7 +32,7 @@ mod info_event {let request = chip .lock () .unwrap () - .request_lines (&rconfig, &lconfig1) + .request_lines (Some (&rconfig), &lconfig1) .unwrap ();// Signal the parent to continue
Signature · 1 line
2.37.2
Re: [libgpiod][PATCH 14/16] bindings: rust: make request_config optional in Chip.request_lines()
Quoted 10 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Request config is not necessary to request lines. In C API we accept a NULL pointer, in C++ it's not necessary to assign a
request_configto the request builder, in Python the consumer and event buffer size arguments are optional. Let's make rust bindings consistent and not require the request config to be always present. Convert the argument inrequest_linesto Option and update the rest of the code.Signed-off-by Bartosz Golaszewski <[email protected]>
Quoted 10 lines
--- bindings/rust/libgpiod/examples/gpio_events.rs | 2 +- .../libgpiod/examples/gpio_threaded_info_events.rs | 2 +- bindings/rust/libgpiod/examples/gpioget.rs | 2 +- bindings/rust/libgpiod/examples/gpiomon.rs | 2 +- bindings/rust/libgpiod/examples/gpioset.rs | 2 +- bindings/rust/libgpiod/src/chip.rs | 10 ++++++++-- bindings/rust/libgpiod/tests/common/config.rs | 2 +- bindings/rust/libgpiod/tests/info_event.rs | 2 +- 8 files changed, 15 insertions(+), 9 deletions(-)
Signature · 1 line
viresh
PATCH 15/16 bindings: rust: make mutators return &mut self
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
In C++ bindings we can chain the mutators as they all return a reference to the object they modify. It's a common practice to allow that in Rust too so make all mutators that don't already do it return a mutable reference to self.
It's also logically incorrect to make mutators borrow an immutable reference to self. Even if that builds - as we're fiddling with C pointers - it could change in the future. It's fine for getters but setters should all use mutable references.
--- .../rust/libgpiod/examples/gpio_events.rs | 2 +- .../examples/gpio_threaded_info_events.rs | 6 +- bindings/rust/libgpiod/examples/gpioget.rs | 4 +- bindings/rust/libgpiod/examples/gpiomon.rs | 2 +- bindings/rust/libgpiod/examples/gpioset.rs | 4 +- bindings/rust/libgpiod/src/line_config.rs | 4 +- bindings/rust/libgpiod/src/line_request.rs | 16 +-- bindings/rust/libgpiod/src/request_config.rs | 8 +- bindings/rust/libgpiod/tests/common/config.rs | 8 +- bindings/rust/libgpiod/tests/info_event.rs | 6 +- bindings/rust/libgpiod/tests/line_config.rs | 2 +- bindings/rust/libgpiod/tests/line_request.rs | 99 ++++++++----------- .../rust/libgpiod/tests/request_config.rs | 2 +- 13 files changed, 74 insertions(+), 89 deletions(-)
bindings/rust/libgpiod/examples/gpio_events.rs +1 -1
@@ -25,7 +25,7 @@ fn main() -> Result<()> { }let mut lsettings = line::Settings ::new ()?; -let lconfig = line::Config ::new ()?; +let mut lconfig = line::Config ::new ()?;let mut offsets =Vec ::<Offset >::new ();for argin &args[2 ..] {
bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs +3 -3
@@ -32,12 +32,12 @@ fn request_reconfigure_line( rx: Receiver<()>, ) { thread::spawn (move || { -let lconfig = line::Config ::new ().unwrap (); +let mut lconfig = line::Config ::new ().unwrap ();let lsettings = line::Settings ::new ().unwrap (); lconfig.add_line_settings (&[offset], lsettings).unwrap ();let rconfig = request::Config ::new ().unwrap (); -let request = chip +let mut request = chip .lock () .unwrap () .request_lines (Some (&rconfig), &lconfig)
@@ -49,7 +49,7 @@ fn request_reconfigure_line(// Wait for parent to signal rx.recv ().expect ("Could not receive from channel" ); -let lconfig = line::Config ::new ().unwrap (); +let mut lconfig = line::Config ::new ().unwrap ();let mut lsettings = line::Settings ::new ().unwrap (); lsettings.set_direction (Direction ::Output ).unwrap (); lconfig.add_line_settings (&[offset], lsettings).unwrap ();
bindings/rust/libgpiod/examples/gpioget.rs +2 -2
@@ -20,7 +20,7 @@ fn main() -> Result<()> { }let mut lsettings = line::Settings ::new ()?; -let lconfig = line::Config ::new ()?; +let mut lconfig = line::Config ::new ()?;let mut offsets =Vec ::<Offset >::new ();for argin &args[2 ..] {
@@ -34,7 +34,7 @@ fn main() -> Result<()> {let path =format! ("/dev/gpiochip{}" , args[1 ]);let chip =Chip ::open (&path)?; -let rconfig = request::Config ::new ()?; +let mut rconfig = request::Config ::new ()?; rconfig.set_consumer (&args[0 ])?;let request = chip.request_lines (Some (&rconfig), &lconfig)?;
bindings/rust/libgpiod/examples/gpiomon.rs +1 -1
@@ -24,7 +24,7 @@ fn main() -> Result<()> { }let mut lsettings = line::Settings ::new ()?; -let lconfig = line::Config ::new ()?; +let mut lconfig = line::Config ::new ()?;let mut offsets =Vec ::<Offset >::new ();for argin &args[2 ..] {
bindings/rust/libgpiod/examples/gpioset.rs +2 -2
@@ -24,7 +24,7 @@ fn main() -> Result<()> {return Err (Error ::InvalidArguments ); } -let lconfig = line::Config ::new ()?; +let mut lconfig = line::Config ::new ()?;for argin &args[2 ..] {let pair:Vec <&str > = arg.split ('=' ).collect ();
@@ -51,7 +51,7 @@ fn main() -> Result<()> {let path =format! ("/dev/gpiochip{}" , args[1 ]);let chip =Chip ::open (&path)?; -let rconfig = request::Config ::new ()?; +let mut rconfig = request::Config ::new ()?; rconfig.set_consumer (&args[0 ])?; chip.request_lines (Some (&rconfig), &lconfig)?;
bindings/rust/libgpiod/src/line_config.rs +2 -2
@@ -56,7 +56,7 @@ impl Config { }/// Add line settings for a set of offsets. -pub fn add_line_settings (&self, offsets: &[Offset ], settings:Settings ) ->Result <()> { +pub fn add_line_settings (&mut self, offsets: &[Offset ], settings:Settings ) ->Result <&mut Self > {// SAFETY: `gpiod_line_config` is guaranteed to be valid here. let ret =unsafe { gpiod::gpiod_line_config_add_line_settings(
@@ -73,7 +73,7 @@ impl Config { errno::errno(), )) }else { -Ok (()) +Ok (self) } }
bindings/rust/libgpiod/src/line_request.rs +8 -8
@@ -93,7 +93,7 @@ impl Request { }/// Set the value of a single line associated with the request. -pub fn set_value (&self, offset:Offset , value:Value ) ->Result <()> { +pub fn set_value (&mut self, offset:Offset , value:Value ) ->Result <&mut Self > {// SAFETY: `gpiod_line_request` is guaranteed to be valid here. let ret =unsafe { gpiod::gpiod_line_request_set_value (self.request, offset, value.value ()) };
@@ -104,12 +104,12 @@ impl Request { errno::errno(), )) }else { -Ok (()) +Ok (self) } }/// Get values of a subset of lines associated with the request. -pub fn set_values_subset (&self, map:ValueMap ) ->Result <()> { +pub fn set_values_subset (&mut self, map:ValueMap ) ->Result <&mut Self > {let mut offsets =Vec ::new ();let mut values =Vec ::new ();
@@ -134,12 +134,12 @@ impl Request { errno::errno(), )) }else { -Ok (()) +Ok (self) } }/// Get values of all lines associated with the request. -pub fn set_values (&self, values: &[Value ]) ->Result <()> { +pub fn set_values (&mut self, values: &[Value ]) ->Result <&mut Self > {if values.len () != self.num_lines ()as usize {return Err (Error ::InvalidArguments ); }
@@ -159,12 +159,12 @@ impl Request { errno::errno(), )) }else { -Ok (()) +Ok (self) } }/// Update the configuration of lines associated with the line request. -pub fn reconfigure_lines (&self, lconfig: &line::Config ) ->Result <()> { +pub fn reconfigure_lines (&mut self, lconfig: &line::Config ) ->Result <&mut Self > {// SAFETY: `gpiod_line_request` is guaranteed to be valid here. let ret =unsafe { gpiod::gpiod_line_request_reconfigure_lines (self.request, lconfig.config) };
@@ -175,7 +175,7 @@ impl Request { errno::errno(), )) }else { -Ok (()) +Ok (self) } }
bindings/rust/libgpiod/src/request_config.rs +5 -3
@@ -40,7 +40,7 @@ impl Config {/// /// If the consumer string is too long, it will be truncated to the max /// accepted length. -pub fn set_consumer (&self, consumer: &str ) ->Result <()> { +pub fn set_consumer (&mut self, consumer: &str ) ->Result <&mut Self > {let consumer =CString ::new (consumer).map_err (|_|Error ::InvalidString )?;// SAFETY: `gpiod_request_config` is guaranteed to be valid here.
@@ -51,7 +51,7 @@ impl Config { ) } -Ok (()) +Ok (self) }/// Get the consumer name configured in the request config.
@@ -73,9 +73,11 @@ impl Config { }/// Set the size of the kernel event buffer for the request. -pub fn set_event_buffer_size (&self, size:usize ) { +pub fn set_event_buffer_size (&mut self, size:usize ) -> &mut Self {// SAFETY: `gpiod_request_config` is guaranteed to be valid here. unsafe { gpiod::gpiod_request_config_set_event_buffer_size (self.config, sizeas c_ulong ) } + + self }/// Get the edge event buffer size setting for the request config.
bindings/rust/libgpiod/tests/common/config.rs +4 -4
@@ -43,7 +43,7 @@ impl TestConfig { } } -pub (crate )fn rconfig_set_consumer (&self, consumer: &str ) { +pub (crate )fn rconfig_set_consumer (&mut self, consumer: &str ) { self.rconfig.set_consumer (consumer).unwrap (); }
@@ -100,7 +100,7 @@ impl TestConfig {pub (crate )fn lconfig_add_settings (&mut self, offsets: &[Offset ]) { self.lconfig .add_line_settings (offsets, self.lsettings.take ().unwrap ()) - .unwrap () + .unwrap (); }pub (crate )fn request_lines (&mut self) ->Result <()> {
@@ -128,8 +128,8 @@ impl TestConfig { self.lsettings.as_mut ().unwrap () } -pub (crate )fn request (&self) -> &request::Request { - self.request.as_ref ().unwrap () +pub (crate )fn request (&mut self) -> &mut request::Request { + self.request.as_mut ().unwrap () } }
bindings/rust/libgpiod/tests/info_event.rs +3 -3
@@ -24,12 +24,12 @@ mod info_event {fn request_reconfigure_line (chip:Arc <Mutex <Chip >>, tx:Sender <()>, rx:Receiver <()>) { thread::spawn (move || { -let lconfig1 = line::Config ::new ().unwrap (); +let mut lconfig1 = line::Config ::new ().unwrap ();let lsettings = line::Settings ::new ().unwrap (); lconfig1.add_line_settings (&[7 ], lsettings).unwrap ();let rconfig = request::Config ::new ().unwrap (); -let request = chip +let mut request = chip .lock () .unwrap () .request_lines (Some (&rconfig), &lconfig1)
@@ -41,7 +41,7 @@ mod info_event {// Wait for parent to signal rx.recv ().expect ("Could not receive from channel" ); -let lconfig2 = line::Config ::new ().unwrap (); +let mut lconfig2 = line::Config ::new ().unwrap ();let mut lsettings = line::Settings ::new ().unwrap (); lsettings.set_direction (Direction ::Output ).unwrap (); lconfig2.add_line_settings (&[7 ], lsettings).unwrap ();
bindings/rust/libgpiod/tests/line_config.rs +1 -1
@@ -33,7 +33,7 @@ mod line_config { .unwrap();// Add settings for multiple lines -let lconfig = line::Config ::new ().unwrap (); +let mut lconfig = line::Config ::new ().unwrap (); lconfig.add_line_settings (&[0 ,1 ,2 ], lsettings1).unwrap (); lconfig.add_line_settings (&[4 ,5 ], lsettings2).unwrap ();
bindings/rust/libgpiod/tests/line_request.rs +41 -58
308 lines not on this page · raw message
bindings/rust/libgpiod/tests/request_config.rs +1 -1
@@ -27,7 +27,7 @@ mod request_config {#[test] fn initialized () {const CONSUMER : &str ="foobar" ; -let rconfig = request::Config ::new ().unwrap (); +let mut rconfig = request::Config ::new ().unwrap (); rconfig.set_consumer (CONSUMER ).unwrap (); rconfig.set_event_buffer_size (64 );
Signature · 1 line
2.37.2
Re: [libgpiod][PATCH 15/16] bindings: rust: make mutators return &mut self
bindings/rust/libgpiod/tests/line_request.rs mod line_request { · line 101
Quoted 10 lines · Bartosz Golaszewski
bindings/rust/libgpiod/tests/line_request.rs +0
-2@@ -97,13 +97,11 @@ mod line_request { config.lconfig_add_settings (&offsets); config.request_lines ().unwrap (); -let request = config.request (); -
Why remove this ? And open-code it ?
Signature · 1 line
viresh
Re: [libgpiod][PATCH 15/16] bindings: rust: make mutators return &mut self
Quoted 10 lines · Viresh Kumar
Quoted 8 lines · Bartosz Golaszewski
bindings/rust/libgpiod/tests/line_request.rs +0
-0@@ -97,13 +97,11 @@ mod line_request { config.lconfig_add_settings (&offsets); config.request_lines ().unwrap ();
Quoted 5 lines
bindings/rust/libgpiod/tests/line_request.rs mod line_request { · line 101
Quoted 2 lines
bindings/rust/libgpiod/tests/line_request.rs +0
-2-let request = config.request (); -Why remove this ? And open-code it ?
Because I'm a Rust noob and couldn't figure out how to deal with the mutable/non-mutable borrow vomit that ensued when I kept the local variable. If you could improve upon this one, it would be great!
Bart
Re: [libgpiod][PATCH 15/16] bindings: rust: make mutators return &mut self
Quoted 17 lines · Bartosz Golaszewski
Quoted 10 lines · Viresh Kumar
Quoted 8 lines · Bartosz Golaszewski
bindings/rust/libgpiod/tests/line_request.rs +0
-0@@ -97,13 +97,11 @@ mod line_request { config.lconfig_add_settings (&offsets); config.request_lines ().unwrap ();Quoted 5 lines
bindings/rust/libgpiod/tests/line_request.rs mod line_request { · line 101
Quoted 2 lines
bindings/rust/libgpiod/tests/line_request.rs +0
-2-let request = config.request (); -Why remove this ? And open-code it ?
Quoted 3 lines
Because I'm a Rust noob and couldn't figure out how to deal with the mutable/non-mutable borrow vomit that ensued when I kept the local variable. If you could improve upon this one, it would be great!
From what I can understand, config.request() returns a mutable reference and you don't need a mutable variable to keep it, since the variable doesn't need to store another reference later. Just moving back to original code should work here.
bindings/rust/libgpiod/tests/line_request.rs +7 -5
@@ -97,11 +97,13 @@ mod line_request { config.lconfig_add_settings (&offsets); config.request_lines ().unwrap (); +let request = config.request (); +// Single values read properly -assert_eq! (config.request ().value (7 ).unwrap (),Value ::Active ); +assert_eq! (request.value (7 ).unwrap (),Value ::Active );// Values read properly -let map = config.request ().values ().unwrap (); +let map = request.values ().unwrap ();for iin 0 ..offsets.len () {assert_eq! ( *map.get (offsets[i].into ()).unwrap (),
@@ -113,7 +115,7 @@ mod line_request { }// Subset of values read properly -let map = config.request ().values_subset (&[2 ,0 ,6 ]).unwrap (); +let map = request.values_subset (&[2 ,0 ,6 ]).unwrap ();assert_eq! (*map.get (2 ).unwrap (),Value ::InActive );assert_eq! (*map.get (0 ).unwrap (),Value ::InActive );assert_eq! (*map.get (6 ).unwrap (),Value ::Active );
@@ -123,8 +125,8 @@ mod line_request { lsettings.set_active_low (true );let mut lconfig = line::Config ::new ().unwrap (); lconfig.add_line_settings (&offsets, lsettings).unwrap (); - config.request ().reconfigure_lines (&lconfig).unwrap (); -assert_eq! (config.request ().value (7 ).unwrap (),Value ::InActive ); + request.reconfigure_lines (&lconfig).unwrap (); +assert_eq! (request.value (7 ).unwrap (),Value ::InActive ); }#[test]
Signature · 1 line
viresh
Re: [libgpiod][PATCH 15/16] bindings: rust: make mutators return &mut self
Quoted 19 lines · Viresh Kumar
Quoted 17 lines · Bartosz Golaszewski
Quoted 15 lines · Viresh Kumar
bindings/rust/libgpiod/tests/line_request.rs mod line_request { · line 101
Quoted 10 lines · Bartosz Golaszewski
bindings/rust/libgpiod/tests/line_request.rs +0
-2@@ -97,13 +97,11 @@ mod line_request { config.lconfig_add_settings (&offsets); config.request_lines ().unwrap (); -let request = config.request (); -Why remove this ? And open-code it ?
Quoted 9 lines
Quoted 3 lines
Because I'm a Rust noob and couldn't figure out how to deal with the mutable/non-mutable borrow vomit that ensued when I kept the local variable. If you could improve upon this one, it would be great!
From what I can understand,
config.request()returns a mutable reference and you don't need a mutable variable to keep it, since the variable doesn't need to store another reference later. Just moving back to original code should work here.
Seems like I went overboard with converting all of those cases but in same places this still fails:
error[E0502]: cannot borrow config as immutable because it is also borrowed as mutable
--> libgpiod/tests/line_request.rs:449:24
|
441 | let request = config.request();
| ---------------- mutable borrow occurs here...
449 | let info = config.chip().line_info(0).unwrap();
| ^^^^^^^^^^^^^ immutable borrow occurs here
...
456 | request.reconfigure_lines(&lconfig).unwrap();
| ----------------------------------- mutable borrowlater used here
If I make config.chip() return &mut Chip then it fails like that:
error[E0499]: cannot borrow config as mutable more than once at a time
--> libgpiod/tests/line_request.rs:449:24
|
441 | let request = config.request();
| ---------------- first mutable borrowoccurs here
...
449 | let info = config.chip().line_info(0).unwrap();
| ^^^^^^^^^^^^^ second mutable borrow occurs here
...
456 | request.reconfigure_lines(&lconfig).unwrap();
| ----------------------------------- first borrowlater used here
Not sure how to go about this.
Bart
Re: [libgpiod][PATCH 15/16] bindings: rust: make mutators return &mut self
Quoted 29 lines · Bartosz Golaszewski
Seems like I went overboard with converting all of those cases but in same places this still fails:
error[E0502]: cannot borrow
configas immutable because it is also borrowed as mutable
-->libgpiod/tests/line_request.rs:449:24| 441 | let request = config.request(); | ---------------- mutable borrow occurs here...
449 | let info = config.chip().line_info(0).unwrap();
| ^^^^^^^^^^^^^ immutable borrow occurs here
...456 | request.reconfigure_lines(&lconfig).unwrap(); | ----------------------------------- mutable borrowlater used here
If I make
config.chip()return &mut Chip then it fails like that:error[E0499]: cannot borrow
configas mutable more than once at a time
-->libgpiod/tests/line_request.rs:449:24| 441 | let request = config.request(); | ---------------- first mutable borrowoccurs here
...
449 | let info = config.chip().line_info(0).unwrap();
| ^^^^^^^^^^^^^ second mutable borrow occurs here
...
Quoted 5 lines
456 | request.reconfigure_lines(&lconfig).unwrap(); | ----------------------------------- first borrowlater used here
Not sure how to go about this.
What you did earlier is the easiest way to get around it probably. i.e. use config.request().reconfigure_lines().
Signature · 1 line
viresh
PATCH 16/16 bindings: rust: provide line_config.set_output_values()
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
Add a new function to line config allowing to set a list of output values for requested lines. This works very similarily to the C++ version of the new C interface.
--- bindings/rust/libgpiod/src/lib.rs | 1 + bindings/rust/libgpiod/src/line_config.rs | 24 +++++++- bindings/rust/libgpiod/tests/line_config.rs | 62 +++++++++++++++++++++ 3 files changed, 86 insertions(+), 1 deletion(-)
bindings/rust/libgpiod/src/lib.rs +1 -0
@@ -71,6 +71,7 @@ pub enum OperationType { InfoEventGetLineInfo, LineConfigNew, LineConfigAddSettings, + LineConfigSetOutputValues, LineConfigGetOffsets, LineConfigGetSettings, LineRequestReconfigLines,
bindings/rust/libgpiod/src/line_config.rs +23 -1
@@ -7,7 +7,7 @@ use std::collections::HashMap;use super ::{ gpiod, - line::{Offset ,Settings }, + line::{Offset ,Settings ,Value },Error ,OperationType ,Result , };
@@ -77,6 +77,28 @@ impl Config { } } +/// Set output values for a number of lines. +pub fn set_output_values (&mut self, values: &[Value ]) ->Result <&mut Self > { +let mut mapped_values =Vec ::new (); +for valuein values { + mapped_values.push (value.value ()); + } + +let ret =unsafe { + gpiod::gpiod_line_config_set_output_values (self.config, mapped_values.as_ptr (), + values.len ()as u64 ) + }; + +if ret == -1 { +Err (Error ::OperationFailed ( +OperationType ::LineConfigSetOutputValues , + errno::errno (), + )) + }else { +Ok (self) + } + } +/// Get a mapping of offsets to line settings stored by this object. pub fn line_settings (&self) ->Result <HashMap <Offset ,Settings >> {let mut map:HashMap <Offset ,Settings > =HashMap ::new ();
bindings/rust/libgpiod/tests/line_config.rs +62 -0
75 lines not on this page · raw message
Signature · 1 line
2.37.2
Re: [libgpiod][PATCH 16/16] bindings: rust: provide line_config.set_output_values()
bindings/rust/libgpiod/src/line_config.rs impl Config { · line 85
Quoted 8 lines · Bartosz Golaszewski
bindings/rust/libgpiod/src/line_config.rs +6
-0+/// Set output values for a number of lines. +pub fn set_output_values (&mut self, values: &[Value ]) ->Result <&mut Self > { +let mut mapped_values =Vec ::new (); +for valuein values { + mapped_values.push (value.value ()); + }
Can be rewritten as this too:
let values: Vec<gpiod::gpiod_line_value> = values.iter().map(|val| val.value()).collect();
Quoted 15 lines
bindings/rust/libgpiod/src/line_config.rs +15
-0+ +let ret =unsafe { + gpiod::gpiod_line_config_set_output_values (self.config, mapped_values.as_ptr (), + values.len ()as u64 ) + }; + +if ret == -1 { +Err (Error ::OperationFailed ( +OperationType ::LineConfigSetOutputValues , + errno::errno (), + )) + }else { +Ok (self) + } + }
Signature · 1 line
viresh