| Message ID | 20220505151851.2421-1-ecurtin@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
On Thu, 5 May 2022 at 16:19, 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. > > Also delete all events in the list, before we event_base_loopbreak. > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > --- > src/cam/event_loop.cpp | 33 +++++++++++++++++++++++++++++++++ > src/cam/event_loop.h | 6 ++++++ > 2 files changed, 39 insertions(+) > > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp > index e25784c0..181c971c 100644 > --- a/src/cam/event_loop.cpp > +++ b/src/cam/event_loop.cpp > @@ -47,6 +47,10 @@ int EventLoop::exec() > void EventLoop::exit(int code) > { > exitCode_ = code; > + for (auto const &e : events_) { > + event_del(e->event_); > + } > + > event_base_loopbreak(base_); > } > > @@ -84,6 +88,35 @@ void EventLoop::addEvent(int fd, EventType type, > events_.push_back(std::move(event)); > } > > +void EventLoop::toTimeval(const std::chrono::milliseconds d, struct timeval &tv) > +{ > + tv.tv_sec = std::chrono::duration_cast<std::chrono::seconds>(d).count(); > + tv.tv_usec = std::chrono::duration_cast<std::chrono::microseconds>(d).count(); > +} > + > +void EventLoop::addTimerEvent(const std::chrono::milliseconds d, > + 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; > + toTimeval(d, tv); > + > + 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..89215dce 100644 > --- a/src/cam/event_loop.h > +++ b/src/cam/event_loop.h > @@ -14,6 +14,8 @@ > > #include <event2/util.h> > > +using namespace std::chrono_literals; > + > struct event_base; > > class EventLoop > @@ -37,6 +39,9 @@ public: > void addEvent(int fd, EventType type, > const std::function<void()> &handler); > > + void addTimerEvent(const std::chrono::milliseconds d, > + const std::function<void()> &handler); > + > private: > struct Event { > Event(const std::function<void()> &callback); > @@ -57,6 +62,7 @@ private: > std::list<std::unique_ptr<Event>> events_; > std::mutex lock_; > > + void toTimeval(const std::chrono::milliseconds d, struct timeval &tv); > static void dispatchCallback(evutil_socket_t fd, short flags, > void *param); > void dispatchCall(); > -- > 2.35.1 > Hi Guys, Friendly reminder if you could review once you get a spare moment. Regards, Eric
Another friendly reminder. In case you guys get the opportunity. On Wed 11 May 2022, 12:33 Eric Curtin, <ecurtin@redhat.com> wrote: > On Thu, 5 May 2022 at 16:19, 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. > > > > Also delete all events in the list, before we event_base_loopbreak. > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > --- > > src/cam/event_loop.cpp | 33 +++++++++++++++++++++++++++++++++ > > src/cam/event_loop.h | 6 ++++++ > > 2 files changed, 39 insertions(+) > > > > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp > > index e25784c0..181c971c 100644 > > --- a/src/cam/event_loop.cpp > > +++ b/src/cam/event_loop.cpp > > @@ -47,6 +47,10 @@ int EventLoop::exec() > > void EventLoop::exit(int code) > > { > > exitCode_ = code; > > + for (auto const &e : events_) { > > + event_del(e->event_); > > + } > > + > > event_base_loopbreak(base_); > > } > > > > @@ -84,6 +88,35 @@ void EventLoop::addEvent(int fd, EventType type, > > events_.push_back(std::move(event)); > > } > > > > +void EventLoop::toTimeval(const std::chrono::milliseconds d, struct > timeval &tv) > > +{ > > + tv.tv_sec = > std::chrono::duration_cast<std::chrono::seconds>(d).count(); > > + tv.tv_usec = > std::chrono::duration_cast<std::chrono::microseconds>(d).count(); > > +} > > + > > +void EventLoop::addTimerEvent(const std::chrono::milliseconds d, > > + 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; > > + toTimeval(d, tv); > > + > > + 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..89215dce 100644 > > --- a/src/cam/event_loop.h > > +++ b/src/cam/event_loop.h > > @@ -14,6 +14,8 @@ > > > > #include <event2/util.h> > > > > +using namespace std::chrono_literals; > > + > > struct event_base; > > > > class EventLoop > > @@ -37,6 +39,9 @@ public: > > void addEvent(int fd, EventType type, > > const std::function<void()> &handler); > > > > + void addTimerEvent(const std::chrono::milliseconds d, > > + const std::function<void()> &handler); > > + > > private: > > struct Event { > > Event(const std::function<void()> &callback); > > @@ -57,6 +62,7 @@ private: > > std::list<std::unique_ptr<Event>> events_; > > std::mutex lock_; > > > > + void toTimeval(const std::chrono::milliseconds d, struct timeval > &tv); > > static void dispatchCallback(evutil_socket_t fd, short flags, > > void *param); > > void dispatchCall(); > > -- > > 2.35.1 > > > > Hi Guys, > > Friendly reminder if you could review once you get a spare moment. > > Regards, > > Eric >
Quoting Eric Curtin via libcamera-devel (2022-05-05 16:18:48) > 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. > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > --- > src/cam/event_loop.cpp | 33 +++++++++++++++++++++++++++++++++ > src/cam/event_loop.h | 6 ++++++ > 2 files changed, 39 insertions(+) > > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp > index e25784c0..181c971c 100644 > --- a/src/cam/event_loop.cpp > +++ b/src/cam/event_loop.cpp > @@ -47,6 +47,10 @@ int EventLoop::exec() > void EventLoop::exit(int code) > { > exitCode_ = code; > + for (auto const &e : events_) { > + event_del(e->event_); > + } No need for { } on a single line statement. > + > event_base_loopbreak(base_); > } > > @@ -84,6 +88,35 @@ void EventLoop::addEvent(int fd, EventType type, > events_.push_back(std::move(event)); > } > > +void EventLoop::toTimeval(const std::chrono::milliseconds d, struct timeval &tv) > +{ > + tv.tv_sec = std::chrono::duration_cast<std::chrono::seconds>(d).count(); Do you need to subtract the seconds from here now? > + tv.tv_usec = std::chrono::duration_cast<std::chrono::microseconds>(d).count(); Or does this wrap accordingly? I.e. would 1.5 seconds end up as : tv.tv_sec = 1; tv.tv_usec = 1500000; With that resolved, I think that's a Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > +} > + > +void EventLoop::addTimerEvent(const std::chrono::milliseconds d, > + 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; > + toTimeval(d, tv); It's a shame that it's frowned upon to override std namespace to have custom constructors or having a construction to allow: struct timeval tv(d); would be nice ... (but I don't think it's something we should do here). -- Kieran > + > + 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..89215dce 100644 > --- a/src/cam/event_loop.h > +++ b/src/cam/event_loop.h > @@ -14,6 +14,8 @@ > > #include <event2/util.h> > > +using namespace std::chrono_literals; > + > struct event_base; > > class EventLoop > @@ -37,6 +39,9 @@ public: > void addEvent(int fd, EventType type, > const std::function<void()> &handler); > > + void addTimerEvent(const std::chrono::milliseconds d, > + const std::function<void()> &handler); > + > private: > struct Event { > Event(const std::function<void()> &callback); > @@ -57,6 +62,7 @@ private: > std::list<std::unique_ptr<Event>> events_; > std::mutex lock_; > > + void toTimeval(const std::chrono::milliseconds d, struct timeval &tv); > static void dispatchCallback(evutil_socket_t fd, short flags, > void *param); > void dispatchCall(); > -- > 2.35.1 >
Hi Eric, On Thu, May 05, 2022 at 04:18:48PM +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. > > Also delete all events in the list, before we event_base_loopbreak. > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> Kieran pointed me to your series, and I've just tested it and I can say it works fine for me Tested-by: Jacopo Mondi <jacopo@jmondi.org> Hope it can be merged soon even if I haven't really looked at the patches though, but since you're at v8 I assume they're in pretty good shape! Thanks j > --- > src/cam/event_loop.cpp | 33 +++++++++++++++++++++++++++++++++ > src/cam/event_loop.h | 6 ++++++ > 2 files changed, 39 insertions(+) > > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp > index e25784c0..181c971c 100644 > --- a/src/cam/event_loop.cpp > +++ b/src/cam/event_loop.cpp > @@ -47,6 +47,10 @@ int EventLoop::exec() > void EventLoop::exit(int code) > { > exitCode_ = code; > + for (auto const &e : events_) { > + event_del(e->event_); > + } > + > event_base_loopbreak(base_); > } > > @@ -84,6 +88,35 @@ void EventLoop::addEvent(int fd, EventType type, > events_.push_back(std::move(event)); > } > > +void EventLoop::toTimeval(const std::chrono::milliseconds d, struct timeval &tv) > +{ > + tv.tv_sec = std::chrono::duration_cast<std::chrono::seconds>(d).count(); > + tv.tv_usec = std::chrono::duration_cast<std::chrono::microseconds>(d).count(); > +} > + > +void EventLoop::addTimerEvent(const std::chrono::milliseconds d, > + 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; > + toTimeval(d, tv); > + > + 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..89215dce 100644 > --- a/src/cam/event_loop.h > +++ b/src/cam/event_loop.h > @@ -14,6 +14,8 @@ > > #include <event2/util.h> > > +using namespace std::chrono_literals; > + > struct event_base; > > class EventLoop > @@ -37,6 +39,9 @@ public: > void addEvent(int fd, EventType type, > const std::function<void()> &handler); > > + void addTimerEvent(const std::chrono::milliseconds d, > + const std::function<void()> &handler); > + > private: > struct Event { > Event(const std::function<void()> &callback); > @@ -57,6 +62,7 @@ private: > std::list<std::unique_ptr<Event>> events_; > std::mutex lock_; > > + void toTimeval(const std::chrono::milliseconds d, struct timeval &tv); > static void dispatchCallback(evutil_socket_t fd, short flags, > void *param); > void dispatchCall(); > -- > 2.35.1 >
Hi Eric, On Tue, May 17, 2022 at 08:19:34PM +0100, Kieran Bingham via libcamera-devel wrote: > Quoting Eric Curtin via libcamera-devel (2022-05-05 16:18:48) > > 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. Can you extend the commit message to explain why this is needed ? > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > --- > > src/cam/event_loop.cpp | 33 +++++++++++++++++++++++++++++++++ > > src/cam/event_loop.h | 6 ++++++ > > 2 files changed, 39 insertions(+) > > > > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp > > index e25784c0..181c971c 100644 > > --- a/src/cam/event_loop.cpp > > +++ b/src/cam/event_loop.cpp > > @@ -47,6 +47,10 @@ int EventLoop::exec() > > void EventLoop::exit(int code) > > { > > exitCode_ = code; > > + for (auto const &e : events_) { > > + event_del(e->event_); > > + } > > No need for { } on a single line statement. And as events_ is a vector of unique_ptr, just events_.clear(); will do. > > + > > event_base_loopbreak(base_); > > } > > > > @@ -84,6 +88,35 @@ void EventLoop::addEvent(int fd, EventType type, > > events_.push_back(std::move(event)); > > } > > > > +void EventLoop::toTimeval(const std::chrono::milliseconds d, struct timeval &tv) > > +{ > > + tv.tv_sec = std::chrono::duration_cast<std::chrono::seconds>(d).count(); > > Do you need to subtract the seconds from here now? > > > + tv.tv_usec = std::chrono::duration_cast<std::chrono::microseconds>(d).count(); > > Or does this wrap accordingly? > > I.e. would 1.5 seconds end up as : > tv.tv_sec = 1; > tv.tv_usec = 1500000; Looks like an issue indeed. There's an implementation in src/libcamera/base/utils.h that you can copy: uint64_t usecs = std::chrono::duration_cast<std::chrono::microseconds(d).count(); tv.tv_sec = usecs / 1000000ULL; tv.tv_usec = usecs % 1000000ULL; As this is only three lines of code, and used once in addTimerEvent(), I'd just move it to that function and drom toTimeval(). > With that resolved, I think that's a > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > +} > > + > > +void EventLoop::addTimerEvent(const std::chrono::milliseconds d, As this adds a periodic timer event, let's call the variable period (or interval, up to you) instead of d. > > + 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; > > + toTimeval(d, tv); > > It's a shame that it's frowned upon to override std namespace to have > custom constructors or having a construction to allow: > struct timeval tv(d); > > would be nice ... (but I don't think it's something we should do here). That's something that the C++ standard library could have provided. We have a helper function in base/utils.h but that's not public. > > + > > + 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..89215dce 100644 > > --- a/src/cam/event_loop.h > > +++ b/src/cam/event_loop.h > > @@ -14,6 +14,8 @@ You should include <chrono> here. > > > > #include <event2/util.h> > > > > +using namespace std::chrono_literals; I think this can be dropped, as you don't use literals in this file. With these small changes you'll have my Rb tag. > > + > > struct event_base; > > > > class EventLoop > > @@ -37,6 +39,9 @@ public: > > void addEvent(int fd, EventType type, > > const std::function<void()> &handler); > > > > + void addTimerEvent(const std::chrono::milliseconds d, > > + const std::function<void()> &handler); > > + > > private: > > struct Event { > > Event(const std::function<void()> &callback); > > @@ -57,6 +62,7 @@ private: > > std::list<std::unique_ptr<Event>> events_; > > std::mutex lock_; > > > > + void toTimeval(const std::chrono::milliseconds d, struct timeval &tv); > > static void dispatchCallback(evutil_socket_t fd, short flags, > > void *param); > > void dispatchCall();
diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp index e25784c0..181c971c 100644 --- a/src/cam/event_loop.cpp +++ b/src/cam/event_loop.cpp @@ -47,6 +47,10 @@ int EventLoop::exec() void EventLoop::exit(int code) { exitCode_ = code; + for (auto const &e : events_) { + event_del(e->event_); + } + event_base_loopbreak(base_); } @@ -84,6 +88,35 @@ void EventLoop::addEvent(int fd, EventType type, events_.push_back(std::move(event)); } +void EventLoop::toTimeval(const std::chrono::milliseconds d, struct timeval &tv) +{ + tv.tv_sec = std::chrono::duration_cast<std::chrono::seconds>(d).count(); + tv.tv_usec = std::chrono::duration_cast<std::chrono::microseconds>(d).count(); +} + +void EventLoop::addTimerEvent(const std::chrono::milliseconds d, + 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; + toTimeval(d, tv); + + 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..89215dce 100644 --- a/src/cam/event_loop.h +++ b/src/cam/event_loop.h @@ -14,6 +14,8 @@ #include <event2/util.h> +using namespace std::chrono_literals; + struct event_base; class EventLoop @@ -37,6 +39,9 @@ public: void addEvent(int fd, EventType type, const std::function<void()> &handler); + void addTimerEvent(const std::chrono::milliseconds d, + const std::function<void()> &handler); + private: struct Event { Event(const std::function<void()> &callback); @@ -57,6 +62,7 @@ private: std::list<std::unique_ptr<Event>> events_; std::mutex lock_; + void toTimeval(const std::chrono::milliseconds d, struct timeval &tv); static void dispatchCallback(evutil_socket_t fd, short flags, void *param); void dispatchCall();
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. Signed-off-by: Eric Curtin <ecurtin@redhat.com> --- src/cam/event_loop.cpp | 33 +++++++++++++++++++++++++++++++++ src/cam/event_loop.h | 6 ++++++ 2 files changed, 39 insertions(+)