[libcamera-devel] [PATCH 05/11] libcamera: Add signal/slot communication mechanism
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jan 7 20:44:42 CET 2019
Hi Jacopo,
On Monday, 7 January 2019 20:28:54 EET Jacopo Mondi wrote:
> On Mon, Jan 07, 2019 at 07:36:43PM +0200, Laurent Pinchart wrote:
> > On Monday, 7 January 2019 18:15:58 EET Jacopo Mondi wrote:
> >> 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
> >
> > [snip]
> >
> >>> 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);
> >
> > This may compile, but isn't right. *iter may not be of type Slot<T,
> > Args...> *, it may point to a Slot with a different object type T than
> > the one this function has been called with. While I may still work,
> > casting a pointer to a potentially incorrect type and then dereferencing
> > it would worry me.
>
> Right, the 'T's might be different :)
> It's probably not a big deal here, as you only access obj_ which is
> provided by the base class, but it's defintely an issue when accessing
> func_
>
> Anyway, doesn't this line suffer from the same problem?
> reinterpret_cast<Slot<T, Args...> *>(slot)->func_ == func)
> That T might be a different one from the one this function has been
> called with. Actually the check for (slot->obj_ == object) might
> protect us from actually accessing anything wrong, so the issue is
> only potential...
That was the rationale behind the implementation, if obj_ matches, then we
know it's a Slot<T, Args...> with the same T.
> >> 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?
> >
> > I don't expect the same signal to be connected to the same slot multiple
> > times, but if it is, I think this function should delete all connections
>
> No break then, this is fine.
>
> >> }
> >>
> >> }
> >>
> >>> + }
> >>> +
> >>> + 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__ */
> >
> > [snip]
> >
> >> > 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.
> >
> > I agree more documentation would be nice, I'll try to expand a bit, but I
> > don't have time now to write dozens of pages about this concept :-)
> >
> >>> + */
> >>> +
> >>> +/**
> >>> + * \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.
> >
> > I'll explain the term slot in the class documentation.
>
> Thanks
>
> >>> + */
> >>> +
> >>> +/**
> >>> + * \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?
> >
> > Should we document the order, or leave it as implementation-defined ? They
> > are currently called in the order they are connected, do you think it
> > could cause a problem later t guarantee that ? Or would it make the API
> > less usable if we don't guarantee the order ?
>
> I think it's worth mentioning the calling order is the registration
> one. If a class derives the Signal one to implement, say, priorities
> when calling slots, this shall be documented even more, otherwise the
> here provided base Signal class provides that already.
Sounds good to me. I'll do that.
> >>> + */
> >>> +
> >>> +} /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list