* [libgpiod][PATCH 01/16] README: update for libgpiod v2
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
@ 2023-01-13 21:51 ` Bartosz Golaszewski
2023-01-14 11:14 ` Andy Shevchenko
2023-01-13 21:51 ` [libgpiod][PATCH 02/16] tests: avoid shadowing local variables with common names in macros Bartosz Golaszewski
` (14 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:51 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Certain parts of the README file still refer to concepts removed from
libgpiod v2. Update whatever needs updating.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
README | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/README b/README
index d51d701..894fc5d 100644
--- a/README
+++ b/README
@@ -1,5 +1,5 @@
# SPDX-License-Identifier: CC-BY-SA-4.0
-# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@gmail.com>
+# SPDX-FileCopyrightText: 2017-2023 Bartosz Golaszewski <brgl@bgdev.pl>
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.
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 01/16] README: update for libgpiod v2
2023-01-13 21:51 ` [libgpiod][PATCH 01/16] README: update for libgpiod v2 Bartosz Golaszewski
@ 2023-01-14 11:14 ` Andy Shevchenko
0 siblings, 0 replies; 41+ messages in thread
From: Andy Shevchenko @ 2023-01-14 11:14 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, Viresh Kumar, linux-gpio,
Bartosz Golaszewski
On Fri, Jan 13, 2023 at 10:51:55PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Certain parts of the README file still refer to concepts removed from
> libgpiod v2. Update whatever needs updating.
...
> 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
Mistakenly removed word 'module' ?
> +must either be built-in or available for loading using kmod. A helper
> +library - libgpiosim - is included to enable straightforward interaction with
> +the module.
...
> -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?
> +libgpiosim library to avoid code duplication when interacting with gpio-sim.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 41+ messages in thread
* [libgpiod][PATCH 02/16] tests: avoid shadowing local variables with common names in macros
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
2023-01-13 21:51 ` [libgpiod][PATCH 01/16] README: update for libgpiod v2 Bartosz Golaszewski
@ 2023-01-13 21:51 ` Bartosz Golaszewski
2023-01-14 11:16 ` Andy Shevchenko
2023-01-13 21:51 ` [libgpiod][PATCH 03/16] build: unify the coding style of source files lists in Makefiles Bartosz Golaszewski
` (13 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:51 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
The name 'ret' if very common for local variables so change it to _ret
in test helper macros to avoid potential shadowing.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
tests/gpiod-test-helpers.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/tests/gpiod-test-helpers.h b/tests/gpiod-test-helpers.h
index 2d86345..b40b820 100644
--- a/tests/gpiod-test-helpers.h
+++ b/tests/gpiod-test-helpers.h
@@ -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)
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 02/16] tests: avoid shadowing local variables with common names in macros
2023-01-13 21:51 ` [libgpiod][PATCH 02/16] tests: avoid shadowing local variables with common names in macros Bartosz Golaszewski
@ 2023-01-14 11:16 ` Andy Shevchenko
0 siblings, 0 replies; 41+ messages in thread
From: Andy Shevchenko @ 2023-01-14 11:16 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, Viresh Kumar, linux-gpio,
Bartosz Golaszewski
On Fri, Jan 13, 2023 at 10:51:56PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> The name 'ret' if very common for local variables so change it to _ret
> in test helper macros to avoid potential shadowing.
Makes sense!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> tests/gpiod-test-helpers.h | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/tests/gpiod-test-helpers.h b/tests/gpiod-test-helpers.h
> index 2d86345..b40b820 100644
> --- a/tests/gpiod-test-helpers.h
> +++ b/tests/gpiod-test-helpers.h
> @@ -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)
>
> --
> 2.37.2
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 41+ messages in thread
* [libgpiod][PATCH 03/16] build: unify the coding style of source files lists in Makefiles
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
2023-01-13 21:51 ` [libgpiod][PATCH 01/16] README: update for libgpiod v2 Bartosz Golaszewski
2023-01-13 21:51 ` [libgpiod][PATCH 02/16] tests: avoid shadowing local variables with common names in macros Bartosz Golaszewski
@ 2023-01-13 21:51 ` Bartosz Golaszewski
2023-01-13 21:51 ` [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions Bartosz Golaszewski
` (12 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:51 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Use the most common and readable convention for listing source files
in Makefiles wherever it's not consistent yet.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
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(-)
diff --git a/bindings/cxx/Makefile.am b/bindings/cxx/Makefile.am
index f719072..f2fc884 100644
--- a/bindings/cxx/Makefile.am
+++ b/bindings/cxx/Makefile.am
@@ -2,22 +2,22 @@
# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@gmail.com>
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
diff --git a/bindings/cxx/examples/Makefile.am b/bindings/cxx/examples/Makefile.am
index c7ec3cf..e4136f5 100644
--- a/bindings/cxx/examples/Makefile.am
+++ b/bindings/cxx/examples/Makefile.am
@@ -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
diff --git a/bindings/cxx/tests/Makefile.am b/bindings/cxx/tests/Makefile.am
index 4971dd4..924a3cd 100644
--- a/bindings/cxx/tests/Makefile.am
+++ b/bindings/cxx/tests/Makefile.am
@@ -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
\ No newline at end of file
+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
diff --git a/lib/Makefile.am b/lib/Makefile.am
index dd90abd..3e7114b 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -2,19 +2,20 @@
# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@gmail.com>
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/
diff --git a/man/Makefile.am b/man/Makefile.am
index 201a52b..1b52e0f 100644
--- a/man/Makefile.am
+++ b/man/Makefile.am
@@ -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
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 4dd5297..0fdbe5b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -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
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
` (2 preceding siblings ...)
2023-01-13 21:51 ` [libgpiod][PATCH 03/16] build: unify the coding style of source files lists in Makefiles Bartosz Golaszewski
@ 2023-01-13 21:51 ` Bartosz Golaszewski
2023-01-16 0:14 ` Kent Gibson
2023-01-16 5:52 ` Viresh Kumar
2023-01-13 21:51 ` [libgpiod][PATCH 05/16] doc: update docs for libgpiod v2 Bartosz Golaszewski
` (11 subsequent siblings)
15 siblings, 2 replies; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:51 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
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.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
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(-)
diff --git a/bindings/cxx/line-config.cpp b/bindings/cxx/line-config.cpp
index f7f1bfa..3ec99f0 100644
--- a/bindings/cxx/line-config.cpp
+++ b/bindings/cxx/line-config.cpp
@@ -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;
diff --git a/bindings/cxx/line-request.cpp b/bindings/cxx/line-request.cpp
index 34c5850..438c0fa 100644
--- a/bindings/cxx/line-request.cpp
+++ b/bindings/cxx/line-request.cpp
@@ -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];
diff --git a/bindings/python/gpiod/ext/request.c b/bindings/python/gpiod/ext/request.c
index d3e1448..a32ff8f 100644
--- a/bindings/python/gpiod/ext/request.c
+++ b/bindings/python/gpiod/ext/request.c
@@ -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)
diff --git a/bindings/rust/libgpiod/src/line_config.rs b/bindings/rust/libgpiod/src/line_config.rs
index 19dc187..0c8b293 100644
--- a/bindings/rust/libgpiod/src/line_config.rs
+++ b/bindings/rust/libgpiod/src/line_config.rs
@@ -2,8 +2,8 @@
// SPDX-FileCopyrightText: 2022 Linaro Ltd.
// SPDX-FileCopyrightTest: 2022 Viresh Kumar <viresh.kumar@linaro.org>
-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> {
- // 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>> {
+ let mut map: HashMap<Offset, Settings> = HashMap::new();
+ let num_lines = unsafe { gpiod::gpiod_line_config_get_num_configured_offsets(self.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,
+ offsets.as_mut_ptr(),
+ num_lines) };
+
+ for offset in 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 num as *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(ptr as *const Offset, num as usize).to_vec() };
-
- // SAFETY: The `ptr` array is guaranteed to be valid here.
- unsafe { libc::free(ptr as *mut c_void) };
-
- Ok(offsets)
+ Ok(map)
}
}
diff --git a/bindings/rust/libgpiod/src/line_request.rs b/bindings/rust/libgpiod/src/line_request.rs
index c16ec9f..b843862 100644
--- a/bindings/rust/libgpiod/src/line_request.rs
+++ b/bindings/rust/libgpiod/src/line_request.rs
@@ -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
}
diff --git a/bindings/rust/libgpiod/tests/line_config.rs b/bindings/rust/libgpiod/tests/line_config.rs
index bebf106..95f2178 100644
--- a/bindings/rust/libgpiod/tests/line_config.rs
+++ b/bindings/rust/libgpiod/tests/line_config.rs
@@ -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]);
- }
}
diff --git a/configure.ac b/configure.ac
index 599c598..a4b92ce 100644
--- a/configure.ac
+++ b/configure.ac
@@ -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])])
diff --git a/include/gpiod.h b/include/gpiod.h
index 56c182f..dfc5334 100644
--- a/include/gpiod.h
+++ b/include/gpiod.h
@@ -780,19 +780,29 @@ struct gpiod_line_settings *
gpiod_line_config_get_line_settings(struct gpiod_line_config *config,
unsigned 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);
+
/**
* @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,
+ unsigned int *offsets,
+ size_t max_offsets);
/**
* @}
@@ -880,16 +890,24 @@ void gpiod_line_request_release(struct gpiod_line_request *request);
* @param request Line request object.
* @return Number of requested lines.
*/
-size_t gpiod_line_request_get_num_lines(struct gpiod_line_request *request);
+size_t
+gpiod_line_request_get_num_requested_lines(struct gpiod_line_request *request);
/**
* @brief Get the offsets of the lines in the request.
* @param request Line request object.
- * @param offsets Array to store offsets. Must be sized to hold the number of
- * lines returned by ::gpiod_line_request_get_num_lines.
+ * @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_request_get_num_requested_lines),
+ * then only up to max_lines offsets will be stored in offsets.
*/
-void gpiod_line_request_get_offsets(struct gpiod_line_request *request,
- unsigned int *offsets);
+size_t
+gpiod_line_request_get_requested_offsets(struct gpiod_line_request *request,
+ unsigned int *offsets,
+ size_t max_offsets);
/**
* @brief Get the value of a single requested line.
@@ -922,10 +940,10 @@ int gpiod_line_request_get_values_subset(struct gpiod_line_request *request,
* @param request GPIO line request.
* @param values Array in which the values will be stored. Must be sized to
* hold the number of lines returned by
- * ::gpiod_line_request_get_num_lines.
+ * ::gpiod_line_request_get_num_requested_lines.
* Each value is associated with the line identified by the
* corresponding entry in the offset array returned by
- * ::gpiod_line_request_get_offsets.
+ * ::gpiod_line_request_get_requested_offsets.
* @return 0 on success, -1 on failure.
*/
int gpiod_line_request_get_values(struct gpiod_line_request *request,
@@ -963,10 +981,10 @@ int gpiod_line_request_set_values_subset(struct gpiod_line_request *request,
* @param request GPIO line request.
* @param values Array containing the values to set. Must be sized to
* contain the number of lines returned by
- * ::gpiod_line_request_get_num_lines.
+ * ::gpiod_line_request_get_num_requested_lines.
* Each value is associated with the line identified by the
* corresponding entry in the offset array returned by
- * ::gpiod_line_request_get_offsets.
+ * ::gpiod_line_request_get_requested_offsets.
*/
int gpiod_line_request_set_values(struct gpiod_line_request *request,
const enum gpiod_line_value *values);
diff --git a/lib/line-config.c b/lib/line-config.c
index bc10059..b00e5e6 100644
--- a/lib/line-config.c
+++ b/lib/line-config.c
@@ -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,
diff --git a/lib/line-request.c b/lib/line-request.c
index c9ad337..e536355 100644
--- a/lib/line-request.c
+++ b/lib/line-request.c
@@ -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
diff --git a/tests/tests-line-config.c b/tests/tests-line-config.c
index 4b5b3bd..5afdf7b 100644
--- a/tests/tests-line-config.c
+++ b/tests/tests-line-config.c
@@ -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);
}
diff --git a/tests/tests-line-request.c b/tests/tests-line-request.c
index 2c2af01..fa02289 100644
--- a/tests/tests-line-request.c
+++ b/tests/tests-line-request.c
@@ -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]);
}
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
2023-01-13 21:51 ` [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions Bartosz Golaszewski
@ 2023-01-16 0:14 ` Kent Gibson
2023-01-16 21:37 ` Bartosz Golaszewski
2023-01-16 5:52 ` Viresh Kumar
1 sibling, 1 reply; 41+ messages in thread
From: Kent Gibson @ 2023-01-16 0:14 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Andy Shevchenko, Viresh Kumar, linux-gpio,
Bartosz Golaszewski
On Fri, Jan 13, 2023 at 10:51:58PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> 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.
>
They differ because they operate in different circumstances.
Requests are immutable, wrt lines/offsets, while configs are not.
More on this below.
> 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.
> 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 <bartosz.golaszewski@linaro.org>
<snip>
> --- a/include/gpiod.h
> +++ b/include/gpiod.h
> @@ -780,19 +780,29 @@ struct gpiod_line_settings *
> gpiod_line_config_get_line_settings(struct gpiod_line_config *config,
> unsigned 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.
> /**
> * @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,
> + 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.
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
2023-01-16 0:14 ` Kent Gibson
@ 2023-01-16 21:37 ` Bartosz Golaszewski
2023-01-16 23:39 ` Kent Gibson
0 siblings, 1 reply; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-16 21:37 UTC (permalink / raw)
To: Kent Gibson
Cc: Linus Walleij, Andy Shevchenko, Viresh Kumar, linux-gpio,
Bartosz Golaszewski
On Mon, Jan 16, 2023 at 1:14 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Jan 13, 2023 at 10:51:58PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > 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.
> >
>
> They differ because they operate in different circumstances.
> Requests are immutable, wrt lines/offsets, while configs are not.
> More on this below.
>
> > 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.
>
> > 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 <bartosz.golaszewski@linaro.org>
>
> <snip>
> > --- a/include/gpiod.h
> > +++ b/include/gpiod.h
> > @@ -780,19 +780,29 @@ struct gpiod_line_settings *
> > gpiod_line_config_get_line_settings(struct gpiod_line_config *config,
> > unsigned 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?
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()? :)
> Similarly requested lines in the request.
>
> > /**
> > * @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,
> > + 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?
>
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.
> 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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
2023-01-16 21:37 ` Bartosz Golaszewski
@ 2023-01-16 23:39 ` Kent Gibson
0 siblings, 0 replies; 41+ messages in thread
From: Kent Gibson @ 2023-01-16 23:39 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Andy Shevchenko, Viresh Kumar, linux-gpio,
Bartosz Golaszewski
On Mon, Jan 16, 2023 at 10:37:14PM +0100, Bartosz Golaszewski wrote:
> On Mon, Jan 16, 2023 at 1:14 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, Jan 13, 2023 at 10:51:58PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > 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.
> > >
> >
> > They differ because they operate in different circumstances.
> > Requests are immutable, wrt lines/offsets, while configs are not.
> > More on this below.
> >
> > > 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.
> >
> > > 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 <bartosz.golaszewski@linaro.org>
> >
> > <snip>
> > > --- a/include/gpiod.h
> > > +++ b/include/gpiod.h
> > > @@ -780,19 +780,29 @@ struct gpiod_line_settings *
> > > gpiod_line_config_get_line_settings(struct gpiod_line_config *config,
> > > unsigned 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?
>
> 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()? :)
>
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.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
2023-01-13 21:51 ` [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions Bartosz Golaszewski
2023-01-16 0:14 ` Kent Gibson
@ 2023-01-16 5:52 ` Viresh Kumar
2023-01-16 21:39 ` Bartosz Golaszewski
` (2 more replies)
1 sibling, 3 replies; 41+ messages in thread
From: Viresh Kumar @ 2023-01-16 5:52 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
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 -D clippy::undocumented_unsafe_blocks
On 13-01-23, 22:51, Bartosz Golaszewski wrote:
> diff --git a/bindings/rust/libgpiod/src/line_config.rs b/bindings/rust/libgpiod/src/line_config.rs
> index 19dc187..0c8b293 100644
> --- a/bindings/rust/libgpiod/src/line_config.rs
> +++ b/bindings/rust/libgpiod/src/line_config.rs
> @@ -2,8 +2,8 @@
> // SPDX-FileCopyrightText: 2022 Linaro Ltd.
> // SPDX-FileCopyrightTest: 2022 Viresh Kumar <viresh.kumar@linaro.org>
>
> -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> {
> - // 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>;
> + 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 ?
> + 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) };
Can the returned value be < num_lines here ?
> +
> + for offset in 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 num as *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(ptr as *const Offset, num as usize).to_vec() };
> -
> - // SAFETY: The `ptr` array is guaranteed to be valid here.
> - unsafe { libc::free(ptr as *mut c_void) };
> -
> - Ok(offsets)
> + Ok(map)
> }
> }
--
viresh
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
2023-01-16 5:52 ` Viresh Kumar
@ 2023-01-16 21:39 ` Bartosz Golaszewski
2023-01-17 5:44 ` Viresh Kumar
2023-01-23 8:24 ` Viresh Kumar
2023-01-23 13:58 ` Bartosz Golaszewski
2 siblings, 1 reply; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-16 21:39 UTC (permalink / raw)
To: Viresh Kumar
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
On Mon, Jan 16, 2023 at 6:52 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> 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 -D clippy::undocumented_unsafe_blocks
>
> On 13-01-23, 22:51, Bartosz Golaszewski wrote:
> > diff --git a/bindings/rust/libgpiod/src/line_config.rs b/bindings/rust/libgpiod/src/line_config.rs
> > index 19dc187..0c8b293 100644
> > --- a/bindings/rust/libgpiod/src/line_config.rs
> > +++ b/bindings/rust/libgpiod/src/line_config.rs
> > @@ -2,8 +2,8 @@
> > // SPDX-FileCopyrightText: 2022 Linaro Ltd.
> > // SPDX-FileCopyrightTest: 2022 Viresh Kumar <viresh.kumar@linaro.org>
> >
> > -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> {
> > - // 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>;
>
> > + 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 ?
>
> > + 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) };
>
> Can the returned value be < num_lines here ?
>
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?
> > +
> > + for offset in 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 num as *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(ptr as *const Offset, num as usize).to_vec() };
> > -
> > - // SAFETY: The `ptr` array is guaranteed to be valid here.
> > - unsafe { libc::free(ptr as *mut c_void) };
> > -
> > - Ok(offsets)
> > + Ok(map)
> > }
> > }
>
> --
> viresh
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
2023-01-16 21:39 ` Bartosz Golaszewski
@ 2023-01-17 5:44 ` Viresh Kumar
2023-01-18 20:51 ` Bartosz Golaszewski
0 siblings, 1 reply; 41+ messages in thread
From: Viresh Kumar @ 2023-01-17 5:44 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
On 16-01-23, 22:39, Bartosz Golaszewski wrote:
> On Mon, Jan 16, 2023 at 6:52 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > + 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) };
> >
> > Can the returned value be < num_lines here ?
> >
>
> 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] {
...
}
--
viresh
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
2023-01-17 5:44 ` Viresh Kumar
@ 2023-01-18 20:51 ` Bartosz Golaszewski
2023-01-19 5:15 ` Viresh Kumar
0 siblings, 1 reply; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-18 20:51 UTC (permalink / raw)
To: Viresh Kumar
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
On Tue, Jan 17, 2023 at 6:44 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 16-01-23, 22:39, Bartosz Golaszewski wrote:
> > On Mon, Jan 16, 2023 at 6:52 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > + 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) };
> > >
> > > Can the returned value be < num_lines here ?
> > >
> >
> > 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] {
> ...
> }
>
It sees 'offset' becomes an instance of std::ops::Range. Is there
anything more to add here?
Bart
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
2023-01-18 20:51 ` Bartosz Golaszewski
@ 2023-01-19 5:15 ` Viresh Kumar
0 siblings, 0 replies; 41+ messages in thread
From: Viresh Kumar @ 2023-01-19 5:15 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
On 18-01-23, 21:51, Bartosz Golaszewski wrote:
> On Tue, Jan 17, 2023 at 6:44 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 16-01-23, 22:39, Bartosz Golaszewski wrote:
> > > On Mon, Jan 16, 2023 at 6:52 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > > + 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) };
> > > >
> > > > Can the returned value be < num_lines here ?
> > > >
> > >
> > > 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] {
> > ...
> > }
> >
>
> It sees 'offset' becomes an instance of std::ops::Range. Is there
> anything more to add here?
This builds fine.
diff --git a/bindings/rust/libgpiod/src/line_config.rs b/bindings/rust/libgpiod/src/line_config.rs
index b276cf0c4931..e30ef7c35328 100644
--- a/bindings/rust/libgpiod/src/line_config.rs
+++ b/bindings/rust/libgpiod/src/line_config.rs
@@ -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)
--
viresh
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
2023-01-16 5:52 ` Viresh Kumar
2023-01-16 21:39 ` Bartosz Golaszewski
@ 2023-01-23 8:24 ` Viresh Kumar
2023-01-23 8:31 ` Bartosz Golaszewski
2023-01-23 13:58 ` Bartosz Golaszewski
2 siblings, 1 reply; 41+ messages in thread
From: Viresh Kumar @ 2023-01-23 8:24 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
Hi Bartosz,
I gave few comments here, and it looks like you missed them ?
On 16-01-23, 11:22, Viresh Kumar wrote:
> 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 -D clippy::undocumented_unsafe_blocks
>
> On 13-01-23, 22:51, Bartosz Golaszewski wrote:
> Just like ValueMap, maybe we can add following in lib.rs for this:
>
> pub type line::SettingsMap = IntMap<line::Settings>;
>
> > + 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
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
2023-01-23 8:24 ` Viresh Kumar
@ 2023-01-23 8:31 ` Bartosz Golaszewski
0 siblings, 0 replies; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-23 8:31 UTC (permalink / raw)
To: Viresh Kumar
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
On Mon, Jan 23, 2023 at 9:24 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Hi Bartosz,
>
> I gave few comments here, and it looks like you missed them ?
>
> On 16-01-23, 11:22, Viresh Kumar wrote:
> > 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 -D clippy::undocumented_unsafe_blocks
> >
> > On 13-01-23, 22:51, Bartosz Golaszewski wrote:
>
> > Just like ValueMap, maybe we can add following in lib.rs for this:
> >
> > pub type line::SettingsMap = IntMap<line::Settings>;
> >
> > > + 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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
2023-01-16 5:52 ` Viresh Kumar
2023-01-16 21:39 ` Bartosz Golaszewski
2023-01-23 8:24 ` Viresh Kumar
@ 2023-01-23 13:58 ` Bartosz Golaszewski
2023-01-24 6:44 ` Viresh Kumar
2 siblings, 1 reply; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-23 13:58 UTC (permalink / raw)
To: Viresh Kumar
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
On Mon, Jan 16, 2023 at 6:52 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> 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 -D clippy::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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions
2023-01-23 13:58 ` Bartosz Golaszewski
@ 2023-01-24 6:44 ` Viresh Kumar
0 siblings, 0 replies; 41+ messages in thread
From: Viresh Kumar @ 2023-01-24 6:44 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
On 23-01-23, 14:58, Bartosz Golaszewski wrote:
> On Mon, Jan 16, 2023 at 6:52 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > 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 -D clippy::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. :)
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.
--
viresh
^ permalink raw reply [flat|nested] 41+ messages in thread
* [libgpiod][PATCH 05/16] doc: update docs for libgpiod v2
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
` (3 preceding siblings ...)
2023-01-13 21:51 ` [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions Bartosz Golaszewski
@ 2023-01-13 21:51 ` Bartosz Golaszewski
2023-01-13 21:52 ` [libgpiod][PATCH 06/16] bindings: cxx: prepend all C symbols with the scope resolution operator Bartosz Golaszewski
` (10 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:51 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Update docs in gpiod.h wherever they're outdated or make incorrect
statements. While at it: fix formatting in some places.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
include/gpiod.h | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/include/gpiod.h b/include/gpiod.h
index dfc5334..8cede47 100644
--- a/include/gpiod.h
+++ b/include/gpiod.h
@@ -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,
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [libgpiod][PATCH 06/16] bindings: cxx: prepend all C symbols with the scope resolution operator
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
` (4 preceding siblings ...)
2023-01-13 21:51 ` [libgpiod][PATCH 05/16] doc: update docs for libgpiod v2 Bartosz Golaszewski
@ 2023-01-13 21:52 ` Bartosz Golaszewski
2023-01-13 21:52 ` [libgpiod][PATCH 07/16] bindings: cxx: allow to copy line_settings Bartosz Golaszewski
` (9 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:52 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
We explicitly resolve all global C symbols from libgpiod to the top-level
namespace. Fix it wherever its missing (mostly for C enum types).
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
bindings/cxx/line-request.cpp | 4 +--
bindings/cxx/line-settings.cpp | 52 ++++++++++++++++++----------------
2 files changed, 30 insertions(+), 26 deletions(-)
diff --git a/bindings/cxx/line-request.cpp b/bindings/cxx/line-request.cpp
index 438c0fa..b0723c3 100644
--- a/bindings/cxx/line-request.cpp
+++ b/bindings/cxx/line-request.cpp
@@ -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");
diff --git a/bindings/cxx/line-settings.cpp b/bindings/cxx/line-settings.cpp
index 7d3d6a5..5ded953 100644
--- a/bindings/cxx/line-settings.cpp
+++ b/bindings/cxx/line-settings.cpp
@@ -22,56 +22,60 @@ make_reverse_maping(const ::std::map<cxx_enum_type, c_enum_type>& mapping)
return ret;
}
-const ::std::map<line::direction, gpiod_line_direction> direction_mapping = {
+const ::std::map<line::direction, ::gpiod_line_direction> direction_mapping = {
{ line::direction::AS_IS, GPIOD_LINE_DIRECTION_AS_IS },
{ line::direction::INPUT, GPIOD_LINE_DIRECTION_INPUT },
{ line::direction::OUTPUT, GPIOD_LINE_DIRECTION_OUTPUT },
};
-const ::std::map<gpiod_line_direction, line::direction>
+const ::std::map<::gpiod_line_direction, line::direction>
reverse_direction_mapping = make_reverse_maping(direction_mapping);
-const ::std::map<line::edge, gpiod_line_edge> edge_mapping = {
+const ::std::map<line::edge, ::gpiod_line_edge> edge_mapping = {
{ line::edge::NONE, GPIOD_LINE_EDGE_NONE },
{ line::edge::FALLING, GPIOD_LINE_EDGE_FALLING },
{ line::edge::RISING, GPIOD_LINE_EDGE_RISING },
{ line::edge::BOTH, GPIOD_LINE_EDGE_BOTH },
};
-const ::std::map<gpiod_line_edge, line::edge> reverse_edge_mapping = make_reverse_maping(edge_mapping);
+const ::std::map<::gpiod_line_edge, line::edge>
+reverse_edge_mapping = make_reverse_maping(edge_mapping);
-const ::std::map<line::bias, gpiod_line_bias> bias_mapping = {
+const ::std::map<line::bias, ::gpiod_line_bias> bias_mapping = {
{ line::bias::AS_IS, GPIOD_LINE_BIAS_AS_IS },
{ line::bias::DISABLED, GPIOD_LINE_BIAS_DISABLED },
{ line::bias::PULL_UP, GPIOD_LINE_BIAS_PULL_UP },
{ line::bias::PULL_DOWN, GPIOD_LINE_BIAS_PULL_DOWN },
};
-const ::std::map<gpiod_line_bias, line::bias> reverse_bias_mapping = make_reverse_maping(bias_mapping);
+const ::std::map<::gpiod_line_bias, line::bias>
+reverse_bias_mapping = make_reverse_maping(bias_mapping);
-const ::std::map<line::drive, gpiod_line_drive> drive_mapping = {
+const ::std::map<line::drive, ::gpiod_line_drive> drive_mapping = {
{ line::drive::PUSH_PULL, GPIOD_LINE_DRIVE_PUSH_PULL },
{ line::drive::OPEN_DRAIN, GPIOD_LINE_DRIVE_OPEN_DRAIN },
{ line::drive::OPEN_SOURCE, GPIOD_LINE_DRIVE_OPEN_SOURCE },
};
-const ::std::map<gpiod_line_drive, line::drive> reverse_drive_mapping = make_reverse_maping(drive_mapping);
+const ::std::map<::gpiod_line_drive, line::drive>
+reverse_drive_mapping = make_reverse_maping(drive_mapping);
-const ::std::map<line::clock, gpiod_line_clock> clock_mapping = {
+const ::std::map<line::clock, ::gpiod_line_clock> clock_mapping = {
{ line::clock::MONOTONIC, GPIOD_LINE_CLOCK_MONOTONIC },
{ line::clock::REALTIME, GPIOD_LINE_CLOCK_REALTIME },
{ line::clock::HTE, GPIOD_LINE_CLOCK_HTE },
};
-const ::std::map<gpiod_line_clock, line::clock>
+const ::std::map<::gpiod_line_clock, line::clock>
reverse_clock_mapping = make_reverse_maping(clock_mapping);
-const ::std::map<line::value, gpiod_line_value> value_mapping = {
+const ::std::map<line::value, ::gpiod_line_value> value_mapping = {
{ line::value::INACTIVE, GPIOD_LINE_VALUE_INACTIVE },
{ line::value::ACTIVE, GPIOD_LINE_VALUE_ACTIVE },
};
-const ::std::map<gpiod_line_value, line::value> reverse_value_mapping = make_reverse_maping(value_mapping);
+const ::std::map<::gpiod_line_value, line::value>
+reverse_value_mapping = make_reverse_maping(value_mapping);
line_settings_ptr make_line_settings()
{
@@ -165,7 +169,7 @@ GPIOD_CXX_API line_settings& line_settings::reset(void) noexcept
GPIOD_CXX_API line_settings& line_settings::set_direction(line::direction direction)
{
- set_mapped_value<line::direction, gpiod_line_direction,
+ set_mapped_value<line::direction, ::gpiod_line_direction,
::gpiod_line_settings_set_direction>(this->_m_priv->settings.get(),
direction, direction_mapping);
@@ -174,7 +178,7 @@ GPIOD_CXX_API line_settings& line_settings::set_direction(line::direction direct
GPIOD_CXX_API line::direction line_settings::direction() const
{
- return get_mapped_value<line::direction, gpiod_line_direction,
+ return get_mapped_value<line::direction, ::gpiod_line_direction,
::gpiod_line_settings_get_direction>(
this->_m_priv->settings.get(),
reverse_direction_mapping);
@@ -182,7 +186,7 @@ GPIOD_CXX_API line::direction line_settings::direction() const
GPIOD_CXX_API line_settings& line_settings::set_edge_detection(line::edge edge)
{
- set_mapped_value<line::edge, gpiod_line_edge,
+ set_mapped_value<line::edge, ::gpiod_line_edge,
::gpiod_line_settings_set_edge_detection>(this->_m_priv->settings.get(),
edge, edge_mapping);
@@ -191,7 +195,7 @@ GPIOD_CXX_API line_settings& line_settings::set_edge_detection(line::edge edge)
GPIOD_CXX_API line::edge line_settings::edge_detection() const
{
- return get_mapped_value<line::edge, gpiod_line_edge,
+ return get_mapped_value<line::edge, ::gpiod_line_edge,
::gpiod_line_settings_get_edge_detection>(
this->_m_priv->settings.get(),
reverse_edge_mapping);
@@ -199,7 +203,7 @@ GPIOD_CXX_API line::edge line_settings::edge_detection() const
GPIOD_CXX_API line_settings& line_settings::set_bias(line::bias bias)
{
- set_mapped_value<line::bias, gpiod_line_bias,
+ set_mapped_value<line::bias, ::gpiod_line_bias,
::gpiod_line_settings_set_bias>(this->_m_priv->settings.get(),
bias, bias_mapping);
@@ -208,14 +212,14 @@ GPIOD_CXX_API line_settings& line_settings::set_bias(line::bias bias)
GPIOD_CXX_API line::bias line_settings::bias() const
{
- return get_mapped_value<line::bias, gpiod_line_bias,
+ return get_mapped_value<line::bias, ::gpiod_line_bias,
::gpiod_line_settings_get_bias>(this->_m_priv->settings.get(),
reverse_bias_mapping);
}
GPIOD_CXX_API line_settings& line_settings::set_drive(line::drive drive)
{
- set_mapped_value<line::drive, gpiod_line_drive,
+ set_mapped_value<line::drive, ::gpiod_line_drive,
::gpiod_line_settings_set_drive>(this->_m_priv->settings.get(),
drive, drive_mapping);
@@ -224,7 +228,7 @@ GPIOD_CXX_API line_settings& line_settings::set_drive(line::drive drive)
GPIOD_CXX_API line::drive line_settings::drive() const
{
- return get_mapped_value<line::drive, gpiod_line_drive,
+ return get_mapped_value<line::drive, ::gpiod_line_drive,
::gpiod_line_settings_get_drive>(this->_m_priv->settings.get(),
reverse_drive_mapping);
}
@@ -257,7 +261,7 @@ GPIOD_CXX_API ::std::chrono::microseconds line_settings::debounce_period() const
GPIOD_CXX_API line_settings& line_settings::set_event_clock(line::clock event_clock)
{
- set_mapped_value<line::clock, gpiod_line_clock,
+ set_mapped_value<line::clock, ::gpiod_line_clock,
::gpiod_line_settings_set_event_clock>(this->_m_priv->settings.get(),
event_clock, clock_mapping);
@@ -266,7 +270,7 @@ GPIOD_CXX_API line_settings& line_settings::set_event_clock(line::clock event_cl
GPIOD_CXX_API line::clock line_settings::event_clock() const
{
- return get_mapped_value<line::clock, gpiod_line_clock,
+ return get_mapped_value<line::clock, ::gpiod_line_clock,
::gpiod_line_settings_get_event_clock>(
this->_m_priv->settings.get(),
reverse_clock_mapping);
@@ -274,7 +278,7 @@ GPIOD_CXX_API line::clock line_settings::event_clock() const
GPIOD_CXX_API line_settings& line_settings::set_output_value(line::value value)
{
- set_mapped_value<line::value, gpiod_line_value,
+ set_mapped_value<line::value, ::gpiod_line_value,
::gpiod_line_settings_set_output_value>(this->_m_priv->settings.get(),
value, value_mapping);
@@ -283,7 +287,7 @@ GPIOD_CXX_API line_settings& line_settings::set_output_value(line::value value)
GPIOD_CXX_API line::value line_settings::output_value() const
{
- return get_mapped_value<line::value, gpiod_line_value,
+ return get_mapped_value<line::value, ::gpiod_line_value,
::gpiod_line_settings_get_output_value>(
this->_m_priv->settings.get(),
reverse_value_mapping);
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [libgpiod][PATCH 07/16] bindings: cxx: allow to copy line_settings
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
` (5 preceding siblings ...)
2023-01-13 21:52 ` [libgpiod][PATCH 06/16] bindings: cxx: prepend all C symbols with the scope resolution operator Bartosz Golaszewski
@ 2023-01-13 21:52 ` Bartosz Golaszewski
2023-01-13 21:52 ` [libgpiod][PATCH 08/16] tests: fix the line config reset test case Bartosz Golaszewski
` (8 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:52 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
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().
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
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(-)
diff --git a/bindings/cxx/gpiodcxx/line-settings.hpp b/bindings/cxx/gpiodcxx/line-settings.hpp
index c1477b1..c18dc52 100644
--- a/bindings/cxx/gpiodcxx/line-settings.hpp
+++ b/bindings/cxx/gpiodcxx/line-settings.hpp
@@ -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.
diff --git a/bindings/cxx/internal.hpp b/bindings/cxx/internal.hpp
index d27aa22..6aceac1 100644
--- a/bindings/cxx/internal.hpp
+++ b/bindings/cxx/internal.hpp
@@ -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;
diff --git a/bindings/cxx/line-settings.cpp b/bindings/cxx/line-settings.cpp
index 5ded953..32f21a3 100644
--- a/bindings/cxx/line-settings.cpp
+++ b/bindings/cxx/line-settings.cpp
@@ -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);
diff --git a/bindings/cxx/tests/tests-line-settings.cpp b/bindings/cxx/tests/tests-line-settings.cpp
index a3f4bc5..dc821bb 100644
--- a/bindings/cxx/tests/tests-line-settings.cpp
+++ b/bindings/cxx/tests/tests-line-settings.cpp
@@ -124,6 +124,49 @@ TEST_CASE("line_settings mutators work", "[line-settings]")
}
}
+TEST_CASE("line_settings can be moved and copied", "[line-settings]")
+{
+ ::gpiod::line_settings settings;
+
+ settings
+ .set_direction(direction::INPUT)
+ .set_edge_detection(edge::BOTH);
+
+ SECTION("copy constructor works")
+ {
+ auto copy(settings);
+ settings.set_direction(direction::OUTPUT);
+ settings.set_edge_detection(edge::NONE);
+ REQUIRE(copy.direction() == direction::INPUT);
+ REQUIRE(copy.edge_detection() == edge::BOTH);
+ }
+
+ SECTION("assignment operator works")
+ {
+ ::gpiod::line_settings copy;
+ copy = settings;
+ settings.set_direction(direction::OUTPUT);
+ settings.set_edge_detection(edge::NONE);
+ REQUIRE(copy.direction() == direction::INPUT);
+ REQUIRE(copy.edge_detection() == edge::BOTH);
+ }
+
+ SECTION("move constructor works")
+ {
+ auto copy(::std::move(settings));
+ REQUIRE(copy.direction() == direction::INPUT);
+ REQUIRE(copy.edge_detection() == edge::BOTH);
+ }
+
+ SECTION("move assignment operator works")
+ {
+ ::gpiod::line_settings copy;
+ copy = ::std::move(settings);
+ REQUIRE(copy.direction() == direction::INPUT);
+ REQUIRE(copy.edge_detection() == edge::BOTH);
+ }
+}
+
TEST_CASE("line_settings stream insertion operator works", "[line-settings]")
{
::gpiod::line_settings settings;
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [libgpiod][PATCH 08/16] tests: fix the line config reset test case
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
` (6 preceding siblings ...)
2023-01-13 21:52 ` [libgpiod][PATCH 07/16] bindings: cxx: allow to copy line_settings Bartosz Golaszewski
@ 2023-01-13 21:52 ` Bartosz Golaszewski
2023-01-13 21:52 ` [libgpiod][PATCH 09/16] tests: add a helper for reading back line settings from line config Bartosz Golaszewski
` (7 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:52 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
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.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
tests/tests-line-config.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/tests-line-config.c b/tests/tests-line-config.c
index 5afdf7b..ef85c3a 100644
--- a/tests/tests-line-config.c
+++ b/tests/tests-line-config.c
@@ -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);
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [libgpiod][PATCH 09/16] tests: add a helper for reading back line settings from line config
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
` (7 preceding siblings ...)
2023-01-13 21:52 ` [libgpiod][PATCH 08/16] tests: fix the line config reset test case Bartosz Golaszewski
@ 2023-01-13 21:52 ` Bartosz Golaszewski
2023-01-13 21:52 ` [libgpiod][PATCH 10/16] core: provide gpiod_line_config_set_output_values() Bartosz Golaszewski
` (6 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:52 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Add a helper for getting line settings from line config that allows to
shrink the test code a bit.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
tests/gpiod-test-helpers.h | 10 ++++++++++
tests/tests-line-config.c | 13 ++++---------
2 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/tests/gpiod-test-helpers.h b/tests/gpiod-test-helpers.h
index b40b820..fb3fd7d 100644
--- a/tests/gpiod-test-helpers.h
+++ b/tests/gpiod-test-helpers.h
@@ -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 = \
diff --git a/tests/tests-line-config.c b/tests/tests-line-config.c
index ef85c3a..7f5e4b1 100644
--- a/tests/tests-line-config.c
+++ b/tests/tests-line-config.c
@@ -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);
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [libgpiod][PATCH 10/16] core: provide gpiod_line_config_set_output_values()
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
` (8 preceding siblings ...)
2023-01-13 21:52 ` [libgpiod][PATCH 09/16] tests: add a helper for reading back line settings from line config Bartosz Golaszewski
@ 2023-01-13 21:52 ` Bartosz Golaszewski
2023-01-16 0:15 ` Kent Gibson
2023-01-13 21:52 ` [libgpiod][PATCH 11/16] gpioset: use gpiod_line_config_set_output_values() Bartosz Golaszewski
` (5 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:52 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
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 <bartosz.golaszewski@linaro.org>
---
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(-)
diff --git a/include/gpiod.h b/include/gpiod.h
index 8cede47..c552135 100644
--- a/include/gpiod.h
+++ b/include/gpiod.h
@@ -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.
diff --git a/lib/line-config.c b/lib/line-config.c
index b00e5e6..901eb02 100644
--- a/lib/line-config.c
+++ b/lib/line-config.c
@@ -25,6 +25,8 @@ struct per_line_config {
struct gpiod_line_config {
struct per_line_config line_configs[LINES_MAX];
size_t num_configs;
+ enum gpiod_line_value output_values[LINES_MAX];
+ size_t num_output_values;
struct settings_node *sref_list;
};
@@ -136,23 +138,60 @@ GPIOD_API struct gpiod_line_settings *
gpiod_line_config_get_line_settings(struct gpiod_line_config *config,
unsigned int offset)
{
+ struct gpiod_line_settings *settings;
struct per_line_config *per_line;
size_t i;
+ int ret;
assert(config);
for (i = 0; i < config->num_configs; i++) {
per_line = &config->line_configs[i];
- if (per_line->offset == offset)
- return gpiod_line_settings_copy(
+ if (per_line->offset == offset) {
+ settings = gpiod_line_settings_copy(
per_line->node->settings);
+ if (!settings)
+ return NULL;
+
+ /*
+ * If a global output value was set for this line - use
+ * it and override the one stored in settings.
+ */
+ if (config->num_output_values > i) {
+ ret = gpiod_line_settings_set_output_value(
+ settings,
+ config->output_values[i]);
+ if (ret) {
+ gpiod_line_settings_free(settings);
+ return NULL;
+ }
+ }
+
+ return settings;
+ }
}
errno = ENOENT;
return NULL;
}
+GPIOD_API int
+gpiod_line_config_set_output_values(struct gpiod_line_config *config,
+ const enum gpiod_line_value *values,
+ size_t num_values)
+{
+ if (num_values > LINES_MAX) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ memcpy(config->output_values, values, num_values * sizeof(*values));
+ config->num_output_values = num_values;
+
+ return 0;
+}
+
GPIOD_API size_t
gpiod_line_config_get_num_configured_offsets(struct gpiod_line_config *config)
{
@@ -209,6 +248,13 @@ static bool has_at_least_one_output_direction(struct gpiod_line_config *config)
return false;
}
+static void set_output_value(uint64_t *vals, size_t bit,
+ enum gpiod_line_value value)
+{
+ gpiod_line_mask_assign_bit(vals, bit,
+ value == GPIOD_LINE_VALUE_ACTIVE ? 1 : 0);
+}
+
static void set_kernel_output_values(uint64_t *mask, uint64_t *vals,
struct gpiod_line_config *config)
{
@@ -230,8 +276,14 @@ static void set_kernel_output_values(uint64_t *mask, uint64_t *vals,
gpiod_line_mask_set_bit(mask, i);
value = gpiod_line_settings_get_output_value(
per_line->node->settings);
- gpiod_line_mask_assign_bit(
- vals, i, value == GPIOD_LINE_VALUE_ACTIVE ? 1 : 0);
+ set_output_value(vals, i, value);
+ }
+
+ /* "Global" output values override the ones from per-line settings. */
+ for (i = 0; i < config->num_output_values; i++) {
+ gpiod_line_mask_set_bit(mask, i);
+ value = config->output_values[i];
+ set_output_value(vals, i, value);
}
}
diff --git a/tests/gpiod-test-helpers.h b/tests/gpiod-test-helpers.h
index fb3fd7d..760949e 100644
--- a/tests/gpiod-test-helpers.h
+++ b/tests/gpiod-test-helpers.h
@@ -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 = \
diff --git a/tests/tests-line-config.c b/tests/tests-line-config.c
index 7f5e4b1..78a4632 100644
--- a/tests/tests-line-config.c
+++ b/tests/tests-line-config.c
@@ -238,3 +238,70 @@ GPIOD_TEST_CASE(get_null_offsets)
NULL, 10);
g_assert_cmpuint(num_offsets, ==, 0);
}
+
+GPIOD_TEST_CASE(set_global_output_values)
+{
+ static const guint offsets[] = { 0, 1, 2, 3 };
+ static const enum gpiod_line_value values[] = {
+ GPIOD_LINE_VALUE_ACTIVE,
+ GPIOD_LINE_VALUE_INACTIVE,
+ GPIOD_LINE_VALUE_ACTIVE,
+ GPIOD_LINE_VALUE_INACTIVE,
+ };
+
+ g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new("num-lines", 4, NULL);
+ g_autoptr(struct_gpiod_line_settings) settings = NULL;
+ g_autoptr(struct_gpiod_line_config) config = NULL;
+ g_autoptr(struct_gpiod_line_request) request = NULL;
+ g_autoptr(struct_gpiod_chip) chip = NULL;
+
+ chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim));
+ settings = gpiod_test_create_line_settings_or_fail();
+ config = gpiod_test_create_line_config_or_fail();
+
+ gpiod_line_settings_set_direction(settings,
+ GPIOD_LINE_DIRECTION_OUTPUT);
+ gpiod_test_line_config_add_line_settings_or_fail(config, offsets, 4,
+ settings);
+ gpiod_test_line_config_set_output_values_or_fail(config, values, 4);
+
+ request = gpiod_test_request_lines_or_fail(chip, NULL, config);
+
+ g_assert_cmpint(g_gpiosim_chip_get_value(sim, 0), ==,
+ GPIOD_LINE_VALUE_ACTIVE);
+ g_assert_cmpint(g_gpiosim_chip_get_value(sim, 1), ==,
+ GPIOD_LINE_VALUE_INACTIVE);
+ g_assert_cmpint(g_gpiosim_chip_get_value(sim, 2), ==,
+ GPIOD_LINE_VALUE_ACTIVE);
+ g_assert_cmpint(g_gpiosim_chip_get_value(sim, 3), ==,
+ GPIOD_LINE_VALUE_INACTIVE);
+}
+
+GPIOD_TEST_CASE(read_back_global_output_values)
+{
+ static const guint offsets[] = { 0, 1, 2, 3 };
+ static const enum gpiod_line_value values[] = {
+ GPIOD_LINE_VALUE_ACTIVE,
+ GPIOD_LINE_VALUE_INACTIVE,
+ GPIOD_LINE_VALUE_ACTIVE,
+ GPIOD_LINE_VALUE_INACTIVE,
+ };
+
+ g_autoptr(struct_gpiod_line_settings) settings = NULL;
+ g_autoptr(struct_gpiod_line_settings) retrieved = NULL;
+ g_autoptr(struct_gpiod_line_config) config = NULL;
+
+ settings = gpiod_test_create_line_settings_or_fail();
+ config = gpiod_test_create_line_config_or_fail();
+
+ gpiod_line_settings_set_direction(settings,
+ GPIOD_LINE_DIRECTION_OUTPUT);
+ gpiod_line_settings_set_output_value(settings, GPIOD_LINE_VALUE_ACTIVE);
+ gpiod_test_line_config_add_line_settings_or_fail(config, offsets, 4,
+ settings);
+ gpiod_test_line_config_set_output_values_or_fail(config, values, 4);
+
+ retrieved = gpiod_test_line_config_get_line_settings_or_fail(config, 1);
+ g_assert_cmpint(gpiod_line_settings_get_output_value(retrieved), ==,
+ GPIOD_LINE_VALUE_INACTIVE);
+}
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 10/16] core: provide gpiod_line_config_set_output_values()
2023-01-13 21:52 ` [libgpiod][PATCH 10/16] core: provide gpiod_line_config_set_output_values() Bartosz Golaszewski
@ 2023-01-16 0:15 ` Kent Gibson
2023-01-16 22:23 ` Bartosz Golaszewski
0 siblings, 1 reply; 41+ messages in thread
From: Kent Gibson @ 2023-01-16 0:15 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Andy Shevchenko, Viresh Kumar, linux-gpio,
Bartosz Golaszewski
On Fri, Jan 13, 2023 at 10:52:04PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> 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 <bartosz.golaszewski@linaro.org>
> ---
> 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(-)
>
> diff --git a/include/gpiod.h b/include/gpiod.h
> index 8cede47..c552135 100644
> --- a/include/gpiod.h
> +++ b/include/gpiod.h
> @@ -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.
> + *
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.
> + * 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.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [libgpiod][PATCH 10/16] core: provide gpiod_line_config_set_output_values()
2023-01-16 0:15 ` Kent Gibson
@ 2023-01-16 22:23 ` Bartosz Golaszewski
0 siblings, 0 replies; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-16 22:23 UTC (permalink / raw)
To: Kent Gibson
Cc: Linus Walleij, Andy Shevchenko, Viresh Kumar, linux-gpio,
Bartosz Golaszewski
On Mon, Jan 16, 2023 at 1:15 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Jan 13, 2023 at 10:52:04PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > 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 <bartosz.golaszewski@linaro.org>
> > ---
> > 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(-)
> >
> > diff --git a/include/gpiod.h b/include/gpiod.h
> > index 8cede47..c552135 100644
> > --- a/include/gpiod.h
> > +++ b/include/gpiod.h
> > @@ -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.
> > + *
>
> 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?
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.
>
> 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.
> > + * 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().
>
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [libgpiod][PATCH 11/16] gpioset: use gpiod_line_config_set_output_values()
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
` (9 preceding siblings ...)
2023-01-13 21:52 ` [libgpiod][PATCH 10/16] core: provide gpiod_line_config_set_output_values() Bartosz Golaszewski
@ 2023-01-13 21:52 ` Bartosz Golaszewski
2023-01-13 21:52 ` [libgpiod][PATCH 12/16] bindings: cxx: add line_config.set_output_values() Bartosz Golaszewski
` (4 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:52 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Use the new line config function to shrink the gpioset code and drop one
for loop.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
tools/gpioset.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/tools/gpioset.c b/tools/gpioset.c
index a871a16..1429d65 100644
--- a/tools/gpioset.c
+++ b/tools/gpioset.c
@@ -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)
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [libgpiod][PATCH 12/16] bindings: cxx: add line_config.set_output_values()
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
` (10 preceding siblings ...)
2023-01-13 21:52 ` [libgpiod][PATCH 11/16] gpioset: use gpiod_line_config_set_output_values() Bartosz Golaszewski
@ 2023-01-13 21:52 ` Bartosz Golaszewski
2023-01-14 11:20 ` Andy Shevchenko
2023-01-13 21:52 ` [libgpiod][PATCH 13/16] bindings: python: provide line_config.set_output_values() Bartosz Golaszewski
` (3 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:52 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Extend line_config to expose a new method - set_output_values() - which
wraps the new C function for setting multiple output values at once.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
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(+)
diff --git a/bindings/cxx/gpiodcxx/line-config.hpp b/bindings/cxx/gpiodcxx/line-config.hpp
index a917913..b76fdff 100644
--- a/bindings/cxx/gpiodcxx/line-config.hpp
+++ b/bindings/cxx/gpiodcxx/line-config.hpp
@@ -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.
diff --git a/bindings/cxx/internal.hpp b/bindings/cxx/internal.hpp
index 6aceac1..8322e12 100644
--- a/bindings/cxx/internal.hpp
+++ b/bindings/cxx/internal.hpp
@@ -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
{
diff --git a/bindings/cxx/line-config.cpp b/bindings/cxx/line-config.cpp
index 3ec99f0..233ba33 100644
--- a/bindings/cxx/line-config.cpp
+++ b/bindings/cxx/line-config.cpp
@@ -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(
diff --git a/bindings/cxx/line-settings.cpp b/bindings/cxx/line-settings.cpp
index 32f21a3..2159062 100644
--- a/bindings/cxx/line-settings.cpp
+++ b/bindings/cxx/line-settings.cpp
@@ -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())
{
diff --git a/bindings/cxx/tests/tests-line-config.cpp b/bindings/cxx/tests/tests-line-config.cpp
index 5fa0f94..5e439a1 100644
--- a/bindings/cxx/tests/tests-line-config.cpp
+++ b/bindings/cxx/tests/tests-line-config.cpp
@@ -4,12 +4,17 @@
#include <catch2/catch.hpp>
#include <gpiod.hpp>
+#include "gpiosim.hpp"
#include "helpers.hpp"
+using ::gpiosim::make_sim;
using namespace ::std::chrono_literals;
using direction = ::gpiod::line::direction;
using drive = ::gpiod::line::drive;
using edge = ::gpiod::line::edge;
+using simval = ::gpiosim::chip::value;
+using value = ::gpiod::line::value;
+using values = ::gpiod::line::values;
namespace {
@@ -72,6 +77,52 @@ TEST_CASE("line_config can be reset", "[line-config]")
REQUIRE(cfg.get_line_settings().size() == 0);
}
+TEST_CASE("output values can be set globally", "[line-config]")
+{
+ const values vals = { value::ACTIVE, value::INACTIVE, value::ACTIVE, value::INACTIVE };
+
+ auto sim = make_sim()
+ .set_num_lines(4)
+ .build();
+
+ ::gpiod::line_config cfg;
+
+ SECTION("request with globally set output values")
+ {
+ cfg
+ .add_line_settings(
+ {0, 1, 2, 3},
+ ::gpiod::line_settings().set_direction(direction::OUTPUT)
+ )
+ .set_output_values(vals);
+
+ auto request = ::gpiod::chip(sim.dev_path())
+ .prepare_request()
+ .set_line_config(cfg)
+ .do_request();
+
+ REQUIRE(sim.get_value(0) == simval::ACTIVE);
+ REQUIRE(sim.get_value(1) == simval::INACTIVE);
+ REQUIRE(sim.get_value(2) == simval::ACTIVE);
+ REQUIRE(sim.get_value(3) == simval::INACTIVE);
+ }
+
+ SECTION("read back global output values")
+ {
+ cfg
+ .add_line_settings(
+ {0, 1, 2, 3},
+ ::gpiod::line_settings()
+ .set_direction(direction::OUTPUT)
+ .set_output_value(value::ACTIVE)
+ )
+ .set_output_values(vals);
+
+ auto settings = cfg.get_line_settings()[1];
+ REQUIRE(settings.output_value() == value::INACTIVE);
+ }
+}
+
TEST_CASE("line_config stream insertion operator works", "[line-config]")
{
::gpiod::line_config cfg;
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 12/16] bindings: cxx: add line_config.set_output_values()
2023-01-13 21:52 ` [libgpiod][PATCH 12/16] bindings: cxx: add line_config.set_output_values() Bartosz Golaszewski
@ 2023-01-14 11:20 ` Andy Shevchenko
0 siblings, 0 replies; 41+ messages in thread
From: Andy Shevchenko @ 2023-01-14 11:20 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, Viresh Kumar, linux-gpio,
Bartosz Golaszewski
On Fri, Jan 13, 2023 at 10:52:06PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Extend line_config to 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.)
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 41+ messages in thread
* [libgpiod][PATCH 13/16] bindings: python: provide line_config.set_output_values()
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
` (11 preceding siblings ...)
2023-01-13 21:52 ` [libgpiod][PATCH 12/16] bindings: cxx: add line_config.set_output_values() Bartosz Golaszewski
@ 2023-01-13 21:52 ` Bartosz Golaszewski
2023-01-13 21:52 ` [libgpiod][PATCH 14/16] bindings: rust: make request_config optional in Chip.request_lines() Bartosz Golaszewski
` (2 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:52 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
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.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
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(+)
diff --git a/bindings/python/gpiod/chip.py b/bindings/python/gpiod/chip.py
index ad2eddd..4f5f9b4 100644
--- a/bindings/python/gpiod/chip.py
+++ b/bindings/python/gpiod/chip.py
@@ -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)
diff --git a/bindings/python/gpiod/ext/line-config.c b/bindings/python/gpiod/ext/line-config.c
index 173ca6b..0bba112 100644
--- a/bindings/python/gpiod/ext/line-config.c
+++ b/bindings/python/gpiod/ext/line-config.c
@@ -89,12 +89,76 @@ line_config_add_line_settings(line_config_object *self, PyObject *args)
Py_RETURN_NONE;
}
+static PyObject *
+line_config_set_output_values(line_config_object *self, PyObject *args)
+{
+ PyObject *values, *iter, *next, *val_stripped;
+ enum gpiod_line_value *valbuf;
+ Py_ssize_t num_values, pos;
+ int ret;
+
+ values = PyTuple_GetItem(args, 0);
+ if (!values)
+ return NULL;
+
+ num_values = PyObject_Size(values);
+ if (num_values < 0)
+ return NULL;
+
+ valbuf = PyMem_Calloc(num_values, sizeof(*valbuf));
+ if (!valbuf)
+ return PyErr_NoMemory();
+
+ iter = PyObject_GetIter(values);
+ if (!iter) {
+ PyMem_Free(valbuf);
+ return NULL;
+ }
+
+ for (pos = 0;; pos++) {
+ next = PyIter_Next(iter);
+ if (!next) {
+ Py_DECREF(iter);
+ break;
+ }
+
+ val_stripped = PyObject_GetAttrString(next, "value");
+ Py_DECREF(next);
+ if (!val_stripped) {
+ PyMem_Free(valbuf);
+ Py_DECREF(iter);
+ return NULL;
+ }
+
+ valbuf[pos] = PyLong_AsLong(val_stripped);
+ Py_DECREF(val_stripped);
+ if (PyErr_Occurred()) {
+ PyMem_Free(valbuf);
+ Py_DECREF(iter);
+ return NULL;
+ }
+ }
+
+ ret = gpiod_line_config_set_output_values(self->cfg,
+ valbuf, num_values);
+ PyMem_Free(valbuf);
+ if (ret)
+ return Py_gpiod_SetErrFromErrno();
+
+ Py_RETURN_NONE;
+}
+
static PyMethodDef line_config_methods[] = {
{
.ml_name = "add_line_settings",
.ml_meth = (PyCFunction)line_config_add_line_settings,
.ml_flags = METH_VARARGS,
},
+ {
+ .ml_name = "set_output_values",
+ .ml_meth = (PyCFunction)line_config_set_output_values,
+ .ml_flags = METH_VARARGS,
+ },
{ }
};
diff --git a/bindings/python/tests/tests_line_request.py b/bindings/python/tests/tests_line_request.py
index c0ac768..1dc2c71 100644
--- a/bindings/python/tests/tests_line_request.py
+++ b/bindings/python/tests/tests_line_request.py
@@ -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"})
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [libgpiod][PATCH 14/16] bindings: rust: make request_config optional in Chip.request_lines()
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
` (12 preceding siblings ...)
2023-01-13 21:52 ` [libgpiod][PATCH 13/16] bindings: python: provide line_config.set_output_values() Bartosz Golaszewski
@ 2023-01-13 21:52 ` Bartosz Golaszewski
2023-01-16 5:55 ` Viresh Kumar
2023-01-13 21:52 ` [libgpiod][PATCH 15/16] bindings: rust: make mutators return &mut self Bartosz Golaszewski
2023-01-13 21:52 ` [libgpiod][PATCH 16/16] bindings: rust: provide line_config.set_output_values() Bartosz Golaszewski
15 siblings, 1 reply; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:52 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
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.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
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(-)
diff --git a/bindings/rust/libgpiod/examples/gpio_events.rs b/bindings/rust/libgpiod/examples/gpio_events.rs
index 04267d9..cbdf1b5 100644
--- a/bindings/rust/libgpiod/examples/gpio_events.rs
+++ b/bindings/rust/libgpiod/examples/gpio_events.rs
@@ -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) {
diff --git a/bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs b/bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs
index e17f0f0..367b2f6 100644
--- a/bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs
+++ b/bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs
@@ -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
diff --git a/bindings/rust/libgpiod/examples/gpioget.rs b/bindings/rust/libgpiod/examples/gpioget.rs
index 6e60833..74baf30 100644
--- a/bindings/rust/libgpiod/examples/gpioget.rs
+++ b/bindings/rust/libgpiod/examples/gpioget.rs
@@ -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);
diff --git a/bindings/rust/libgpiod/examples/gpiomon.rs b/bindings/rust/libgpiod/examples/gpiomon.rs
index f17a81f..a09ddfc 100644
--- a/bindings/rust/libgpiod/examples/gpiomon.rs
+++ b/bindings/rust/libgpiod/examples/gpiomon.rs
@@ -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) {
diff --git a/bindings/rust/libgpiod/examples/gpioset.rs b/bindings/rust/libgpiod/examples/gpioset.rs
index 875a3ad..6247996 100644
--- a/bindings/rust/libgpiod/examples/gpioset.rs
+++ b/bindings/rust/libgpiod/examples/gpioset.rs
@@ -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();
diff --git a/bindings/rust/libgpiod/src/chip.rs b/bindings/rust/libgpiod/src/chip.rs
index 91b4c94..9c3c2b4 100644
--- a/bindings/rust/libgpiod/src/chip.rs
+++ b/bindings/rust/libgpiod/src/chip.rs
@@ -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() {
diff --git a/bindings/rust/libgpiod/tests/common/config.rs b/bindings/rust/libgpiod/tests/common/config.rs
index 842a70a..b838b66 100644
--- a/bindings/rust/libgpiod/tests/common/config.rs
+++ b/bindings/rust/libgpiod/tests/common/config.rs
@@ -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(())
diff --git a/bindings/rust/libgpiod/tests/info_event.rs b/bindings/rust/libgpiod/tests/info_event.rs
index bfa0058..6bf7a0f 100644
--- a/bindings/rust/libgpiod/tests/info_event.rs
+++ b/bindings/rust/libgpiod/tests/info_event.rs
@@ -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
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 14/16] bindings: rust: make request_config optional in Chip.request_lines()
2023-01-13 21:52 ` [libgpiod][PATCH 14/16] bindings: rust: make request_config optional in Chip.request_lines() Bartosz Golaszewski
@ 2023-01-16 5:55 ` Viresh Kumar
0 siblings, 0 replies; 41+ messages in thread
From: Viresh Kumar @ 2023-01-16 5:55 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
On 13-01-23, 22:52, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> 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.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> 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(-)
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply [flat|nested] 41+ messages in thread
* [libgpiod][PATCH 15/16] bindings: rust: make mutators return &mut self
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
` (13 preceding siblings ...)
2023-01-13 21:52 ` [libgpiod][PATCH 14/16] bindings: rust: make request_config optional in Chip.request_lines() Bartosz Golaszewski
@ 2023-01-13 21:52 ` Bartosz Golaszewski
2023-01-16 6:02 ` Viresh Kumar
2023-01-13 21:52 ` [libgpiod][PATCH 16/16] bindings: rust: provide line_config.set_output_values() Bartosz Golaszewski
15 siblings, 1 reply; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:52 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
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.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
.../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(-)
diff --git a/bindings/rust/libgpiod/examples/gpio_events.rs b/bindings/rust/libgpiod/examples/gpio_events.rs
index cbdf1b5..b26c60b 100644
--- a/bindings/rust/libgpiod/examples/gpio_events.rs
+++ b/bindings/rust/libgpiod/examples/gpio_events.rs
@@ -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 arg in &args[2..] {
diff --git a/bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs b/bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs
index 367b2f6..620f4ec 100644
--- a/bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs
+++ b/bindings/rust/libgpiod/examples/gpio_threaded_info_events.rs
@@ -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();
diff --git a/bindings/rust/libgpiod/examples/gpioget.rs b/bindings/rust/libgpiod/examples/gpioget.rs
index 74baf30..a71612b 100644
--- a/bindings/rust/libgpiod/examples/gpioget.rs
+++ b/bindings/rust/libgpiod/examples/gpioget.rs
@@ -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 arg in &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)?;
diff --git a/bindings/rust/libgpiod/examples/gpiomon.rs b/bindings/rust/libgpiod/examples/gpiomon.rs
index a09ddfc..8f2a71a 100644
--- a/bindings/rust/libgpiod/examples/gpiomon.rs
+++ b/bindings/rust/libgpiod/examples/gpiomon.rs
@@ -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 arg in &args[2..] {
diff --git a/bindings/rust/libgpiod/examples/gpioset.rs b/bindings/rust/libgpiod/examples/gpioset.rs
index 6247996..4b43010 100644
--- a/bindings/rust/libgpiod/examples/gpioset.rs
+++ b/bindings/rust/libgpiod/examples/gpioset.rs
@@ -24,7 +24,7 @@ fn main() -> Result<()> {
return Err(Error::InvalidArguments);
}
- let lconfig = line::Config::new()?;
+ let mut lconfig = line::Config::new()?;
for arg in &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)?;
diff --git a/bindings/rust/libgpiod/src/line_config.rs b/bindings/rust/libgpiod/src/line_config.rs
index 0c8b293..2169bf1 100644
--- a/bindings/rust/libgpiod/src/line_config.rs
+++ b/bindings/rust/libgpiod/src/line_config.rs
@@ -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)
}
}
diff --git a/bindings/rust/libgpiod/src/line_request.rs b/bindings/rust/libgpiod/src/line_request.rs
index b843862..8ad8e25 100644
--- a/bindings/rust/libgpiod/src/line_request.rs
+++ b/bindings/rust/libgpiod/src/line_request.rs
@@ -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)
}
}
diff --git a/bindings/rust/libgpiod/src/request_config.rs b/bindings/rust/libgpiod/src/request_config.rs
index 9d38548..939838c 100644
--- a/bindings/rust/libgpiod/src/request_config.rs
+++ b/bindings/rust/libgpiod/src/request_config.rs
@@ -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, size as c_ulong) }
+
+ self
}
/// Get the edge event buffer size setting for the request config.
diff --git a/bindings/rust/libgpiod/tests/common/config.rs b/bindings/rust/libgpiod/tests/common/config.rs
index b838b66..36ccc94 100644
--- a/bindings/rust/libgpiod/tests/common/config.rs
+++ b/bindings/rust/libgpiod/tests/common/config.rs
@@ -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()
}
}
diff --git a/bindings/rust/libgpiod/tests/info_event.rs b/bindings/rust/libgpiod/tests/info_event.rs
index 6bf7a0f..f06dd2d 100644
--- a/bindings/rust/libgpiod/tests/info_event.rs
+++ b/bindings/rust/libgpiod/tests/info_event.rs
@@ -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();
diff --git a/bindings/rust/libgpiod/tests/line_config.rs b/bindings/rust/libgpiod/tests/line_config.rs
index 95f2178..92a7af3 100644
--- a/bindings/rust/libgpiod/tests/line_config.rs
+++ b/bindings/rust/libgpiod/tests/line_config.rs
@@ -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();
diff --git a/bindings/rust/libgpiod/tests/line_request.rs b/bindings/rust/libgpiod/tests/line_request.rs
index c3fc37b..561f4e8 100644
--- a/bindings/rust/libgpiod/tests/line_request.rs
+++ b/bindings/rust/libgpiod/tests/line_request.rs
@@ -97,13 +97,11 @@ mod line_request {
config.lconfig_add_settings(&offsets);
config.request_lines().unwrap();
- let request = config.request();
-
// Single values read properly
- assert_eq!(request.value(7).unwrap(), Value::Active);
+ assert_eq!(config.request().value(7).unwrap(), Value::Active);
// Values read properly
- let map = request.values().unwrap();
+ let map = config.request().values().unwrap();
for i in 0..offsets.len() {
assert_eq!(
*map.get(offsets[i].into()).unwrap(),
@@ -115,7 +113,7 @@ mod line_request {
}
// Subset of values read properly
- let map = request.values_subset(&[2, 0, 6]).unwrap();
+ let map = config.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,10 +121,10 @@ mod line_request {
// Value read properly after reconfigure
let mut lsettings = line::Settings::new().unwrap();
lsettings.set_active_low(true);
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
lconfig.add_line_settings(&offsets, lsettings).unwrap();
- request.reconfigure_lines(&lconfig).unwrap();
- assert_eq!(request.value(7).unwrap(), Value::InActive);
+ config.request().reconfigure_lines(&lconfig).unwrap();
+ assert_eq!(config.request().value(7).unwrap(), Value::InActive);
}
#[test]
@@ -153,22 +151,21 @@ mod line_request {
config.lconfig_val(Some(Direction::Output), Some(Value::InActive));
config.lconfig_add_settings(&offsets);
config.request_lines().unwrap();
- let request = config.request();
// Set single value
- request.set_value(1, Value::Active).unwrap();
+ config.request().set_value(1, Value::Active).unwrap();
assert_eq!(config.sim_val(0).unwrap(), SimValue::InActive);
assert_eq!(config.sim_val(1).unwrap(), SimValue::Active);
assert_eq!(config.sim_val(3).unwrap(), SimValue::InActive);
assert_eq!(config.sim_val(4).unwrap(), SimValue::InActive);
- request.set_value(1, Value::InActive).unwrap();
+ config.request().set_value(1, Value::InActive).unwrap();
assert_eq!(config.sim_val(1).unwrap(), SimValue::InActive);
// Set values of subset
let mut map = ValueMap::new();
map.insert(4, Value::Active);
map.insert(3, Value::Active);
- request.set_values_subset(map).unwrap();
+ config.request().set_values_subset(map).unwrap();
assert_eq!(config.sim_val(0).unwrap(), SimValue::InActive);
assert_eq!(config.sim_val(1).unwrap(), SimValue::InActive);
assert_eq!(config.sim_val(3).unwrap(), SimValue::Active);
@@ -177,12 +174,12 @@ mod line_request {
let mut map = ValueMap::new();
map.insert(4, Value::InActive);
map.insert(3, Value::InActive);
- request.set_values_subset(map).unwrap();
+ config.request().set_values_subset(map).unwrap();
assert_eq!(config.sim_val(3).unwrap(), SimValue::InActive);
assert_eq!(config.sim_val(4).unwrap(), SimValue::InActive);
// Set all values
- request
+ config.request()
.set_values(&[
Value::Active,
Value::InActive,
@@ -194,7 +191,7 @@ mod line_request {
assert_eq!(config.sim_val(1).unwrap(), SimValue::InActive);
assert_eq!(config.sim_val(3).unwrap(), SimValue::Active);
assert_eq!(config.sim_val(4).unwrap(), SimValue::InActive);
- request
+ config.request()
.set_values(&[
Value::InActive,
Value::InActive,
@@ -246,12 +243,10 @@ mod line_request {
config.lconfig_add_settings(&offsets);
config.request_lines().unwrap();
- let request = config.request();
-
// Reconfigure
let mut lsettings = line::Settings::new().unwrap();
lsettings.set_direction(Direction::Input).unwrap();
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
// The uAPI config has only 10 attribute slots, this should pass.
for offset in offsets {
@@ -261,7 +256,7 @@ mod line_request {
.unwrap();
}
- assert!(request.reconfigure_lines(&lconfig).is_ok());
+ assert!(config.request().reconfigure_lines(&lconfig).is_ok());
// The uAPI config has only 10 attribute slots, and this is the 11th entry.
// This should fail with E2BIG.
@@ -269,7 +264,7 @@ mod line_request {
lconfig.add_line_settings(&[11], lsettings).unwrap();
assert_eq!(
- request.reconfigure_lines(&lconfig).unwrap_err(),
+ config.request().reconfigure_lines(&lconfig).unwrap_err(),
ChipError::OperationFailed(
OperationType::LineRequestReconfigLines,
errno::Errno(E2BIG),
@@ -285,10 +280,8 @@ mod line_request {
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.bias().unwrap(), None);
- let request = config.request();
-
// Reconfigure
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
let mut lsettings = line::Settings::new().unwrap();
lsettings
.set_prop(&[
@@ -297,11 +290,11 @@ mod line_request {
])
.unwrap();
lconfig.add_line_settings(&[0], lsettings).unwrap();
- request.reconfigure_lines(&lconfig).unwrap();
+ config.request().reconfigure_lines(&lconfig).unwrap();
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.bias().unwrap(), Some(Bias::PullUp));
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
let mut lsettings = line::Settings::new().unwrap();
lsettings
.set_prop(&[
@@ -310,11 +303,11 @@ mod line_request {
])
.unwrap();
lconfig.add_line_settings(&[0], lsettings).unwrap();
- request.reconfigure_lines(&lconfig).unwrap();
+ config.request().reconfigure_lines(&lconfig).unwrap();
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.bias().unwrap(), Some(Bias::PullDown));
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
let mut lsettings = line::Settings::new().unwrap();
lsettings
.set_prop(&[
@@ -323,7 +316,7 @@ mod line_request {
])
.unwrap();
lconfig.add_line_settings(&[0], lsettings).unwrap();
- request.reconfigure_lines(&lconfig).unwrap();
+ config.request().reconfigure_lines(&lconfig).unwrap();
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.bias().unwrap(), Some(Bias::Disabled));
}
@@ -336,10 +329,8 @@ mod line_request {
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.drive().unwrap(), Drive::PushPull);
- let request = config.request();
-
// Reconfigure
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
let mut lsettings = line::Settings::new().unwrap();
lsettings
.set_prop(&[
@@ -348,11 +339,11 @@ mod line_request {
])
.unwrap();
lconfig.add_line_settings(&[0], lsettings).unwrap();
- request.reconfigure_lines(&lconfig).unwrap();
+ config.request().reconfigure_lines(&lconfig).unwrap();
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.drive().unwrap(), Drive::PushPull);
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
let mut lsettings = line::Settings::new().unwrap();
lsettings
.set_prop(&[
@@ -361,11 +352,11 @@ mod line_request {
])
.unwrap();
lconfig.add_line_settings(&[0], lsettings).unwrap();
- request.reconfigure_lines(&lconfig).unwrap();
+ config.request().reconfigure_lines(&lconfig).unwrap();
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.drive().unwrap(), Drive::OpenDrain);
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
let mut lsettings = line::Settings::new().unwrap();
lsettings
.set_prop(&[
@@ -374,7 +365,7 @@ mod line_request {
])
.unwrap();
lconfig.add_line_settings(&[0], lsettings).unwrap();
- request.reconfigure_lines(&lconfig).unwrap();
+ config.request().reconfigure_lines(&lconfig).unwrap();
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.drive().unwrap(), Drive::OpenSource);
}
@@ -387,10 +378,8 @@ mod line_request {
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.edge_detection().unwrap(), None);
- let request = config.request();
-
// Reconfigure
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
let mut lsettings = line::Settings::new().unwrap();
lsettings
.set_prop(&[
@@ -399,11 +388,11 @@ mod line_request {
])
.unwrap();
lconfig.add_line_settings(&[0], lsettings).unwrap();
- request.reconfigure_lines(&lconfig).unwrap();
+ config.request().reconfigure_lines(&lconfig).unwrap();
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.edge_detection().unwrap(), Some(Edge::Both));
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
let mut lsettings = line::Settings::new().unwrap();
lsettings
.set_prop(&[
@@ -412,11 +401,11 @@ mod line_request {
])
.unwrap();
lconfig.add_line_settings(&[0], lsettings).unwrap();
- request.reconfigure_lines(&lconfig).unwrap();
+ config.request().reconfigure_lines(&lconfig).unwrap();
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.edge_detection().unwrap(), Some(Edge::Rising));
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
let mut lsettings = line::Settings::new().unwrap();
lsettings
.set_prop(&[
@@ -425,7 +414,7 @@ mod line_request {
])
.unwrap();
lconfig.add_line_settings(&[0], lsettings).unwrap();
- request.reconfigure_lines(&lconfig).unwrap();
+ config.request().reconfigure_lines(&lconfig).unwrap();
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.edge_detection().unwrap(), Some(Edge::Falling));
}
@@ -438,22 +427,20 @@ mod line_request {
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.event_clock().unwrap(), EventClock::Monotonic);
- let request = config.request();
-
// Reconfigure
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
let mut lsettings = line::Settings::new().unwrap();
lsettings.set_event_clock(EventClock::Monotonic).unwrap();
lconfig.add_line_settings(&[0], lsettings).unwrap();
- request.reconfigure_lines(&lconfig).unwrap();
+ config.request().reconfigure_lines(&lconfig).unwrap();
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.event_clock().unwrap(), EventClock::Monotonic);
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
let mut lsettings = line::Settings::new().unwrap();
lsettings.set_event_clock(EventClock::Realtime).unwrap();
lconfig.add_line_settings(&[0], lsettings).unwrap();
- request.reconfigure_lines(&lconfig).unwrap();
+ config.request().reconfigure_lines(&lconfig).unwrap();
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.event_clock().unwrap(), EventClock::Realtime);
}
@@ -467,14 +454,12 @@ mod line_request {
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.event_clock().unwrap(), EventClock::Monotonic);
- let request = config.request();
-
// Reconfigure
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
let mut lsettings = line::Settings::new().unwrap();
lsettings.set_event_clock(EventClock::HTE).unwrap();
lconfig.add_line_settings(&[0], lsettings).unwrap();
- request.reconfigure_lines(&lconfig).unwrap();
+ config.request().reconfigure_lines(&lconfig).unwrap();
let info = config.chip().line_info(0).unwrap();
assert_eq!(info.event_clock().unwrap(), EventClock::HTE);
}
@@ -488,10 +473,8 @@ mod line_request {
assert!(!info.is_debounced());
assert_eq!(info.debounce_period(), Duration::from_millis(0));
- let request = config.request();
-
// Reconfigure
- let lconfig = line::Config::new().unwrap();
+ let mut lconfig = line::Config::new().unwrap();
let mut lsettings = line::Settings::new().unwrap();
lsettings
.set_prop(&[
@@ -500,7 +483,7 @@ mod line_request {
])
.unwrap();
lconfig.add_line_settings(&[0], lsettings).unwrap();
- request.reconfigure_lines(&lconfig).unwrap();
+ config.request().reconfigure_lines(&lconfig).unwrap();
let info = config.chip().line_info(0).unwrap();
assert!(info.is_debounced());
assert_eq!(info.debounce_period(), Duration::from_millis(100));
diff --git a/bindings/rust/libgpiod/tests/request_config.rs b/bindings/rust/libgpiod/tests/request_config.rs
index 8c67638..d78c4bd 100644
--- a/bindings/rust/libgpiod/tests/request_config.rs
+++ b/bindings/rust/libgpiod/tests/request_config.rs
@@ -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);
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 15/16] bindings: rust: make mutators return &mut self
2023-01-13 21:52 ` [libgpiod][PATCH 15/16] bindings: rust: make mutators return &mut self Bartosz Golaszewski
@ 2023-01-16 6:02 ` Viresh Kumar
2023-01-16 8:42 ` Bartosz Golaszewski
0 siblings, 1 reply; 41+ messages in thread
From: Viresh Kumar @ 2023-01-16 6:02 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
On 13-01-23, 22:52, Bartosz Golaszewski wrote:
> diff --git a/bindings/rust/libgpiod/tests/line_request.rs b/bindings/rust/libgpiod/tests/line_request.rs
> index c3fc37b..561f4e8 100644
> --- a/bindings/rust/libgpiod/tests/line_request.rs
> +++ b/bindings/rust/libgpiod/tests/line_request.rs
> @@ -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 ?
--
viresh
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 15/16] bindings: rust: make mutators return &mut self
2023-01-16 6:02 ` Viresh Kumar
@ 2023-01-16 8:42 ` Bartosz Golaszewski
2023-01-16 9:40 ` Viresh Kumar
0 siblings, 1 reply; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-16 8:42 UTC (permalink / raw)
To: Viresh Kumar
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
On Mon, Jan 16, 2023 at 7:02 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 13-01-23, 22:52, Bartosz Golaszewski wrote:
> > diff --git a/bindings/rust/libgpiod/tests/line_request.rs b/bindings/rust/libgpiod/tests/line_request.rs
> > index c3fc37b..561f4e8 100644
> > --- a/bindings/rust/libgpiod/tests/line_request.rs
> > +++ b/bindings/rust/libgpiod/tests/line_request.rs
> > @@ -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 ?
>
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
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 15/16] bindings: rust: make mutators return &mut self
2023-01-16 8:42 ` Bartosz Golaszewski
@ 2023-01-16 9:40 ` Viresh Kumar
2023-01-16 12:57 ` Bartosz Golaszewski
0 siblings, 1 reply; 41+ messages in thread
From: Viresh Kumar @ 2023-01-16 9:40 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
On 16-01-23, 09:42, Bartosz Golaszewski wrote:
> On Mon, Jan 16, 2023 at 7:02 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 13-01-23, 22:52, Bartosz Golaszewski wrote:
> > > diff --git a/bindings/rust/libgpiod/tests/line_request.rs b/bindings/rust/libgpiod/tests/line_request.rs
> > > index c3fc37b..561f4e8 100644
> > > --- a/bindings/rust/libgpiod/tests/line_request.rs
> > > +++ b/bindings/rust/libgpiod/tests/line_request.rs
> > > @@ -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 ?
> >
>
> 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.
diff --git a/bindings/rust/libgpiod/tests/line_request.rs b/bindings/rust/libgpiod/tests/line_request.rs
index 561f4e80d189..5644292d486a 100644
--- a/bindings/rust/libgpiod/tests/line_request.rs
+++ b/bindings/rust/libgpiod/tests/line_request.rs
@@ -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 i in 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]
--
viresh
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 15/16] bindings: rust: make mutators return &mut self
2023-01-16 9:40 ` Viresh Kumar
@ 2023-01-16 12:57 ` Bartosz Golaszewski
2023-01-17 5:19 ` Viresh Kumar
0 siblings, 1 reply; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-16 12:57 UTC (permalink / raw)
To: Viresh Kumar
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
On Mon, Jan 16, 2023 at 10:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 16-01-23, 09:42, Bartosz Golaszewski wrote:
> > On Mon, Jan 16, 2023 at 7:02 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 13-01-23, 22:52, Bartosz Golaszewski wrote:
> > > > diff --git a/bindings/rust/libgpiod/tests/line_request.rs b/bindings/rust/libgpiod/tests/line_request.rs
> > > > index c3fc37b..561f4e8 100644
> > > > --- a/bindings/rust/libgpiod/tests/line_request.rs
> > > > +++ b/bindings/rust/libgpiod/tests/line_request.rs
> > > > @@ -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 ?
> > >
> >
> > 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 borrow
later 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 borrow
occurs here
...
449 | let info = config.chip().line_info(0).unwrap();
| ^^^^^^^^^^^^^ second mutable borrow occurs here
...
456 | request.reconfigure_lines(&lconfig).unwrap();
| ----------------------------------- first borrow
later used here
Not sure how to go about this.
Bart
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 15/16] bindings: rust: make mutators return &mut self
2023-01-16 12:57 ` Bartosz Golaszewski
@ 2023-01-17 5:19 ` Viresh Kumar
0 siblings, 0 replies; 41+ messages in thread
From: Viresh Kumar @ 2023-01-17 5:19 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
On 16-01-23, 13:57, Bartosz Golaszewski wrote:
> 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 borrow
> later 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 borrow
> occurs here
> ...
> 449 | let info = config.chip().line_info(0).unwrap();
> | ^^^^^^^^^^^^^ second mutable borrow occurs here
> ...
> 456 | request.reconfigure_lines(&lconfig).unwrap();
> | ----------------------------------- first borrow
> later 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().
--
viresh
^ permalink raw reply [flat|nested] 41+ messages in thread
* [libgpiod][PATCH 16/16] bindings: rust: provide line_config.set_output_values()
2023-01-13 21:51 [libgpiod][PATCH 00/16] treewide: continue beating libgpiod v2 into shape for an upcoming release Bartosz Golaszewski
` (14 preceding siblings ...)
2023-01-13 21:52 ` [libgpiod][PATCH 15/16] bindings: rust: make mutators return &mut self Bartosz Golaszewski
@ 2023-01-13 21:52 ` Bartosz Golaszewski
2023-01-16 6:09 ` Viresh Kumar
15 siblings, 1 reply; 41+ messages in thread
From: Bartosz Golaszewski @ 2023-01-13 21:52 UTC (permalink / raw)
To: Kent Gibson, Linus Walleij, Andy Shevchenko, Viresh Kumar
Cc: linux-gpio, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
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.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
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(-)
diff --git a/bindings/rust/libgpiod/src/lib.rs b/bindings/rust/libgpiod/src/lib.rs
index a5d018c..f268ceb 100644
--- a/bindings/rust/libgpiod/src/lib.rs
+++ b/bindings/rust/libgpiod/src/lib.rs
@@ -71,6 +71,7 @@ pub enum OperationType {
InfoEventGetLineInfo,
LineConfigNew,
LineConfigAddSettings,
+ LineConfigSetOutputValues,
LineConfigGetOffsets,
LineConfigGetSettings,
LineRequestReconfigLines,
diff --git a/bindings/rust/libgpiod/src/line_config.rs b/bindings/rust/libgpiod/src/line_config.rs
index 2169bf1..b276cf0 100644
--- a/bindings/rust/libgpiod/src/line_config.rs
+++ b/bindings/rust/libgpiod/src/line_config.rs
@@ -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 value in 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();
diff --git a/bindings/rust/libgpiod/tests/line_config.rs b/bindings/rust/libgpiod/tests/line_config.rs
index 92a7af3..96ce127 100644
--- a/bindings/rust/libgpiod/tests/line_config.rs
+++ b/bindings/rust/libgpiod/tests/line_config.rs
@@ -5,9 +5,11 @@
mod common;
mod line_config {
+ use libgpiod::chip::Chip;
use libgpiod::line::{
self, Bias, Direction, Drive, Edge, EventClock, SettingKind, SettingVal, Value,
};
+ use gpiosim_sys::Sim;
#[test]
fn settings() {
@@ -76,4 +78,64 @@ mod line_config {
SettingVal::OutputValue(Value::Active)
);
}
+
+ #[test]
+ fn set_global_output_values() {
+ let sim = Sim::new(Some(4), None, true).unwrap();
+ let mut settings = line::Settings::new().unwrap();
+ settings.set_direction(Direction::Output).unwrap();
+
+ let mut config = line::Config::new().unwrap();
+ config
+ .add_line_settings(&[0, 1, 2, 3], settings)
+ .unwrap()
+ .set_output_values(&[
+ Value::Active,
+ Value::InActive,
+ Value::Active,
+ Value::InActive
+ ])
+ .unwrap();
+
+ let chip = Chip::open(&sim.dev_path()).unwrap();
+ let _request = chip.request_lines(None, &config);
+
+ assert_eq!(sim.val(0).unwrap(), gpiosim_sys::Value::Active);
+ assert_eq!(sim.val(1).unwrap(), gpiosim_sys::Value::InActive);
+ assert_eq!(sim.val(2).unwrap(), gpiosim_sys::Value::Active);
+ assert_eq!(sim.val(3).unwrap(), gpiosim_sys::Value::InActive);
+ }
+
+ #[test]
+ fn read_back_global_output_values() {
+ let mut settings = line::Settings::new().unwrap();
+ settings
+ .set_direction(Direction::Output)
+ .unwrap()
+ .set_output_value(Value::Active)
+ .unwrap();
+
+ let mut config = line::Config::new().unwrap();
+ config
+ .add_line_settings(&[0, 1, 2, 3], settings)
+ .unwrap()
+ .set_output_values(&[
+ Value::Active,
+ Value::InActive,
+ Value::Active,
+ Value::InActive,
+ ])
+ .unwrap();
+
+ assert_eq!(
+ config
+ .line_settings()
+ .unwrap()
+ .get(&1)
+ .unwrap()
+ .output_value()
+ .unwrap(),
+ Value::InActive
+ );
+ }
}
--
2.37.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [libgpiod][PATCH 16/16] bindings: rust: provide line_config.set_output_values()
2023-01-13 21:52 ` [libgpiod][PATCH 16/16] bindings: rust: provide line_config.set_output_values() Bartosz Golaszewski
@ 2023-01-16 6:09 ` Viresh Kumar
0 siblings, 0 replies; 41+ messages in thread
From: Viresh Kumar @ 2023-01-16 6:09 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Linus Walleij, Andy Shevchenko, linux-gpio,
Bartosz Golaszewski
On 13-01-23, 22:52, Bartosz Golaszewski wrote:
> diff --git a/bindings/rust/libgpiod/src/line_config.rs b/bindings/rust/libgpiod/src/line_config.rs
>
> + /// 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 value in 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();
> +
> + 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)
> + }
> + }
--
viresh
^ permalink raw reply [flat|nested] 41+ messages in thread