Re: [libgpiod v2][PATCH v2 5/5] bindings: python: add the implementation for v2 API

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Fri, Jul 1, 2022 at 10:02 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Fri, Jul 01, 2022 at 03:33:38PM +0800, Kent Gibson wrote:
> > On Fri, Jul 01, 2022 at 09:29:53AM +0200, Bartosz Golaszewski wrote:
> > > On Fri, Jul 1, 2022 at 9:27 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > >
> > > > On Fri, Jul 01, 2022 at 09:21:58AM +0200, Bartosz Golaszewski wrote:
> > > > > On Fri, Jul 1, 2022 at 8:07 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > > > >
> > > > > > On Thu, Jun 30, 2022 at 04:38:51PM +0800, Kent Gibson wrote:
> > > > > > > On Thu, Jun 30, 2022 at 04:14:50PM +0800, Kent Gibson wrote:
> > > > > > > > On Thu, Jun 30, 2022 at 08:54:24AM +0200, Bartosz Golaszewski wrote:
> > > > > > > > > On Thu, Jun 30, 2022 at 4:25 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > > > > > > > >
> > > > > > > > > > On Tue, Jun 28, 2022 at 10:42:26AM +0200, Bartosz Golaszewski wrote:
> > > > > > > > > > > This is the implementation of the new python API for libgpiod v2.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > [snip]
> > > > > > > > > >
> > > > > > > > > > > +     }
> > > > > > > > > > > +
> > > > > > > > > > > +     res = PyObject_Call(method, args, line_cfg_kwargs);
> > > > > > > > > > > +     Py_DECREF(args);
> > > > > > > > > > > +     Py_DECREF(method);
> > > > > > > > > > > +     if (!Py_IsNone(res)) {
> > > > > > > > > > > +             Py_DECREF(res);
> > > > > > > > > > > +             return NULL;
> > > > > > > > > > > +     }
> > > > > > > > > > > +
> > > > > > > > > >
> > > > > > > > > > Building against python 3.9 (the min required by configure.ac) gives:
> > > > > > > > > >
> > > > > > > > > > module.c:276:7: warning: implicit declaration of function ‘Py_IsNone’; did you mean ‘Py_None’? [-Wimplicit-function-declaration]
> > > > > > > > > >   276 |  if (!Py_IsNone(res)) {
> > > > > > > > > >       |       ^~~~~~~~~
> > > > > > > > > >       |       Py_None
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Py_IsNone didn't get added to the Stable ABI until 3.10.
> > > > > > > > > >
> > > > > > > > > > Cheers,
> > > > > > > > > > Kent.
> > > > > > > > >
> > > > > > > > > It seems like most distros still ship python 3.9, I don't want to make
> > > > > > > > > 3.10 the requirement. This can be replaced by `if (res != Py_None)`.
> > > > > > > > > Are there any more build issues?
> > > > > > > > >
> > > > > > > >
> > > > > > > > No, that was the only one.
> > > > > > > >
> > > > > > >
> > > > > > > But I am seeing a test failure:
> > > > > > >
> > > > > > > $ sudo bindings/python/tests/gpiod_py_test.py
> > > > > > > .............................................................................F................................
> > > > > > > ======================================================================
> > > > > > > FAIL: test_module_line_request_edge_detection (cases.tests_line_request.ModuleLineRequestWorks)
> > > > > > > ----------------------------------------------------------------------
> > > > > > > Traceback (most recent call last):
> > > > > > >   File "/home/dev/libgpiod/bindings/python/tests/cases/tests_line_request.py", line 71, in test_module_line_request_edge_detection
> > > > > > >     self.assertTrue(req.wait_edge_event())
> > > > > > > AssertionError: False is not true
> > > > > > >
> > > > > > > ----------------------------------------------------------------------
> > > > > > > Ran 110 tests in 2.652s
> > > > > > >
> > > > > > > FAILED (failures=1)
> > > > > > >
> > > > > >
> > > > > > The req.wait_edge_event() does not wait without a timeout parameter,
> > > > > > which is a bit nonintuitive, so the test has a race.
> > > > >
> > > > > Ah, makes sense.
> > > > >
> > > > > > Adding a timeout=datetime.timedelta(microseconds=1) (the shortest
> > > > > > possible) works for me, so anything that triggers a context switch is
> > > > > > probably sufficient, though a longer timeout probably wouldn't hurt.
> > > > > >
> > > > >
> > > > > I'll change that.
> > > > >
> > > > > > The Python API should take timeout=NONE to mean wait indefinitely, and
> > > > > > 0 as a poll.
> > > > >
> > > > > This makes sense but I'd still want to have some default behavior for
> > > > > when timeout is not given. Maybe wait indefinitely?
> > > >
> > > > That is what I said - you get timeout=None if the kwarg is not specified.
> > > >
> > > > >
> > > > > > And it should take the timeout as a float, not a
> > > > > > timedelta, as per select.select.  From its doc:
> > > > >
> > > > > I don't necessarily want to mirror select's interface. Why would we
> > > > > prefer a float over a class that's the standard python interface for
> > > > > storing time deltas?
> > > > >
> > > >
> > > > Cos you are forcing the user to create a timedelta, which is a PITA,
> > > > and both time.sleep and select.select (i.e. standard Python modules)
> > > > do it that way.  The float is the Pythonic way.
> > > >
> > >
> > > Timedelta constructor is much more explicit than a float IMO. How
> > > about a compromise and taking both (mutually exclusive)?
> > > timeout=datettime.timedelta(seconds=1) == timeout_sec=float(1.0)?
> > >
> >
> > Maybe, but float seconds seems to be the way they do it.
> > If you insist on both then just the one timeout parameter and work the
> > type out on the fly. (it's Python, so dynamic typing...)
> >
>
> Same issue for chip.wait_info_event(), btw.
> Still working through a full review - but it'll probably take a while.
>
> Wrt the wait, does the C API have a blocking wait, or do you have to
> poll() the fd?
>

Blocking wait is simply reading the event without checking if an event
is there to be read. select() (the system call) waits indefinitely if
the timeval struct is NULL, ppoll() behaves the same for a NULL
timespec, poll() does the same for a negative timeout (which is an
int). We take an uint64_t so we can't do it. Either we need to switch
to int64_t and interpret a negative value as indefinite wait or just
not do it at all and tell users to just call the (blocking)
read_edge_event().

Bart

> And can you add a description of the timeout=0 behaviour to
> gpiod_chip_wait_info_event() etc, as 0 is sometimes taken as block.
>