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

[Kent Gibson <warthog618@gmail.com>]

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@gmail.com> 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@gmail.com> 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@gmail.com> 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...)

Cheers,
Kent.