Re: [libgpiod][RFC 0/6] first draft of libgpiod v2.0 API

[Kent Gibson <warthog618@xxxxxxxxx>]

On Sat, Apr 10, 2021 at 04:51:51PM +0200, Bartosz Golaszewski wrote:
> This series introduces an entirely reworked API for the core C part of
> libgpiod. It's not fully functional as the bindings are not modified,
> and starting from patch 5, even the tests stop working. This is on
> purpose as I'd like to reach an agreement on the interface before
> spending time on reworking the tests.
> 
> My plan for the development of v2.0 is to keep the changes in a separate
> branch until all bits & pieces are complete and then rearrange them into
> a bisectable series that will be merged into the master branch.
> 
> A couple points regarding the design of the new API:
> - all objects have become opaque and can only be accessed using dedicated
>   functions
> - line objects as well as bulk containers have been removed
> - line requests are now configured using three types of structures: attributes,
>   line config and request config structures, which follows the kernel API
> - line request handles have now a lifetime separate from the parent chips to
>   leverage the separation of chip and request file descriptors
> - line events are now opaque but they can be stored in a dedicated container
>   so that memory allocations are not necessary everytime an event is read from
>   the kernel
> - the library code has been split into several compilation units for easier
>   maintenance
> 
> The new API is completely documented in include/gpiod.h doxygen comments.
> 
> Please let me know what you think. I am aware that these patches are huge and
> difficult to review but I'm really only expecting a general API review - all
> other implementation details can be improved later.
> 

In that vein, I'll lump my comments here, rather than scattering them
throughout the patches...

Overall it feels much tighter and clearer than the old API, though that
could just be personal bias, as it is also closer to the new uAPI.

I find the ownership and lifetime of objects confusing.  I presume you
want to use the reference counting to simplify the object lifecycle, but
that just formalises the possibility that objects are shared - further
confusing the ownership issue.
e.g. gpiod_line_config_add_attribute()
- who owns the attr after the call?  What happens if it is subsequently
modified? Is the attr copied or does ownership pass to the config?
As written it is neither - the attr becomes shared.  How is that
preferable?
Similarly gpiod_line_get_consumer() - who owns the returned pointer and
what is it's lifetime?
I would prefer the opaque objects to be able to be free()d normally, and
for any calls that involve a change of ownership to explicitly document
that fact.
For objects that require additional setup/cleanup, try to make use of 
open()/close() or request()/release() function name pairings.
So gpiod_chip would have open()/close(), gpiod_request_handle would have
request()/release(), and the others would all be new()/free()d.

Conceptually the config for each line can be considered to be independent
- the uAPI encoding using attributes and masks is only there to keep the
uAPI structs to a manageable size. 
At this level you can model it as config per line, and only map
between the uAPI structures when calling the ioctl()s.
So I would drop the gpiod_line_attr, and instead provide accessors and
mutators on the gpiod_line_config for each attr type.
That removes the whole lifecycle issue for attributes, and allows you to
provide a simpler abstraction of the config than that provided in the
uAPI.
For the mutators there can be two flavours, one that sets the config for
the whole set, and another that accepts a subset of applicable lines.
Accessors would provide the config attr for a particular line.
Both accessor and mutator would use chip offsets to identify the lines.

Not sure I like merging the direction and edge into the request_type.
I would tend to keep those separate, with set_direction and
set_edge_detection functions, with the latter forcing input direction.

I would rename gpiod_request_config to gpiod_request_options.  Config
is long lived and can be modified, whereas options are one-off.
And it would reduce any chance of confusion with gpiod_line_config.

gpiod_line_mask should highlight that the bits correspond to lines on
the request, in the order provided to gpiod_request_config_set_offsets(),
not line offsets on the chip.  Perhaps the gpiod_request_config or the
gpiod_request_handle should provide the mask functions for a given a
qchip offset, rather than require the user to track the mapping?
Then the gpiod_line_mask can be opaque as well.

I would rename gpiod_request_handle to gpiod_line_request.
And request.c to options.c, and handle.c to request.c.

gpiod_line_attr_set_debounce() can use a zero period to identify
disabling debounce, so the debounce flag is redundant.

Cheers,
Kent.