[libcamera-devel] [PATCH v1 1/3] utils: timer: Allow Timer::start to use utils::Duration

[Laurent Pinchart]

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