Re: [libgpiod] Rethinking struct gpiod_line_bulk

[Kent Gibson <warthog618@xxxxxxxxx>]

On Mon, Oct 19, 2020 at 03:31:07PM +0200, Bartosz Golaszewski wrote:
> On Tue, Oct 13, 2020 at 10:53 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> 
> [snip]
> 
> > >
> > > struct gpiod_line_bulk {
> > >     struct gpiod_chip *owner;
> > >     unsigned int num_lines;
> > >     uint64_t lines;
> > > };
> > >
> > > And the 'lines' bitmap should actually refer to offsets at which the
> > > owning chip stores the line pointers in its own 'lines' array - up to
> > > 64 lines.
> > >
> > > But we'd still have to sanitize the values when adding lines to a bulk
> > > object and probably check the return value. I'm wondering if there's a
> > > better way to store group references to lines on the stack but I'm out
> > > of ideas.
> > >
> >
> > So you are proposing keeping the bulk of the bulk in the background and
> > passing around a flyweight in its place.  Makes sense.
> >
> 
> So this won't fly of course because a bitmap doesn't hold enough
> information to reference an arbitrary number of lines in the chip in
> any meaningful way.
> 

Yeah, that is what I was trying to get at earlier, but from your replies
I assumed you would have an array mapping from bitmap index to line
hidden somewhere behind the scenes on a per-bulk basis...

> I have another idea. I can live with struct gpiod_bulk being allocated
> dynamically whenever users of the library use it - because it's quite
> unlikely they'd do it all that often. What I'd like to avoid is
> allocating a new bulk object whenever we want to package a single line
> passed to e.g. gpiod_line_request() before we propagate it to
> gpiod_line_request_bulk().
> 
> How about we define struct gpiod_line_bulk as:
> 
> struct gpiod_line_bulk {
>     unsigned int num_lines;
>     unsigned int max_num_lines;
>     struct gpiod_line *lines[1];
> };
> 
> And expose it in gpiod.h header?
> 

I don't have a problem with that definition, appropriately documented,
but I wouldn't want to expose that to the user, so my preference would
be to make it opaque, but....

... that will make the API more awkward to use, as the methods that
populate a bulk will now have to have that bulk constructed beforehand.
Or you change them to accept a struct gpiod_line_bulk ** to return the
bulk constructed by libgpiod.  Either way the user will have to free the
bulk afterwards.  But there are only a few of those of those methods.

And all the bulk accesor methods are currently inline - that would also
have to change if you go opaque, so you will get a code size and
performance hit as well.

Maybe keeping it exposed is the best - just heavily documented?
At least start out that way, since it is closest to what is already there.

Cheers,
Kent.

> That way we can still allocate it on the stack while using very little
> memory - whenever packaging a single line - or we can allocate it
> dynamically with the following interface:
> 
> struct gpiod_line_bulk *gpiod_line_bulk_new(unsigned int max_lines)
> {
>     struct gpiod_line_bulk *ret;
> 
>     if (max_lines < 1) {
>         errno = EINVAL;
>         return NULL;
>     }
> 
>     ret = malloc(sizeof(struct gpiod_line_bulk) + (max_lines - 1) *
> sizeof(struct gpiod_line *));
>     if (!ret)
>         return NULL;
> 
>     gpiod_line_bulk_init(max_lines);
> 
>     return ret;
> }
> 
> Or we can even not expose it to users, make it completely opaque,
> provide needed accessors and only allow internal users inside the
> library to use the stack for single line bulks.
> 
> Bartosz