[libgpiod][PATCH 0/7] teach libgpiod to read multiple line events at once

[libgpiod][PATCH 0/7] teach libgpiod to read multiple line events at once

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

In v2 of my series adding the line status watch I introduced a regression
that made it impossible to read more than one event at the same time. This
is because I use libgpiod for testing and it doesn't allow to read more
than one event from user-space.

This series adds this missing functionality so that we can test v3 and avoid
this problem.

Bartosz Golaszewski (7):
  core: use gpiod_line_event_get_fd() in gpiod_line_event_read()
  core: provide functions for reading multiple line events at once
  tests: event: extend test coverage for reading multiple line events at
    once
  bindings: cxx: provide a method for reading multiple line events
  bindings: cxx: tests: add a test-case for reading multiple line events
  bindings: python: add a method for reading multiple line events
  bindings: python: tests: add a test-case for reading multiple line
    events

 bindings/cxx/gpiod.hpp                 |  7 +++
 bindings/cxx/line.cpp                  | 45 +++++++++++---
 bindings/cxx/tests/tests-event.cpp     | 31 ++++++++++
 bindings/python/gpiodmodule.c          | 57 ++++++++++++++++++
 bindings/python/tests/gpiod_py_test.py | 22 +++++++
 include/gpiod.h                        | 25 ++++++++
 lib/core.c                             | 68 ++++++++++++++++-----
 tests/tests-event.c                    | 83 ++++++++++++++++++++++++++
 8 files changed, 314 insertions(+), 24 deletions(-)

-- 
2.23.0

[libgpiod][PATCH 1/7] core: use gpiod_line_event_get_fd() in gpiod_line_event_read()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We can drop the redundant line state check if we directly call
gpiod_line_event_get_fd() from gpiod_line_event_read(). As opposed
to line_get_fd() it checks if the line was requested for events.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 lib/core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/lib/core.c b/lib/core.c
index b9fd6f4..89f5465 100644
--- a/lib/core.c
+++ b/lib/core.c
@@ -1012,12 +1012,9 @@ int gpiod_line_event_read(struct gpiod_line *line,
 {
 	int fd;
 
-	if (line->state != LINE_REQUESTED_EVENTS) {
-		errno = EPERM;
+	fd = gpiod_line_event_get_fd(line);
+	if (fd < 0)
 		return -1;
-	}
-
-	fd = line_get_fd(line);
 
 	return gpiod_line_event_read_fd(fd, event);
 }
-- 
2.23.0

[libgpiod][PATCH 2/7] core: provide functions for reading multiple line events at once

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

The kernel allows us to read multiple (up to 16) line events with
a single read() call, but up to this point libgpiod only supported
reading one event at a time. Add two new functions that allow users
to read more events at the same time.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/gpiod.h | 25 ++++++++++++++++++++
 lib/core.c      | 61 ++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 75 insertions(+), 11 deletions(-)

diff --git a/include/gpiod.h b/include/gpiod.h
index 0ecb804..b275f4a 100644
--- a/include/gpiod.h
+++ b/include/gpiod.h
@@ -1479,6 +1479,19 @@ int gpiod_line_event_wait_bulk(struct gpiod_line_bulk *bulk,
 int gpiod_line_event_read(struct gpiod_line *line,
 			  struct gpiod_line_event *event) GPIOD_API;
 
+/**
+ * @brief Read up to a certain number of events from the GPIO line.
+ * @param line GPIO line object.
+ * @param events Buffer to which the event data will be copied. Must hold at
+ *               least the amount of events specified in num_events.
+ * @param num_events Specifies how many events can be stored in the buffer.
+ * @return On success returns the number of events stored in the buffer, on
+ *         failure -1 is returned.
+ */
+int gpiod_line_event_read_multiple(struct gpiod_line *line,
+				   struct gpiod_line_event *events,
+				   unsigned int num_events) GPIOD_API;
+
 /**
  * @brief Get the event file descriptor.
  * @param line GPIO line object.
@@ -1503,6 +1516,18 @@ int gpiod_line_event_get_fd(struct gpiod_line *line) GPIOD_API;
  */
 int gpiod_line_event_read_fd(int fd, struct gpiod_line_event *event) GPIOD_API;
 
+/**
+ * @brief Read up to a certain number of events directly from a file descriptor.
+ * @param fd File descriptor.
+ * @param events Buffer to which the event data will be copied. Must hold at
+ *               least the amount of events specified in num_events.
+ * @param num_events Specifies how many events can be stored in the buffer.
+ * @return On success returns the number of events stored in the buffer, on
+ *         failure -1 is returned.
+ */
+int gpiod_line_event_read_fd_multiple(int fd, struct gpiod_line_event *events,
+				      unsigned int num_events) GPIOD_API;
+
 /**
  * @}
  *
diff --git a/lib/core.c b/lib/core.c
index 89f5465..8352e18 100644
--- a/lib/core.c
+++ b/lib/core.c
@@ -1009,6 +1009,19 @@ int gpiod_line_event_wait_bulk(struct gpiod_line_bulk *bulk,
 
 int gpiod_line_event_read(struct gpiod_line *line,
 			  struct gpiod_line_event *event)
+{
+	int ret;
+
+	ret = gpiod_line_event_read_multiple(line, event, 1);
+	if (ret < 0)
+		return -1;
+
+	return 0;
+}
+
+int gpiod_line_event_read_multiple(struct gpiod_line *line,
+				   struct gpiod_line_event *events,
+				   unsigned int num_events)
 {
 	int fd;
 
@@ -1016,7 +1029,7 @@ int gpiod_line_event_read(struct gpiod_line *line,
 	if (fd < 0)
 		return -1;
 
-	return gpiod_line_event_read_fd(fd, event);
+	return gpiod_line_event_read_fd_multiple(fd, events, num_events);
 }
 
 int gpiod_line_event_get_fd(struct gpiod_line *line)
@@ -1031,25 +1044,51 @@ int gpiod_line_event_get_fd(struct gpiod_line *line)
 
 int gpiod_line_event_read_fd(int fd, struct gpiod_line_event *event)
 {
-	struct gpioevent_data evdata;
+	int ret;
+
+	ret = gpiod_line_event_read_fd_multiple(fd, event, 1);
+	if (ret < 0)
+		return -1;
+
+	return 0;
+}
+
+int gpiod_line_event_read_fd_multiple(int fd, struct gpiod_line_event *events,
+				      unsigned int num_events)
+{
+	/*
+	 * 16 is the maximum number of events the kernel can store in the FIFO
+	 * so we can allocate the buffer on the stack.
+	 */
+	struct gpioevent_data evdata[16], *curr;
+	struct gpiod_line_event *event;
+	unsigned int events_read, i;
 	ssize_t rd;
 
-	memset(&evdata, 0, sizeof(evdata));
+	memset(evdata, 0, sizeof(evdata));
 
-	rd = read(fd, &evdata, sizeof(evdata));
+	rd = read(fd, evdata, sizeof(evdata));
 	if (rd < 0) {
 		return -1;
-	} else if (rd != sizeof(evdata)) {
+	} else if ((unsigned int)rd < sizeof(*evdata)) {
 		errno = EIO;
 		return -1;
 	}
 
-	event->event_type = evdata.id == GPIOEVENT_EVENT_RISING_EDGE
-						? GPIOD_LINE_EVENT_RISING_EDGE
-						: GPIOD_LINE_EVENT_FALLING_EDGE;
+	events_read = rd / sizeof(*evdata);
+	if (events_read < num_events)
+		num_events = events_read;
 
-	event->ts.tv_sec = evdata.timestamp / 1000000000ULL;
-	event->ts.tv_nsec = evdata.timestamp % 1000000000ULL;
+	for (i = 0; i < num_events; i++) {
+		curr = &evdata[i];
+		event = &events[i];
 
-	return 0;
+		event->event_type = curr->id == GPIOEVENT_EVENT_RISING_EDGE
+					? GPIOD_LINE_EVENT_RISING_EDGE
+					: GPIOD_LINE_EVENT_FALLING_EDGE;
+		event->ts.tv_sec = curr->timestamp / 1000000000ULL;
+		event->ts.tv_nsec = curr->timestamp % 1000000000ULL;
+	}
+
+	return i;
 }
-- 
2.23.0

[libgpiod][PATCH 3/7] tests: event: extend test coverage for reading multiple line events at once

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add test cases for new helpers allowing users to read multiple events
at once.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 tests/tests-event.c | 83 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/tests/tests-event.c b/tests/tests-event.c
index d425d1a..1f4a2eb 100644
--- a/tests/tests-event.c
+++ b/tests/tests-event.c
@@ -552,3 +552,86 @@ GPIOD_TEST_CASE(invalid_fd, 0, { 8 })
 	g_assert_cmpint(ret, ==, -1);
 	g_assert_cmpint(errno, ==, EINVAL);
 }
+
+GPIOD_TEST_CASE(read_multiple_events, 0, { 8 })
+{
+	g_autoptr(gpiod_chip_struct) chip = NULL;
+	struct gpiod_line_event events[3];
+	struct timespec ts = { 1, 0 };
+	struct gpiod_line *line;
+	gint ret;
+
+	chip = gpiod_chip_open(gpiod_test_chip_path(0));
+	g_assert_nonnull(chip);
+	gpiod_test_return_if_failed();
+
+	line = gpiod_chip_get_line(chip, 4);
+	g_assert_nonnull(line);
+	gpiod_test_return_if_failed();
+
+	ret = gpiod_line_request_both_edges_events(line, GPIOD_TEST_CONSUMER);
+	g_assert_cmpint(ret, ==, 0);
+
+	gpiod_test_chip_set_pull(0, 4, 1);
+	usleep(10000);
+	gpiod_test_chip_set_pull(0, 4, 0);
+	usleep(10000);
+	gpiod_test_chip_set_pull(0, 4, 1);
+	usleep(10000);
+
+	ret = gpiod_line_event_wait(line, &ts);
+	g_assert_cmpint(ret, ==, 1);
+
+	ret = gpiod_line_event_read_multiple(line, events, 3);
+	g_assert_cmpint(ret, ==, 3);
+
+	g_assert_cmpint(events[0].event_type, ==,
+			GPIOD_LINE_EVENT_RISING_EDGE);
+	g_assert_cmpint(events[1].event_type, ==,
+			GPIOD_LINE_EVENT_FALLING_EDGE);
+	g_assert_cmpint(events[2].event_type, ==,
+			GPIOD_LINE_EVENT_RISING_EDGE);
+}
+
+GPIOD_TEST_CASE(read_multiple_events_fd, 0, { 8 })
+{
+	g_autoptr(gpiod_chip_struct) chip = NULL;
+	struct gpiod_line_event events[3];
+	struct timespec ts = { 1, 0 };
+	struct gpiod_line *line;
+	gint ret, fd;
+
+	chip = gpiod_chip_open(gpiod_test_chip_path(0));
+	g_assert_nonnull(chip);
+	gpiod_test_return_if_failed();
+
+	line = gpiod_chip_get_line(chip, 4);
+	g_assert_nonnull(line);
+	gpiod_test_return_if_failed();
+
+	ret = gpiod_line_request_both_edges_events(line, GPIOD_TEST_CONSUMER);
+	g_assert_cmpint(ret, ==, 0);
+
+	gpiod_test_chip_set_pull(0, 4, 1);
+	usleep(10000);
+	gpiod_test_chip_set_pull(0, 4, 0);
+	usleep(10000);
+	gpiod_test_chip_set_pull(0, 4, 1);
+	usleep(10000);
+
+	ret = gpiod_line_event_wait(line, &ts);
+	g_assert_cmpint(ret, ==, 1);
+
+	fd = gpiod_line_event_get_fd(line);
+	g_assert_cmpint(fd, >=, 0);
+
+	ret = gpiod_line_event_read_fd_multiple(fd, events, 3);
+	g_assert_cmpint(ret, ==, 3);
+
+	g_assert_cmpint(events[0].event_type, ==,
+			GPIOD_LINE_EVENT_RISING_EDGE);
+	g_assert_cmpint(events[1].event_type, ==,
+			GPIOD_LINE_EVENT_FALLING_EDGE);
+	g_assert_cmpint(events[2].event_type, ==,
+			GPIOD_LINE_EVENT_RISING_EDGE);
+}
-- 
2.23.0

Re: [libgpiod][PATCH 3/7] tests: event: extend test coverage for reading multiple line events at once

[Kent Gibson]

On Wed, Dec 18, 2019 at 03:24:45PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Add test cases for new helpers allowing users to read multiple events
> at once.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  tests/tests-event.c | 83 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/tests/tests-event.c b/tests/tests-event.c
> index d425d1a..1f4a2eb 100644
> --- a/tests/tests-event.c
> +++ b/tests/tests-event.c
> @@ -552,3 +552,86 @@ GPIOD_TEST_CASE(invalid_fd, 0, { 8 })
>  	g_assert_cmpint(ret, ==, -1);
>  	g_assert_cmpint(errno, ==, EINVAL);
>  }
> +
> +GPIOD_TEST_CASE(read_multiple_events, 0, { 8 })
> +{
> +	g_autoptr(gpiod_chip_struct) chip = NULL;
> +	struct gpiod_line_event events[3];
> +	struct timespec ts = { 1, 0 };
> +	struct gpiod_line *line;
> +	gint ret;
> +
> +	chip = gpiod_chip_open(gpiod_test_chip_path(0));
> +	g_assert_nonnull(chip);
> +	gpiod_test_return_if_failed();
> +
> +	line = gpiod_chip_get_line(chip, 4);
> +	g_assert_nonnull(line);
> +	gpiod_test_return_if_failed();
> +
> +	ret = gpiod_line_request_both_edges_events(line, GPIOD_TEST_CONSUMER);
> +	g_assert_cmpint(ret, ==, 0);
> +
> +	gpiod_test_chip_set_pull(0, 4, 1);
> +	usleep(10000);
> +	gpiod_test_chip_set_pull(0, 4, 0);
> +	usleep(10000);
> +	gpiod_test_chip_set_pull(0, 4, 1);
> +	usleep(10000);
> +

I assume the sleep is to wait for the event to be generated from the
call gpiod_test_chip_set_pull, which is not guaranteed to occur before
the call returns, otherwise you can toggle the line too fast and may
miss events.
Arbitrary sleeps in code, including tests, should be avoided as they
are brittle and obsure what you are actually waiting for.
An alternative in this case is to add a second event fd and wait for
the event to arrive there before continuing.

Kent.

> +	ret = gpiod_line_event_wait(line, &ts);
> +	g_assert_cmpint(ret, ==, 1);
> +
> +	ret = gpiod_line_event_read_multiple(line, events, 3);
> +	g_assert_cmpint(ret, ==, 3);
> +
> +	g_assert_cmpint(events[0].event_type, ==,
> +			GPIOD_LINE_EVENT_RISING_EDGE);
> +	g_assert_cmpint(events[1].event_type, ==,
> +			GPIOD_LINE_EVENT_FALLING_EDGE);
> +	g_assert_cmpint(events[2].event_type, ==,
> +			GPIOD_LINE_EVENT_RISING_EDGE);
> +}
> +
> +GPIOD_TEST_CASE(read_multiple_events_fd, 0, { 8 })
> +{
> +	g_autoptr(gpiod_chip_struct) chip = NULL;
> +	struct gpiod_line_event events[3];
> +	struct timespec ts = { 1, 0 };
> +	struct gpiod_line *line;
> +	gint ret, fd;
> +
> +	chip = gpiod_chip_open(gpiod_test_chip_path(0));
> +	g_assert_nonnull(chip);
> +	gpiod_test_return_if_failed();
> +
> +	line = gpiod_chip_get_line(chip, 4);
> +	g_assert_nonnull(line);
> +	gpiod_test_return_if_failed();
> +
> +	ret = gpiod_line_request_both_edges_events(line, GPIOD_TEST_CONSUMER);
> +	g_assert_cmpint(ret, ==, 0);
> +
> +	gpiod_test_chip_set_pull(0, 4, 1);
> +	usleep(10000);
> +	gpiod_test_chip_set_pull(0, 4, 0);
> +	usleep(10000);
> +	gpiod_test_chip_set_pull(0, 4, 1);
> +	usleep(10000);
> +
> +	ret = gpiod_line_event_wait(line, &ts);
> +	g_assert_cmpint(ret, ==, 1);
> +
> +	fd = gpiod_line_event_get_fd(line);
> +	g_assert_cmpint(fd, >=, 0);
> +
> +	ret = gpiod_line_event_read_fd_multiple(fd, events, 3);
> +	g_assert_cmpint(ret, ==, 3);
> +
> +	g_assert_cmpint(events[0].event_type, ==,
> +			GPIOD_LINE_EVENT_RISING_EDGE);
> +	g_assert_cmpint(events[1].event_type, ==,
> +			GPIOD_LINE_EVENT_FALLING_EDGE);
> +	g_assert_cmpint(events[2].event_type, ==,
> +			GPIOD_LINE_EVENT_RISING_EDGE);
> +}
> -- 
> 2.23.0
> 

Re: [libgpiod][PATCH 3/7] tests: event: extend test coverage for reading multiple line events at once

[Bartosz Golaszewski]

czw., 19 gru 2019 o 14:35 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> > +
> > +     ret = gpiod_line_request_both_edges_events(line, GPIOD_TEST_CONSUMER);
> > +     g_assert_cmpint(ret, ==, 0);
> > +
> > +     gpiod_test_chip_set_pull(0, 4, 1);
> > +     usleep(10000);
> > +     gpiod_test_chip_set_pull(0, 4, 0);
> > +     usleep(10000);
> > +     gpiod_test_chip_set_pull(0, 4, 1);
> > +     usleep(10000);
> > +
>
> I assume the sleep is to wait for the event to be generated from the
> call gpiod_test_chip_set_pull, which is not guaranteed to occur before
> the call returns, otherwise you can toggle the line too fast and may
> miss events.

Yes, this is why I put it there. Otherwise, some simulated interrupts
were being dropped when they fired while the previous ones were still
served.

> Arbitrary sleeps in code, including tests, should be avoided as they
> are brittle and obsure what you are actually waiting for.

Indeed.

> An alternative in this case is to add a second event fd and wait for
> the event to arrive there before continuing.
>

I'm not sure I understand. We can't have two event fd's for the same
line. Or are you thinking about setting up a second line, generating
events on it and consuming them so that we can rely on the timing to
make sure the events were registered for the first one too?

Bart

Re: [libgpiod][PATCH 3/7] tests: event: extend test coverage for reading multiple line events at once

[Kent Gibson]

On Thu, Dec 19, 2019 at 02:48:48PM +0100, Bartosz Golaszewski wrote:
> czw., 19 gru 2019 o 14:35 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > > +
> > > +     ret = gpiod_line_request_both_edges_events(line, GPIOD_TEST_CONSUMER);
> > > +     g_assert_cmpint(ret, ==, 0);
> > > +
> > > +     gpiod_test_chip_set_pull(0, 4, 1);
> > > +     usleep(10000);
> > > +     gpiod_test_chip_set_pull(0, 4, 0);
> > > +     usleep(10000);
> > > +     gpiod_test_chip_set_pull(0, 4, 1);
> > > +     usleep(10000);
> > > +
> >
> > I assume the sleep is to wait for the event to be generated from the
> > call gpiod_test_chip_set_pull, which is not guaranteed to occur before
> > the call returns, otherwise you can toggle the line too fast and may
> > miss events.
> 
> Yes, this is why I put it there. Otherwise, some simulated interrupts
> were being dropped when they fired while the previous ones were still
> served.
> 
> > Arbitrary sleeps in code, including tests, should be avoided as they
> > are brittle and obsure what you are actually waiting for.
> 
> Indeed.
> 
> > An alternative in this case is to add a second event fd and wait for
> > the event to arrive there before continuing.
> >
> 
> I'm not sure I understand. We can't have two event fd's for the same
> line. Or are you thinking about setting up a second line, generating
> events on it and consuming them so that we can rely on the timing to
> make sure the events were registered for the first one too?
> 

I was thinking of two event fds on the one line, and you are
correct, there can only be one, so that wont work.
Wrt using two lines, I'm not sure ordering can be guaranteed as you
end up with two separate debugfs writes...

Kent.

Re: [libgpiod][PATCH 3/7] tests: event: extend test coverage for reading multiple line events at once

[Bartosz Golaszewski]

czw., 19 gru 2019 o 15:05 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> On Thu, Dec 19, 2019 at 02:48:48PM +0100, Bartosz Golaszewski wrote:
> > czw., 19 gru 2019 o 14:35 Kent Gibson <warthog618@gmail.com> napisał(a):
> > >
> > > > +
> > > > +     ret = gpiod_line_request_both_edges_events(line, GPIOD_TEST_CONSUMER);
> > > > +     g_assert_cmpint(ret, ==, 0);
> > > > +
> > > > +     gpiod_test_chip_set_pull(0, 4, 1);
> > > > +     usleep(10000);
> > > > +     gpiod_test_chip_set_pull(0, 4, 0);
> > > > +     usleep(10000);
> > > > +     gpiod_test_chip_set_pull(0, 4, 1);
> > > > +     usleep(10000);
> > > > +
> > >
> > > I assume the sleep is to wait for the event to be generated from the
> > > call gpiod_test_chip_set_pull, which is not guaranteed to occur before
> > > the call returns, otherwise you can toggle the line too fast and may
> > > miss events.
> >
> > Yes, this is why I put it there. Otherwise, some simulated interrupts
> > were being dropped when they fired while the previous ones were still
> > served.
> >
> > > Arbitrary sleeps in code, including tests, should be avoided as they
> > > are brittle and obsure what you are actually waiting for.
> >
> > Indeed.
> >
> > > An alternative in this case is to add a second event fd and wait for
> > > the event to arrive there before continuing.
> > >
> >
> > I'm not sure I understand. We can't have two event fd's for the same
> > line. Or are you thinking about setting up a second line, generating
> > events on it and consuming them so that we can rely on the timing to
> > make sure the events were registered for the first one too?
> >
>
> I was thinking of two event fds on the one line, and you are
> correct, there can only be one, so that wont work.
> Wrt using two lines, I'm not sure ordering can be guaranteed as you
> end up with two separate debugfs writes...
>

Yeah I gave it a spin and it turns out it's not reliable - some events
still get dropped albeit less than without any syncing. The usleep()
calls are still better than this. Any other ideas? I agree this is not
optimal, but couldn't come up with anything else.

Bart

Re: [libgpiod][PATCH 3/7] tests: event: extend test coverage for reading multiple line events at once

[Kent Gibson]

On Thu, Dec 19, 2019 at 03:07:10PM +0100, Bartosz Golaszewski wrote:
> czw., 19 gru 2019 o 15:05 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > On Thu, Dec 19, 2019 at 02:48:48PM +0100, Bartosz Golaszewski wrote:
> > > czw., 19 gru 2019 o 14:35 Kent Gibson <warthog618@gmail.com> napisał(a):
> > > >
> > > > > +
> > > > > +     ret = gpiod_line_request_both_edges_events(line, GPIOD_TEST_CONSUMER);
> > > > > +     g_assert_cmpint(ret, ==, 0);
> > > > > +
> > > > > +     gpiod_test_chip_set_pull(0, 4, 1);
> > > > > +     usleep(10000);
> > > > > +     gpiod_test_chip_set_pull(0, 4, 0);
> > > > > +     usleep(10000);
> > > > > +     gpiod_test_chip_set_pull(0, 4, 1);
> > > > > +     usleep(10000);
> > > > > +
> > > >
> > > > I assume the sleep is to wait for the event to be generated from the
> > > > call gpiod_test_chip_set_pull, which is not guaranteed to occur before
> > > > the call returns, otherwise you can toggle the line too fast and may
> > > > miss events.
> > >
> > > Yes, this is why I put it there. Otherwise, some simulated interrupts
> > > were being dropped when they fired while the previous ones were still
> > > served.
> > >
> > > > Arbitrary sleeps in code, including tests, should be avoided as they
> > > > are brittle and obsure what you are actually waiting for.
> > >
> > > Indeed.
> > >
> > > > An alternative in this case is to add a second event fd and wait for
> > > > the event to arrive there before continuing.
> > > >
> > >
> > > I'm not sure I understand. We can't have two event fd's for the same
> > > line. Or are you thinking about setting up a second line, generating
> > > events on it and consuming them so that we can rely on the timing to
> > > make sure the events were registered for the first one too?
> > >
> >
> > I was thinking of two event fds on the one line, and you are
> > correct, there can only be one, so that wont work.
> > Wrt using two lines, I'm not sure ordering can be guaranteed as you
> > end up with two separate debugfs writes...
> >
> 
> Yeah I gave it a spin and it turns out it's not reliable - some events
> still get dropped albeit less than without any syncing. The usleep()
> calls are still better than this. Any other ideas? I agree this is not
> optimal, but couldn't come up with anything else.
> 

The two options I can think of are getting the debugfs write
to block until the simulated interrupt has been serviced, or adding a
multi-line set to the mockup so the two lines can be set simultaneously.
I'm not sure the first is possible.
And the second wont help if it results in two interrupts unless the
order the interrupts are serviced is guaranteed.
Either way it seems like a whole lot of work just to remove the sleeps,
so the sleeps seem like a reasonable workaround :(.

Kent.

Re: [libgpiod][PATCH 3/7] tests: event: extend test coverage for reading multiple line events at once

[Bartosz Golaszewski]

czw., 19 gru 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> > > >
> > > > I'm not sure I understand. We can't have two event fd's for the same
> > > > line. Or are you thinking about setting up a second line, generating
> > > > events on it and consuming them so that we can rely on the timing to
> > > > make sure the events were registered for the first one too?
> > > >
> > >
> > > I was thinking of two event fds on the one line, and you are
> > > correct, there can only be one, so that wont work.
> > > Wrt using two lines, I'm not sure ordering can be guaranteed as you
> > > end up with two separate debugfs writes...
> > >
> >
> > Yeah I gave it a spin and it turns out it's not reliable - some events
> > still get dropped albeit less than without any syncing. The usleep()
> > calls are still better than this. Any other ideas? I agree this is not
> > optimal, but couldn't come up with anything else.
> >
>
> The two options I can think of are getting the debugfs write
> to block until the simulated interrupt has been serviced, or adding a
> multi-line set to the mockup so the two lines can be set simultaneously.
> I'm not sure the first is possible.
> And the second wont help if it results in two interrupts unless the
> order the interrupts are serviced is guaranteed.
> Either way it seems like a whole lot of work just to remove the sleeps,
> so the sleeps seem like a reasonable workaround :(.
>

Now when I looked at gpio-mockup again I'm under the impression that
the interrupt should be fired from the set() callback, not from the
debugfs write callback. But this is a minor detail and it won't help
here.

Anyway, I'm really not sure how to do it any differently. We can also
be less strict in user-space - we can schedule, let's say, 16 events
without sleeps and not really verify the number of read events - just
check if it's bigger than 1 and that what we read are actual events
(valid event_type).

Bart

Re: [libgpiod][PATCH 3/7] tests: event: extend test coverage for reading multiple line events at once

[Bartosz Golaszewski]

czw., 19 gru 2019 o 17:19 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
>
> czw., 19 gru 2019 o 15:36 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > > > >
> > > > > I'm not sure I understand. We can't have two event fd's for the same
> > > > > line. Or are you thinking about setting up a second line, generating
> > > > > events on it and consuming them so that we can rely on the timing to
> > > > > make sure the events were registered for the first one too?
> > > > >
> > > >
> > > > I was thinking of two event fds on the one line, and you are
> > > > correct, there can only be one, so that wont work.
> > > > Wrt using two lines, I'm not sure ordering can be guaranteed as you
> > > > end up with two separate debugfs writes...
> > > >
> > >
> > > Yeah I gave it a spin and it turns out it's not reliable - some events
> > > still get dropped albeit less than without any syncing. The usleep()
> > > calls are still better than this. Any other ideas? I agree this is not
> > > optimal, but couldn't come up with anything else.
> > >
> >
> > The two options I can think of are getting the debugfs write
> > to block until the simulated interrupt has been serviced, or adding a
> > multi-line set to the mockup so the two lines can be set simultaneously.
> > I'm not sure the first is possible.
> > And the second wont help if it results in two interrupts unless the
> > order the interrupts are serviced is guaranteed.
> > Either way it seems like a whole lot of work just to remove the sleeps,
> > so the sleeps seem like a reasonable workaround :(.
> >
>
> Now when I looked at gpio-mockup again I'm under the impression that
> the interrupt should be fired from the set() callback, not from the
> debugfs write callback. But this is a minor detail and it won't help
> here.
>
> Anyway, I'm really not sure how to do it any differently. We can also
> be less strict in user-space - we can schedule, let's say, 16 events
> without sleeps and not really verify the number of read events - just
> check if it's bigger than 1 and that what we read are actual events
> (valid event_type).
>
> Bart

If there are no other issues, I'll apply this series as is (with some
comments explaining why we do this). We can improve the tests later
once we have something better in place in the kernel. For now - they
are pretty stable, I've been running them a lot and never saw an issue
with missed events.

Bart

[libgpiod][PATCH 4/7] bindings: cxx: provide a method for reading multiple line events

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add a new method to gpiod::line class allowing to read up to 16 line
events at once.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 bindings/cxx/gpiod.hpp |  7 +++++++
 bindings/cxx/line.cpp  | 45 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/bindings/cxx/gpiod.hpp b/bindings/cxx/gpiod.hpp
index 078201b..bd99d28 100644
--- a/bindings/cxx/gpiod.hpp
+++ b/bindings/cxx/gpiod.hpp
@@ -421,6 +421,12 @@ public:
 	 */
 	GPIOD_API line_event event_read(void) const;
 
+	/**
+	 * @brief Read up to 16 line events.
+	 * @return Vector of line event objects.
+	 */
+	GPIOD_API ::std::vector<line_event> event_read_multiple(void) const;
+
 	/**
 	 * @brief Get the event file descriptor associated with this line.
 	 * @return File descriptor number.
@@ -513,6 +519,7 @@ private:
 	line(::gpiod_line* line, const chip& owner);
 
 	void throw_if_null(void) const;
+	line_event make_line_event(const ::gpiod_line_event& event) const noexcept;
 
 	::gpiod_line* _m_line;
 	chip _m_chip;
diff --git a/bindings/cxx/line.cpp b/bindings/cxx/line.cpp
index ed6ef55..11deae6 100644
--- a/bindings/cxx/line.cpp
+++ b/bindings/cxx/line.cpp
@@ -206,6 +206,23 @@ bool line::event_wait(const ::std::chrono::nanoseconds& timeout) const
 	return !!event_bulk;
 }
 
+line_event line::make_line_event(const ::gpiod_line_event& event) const noexcept
+{
+	line_event ret;
+
+	if (event.event_type == GPIOD_LINE_EVENT_RISING_EDGE)
+		ret.event_type = line_event::RISING_EDGE;
+	else if (event.event_type == GPIOD_LINE_EVENT_FALLING_EDGE)
+		ret.event_type = line_event::FALLING_EDGE;
+
+	ret.timestamp = ::std::chrono::nanoseconds(
+				event.ts.tv_nsec + (event.ts.tv_sec * 1000000000));
+
+	ret.source = *this;
+
+	return ret;
+}
+
 line_event line::event_read(void) const
 {
 	this->throw_if_null();
@@ -219,17 +236,29 @@ line_event line::event_read(void) const
 		throw ::std::system_error(errno, ::std::system_category(),
 					  "error reading line event");
 
-	if (event_buf.event_type == GPIOD_LINE_EVENT_RISING_EDGE)
-		event.event_type = line_event::RISING_EDGE;
-	else if (event_buf.event_type == GPIOD_LINE_EVENT_FALLING_EDGE)
-		event.event_type = line_event::FALLING_EDGE;
+	return this->make_line_event(event_buf);
+}
+
+::std::vector<line_event> line::event_read_multiple(void) const
+{
+	this->throw_if_null();
 
-	event.timestamp = ::std::chrono::nanoseconds(
-				event_buf.ts.tv_nsec + (event_buf.ts.tv_sec * 1000000000));
+	/* 16 is the maximum number of events stored in the kernel FIFO. */
+	::std::array<::gpiod_line_event, 16> event_buf;
+	::std::vector<line_event> events;
+	int rv;
+
+	rv = ::gpiod_line_event_read_multiple(this->_m_line,
+					      event_buf.data(), event_buf.size());
+	if (rv < 0)
+		throw ::std::system_error(errno, ::std::system_category(),
+					  "error reading multiple line events");
 
-	event.source = *this;
+	events.reserve(rv);
+	for (int i = 0; i < rv; i++)
+		events.push_back(this->make_line_event(event_buf[i]));
 
-	return event;
+	return events;
 }
 
 int line::event_get_fd(void) const
-- 
2.23.0

[libgpiod][PATCH 5/7] bindings: cxx: tests: add a test-case for reading multiple line events

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Extend the test coverage of C++ bindings with tests of reading of
multiple line events at once.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 bindings/cxx/tests/tests-event.cpp | 31 ++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/bindings/cxx/tests/tests-event.cpp b/bindings/cxx/tests/tests-event.cpp
index 8713333..63b6cc5 100644
--- a/bindings/cxx/tests/tests-event.cpp
+++ b/bindings/cxx/tests/tests-event.cpp
@@ -8,6 +8,7 @@
 #include <catch2/catch.hpp>
 #include <gpiod.hpp>
 #include <poll.h>
+#include <thread>
 
 #include "gpio-mockup.hpp"
 
@@ -217,3 +218,33 @@ TEST_CASE("It's possible to read values from lines requested for events", "[even
 		REQUIRE(line.get_value() == 0);
 	}
 }
+
+TEST_CASE("It's possible to read more than one line event", "[event][line]")
+{
+	mockup::probe_guard mockup_chips({ 8 });
+	::gpiod::chip chip(mockup::instance().chip_name(0));
+	auto line = chip.get_line(4);
+	::gpiod::line_request config;
+
+	config.consumer = consumer.c_str();
+	config.request_type = ::gpiod::line_request::EVENT_BOTH_EDGES;
+
+	line.request(config);
+
+	mockup::instance().chip_set_pull(0, 4, 1);
+	::std::this_thread::sleep_for(::std::chrono::milliseconds(10));
+	mockup::instance().chip_set_pull(0, 4, 0);
+	::std::this_thread::sleep_for(::std::chrono::milliseconds(10));
+	mockup::instance().chip_set_pull(0, 4, 1);
+	::std::this_thread::sleep_for(::std::chrono::milliseconds(10));
+
+	auto events = line.event_read_multiple();
+
+	REQUIRE(events.size() == 3);
+	REQUIRE(events.at(0).event_type == ::gpiod::line_event::RISING_EDGE);
+	REQUIRE(events.at(1).event_type == ::gpiod::line_event::FALLING_EDGE);
+	REQUIRE(events.at(2).event_type == ::gpiod::line_event::RISING_EDGE);
+	REQUIRE(events.at(0).source == line);
+	REQUIRE(events.at(1).source == line);
+	REQUIRE(events.at(2).source == line);
+}
-- 
2.23.0

[libgpiod][PATCH 6/7] bindings: python: add a method for reading multiple line events

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add a new method to the Line class allowing to read up to 16 line
events at once.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 bindings/python/gpiodmodule.c | 57 +++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/bindings/python/gpiodmodule.c b/bindings/python/gpiodmodule.c
index 27a8118..b0b52ee 100644
--- a/bindings/python/gpiodmodule.c
+++ b/bindings/python/gpiodmodule.c
@@ -850,6 +850,57 @@ static gpiod_LineEventObject *gpiod_Line_event_read(gpiod_LineObject *self,
 	return ret;
 }
 
+PyDoc_STRVAR(gpiod_Line_event_read_multiple_doc,
+"event_read_multiple() -> list of gpiod.LineEvent object\n"
+"\n"
+"Read up to 16 line events from this GPIO line object.");
+
+static PyObject *gpiod_Line_event_read_multiple(gpiod_LineObject *self,
+						PyObject *Py_UNUSED(ignored))
+{
+	struct gpiod_line_event evbuf[16];
+	gpiod_LineEventObject *event;
+	int rv, num_events, i;
+	PyObject *events;
+
+	if (gpiod_ChipIsClosed(self->owner))
+		return NULL;
+
+	memset(evbuf, 0, sizeof(evbuf));
+	Py_BEGIN_ALLOW_THREADS;
+	num_events = gpiod_line_event_read_multiple(self->line, evbuf,
+					sizeof(evbuf) / sizeof(*evbuf));
+	Py_END_ALLOW_THREADS;
+	if (num_events < 0)
+		return PyErr_SetFromErrno(PyExc_OSError);
+
+	events = PyList_New(num_events);
+	if (!events)
+		return NULL;
+
+	for (i = 0; i < num_events; i++) {
+		event = PyObject_New(gpiod_LineEventObject,
+				     &gpiod_LineEventType);
+		if (!event) {
+			Py_DECREF(events);
+			return NULL;
+		}
+
+		memcpy(&event->event, &evbuf[i], sizeof(event->event));
+		Py_INCREF(self);
+		event->source = self;
+
+		rv = PyList_SetItem(events, i, (PyObject *)event);
+		if (rv < 0) {
+			Py_DECREF(events);
+			Py_DECREF(event);
+			return NULL;
+		}
+	}
+
+	return events;
+}
+
 PyDoc_STRVAR(gpiod_Line_event_get_fd_doc,
 "event_get_fd() -> integer\n"
 "\n"
@@ -1026,6 +1077,12 @@ static PyMethodDef gpiod_Line_methods[] = {
 		.ml_flags = METH_NOARGS,
 		.ml_doc = gpiod_Line_event_read_doc,
 	},
+	{
+		.ml_name = "event_read_multiple",
+		.ml_meth = (PyCFunction)gpiod_Line_event_read_multiple,
+		.ml_flags = METH_NOARGS,
+		.ml_doc = gpiod_Line_event_read_multiple_doc,
+	},
 	{
 		.ml_name = "event_get_fd",
 		.ml_meth = (PyCFunction)gpiod_Line_event_get_fd,
-- 
2.23.0

[libgpiod][PATCH 7/7] bindings: python: tests: add a test-case for reading multiple line events

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Extend the test coverage of Python bindings with tests of reading of
multiple line events at once.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 bindings/python/tests/gpiod_py_test.py | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/bindings/python/tests/gpiod_py_test.py b/bindings/python/tests/gpiod_py_test.py
index 53d99d8..572aad8 100755
--- a/bindings/python/tests/gpiod_py_test.py
+++ b/bindings/python/tests/gpiod_py_test.py
@@ -12,6 +12,7 @@ import gpiod
 import gpiomockup
 import os
 import select
+import time
 import threading
 import unittest
 
@@ -833,6 +834,27 @@ class EventSingleLine(MockupTestCase):
                 self.assertEqual(event.type, gpiod.LineEvent.RISING_EDGE)
                 self.assertEqual(event.source.offset(), 4)
 
+    def test_single_line_read_multiple_events(self):
+        with gpiod.Chip(mockup.chip_name(0)) as chip:
+            line = chip.get_line(4)
+            line.request(consumer=default_consumer,
+                         type=gpiod.LINE_REQ_EV_BOTH_EDGES)
+            mockup.chip_set_pull(0, 4, 1)
+            time.sleep(0.01)
+            mockup.chip_set_pull(0, 4, 0)
+            time.sleep(0.01)
+            mockup.chip_set_pull(0, 4, 1)
+            time.sleep(0.01)
+            self.assertTrue(line.event_wait(sec=1))
+            events = line.event_read_multiple()
+            self.assertEqual(len(events), 3)
+            self.assertEqual(events[0].type, gpiod.LineEvent.RISING_EDGE)
+            self.assertEqual(events[1].type, gpiod.LineEvent.FALLING_EDGE)
+            self.assertEqual(events[2].type, gpiod.LineEvent.RISING_EDGE)
+            self.assertEqual(events[0].source.offset(), 4)
+            self.assertEqual(events[1].source.offset(), 4)
+            self.assertEqual(events[2].source.offset(), 4)
+
 class EventBulk(MockupTestCase):
 
     chip_sizes = ( 8, )
-- 
2.23.0