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

[Jacopo Mondi]

Hi Daniel,

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:
> >
> > Hi Daniel,
> >
> > On Fri, Jun 17, 2022 at 09:24:50AM +0200, Daniel Semkowicz wrote:
> > > Hi Jacopo,
> > >
> > > 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
> > ~~~~~~~~~~~~~~~~~~~
> >
> > 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"
>
> >
> > 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!

Thanks
   j

>
> Best regards
> Daniel
>
> >
> > .. 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
> > > > >  ---------------------------------
> > > > > --
> > > > > 2.34.1
> > > > >