diff mbox series

[libgpiod] libgpiod: Allow to gracefully exit from a wait event

Message ID 20231113143219.43498-1-zinger.ad@gmail.com
State New
Headers show
Series [libgpiod] libgpiod: Allow to gracefully exit from a wait event | expand

Commit Message

Adrien Zinger Nov. 13, 2023, 2:32 p.m. UTC 
Add a function in core `gpiod_line_request_wait_edge_events_or` with an
additional argument `fd` to trigger ppoll manually from another thread.

It allows users to gracefully cancel and join a worker thread waiting
for an edge event.

Signed-off-by: Adrien Zinger <zinger.ad@gmail.com>
---
 include/gpiod.h          | 17 ++++++++++++++
 lib/internal.c           | 38 ++++++++++++++++++++++++------
 lib/internal.h           |  1 +
 lib/line-request.c       |  9 +++++++
 tests/tests-edge-event.c | 51 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 109 insertions(+), 7 deletions(-)

Comments

Erik Schilling Nov. 13, 2023, 5:39 p.m. UTC | #1 
On Mon Nov 13, 2023 at 3:32 PM CET, Adrien Zinger wrote:
> Add a function in core `gpiod_line_request_wait_edge_events_or` with an
> additional argument `fd` to trigger ppoll manually from another thread.
>
> It allows users to gracefully cancel and join a worker thread waiting
> for an edge event.
>
> Signed-off-by: Adrien Zinger <zinger.ad@gmail.com>
> ---
>  include/gpiod.h          | 17 ++++++++++++++
>  lib/internal.c           | 38 ++++++++++++++++++++++++------
>  lib/internal.h           |  1 +
>  lib/line-request.c       |  9 +++++++
>  tests/tests-edge-event.c | 51 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 109 insertions(+), 7 deletions(-)
>
> diff --git a/include/gpiod.h b/include/gpiod.h
> index d86c6ac..cbc83f9 100644
> --- a/include/gpiod.h
> +++ b/include/gpiod.h
> @@ -1176,6 +1176,23 @@ int gpiod_line_request_get_fd(struct gpiod_line_request *request);
>  int gpiod_line_request_wait_edge_events(struct gpiod_line_request *request,
>  					int64_t timeout_ns);
>  
> +/**
> + * @brief Wait for edge events on any of the requested lines or a
> + *        POLLHUP/POLLERR event on the given file descriptor.
> + * @param request GPIO line request.
> + * @param fd file descriptor from any I/O, channel, fifo, or pipe.
> + * @param timeout_ns Wait time limit in nanoseconds. If set to 0, the function
> + *                   returns immediately. If set to a negative number, the
> + *                   function blocks indefinitely until an event becomes
> + *                   available.
> + * @return 0 if wait timed out, -1 if an error occurred, 1 if an event is
> + *         pending, 2 if the file descriptor raised an event.
> + *
> + * Lines must have edge detection set for edge events to be emitted.
> + * By default edge detection is disabled.
> + */
> +int gpiod_line_request_wait_edge_events_or(struct gpiod_line_request *request,
> +					int fd, int64_t timeout_ns);

This sounds like an oddly specific API... I wonder, why does
gpiod_line_request_get_fd [1] + doing the polling in your code not work
for you? Since you already got a file descriptor for the notification
that seems like little extra work. Or did you consider that but find it
too verbose?

[1] https://libgpiod.readthedocs.io/en/latest/group__line__request.html#ga5c0dbdcd8608b76e77b78bca9a6b03d7

- Erik
Adrien Zinger Nov. 14, 2023, 10:09 a.m. UTC | #2 
> This sounds like an oddly specific API... I wonder, why does
> gpiod_line_request_get_fd [1] + doing the polling in your code
> not work
> for you? Since you already got a file descriptor for the
> notification that seems like little extra work. Or
> did you consider that but find
> it too verbose?

Until now it works like you said and it's not so verbose.

Actually I had the issue some weeks ago. And before digging inside the
library I have searched in the documentation and also asked on
stackoverflow [1] for a suggestion. Despite everything me and other
developers were not able to guess how to cancel a wait event from
another thread, without reading the code of the library. I thought
that it could be a good thing to provide that feature.

[1]
https://stackoverflow.com/questions/77222869/how-to-gracefully-cancel-gpiod-line-event-wait

Best regards,
Adrien
Bartosz Golaszewski Nov. 15, 2023, 5:10 p.m. UTC | #3 
On Tue, Nov 14, 2023 at 11:10 AM Adrien Zinger <zinger.ad@gmail.com> wrote:
>
> > This sounds like an oddly specific API... I wonder, why does
> > gpiod_line_request_get_fd [1] + doing the polling in your code
> > not work
> > for you? Since you already got a file descriptor for the
> > notification that seems like little extra work. Or
> > did you consider that but find
> > it too verbose?
>
> Until now it works like you said and it's not so verbose.
>
> Actually I had the issue some weeks ago. And before digging inside the
> library I have searched in the documentation and also asked on
> stackoverflow [1] for a suggestion. Despite everything me and other
> developers were not able to guess how to cancel a wait event from
> another thread, without reading the code of the library. I thought
> that it could be a good thing to provide that feature.
>
> [1]
> https://stackoverflow.com/questions/77222869/how-to-gracefully-cancel-gpiod-line-event-wait
>
> Best regards,
> Adrien
>
>

I agree with Erik, this looks like a very specific (obscure?) use-case
that's best implemented in-place using the fd getter. I try to keep
the library minimal and simple and this doesn't seem like a good fit
IMO.

Bart
diff mbox series

Patch

diff --git a/include/gpiod.h b/include/gpiod.h
index d86c6ac..cbc83f9 100644
--- a/include/gpiod.h
+++ b/include/gpiod.h
@@ -1176,6 +1176,23 @@  int gpiod_line_request_get_fd(struct gpiod_line_request *request);
 int gpiod_line_request_wait_edge_events(struct gpiod_line_request *request,
 					int64_t timeout_ns);
 
+/**
+ * @brief Wait for edge events on any of the requested lines or a
+ *        POLLHUP/POLLERR event on the given file descriptor.
+ * @param request GPIO line request.
+ * @param fd file descriptor from any I/O, channel, fifo, or pipe.
+ * @param timeout_ns Wait time limit in nanoseconds. If set to 0, the function
+ *                   returns immediately. If set to a negative number, the
+ *                   function blocks indefinitely until an event becomes
+ *                   available.
+ * @return 0 if wait timed out, -1 if an error occurred, 1 if an event is
+ *         pending, 2 if the file descriptor raised an event.
+ *
+ * Lines must have edge detection set for edge events to be emitted.
+ * By default edge detection is disabled.
+ */
+int gpiod_line_request_wait_edge_events_or(struct gpiod_line_request *request,
+					int fd, int64_t timeout_ns);
 /**
  * @brief Read a number of edge events from a line request.
  * @param request GPIO line request.
diff --git a/lib/internal.c b/lib/internal.c
index c80d01f..aaf96ff 100644
--- a/lib/internal.c
+++ b/lib/internal.c
@@ -81,22 +81,18 @@  out:
 	return ret;
 }
 
-int gpiod_poll_fd(int fd, int64_t timeout_ns)
+static int
+gpiod_poll_fds(struct pollfd *pfds, uint8_t len, int64_t timeout_ns)
 {
 	struct timespec ts;
-	struct pollfd pfd;
 	int ret;
 
-	memset(&pfd, 0, sizeof(pfd));
-	pfd.fd = fd;
-	pfd.events = POLLIN | POLLPRI;
-
 	if (timeout_ns >= 0) {
 		ts.tv_sec = timeout_ns / 1000000000ULL;
 		ts.tv_nsec = timeout_ns % 1000000000ULL;
 	}
 
-	ret = ppoll(&pfd, 1, timeout_ns < 0 ? NULL : &ts, NULL);
+	ret = ppoll(pfds, len, timeout_ns < 0 ? NULL : &ts, NULL);
 	if (ret < 0)
 		return -1;
 	else if (ret == 0)
@@ -105,6 +101,34 @@  int gpiod_poll_fd(int fd, int64_t timeout_ns)
 	return 1;
 }
 
+int gpiod_poll_fd(int fd, int64_t timeout_ns)
+{
+	struct pollfd pfd;
+
+	memset(&pfd, 0, sizeof(pfd));
+	pfd.fd = fd;
+	pfd.events = POLLIN | POLLPRI;
+
+	return gpiod_poll_fds(&pfd, 1, timeout_ns);
+}
+
+int gpiod_poll_fd_or(int fd1, int fd2, int64_t timeout_ns)
+{
+	struct pollfd pfds[2];
+	int ret;
+
+	memset(&pfds, 0, sizeof(pfds));
+	pfds[0].fd = fd1;
+	pfds[0].events = POLLIN | POLLPRI;
+	pfds[1].fd = fd2;
+	pfds[1].events = POLLIN | POLLERR;
+
+	ret = gpiod_poll_fds(pfds, 2, timeout_ns);
+	if (ret >= 1 && pfds[1].revents != 0)
+		return 2;
+	return ret;
+}
+
 int gpiod_set_output_value(enum gpiod_line_value in, enum gpiod_line_value *out)
 {
 	switch (in) {
diff --git a/lib/internal.h b/lib/internal.h
index 61d7aec..6b2105b 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -36,6 +36,7 @@  gpiod_info_event_from_uapi(struct gpio_v2_line_info_changed *uapi_evt);
 struct gpiod_info_event *gpiod_info_event_read_fd(int fd);
 
 int gpiod_poll_fd(int fd, int64_t timeout);
+int gpiod_poll_fd_or(int fd1, int fd2, int64_t timeout);
 int gpiod_set_output_value(enum gpiod_line_value in,
 			   enum gpiod_line_value *out);
 
diff --git a/lib/line-request.c b/lib/line-request.c
index e867d91..dd2a5a2 100644
--- a/lib/line-request.c
+++ b/lib/line-request.c
@@ -295,6 +295,15 @@  gpiod_line_request_wait_edge_events(struct gpiod_line_request *request,
 	return gpiod_poll_fd(request->fd, timeout_ns);
 }
 
+GPIOD_API int
+gpiod_line_request_wait_edge_events_or(struct gpiod_line_request *request,
+				    int fd, int64_t timeout_ns)
+{
+	assert(request);
+
+	return gpiod_poll_fd_or(request->fd, fd, timeout_ns);
+}
+
 GPIOD_API int
 gpiod_line_request_read_edge_events(struct gpiod_line_request *request,
 				    struct gpiod_edge_event_buffer *buffer,
diff --git a/tests/tests-edge-event.c b/tests/tests-edge-event.c
index b744ca5..3281831 100644
--- a/tests/tests-edge-event.c
+++ b/tests/tests-edge-event.c
@@ -81,6 +81,17 @@  GPIOD_TEST_CASE(cannot_request_lines_in_output_mode_with_edge_detection)
 	gpiod_test_expect_errno(EINVAL);
 }
 
+static gpointer trigger_cancel_channel(gpointer data)
+{
+	int cancel_sender = ((int *)data)[1];
+
+	g_usleep(1000);
+
+	g_assert_cmpint(write(cancel_sender, "\0", 1), ==, 1);
+
+	return NULL;
+}
+
 static gpointer falling_and_rising_edge_events(gpointer data)
 {
 	GPIOSimChip *sim = data;
@@ -361,6 +372,46 @@  GPIOD_TEST_CASE(read_rising_edge_event_polled)
 	g_thread_join(thread);
 }
 
+GPIOD_TEST_CASE(cancel_edge_event_wait)
+{
+	static const guint offset = 2;
+	gint channel[2];
+
+	g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new("num-lines", 8, NULL);
+	g_autoptr(struct_gpiod_chip) chip = NULL;
+	g_autoptr(struct_gpiod_line_settings) settings = NULL;
+	g_autoptr(struct_gpiod_line_config) line_cfg = NULL;
+	g_autoptr(struct_gpiod_line_request) request = NULL;
+	g_autoptr(GThread) thread = NULL;
+	g_autoptr(struct_gpiod_edge_event_buffer) buffer = NULL;
+	gint ret;
+
+	chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim));
+	settings = gpiod_test_create_line_settings_or_fail();
+	line_cfg = gpiod_test_create_line_config_or_fail();
+	buffer = gpiod_test_create_edge_event_buffer_or_fail(64);
+
+	gpiod_line_settings_set_direction(settings, GPIOD_LINE_DIRECTION_INPUT);
+	gpiod_line_settings_set_edge_detection(settings,
+					       GPIOD_LINE_EDGE_RISING);
+
+	gpiod_test_line_config_add_line_settings_or_fail(line_cfg, &offset, 1,
+							 settings);
+
+	request = gpiod_test_chip_request_lines_or_fail(chip, NULL, line_cfg);
+
+	g_assert_cmpint(pipe(channel), ==, 0);
+
+	thread = g_thread_new("trigger-cancel-channel",
+			      trigger_cancel_channel, channel);
+
+	ret = gpiod_line_request_wait_edge_events_or(request, channel[0], 1000000000);
+	g_assert_cmpint(ret, ==, 2); /* Canceled */
+	close(channel[0]);
+	close(channel[1]);
+	g_thread_join(thread);
+}
+
 GPIOD_TEST_CASE(read_both_events_blocking)
 {
 	/*