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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Fri, Jul 8, 2022 at 3:38 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> 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.
>

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
way more confusion than simply providing a set of defaults (either
implicitly from a freshly created LineConfig or by defining a set of
defaults manually). I would argue that deriving the defaults from the
most common values would be much more dangerous than the case you
describe below. :)

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

I disagree. They are defined (if you actually do request them) as:
take the safe defaults from a new LineConfig and set direction to
input and detect both edge interrupts. Without the first call, they
would use the defaults from LineConfig. It's not that complicated.

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

Yes, the implicit defaults are safe. That's not an argument against
allowing the defaults to be EXPLICITLY changed by the user though.

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

I don't buy this argument. The same user could forget any number of
other configuration options. You want to be safe? You use a fresh
LineConfig when requesting lines. There's always a trade-off between
giving more power to users and not letting them shoot themselves in
the foot. It's just a matter of where the line is drawn.

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

It's a question of the most common use-cases vs advanced usage. With
the module-wide request function in Python, the code can be very
concise and intuitive and you don't need to think about defaults and
overrides at all. But if you want to do something else, you can.

Bart