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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Fri, Jul 08, 2022 at 05:26:52PM +0200, Bartosz Golaszewski wrote:
> On Fri, Jul 8, 2022 at 1:28 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> >
> > On Fri, Jul 8, 2022 at 12:56 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > >
> >
> > [snip]
> >
> > > >
> > > > The limitation of the uAPI is what keeps us from making it true in
> > > > user-space (that each line can have its own config). As it is, only up
> > > > to 9-10 lines can have distinct configs and making the API look and
> > > > behave as if it wasn't the case is more confusing (E2BIG errors) than
> > > > simply admitting we have the concept of defaults and overrides (to
> > > > which the interface is greatly simplified in the high-level
> > > > libraries). The idea about making the most common config attributes
> > > > become the defaults is simply bad. It would require the user to
> > > > anticipate how the library will behave for every attribute and lead to
> > >
> > > It requires nothing from the user.  They are not even aware of the
> > > concept of "defaults" or "overrides".  They just set config on lines.
> > > If that is too complicated, which is quite unlikely, then they get
> > > E2BIG and they need to repartition their lines into multiple requests.
> > >
> > > Anyway, that horse is dead.
> > >
> >
> > For a python user, this:
> >
> > lc = gpiod.LineConfig()
> > lc.set_props(offsets=[2, 3], direction=Direction.OUTPUT)
> > req = gpiod.request_lines("/dev/gpiochip0", line_cfg=lc)
> >
> > is pretty much as simple as it gets. They still don't need to be aware
> > of the underlying split into defaults and overrides. I believe it's
> > GoodEnough™.
> >
> > I imagine in Rust bindings we'll be able to chain set_props() as is
> > customary and we'll get a one-liner out of that.
> >
> 
> The code I posted here is wrong as it's missing the request config but
> it made me think: how about in case of req_cfg=None or not passed at
> all, we derive the lines to request from overridden offsets in the
> line config? In that case if the user does:
> 
>   lc = gpiod.LineConfig()
>   lc.set_props(offsets=[0, 1], direction=Direction.OUTPUT,
> output_value=Value.ACTIVE)
>   lc.set_props(offset=4, direction=Direction.INPUT)
>   req = gpiod.request_lines("/dev/gpiochip0", line_cfg=lc)
> 
> Then it will be interpreted as lines=[0, 1, 4]?
> 

That makes sense to me - I also dropped the req_cfg from my examples
as it was redundant.
Why force the user to provide the req_cfg merely to repeat the lines in
the line_cfg?

> I'm also thinking that we could allow the output values to be mapped
> as <line name> -> <value> within gpiod.LineConfig like that:
> 
>   lc.set_props(lines=["foo", 4], direction=Direction.OUTPUT)
>   lc.set_output_values({"foo": Value.Active, 4: Value.INACTIVE})
> 
> It would require us to retrieve the names of all lines from the chip
> at the time of the request and store them in the request structure
> (for reconfigure to work) but it would make the entire thing even more
> "pythonic".
> 

This is what I meant by giving LineConfig "the line kwarg treatment" in
my original review comments - wherever you were taking 'offset' accept
'line' instead (being a name string or offset int).
It makes the binding more complex as the mapping is deferred, but the
result is more pythonic and consistent wrt line identification across
the API.

OTOH I can understand if you think the API benefits aren't worth the
added internal complexity, which is why I didn't worry too much when you
didn't change it in this version - just thought you decided it wasn't
worth the effort.

Cheers,
Kent.