[libcamera-devel,v4,4/5] libcamera: controls: Improve documentation for ExposureTime and AnalogueGain

10539 diff mbox series

Setting these controls "fixes" them and the AE may not change them;
setting them back to zero returns them to the control of the AE
algorithm.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/libcamera/control_ids.yaml | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Hi David,

Thank you for the patch.

On Tue, Dec 01, 2020 at 05:55:35PM +0000, David Plowman wrote:
> Setting these controls "fixes" them and the AE may not change them;
> setting them back to zero returns them to the control of the AE
> algorithm.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/libcamera/control_ids.yaml | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index a883e27e..fba1f545 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -125,8 +125,15 @@ controls:
>          Exposure time (shutter speed) for the frame applied in the sensor
>          device. This value is specified in micro-seconds.
>  
> +        Setting this value means that it is now fixed and the AE algorithm may
> +        not change it. Setting it back to zero returns it to the control of the
> +        AE algorithm.
> +
>          \sa AnalogueGain AeEnable
>  
> +        \todo Consider how setting the exposure time interacts with other AE
> +              features, such as aperture, aperture/shutter priority modes etc.

No need for the additional indentation. I'd also expand this a bit to
mention AeEnable. Maybe something along the lines of the following ?

        \todo Document the interactions between AeEnable and setting a fixed
        value for this control. Consider interactions with other AE features,
        such as aperture and aperture/shutter priority mode, and decide if
        control of which features should be automatically adjusted shouldn't
        better be handled through a separate AE mode control.

If you're fine with this, I'll use this text when applying, there's no
need to resend this patch.

> +
>    - AnalogueGain:
>        type: float
>        description: |
> @@ -134,8 +141,15 @@ controls:
>          The value of the control specifies the gain multiplier applied to all
>          colour channels. This value cannot be lower than 1.0.
>  
> +        Setting this value means that it is now fixed and the AE algorithm may
> +        not change it. Setting it back to zero returns it to the control of the
> +        AE algorithm.
> +
>          \sa ExposureTime AeEnable
>  
> +        \todo Consider how setting the analogue gain interacts with other AE
> +              features, such as aperture, aperture/shutter priority modes etc.
> +
>    - Brightness:
>        type: float
>        description: |

Hi Laurent

Thanks very much for the review. All sounds great to me!

Best regards and thank you

David

On Tue, 1 Dec 2020 at 18:41, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Tue, Dec 01, 2020 at 05:55:35PM +0000, David Plowman wrote:
> > Setting these controls "fixes" them and the AE may not change them;
> > setting them back to zero returns them to the control of the AE
> > algorithm.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/libcamera/control_ids.yaml | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index a883e27e..fba1f545 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -125,8 +125,15 @@ controls:
> >          Exposure time (shutter speed) for the frame applied in the sensor
> >          device. This value is specified in micro-seconds.
> >
> > +        Setting this value means that it is now fixed and the AE algorithm may
> > +        not change it. Setting it back to zero returns it to the control of the
> > +        AE algorithm.
> > +
> >          \sa AnalogueGain AeEnable
> >
> > +        \todo Consider how setting the exposure time interacts with other AE
> > +              features, such as aperture, aperture/shutter priority modes etc.
>
> No need for the additional indentation. I'd also expand this a bit to
> mention AeEnable. Maybe something along the lines of the following ?
>
>         \todo Document the interactions between AeEnable and setting a fixed
>         value for this control. Consider interactions with other AE features,
>         such as aperture and aperture/shutter priority mode, and decide if
>         control of which features should be automatically adjusted shouldn't
>         better be handled through a separate AE mode control.
>
> If you're fine with this, I'll use this text when applying, there's no
> need to resend this patch.
>
> > +
> >    - AnalogueGain:
> >        type: float
> >        description: |
> > @@ -134,8 +141,15 @@ controls:
> >          The value of the control specifies the gain multiplier applied to all
> >          colour channels. This value cannot be lower than 1.0.
> >
> > +        Setting this value means that it is now fixed and the AE algorithm may
> > +        not change it. Setting it back to zero returns it to the control of the
> > +        AE algorithm.
> > +
> >          \sa ExposureTime AeEnable
> >
> > +        \todo Consider how setting the analogue gain interacts with other AE
> > +              features, such as aperture, aperture/shutter priority modes etc.
> > +
> >    - Brightness:
> >        type: float
> >        description: |
>
> --
> Regards,
>
> Laurent Pinchart