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

[David Plowman]

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