Re: [libgpiod v2][PATCH v5] bindings: cxx: implement C++ bindings for libgpiod v2.0

[Kent Gibson <warthog618@xxxxxxxxx>]

On Mon, Apr 25, 2022 at 04:48:40PM +0200, Bartosz Golaszewski wrote:
> On Sun, Mar 27, 2022 at 2:22 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> 
> [snip]
> 
> >
> > > +     ::gpiod::edge_event_buffer buffer;
> > >
> > >       for (;;) {
> > > -             auto events = lines.event_wait(::std::chrono::seconds(1));
> > > -             if (events) {
> > > -                     for (auto& it: events)
> > > -                             print_event(it.event_read());
> > > +             if (request.wait_edge_event(::std::chrono::seconds(5))) {
> > > +                     request.read_edge_event(buffer);
> > > +
> > > +                     for (const auto& event: buffer)
> > > +                             print_event(event);
> > >               }
> > >       }
> > >
> >
> > What is the purpose of the wait_edge_event() here?
> > Wouldn't read_edge_event() block until the next event?
> >
> > This example should be minimal and demonstrate how the code should
> > normally be used. e.g.
> >
> >         for (const auto& event: request.events_iter())
> >                   print_event(event);
> >
> 
> We're making the request's file descriptor non-blocking in the C
> library. Do you think we should keep it in blocking mode?
> 

Ok, didn't realise that.

The function documentation for gpiod_line_request_read_edge_event()
says:

@note This function will block if no event was queued for the line request.

and I was assuming everything was built on that - so blocking.

But in gpiod_chip_request_lines() you do fcntl() the request fd to
non-blocking. So that documentation is wrong - or you should not be
setting the NONBLOCK.

If there are no events available, gpiod_line_request_read_edge_event()
will in fact return -1 (EIO), as returned by
gpiod_edge_event_buffer_read_fd().
If you want to go non-blocking, that should return 0?

Also, the line_request::read_edge_event() methods don't specify if they
block or return 0 if no events are available.  Whichever way you go,
document it.

> I'm no longer sure why I did that honestly.
> 

Hmmm, the only reason I can see is so gpiod_edge_event_buffer_read_fd()
can read up to max_events events in one read and not block if there were
no events available?
It could poll() first to check if there are events available - but it
doesn't.  Keeping syscalls to a minimum?

> Maybe a request config flag for that?
> 

I'd rather not - then you need to explain that the functions mentioned
earlier may block or return 0, depending.
I would rather make the wait/read the standard approach, with the read
blocking if no events are available.  That is fine for the vast majority
of cases.

Having said that, given you expose the fd, the user can always fcntl()
it themselves - in which case libgpiod should not assume that
gpiod_line_request_read_edge_event() will block - it may return -1 (EIO)
instead, as that is how gpiod_edge_event_buffer_read_fd() currently
behaves. So libgpiod should handle both cases, even if not explicitly
supporting a non-blocking read.

Cheers,
Kent.