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

[Laurent Pinchart]

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.

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