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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Thu, Jul 07, 2022 at 10:09:44PM +0200, Bartosz Golaszewski wrote:
> 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.
> 

I suspect you are right that we've been here before and I'm flogging a
dead horse, but you get that - I must think there is still a bit of life
in the old nag ;-).

I find it ironic that a feature of the uAPI that is there due to
the constraints on the uAPI, i.e. to keep the line_config to a
manageable size, gets propagated this highly.  In my mind the
configuration for each line has always been distinct, and the uAPI
line_config is just a reduced form.

(it was also a logical stepping stone from the v1 "all lines have the
same config", which maybe where your attachment to a default originates,
to "all lines can be configured independently" that I was going for in
v2, but practicality limited to "all lines can be configured
independently - to a point", basically by supporting a limited number
of deltas - which you refer to as overrides)

> 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.
> 

I could argue that I don't like the implictness of lines 1, 2 and 3 here.

The LineConfig defaults are safe.  Allowing the user to redefine the
defaults that apply to a request, separately from where the request is
made mind you, is potentially dangerous.

What if you make the default OUTPUT, and then elsewhere you request
lines and include line 6, forgetting to set it to an INPUT or assuming
that by default you'll get an INPUT?  Kiss that board goodbye.

OTOH the LineConfig "well defined default values" are safe.
Forget to define the config for a line - get a vanilla input.

I wasn't overly concerned about this in the uAPI itself as I was assuming
that at a higher level the lines would be configured separately, and
the higher level language binding would perform the encoding to uAPI.
Turns out that isn't always the case :-|.

Cheers,
Kent.