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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, Jul 7, 2022 at 3:10 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Thu, Jul 07, 2022 at 02:19:17PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Jul 5, 2022 at 4:09 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:

[snip]

> > >
> > > How about merging the _default and _offset forms by adding an offsets
> > > kwarg?
> > > offsets=None (or unspecified) -> default
> > > offsets=int -> offset
> > > offsets=iterable -> offsets
> > >
> > > Off on a bit of a tangent... why should the end user care about
> > > defaults and overrides?
> > > For a higher level abstraction I'd prefer to see the whole "default"
> > > concept disappear in favour of the config for each line.  That would
> > > remove a lot of the complexity from the LineConfig interface.
> > > Though it would add complexity to the binding internals.
> > >
> >
> > What would that look like (in python code) if I wanted to request 5
> > lines and use the same config for them?
> >
>
> That is the trivial case - you use the module level
> gpiod.request_lines() as is and pass in the config parameters and list of
> lines you want.
>
> req = gpiod.request_lines(chip="gpiochip0", offsets=[1,2,3,4,5],
>                           direction="output", values=[1,0,1,0,0])
>

This is close to what I have now in my v3 branch. Except that values
is called output_values and takes a dictionary like its counterpart in
LineConfig but that can be extended to interpreting a list as
providing the values for corresponding offsets/lines. Current version
of request_lines() takes all LineConfig options and uses them as the
defaults.

> The more complicated case is where the lines config differs.
> Then you have to build the LineConfig by adding the config for each set
> of lines in a separate call to set_props().
> Then you provide that LineConfig to the request_lines(), along with the
> set of lines.
>
> lc.set_props(offsets=[1,2,3], direction="input")
> lc.set_props(offsets=[4,5], direction="output", values=[1,0])
> req = gpiod.request_lines(chip="gpiochip0", line_cfg=lc)
>
> (simplified examples using stringified prop values etc - hope you get
> the idea)
>
> Building that on top of the C API, you would determine the "default"
> config based on the most common attribute values, then override the
> config for the lines that differ from that default.
> That is the internal complexity I mentioned.
>

Internal complexity is fine - it's the implicitness of the defaults
that make me not like this idea. I think we discussed something
similar for the C API and I was against it too. Your examples are fine
but the defaults for lines not mentioned in set_props() would be
filled by a freshly created LineConfig with its well defined default
values. In other words I prefer to keep the override mechanism visible
in python but unification of the property setters is something I will
consider.

To me it should look like:

lc.set_props(direction=Direction.INPUT, edge_detection=Edge.BOTH) sets
the defaults
lc.set_props(offset=4, direction=Direction.OUTPUT) sets a single override
lc.set_props(offsets=[5, 1], direction=Direction.OUTPUT,
output_value=Value.ACTIVE) sets a set of overrides.

Bart

> > > [snip]
> > > > +     static char *kwlist[] = {
> > > > +             "path",
> > > > +             "req_cfg",
> > > > +             "line_cfg",
> > > > +             "lines",
> > > > +             "direction",
> > > > +             "edge_detection",
> > > > +             "bias",
> > > > +             "drive",
> > > > +             "active_low",
> > > > +             "debounce_period",
> > > > +             "event_clock",
> > > > +             "output_value",
> > > > +             "output_values",
> > > > +             NULL
> > > > +     };
> > > > +
> > >
> > > My suggestion to provide a lines parameter here was actually a poor one,
> > > given the LineConfig only deals with offsets - which is totally reasonable
> > > as supporting line names in LineConfig would be complicated.
> > > I would prefer the two to be consistent, and so use offsets.
> > >
> >
> > I disagree. In the module-wide request function we have the chip
> > already, we can map the names to offsets. It makes perfect sense to do
> > it implicitly here as a pythonic shorthand for opening the chip
> > manually and requesting lines separately. This function already got
> > improved a lot in my v3.
> >
>
> Yeah, good point - the caller of the module level function won't have a
> Chip object to do the mapping.  And forcing them to create one defeats
> the purpose of having the module level function in the first place.
>
> Cheers,
> Kent.