[libgpiod][PATCH 0/3] fix get_values for events

[libgpiod][PATCH 0/3] fix get_values for events

[Kent Gibson]

This patch series fixes getting the values of a bulk of lineevents.

The problem was reported by Gerrit Wyen <ml@ionscale.com>, using the
cxx bindings.  The first patch adds test cases to confirm the reported
behaviour with cxx.  The second adds corresponding tests for the
underlying gpiod_line_get_value_bulk with events.  The third fixes
gpiod_line_get_value_bulk so that it returns the correct values
for a bulk of events.

Kent Gibson (3):
  bindings: cxx: tests: add tests for bulk events get_values
  tests: event: add tests for gpiod_line_get_value_bulk events
  core: fix gpiod_line_get_value_bulk for events

 bindings/cxx/tests/tests-event.cpp |  36 +++++++++-
 lib/core.c                         |  33 ++++++---
 tests/tests-event.c                | 110 +++++++++++++++++++++++++++++
 3 files changed, 169 insertions(+), 10 deletions(-)


base-commit: 2efb0075a7779fa61bfe01c40aa97f7d23a8e528
-- 
2.27.0

[PATCH 1/3] bindings: cxx: tests: add tests for bulk events get_values

[Kent Gibson]

Add tests to verify the behaviour of get_values when applied to a bulk
of event lines.

Reported-by: Gerrit Wyen <ml@ionscale.com>
Signed-off-by: Kent Gibson <warthog618@gmail.com>

---

The reported behaviour is that only the first line value is returned correctly.
The tests verify that behaviour in v1.5.

 bindings/cxx/tests/tests-event.cpp | 36 +++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/bindings/cxx/tests/tests-event.cpp b/bindings/cxx/tests/tests-event.cpp
index 63b6cc5..6d1c069 100644
--- a/bindings/cxx/tests/tests-event.cpp
+++ b/bindings/cxx/tests/tests-event.cpp
@@ -193,7 +193,7 @@ TEST_CASE("Event file descriptors can be used for polling", "[event]")
 	}
 }
 
-TEST_CASE("It's possible to read values from lines requested for events", "[event][line]")
+TEST_CASE("It's possible to read a value from a line requested for events", "[event][line]")
 {
 	mockup::probe_guard mockup_chips({ 8 });
 	::gpiod::chip chip(mockup::instance().chip_name(0));
@@ -219,6 +219,40 @@ TEST_CASE("It's possible to read values from lines requested for events", "[even
 	}
 }
 
+TEST_CASE("It's possible to read values from lines requested for events", "[event][bulk]")
+{
+	mockup::probe_guard mockup_chips({ 8 });
+	::gpiod::chip chip(mockup::instance().chip_name(0));
+	auto lines = chip.get_lines({ 0, 1, 2, 3, 4 });
+	::gpiod::line_request config;
+
+	config.consumer = consumer.c_str();
+	config.request_type = ::gpiod::line_request::EVENT_BOTH_EDGES;
+
+	mockup::instance().chip_set_pull(0, 5, 1);
+
+	SECTION("active-high (default)")
+	{
+		lines.request(config);
+		REQUIRE(lines.get_values() == ::std::vector<int>({ 0, 0, 0, 0, 0 }));
+		mockup::instance().chip_set_pull(0, 1, 1);
+		mockup::instance().chip_set_pull(0, 3, 1);
+		mockup::instance().chip_set_pull(0, 4, 1);
+		REQUIRE(lines.get_values() == ::std::vector<int>({ 0, 1, 0, 1, 1 }));
+	}
+
+	SECTION("active-low")
+	{
+		config.flags = ::gpiod::line_request::FLAG_ACTIVE_LOW;
+		lines.request(config);
+		REQUIRE(lines.get_values() == ::std::vector<int>({ 1, 1, 1, 1, 1 }));
+		mockup::instance().chip_set_pull(0, 1, 1);
+		mockup::instance().chip_set_pull(0, 3, 1);
+		mockup::instance().chip_set_pull(0, 4, 1);
+		REQUIRE(lines.get_values() == ::std::vector<int>({ 1, 0, 1, 0, 0 }));
+	}
+}
+
 TEST_CASE("It's possible to read more than one line event", "[event][line]")
 {
 	mockup::probe_guard mockup_chips({ 8 });
-- 
2.27.0

[PATCH 2/3] tests: event: add tests for gpiod_line_get_value_bulk events

[Kent Gibson]

Add tests to verify the behaviour of gpiod_line_get_value_bulk when
applied to a bulk of event lines.

Signed-off-by: Kent Gibson <warthog618@gmail.com>

---

In v1.5 only the value of the first line is returned correctly.

 tests/tests-event.c | 110 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/tests/tests-event.c b/tests/tests-event.c
index b59c995..e323060 100644
--- a/tests/tests-event.c
+++ b/tests/tests-event.c
@@ -421,6 +421,116 @@ GPIOD_TEST_CASE(get_value_active_low, 0, { 8 })
 	g_assert_cmpint(ev.event_type, ==, GPIOD_LINE_EVENT_FALLING_EDGE);
 }
 
+GPIOD_TEST_CASE(get_values, 0, { 8 })
+{
+	g_autoptr(gpiod_chip_struct) chip = NULL;
+	struct gpiod_line_bulk bulk;
+	struct gpiod_line *line;
+	gint i, ret, vals[8];
+
+	chip = gpiod_chip_open(gpiod_test_chip_path(0));
+	g_assert_nonnull(chip);
+	gpiod_test_return_if_failed();
+
+	gpiod_line_bulk_init(&bulk);
+	for (i = 0; i < 8; i++) {
+		line = gpiod_chip_get_line(chip, i);
+		g_assert_nonnull(line);
+		gpiod_test_return_if_failed();
+
+		gpiod_line_bulk_add(&bulk, line);
+		gpiod_test_chip_set_pull(0, i, 1);
+	}
+
+	ret = gpiod_line_request_bulk_rising_edge_events(&bulk,
+							 GPIOD_TEST_CONSUMER);
+	g_assert_cmpint(ret, ==, 0);
+
+	memset(vals, 0, sizeof(vals));
+	ret = gpiod_line_get_value_bulk(&bulk, vals);
+	g_assert_cmpint(ret, ==, 0);
+	g_assert_cmpint(vals[0], ==, 1);
+	g_assert_cmpint(vals[1], ==, 1);
+	g_assert_cmpint(vals[2], ==, 1);
+	g_assert_cmpint(vals[3], ==, 1);
+	g_assert_cmpint(vals[4], ==, 1);
+	g_assert_cmpint(vals[5], ==, 1);
+	g_assert_cmpint(vals[6], ==, 1);
+	g_assert_cmpint(vals[7], ==, 1);
+
+	gpiod_test_chip_set_pull(0, 1, 0);
+	gpiod_test_chip_set_pull(0, 3, 0);
+	gpiod_test_chip_set_pull(0, 4, 0);
+	gpiod_test_chip_set_pull(0, 7, 0);
+
+	memset(vals, 0, sizeof(vals));
+	ret = gpiod_line_get_value_bulk(&bulk, vals);
+	g_assert_cmpint(ret, ==, 0);
+	g_assert_cmpint(vals[0], ==, 1);
+	g_assert_cmpint(vals[1], ==, 0);
+	g_assert_cmpint(vals[2], ==, 1);
+	g_assert_cmpint(vals[3], ==, 0);
+	g_assert_cmpint(vals[4], ==, 0);
+	g_assert_cmpint(vals[5], ==, 1);
+	g_assert_cmpint(vals[6], ==, 1);
+	g_assert_cmpint(vals[7], ==, 0);
+}
+
+GPIOD_TEST_CASE(get_values_active_low, 0, { 8 })
+{
+	g_autoptr(gpiod_chip_struct) chip = NULL;
+	struct gpiod_line_bulk bulk;
+	struct gpiod_line *line;
+	gint i, ret, vals[8];
+
+	chip = gpiod_chip_open(gpiod_test_chip_path(0));
+	g_assert_nonnull(chip);
+	gpiod_test_return_if_failed();
+
+	gpiod_line_bulk_init(&bulk);
+	for (i = 0; i < 8; i++) {
+		line = gpiod_chip_get_line(chip, i);
+		g_assert_nonnull(line);
+		gpiod_test_return_if_failed();
+
+		gpiod_line_bulk_add(&bulk, line);
+		gpiod_test_chip_set_pull(0, i, 0);
+	}
+
+	ret = gpiod_line_request_bulk_rising_edge_events_flags(&bulk,
+			GPIOD_TEST_CONSUMER, GPIOD_LINE_REQUEST_FLAG_ACTIVE_LOW);
+	g_assert_cmpint(ret, ==, 0);
+
+	memset(vals, 0, sizeof(vals));
+	ret = gpiod_line_get_value_bulk(&bulk, vals);
+	g_assert_cmpint(ret, ==, 0);
+	g_assert_cmpint(vals[0], ==, 1);
+	g_assert_cmpint(vals[1], ==, 1);
+	g_assert_cmpint(vals[2], ==, 1);
+	g_assert_cmpint(vals[3], ==, 1);
+	g_assert_cmpint(vals[4], ==, 1);
+	g_assert_cmpint(vals[5], ==, 1);
+	g_assert_cmpint(vals[6], ==, 1);
+	g_assert_cmpint(vals[7], ==, 1);
+
+	gpiod_test_chip_set_pull(0, 1, 1);
+	gpiod_test_chip_set_pull(0, 3, 1);
+	gpiod_test_chip_set_pull(0, 4, 1);
+	gpiod_test_chip_set_pull(0, 7, 1);
+
+	memset(vals, 0, sizeof(vals));
+	ret = gpiod_line_get_value_bulk(&bulk, vals);
+	g_assert_cmpint(ret, ==, 0);
+	g_assert_cmpint(vals[0], ==, 1);
+	g_assert_cmpint(vals[1], ==, 0);
+	g_assert_cmpint(vals[2], ==, 1);
+	g_assert_cmpint(vals[3], ==, 0);
+	g_assert_cmpint(vals[4], ==, 0);
+	g_assert_cmpint(vals[5], ==, 1);
+	g_assert_cmpint(vals[6], ==, 1);
+	g_assert_cmpint(vals[7], ==, 0);
+}
+
 GPIOD_TEST_CASE(wait_multiple, 0, { 8 })
 {
 	g_autoptr(GpiodTestEventThread) ev_thread = NULL;
-- 
2.27.0

[PATCH 3/3] core: fix gpiod_line_get_value_bulk for events

[Kent Gibson]

Extend gpiod_line_get_value_bulk so that it works for bulks of
lineevents, not only linehandles.

Reported-by: Gerrit Wyen <ml@ionscale.com>
Signed-off-by: Kent Gibson <warthog618@gmail.com>

---
 lib/core.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/lib/core.c b/lib/core.c
index f704b44..ad76051 100644
--- a/lib/core.c
+++ b/lib/core.c
@@ -773,26 +773,41 @@ int gpiod_line_get_value(struct gpiod_line *line)
 int gpiod_line_get_value_bulk(struct gpiod_line_bulk *bulk, int *values)
 {
 	struct gpiohandle_data data;
-	struct gpiod_line *first;
+	struct gpiod_line *line;
 	unsigned int i;
 	int rv, fd;
 
 	if (!line_bulk_same_chip(bulk) || !line_bulk_all_requested(bulk))
 		return -1;
 
-	first = gpiod_line_bulk_get_line(bulk, 0);
+	line = gpiod_line_bulk_get_line(bulk, 0);
 
-	memset(&data, 0, sizeof(data));
+	if (line->state == LINE_REQUESTED_VALUES) {
+		memset(&data, 0, sizeof(data));
 
-	fd = line_get_fd(first);
+		fd = line_get_fd(line);
 
-	rv = ioctl(fd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, &data);
-	if (rv < 0)
-		return -1;
+		rv = ioctl(fd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, &data);
+		if (rv < 0)
+			return -1;
 
-	for (i = 0; i < gpiod_line_bulk_num_lines(bulk); i++)
-		values[i] = data.values[i];
+		for (i = 0; i < gpiod_line_bulk_num_lines(bulk); i++)
+			values[i] = data.values[i];
 
+	} else if (line->state == LINE_REQUESTED_EVENTS) {
+		for (i = 0; i < gpiod_line_bulk_num_lines(bulk); i++) {
+			line = gpiod_line_bulk_get_line(bulk, i);
+
+			fd = line_get_fd(line);
+			rv = ioctl(fd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, &data);
+			if (rv < 0)
+				return -1;
+			values[i] = data.values[0];
+		}
+	} else {
+		errno = EINVAL;
+		return -1;
+	}
 	return 0;
 }
 
-- 
2.27.0

Re: [libgpiod][PATCH 0/3] fix get_values for events

[Bartosz Golaszewski]

śr., 17 cze 2020 o 05:07 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> This patch series fixes getting the values of a bulk of lineevents.
>
> The problem was reported by Gerrit Wyen <ml@ionscale.com>, using the
> cxx bindings.  The first patch adds test cases to confirm the reported
> behaviour with cxx.  The second adds corresponding tests for the
> underlying gpiod_line_get_value_bulk with events.  The third fixes
> gpiod_line_get_value_bulk so that it returns the correct values
> for a bulk of events.
>
> Kent Gibson (3):
>   bindings: cxx: tests: add tests for bulk events get_values
>   tests: event: add tests for gpiod_line_get_value_bulk events
>   core: fix gpiod_line_get_value_bulk for events
>
>  bindings/cxx/tests/tests-event.cpp |  36 +++++++++-
>  lib/core.c                         |  33 ++++++---
>  tests/tests-event.c                | 110 +++++++++++++++++++++++++++++
>  3 files changed, 169 insertions(+), 10 deletions(-)
>
>
> base-commit: 2efb0075a7779fa61bfe01c40aa97f7d23a8e528
> --
> 2.27.0
>

I applied the whole series to master and backported patch 3/3 to
stable branches (v1.4.x and v1.5.x).

Thanks for a quick fix!

Bartosz