chip: don't set the request's file descriptor to non-blocking
Subsystems (2)
PATCH v2 1/1 chip: don't set the request's file descriptor to non-blocking
Make the behavior consistent with the documentation and don't set the line request's fd to non-blocking. Add two more test-cases that make sure the library behaves correctly.
--- lib/chip.c | 23 -------- tests/tests-edge-event.c | 119 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 23 deletions(-)
lib/chip.c +0 -23
@@ -181,23 +181,6 @@ GPIOD_API int gpiod_chip_get_line_offset_from_name(struct gpiod_chip *chip,return -1 ; } -static int set_fd_noblock (int fd) -{ -int ret, flags; - - flags =fcntl (fd,F_GETFL ,0 ); -if (flags <0 ) -return -1 ; - - flags |=O_NONBLOCK ; - - ret =fcntl (fd,F_SETFL , flags); -if (ret <0 ) -return -1 ; - -return 0 ; -} - GPIOD_API struct gpiod_line_request * gpiod_chip_request_lines(struct gpiod_chip *chip, struct gpiod_request_config *req_cfg,
@@ -222,12 +205,6 @@ gpiod_chip_request_lines(struct gpiod_chip *chip,if (ret <0 )return NULL ; - ret =set_fd_noblock (uapi_req.fd); -if (ret) { -close (uapi_req.fd); -return NULL ; - } - request =gpiod_line_request_from_uapi (&uapi_req);if (!request) {close (uapi_req.fd);
tests/tests-edge-event.c +119 -0
128 lines not on this page · raw message
Signature · 1 line
2.32.0
Quoted 85 lines · Bartosz Golaszewski
Make the behavior consistent with the documentation and don't set the line request's fd to non-blocking. Add two more test-cases that make sure the library behaves correctly.
Signed-off-by Bartosz Golaszewski <[email protected]>--- lib/chip.c | 23 -------- tests/tests-edge-event.c | 119 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 23 deletions(-)lib/chip.c +0
-23@@ -181,23 +181,6 @@ GPIOD_API int gpiod_chip_get_line_offset_from_name(struct gpiod_chip *chip,return -1 ; } -static int set_fd_noblock (int fd) -{ -int ret, flags; - - flags =fcntl (fd,F_GETFL ,0 ); -if (flags <0 ) -return -1 ; - - flags |=O_NONBLOCK ; - - ret =fcntl (fd,F_SETFL , flags); -if (ret <0 ) -return -1 ; - -return 0 ; -} - GPIOD_API struct gpiod_line_request * gpiod_chip_request_lines(struct gpiod_chip *chip, struct gpiod_request_config *req_cfg,@@ -222,12 +205,6 @@ gpiod_chip_request_lines(struct gpiod_chip *chip,if (ret <0 )return NULL ; - ret =set_fd_noblock (uapi_req.fd); -if (ret) { -close (uapi_req.fd); -return NULL ; - } - request =gpiod_line_request_from_uapi (&uapi_req);if (!request) {close (uapi_req.fd);tests/tests-edge-event.c +26
-0@@ -357,6 +357,75 @@ GPIOD_TEST_CASE(read_rising_edge_event_polled)g_thread_join (thread); } +GPIOD_TEST_CASE (read_both_events_blocking) +{ +/* +* This time without polling so that the read gets a chance to block +* and we can make sure it doesn't immediately return an error. +*/ + +static const guint offset =2 ; + +g_autoptr (GPIOSimChip) sim =g_gpiosim_chip_new ("num-lines" ,8 ,NULL ); +g_autoptr (struct_gpiod_chip) chip =NULL ; +g_autoptr (struct_gpiod_request_config) req_cfg =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 ; +struct gpiod_edge_event *event; +gint ret; + + chip =gpiod_test_open_chip_or_fail (g_gpiosim_chip_get_dev_path (sim)); + req_cfg =gpiod_test_create_request_config_or_fail (); + line_cfg =gpiod_test_create_line_config_or_fail (); + buffer =gpiod_test_create_edge_event_buffer_or_fail (64 ); + +gpiod_request_config_set_offsets (req_cfg,1 , &offset); + gpiod_line_config_set_direction_default(line_cfg,
tests/tests-edge-event.c GPIOD_TEST_CASE(read_rising_edge_event_polled) · line 395
Quoted 10 lines
tests/tests-edge-event.c +10
-0+ GPIOD_LINE_DIRECTION_INPUT); +gpiod_line_config_set_edge_detection_default (line_cfg, +GPIOD_LINE_EDGE_BOTH ); + + request =gpiod_test_request_lines_or_fail (chip, req_cfg, line_cfg); + + thread =g_thread_new ("request-release" , + falling_and_rising_edge_events, sim); +g_thread_ref (thread); +
Not a fan of using usleep in the tests (not shown here but in falling_and_rising_edge_events()) to get the background thread to generate events after the main thread is waiting - would prefer to flip that around so that the background thread blocks, and can be shown to be blocked, and the main thread generates the events and checks that the background thread got them, so the handshaking doesn't involve sleeps.
Quoted 72 lines
tests/tests-edge-event.c +65
-0+/* First event. */ + + ret =gpiod_line_request_read_edge_event (request, buffer,1 ); + g_assert_cmpint(ret, ==, 1); +gpiod_test_join_thread_and_return_if_failed (thread); + + g_assert_cmpuint(gpiod_edge_event_buffer_get_num_events(buffer), ==, 1); + event =gpiod_edge_event_buffer_get_event (buffer,0 ); +g_assert_nonnull (event); +gpiod_test_join_thread_and_return_if_failed (thread); + +g_assert_cmpint (gpiod_edge_event_get_event_type (event), + ==, GPIOD_EDGE_EVENT_RISING_EDGE); + g_assert_cmpuint(gpiod_edge_event_get_line_offset(event), ==, 2); + +/* Second event. */ + + ret =gpiod_line_request_read_edge_event (request, buffer,1 ); + g_assert_cmpint(ret, ==, 1); +gpiod_test_join_thread_and_return_if_failed (thread); + + g_assert_cmpuint(gpiod_edge_event_buffer_get_num_events(buffer), ==, 1); + event =gpiod_edge_event_buffer_get_event (buffer,0 ); +g_assert_nonnull (event); +gpiod_test_join_thread_and_return_if_failed (thread); + +g_assert_cmpint (gpiod_edge_event_get_event_type (event), + ==, GPIOD_EDGE_EVENT_FALLING_EDGE); + g_assert_cmpuint(gpiod_edge_event_get_line_offset(event), ==, 2); + +g_thread_join (thread); +} +static gpointer rising_edge_events_on_two_offsets (gpointer data) {GPIOSimChip *sim = data;@@ -488,3 +557,53 @@ GPIOD_TEST_CASE(event_copy)g_assert_nonnull (copy);g_assert_true (copy != event); } + +GPIOD_TEST_CASE (reading_more_events_than_the_queue_contains_doesnt_block) +{ +static const guint offset =2 ; + +g_autoptr (GPIOSimChip) sim =g_gpiosim_chip_new ("num-lines" ,8 ,NULL ); +g_autoptr (struct_gpiod_chip) chip =NULL ; +g_autoptr (struct_gpiod_request_config) req_cfg =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)); + req_cfg =gpiod_test_create_request_config_or_fail (); + line_cfg =gpiod_test_create_line_config_or_fail (); + buffer =gpiod_test_create_edge_event_buffer_or_fail (64 ); + +gpiod_request_config_set_offsets (req_cfg,1 , &offset); +gpiod_line_config_set_direction_default (line_cfg, +GPIOD_LINE_DIRECTION_INPUT ); +gpiod_line_config_set_edge_detection_default (line_cfg, +GPIOD_LINE_EDGE_BOTH ); + + request =gpiod_test_request_lines_or_fail (chip, req_cfg, line_cfg); + +g_gpiosim_chip_set_pull (sim,2 ,G_GPIOSIM_PULL_UP ); +g_usleep (500 ); +g_gpiosim_chip_set_pull (sim,2 ,G_GPIOSIM_PULL_DOWN ); +g_usleep (500 ); +g_gpiosim_chip_set_pull (sim,2 ,G_GPIOSIM_PULL_UP );
tests/tests-edge-event.c GPIOD_TEST_CASE(event_copy) · line 601
Quoted 10 lines
tests/tests-edge-event.c +10
-0+g_usleep (500 ); +g_gpiosim_chip_set_pull (sim,2 ,G_GPIOSIM_PULL_DOWN ); +g_usleep (500 ); +g_gpiosim_chip_set_pull (sim,2 ,G_GPIOSIM_PULL_UP ); +g_usleep (500 ); +g_gpiosim_chip_set_pull (sim,2 ,G_GPIOSIM_PULL_DOWN ); +g_usleep (500 ); +g_gpiosim_chip_set_pull (sim,2 ,G_GPIOSIM_PULL_UP ); +g_usleep (500 ); +
What is the sleep waiting for?
Why 500?
Is 1 sufficient?
Is there a critical threshold?
Perhaps that should be incorporated into g_gpiosim_chip_set_pull()?
Quoted 11 lines
tests/tests-edge-event.c +8
-0+ ret =gpiod_line_request_read_edge_event (request, buffer,12 ); + g_assert_cmpint(ret, ==, 7); +gpiod_test_return_if_failed (); + + ret =gpiod_line_request_wait_edge_event (request,1000 ); + g_assert_cmpint(ret, ==, 0); +gpiod_test_return_if_failed (); +}Signature · 1 line
2.32.0
I'm fine with the code change.
As written the tests are probably ok, but to be more robust I would rework them - arbitrary sleeps in tests are a red flag for me.
That is probably outside the scope of this patch though, and getting the code change applied is far more important, so...
Cheers,
Kent.