[libcamera-devel] [PATCH v9 2/4] cam: event_loop: Add timer events to event loop

[Laurent Pinchart]

Hi Eric,

On Sun, May 22, 2022 at 01:07:47PM +0300, Laurent Pinchart via libcamera-devel wrote:
> On Fri, May 20, 2022 at 08:01:04PM +0100, Eric Curtin wrote:
> > Extend the EventLoop class to support periodic timer events. This can be
> > used to run tasks periodically, such as handling the event loop of SDL.
> > 
> > Also delete all events in the list, before we event_base_loopbreak, in
> > an effort to avoid race conditions.
> 
> What race condition in particular does this solve ?

This is the only part I'm not sure about in the whole series. If there's
a race condition you've identified, could you please describe it ? I'll
then update the commit message accordingly and push. If the race is
theoritical only, I'd prefer splitting this to a separate patch, to be
discussed separately.

> > Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Tested-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/cam/event_loop.cpp | 27 +++++++++++++++++++++++++++
> >  src/cam/event_loop.h   |  7 ++++++-
> >  2 files changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> > index 2e3ce995..315da38a 100644
> > --- a/src/cam/event_loop.cpp
> > +++ b/src/cam/event_loop.cpp
> > @@ -47,6 +47,8 @@ int EventLoop::exec()
> >  void EventLoop::exit(int code)
> >  {
> >  	exitCode_ = code;
> > +	events_.clear();
> > +
> >  	event_base_loopbreak(base_);
> >  }
> >  
> > @@ -84,6 +86,31 @@ void EventLoop::addFdEvent(int fd, EventType type,
> >  	events_.push_back(std::move(event));
> >  }
> >  
> > +void EventLoop::addTimerEvent(const duration period,
> > +			      const std::function<void()> &callback)
> > +{
> > +	std::unique_ptr<Event> event = std::make_unique<Event>(callback);
> > +	event->event_ = event_new(base_, -1, EV_PERSIST, &EventLoop::Event::dispatch,
> > +				  event.get());
> > +	if (!event->event_) {
> > +		std::cerr << "Failed to create timer event" << std::endl;
> > +		return;
> > +	}
> > +
> > +	struct timeval tv;
> > +	const uint64_t usecs = std::chrono::duration_cast<std::chrono::microseconds>(period).count();
> > +	tv.tv_sec = usecs / 1000000ULL;
> > +	tv.tv_usec = usecs % 1000000ULL;
> > +
> > +	const int ret = event_add(event->event_, &tv);
> 
> No need for const here.
> 
> > +	if (ret < 0) {
> > +		std::cerr << "Failed to add timer event" << std::endl;
> > +		return;
> > +	}
> > +
> > +	events_.push_back(std::move(event));
> > +}
> > +
> >  void EventLoop::dispatchCallback([[maybe_unused]] evutil_socket_t fd,
> >  				 [[maybe_unused]] short flags, void *param)
> >  {
> > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> > index 79902d87..22769ea5 100644
> > --- a/src/cam/event_loop.h
> > +++ b/src/cam/event_loop.h
> > @@ -7,9 +7,10 @@
> >  
> >  #pragma once
> >  
> > +#include <chrono>
> >  #include <functional>
> > -#include <memory>
> >  #include <list>
> > +#include <memory>
> >  #include <mutex>
> >  
> >  #include <event2/util.h>
> > @@ -37,6 +38,10 @@ public:
> >  	void addFdEvent(int fd, EventType type,
> >  			const std::function<void()> &handler);
> >  
> > +	using duration = std::chrono::steady_clock::duration;
> 
> Is there a need to use the duration of steady_clock, is there any
> drawback in using microseconds or milliseconds ?
> 
> I can address these small issues when applying if the other patches in
> the series are fine.
> 
> > +	void addTimerEvent(const duration period,
> > +			   const std::function<void()> &handler);
> > +
> >  private:
> >  	struct Event {
> >  		Event(const std::function<void()> &callback);

-- 
Regards,

Laurent Pinchart