[libcamera-devel] [PATCH v2 06/14] libcamera: controls: Add AeEnable

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Aug 30, 2019 at 01:26:45AM +0200, Niklas Söderlund wrote:
> Add a control to turn Auto Exposure on or off.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  include/libcamera/control_ids.h |  1 +
>  src/libcamera/controls.cpp      | 15 +++++++++++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/control_ids.h b/include/libcamera/control_ids.h
> index 75b6a2d5cafeca72..8cd44e571f705ac5 100644
> --- a/include/libcamera/control_ids.h
> +++ b/include/libcamera/control_ids.h
> @@ -13,6 +13,7 @@
>  namespace libcamera {
>  
>  enum ControlId {
> +	AeEnable,
>  	AwbEnable,
>  	Brightness,
>  	Contrast,
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 727fdbd9450d2f40..35861e27401d5241 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -186,6 +186,13 @@ std::string ControlValue::toString() const
>   * \brief Numerical control ID
>   */
>  
> +/**
> + * \var AeEnable
> + * ControlType: Bool
> + *
> + * Enables or disables the AE. See also \a libcamera::ControlId::ManualExposure.

s/Enables or disables/Enable or disable/
s/AE/auto-exposure algorithm/ or s/the AE/auto-expose/

You should also use \sa instead of See also.

 * \sa ControlId::ManualExposure

> + */
> +
>  /**
>   * \var AwbEnable
>   * ControlType: Bool
> @@ -218,14 +225,18 @@ std::string ControlValue::toString() const
>   * \var ManualExposure
>   * ControlType: Integer
>   *
> - * Specify a fixed exposure time in milli-seconds
> + * Specify a fixed exposure time in milli-seconds.
> + *
> + * This control is only considered if AeEnable is not enabled.

What happens if AE is enabled ? Will the fixed exposure time be recorded
internally and applied when AE gets disabled ? Same for the gain below.

>   */
>  
>  /**
>   * \var ManualGain
>   * ControlType: Integer
>   *
> - * Specify a fixed gain parameter
> + * Specify a fixed gain parameter.
> + *
> + * This control is only considered if AeEnable is not enabled.

Should AE be renamed to auto exposure and gain ? Or should we have an
AGCEnable control ?

>   */
>  
>  /**

-- 
Regards,

Laurent Pinchart