Re: [libgpiod] cxx bindings: time_point vs duration

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, Oct 15, 2020 at 11:35 AM Helmut Grohne <helmut.grohne@xxxxxxxxxx> wrote:
>
> 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.
>

In case of the event timestamps - we get them from the kernel as
64-bit unsigned integers. They're then converted to struct timespec as
defined by libc and eventually to ::std::chrono:duration. The
timestamps use the MONOTONIC kernel clock - the same that you would
use by calling clock_gettime(CLOCK_MONOTONIC, ...). Is there any way
to couple the C++ time_point to this clock?

> 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.
>

Me neither, but the new user API exposed by the kernel addresses a lot
of issues and adds new features and it's really impossible to keep the
current library API while also leveraging them. I'll keep supporting
the v1.6 stable branch for a long time though.

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

Yes, it's what I intended indeed. I'm however worried that this isn't
the best approach. Having learned more, I now think that lines should
somehow weakly reference the chip - to emphasize that they don't
control its lifetime in any way (which is the case now with each line
storing a hard reference to the chip). Also what happens currently is
the fact that a new line object is created each time get_line() method
is called because we can't store lines in some array within the chip
because that would lead to circular references. Maybe a line should
store a weak_ptr to the underlying ::gpiod_chip? But then it would
create a new chip object every time get_chip() is called. Is there any
way around it? Making the chip and line non-copyable and expediting
any shared_ptr management to the user?

> 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.
>

The thing with gpiod_line struct (the one from C libgpiod, not C++
class) is that the owner is the chip (struct gpiod_chip) - there's no
need to free any resources, they'll be freed when the chip goes out of
scope. You can copy the line pointer all you want, there's always a
single line behind stored in the opaque struct gpiod_chip. So in C++ -
I suppose - the chip should really own the C++ line (stored in a
vector maybe) and the line should at most weakly reference the chip
object. I'm just not sure how to correctly approach this so any advice
is welcome.

Best Regards
Bartosz