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 02:19:17PM +0200, Bartosz Golaszewski wrote:
> On Tue, Jul 5, 2022 at 4:09 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> 
> [snip]
> 
> > > +PyDoc_STRVAR(chip_get_line_offset_from_name_doc,
> > > +"Map a line's name to its offset within the chip.\n"
> > > +"\n"
> > > +"Args:\n"
> > > +"  name:\n"
> > > +"    Name of the GPIO line to map.\n"
> > > +"\n"
> > > +"Returns:\n"
> > > +"  Line offset corresponding with the name or None if a line with given name\n"
> > > +"  is not exposed by this chip.");
> > > +
> >
> > It should throw if the name search fails.
> >
> > If name is already an int then just return the int.
> > (to allow the method to be used as a mapping function on a mixed
> > list.)  Though ironically the name isn't the best then.
> > Perhaps just get_line_offset() or map_line_offset()?
> >
> 
> Do you think we should change the C function name to
> gpiod_chip_map_line_offset() too? And possibly make it parse strings
> representing integers as well?
> 

No, Python is different due to dynamic typing.

For C, the function we already provide is fundamental.  It can be used
to build the function you are describing, but we should leave it as is.

If you are asking if we should add that higher level mapper as well...
maybe.

> > [snip]
> >
> > Provide a helper to convert timestamp_ns to time.datetime.
> > This one is a bit trickier as the kernel only ever provides monotonic
> > clock, so need to perform the monotonic -> realtime conversion.
> > (for reference my proposed gpiowatch tool does this)
> >
> 
> Should this be put into libgpiod C API directly maybe?
> 

If you mean converting between clocks, like gpiowatch does, I'd rather
not - not something I want to actively encourage.
Only doing it here as a last resort - would prefer to be able to request
the appropriate clock from the kernel.

> > [snip]
> > > +PyDoc_STRVAR(line_config_set_props_default_doc,
> > > +"Set the defaults for properties.\n"
> > > +"\n"
> > > +"Args:\n"
> > > +"  direction:\n"
> > > +"    Default direction.\n"
> > > +"  edge_detection:\n"
> > > +"    Default edge detection.\n"
> > > +"  bias:\n"
> > > +"    Default bias.\n"
> > > +"  drive:\n"
> > > +"    Default drive.\n"
> > > +"  active_low:\n"
> > > +"    Default active-low setting.\n"
> > > +"  debounce_period:\n"
> > > +"    Default debounce period.\n"
> > > +"  event_clock:\n"
> > > +"    Default event clock.\n"
> > > +"  output_value:\n"
> > > +"    Default output value.");
> > > +
> >
> > 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])

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.

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