[libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot communication mechanism
Jacopo Mondi
jacopo at jmondi.org
Mon Jan 7 17:15:58 CET 2019
Hi Laurent,
a few more things I have noticed while staring at path 6/11..
On Sun, Jan 06, 2019 at 04:33:22AM +0200, Laurent Pinchart wrote:
> Introduce a Signal class that allows connecting event sources (signals)
> to event listeners (slots) without adding any boilerplate code usually
> associated with the observer or listener design patterns.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> Documentation/Doxyfile.in | 3 +-
> include/libcamera/meson.build | 1 +
> include/libcamera/signal.h | 117 ++++++++++++++++++++++++++++++++++
> src/libcamera/meson.build | 1 +
> src/libcamera/signal.cpp | 44 +++++++++++++
> 5 files changed, 165 insertions(+), 1 deletion(-)
> create mode 100644 include/libcamera/signal.h
> create mode 100644 src/libcamera/signal.cpp
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index b1a70d36eee5..558a1ce04377 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -860,7 +860,8 @@ EXCLUDE_PATTERNS =
> # Note that the wildcards are matched against the file with absolute path, so to
> # exclude all test directories use the pattern */test/*
>
> -EXCLUDE_SYMBOLS =
> +EXCLUDE_SYMBOLS = libcamera::SlotBase \
> + libcamera::Slot
>
> # The EXAMPLE_PATH tag can be used to specify one or more files or directories
> # that contain example code fragments that are included (see the \include
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 3e04557d66b1..6f87689ea528 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -2,6 +2,7 @@ libcamera_api = files([
> 'camera.h',
> 'camera_manager.h',
> 'libcamera.h',
> + 'signal.h',
> ])
>
> install_headers(libcamera_api,
> diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h
> new file mode 100644
> index 000000000000..fceb852158ec
> --- /dev/null
> +++ b/include/libcamera/signal.h
> @@ -0,0 +1,117 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * signal.h - Signal & slot implementation
> + */
> +#ifndef __LIBCAMERA_SIGNAL_H__
> +#define __LIBCAMERA_SIGNAL_H__
> +
> +#include <list>
> +#include <vector>
> +
> +namespace libcamera {
> +
> +template<typename... Args>
> +class Signal;
> +
> +template<typename... Args>
> +class SlotBase
> +{
> +public:
> + SlotBase(void *obj)
> + : obj_(obj) { }
> + virtual ~SlotBase() { }
> +
> + virtual void invoke(Args... args) = 0;
> +
> +protected:
> + friend class Signal<Args...>;
> + void *obj_;
> +};
> +
> +template<typename T, typename... Args>
> +class Slot : public SlotBase<Args...>
> +{
> +public:
> + Slot(T *obj, void(T::*func)(Args...))
> + : SlotBase<Args...>(obj), func_(func) { }
> +
> + void invoke(Args... args) { (reinterpret_cast<T *>(this->obj_)->*func_)(args...); }
> +
> +private:
> + friend class Signal<Args...>;
> + void(T::*func_)(Args...);
> +};
> +
> +template<typename... Args>
> +class Signal
> +{
> +public:
> + Signal() { }
> + ~Signal()
> + {
> + for (SlotBase<Args...> *slot : slots_)
> + delete slot;
> + }
> +
> + template<typename T>
> + void connect(T *object, void(T::*func)(Args...))
> + {
> + slots_.push_back(new Slot<T, Args...>(object, func));
> + }
> +
> + void disconnect()
> + {
> + for (SlotBase<Args...> *slot : slots_)
> + delete slot;
> + slots_.clear();
> + }
> +
> + template<typename T>
> + void disconnect(T *object)
> + {
> + for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> + SlotBase<Args...> *slot = *iter;
> + if (slot->obj_ == object) {
> + iter = slots_.erase(iter);
> + delete slot;
> + } else {
> + ++iter;
> + }
> + }
> + }
> +
> + template<typename T>
> + void disconnect(T *object, void(T::*func)(Args...))
> + {
> + for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> + SlotBase<Args...> *slot = *iter;
I wonder if it wouldn't be cleaner to downcast this to Slot here
instead of in the condition
> + if (slot->obj_ == object &&
> + reinterpret_cast<Slot<T, Args...> *>(slot)->func_ == func) {
> + iter = slots_.erase(iter);
> + delete slot;
> + } else {
> + ++iter;
> + }
This one and the above would probably read better as:
template<typename T>
void disconnect(T *object)
{
for (auto iter = slots_.begin(); iter != slots_.end(); ) {
auto *slot = reinterpret_cast<Slot<T, Args...> *>(*iter);
if (slot->obj_ != object)
++iter;
iter = slots_.erase(iter);
delete slot;
}
}
template<typename T>
void disconnect(T *object, void(T::*func)(Args...))
{
for (auto iter = slots_.begin(); iter != slots_.end(); ) {
auto *slot = reinterpret_cast<Slot<T, Args...> *>(*iter);
if (slot->obj_ != object || slot->func_ != func) {
++ iter;
continue;
}
iter = slots_.erase(iter);
delete slot;
break; <--- I think you could break here, or
could the same slot be registered
twice? In that case, would this
call delete all of them or just
the first one?
}
}
> + }
> +
> + void emit(Args... args)
> + {
> + /*
> + * Make a copy of the slots list as the slot could call the
> + * disconnect operation, invalidating the iterator.
> + */
> + std::vector<SlotBase<Args...> *> slots{ slots_.begin(), slots_.end() };
> + for (SlotBase<Args...> *slot : slots)
> + slot->invoke(args...);
> + }
> +
> +private:
> + std::list<SlotBase<Args...> *> slots_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_SIGNAL_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 78562299fc42..3ec86e75b57e 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -6,6 +6,7 @@ libcamera_sources = files([
> 'media_device.cpp',
> 'media_object.cpp',
> 'pipeline_handler.cpp',
> + 'signal.cpp',
> ])
>
> libcamera_headers = files([
> diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
> new file mode 100644
> index 000000000000..8b5a6c285c55
> --- /dev/null
> +++ b/src/libcamera/signal.cpp
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * signal.cpp - Signal & slot implementation
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class Signal
> + * \brief Generic signal and slot communication mechanism
> + *
> + * Signals and slots are a language construct aimed at communication between
> + * objects through the observer pattern without the need for boilerplate code.
> + * See http://doc.qt.io/qt-5/signalsandslots.html for more information.
As much as I could like Qt's signals and slots, I feel for the
Qt-uneducated ones, the documentation we have here is pretty thin. Mentioning
the Qt implementation (from which we borrow the concept and the
terminology) is imho not enough, and a few more words in the functions
documentation might help. At least, I would have apreciated to have
them here when i first tried to get my head around this.
> + */
> +
> +/**
> + * \fn Signal::connect()
> + * \brief Connect the signal to a slot
Slots are not part of the generated documentation, and we rely on the
Qt definition. I'm not against using slots internally, but or we
either document them, or we have to be careful introducing terms.o
In example, I would here say that connect() ties a Signal instance to a
callback \a func, that will be executed on the template \a object
argument.
Multiple slots can be connected to the same signal, and each slot will
be executed upon a signal emission, which is triggered by the
Signal::emit() function.
> + */
> +
> +/**
> + * \fn Signal::disconnect()
> + * \brief Disconnect the signal from all slots
> + */
> +
> +/**
> + * \fn Signal::disconnect(T *object)
> + * \brief Disconnect the signal from all slots of the \a object
> + */
> +
> +/**
> + * \fn Signal::disconnect(T *object, void(T::*func)(Args...))
> + * \brief Disconnect the signal from a slot of the \a object
Feel free to expand these if you think it is useful.
> + */
> +
> +/**
> + * \fn Signal::emit()
> + * \brief Emit the signal and call all connected slots
"When emit() is called on a Signal instance, the list of connected
slots is traversed and each one of them is called one after the
other."
Are there more thing worth being mentioned here, such as the calling
order and possible conflicts if the same slot is registered more than
once?
> + */
> +
> +} /* namespace libcamera */
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190107/8b57bbcb/attachment-0001.sig>
More information about the libcamera-devel
mailing list