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

[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).

-- 
Regards,

Laurent Pinchart