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

[Kent Gibson <warthog618@xxxxxxxxx>]

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?

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.

Cheers,
Kent.