On Fri, Jul 02, 2021 at 10:50:57AM +0200, Bartosz Golaszewski wrote:

On Sun, Jun 27, 2021 at 10:48 AM Kent Gibson [off-list ref] wrote:

quoted

[snip]

quoted

And dislike converting the chip to a bool - it isn't intuitive that
it should mean the underlying handle is open.  Provide a suitably named
method instead, say is_open() or is_closed()?

Similar comment for line_request.

It's actually a common pattern in STL. I find overloaded operators
more intuitive for checking the state of the wrapping object as
opposed to getters for reading the state and properties of the
associated GPIO chip character device stored in said object.

I understand it for simple wrappers like the smart pointers and streams,
but those are very single purpose - chips and line_requests aren't.

If you are using it as a "good to go" indicator then document it
as that, rather than specifically tying to some internal state like the
handle being open.  Does the user even need to know about the handle -
isn't that implementation detail in the pimpl?

[snip]

quoted

quoted

+
+     /**
+      * @brief Retrieve the current snapshot of line information for a
+      *        single line.
+      * @param offset Offset of the line to get the info for.
+      * @param watch Indicates whether the caller wants to watch for line
+      *              info changes.
+      * @return New ::gpiod::line_info object.
+      */
+     line_info get_line_info(unsigned int offset, bool watch = false) const;
+

What is the benefit of the watch option here?
It is clearer to keep the get_line_info() and watch_line_info() distinct,
as they are in the C API.

Actually I would have used this pattern in the C API if I could
overload functions or provide default values for arguments.
watch_line_info(0) is just a helper for get_line_info(0, true). I
would even be fine with just get_line_info() alone with a switch for
watching the lines but since we already have gpiod_watch_line_info(),
I decided to replicate it here.

Fair enough if you have multiple parameter values, like for bias or drive,
but for a bool it is frequently clearer and simpler to split it into two
functions - IMHO.

Further, watch is a very uncommon use case, so I'd prefer to keep them
separate - the average user doesn't even need to know that the watch
capability exists.

[snip]

quoted

quoted

+
+     /**
+      * @brief Constructor.
+      * @param direction Global direction setting.
+      * @param output_values Default output values.
+      * @param edge Global edge detection.
+      * @param bias Global bias setting.
+      * @param drive Global drive setting.
+      * @param active_low Global active_low setting.
+      * @param debounce_period Global debounce period in microseconds.
+      * @param event_clock Default event clock mode.
+      */
+     line_config(int direction = DIRECTION_INPUT,
+                 const line_value_mappings& output_values = no_output_values,
+                 int edge = EDGE_NONE, int bias = BIAS_AS_IS,
+                 int drive = DRIVE_PUSH_PULL, bool active_low = false,
+                 unsigned long debounce_period = 0,
+                 int event_clock = EVENT_CLOCK_MONOTONIC);
+

The parameter ordering should follow the probability of use?
So I would make active_low earlier, before edge and output_values.
And maybe output_values after edge?

Yeah this was my goal. active_low could come before bias and drive but
after output_values IMO. edge could come earlier too (before
output_values).

So you suggest direction, edge, output_values, active_low, bias, drive?

Perhaps you use active low a lot less than I do :-).

I figured that, even if active low is used less frequently than output, 
forcing output users to provide active level was less demanding than
having active level users provide empty ouputs, so I bumped it before
output.  Similarly edge, though I was more on the fence on that one.

[snip]

quoted

quoted

+
+     /**
+      * @brief Set the output values for a set of line offsets.
+      * @param offsets Vector of line offsets for which to set output values.
+      * @param values Vector of new line values with indexes of values
+      *               corresponding to the indexes of offsets.
+      */
+     void set_output_values(const line_offsets& offsets, const line_values& values) const;
+

Painful that the user has to keep track of the offsets independent of
the request.  Any way to provide a method that applies values to the
requested lines without explicitly providing the offsets?
See related get_values() comments below...

I think we already discussed this under the C API patches. The line
config object can live independently from the request config. We could
theoretically pass an array containing just values (with the value at
given index later assigned to the offset at the same index in the
request config) but that would make it less clear IMO. In most cases
the user will already have an array containing the offsets ready in
order to pass it to the request method. I think you mentioned
previously some Go feature that allowed you to do it differently but
in C it's not really possible.

quoted

And in general, what happens if the offsets provided aren't a subset of
the requested offsets?  That applies to the C API as well.

At request-time we'll return EINVAL from set_kernel_output_values()
(or in the case of C++ - we'll throw a system_error exception).

[snip]

quoted

quoted

+
+     /**
+      * @brief Check if this line is debounced (either by hardware or by the
+      *        kernel software debouncer).
+      * @return True if the line is debounced, false otherwise.
+      */
+     bool debounced(void) const noexcept;
+

Not sure if I had the same comment for the C API, but what is the
benefit of this given it is equivalent to debounce_period() == 0?

Clarity of the resulting code.

if (info.debounced())

is clearer than

if (info.debounce_period())

if you're only checking if debouncing is enabled and not what the period is.

[snip]

quoted

quoted

+
+     /**
+      * @brief Check if the object is associated with an open GPIO chip.
+      * @return True if the chip is open, false if it has been closed.
+      */
+     explicit operator bool(void) const noexcept;

This is even less intuitive than for chip.
It could mean chip is open, or line is requested, or ...
So make it a method with an appropriate name instead.

Well it would be much less confusing if I didn't stupidly copy-paste
the comment from the chip... It was supposed to say: Check if this
request hasn't been released or something along those lines.

[snip]

quoted

quoted

+
+     /**
+      * @brief Read the values of a subset of requested lines.
+      * @param offsets Vector of line offsets
+      * @return Vector of lines values with indexes of values corresponding
+      *         to those of the offsets.
+      */
+     line_values get_values(const line_offsets& offsets) const;
+

A common use case would be to get all lines in the request.
As written, to do that the user needs to maintain their own copy of the
offsets.

Maybe if offsets is empty then returns all requested lines?  And provide
line_offsets() as a default parameter?

Same argument applies for the C API.
In gpiod.h you could rename the existing _get_values() to

int gpiod_line_request_get_values_subset(struct gpiod_line_request *request,
                                  unsigned num_lines,
                                  const unsigned int *offsets, int *values);

and add

int gpiod_line_request_get_values(struct gpiod_line_request *request, int *values);

Or interpret a zero offsets param to mean use the requested offsets??

Hmm, I can't find it now but I could swear we discussed this
previously and you raised some issue with this. I'm definitely in
favor of that so let me add it in the next iteration.

Perhaps you are thinking of the discussion we had regarding
line_config all/offset/subset variants?
I think we just forgot to apply the same logic to the get/sets here.
I don't recall having any problem with it, but if anything comes to me
I'll let you know.

[snip]

quoted

quoted

+
+     /**
+      * @brief Constructor.
+      * @param offsets Vector of line offsets to request.
+      * @param consumer Consumer name.
+      * @param event_buffer_size Event buffer size.
+      */
+     request_config(const line_offsets& offsets = line_offsets(),
+                    const ::std::string& consumer = ::std::string(),
+                    unsigned int event_buffer_size = 0);
+

When would you call this without providing offsets?

This is to mirror the C API which allows to create an empty request
config. If the user never sets the offsets, we'll catch that at
request-time.

Yeah, I get that, but when would a user ever need an empty
request_config?  If they never need it then why give them the option?
That also applies to the C API.

Is there ever a need to mutate the offsets in the request_config?
If not, just make the offsets manditory in the new/constructor?
Just putting the idea out there.


All minor points, so fine whichever way you go with them.

Cheers,
Kent.