[libcamera-devel,v1,1/3] utils: timer: Allow Timer::start to use utils::Duration
diff mbox series

Message ID 20220322131635.2869526-2-naush@raspberrypi.com
State Superseded
Headers show
Series
  • V4L2 dequeue timeout
Related show

Commit Message

Naushir Patuck March 22, 2022, 1:16 p.m. UTC 
Add an overload of the Timer::start member function to accept a utils::Duration
type for the timer duration.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/base/timer.h |  2 ++
 src/libcamera/base/timer.cpp   | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

Comments

Laurent Pinchart March 22, 2022, 8:45 p.m. UTC | #1 
Hi Naush,

Thank you for the patch.

On Tue, Mar 22, 2022 at 01:16:33PM +0000, Naushir Patuck via libcamera-devel wrote:
> Add an overload of the Timer::start member function to accept a utils::Duration
> type for the timer duration.

I'm actually considering moving the Duration class from libcamera-base to
libcamera. The rationale is that the class picks one particular
representation of a duration that we deem suitable for the libcamera
public API. It is of limited usefulness in the base library itself, as
it's not generic. This would prevent the Timer class from using it. How
much hassle is it to use the Timer class in subsequent patches in this
series if we dropped this start() overload ?

> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/base/timer.h |  2 ++
>  src/libcamera/base/timer.cpp   | 14 ++++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/include/libcamera/base/timer.h b/include/libcamera/base/timer.h
> index 09f1d3229bd5..f42a8cfd4d6d 100644
> --- a/include/libcamera/base/timer.h
> +++ b/include/libcamera/base/timer.h
> @@ -14,6 +14,7 @@
>  
>  #include <libcamera/base/object.h>
>  #include <libcamera/base/signal.h>
> +#include <libcamera/base/utils.h>
>  
>  namespace libcamera {
>  
> @@ -27,6 +28,7 @@ public:
>  
>  	void start(unsigned int msec) { start(std::chrono::milliseconds(msec)); }
>  	void start(std::chrono::milliseconds duration);
> +	void start(utils::Duration duration);
>  	void start(std::chrono::steady_clock::time_point deadline);
>  	void stop();
>  	bool isRunning() const;
> diff --git a/src/libcamera/base/timer.cpp b/src/libcamera/base/timer.cpp
> index 187336e3a1a4..3ff526aea8ae 100644
> --- a/src/libcamera/base/timer.cpp
> +++ b/src/libcamera/base/timer.cpp
> @@ -85,6 +85,20 @@ void Timer::start(std::chrono::milliseconds duration)
>  	start(utils::clock::now() + duration);
>  }
>  
> +/**
> + * \brief Start or restart the timer with a timeout of \a duration
> + * \param[in] duration The timer duration given by \a utils::Duration
> + *
> + * If the timer is already running it will be stopped and restarted.
> + *
> + * \context This function is \threadbound.
> + */
> +void Timer::start(utils::Duration duration)
> +{
> +	auto msec = std::chrono::duration_cast<std::chrono::milliseconds>(duration);
> +	start(utils::clock::now() + msec);
> +}
> +
>  /**
>   * \brief Start or restart the timer with a \a deadline
>   * \param[in] deadline The timer deadline
Naushir Patuck March 23, 2022, 9:07 a.m. UTC | #2 
Hi Laurent,


On Tue, 22 Mar 2022 at 20:46, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Tue, Mar 22, 2022 at 01:16:33PM +0000, Naushir Patuck via
> libcamera-devel wrote:
> > Add an overload of the Timer::start member function to accept a
> utils::Duration
> > type for the timer duration.
>
> I'm actually considering moving the Duration class from libcamera-base to
> libcamera. The rationale is that the class picks one particular
> representation of a duration that we deem suitable for the libcamera
> public API. It is of limited usefulness in the base library itself, as
> it's not generic. This would prevent the Timer class from using it. How
> much hassle is it to use the Timer class in subsequent patches in this
> series if we dropped this start() overload ?
>

As mentioned earlier, there is minimal hassle if we decide not to use
utils::Duration
in this API, so I am ok with it.

However, utils::Duration is very nice, so I was curious what would make it
unusable
here if it were in the public API? Would this affect our usage in the IPA
as well?  The
IPA does make significant use of utils::Duration, it would be painful to
stop using it
there.

Regards,
Naush


>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  include/libcamera/base/timer.h |  2 ++
> >  src/libcamera/base/timer.cpp   | 14 ++++++++++++++
> >  2 files changed, 16 insertions(+)
> >
> > diff --git a/include/libcamera/base/timer.h
> b/include/libcamera/base/timer.h
> > index 09f1d3229bd5..f42a8cfd4d6d 100644
> > --- a/include/libcamera/base/timer.h
> > +++ b/include/libcamera/base/timer.h
> > @@ -14,6 +14,7 @@
> >
> >  #include <libcamera/base/object.h>
> >  #include <libcamera/base/signal.h>
> > +#include <libcamera/base/utils.h>
> >
> >  namespace libcamera {
> >
> > @@ -27,6 +28,7 @@ public:
> >
> >       void start(unsigned int msec) {
> start(std::chrono::milliseconds(msec)); }
> >       void start(std::chrono::milliseconds duration);
> > +     void start(utils::Duration duration);
> >       void start(std::chrono::steady_clock::time_point deadline);
> >       void stop();
> >       bool isRunning() const;
> > diff --git a/src/libcamera/base/timer.cpp b/src/libcamera/base/timer.cpp
> > index 187336e3a1a4..3ff526aea8ae 100644
> > --- a/src/libcamera/base/timer.cpp
> > +++ b/src/libcamera/base/timer.cpp
> > @@ -85,6 +85,20 @@ void Timer::start(std::chrono::milliseconds duration)
> >       start(utils::clock::now() + duration);
> >  }
> >
> > +/**
> > + * \brief Start or restart the timer with a timeout of \a duration
> > + * \param[in] duration The timer duration given by \a utils::Duration
> > + *
> > + * If the timer is already running it will be stopped and restarted.
> > + *
> > + * \context This function is \threadbound.
> > + */
> > +void Timer::start(utils::Duration duration)
> > +{
> > +     auto msec =
> std::chrono::duration_cast<std::chrono::milliseconds>(duration);
> > +     start(utils::clock::now() + msec);
> > +}
> > +
> >  /**
> >   * \brief Start or restart the timer with a \a deadline
> >   * \param[in] deadline The timer deadline
>
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart March 23, 2022, 10:53 a.m. UTC | #3 
Hi Naush,

On Wed, Mar 23, 2022 at 09:07:30AM +0000, Naushir Patuck wrote:
> On Tue, 22 Mar 2022 at 20:46, Laurent Pinchart wrote:
> > On Tue, Mar 22, 2022 at 01:16:33PM +0000, Naushir Patuck via
> > libcamera-devel wrote:
> > > Add an overload of the Timer::start member function to accept a
> > utils::Duration
> > > type for the timer duration.
> >
> > I'm actually considering moving the Duration class from libcamera-base to
> > libcamera. The rationale is that the class picks one particular
> > representation of a duration that we deem suitable for the libcamera
> > public API. It is of limited usefulness in the base library itself, as
> > it's not generic. This would prevent the Timer class from using it. How
> > much hassle is it to use the Timer class in subsequent patches in this
> > series if we dropped this start() overload ?
> 
> As mentioned earlier, there is minimal hassle if we decide not to use utils::Duration
> in this API, so I am ok with it.
> 
> However, utils::Duration is very nice, so I was curious what would make it unusable
> here if it were in the public API? Would this affect our usage in the IPA as well?  The
> IPA does make significant use of utils::Duration, it would be painful to stop using it
> there.

You can still use it in the IPA module, but you wouldn't be able to use
it in the base layer itself, as it would be a libcamera concept, not a
libcamera-base (think of libcamera-based as our custom version of boost
:-)) concept.

> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  include/libcamera/base/timer.h |  2 ++
> > >  src/libcamera/base/timer.cpp   | 14 ++++++++++++++
> > >  2 files changed, 16 insertions(+)
> > >
> > > diff --git a/include/libcamera/base/timer.h b/include/libcamera/base/timer.h
> > > index 09f1d3229bd5..f42a8cfd4d6d 100644
> > > --- a/include/libcamera/base/timer.h
> > > +++ b/include/libcamera/base/timer.h
> > > @@ -14,6 +14,7 @@
> > >
> > >  #include <libcamera/base/object.h>
> > >  #include <libcamera/base/signal.h>
> > > +#include <libcamera/base/utils.h>
> > >
> > >  namespace libcamera {
> > >
> > > @@ -27,6 +28,7 @@ public:
> > >
> > >       void start(unsigned int msec) { start(std::chrono::milliseconds(msec)); }
> > >       void start(std::chrono::milliseconds duration);
> > > +     void start(utils::Duration duration);
> > >       void start(std::chrono::steady_clock::time_point deadline);
> > >       void stop();
> > >       bool isRunning() const;
> > > diff --git a/src/libcamera/base/timer.cpp b/src/libcamera/base/timer.cpp
> > > index 187336e3a1a4..3ff526aea8ae 100644
> > > --- a/src/libcamera/base/timer.cpp
> > > +++ b/src/libcamera/base/timer.cpp
> > > @@ -85,6 +85,20 @@ void Timer::start(std::chrono::milliseconds duration)
> > >       start(utils::clock::now() + duration);
> > >  }
> > >
> > > +/**
> > > + * \brief Start or restart the timer with a timeout of \a duration
> > > + * \param[in] duration The timer duration given by \a utils::Duration
> > > + *
> > > + * If the timer is already running it will be stopped and restarted.
> > > + *
> > > + * \context This function is \threadbound.
> > > + */
> > > +void Timer::start(utils::Duration duration)
> > > +{
> > > +     auto msec = std::chrono::duration_cast<std::chrono::milliseconds>(duration);
> > > +     start(utils::clock::now() + msec);
> > > +}
> > > +
> > >  /**
> > >   * \brief Start or restart the timer with a \a deadline
> > >   * \param[in] deadline The timer deadline

Patch
diff mbox series 

diff --git a/include/libcamera/base/timer.h b/include/libcamera/base/timer.h
index 09f1d3229bd5..f42a8cfd4d6d 100644
--- a/include/libcamera/base/timer.h
+++ b/include/libcamera/base/timer.h
@@ -14,6 +14,7 @@ 
 
 #include <libcamera/base/object.h>
 #include <libcamera/base/signal.h>
+#include <libcamera/base/utils.h>
 
 namespace libcamera {
 
@@ -27,6 +28,7 @@  public:
 
 	void start(unsigned int msec) { start(std::chrono::milliseconds(msec)); }
 	void start(std::chrono::milliseconds duration);
+	void start(utils::Duration duration);
 	void start(std::chrono::steady_clock::time_point deadline);
 	void stop();
 	bool isRunning() const;
diff --git a/src/libcamera/base/timer.cpp b/src/libcamera/base/timer.cpp
index 187336e3a1a4..3ff526aea8ae 100644
--- a/src/libcamera/base/timer.cpp
+++ b/src/libcamera/base/timer.cpp
@@ -85,6 +85,20 @@  void Timer::start(std::chrono::milliseconds duration)
 	start(utils::clock::now() + duration);
 }
 
+/**
+ * \brief Start or restart the timer with a timeout of \a duration
+ * \param[in] duration The timer duration given by \a utils::Duration
+ *
+ * If the timer is already running it will be stopped and restarted.
+ *
+ * \context This function is \threadbound.
+ */
+void Timer::start(utils::Duration duration)
+{
+	auto msec = std::chrono::duration_cast<std::chrono::milliseconds>(duration);
+	start(utils::clock::now() + msec);
+}
+
 /**
  * \brief Start or restart the timer with a \a deadline
  * \param[in] deadline The timer deadline