[libcamera-devel] [PATCH] libcamera: utils: Add arithmetic operators for struct timespec
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 23 19:38:47 CET 2019
Hi Shik,
On Thu, Jan 24, 2019 at 02:08:25AM +0800, Shik Chen wrote:
> On Thu, Jan 24, 2019 at 12:04 AM Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> > On Wed, Jan 23, 2019 at 11:00:21PM +0800, Shik Chen wrote:
> >> On Wed, Jan 23, 2019 at 4:28 PM Laurent Pinchart <
> >> laurent.pinchart at ideasonboard.com> wrote:
> >>>
> >>> Implement additions and subtraction of struct timespec through
> >>> non-member operators to simplify timespec handling.
> >>
> >> IIUC, operator overloading on types of other libs is banned by the style guide
> >> :Q
> >> https://google.github.io/styleguide/cppguide.html#Operator_Overloading
> >
> > But it also states "Don't go out of your way to avoid defining operator
> > overloads. For example, prefer to define ==, =, and <<, rather than
> > Equals(), CopyFrom(), and PrintTo().". Would you recommend create
> > timespecAdd() and timespecSub() functions instead ? I think this is a
> > good case for operator overloading, as adding or subtracting timespec
> > structures will be rejected if utils.h isn't included (so no risk of
> > different behaviours unknown to the developer), and the + and -
> > operators map very well to what we need to do.
>
> IMO the rule "Define operators only on your own types" has higher precedence.
> Two libraries might have conflict definitions if they violate this rule.
That's true, but those operators are internal to the library, they're
not exposed to applications, so that shouldn't be a problem in this
case.
> > For reference, my use case was
> >
> > struct timespec start;
> > clock_gettime(CLOCK_MONOTINIC, &start);
> >
> > ...
> >
> > struct timespec now;
> > clock_gettime(CLOCK_MONOTINIC, &now);
> > struct timespec timeout = old_timeout + start - now;
> >
> > which could be written
> >
> > struct timespec timeout = old_timeout;
> > timespecAdd(&timeout, start);
> > timespecSub(&timeout, now);
> >
> > but is a bit less convenient.
> >
> > What do you advise ?
>
> Ideally most unavoidable C fashion code snippets should be wrapped in idiomatic
> C++ if possible. Can we replace it with standard C++ types such as
> std::chrono::duration and std::chrono::time_point, which already provide
> arithmetic operators? Using things in std::chrono is a little bit verbose and
> template heavy though (it's too flexible...).
That seems pretty painful to use indeed, and the guarantees on
resolution are pretty vague.
> Another option is to create/borrow some time manipulating classes and only
> convert to C structs when needed. A good example is absl time library [1] [2],
> which has a nicer API than std::chrono and can be easily converted to/from the
> types in std::chrono/timespec/timeval.
We could use our own time class, and may do so in the future, or a class
borrowed from another project, but for now, given that we don't need
these operators, I propose deferring the decision.
Please note that I'm using an uint64_t to store a duration in
nanoseconds in the timer code, and only convert from/to timespec as
required per the C library API and/or system calls. We could standardize
on uint64_t durations internally in libcamera. This would still leave
potential arithmetic operations on timespec (or timeval in other
places), but they would be confined and timespec/timeval wouldn't be
used in APIs.
> [1] https://abseil.io/docs/cpp/guides/time
> [2] https://github.com/abseil/abseil-cpp/blob/master/absl/time/time.h
>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>> ---
> >>>
> >>> I wrote this code while working on EINTR handling for the event
> >>> dispatcher, and later refactored the implementation in a way that
> >>> doesn't make use of these operators anymore. I thought they could still
> >>> be useful as reference for later use. This patch is thus not meant to be
> >>> merged now, but can be picked up later if anyone needs it.
> >>>
> >>> src/libcamera/include/utils.h | 6 +++++
> >>> src/libcamera/meson.build | 1 +
> >>> src/libcamera/utils.cpp | 46 +++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 53 insertions(+)
> >>> create mode 100644 src/libcamera/utils.cpp
> >>>
> >>> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> >>> index 73fa2e69b029..7df20976f36f 100644
> >>> --- a/src/libcamera/include/utils.h
> >>> +++ b/src/libcamera/include/utils.h
> >>> @@ -8,6 +8,7 @@
> >>> #define __LIBCAMERA_UTILS_H__
> >>>
> >>> #include <memory>
> >>> +#include <time.h>
> >>>
> >>> #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
> >>>
> >>> @@ -24,6 +25,11 @@ std::unique_ptr<T> make_unique(Args&&... args)
> >>>
> >>> } /* namespace utils */
> >>>
> >>> +struct timespec &operator+=(struct timespec &lhs, const struct timespec &
> >> rhs);
> >>> +struct timespec operator+(struct timespec lhs, const struct timespec &rhs);
> >>> +struct timespec &operator-=(struct timespec &lhs, const struct timespec &
> >> rhs);
> >>> +struct timespec operator-(struct timespec lhs, const struct timespec &rhs);
> >>> +
> >>> } /* namespace libcamera */
> >>>
> >>> #endif /* __LIBCAMERA_UTILS_H__ */
> >>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >>> index f9f25c0ecf15..72d4a194e19a 100644
> >>> --- a/src/libcamera/meson.build
> >>> +++ b/src/libcamera/meson.build
> >>> @@ -11,6 +11,7 @@ libcamera_sources = files([
> >>> 'pipeline_handler.cpp',
> >>> 'signal.cpp',
> >>> 'timer.cpp',
> >>> + 'utils.cpp',
> >>> 'v4l2_device.cpp',
> >>> ])
> >>>
> >>> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> >>> new file mode 100644
> >>> index 000000000000..5014b27d8c7d
> >>> --- /dev/null
> >>> +++ b/src/libcamera/utils.cpp
> >>> @@ -0,0 +1,46 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2018, Google Inc.
> >>> + *
> >>> + * utils.cpp - Miscellaneous utility functions
> >>> + */
> >>> +
> >>> +#include "utils.h"
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +struct timespec &operator+=(struct timespec &lhs, const struct timespec &
> >> rhs)
> >>> +{
> >>> + lhs.tv_sec += rhs.tv_sec;
> >>> + lhs.tv_nsec += rhs.tv_nsec;
> >>> + if (lhs.tv_nsec >= 1000000000) {
> >>> + lhs.tv_sec++;
> >>> + lhs.tv_nsec -= 1000000000;
> >>> + }
> >>> +
> >>> + return lhs;
> >>> +}
> >>> +
> >>> +struct timespec operator+(struct timespec lhs, const struct timespec &rhs)
> >>> +{
> >>> + return lhs += rhs;
> >>> +}
> >>> +
> >>> +struct timespec &operator-=(struct timespec &lhs, const struct timespec &
> >> rhs)
> >>> +{
> >>> + lhs.tv_sec -= rhs.tv_sec;
> >>> + lhs.tv_nsec -= rhs.tv_nsec;
> >>> + if (lhs.tv_nsec < 0) {
> >>> + lhs.tv_sec--;
> >>> + lhs.tv_nsec += 1000000000;
> >>> + }
> >>> +
> >>> + return lhs;
> >>> +}
> >>> +
> >>> +struct timespec operator-(struct timespec lhs, const struct timespec &rhs)
> >>> +{
> >>> + return lhs - rhs;
> >>> +}
> >>> +
> >>> +} /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list