On Tue, Aug 10, 2021 at 12:31 PM Kent Gibson [off-list ref] wrote:

On Tue, Aug 10, 2021 at 09:52:04AM +0200, Bartosz Golaszewski wrote:

quoted

On Mon, Aug 9, 2021 at 1:10 AM Kent Gibson [off-list ref] wrote:

quoted

On Sun, Aug 08, 2021 at 09:11:14PM +0200, Bartosz Golaszewski wrote:

quoted

On Sat, Aug 7, 2021 at 10:48 AM Kent Gibson [off-list ref] wrote:

quoted

On Fri, Aug 06, 2021 at 03:28:10PM +0200, Bartosz Golaszewski wrote:

quoted

Kent suggested that we may want to add getters for the config objects in
his reviews under the C++ patches. Indeed when working on Python bindings
I noticed it would be useful for implementing __str__ and __repr__
callbacks. In C++ too we could use them for overloading stream operators.

This extends the config objects with getters. They are straightforward for
the request config but for the line config, they allow to only read
per-offset values that would be used if the object was used in a request
at this moment. We also add getters for the output values: both taking
the line offset as argument as well as ones that take the index and allow
to iterate over all configured output values.

The sanitization of input for the getters has subsequently been changed
so that we never return invalid values. The input values are verified
immediately and if an invalid value is passed, it's silently replaced
by the default value for given setting.

This patch also adds the reset function for the line config object - it
can be used to reset all stored configuration if - for example - the
config has become too complex.

As this patch will be squashed into the big v2 patch anyway, I allowed
myself to include some additional changes: some variable renames and
other minor tweaks.

Signed-off-by: Bartosz Golaszewski <redacted>

A few minor nit-picks in the gpiod.h documentation below...

Cheers,
Kent.

Thanks,

With that fixed, do you think it's good to be applied?

Sure.

I was also wondering if anything could be done to simplify the
structures in line-config.c, but that isn't specific to this patch.
Not having access to the offsets, or even num_lines, and doing the
allocation up-front makes it rather painful.  Especially if the most
common case is only one or two lines.
But, as long as you are happy with the external API, that is just
implementation detail that can be optimised later.

Cheers,
Kent.

I'm fine with how it works now for most part. I understand your
concerns about splitting the output value configuration from the
requested offsets but I think we can live with it. If anything: we can
add a function to set an array of output values in line_config where
the offsets would be assigned automatically based on the index in the
array. Something like this:

int values[] = { 0, 0, 1, 1, 0, 1 };
unsigned int offsets[] = { 0, 3, 4, 7, 12, 13 };

gpiod_line_config_set_output_values_auto(line_cfg, 6, values);
gpiod_request_config_set_offsets(req_cfg, 6, offsets);

request = gpiod_chip_request_lines(chip, line_cfg, req_cfg);

This would result in the following mapping: 0 -> 0, 3 -> 0, 4 -> 1, 7
-> 1, 12 -> 0, 13 -> 1.

We could store the auto array as a separate array in line_config and
the offset -> value mappings would take precedence. For
reconfiguration we would do the same. Does this make sense?

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"?

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.

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.

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. We could also define the size when allocating the
config but it's a sub-par approach too.

Bart