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

[Laurent Pinchart]

Hi Jacopo,

On Wed, Sep 04, 2019 at 02:29:23PM +0200, Jacopo Mondi wrote:
> On Tue, Sep 03, 2019 at 11:17:46PM +0300, Laurent Pinchart wrote:
> > 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 ?

It's not about documentation, there's an automatically generated table
of data in control_types.cpp.

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