[libcamera-devel] [PATCH 3/3] Documentation: Update the "Start an event loop" section

[Laurent Pinchart]

Hello,

On Fri, Jun 17, 2022 at 11:39:45AM +0200, Jacopo Mondi via libcamera-devel wrote:
> On Fri, Jun 17, 2022 at 11:28:19AM +0200, Daniel Semkowicz wrote:
> > The description sounds clear. I would just add minor changes.
> >
> > On Fri, Jun 17, 2022 at 10:01 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > On Fri, Jun 17, 2022 at 09:24:50AM +0200, Daniel Semkowicz wrote:
> > > > On Thu, Jun 16, 2022 at 1:06 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > > >
> > > > > Hi Daniel,
> > > > >
> > > > >    I wonder if we shouldn't just drop the whole
> > > > >
> > > > > Start an event loop
> > > > > ~~~~~~~~~~~~~~~~~~~
> > > > >
> > > > > paragraph
> > > >
> > > > I was also thinking about that when I was editing the current text.
> > > > It is always the decision on how many details about the internal
> > > > architecture We want to present to the developer in this guide.
> > > > As it is the introduction guide then maybe indeed it will be better to
> > > > keep it simple and do not talk too much about the internals.
> > > >
> > > > There will still be this one "sleep_for(3000ms)" line needed and I think
> > > > it would be good to leave some short explanation why it is here. Do you
> > > > think that adding it at the end of "Request queueing" section is a good
> > > > idea or should I create a separate section about it?
> > >
> > > You're right, throwing a sleep_for() without saying anything might be
> > > not the best idea :)
> > >
> > > Let's resume your original proposal and let me try to integrate it
> > > with a full rewrite of the paragraph.
> > >
> > > > > > -Start an event loop
> > > > > > +Event loop
> > > > > >  ~~~~~~~~~~~~~~~~~~~
> > > > > >
> > > > > >  The libcamera library needs an event loop to monitor and dispatch events
> > > > > > @@ -524,17 +526,17 @@ a notifier it is monitoring, it emits the notifier's
> > > > > >  notifiers' signals and emit application visible events, such as the
> > > > > >  ``Camera::bufferReady`` and ``Camera::requestCompleted`` signals.
> > > > > >
> > > > > > -The code below retrieves a reference to the system-wide event dispatcher and for
> > > > > > -the a fixed duration of 3 seconds, processes all the events detected in the
> > > > > > -system.
> > > > > > +Event loop is handled internally by ``CameraManager`` instance in a separate
> > > > > > +thread. ``CameraManager::start()`` creates a new thread and starts the event
> > > > > > +loop processing.
> > >
> > > Event loop
> > > ~~~~~~~~~~~~~~~~~~~

Nitpicking, the underline should be shortened.

> > >
> > > The libcamera library needs an event loop to monitor and dispatch
> > > events generated by the video devices part of the capture pipeline.
> > > libcamera runs its own event loop in a separate thread, created at
> > > ``CameraManager::start()_`` time. This means applications will run in
> > > their own thread and need to manage their own execution opportunely,
> > > to respond to user generate events and to libcamera generated events
> > > emitted through signals.
> >
> > typo: "user generate" -> "user generated"

Or even "user-generated" I think.

I'd write this a bit differently to avoid mentioning the internal event
loop as such. How about the following ?

libcamera creates an internal execution thread at '`CameraManager::start()_``
time to decouple its own event processing from the application's main thread.
Applications are thus free to manage their own execution opportunely, and only
need to respond to events generated by libcamera emitted through signals.

Real-world applications will likely either integrate with the event loop of the
framework they use, or create their own event loop to respond to user events.
For the simple application presented in this example, it is enough to prevent
immediate termination by pausing for 3 seconds. During that time, the libcamera
thread will generate request completion events that the application will handle
in the ``requestComplete()`` slot connected to the ``Camera::requestCompleted``
signal.

> > > Real-world applications will likely either integrate in the event loop
> > > of higher level frameworks or create their own one to respond to user
> > > generated events, while for the simple application presented in this
> > > example it is enough to prevent immediate termination by pausing for 3
> > > seconds, during which the asynchronous request completion events
> > > generated by libcamera will be handled by the ``requestComplete()``
> > > slot connected to the ``Camera::requestCompleted`` signal.
> >
> > I would break down this part into less complex sentences to improve
> > readability. Something like:
> >
> > Real-world applications will likely either integrate in the event loop
> > of higher level frameworks or create their own one to respond to user
> > generated events. For the simple application presented in this example,
> > it is enough to prevent immediate termination by pausing for 3 seconds.
> > During this time, the asynchronous request completion events generated
> > by libcamera will be handled by the ``requestComplete()`` slot connected
> > to the ``Camera::requestCompleted`` signal.
> >
> > Is it ok?
> 
> Thanks, much better!
> 
> > > .. code:: cpp
> > >
> > >    std::this_thread::sleep_for(3000ms);
> > >
> > > What do you think ?
> > >
> > > > > > +
> > > > > > +As the ``CameraManager`` was already started in our example, we need to prevent
> > > > > > +the immediate termination of the application. The code below pauses the main
> > > > > > +thread for 3 seconds, so that the event loop thread can process the requests.
> > > > > >
> > > > > >  .. code:: cpp
> > > > > >
> > > > > > -   EventDispatcher *dispatcher = cm->eventDispatcher();
> > > > > > -   Timer timer;
> > > > > > -   timer.start(3000);
> > > > > > -   while (timer.isRunning())
> > > > > > -       dispatcher->processEvents();
> > > > > > +   std::this_thread::sleep_for(3000ms);
> > > > > >
> > > > > >  Clean up and stop the application
> > > > > >  ---------------------------------

-- 
Regards,

Laurent Pinchart