[libcamera-devel] libcamera: camera_data: Document requestSequence_

11858 diff mbox series

The CameraData::requestSequence_ field was not documented. Fix it.

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.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

---
Isn't it a bit fishy that the counter is never reset ?

---
 include/libcamera/internal/pipeline_handler.h | 1 -
 src/libcamera/pipeline_handler.cpp            | 8 ++++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

--
2.31.1

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@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.

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

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@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

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@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