[libcamera-devel] [PATCH] libcamera: camera_data: Document requestSequence_

[Jacopo Mondi]

Hi Laurent

On Wed, Apr 07, 2021 at 05:32:25PM +0300, Laurent Pinchart wrote:

> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Apr 07, 2021 at 04:10:41PM +0200, Jacopo Mondi wrote:
> > The CameraData::requestSequence_ field was not documented. Fix it.
>
> The only thing worse than pushing to production on a Friday is pushing
> to production just before a week of holiday :-) Kieran said he would fix
> this (and the other doxygen warnings introduced at the same time) when
> coming back.
>
> > Also remove an empty line the class declaration as the field is not
> > special compared to the other existing ones.
> >
> > Fixes the doxygen warning
> > /include/libcamera/internal/pipeline_handler.h:51: warning: Member
> > requestSequence_ (variable) of class libcamera::CameraData is not
> > documented.
> >
>
> A Fixes: line could be nice.
>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > ---
> > Isn't it a bit fishy that the counter is never reset ?
>
> It's on purpose, to uniquely identify requests across stop()/start()
> sequences.
>
> > ---
> >  include/libcamera/internal/pipeline_handler.h | 1 -
> >  src/libcamera/pipeline_handler.cpp            | 8 ++++++++
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index c6454db6b2c4..82fdc1fee9b9 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -47,7 +47,6 @@ public:
> >  	std::list<Request *> queuedRequests_;
> >  	ControlInfoMap controlInfo_;
> >  	ControlList properties_;
> > -
> >  	uint32_t requestSequence_;
> >
> >  private:
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 433c05f60db0..7f4fa1b4ba89 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -97,6 +97,14 @@ LOG_DEFINE_CATEGORY(Pipeline)
> >   * when creating the camera, and shall not be modified afterwards.
> >   */
> >
> > +/**
> > + * \var CameraData::requestSequence_
> > + * \brief The request sequence counter
> > + *
> > + * The request sequence counter is increased when a new Request is queued to
> > + * the camera and monotonically increases during the whole library lifetime.
>
> It's actually the lifetime of the camera.
>
> It would be good to also document the purpose of this field.
>

I'm not 100% sure what does it serve to. Happy to have Kieran continue
on this if he's meant to fix the other warnings he introduced.

Thanks
  j

> > + */
> > +
> >  /**
> >   * \class PipelineHandler
> >   * \brief Create and manage cameras based on a set of media devices
>
> --
> Regards,
>
> Laurent Pinchart