Re: [libgpiod] cxx bindings: time_point vs duration

[Helmut Grohne <helmut.grohne@xxxxxxxxxx>]

Hi,

On Thu, Oct 15, 2020 at 11:26:47AM +0200, Bartosz Golaszewski wrote:
> I probably just didn't know any better. :) I'm a kernel developer and
> writing these bindings was basically me learning C++.

Oh, ok. Then let me elaborte on the difference of duration and
time_point a little and explain the implications of changing the type.

A duration denots an interval in time. A time_point denotes a specific
point in time with respect to a particular clock. Every clock has an
epoch and you can convert a time_stamp to a duration by computing the
time_since_epoch. Comparing time_points with different clocks is thus
meaningless. Internally a time_point is represented exactly like a
duration. It just has the connection to the clock.

So what clock would you use here? There are a few standard clocks such
as std::chrono::steady_clock. This clock has an unspecified epoch. The
epoch will not change during use, but e.g. after a reboot of the system,
the epoch may be different. A clock also knows whether time increases
monotonically and a steady_clock does. On the other hand, the
system_clock has a well-specified epoch, but it is not required to be
steady. If you change the time on your machine, your system_clock may go
backwards. While system_clock may be what we want here, it has an
unspecified representation. libgpiod wants a specific representation
though to preserve the high precision of the included timestamps. We
therefore cannot use any of the standard clocks.

libgpiod should add its own clock class. The clock is never instaniated.
It is more a collection of functionality and a tag to be included in
templates to differentiate types. I think your clock would look like
this:

struct gpiod_clock {
    using duration = std::chrono::nanoseconds;
    using rep = duration::rep;
    using period = duration::period;
    using time_point = std::chrono::time_point<gpiod_clock>;
    static constexpr bool is_steady = std::chrono::system_clock::is_steady;
    static time_point now() {
        return time_point(std::chrono::system_clock::now().time_since_epoch());
    }
};

(The code might not work as is, but you get the idea.)

In essence, it's a system_clock with a different underlying duration
type for representing high resolution timestamps.

Even though changing the type does likely not change the binary
representation of timestamps, it still consitutes an API break and an
ABI break (due to changed symbol names).

> Thanks for the suggestion - it's a good moment to make it, because
> we're in the process of changing the API to accommodate the new uAPI
> that will be released in v5.10 so I'll definitely make sure to change
> it too.

Ok. I'm not particularly enthusiastic about updating client code all the
time for covering upstream API breaks, but sometimes it is unavoidable.
Seems to be the case here.

> Are you by any chance well versed in C++ and would like to help out by
> giving me some advice? I want to fix the way GPIO line objects
> reference their owning chips but I'm not sure how to.

I would consider myself experienced in C++.

As far as I can see, the chip class uses the pimpl pattern, so a chip
practically is a reference counted pointer to the actual gpiod_chip. A
line has a plain chip member and this seems totally fine. As long as the
line exists, so does the underlying chip. Is this not what you intended?

What is less clear to me is the connection between a line and its
underlying gpiod_line. It is unclear to me who "owns" a gpiod_line and
who is responsible for disposing it. Since the line class is copy
constructible, the line class clearly cannot own a gpiod_line. I would
question whether this is a good decision. I'm not sure that a line must
be copyable. It should be movable. So if you delete the copy constructor
and the copy assignment for the line class, then you could argue that a
line owns its referenced gpiod_line and then it could automatically
handle release of resources upon destruction.

Does this help?

Helmut