[libcamera-devel,v7,1/2] cam: event_loop: Add timer events to event loop
diff mbox series

Message ID 20220408160015.33612-1-ecurtin@redhat.com
State Accepted
Headers show
Series
  • [libcamera-devel,v7,1/2] cam: event_loop: Add timer events to event loop
Related show

Commit Message

Eric Curtin April 8, 2022, 4 p.m. UTC 
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.

Signed-off-by: Eric Curtin <ecurtin@redhat.com>
---
 src/cam/event_loop.cpp | 37 +++++++++++++++++++++++++++++++++----
 src/cam/event_loop.h   |  3 +++
 2 files changed, 36 insertions(+), 4 deletions(-)

Comments

Eric Curtin April 15, 2022, 11:14 a.m. UTC | #1 
Hi Guys,

I know holidays are coming up. Just a friendly reminder if you get a
chance to review.

Is mise le meas/Regards,

Eric Curtin


On Fri, 8 Apr 2022 at 17:00, Eric Curtin <ecurtin@redhat.com> 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.
>
> Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> ---
>  src/cam/event_loop.cpp | 37 +++++++++++++++++++++++++++++++++----
>  src/cam/event_loop.h   |  3 +++
>  2 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> index e25784c0..1e1a4269 100644
> --- a/src/cam/event_loop.cpp
> +++ b/src/cam/event_loop.cpp
> @@ -60,6 +60,17 @@ void EventLoop::callLater(const std::function<void()> &func)
>         event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, nullptr);
>  }
>
> +int EventLoop::eventNew(const std::unique_ptr<Event> &event, const int fd, const short events)
> +{
> +       event->event_ = event_new(base_, fd, events, &EventLoop::Event::dispatch, event.get());
> +       if (!event->event_) {
> +               std::cerr << "Failed to create event for fd " << fd << std::endl;
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
>  void EventLoop::addEvent(int fd, EventType type,
>                          const std::function<void()> &callback)
>  {
> @@ -68,10 +79,7 @@ void EventLoop::addEvent(int fd, EventType type,
>                      | (type & Write ? EV_WRITE : 0)
>                      | EV_PERSIST;
>
> -       event->event_ = event_new(base_, fd, events, &EventLoop::Event::dispatch,
> -                                 event.get());
> -       if (!event->event_) {
> -               std::cerr << "Failed to create event for fd " << fd << std::endl;
> +       if (eventNew(event, fd, events)) {
>                 return;
>         }
>
> @@ -84,6 +92,27 @@ void EventLoop::addEvent(int fd, EventType type,
>         events_.push_back(std::move(event));
>  }
>
> +void EventLoop::addTimerEvent(const suseconds_t period,
> +                             const std::function<void()> &callback)
> +{
> +       std::unique_ptr<Event> event = std::make_unique<Event>(callback);
> +       if (eventNew(event, -1, EV_PERSIST)) {
> +               return;
> +       }
> +
> +       struct timeval tv;
> +       tv.tv_sec = 0;
> +       tv.tv_usec = period;
> +
> +       int ret = event_add(event->event_, &tv);
> +       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 a4613eb2..3d3561e5 100644
> --- a/src/cam/event_loop.h
> +++ b/src/cam/event_loop.h
> @@ -36,6 +36,8 @@ public:
>
>         void addEvent(int fd, EventType type,
>                       const std::function<void()> &handler);
> +       void addTimerEvent(const suseconds_t period,
> +                          const std::function<void()> &handler);
>
>  private:
>         struct Event {
> @@ -60,4 +62,5 @@ private:
>         static void dispatchCallback(evutil_socket_t fd, short flags,
>                                      void *param);
>         void dispatchCall();
> +       int eventNew(const std::unique_ptr<Event> &event, const int fd, const short events);
>  };
> --
> 2.35.1
>
Laurent Pinchart April 15, 2022, 4:25 p.m. UTC | #2 
Hi Eric,

Thank you for the patch.

On Fri, Apr 08, 2022 at 05:00:14PM +0100, Eric Curtin via libcamera-devel 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.
> 
> Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> ---
>  src/cam/event_loop.cpp | 37 +++++++++++++++++++++++++++++++++----
>  src/cam/event_loop.h   |  3 +++
>  2 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> index e25784c0..1e1a4269 100644
> --- a/src/cam/event_loop.cpp
> +++ b/src/cam/event_loop.cpp
> @@ -60,6 +60,17 @@ void EventLoop::callLater(const std::function<void()> &func)
>  	event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, nullptr);
>  }
>  
> +int EventLoop::eventNew(const std::unique_ptr<Event> &event, const int fd, const short events)
> +{
> +	event->event_ = event_new(base_, fd, events, &EventLoop::Event::dispatch, event.get());
> +	if (!event->event_) {
> +		std::cerr << "Failed to create event for fd " << fd << std::endl;

This message will be a bit confusing for timer events, as it will print

	Failed to create event for fd -1

As there's really no other code sharing in this function, how about
dropping it, keeping addEvent() unchanged, and called event_new()
directlry in addTimerEvent() with a more appropriate message (such as
"Failed to create timer event") ?

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  void EventLoop::addEvent(int fd, EventType type,
>  			 const std::function<void()> &callback)
>  {
> @@ -68,10 +79,7 @@ void EventLoop::addEvent(int fd, EventType type,
>  		     | (type & Write ? EV_WRITE : 0)
>  		     | EV_PERSIST;
>  
> -	event->event_ = event_new(base_, fd, events, &EventLoop::Event::dispatch,
> -				  event.get());
> -	if (!event->event_) {
> -		std::cerr << "Failed to create event for fd " << fd << std::endl;
> +	if (eventNew(event, fd, events)) {
>  		return;
>  	}
>  
> @@ -84,6 +92,27 @@ void EventLoop::addEvent(int fd, EventType type,
>  	events_.push_back(std::move(event));
>  }
>  
> +void EventLoop::addTimerEvent(const suseconds_t period,
> +			      const std::function<void()> &callback)
> +{
> +	std::unique_ptr<Event> event = std::make_unique<Event>(callback);
> +	if (eventNew(event, -1, EV_PERSIST)) {
> +		return;
> +	}
> +
> +	struct timeval tv;
> +	tv.tv_sec = 0;
> +	tv.tv_usec = period;
> +
> +	int ret = event_add(event->event_, &tv);
> +	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 a4613eb2..3d3561e5 100644
> --- a/src/cam/event_loop.h
> +++ b/src/cam/event_loop.h
> @@ -36,6 +36,8 @@ public:
>  
>  	void addEvent(int fd, EventType type,
>  		      const std::function<void()> &handler);
> +	void addTimerEvent(const suseconds_t period,

This could be called addEvent() too, C++ will pick the right function.
The name addTimerEvent() makes the code more explicit though, which is
nice, but I'd then rename addEvent to addFdEvent() (in a patch before
this one).

> +			   const std::function<void()> &handler);
>  
>  private:
>  	struct Event {
> @@ -60,4 +62,5 @@ private:
>  	static void dispatchCallback(evutil_socket_t fd, short flags,
>  				     void *param);
>  	void dispatchCall();
> +	int eventNew(const std::unique_ptr<Event> &event, const int fd, const short events);
>  };
Eric Curtin April 19, 2022, 2:48 p.m. UTC | #3 
On Fri, 15 Apr 2022 at 17:25, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Eric,
>
> Thank you for the patch.
>
> On Fri, Apr 08, 2022 at 05:00:14PM +0100, Eric Curtin via libcamera-devel 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.
> >
> > Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> > ---
> >  src/cam/event_loop.cpp | 37 +++++++++++++++++++++++++++++++++----
> >  src/cam/event_loop.h   |  3 +++
> >  2 files changed, 36 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> > index e25784c0..1e1a4269 100644
> > --- a/src/cam/event_loop.cpp
> > +++ b/src/cam/event_loop.cpp
> > @@ -60,6 +60,17 @@ void EventLoop::callLater(const std::function<void()> &func)
> >       event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, nullptr);
> >  }
> >
> > +int EventLoop::eventNew(const std::unique_ptr<Event> &event, const int fd, const short events)
> > +{
> > +     event->event_ = event_new(base_, fd, events, &EventLoop::Event::dispatch, event.get());
> > +     if (!event->event_) {
> > +             std::cerr << "Failed to create event for fd " << fd << std::endl;
>
> This message will be a bit confusing for timer events, as it will print
>
>         Failed to create event for fd -1
>
> As there's really no other code sharing in this function, how about
> dropping it, keeping addEvent() unchanged, and called event_new()
> directlry in addTimerEvent() with a more appropriate message (such as
> "Failed to create timer event") ?
>
> > +             return -EINVAL;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  void EventLoop::addEvent(int fd, EventType type,
> >                        const std::function<void()> &callback)
> >  {
> > @@ -68,10 +79,7 @@ void EventLoop::addEvent(int fd, EventType type,
> >                    | (type & Write ? EV_WRITE : 0)
> >                    | EV_PERSIST;
> >
> > -     event->event_ = event_new(base_, fd, events, &EventLoop::Event::dispatch,
> > -                               event.get());
> > -     if (!event->event_) {
> > -             std::cerr << "Failed to create event for fd " << fd << std::endl;
> > +     if (eventNew(event, fd, events)) {
> >               return;
> >       }
> >
> > @@ -84,6 +92,27 @@ void EventLoop::addEvent(int fd, EventType type,
> >       events_.push_back(std::move(event));
> >  }
> >
> > +void EventLoop::addTimerEvent(const suseconds_t period,
> > +                           const std::function<void()> &callback)
> > +{
> > +     std::unique_ptr<Event> event = std::make_unique<Event>(callback);
> > +     if (eventNew(event, -1, EV_PERSIST)) {
> > +             return;
> > +     }
> > +
> > +     struct timeval tv;
> > +     tv.tv_sec = 0;
> > +     tv.tv_usec = period;
> > +
> > +     int ret = event_add(event->event_, &tv);
> > +     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 a4613eb2..3d3561e5 100644
> > --- a/src/cam/event_loop.h
> > +++ b/src/cam/event_loop.h
> > @@ -36,6 +36,8 @@ public:
> >
> >       void addEvent(int fd, EventType type,
> >                     const std::function<void()> &handler);
> > +     void addTimerEvent(const suseconds_t period,
>
> This could be called addEvent() too, C++ will pick the right function.
> The name addTimerEvent() makes the code more explicit though, which is
> nice, but I'd then rename addEvent to addFdEvent() (in a patch before
> this one).

Funnily enough I started changing to addFdEvent, but then reverted as
it was touching many files, and didn't want the patch to spread to so
many files. I'll go back to doing that.

>
> > +                        const std::function<void()> &handler);
> >
> >  private:
> >       struct Event {
> > @@ -60,4 +62,5 @@ private:
> >       static void dispatchCallback(evutil_socket_t fd, short flags,
> >                                    void *param);
> >       void dispatchCall();
> > +     int eventNew(const std::unique_ptr<Event> &event, const int fd, const short events);
> >  };
>
> --
> Regards,
>
> Laurent Pinchart
>
Kieran Bingham April 19, 2022, 3:22 p.m. UTC | #4 
Quoting Eric Curtin via libcamera-devel (2022-04-19 15:48:54)
> On Fri, 15 Apr 2022 at 17:25, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Eric,
> >
> > Thank you for the patch.
> >
> > On Fri, Apr 08, 2022 at 05:00:14PM +0100, Eric Curtin via libcamera-devel 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.
> > >
> > > Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> > > ---
> > >  src/cam/event_loop.cpp | 37 +++++++++++++++++++++++++++++++++----
> > >  src/cam/event_loop.h   |  3 +++
> > >  2 files changed, 36 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> > > index e25784c0..1e1a4269 100644
> > > --- a/src/cam/event_loop.cpp
> > > +++ b/src/cam/event_loop.cpp
> > > @@ -60,6 +60,17 @@ void EventLoop::callLater(const std::function<void()> &func)
> > >       event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, nullptr);
> > >  }
> > >
> > > +int EventLoop::eventNew(const std::unique_ptr<Event> &event, const int fd, const short events)
> > > +{
> > > +     event->event_ = event_new(base_, fd, events, &EventLoop::Event::dispatch, event.get());
> > > +     if (!event->event_) {
> > > +             std::cerr << "Failed to create event for fd " << fd << std::endl;
> >
> > This message will be a bit confusing for timer events, as it will print
> >
> >         Failed to create event for fd -1
> >
> > As there's really no other code sharing in this function, how about
> > dropping it, keeping addEvent() unchanged, and called event_new()
> > directlry in addTimerEvent() with a more appropriate message (such as
> > "Failed to create timer event") ?
> >
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  void EventLoop::addEvent(int fd, EventType type,
> > >                        const std::function<void()> &callback)
> > >  {
> > > @@ -68,10 +79,7 @@ void EventLoop::addEvent(int fd, EventType type,
> > >                    | (type & Write ? EV_WRITE : 0)
> > >                    | EV_PERSIST;
> > >
> > > -     event->event_ = event_new(base_, fd, events, &EventLoop::Event::dispatch,
> > > -                               event.get());
> > > -     if (!event->event_) {
> > > -             std::cerr << "Failed to create event for fd " << fd << std::endl;
> > > +     if (eventNew(event, fd, events)) {
> > >               return;
> > >       }
> > >
> > > @@ -84,6 +92,27 @@ void EventLoop::addEvent(int fd, EventType type,
> > >       events_.push_back(std::move(event));
> > >  }
> > >
> > > +void EventLoop::addTimerEvent(const suseconds_t period,
> > > +                           const std::function<void()> &callback)
> > > +{
> > > +     std::unique_ptr<Event> event = std::make_unique<Event>(callback);
> > > +     if (eventNew(event, -1, EV_PERSIST)) {
> > > +             return;
> > > +     }
> > > +
> > > +     struct timeval tv;
> > > +     tv.tv_sec = 0;
> > > +     tv.tv_usec = period;
> > > +
> > > +     int ret = event_add(event->event_, &tv);
> > > +     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 a4613eb2..3d3561e5 100644
> > > --- a/src/cam/event_loop.h
> > > +++ b/src/cam/event_loop.h
> > > @@ -36,6 +36,8 @@ public:
> > >
> > >       void addEvent(int fd, EventType type,
> > >                     const std::function<void()> &handler);
> > > +     void addTimerEvent(const suseconds_t period,
> >
> > This could be called addEvent() too, C++ will pick the right function.
> > The name addTimerEvent() makes the code more explicit though, which is
> > nice, but I'd then rename addEvent to addFdEvent() (in a patch before
> > this one).
> 
> Funnily enough I started changing to addFdEvent, but then reverted as
> it was touching many files, and didn't want the patch to spread to so
> many files. I'll go back to doing that.

As long as the rename for addFdEvent is in it's own patch, before this
one, that's fine I think.

--
Kieran

> 
> >
> > > +                        const std::function<void()> &handler);
> > >
> > >  private:
> > >       struct Event {
> > > @@ -60,4 +62,5 @@ private:
> > >       static void dispatchCallback(evutil_socket_t fd, short flags,
> > >                                    void *param);
> > >       void dispatchCall();
> > > +     int eventNew(const std::unique_ptr<Event> &event, const int fd, const short events);
> > >  };
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
>

Patch
diff mbox series 

diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
index e25784c0..1e1a4269 100644
--- a/src/cam/event_loop.cpp
+++ b/src/cam/event_loop.cpp
@@ -60,6 +60,17 @@  void EventLoop::callLater(const std::function<void()> &func)
 	event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, nullptr);
 }
 
+int EventLoop::eventNew(const std::unique_ptr<Event> &event, const int fd, const short events)
+{
+	event->event_ = event_new(base_, fd, events, &EventLoop::Event::dispatch, event.get());
+	if (!event->event_) {
+		std::cerr << "Failed to create event for fd " << fd << std::endl;
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 void EventLoop::addEvent(int fd, EventType type,
 			 const std::function<void()> &callback)
 {
@@ -68,10 +79,7 @@  void EventLoop::addEvent(int fd, EventType type,
 		     | (type & Write ? EV_WRITE : 0)
 		     | EV_PERSIST;
 
-	event->event_ = event_new(base_, fd, events, &EventLoop::Event::dispatch,
-				  event.get());
-	if (!event->event_) {
-		std::cerr << "Failed to create event for fd " << fd << std::endl;
+	if (eventNew(event, fd, events)) {
 		return;
 	}
 
@@ -84,6 +92,27 @@  void EventLoop::addEvent(int fd, EventType type,
 	events_.push_back(std::move(event));
 }
 
+void EventLoop::addTimerEvent(const suseconds_t period,
+			      const std::function<void()> &callback)
+{
+	std::unique_ptr<Event> event = std::make_unique<Event>(callback);
+	if (eventNew(event, -1, EV_PERSIST)) {
+		return;
+	}
+
+	struct timeval tv;
+	tv.tv_sec = 0;
+	tv.tv_usec = period;
+
+	int ret = event_add(event->event_, &tv);
+	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 a4613eb2..3d3561e5 100644
--- a/src/cam/event_loop.h
+++ b/src/cam/event_loop.h
@@ -36,6 +36,8 @@  public:
 
 	void addEvent(int fd, EventType type,
 		      const std::function<void()> &handler);
+	void addTimerEvent(const suseconds_t period,
+			   const std::function<void()> &handler);
 
 private:
 	struct Event {
@@ -60,4 +62,5 @@  private:
 	static void dispatchCallback(evutil_socket_t fd, short flags,
 				     void *param);
 	void dispatchCall();
+	int eventNew(const std::unique_ptr<Event> &event, const int fd, const short events);
 };