diff mbox series

[libgpiod,v2,2/4] tools: use ppoll() where higher timeout resolution makes sense

Message ID 20240416215222.175166-3-brgl@bgdev.pl
State New
Headers show
Series tools: timeout handling improvements | expand

Commit Message

Bartosz Golaszewski April 16, 2024, 9:52 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We allow timeout units to be specified in microseconds but for poll() we
need to round them up to milliseconds. Switch to ppoll() to avoid losing
precision.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 tools/gpiomon.c    | 12 ++++++++++--
 tools/gpionotify.c | 12 ++++++++++--
 2 files changed, 20 insertions(+), 4 deletions(-)

Comments

Bartosz Golaszewski April 22, 2024, 6:15 p.m. UTC | #1 
On Wed, Apr 17, 2024 at 9:23 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Tue, Apr 16, 2024 at 11:52:20PM +0200, Bartosz Golaszewski wrote:
> >
> > -             ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
> > +             if (cfg.idle_timeout > 0) {
> > +                     idle_timeout.tv_sec = cfg.idle_timeout / 1000000;
> > +                     idle_timeout.tv_nsec =
> > +                                     (cfg.idle_timeout % 1000000) * 1000;
> > +             }
> > +
> > +             ret = ppoll(pollfds, resolver->num_chips,
> > +                         cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL);
> >               if (ret < 0)
> >                       die_perror("error polling for events");
> >
>
> One minor nit - I would introduce a timespec pointer initialised to NULL
> and set to point to idle_timeout within the if rather than repeat the
> cfg.idle_timeout > 0 test.
>

Actually we can avoid it by doing it once before we enter the for(;;)
loop. It's passed by constant pointer to ppoll() anyway and having the
struct AND pointer to it initialized to NULL sounds more complex than
it needs to be. I'll do it in v2.

> But that is just personal preference, so either way,
>
> Reviewed-by: Kent Gibson <warthog618@gmail.com>
>

Thanks, I'll keep the tag if you don't mind the above solution?

Bart
Kent Gibson April 22, 2024, 11:31 p.m. UTC | #2 
On Mon, Apr 22, 2024 at 08:15:46PM +0200, Bartosz Golaszewski wrote:
> On Wed, Apr 17, 2024 at 9:23 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Tue, Apr 16, 2024 at 11:52:20PM +0200, Bartosz Golaszewski wrote:
> > >
> > > -             ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
> > > +             if (cfg.idle_timeout > 0) {
> > > +                     idle_timeout.tv_sec = cfg.idle_timeout / 1000000;
> > > +                     idle_timeout.tv_nsec =
> > > +                                     (cfg.idle_timeout % 1000000) * 1000;
> > > +             }
> > > +
> > > +             ret = ppoll(pollfds, resolver->num_chips,
> > > +                         cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL);
> > >               if (ret < 0)
> > >                       die_perror("error polling for events");
> > >
> >
> > One minor nit - I would introduce a timespec pointer initialised to NULL
> > and set to point to idle_timeout within the if rather than repeat the
> > cfg.idle_timeout > 0 test.
> >
>
> Actually we can avoid it by doing it once before we enter the for(;;)
> loop. It's passed by constant pointer to ppoll() anyway and having the
> struct AND pointer to it initialized to NULL sounds more complex than
> it needs to be. I'll do it in v2.
>

Ah, I overlooked the for loop, so you are right that it should be
initialised before that.

> > But that is just personal preference, so either way,
> >
> > Reviewed-by: Kent Gibson <warthog618@gmail.com>
> >
>
> Thanks, I'll keep the tag if you don't mind the above solution?
>

Sure.

Cheers,
Kent.
diff mbox series

Patch

diff --git a/tools/gpiomon.c b/tools/gpiomon.c
index 40e6ac2..728a671 100644
--- a/tools/gpiomon.c
+++ b/tools/gpiomon.c
@@ -176,7 +176,7 @@  static int parse_config(int argc, char **argv, struct config *cfg)
 			cfg->fmt = optarg;
 			break;
 		case 'i':
-			cfg->idle_timeout = parse_period_or_die(optarg) / 1000;
+			cfg->idle_timeout = parse_period_or_die(optarg);
 			break;
 		case 'l':
 			cfg->active_low = true;
@@ -362,6 +362,7 @@  int main(int argc, char **argv)
 	int num_lines, events_done = 0;
 	struct gpiod_edge_event *event;
 	struct line_resolver *resolver;
+	struct timespec idle_timeout;
 	struct gpiod_chip *chip;
 	struct pollfd *pollfds;
 	unsigned int *offsets;
@@ -453,7 +454,14 @@  int main(int argc, char **argv)
 	for (;;) {
 		fflush(stdout);
 
-		ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
+		if (cfg.idle_timeout > 0) {
+			idle_timeout.tv_sec = cfg.idle_timeout / 1000000;
+			idle_timeout.tv_nsec =
+					(cfg.idle_timeout % 1000000) * 1000;
+		}
+
+		ret = ppoll(pollfds, resolver->num_chips,
+			    cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL);
 		if (ret < 0)
 			die_perror("error polling for events");
 
diff --git a/tools/gpionotify.c b/tools/gpionotify.c
index d2aee15..962896c 100644
--- a/tools/gpionotify.c
+++ b/tools/gpionotify.c
@@ -132,7 +132,7 @@  static int parse_config(int argc, char **argv, struct config *cfg)
 			cfg->fmt = optarg;
 			break;
 		case 'i':
-			cfg->idle_timeout = parse_period_or_die(optarg) / 1000;
+			cfg->idle_timeout = parse_period_or_die(optarg);
 			break;
 		case 'n':
 			cfg->events_wanted = parse_uint_or_die(optarg);
@@ -374,6 +374,7 @@  int main(int argc, char **argv)
 	int i, j, ret, events_done = 0, evtype;
 	struct line_resolver *resolver;
 	struct gpiod_info_event *event;
+	struct timespec idle_timeout;
 	struct gpiod_chip **chips;
 	struct gpiod_chip *chip;
 	struct pollfd *pollfds;
@@ -422,7 +423,14 @@  int main(int argc, char **argv)
 	for (;;) {
 		fflush(stdout);
 
-		ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
+		if (cfg.idle_timeout > 0) {
+			idle_timeout.tv_sec = cfg.idle_timeout / 1000000;
+			idle_timeout.tv_nsec =
+					(cfg.idle_timeout % 1000000) * 1000;
+		}
+
+		ret = ppoll(pollfds, resolver->num_chips,
+			    cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL);
 		if (ret < 0)
 			die_perror("error polling for events");