Re: [libgpiod v2.0][PATCH] core: extend config objects

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, Aug 12, 2021 at 3:53 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Thu, Aug 12, 2021 at 02:51:02PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Aug 12, 2021 at 12:29 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > >
> >
> > [snip]
> >
> > > > >
> > > > > My preference would be for gpiod_line_config_set_output_value() and
> > > > > variants to also set the direction for those lines to output.
> > > > > And maybe rename it to gpiod_line_config_set_output().
> > > > > And maybe add a set_input for symmetry.
> > > > >
> > > >
> > > > Any naming idea for setting the direction to "AS_IS"?
> > > >
> > >
> > > Since as-is is vague you need to include the field name.
> > > So I would remove set_direction and replace it with set_input, set_output
> > > and set_direction_as_is (which I would expect to see used very rarely in
> > > the wild, as the only use case I can think of for it is undoing a
> > > set_input or set_output call).
> > >
> > > > > But my concern above was actually the secondary array - that confused me.
> > > > > And it's big - always. (OTOH it's on the heap so who cares?)
> > > > > The array is of size GPIO_V2_LINE_NUM_ATTRS_MAX, yet each entry could
> > > > > have multiple attributes set - so long as the offsets subsets match?
> > > > > What happens if both debounce and flags are set for the same subset?
> > > > > Looks like debounce wins and the flags get discarded in
> > > > > gpiod_line_config_to_kernel().
> > > > >
> > > >
> > > > Yeah, I aimed at ironing it out when writing tests. You're probably right here.
> > > >
> > >
> > > Same reason I hadn't paid much attention to the implementation.
> > >
> > > > > What I had in mind for the config was an array of config for each line,
> > > > > only performing the mapping to uAPI when the actual request or
> > > > > reconfigure is performed, though that requires knowledge of the number
> > > > > of lines in the request to be sized efficiently in the
> > > > > gpiod_line_config_new().  Sizing it as GPIO_V2_LINES_MAX would be even
> > > > > worse than your secondary array, so don't want that.
> > > >
> > > > Or would it? Currently the full config structure is 3784 bytes on a 64
> > > > bit arch. The base config is 32 bytes. If we added the default value
> > > > to base_config that would make it 36 bytes x GPIO_V2_LINES_MAX = 2304
> > > > bytes. We'd need another base_config for default settings.
> > > >
> > > > Unless I'm missing something this does seem like the better option.
> > > >
> > >
> > > Yikes, I overlooked the size of the offsets array in the secondary
> > > config - that is a significant contributor to the config size as well.
> > > For some reason I was thinking that was a bitmap, but that couldn't work.
> > >
> > > In that case a GPIO_V2_LINES_MAX sized array is clearly a better way to
> > > go, and that is a surprise.
> > > Though those array elements will require the line offset as well as the
> > > base_config, unless you have some other way to map offset to config?
> > >
> >
> > No, but that's fine - see below.
> >
> > > > > My Go library uses a map, but that isn't an option here.
> > > > > Resizing it dynamically is the option I was last pondering,
> > > > > but my preference would be to add a num_lines parameter to the new.
> > > > > Anyway, that was what I was wondering about...
> > > > >
> > > >
> > > > We could resize the array dynamically but we'd need to return error
> > > > codes from getters.
> > >
> > > Why? If there is no config for the requested line then you return the
> > > global default value, right?
> > > Why does that change if the array is resizable?
> > > Btw, I'm assuming that the gpiod_line_config would contain a pointer to
> > > the dynamic array, so the handle the user has would remain unchanged.
> > > And the getters all return ints, not pointers to internal fields.
> > > What am I missing?
> >
> > Memory allocation failures when resizing.
> >
>
> Why are you resizing in a getter?
> If you mean setters then fair enough - though you could just sit on
> the error until the request or reconfigure and return it there instead.
>

I meant setters of course. My bad.

Bart