[libcamera-devel] libcamera: utils: Add arithmetic operators for struct timespec

Message ID 20190123082830.10572-1-laurent.pinchart@ideasonboard.com
State RFC, archived
Delegated to: Laurent Pinchart
Headers show
Series
  • [libcamera-devel] libcamera: utils: Add arithmetic operators for struct timespec
Related show

Commit Message

Laurent Pinchart Jan. 23, 2019, 8:28 a.m. UTC
Implement additions and subtraction of struct timespec through
non-member operators to simplify timespec handling.

Signed-off-by: Laurent Pinchart <laurent.pinchart@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

Comments

Kieran Bingham Jan. 23, 2019, 10:19 a.m. UTC | #1
Hi Laurent,

On 23/01/2019 08:28, Laurent Pinchart wrote:
> Implement additions and subtraction of struct timespec through
> non-member operators to simplify timespec handling.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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.


Thank you, I do foresee this being useful in the future when handling
buffer timestamps and such. Even if just for printing debug statements
and measuring durations between frames.

Most of the time the library itself shouldn't care too much about buffer
timestamps, but perhaps even that might change when we look at 3a.

Perhaps because of this I'd almost be tempted to add these utils anyway
if we wrapped a test that used them ? But then we're dragging unused
code around - so it can wait until it's necessary.

--
Kieran


>  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 */
>
Shik Chen Jan. 23, 2019, 3 p.m. UTC | #2
Hi Laurent,

On Wed, Jan 23, 2019 at 4:28 PM Laurent Pinchart <
laurent.pinchart@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

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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

Sincerely,
Shik
Laurent Pinchart Jan. 23, 2019, 4:04 p.m. UTC | #3
Hi Shik,

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

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 ?

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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 */
Shik Chen Jan. 23, 2019, 6:08 p.m. UTC | #4
Hi Laurent,

On Thu, Jan 24, 2019 at 12:04 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Shik,
>
> 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@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.

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

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.

[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@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

Sincerely,
Shik
Laurent Pinchart Jan. 23, 2019, 6:38 p.m. UTC | #5
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@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@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@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 */

Patch

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 */