[libcamera-devel] [PATCH v2 2/7] libcamera: controls: Document control_ids.h

[Jacopo Mondi]

Hello Niklas, Laurent,

On Tue, Sep 03, 2019 at 11:17:46PM +0300, Laurent Pinchart wrote:
> Hello,
>
> On Wed, Aug 28, 2019 at 05:18:59PM +0200, Jacopo Mondi wrote:
> > On Wed, Aug 28, 2019 at 11:58:16AM +0200, Niklas Söderlund wrote:
> > > On 2019-08-27 11:50:02 +0200, Jacopo Mondi wrote:
> > >> The control identifiers documentation was not generated as they're not
> > >> part of the controls.h file documented in controls.cpp.
> > >>
> > >> Move documentation for control id to the end of the controls.cpp and
> > >> document the control_ids.h file to have documentation for controls
> > >> properly generated.
> > >
> > > I'm a bit debated about this solution. I see why the change is needed
> > > but I wonder if it's not nicer to create a control_ids.cpp file to hold
> > > the documentation?
> >
> > I was just reading ipa_module.c and that's what I've found there
> >
> > /**
> >  * \file ipa_module.h
> >  * \brief Image Processing Algorithm module
> >  */
> >
> > /**
> >  * \file ipa_module_info.h
> >  * \brief Image Processing Algorithm module information
> >  */
> >
> > It's not a new thing then
>
> It's not new, but we also have precedents for empty .cpp files just for
> documentation purpose (ipa_module.cpp is one of them), so I'm not
> opposed to creating control_ids.cpp. As I still plan to rework the
> controls implementation, decoupling it from the control ids
> documentation may make sense and make things simpler.
>

When I first changed this, I have missed the following machinery in
src/libcamera/meson.build

gen_controls = files('gen-controls.awk')
control_types_cpp = custom_target('control_types_cpp',
                                  input : files('controls.cpp'),
                                  output : 'control_types.cpp',
                                  depend_files : gen_controls,
                                  command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@'])

That creates a control_types.cpp file parsing controls.cpp to extract
the documentation for control. Why this complication instead of simply
documenting controls in their own .cpp file or create a
/** \file control_ids.h */ section in controls.cpp like this patch
does ?

Thanks
   j

> > > As this is as far as I know the first time we document two header files
> > > in one cpp file I want to know what others think about this practice.
> > > I'm not strongly against it, but found myself reading the diff 3 times
> > > before I understood it ;-)
> > >
> > > If the consensus is that we should document multiple header files in one
> > > cpp file feel free to add (with the typo bellow fixed),
> > >
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > >
> > >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > >> ---
> > >>  src/libcamera/controls.cpp | 100 ++++++++++++++++++++-----------------
> > >>  1 file changed, 53 insertions(+), 47 deletions(-)
> > >>
> > >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > >> index 727fdbd9450d..9adc3badc254 100644
> > >> --- a/src/libcamera/controls.cpp
> > >> +++ b/src/libcamera/controls.cpp
> > >> @@ -181,53 +181,6 @@ std::string ControlValue::toString() const
> > >>  	return "<ValueType Error>";
> > >>  }
> > >>
> > >> -/**
> > >> - * \enum ControlId
> > >> - * \brief Numerical control ID
> > >> - */
> > >> -
> > >> -/**
> > >> - * \var AwbEnable
> > >> - * ControlType: Bool
> > >> - *
> > >> - * Enables or disables the AWB. See also \a libcamera::ControlId::ManualGain
> > >> - */
> > >> -
> > >> -/**
> > >> - * \var Brightness
> > >> - * ControlType: Integer
> > >> - *
> > >> - * Specify a fixed brightness parameter.
> > >> - */
> > >> -
> > >> -/**
> > >> - * \var Contrast
> > >> - * ControlType: Integer
> > >> - *
> > >> - * Specify a fixed contrast parameter.
> > >> - */
> > >> -
> > >> -/**
> > >> - * \var Saturation
> > >> - * ControlType: Integer
> > >> - *
> > >> - * Specify a fixed saturation parameter.
> > >> - */
> > >> -
> > >> -/**
> > >> - * \var ManualExposure
> > >> - * ControlType: Integer
> > >> - *
> > >> - * Specify a fixed exposure time in milli-seconds
> > >> - */
> > >> -
> > >> -/**
> > >> - * \var ManualGain
> > >> - * ControlType: Integer
> > >> - *
> > >> - * Specify a fixed gain parameter
> > >> - */
> > >> -
> > >>  /**
> > >>   * \struct ControlIdentifier
> > >>   * \brief Describe a ControlId with control specific constant meta-data
> > >> @@ -549,4 +502,57 @@ void ControlList::update(const ControlList &other)
> > >>  	}
> > >>  }
> > >>
> > >> +/**
> > >> + * \file control_ids.h
> > >> + * \brief Definition of numerical identifiers for libcamera control and
> > >> + * properties
> > >
> > > s/control/controls/
> > >
> > >> + */
> > >> +
> > >> +/**
> > >> + * \enum ControlId
> > >> + * \brief Numerical control ID
> > >> + */
> > >> +
> > >> +/**
> > >> + * \var AwbEnable
> > >> + * ControlType: Bool
> > >> + *
> > >> + * Enables or disables the AWB. See also \a libcamera::ControlId::ManualGain
> > >> + */
> > >> +
> > >> +/**
> > >> + * \var Brightness
> > >> + * ControlType: Integer
> > >> + *
> > >> + * Specify a fixed brightness parameter.
> > >> + */
> > >> +
> > >> +/**
> > >> + * \var Contrast
> > >> + * ControlType: Integer
> > >> + *
> > >> + * Specify a fixed contrast parameter.
> > >> + */
> > >> +
> > >> +/**
> > >> + * \var Saturation
> > >> + * ControlType: Integer
> > >> + *
> > >> + * Specify a fixed saturation parameter.
> > >> + */
> > >> +
> > >> +/**
> > >> + * \var ManualExposure
> > >> + * ControlType: Integer
> > >> + *
> > >> + * Specify a fixed exposure time in milli-seconds
> > >> + */
> > >> +
> > >> +/**
> > >> + * \var ManualGain
> > >> + * ControlType: Integer
> > >> + *
> > >> + * Specify a fixed gain parameter
> > >> + */
> > >> +
> > >>  } /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190904/dc47040a/attachment.sig>