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

[Laurent Pinchart]

Hi Jacopo,

On Wed, Apr 07, 2021 at 04:42:30PM +0200, Jacopo Mondi wrote:
> On Wed, Apr 07, 2021 at 05:32:25PM +0300, Laurent Pinchart wrote:
> > 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.

Hopefully he will know the purpose of the field he introduced :-)
Kieran, how about picking this patch up, and posting a v2 with the
documentation expanded to explain what the purpose of the field is ?

> > > + */
> > > +
> > >  /**
> > >   * \class PipelineHandler
> > >   * \brief Create and manage cameras based on a set of media devices

-- 
Regards,

Laurent Pinchart