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

[Kieran Bingham]

Quoting Eric Curtin via libcamera-devel (2022-04-19 15:48:54)
> On Fri, 15 Apr 2022 at 17:25, Laurent Pinchart
> <laurent.pinchart at 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 at 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
> >
>