[libcamera-devel,06/11] libcamera: Add event notification infrastructure

155 diff mbox series

Add three new classes, EventDispatcher, EventNotifier and Timer, that
define APIs for file descriptor event notification and timers. The
implementation of the EventDispatcher is meant to be provided to
libcamera by the application.

The event dispatcher is integrated twith the camera manager to implement
automatic registration of timers and events.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/camera_manager.h   |   7 ++
 include/libcamera/event_dispatcher.h |  33 ++++++++
 include/libcamera/event_notifier.h   |  42 ++++++++++
 include/libcamera/libcamera.h        |   1 +
 include/libcamera/meson.build        |   3 +
 include/libcamera/timer.h            |  37 +++++++++
 src/libcamera/camera_manager.cpp     |  42 +++++++++-
 src/libcamera/event_dispatcher.cpp   | 102 +++++++++++++++++++++++
 src/libcamera/event_notifier.cpp     | 118 +++++++++++++++++++++++++++
 src/libcamera/meson.build            |   3 +
 src/libcamera/timer.cpp              | 105 ++++++++++++++++++++++++
 11 files changed, 492 insertions(+), 1 deletion(-)
 create mode 100644 include/libcamera/event_dispatcher.h
 create mode 100644 include/libcamera/event_notifier.h
 create mode 100644 include/libcamera/timer.h
 create mode 100644 src/libcamera/event_dispatcher.cpp
 create mode 100644 src/libcamera/event_notifier.cpp
 create mode 100644 src/libcamera/timer.cpp

Hi Laurent,

Thanks for your work.

On 2019-01-06 04:33:23 +0200, Laurent Pinchart wrote:
> Add three new classes, EventDispatcher, EventNotifier and Timer, that
> define APIs for file descriptor event notification and timers. The
> implementation of the EventDispatcher is meant to be provided to
> libcamera by the application.
> 
> The event dispatcher is integrated twith the camera manager to implement
> automatic registration of timers and events.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I find this patch rather large and would have preferred if had been 
split up to ease review as it adds a few discrete things. That being 
said I like the code none the less :-)

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  include/libcamera/camera_manager.h   |   7 ++
>  include/libcamera/event_dispatcher.h |  33 ++++++++
>  include/libcamera/event_notifier.h   |  42 ++++++++++
>  include/libcamera/libcamera.h        |   1 +
>  include/libcamera/meson.build        |   3 +
>  include/libcamera/timer.h            |  37 +++++++++
>  src/libcamera/camera_manager.cpp     |  42 +++++++++-
>  src/libcamera/event_dispatcher.cpp   | 102 +++++++++++++++++++++++
>  src/libcamera/event_notifier.cpp     | 118 +++++++++++++++++++++++++++
>  src/libcamera/meson.build            |   3 +
>  src/libcamera/timer.cpp              | 105 ++++++++++++++++++++++++
>  11 files changed, 492 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/event_dispatcher.h
>  create mode 100644 include/libcamera/event_notifier.h
>  create mode 100644 include/libcamera/timer.h
>  create mode 100644 src/libcamera/event_dispatcher.cpp
>  create mode 100644 src/libcamera/event_notifier.cpp
>  create mode 100644 src/libcamera/timer.cpp
> 
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 56a3f32d8b6f..cb6a6d084a4a 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -14,6 +14,7 @@ namespace libcamera {
>  
>  class Camera;
>  class DeviceEnumerator;
> +class EventDispatcher;
>  class PipelineHandler;
>  
>  class CameraManager
> @@ -27,11 +28,17 @@ public:
>  
>  	static CameraManager *instance();
>  
> +	void setEventDispatcher(EventDispatcher *dispatcher);
> +	EventDispatcher *eventDispatcher();
> +
>  private:
>  	CameraManager();
> +	~CameraManager();
>  
>  	DeviceEnumerator *enumerator_;
>  	std::vector<PipelineHandler *> pipes_;
> +
> +	EventDispatcher *dispatcher_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/include/libcamera/event_dispatcher.h b/include/libcamera/event_dispatcher.h
> new file mode 100644
> index 000000000000..c20518c6866d
> --- /dev/null
> +++ b/include/libcamera/event_dispatcher.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * event_dispatcher.h - Event dispatcher
> + */
> +#ifndef __LIBCAMERA_EVENT_DISPATCHER_H__
> +#define __LIBCAMERA_EVENT_DISPATCHER_H__
> +
> +#include <vector>
> +
> +namespace libcamera {
> +
> +class EventNotifier;
> +class Timer;
> +
> +class EventDispatcher
> +{
> +public:
> +	virtual ~EventDispatcher();
> +
> +	virtual void registerEventNotifier(EventNotifier *notifier) = 0;
> +	virtual void unregisterEventNotifier(EventNotifier *notifier) = 0;
> +
> +	virtual void registerTimer(Timer *timer) = 0;
> +	virtual void unregisterTimer(Timer *timer) = 0;
> +
> +	virtual void processEvents() = 0;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_EVENT_DISPATCHER_H__ */
> diff --git a/include/libcamera/event_notifier.h b/include/libcamera/event_notifier.h
> new file mode 100644
> index 000000000000..278a5ff6f9e5
> --- /dev/null
> +++ b/include/libcamera/event_notifier.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * event_notifer.h - Event notifier
> + */
> +#ifndef __LIBCAMERA_EVENT_NOTIFIER_H__
> +#define __LIBCAMERA_EVENT_NOTIFIER_H__
> +
> +#include <libcamera/signal.h>
> +
> +namespace libcamera {
> +
> +class EventNotifier
> +{
> +public:
> +	enum Type {
> +		Read,
> +		Write,
> +		Exception,
> +	};
> +
> +	EventNotifier(int fd, Type type);
> +	virtual ~EventNotifier();
> +
> +	Type type() const { return type_; }
> +	int fd() const { return fd_; }
> +
> +	bool isEnabled() const { return enabled_; }
> +	void setEnabled(bool enable);
> +
> +	Signal<EventNotifier *> activated;
> +
> +private:
> +	int fd_;
> +	Type type_;
> +	bool enabled_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_EVENT_NOTIFIER_H__ */
> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h
> index f9556a8bce62..785babefa1c8 100644
> --- a/include/libcamera/libcamera.h
> +++ b/include/libcamera/libcamera.h
> @@ -9,5 +9,6 @@
>  
>  #include <libcamera/camera.h>
>  #include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
>  
>  #endif /* __LIBCAMERA_LIBCAMERA_H__ */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 6f87689ea528..d7cb55ba4a49 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -1,8 +1,11 @@
>  libcamera_api = files([
>      'camera.h',
>      'camera_manager.h',
> +    'event_dispatcher.h',
> +    'event_notifier.h',
>      'libcamera.h',
>      'signal.h',
> +    'timer.h',
>  ])
>  
>  install_headers(libcamera_api,
> diff --git a/include/libcamera/timer.h b/include/libcamera/timer.h
> new file mode 100644
> index 000000000000..97dcc01f493d
> --- /dev/null
> +++ b/include/libcamera/timer.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * timer.h - Generic timer
> + */
> +#ifndef __LIBCAMERA_TIMER_H__
> +#define __LIBCAMERA_TIMER_H__
> +
> +#include <cstdint>
> +
> +#include <libcamera/signal.h>
> +
> +namespace libcamera {
> +
> +class Timer
> +{
> +public:
> +	Timer();
> +
> +	void start(unsigned int msec);
> +	void stop();
> +	bool isRunning() const;
> +
> +	unsigned int interval() const { return interval_; }
> +	uint64_t deadline() const { return deadline_; }
> +
> +	Signal<Timer *> timeout;
> +
> +private:
> +	unsigned int interval_;
> +	uint64_t deadline_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_TIMER_H__ */
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index db2bc4b7424e..0dbf31f47039 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -6,8 +6,10 @@
>   */
>  
>  #include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
>  
>  #include "device_enumerator.h"
> +#include "log.h"
>  #include "pipeline_handler.h"
>  
>  /**
> @@ -37,10 +39,15 @@
>  namespace libcamera {
>  
>  CameraManager::CameraManager()
> -	: enumerator_(nullptr)
> +	: enumerator_(nullptr), dispatcher_(nullptr)
>  {
>  }
>  
> +CameraManager::~CameraManager()
> +{
> +	delete dispatcher_;
> +}
> +
>  /**
>   * \brief Start the camera manager
>   *
> @@ -176,4 +183,37 @@ CameraManager *CameraManager::instance()
>  	return &manager;
>  }
>  
> +/**
> + * \brief Set the event dispatcher
> + * \param dispatcher Pointer to the event dispatcher
> + *
> + * libcamera requires an event dispatcher to integrate event notification and
> + * timers with the application event loop. Applications shall call this function
> + * once and only once before the camera manager is started with start() to set
> + * the event dispatcher.
> + *
> + * The CameraManager takes ownership of the event dispatcher and will delete it
> + * when the application terminates.
> + */
> +void CameraManager::setEventDispatcher(EventDispatcher *dispatcher)
> +{
> +	if (dispatcher_) {
> +		LOG(Warning) << "Event dispatcher is already set";
> +		return;
> +	}
> +
> +	dispatcher_ = dispatcher;
> +}
> +
> +/**
> + * \fn CameraManager::eventDispatcher()
> + * \brief Retrieve the event dispatcher
> + * \return Pointer to the event dispatcher, or nullptr if no event dispatcher
> + * has been set
> + */
> +EventDispatcher *CameraManager::eventDispatcher()
> +{
> +	return dispatcher_;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/event_dispatcher.cpp b/src/libcamera/event_dispatcher.cpp
> new file mode 100644
> index 000000000000..61013f218887
> --- /dev/null
> +++ b/src/libcamera/event_dispatcher.cpp
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * event_dispatcher.cpp - Event dispatcher
> + */
> +
> +#include <libcamera/event_dispatcher.h>
> +
> +/**
> + * \file event_dispatcher.h
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class EventDispatcher
> + * \brief Interface to manage the libcamera events and timers
> + *
> + * The EventDispatcher class allows the integration of the application event
> + * loop with libcamera by abstracting how events and timers are managed and
> + * processed.
> + *
> + * To listen to events, libcamera creates EventNotifier instances and register
> + * them with the dispatcher with registerEventNotifier(). The event notifier
> + * \ref EventNotifier::activated signal is then emitted by the dispatcher
> + * whenever the event is detected.
> + *
> + * To set timers, libcamera creates Timer instances and register them with the
> + * dispatcher with registerTimer(). The timer \ref Timer::timeout signal is then
> + * emitted by the dispatcher when the timer times out.
> + */
> +
> +EventDispatcher::~EventDispatcher()
> +{
> +}
> +
> +/**
> + * \fn EventDispatcher::registerEventNotifier()
> + * \brief Register an event notifier
> + * \param notifier The event notifier to register
> + *
> + * Once the \a notifier is registered with the dispatcher, the dispatcher will
> + * emit the notifier \ref EventNotifier::activated signal whenever a
> + * corresponding event is detected on the notifier's file descriptor. The event
> + * is monitored until the notifier is unregistered with
> + * unregisterEventNotifier().
> + *
> + * Registering multiple notifiers for the same file descriptor and event type is
> + * not allowed.
> + */
> +
> +/**
> + * \fn EventDispatcher::unregisterEventNotifier()
> + * \brief Unregister an event notifier
> + * \param notifier The event notifier to unregister
> + *
> + * After this function returns the \a notifier is guaranteed not to emit the
> + * \ref EventNotifier::activated signal.
> + *
> + * If the notifier isn't registered, this function performs no operation.
> + */
> +
> +/**
> + * \fn EventDispatcher::registerTimer()
> + * \brief Register a timer
> + * \param timer The timer to register
> + *
> + * Once the \a timer is registered with the dispatcher, the dispatcher will emit
> + * the timer \ref Timer::timeout signal when the timer times out. The timer can
> + * be unregistered with unregisterTimer() before it times out, in which case the
> + * signal will not be emitted.
> + *
> + * When the \a timer times out, it is automatically unregistered by the
> + * dispatcher and can be registered back as early as from the \ref Timer::timeout
> + * signal handlers.
> + *
> + * Registering the same timer multiple times is not allowed.
> + */
> +
> +/**
> + * \fn EventDispatcher::unregisterTimer()
> + * \brief Unregister a timer
> + * \param timer The timer to unregister
> + *
> + * After this function returns the \a timer is guaranteed not to emit the
> + * \ref Timer::timeout signal.
> + *
> + * If the timer isn't registered, this function performs no operation.
> + */
> +
> +/**
> + * \fn EventDispatcher::processEvents()
> + * \brief Wait for and process pending events
> + *
> + * This function processes all pending events associated with registered event
> + * notifiers and timers and signals the corresponding EventNotifier and Timer
> + * objects. If no events are pending, it waits for the first event and processes
> + * it before returning.
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/event_notifier.cpp b/src/libcamera/event_notifier.cpp
> new file mode 100644
> index 000000000000..edaf66725dc5
> --- /dev/null
> +++ b/src/libcamera/event_notifier.cpp
> @@ -0,0 +1,118 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * event_notifier.cpp - Event notifier
> + */
> +
> +#include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/event_notifier.h>
> +
> +/**
> + * \file event_notifier.h
> + * \brief Event notifier
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class EventNotifier
> + * \brief Notify of activity on a file descriptor
> + *
> + * The EventNotifier models a file descriptor event source that can be
> + * monitored. It is created with the file descriptor to be monitored and the
> + * type of event, and is enabled by default. It will emit the \ref activated
> + * signal whenever an event of the monitored type occurs on the file descriptor.
> + *
> + * Supported type of events are EventNotifier::Read, EventNotifier::Write and
> + * EventNotifier::Exception. The type is specified when constructed the
> + * notifier, and can be retrieved using the type() function. To listen to
> + * multiple event types on the same file descriptor multiple notifiers must be
> + * created.
> + *
> + * The notifier can be disabled with the setEnable() function. When the notifier
> + * is disabled it ignores events and does not emit the \ref activated signal.
> + * The notifier can then be re-enabled with the setEnable() function.
> + *
> + * Notifier events are detected and dispatched from the
> + * EventDispatcher::processEvents() function.
> + */
> +
> +/**
> + * \enum EventNotifier::Type
> + * Type of file descriptor event to listen for.
> + * \var EventNotifier::Read
> + * Data is available to be read from the file descriptor
> + * \var EventNotifier::Write
> + * Data can be written to the file descriptor
> + * \var EventNotifier::Exception
> + * An exception has occurred on the file descriptor
> + */
> +
> +/**
> + * \brief Construct an event notifier with a file descriptor and event type
> + * \param fd The file descriptor to monitor
> + * \param type The event type to monitor
> + */
> +EventNotifier::EventNotifier(int fd, Type type)
> +	: fd_(fd), type_(type), enabled_(false)
> +{
> +	setEnabled(true);
> +}
> +
> +EventNotifier::~EventNotifier()
> +{
> +	setEnabled(false);
> +}
> +
> +/**
> + * \fn EventNotifier::type()
> + * \brief Retrive the type of the event being monitored
> + * \return The type of the event
> + */
> +
> +/**
> + * \fn EventNotifier::fd()
> + * \brief Retrive the file descriptor being monitored
> + * \return The file descriptor
> + */
> +
> +/**
> + * \fn EventNotifier::isEnabled()
> + * \brief Retrieve the notifier state
> + * \return true if the notifier is enabled, or false otherwise
> + * \sa setEnable()
> + */
> +
> +/**
> + * \brief Enable or disable the notifier
> + * \param enable true to enable the notifier, false to disable it
> + *
> + * This function enables or disables the notifier. A disabled notifier ignores
> + * events and does not emit the \ref activated signal.
> + */
> +void EventNotifier::setEnabled(bool enable)
> +{
> +	if (enabled_ == enable)
> +		return;
> +
> +	enabled_ = enable;
> +
> +	EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
> +	if (enable)
> +		dispatcher->registerEventNotifier(this);
> +	else
> +		dispatcher->unregisterEventNotifier(this);
> +}
> +
> +/**
> + * \var EventNotifier::activated
> + * \brief Signal emitted when the event occurs
> + *
> + * This signal is emitted when the event \ref type() occurs on the file
> + * descriptor monitored by the notifier. The notifier pointer is passed as a
> + * parameter.
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 3ec86e75b57e..61fb93205b34 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -2,11 +2,14 @@ libcamera_sources = files([
>      'camera.cpp',
>      'camera_manager.cpp',
>      'device_enumerator.cpp',
> +    'event_dispatcher.cpp',
> +    'event_notifier.cpp',
>      'log.cpp',
>      'media_device.cpp',
>      'media_object.cpp',
>      'pipeline_handler.cpp',
>      'signal.cpp',
> +    'timer.cpp',
>  ])
>  
>  libcamera_headers = files([
> diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp
> new file mode 100644
> index 000000000000..57c49aa2ef36
> --- /dev/null
> +++ b/src/libcamera/timer.cpp
> @@ -0,0 +1,105 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * timer.cpp - Generic timer
> + */
> +
> +#include <time.h>
> +
> +#include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/timer.h>
> +
> +#include "log.h"
> +
> +/**
> + * \file timer.h
> + * \brief Generic timer
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class Timer
> + * \brief Single-shot timer interface
> + *
> + * The Timer class models a single-shot timer that is started with start() and
> + * emits the \ref timeout signal when it times out.
> + *
> + * Once started the timer will run until it times out. It can be stopped with
> + * stop(), and once it times out or is stopped, can be started again with
> + * start().
> + */
> +
> +/**
> + * \brief Construct a timer
> + */
> +Timer::Timer()
> +{
> +}
> +
> +/**
> + * \brief Start or restart the timer with a timeout of \a msec
> + * \param msec The timer duration in milliseconds
> + *
> + * If the timer is already running it will be stopped and restarted.
> + */
> +void Timer::start(unsigned int msec)
> +{
> +	struct timespec tp;
> +	clock_gettime(CLOCK_MONOTONIC, &tp);
> +
> +	interval_ = msec;
> +	deadline_ = tp.tv_sec * 1000000000ULL + tp.tv_nsec + msec * 1000000;
> +
> +	LOG(Debug) << "Starting timer " << this << " with interval " << msec
> +		   << ": deadline " << deadline_;
> +
> +	CameraManager::instance()->eventDispatcher()->registerTimer(this);
> +}
> +
> +/**
> + * \brief Stop the timer
> + *
> + * After this function returns the timer is guaranteed not to emit the
> + * \ref timeout signal.
> + *
> + * If the timer is not running this function performs no operation.
> + */
> +void Timer::stop()
> +{
> +	CameraManager::instance()->eventDispatcher()->unregisterTimer(this);
> +
> +	deadline_ = 0;
> +}
> +
> +/**
> + * \brief
> + * \return true if the timer is running, false otherwise
> + */
> +bool Timer::isRunning() const
> +{
> +	return deadline_ != 0;
> +}
> +
> +/**
> + * \fn Timer::interval()
> + * \brief Retrieve the timer interval
> + * \return The timer interval in milliseconds
> + */
> +
> +/**
> + * \fn Timer::deadline()
> + * \brief Retrieve the timer deadline
> + * \return The timer deadline in nanoseconds
> + */
> +
> +/**
> + * \var Timer::timeout
> + * \brief Signal emitted when the timer times out
> + *
> + * The timer pointer is passed as a parameter.
> + */
> +
> +} /* namespace libcamera */
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Laurent,
   wow, this is a lot to process... let's try...

On Sun, Jan 06, 2019 at 04:33:23AM +0200, Laurent Pinchart wrote:
> Add three new classes, EventDispatcher, EventNotifier and Timer, that
> define APIs for file descriptor event notification and timers. The
> implementation of the EventDispatcher is meant to be provided to
> libcamera by the application.
>
> The event dispatcher is integrated twith the camera manager to implement
> automatic registration of timers and events.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/camera_manager.h   |   7 ++
>  include/libcamera/event_dispatcher.h |  33 ++++++++
>  include/libcamera/event_notifier.h   |  42 ++++++++++
>  include/libcamera/libcamera.h        |   1 +
>  include/libcamera/meson.build        |   3 +
>  include/libcamera/timer.h            |  37 +++++++++
>  src/libcamera/camera_manager.cpp     |  42 +++++++++-
>  src/libcamera/event_dispatcher.cpp   | 102 +++++++++++++++++++++++
>  src/libcamera/event_notifier.cpp     | 118 +++++++++++++++++++++++++++
>  src/libcamera/meson.build            |   3 +
>  src/libcamera/timer.cpp              | 105 ++++++++++++++++++++++++
>  11 files changed, 492 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/event_dispatcher.h
>  create mode 100644 include/libcamera/event_notifier.h
>  create mode 100644 include/libcamera/timer.h
>  create mode 100644 src/libcamera/event_dispatcher.cpp
>  create mode 100644 src/libcamera/event_notifier.cpp
>  create mode 100644 src/libcamera/timer.cpp
>
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 56a3f32d8b6f..cb6a6d084a4a 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -14,6 +14,7 @@ namespace libcamera {
>
>  class Camera;
>  class DeviceEnumerator;
> +class EventDispatcher;
>  class PipelineHandler;
>
>  class CameraManager
> @@ -27,11 +28,17 @@ public:
>
>  	static CameraManager *instance();
>
> +	void setEventDispatcher(EventDispatcher *dispatcher);
> +	EventDispatcher *eventDispatcher();
> +
>  private:
>  	CameraManager();
> +	~CameraManager();
>
>  	DeviceEnumerator *enumerator_;
>  	std::vector<PipelineHandler *> pipes_;
> +
> +	EventDispatcher *dispatcher_;
>  };
>
>  } /* namespace libcamera */
> diff --git a/include/libcamera/event_dispatcher.h b/include/libcamera/event_dispatcher.h
> new file mode 100644
> index 000000000000..c20518c6866d
> --- /dev/null
> +++ b/include/libcamera/event_dispatcher.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * event_dispatcher.h - Event dispatcher
> + */
> +#ifndef __LIBCAMERA_EVENT_DISPATCHER_H__
> +#define __LIBCAMERA_EVENT_DISPATCHER_H__
> +
> +#include <vector>
> +
> +namespace libcamera {
> +
> +class EventNotifier;
> +class Timer;

Any reason why forward declaring instead of including headers? afaict
there is no risk of circular inclusion...

> +
> +class EventDispatcher
> +{
> +public:
> +	virtual ~EventDispatcher();
> +
> +	virtual void registerEventNotifier(EventNotifier *notifier) = 0;
> +	virtual void unregisterEventNotifier(EventNotifier *notifier) = 0;
> +
> +	virtual void registerTimer(Timer *timer) = 0;
> +	virtual void unregisterTimer(Timer *timer) = 0;
> +
> +	virtual void processEvents() = 0;

Care to clarify me how you see the processEvents() function will be integrated
in the application event loop?

The application should call into this to trigger processing of new
events, and according to the below documentation, this call is expected
to be blocking (for the poll based dispatcher, if no timer is active,
it surely is).

Do we expect application to deal with this running this in a dedicated
thread? Should we take care of that and perform asynchronous event
dispatching?

> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_EVENT_DISPATCHER_H__ */
> diff --git a/include/libcamera/event_notifier.h b/include/libcamera/event_notifier.h
> new file mode 100644
> index 000000000000..278a5ff6f9e5
> --- /dev/null
> +++ b/include/libcamera/event_notifier.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * event_notifer.h - Event notifier
> + */
> +#ifndef __LIBCAMERA_EVENT_NOTIFIER_H__
> +#define __LIBCAMERA_EVENT_NOTIFIER_H__
> +
> +#include <libcamera/signal.h>
> +
> +namespace libcamera {
> +
> +class EventNotifier
> +{
> +public:
> +	enum Type {
> +		Read,
> +		Write,
> +		Exception,
> +	};
> +
> +	EventNotifier(int fd, Type type);
> +	virtual ~EventNotifier();
> +
> +	Type type() const { return type_; }
> +	int fd() const { return fd_; }
> +
> +	bool isEnabled() const { return enabled_; }

We have used the 'enabled()' syntax so far for other getters in the
library. Up to you.


> +	void setEnabled(bool enable);
> +
> +	Signal<EventNotifier *> activated;
> +
> +private:
> +	int fd_;
> +	Type type_;
> +	bool enabled_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_EVENT_NOTIFIER_H__ */
> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h
> index f9556a8bce62..785babefa1c8 100644
> --- a/include/libcamera/libcamera.h
> +++ b/include/libcamera/libcamera.h
> @@ -9,5 +9,6 @@
>
>  #include <libcamera/camera.h>
>  #include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
>
>  #endif /* __LIBCAMERA_LIBCAMERA_H__ */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 6f87689ea528..d7cb55ba4a49 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -1,8 +1,11 @@
>  libcamera_api = files([
>      'camera.h',
>      'camera_manager.h',
> +    'event_dispatcher.h',
> +    'event_notifier.h',
>      'libcamera.h',
>      'signal.h',
> +    'timer.h',
>  ])
>
>  install_headers(libcamera_api,
> diff --git a/include/libcamera/timer.h b/include/libcamera/timer.h
> new file mode 100644
> index 000000000000..97dcc01f493d
> --- /dev/null
> +++ b/include/libcamera/timer.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * timer.h - Generic timer
> + */
> +#ifndef __LIBCAMERA_TIMER_H__
> +#define __LIBCAMERA_TIMER_H__
> +
> +#include <cstdint>
> +
> +#include <libcamera/signal.h>
> +
> +namespace libcamera {
> +
> +class Timer
> +{
> +public:
> +	Timer();
> +
> +	void start(unsigned int msec);
> +	void stop();
> +	bool isRunning() const;
> +
> +	unsigned int interval() const { return interval_; }
> +	uint64_t deadline() const { return deadline_; }
> +
> +	Signal<Timer *> timeout;
> +
> +private:
> +	unsigned int interval_;
> +	uint64_t deadline_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_TIMER_H__ */
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index db2bc4b7424e..0dbf31f47039 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -6,8 +6,10 @@
>   */
>
>  #include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
>
>  #include "device_enumerator.h"
> +#include "log.h"
>  #include "pipeline_handler.h"
>
>  /**
> @@ -37,10 +39,15 @@
>  namespace libcamera {
>
>  CameraManager::CameraManager()
> -	: enumerator_(nullptr)
> +	: enumerator_(nullptr), dispatcher_(nullptr)
>  {
>  }
>
> +CameraManager::~CameraManager()
> +{
> +	delete dispatcher_;
> +}
> +
>  /**
>   * \brief Start the camera manager
>   *
> @@ -176,4 +183,37 @@ CameraManager *CameraManager::instance()
>  	return &manager;
>  }
>
> +/**
> + * \brief Set the event dispatcher
> + * \param dispatcher Pointer to the event dispatcher
> + *
> + * libcamera requires an event dispatcher to integrate event notification and
> + * timers with the application event loop. Applications shall call this function
> + * once and only once before the camera manager is started with start() to set
> + * the event dispatcher.
> + *
> + * The CameraManager takes ownership of the event dispatcher and will delete it
> + * when the application terminates.
> + */
> +void CameraManager::setEventDispatcher(EventDispatcher *dispatcher)
> +{
> +	if (dispatcher_) {
> +		LOG(Warning) << "Event dispatcher is already set";
> +		return;
> +	}
> +
> +	dispatcher_ = dispatcher;
> +}
> +
> +/**
> + * \fn CameraManager::eventDispatcher()
> + * \brief Retrieve the event dispatcher
> + * \return Pointer to the event dispatcher, or nullptr if no event dispatcher
> + * has been set
> + */
> +EventDispatcher *CameraManager::eventDispatcher()
> +{
> +	return dispatcher_;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/event_dispatcher.cpp b/src/libcamera/event_dispatcher.cpp
> new file mode 100644
> index 000000000000..61013f218887
> --- /dev/null
> +++ b/src/libcamera/event_dispatcher.cpp
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * event_dispatcher.cpp - Event dispatcher
> + */
> +
> +#include <libcamera/event_dispatcher.h>
> +
> +/**
> + * \file event_dispatcher.h
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class EventDispatcher
> + * \brief Interface to manage the libcamera events and timers
> + *
> + * The EventDispatcher class allows the integration of the application event
> + * loop with libcamera by abstracting how events and timers are managed and
> + * processed.
> + *
> + * To listen to events, libcamera creates EventNotifier instances and register
> + * them with the dispatcher with registerEventNotifier(). The event notifier
> + * \ref EventNotifier::activated signal is then emitted by the dispatcher
> + * whenever the event is detected.
> + *
> + * To set timers, libcamera creates Timer instances and register them with the
> + * dispatcher with registerTimer(). The timer \ref Timer::timeout signal is then
> + * emitted by the dispatcher when the timer times out.
> + */
> +
> +EventDispatcher::~EventDispatcher()
> +{
> +}
> +
> +/**
> + * \fn EventDispatcher::registerEventNotifier()
> + * \brief Register an event notifier
> + * \param notifier The event notifier to register
> + *
> + * Once the \a notifier is registered with the dispatcher, the dispatcher will
> + * emit the notifier \ref EventNotifier::activated signal whenever a
> + * corresponding event is detected on the notifier's file descriptor. The event
> + * is monitored until the notifier is unregistered with
> + * unregisterEventNotifier().
> + *
> + * Registering multiple notifiers for the same file descriptor and event type is
> + * not allowed.
> + */
> +
> +/**
> + * \fn EventDispatcher::unregisterEventNotifier()
> + * \brief Unregister an event notifier
> + * \param notifier The event notifier to unregister
> + *
> + * After this function returns the \a notifier is guaranteed not to emit the
> + * \ref EventNotifier::activated signal.
> + *
> + * If the notifier isn't registered, this function performs no operation.
> + */
> +
> +/**
> + * \fn EventDispatcher::registerTimer()
> + * \brief Register a timer
> + * \param timer The timer to register
> + *
> + * Once the \a timer is registered with the dispatcher, the dispatcher will emit
> + * the timer \ref Timer::timeout signal when the timer times out. The timer can
> + * be unregistered with unregisterTimer() before it times out, in which case the
> + * signal will not be emitted.
> + *
> + * When the \a timer times out, it is automatically unregistered by the
> + * dispatcher and can be registered back as early as from the \ref Timer::timeout

Is 'back as early as' intentional?

> + * signal handlers.
> + *
> + * Registering the same timer multiple times is not allowed.
> + */
> +
> +/**
> + * \fn EventDispatcher::unregisterTimer()
> + * \brief Unregister a timer
> + * \param timer The timer to unregister
> + *
> + * After this function returns the \a timer is guaranteed not to emit the
> + * \ref Timer::timeout signal.
> + *
> + * If the timer isn't registered, this function performs no operation.
> + */
> +
> +/**
> + * \fn EventDispatcher::processEvents()
> + * \brief Wait for and process pending events
> + *
> + * This function processes all pending events associated with registered event
> + * notifiers and timers and signals the corresponding EventNotifier and Timer
> + * objects. If no events are pending, it waits for the first event and processes
> + * it before returning.
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/event_notifier.cpp b/src/libcamera/event_notifier.cpp
> new file mode 100644
> index 000000000000..edaf66725dc5
> --- /dev/null
> +++ b/src/libcamera/event_notifier.cpp
> @@ -0,0 +1,118 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * event_notifier.cpp - Event notifier

This is actually a 'file events' notifier, it's probably worth
mentioning it somewhere

> + */
> +
> +#include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/event_notifier.h>
> +
> +/**
> + * \file event_notifier.h
> + * \brief Event notifier
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class EventNotifier
> + * \brief Notify of activity on a file descriptor
> + *

Ah, right :)

> + * The EventNotifier models a file descriptor event source that can be
> + * monitored. It is created with the file descriptor to be monitored and the
> + * type of event, and is enabled by default. It will emit the \ref activated
> + * signal whenever an event of the monitored type occurs on the file descriptor.
> + *
> + * Supported type of events are EventNotifier::Read, EventNotifier::Write and
> + * EventNotifier::Exception. The type is specified when constructed the

When the notifier is constructed

> + * notifier, and can be retrieved using the type() function. To listen to
> + * multiple event types on the same file descriptor multiple notifiers must be
> + * created.
> + *
> + * The notifier can be disabled with the setEnable() function. When the notifier
> + * is disabled it ignores events and does not emit the \ref activated signal.
> + * The notifier can then be re-enabled with the setEnable() function.
> + *
> + * Notifier events are detected and dispatched from the
> + * EventDispatcher::processEvents() function.
> + */
> +
> +/**
> + * \enum EventNotifier::Type
> + * Type of file descriptor event to listen for.
> + * \var EventNotifier::Read
> + * Data is available to be read from the file descriptor
> + * \var EventNotifier::Write
> + * Data can be written to the file descriptor
> + * \var EventNotifier::Exception
> + * An exception has occurred on the file descriptor
> + */
> +
> +/**
> + * \brief Construct an event notifier with a file descriptor and event type
> + * \param fd The file descriptor to monitor
> + * \param type The event type to monitor
> + */
> +EventNotifier::EventNotifier(int fd, Type type)
> +	: fd_(fd), type_(type), enabled_(false)
> +{
> +	setEnabled(true);
> +}
> +
> +EventNotifier::~EventNotifier()
> +{
> +	setEnabled(false);
> +}
> +
> +/**
> + * \fn EventNotifier::type()
> + * \brief Retrive the type of the event being monitored
> + * \return The type of the event
> + */
> +
> +/**
> + * \fn EventNotifier::fd()
> + * \brief Retrive the file descriptor being monitored
> + * \return The file descriptor
> + */
> +
> +/**
> + * \fn EventNotifier::isEnabled()
> + * \brief Retrieve the notifier state
> + * \return true if the notifier is enabled, or false otherwise
> + * \sa setEnable()
> + */
> +
> +/**
> + * \brief Enable or disable the notifier
> + * \param enable true to enable the notifier, false to disable it
> + *
> + * This function enables or disables the notifier. A disabled notifier ignores
> + * events and does not emit the \ref activated signal.
> + */
> +void EventNotifier::setEnabled(bool enable)
> +{
> +	if (enabled_ == enable)
> +		return;
> +
> +	enabled_ = enable;
> +
> +	EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
> +	if (enable)
> +		dispatcher->registerEventNotifier(this);
> +	else
> +		dispatcher->unregisterEventNotifier(this);
> +}
> +
> +/**
> + * \var EventNotifier::activated
> + * \brief Signal emitted when the event occurs
> + *
> + * This signal is emitted when the event \ref type() occurs on the file
> + * descriptor monitored by the notifier. The notifier pointer is passed as a
> + * parameter.
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 3ec86e75b57e..61fb93205b34 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -2,11 +2,14 @@ libcamera_sources = files([
>      'camera.cpp',
>      'camera_manager.cpp',
>      'device_enumerator.cpp',
> +    'event_dispatcher.cpp',
> +    'event_notifier.cpp',
>      'log.cpp',
>      'media_device.cpp',
>      'media_object.cpp',
>      'pipeline_handler.cpp',
>      'signal.cpp',
> +    'timer.cpp',
>  ])
>
>  libcamera_headers = files([
> diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp
> new file mode 100644
> index 000000000000..57c49aa2ef36
> --- /dev/null
> +++ b/src/libcamera/timer.cpp
> @@ -0,0 +1,105 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * timer.cpp - Generic timer
> + */
> +
> +#include <time.h>
> +
> +#include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/timer.h>
> +
> +#include "log.h"
> +
> +/**
> + * \file timer.h
> + * \brief Generic timer

Maybe it is just me not having the Signal/Slot thing clear enough, but
it would help mentioning that library components wanting to use timers
(and even notifiers) should instantiate an instance of this class and
connect the timeout (and activated) signals to the slots they want to be
called upon events.

If this is specified somewhere else you can ignore this. If this is
assumed to be known by how Signal/Slot works, maybe it is worth
specifying it here for people not familiar with the concept.

I'm now thinking how I would do that without Signals/Slots, and I
would expect that I would create a Timer and register a timeout
with a callback to be invoked at the timer expiration, and that's basically
it. This can of course be built with the nice Signal/Slot implementation,
but shouldn't we abstract this away to Timer (and EventNotifier) users
providing a method to register a callback with an associated timeout?
What are the advantages of having signal and slot connections exposed?

> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class Timer
> + * \brief Single-shot timer interface
> + *
> + * The Timer class models a single-shot timer that is started with start() and
> + * emits the \ref timeout signal when it times out.
> + *
> + * Once started the timer will run until it times out. It can be stopped with
> + * stop(), and once it times out or is stopped, can be started again with
> + * start().
> + */
> +
> +/**
> + * \brief Construct a timer
> + */
> +Timer::Timer()
> +{
> +}
> +
> +/**
> + * \brief Start or restart the timer with a timeout of \a msec
> + * \param msec The timer duration in milliseconds
> + *
> + * If the timer is already running it will be stopped and restarted.
> + */
> +void Timer::start(unsigned int msec)
> +{
> +	struct timespec tp;
> +	clock_gettime(CLOCK_MONOTONIC, &tp);
> +
> +	interval_ = msec;
> +	deadline_ = tp.tv_sec * 1000000000ULL + tp.tv_nsec + msec * 1000000;
> +
> +	LOG(Debug) << "Starting timer " << this << " with interval " << msec
> +		   << ": deadline " << deadline_;
> +
> +	CameraManager::instance()->eventDispatcher()->registerTimer(this);
> +}
> +
> +/**
> + * \brief Stop the timer
> + *
> + * After this function returns the timer is guaranteed not to emit the
> + * \ref timeout signal.
> + *
> + * If the timer is not running this function performs no operation.
> + */
> +void Timer::stop()
> +{
> +	CameraManager::instance()->eventDispatcher()->unregisterTimer(this);
> +
> +	deadline_ = 0;
> +}
> +
> +/**
> + * \brief
> + * \return true if the timer is running, false otherwise
> + */
> +bool Timer::isRunning() const
> +{
> +	return deadline_ != 0;
> +}
> +
> +/**
> + * \fn Timer::interval()
> + * \brief Retrieve the timer interval
> + * \return The timer interval in milliseconds
> + */
> +
> +/**
> + * \fn Timer::deadline()
> + * \brief Retrieve the timer deadline
> + * \return The timer deadline in nanoseconds
> + */
> +
> +/**
> + * \var Timer::timeout
> + * \brief Signal emitted when the timer times out
> + *
> + * The timer pointer is passed as a parameter.
> + */
> +
> +} /* namespace libcamera */
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Monday, 7 January 2019 20:14:21 EET Jacopo Mondi wrote:
> Hi Laurent,
>    wow, this is a lot to process... let's try...
> 
> On Sun, Jan 06, 2019 at 04:33:23AM +0200, Laurent Pinchart wrote:
> > Add three new classes, EventDispatcher, EventNotifier and Timer, that
> > define APIs for file descriptor event notification and timers. The
> > implementation of the EventDispatcher is meant to be provided to
> > libcamera by the application.
> > 
> > The event dispatcher is integrated twith the camera manager to implement
> > automatic registration of timers and events.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  include/libcamera/camera_manager.h   |   7 ++
> >  include/libcamera/event_dispatcher.h |  33 ++++++++
> >  include/libcamera/event_notifier.h   |  42 ++++++++++
> >  include/libcamera/libcamera.h        |   1 +
> >  include/libcamera/meson.build        |   3 +
> >  include/libcamera/timer.h            |  37 +++++++++
> >  src/libcamera/camera_manager.cpp     |  42 +++++++++-
> >  src/libcamera/event_dispatcher.cpp   | 102 +++++++++++++++++++++++
> >  src/libcamera/event_notifier.cpp     | 118 +++++++++++++++++++++++++++
> >  src/libcamera/meson.build            |   3 +
> >  src/libcamera/timer.cpp              | 105 ++++++++++++++++++++++++
> >  11 files changed, 492 insertions(+), 1 deletion(-)
> >  create mode 100644 include/libcamera/event_dispatcher.h
> >  create mode 100644 include/libcamera/event_notifier.h
> >  create mode 100644 include/libcamera/timer.h
> >  create mode 100644 src/libcamera/event_dispatcher.cpp
> >  create mode 100644 src/libcamera/event_notifier.cpp
> >  create mode 100644 src/libcamera/timer.cpp

[snip]

> > diff --git a/include/libcamera/event_dispatcher.h
> > b/include/libcamera/event_dispatcher.h new file mode 100644
> > index 000000000000..c20518c6866d
> > --- /dev/null
> > +++ b/include/libcamera/event_dispatcher.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * event_dispatcher.h - Event dispatcher
> > + */
> > +#ifndef __LIBCAMERA_EVENT_DISPATCHER_H__
> > +#define __LIBCAMERA_EVENT_DISPATCHER_H__
> > +
> > +#include <vector>
> > +
> > +namespace libcamera {
> > +
> > +class EventNotifier;
> > +class Timer;
> 
> Any reason why forward declaring instead of including headers? afaict
> there is no risk of circular inclusion...

To lower the compilation time and to avoid potential future circular inclusion 
problems.

> > +
> > +class EventDispatcher
> > +{
> > +public:
> > +	virtual ~EventDispatcher();
> > +
> > +	virtual void registerEventNotifier(EventNotifier *notifier) = 0;
> > +	virtual void unregisterEventNotifier(EventNotifier *notifier) = 0;
> > +
> > +	virtual void registerTimer(Timer *timer) = 0;
> > +	virtual void unregisterTimer(Timer *timer) = 0;
> > +
> > +	virtual void processEvents() = 0;
> 
> Care to clarify me how you see the processEvents() function will be
> integrated in the application event loop?

Assuming usage of EventDispatcherPoll, it can be as simple as running, in the 
application,

	CameraManager *cm = CameraManager::instance();
	while (1)
		cm->eventDispatcher()->processEvents();

> The application should call into this to trigger processing of new
> events, and according to the below documentation, this call is expected
> to be blocking (for the poll based dispatcher, if no timer is active,
> it surely is).
> 
> Do we expect application to deal with this running this in a dedicated
> thread? Should we take care of that and perform asynchronous event
> dispatching?

No, applications are not expected to run this in a separate thread, they're 
expected to use the processEvents() function as part of their mail loop. If an 
application uses an external main loop, it is expected to implement the 
register and unregister functions so that its main loop listens for the file 
descriptor events and timers, and call its own implementation of 
processEvents() once the event loop guarantees than an event is available.

With the internal EventDispatcherPoll implementation, the poll() call is 
inside processEvents(). When integrating an external event loop, the 
equivalent to the poll() call is expected to be outside of processEvents(), in 
the external event loop, and processEvents() is expected to be called from the 
main loop to process the events detected by the poll() equivalent.

Note that libcamera does not call processEvents() internally, at least for 
now. If we later need to spawn threads, with per-thread events, I expect we 
will use EventDispatcherPoll internally for implement an event loop for those 
threads. If we then still want to offer to the user the ability to use their 
own event dispatcher for threads internal to libcamera (I'm not sure what the 
use case would be though) then we would have to revisit this.

There is no technical requirement for processEvents() to be blocking right 
now, except for the ability to implement the main event loop in a very simple 
way using the above code. A custom EventDispatcher can thus implement 
processEvents() in a non-blocking fashion as long as the event loop 
implementation has a poll()-like call before calling processEvents(). The 
processEvents() implementation could even be completely empty in that case.

When we will implement support for threads this will be different, the thread 
event dispatcher will need to integrate with our internal thread loop, and 
thus offer a blocking processEvents().

> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_EVENT_DISPATCHER_H__ */
> > diff --git a/include/libcamera/event_notifier.h
> > b/include/libcamera/event_notifier.h new file mode 100644
> > index 000000000000..278a5ff6f9e5
> > --- /dev/null
> > +++ b/include/libcamera/event_notifier.h
> > @@ -0,0 +1,42 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * event_notifer.h - Event notifier
> > + */
> > +#ifndef __LIBCAMERA_EVENT_NOTIFIER_H__
> > +#define __LIBCAMERA_EVENT_NOTIFIER_H__
> > +
> > +#include <libcamera/signal.h>
> > +
> > +namespace libcamera {
> > +
> > +class EventNotifier
> > +{
> > +public:
> > +	enum Type {
> > +		Read,
> > +		Write,
> > +		Exception,
> > +	};
> > +
> > +	EventNotifier(int fd, Type type);
> > +	virtual ~EventNotifier();
> > +
> > +	Type type() const { return type_; }
> > +	int fd() const { return fd_; }
> > +
> > +	bool isEnabled() const { return enabled_; }
> 
> We have used the 'enabled()' syntax so far for other getters in the
> library. Up to you.

I'll change this, consistency is important.

> > +	void setEnabled(bool enable);
> > +
> > +	Signal<EventNotifier *> activated;
> > +
> > +private:
> > +	int fd_;
> > +	Type type_;
> > +	bool enabled_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_EVENT_NOTIFIER_H__ */

[snip]

> > diff --git a/src/libcamera/event_dispatcher.cpp
> > b/src/libcamera/event_dispatcher.cpp new file mode 100644
> > index 000000000000..61013f218887
> > --- /dev/null
> > +++ b/src/libcamera/event_dispatcher.cpp

[snip]

> > +/**
> > + * \fn EventDispatcher::registerTimer()
> > + * \brief Register a timer
> > + * \param timer The timer to register
> > + *
> > + * Once the \a timer is registered with the dispatcher, the dispatcher
> > will emit + * the timer \ref Timer::timeout signal when the timer times
> > out. The timer can + * be unregistered with unregisterTimer() before it
> > times out, in which case the + * signal will not be emitted.
> > + *
> > + * When the \a timer times out, it is automatically unregistered by the
> > + * dispatcher and can be registered back as early as from the \ref
> > Timer::timeout
> 
> Is 'back as early as' intentional?

Yes, "back" refers to "registered back", and the timer can be registered back 
at any point, as early as from the Timer::timeout signal handlers. This points 
out that the timer can be registered back from the signal handler, and that 
the event dispatcher implementation must support that. This is why the 
EventDispatcherPoll::processTimers() function uses a while (!timers_.empty()) 
loop, as the usual range-based for (auto timer : timers_) can't be used if we 
invalidate the iterator from within the loop (which can be the case if timers 
are added from the signal handler).

> > + * signal handlers.
> > + *
> > + * Registering the same timer multiple times is not allowed.
> > + */

[snip]

> > diff --git a/src/libcamera/event_notifier.cpp
> > b/src/libcamera/event_notifier.cpp new file mode 100644
> > index 000000000000..edaf66725dc5
> > --- /dev/null
> > +++ b/src/libcamera/event_notifier.cpp
> > @@ -0,0 +1,118 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * event_notifier.cpp - Event notifier
> 
> This is actually a 'file events' notifier, it's probably worth
> mentioning it somewhere

I'll change it to "File descriptor event notifier".

> > + */
> > +
> > +#include <libcamera/camera_manager.h>
> > +#include <libcamera/event_dispatcher.h>
> > +#include <libcamera/event_notifier.h>
> > +
> > +/**
> > + * \file event_notifier.h
> > + * \brief Event notifier
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class EventNotifier
> > + * \brief Notify of activity on a file descriptor
> > + *
> 
> Ah, right :)
> 
> > + * The EventNotifier models a file descriptor event source that can be
> > + * monitored. It is created with the file descriptor to be monitored and
> > the + * type of event, and is enabled by default. It will emit the \ref
> > activated + * signal whenever an event of the monitored type occurs on
> > the file descriptor. + *
> > + * Supported type of events are EventNotifier::Read, EventNotifier::Write
> > and + * EventNotifier::Exception. The type is specified when constructed
> > the
> 
> When the notifier is constructed

I meant to say "when constructing the notifier". Fixed.

> > + * notifier, and can be retrieved using the type() function. To listen to
> > + * multiple event types on the same file descriptor multiple notifiers
> > must be + * created.
> > + *
> > + * The notifier can be disabled with the setEnable() function. When the
> > notifier + * is disabled it ignores events and does not emit the \ref
> > activated signal. + * The notifier can then be re-enabled with the
> > setEnable() function. + *
> > + * Notifier events are detected and dispatched from the
> > + * EventDispatcher::processEvents() function.
> > + */

[snip]

> > diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp
> > new file mode 100644
> > index 000000000000..57c49aa2ef36
> > --- /dev/null
> > +++ b/src/libcamera/timer.cpp
> > @@ -0,0 +1,105 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * timer.cpp - Generic timer
> > + */
> > +
> > +#include <time.h>
> > +
> > +#include <libcamera/camera_manager.h>
> > +#include <libcamera/event_dispatcher.h>
> > +#include <libcamera/timer.h>
> > +
> > +#include "log.h"
> > +
> > +/**
> > + * \file timer.h
> > + * \brief Generic timer
> 
> Maybe it is just me not having the Signal/Slot thing clear enough, but
> it would help mentioning that library components wanting to use timers
> (and even notifiers) should instantiate an instance of this class and
> connect the timeout (and activated) signals to the slots they want to be
> called upon events.

The documentation for EventNotifier already states that it "will emit the \ref 
activated signal whenever an event of the monitored type occurs on the file 
descriptor". And the Timer "class models a single-shot timer that is started 
with start() and emits the \ref timeout signal when it times out". What do you 
think is missing ?

> If this is specified somewhere else you can ignore this. If this is
> assumed to be known by how Signal/Slot works, maybe it is worth
> specifying it here for people not familiar with the concept.
> 
> I'm now thinking how I would do that without Signals/Slots, and I
> would expect that I would create a Timer and register a timeout
> with a callback to be invoked at the timer expiration, and that's basically
> it. This can of course be built with the nice Signal/Slot implementation,
> but shouldn't we abstract this away to Timer (and EventNotifier) users
> providing a method to register a callback with an associated timeout?
> What are the advantages of having signal and slot connections exposed?

Signals and slots *are* the abstraction layer. They offer connection handling 
(both the EventNotifier and Timer class would otherwise need to implement a 
callback registration system that stores the callback in a list), ease slot 
implementation without boilerplate code (otherwise you would likely have to 
pass both the object pointer as a void pointer and a static method pointer to 
the callback registration, and implement a static callback method that cast 
the void pointer to the appropriate class pointer and calls the corresponding 
class method, and this for every method of every object that can be used as a 
callback), and offer compilation-time type safety.

Compare the following two implementations:

------------------------------- Without signal ------------------------------
class Emitter
{
public:
	struct Callback {
		void *obj;
		void(*func)(int value);
	};

	~Emitter()
	{
		for (Callback *callback : callback_)
			delete callback;
	}

	void registerValueChangedCallback(void *obj, void(*func)(int value))
	{
		callbacks_.push_back(new Callback{obj, func});
	}

	void unregisterValueChangedCallback(void *obj, void(*func)(int value))
	{
		/* Exercise let for the reader */
	}

	void changeValue(int value)
	{
		value_ = value;

		/*
		 * This is flawed as it doesn't handle registration and
		 * unregistration of callbacks from within the callback
		 * functions. A more complex implementation is needed.
		 */
		foreach (Callback *callback : callback_)
			callback->func(callback->obj, value);
	}

private:
	int value_;
	std::list<Callback *> callbacks_;
};

class Observer;

static void static_value_changed_callback(void *obj, int value)
{
	static_cast<Observer *>(obj)->valueChanged(value);
}

class Observer
{
public:
	void valueChanged(int value) { ... };

	void listen(Emitter *e)
	{
		e->registerValueChangedCallback(static_cast<void *>(this),
						static_value_changed_callback);
	}
};
-----------------------------------------------------------------------------

-------------------------------- With signal ---------------------------------
class Emitter
{
public:
	void changeValue(int value)
	{
		value_ = value;
		valueChanged.emit(value);
	}

	Signal<int> valueChanged;

private:
	int value_;
};

class Observer
{
public:
	void valueChanged(int value) { ... };

	void listen(Emitter *e)
	{
		e->valueChanged.connect(this, &Observer::valueChanged);
	}
};-----------------------------------------------------------------------------

Which one do you like best ? :-)

> > + */

[snip]

Hi Laurent,
   not sure it is worth replying, since v2 is out, but since you
spent time writing this....

On Mon, Jan 07, 2019 at 10:25:07PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>

[snip]

> On Monday, 7 January 2019 20:14:21 EET Jacopo Mondi wrote:
> > > diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp
> > > new file mode 100644
> > > index 000000000000..57c49aa2ef36
> > > --- /dev/null
> > > +++ b/src/libcamera/timer.cpp
> > > @@ -0,0 +1,105 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * timer.cpp - Generic timer
> > > + */
> > > +
> > > +#include <time.h>
> > > +
> > > +#include <libcamera/camera_manager.h>
> > > +#include <libcamera/event_dispatcher.h>
> > > +#include <libcamera/timer.h>
> > > +
> > > +#include "log.h"
> > > +
> > > +/**
> > > + * \file timer.h
> > > + * \brief Generic timer
> >
> > Maybe it is just me not having the Signal/Slot thing clear enough, but
> > it would help mentioning that library components wanting to use timers
> > (and even notifiers) should instantiate an instance of this class and
> > connect the timeout (and activated) signals to the slots they want to be
> > called upon events.
>
> The documentation for EventNotifier already states that it "will emit the \ref
> activated signal whenever an event of the monitored type occurs on the file
> descriptor". And the Timer "class models a single-shot timer that is started
> with start() and emits the \ref timeout signal when it times out". What do you
> think is missing ?
>

It was not clear to me that the user of a timer should have had
connected the Timer signal to its own slot. Again, it might just be my
poor understanding of this.

> > If this is specified somewhere else you can ignore this. If this is
> > assumed to be known by how Signal/Slot works, maybe it is worth
> > specifying it here for people not familiar with the concept.
> >
> > I'm now thinking how I would do that without Signals/Slots, and I
> > would expect that I would create a Timer and register a timeout
> > with a callback to be invoked at the timer expiration, and that's basically
> > it. This can of course be built with the nice Signal/Slot implementation,
> > but shouldn't we abstract this away to Timer (and EventNotifier) users
> > providing a method to register a callback with an associated timeout?
> > What are the advantages of having signal and slot connections exposed?
>
> Signals and slots *are* the abstraction layer. They offer connection handling
> (both the EventNotifier and Timer class would otherwise need to implement a
> callback registration system that stores the callback in a list), ease slot
> implementation without boilerplate code (otherwise you would likely have to
> pass both the object pointer as a void pointer and a static method pointer to
> the callback registration, and implement a static callback method that cast
> the void pointer to the appropriate class pointer and calls the corresponding
> class method, and this for every method of every object that can be used as a
> callback), and offer compilation-time type safety.
>
> Compare the following two implementations:
>
> ------------------------------- Without signal ------------------------------
> class Emitter
> {
> public:
> 	struct Callback {
> 		void *obj;
> 		void(*func)(int value);
> 	};
>
> 	~Emitter()
> 	{
> 		for (Callback *callback : callback_)
> 			delete callback;
> 	}
>
> 	void registerValueChangedCallback(void *obj, void(*func)(int value))
> 	{
> 		callbacks_.push_back(new Callback{obj, func});
> 	}
>
> 	void unregisterValueChangedCallback(void *obj, void(*func)(int value))
> 	{
> 		/* Exercise let for the reader */
> 	}
>
> 	void changeValue(int value)
> 	{
> 		value_ = value;
>
> 		/*
> 		 * This is flawed as it doesn't handle registration and
> 		 * unregistration of callbacks from within the callback
> 		 * functions. A more complex implementation is needed.
> 		 */
> 		foreach (Callback *callback : callback_)
> 			callback->func(callback->obj, value);
> 	}
>
> private:
> 	int value_;
> 	std::list<Callback *> callbacks_;
> };
>
> class Observer;
>
> static void static_value_changed_callback(void *obj, int value)
> {
> 	static_cast<Observer *>(obj)->valueChanged(value);
> }
>
> class Observer
> {
> public:
> 	void valueChanged(int value) { ... };
>
> 	void listen(Emitter *e)
> 	{
> 		e->registerValueChangedCallback(static_cast<void *>(this),
> 						static_value_changed_callback);
> 	}
> };
> -----------------------------------------------------------------------------
>
> -------------------------------- With signal ---------------------------------
> class Emitter
> {
> public:
> 	void changeValue(int value)
> 	{
> 		value_ = value;
> 		valueChanged.emit(value);
> 	}
>
> 	Signal<int> valueChanged;
>
> private:
> 	int value_;
> };
>
> class Observer
> {
> public:
> 	void valueChanged(int value) { ... };
>
> 	void listen(Emitter *e)
> 	{
> 		e->valueChanged.connect(this, &Observer::valueChanged);
> 	}
> };-----------------------------------------------------------------------------
>
> Which one do you like best ? :-)

Oh, the Signal one, absolutely.

I was just wondering if the timer class should expose something like

        Timer::registerCallback(void *, void(*func)(...))

and deal internally with the connection to the slot. It is probably
not worth going through all those void * casts, and I'm not sure this
even works. So please ditch my comment.

Thanks
  j

>
> > > + */
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Hi Jacopo,

On Tuesday, 8 January 2019 10:48:41 EET Jacopo Mondi wrote:
> Hi Laurent,
>    not sure it is worth replying, since v2 is out, but since you
> spent time writing this....

It's worth it :-) Please see my answers below.

> On Mon, Jan 07, 2019 at 10:25:07PM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> 
> [snip]
> 
> > On Monday, 7 January 2019 20:14:21 EET Jacopo Mondi wrote:
> > > > diff --git a/src/libcamera/timer.cpp b/src/libcamera/timer.cpp
> > > > new file mode 100644
> > > > index 000000000000..57c49aa2ef36
> > > > --- /dev/null
> > > > +++ b/src/libcamera/timer.cpp
> > > > @@ -0,0 +1,105 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2019, Google Inc.
> > > > + *
> > > > + * timer.cpp - Generic timer
> > > > + */
> > > > +
> > > > +#include <time.h>
> > > > +
> > > > +#include <libcamera/camera_manager.h>
> > > > +#include <libcamera/event_dispatcher.h>
> > > > +#include <libcamera/timer.h>
> > > > +
> > > > +#include "log.h"
> > > > +
> > > > +/**
> > > > + * \file timer.h
> > > > + * \brief Generic timer
> > > 
> > > Maybe it is just me not having the Signal/Slot thing clear enough, but
> > > it would help mentioning that library components wanting to use timers
> > > (and even notifiers) should instantiate an instance of this class and
> > > connect the timeout (and activated) signals to the slots they want to be
> > > called upon events.
> > 
> > The documentation for EventNotifier already states that it "will emit the
> > \ref activated signal whenever an event of the monitored type occurs on
> > the file descriptor". And the Timer "class models a single-shot timer
> > that is started with start() and emits the \ref timeout signal when it
> > times out". What do you think is missing ?
> 
> It was not clear to me that the user of a timer should have had
> connected the Timer signal to its own slot. Again, it might just be my
> poor understanding of this.

I'll try to add a sentence to the Timer and EventNotifier documentation to 
clarify this.

> > > If this is specified somewhere else you can ignore this. If this is
> > > assumed to be known by how Signal/Slot works, maybe it is worth
> > > specifying it here for people not familiar with the concept.
> > > 
> > > I'm now thinking how I would do that without Signals/Slots, and I
> > > would expect that I would create a Timer and register a timeout
> > > with a callback to be invoked at the timer expiration, and that's
> > > basically it. This can of course be built with the nice Signal/Slot
> > > implementation, but shouldn't we abstract this away to Timer (and
> > > EventNotifier) users providing a method to register a callback with an
> > > associated timeout? What are the advantages of having signal and slot
> > > connections exposed?
> > 
> > Signals and slots *are* the abstraction layer. They offer connection
> > handling (both the EventNotifier and Timer class would otherwise need to
> > implement a callback registration system that stores the callback in a
> > list), ease slot implementation without boilerplate code (otherwise you
> > would likely have to pass both the object pointer as a void pointer and a
> > static method pointer to the callback registration, and implement a
> > static callback method that cast the void pointer to the appropriate
> > class pointer and calls the corresponding class method, and this for
> > every method of every object that can be used as a callback), and offer
> > compilation-time type safety.
> > 
> > Compare the following two implementations:
> > 
> > ----------------------------- Without signal ----------------------------
> > class Emitter
> > {
> > public:
> > 	struct Callback {
> > 		void *obj;
> > 		void(*func)(int value);
> > 	};
> > 	
> > 	~Emitter()
> > 	{
> > 		for (Callback *callback : callback_)
> > 			delete callback;
> > 	}
> > 	
> > 	void registerValueChangedCallback(void *obj, void(*func)(int value))
> > 	{
> > 		callbacks_.push_back(new Callback{obj, func});
> > 	}
> > 	
> > 	void unregisterValueChangedCallback(void *obj, void(*func)(int value))
> > 	{
> > 		/* Exercise let for the reader */
> > 	}
> > 	
> > 	void changeValue(int value)
> > 	{
> > 		value_ = value;
> > 		/*
> > 		 * This is flawed as it doesn't handle registration and
> > 		 * unregistration of callbacks from within the callback
> > 		 * functions. A more complex implementation is needed.
> > 		 */
> > 		foreach (Callback *callback : callback_)
> > 			callback->func(callback->obj, value);
> > 	}
> > 
> > private:
> > 	int value_;
> > 	std::list<Callback *> callbacks_;
> > };
> > 
> > class Observer;
> > 
> > static void static_value_changed_callback(void *obj, int value)
> > {
> > 	static_cast<Observer *>(obj)->valueChanged(value);
> > }
> > 
> > class Observer
> > {
> > public:
> > 	void valueChanged(int value) { ... };
> > 	
> > 	void listen(Emitter *e)
> > 	{
> > 		e->registerValueChangedCallback(static_cast<void *>(this),
> > 						static_value_changed_callback);
> > 	}
> > };
> > --------------------------------------------------------------------------
> > 
> > ------------------------------ With signal -------------------------------
> > class Emitter
> > {
> > public:
> > 	void changeValue(int value)
> > 	{
> > 		value_ = value;
> > 		valueChanged.emit(value);
> > 	}
> > 	
> > 	Signal<int> valueChanged;
> > 
> > private:
> > 	int value_;
> > };
> > 
> > class Observer
> > {
> > public:
> > 	void valueChanged(int value) { ... };
> > 	
> > 	void listen(Emitter *e)
> > 	{
> > 		e->valueChanged.connect(this, &Observer::valueChanged);
> > 	}
> > };
> > ------------------------------------------------------------------------
> > 
> > Which one do you like best ? :-)
> 
> Oh, the Signal one, absolutely.

I'm glad you like it too :-)

> I was just wondering if the timer class should expose something like
> 
>         Timer::registerCallback(void *, void(*func)(...))
> 
> and deal internally with the connection to the slot. It is probably
> not worth going through all those void * casts, and I'm not sure this
> even works. So please ditch my comment.

I don't think it's worth it, as the observer would in that case still need to 
define

static void static_value_changed_callback(void *obj, int value)
{
	static_cast<Observer *>(obj)->valueChanged(value);
}

and the registration would be

-		e->valueChanged.connect(this, &Observer::valueChanged);
+		e->registerValueChangedCallback(static_cast<void *>(this),
+						static_value_changed_callback);

which I don't think is easier or clearer.

Note that since v2 signals also support static slots, so you can connect a 
signal to a global function if you don't need to connect it to a particular 
object. We could even implement connection of signals to other signals later 
if needed (so that emitting the first signal would emit the second signal 
automatically).