[libcamera-devel] [PATCH 4/9] fixup: Expand AeState documentation

[paul.elder at ideasonboard.com]

Hi Jacopo,

On Fri, Jul 01, 2022 at 05:46:56PM +0200, Jacopo Mondi wrote:
> The AeState control is tricky, it describes the state of the whole AEGC
> block which is independently controlled by ExposureTimeMode and
> AnalogueGainMode.
> 
> Try to expand the documentation.
> 
> Use "AEGC" when referring to the algorithm.
> 
> Maybe the control should be named AEGCState ?

It's uuuugly.

But also maybe more correct. idk I'm neutral maybe people that have a
bigger preference can do the bikeshedding :)

> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/control_ids.yaml | 64 ++++++++++++++++++++++++----------
>  1 file changed, 46 insertions(+), 18 deletions(-)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index bb5eeb1507a9..d50df8bcad28 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -10,43 +10,71 @@ controls:
>    - AeState:
>        type: int32_t
>        description: |
> -        Control to report the AE algorithm state associated with the capture
> -        result.
> +        Control to report the AEGC algorithm state.
>  
> -        The state is still reported even if ExposureTimeMode or
> -        AnalogueGainMode is set to Manual.
> +        The AEGC algorithm computes the exposure time and the analogue gain
> +        values to be applied to the image sensor.
> +
> +        The AEGC algorithm behaviour is controlled by the ExposureTimeMode and
> +        AnalogueGainMode controls, which allow applications to decide how
> +        the exposure time and gain are computed, in Auto or Manual mode,
> +        independently one from the other.

s/one from the other/from one another/

> +
> +        The AeState control reports the AEGC algorithm state through a single
> +        value and describes it a single computation block which computes

s/it //

> +        both the exposure time and the analogue gain values.
> +
> +        When both the exposure time and analogue gain values are configured to
> +        be in Manual mode, the AEGC algorithm is quiescent and does not actively
> +        compute any value and the AeState control will report AeStateIdle.
> +
> +        When at least the exposure time or analogue gain are configured to be
> +        computed by the AEGC algorithm, the AeState control will report if the
> +        algorithm has converged to stable values for any of the controls set

I would've s/any/all/, but now I'm wondering if it's even possible for
AEGC with both E and G set to auto to have only one converged but not
the other.

Although maybe s/any/all/ is proper, because this is *one* state to
describe *all* of AEGC, so I think if at least one is still converging
it should be converging, and only if both are converged then it should
be converged. Even if it's not really possible, that's probably the more
correct description.

> +        to be computed in Auto mode.
>  
> -        \sa AnalogueGain
>          \sa AnalogueGainMode
> -        \sa ExposureTime
>          \sa ExposureTimeMode
>  
>        enum:
>          - name: AeStateIdle
>            value: 0
>            description: |
> -            The AE algorithm is inactive.
> +            The AEGC algorithm is inactive.
> +
> +            This state is returned when both AnalogueGainMode and
> +            ExposureTimeMode are set to Manual and the algorithm is not
> +            actively computing any value.

Do we need to explicitly state an exception for if only one is
manual-able?  Although due to the constraints that we've defined, if
only one is manual-able then both are auto-able. So maybe it's implicit?

>  
> -            This state should be returned if both AnalogueGainMode and
> -            ExposureTimeMode are set to manual (or one, if the camera only
> -            supports one of the two controls).
> +            When ExposureTimeMode or AnalogueGainMode are set to Auto mode, the
> +            AEGC algorithm might spontaneously initiate a new scan, in which
> +            case the AeState control is moved to AeStateSearching.
>          - name: AeStateSearching
>            value: 1
>            description: |
> -            The AE algorithm has not converged yet.
> +            The AEGC algorithm is actively computing new values, for either the
> +            exposure time or the analogue gain, but has not converged to a
> +            stable result yet.
> +
> +            This state is returned if at least one of AnalogueGainMode
> +            or ExposureTimeMode is set to auto and the algorithm hasn't
> +            converged yet.
>  
> -            This state should be returned if at least one of AnalogueGainMode
> -            or ExposureTimeMode is set to auto, and the AE algorithm hasn't
> -            converged yet. If the AE algorithm converges, the state shall go to
> -            AeStateConverged.
> +            The AEGC algorithm converges once stable values are computed for
> +            any of the controls set to be computed in Auto mode.
> +
> +            Once the algorithm converges the state is moved to AeStateConverged.
>          - name: AeStateConverged
>            value: 2
>            description: |
>              The AE algorithm has converged.

s/AE/AEGC/?


Otherwise looks good.


Thanks,

Paul


>  
> -            This state should be returned if at least one of AnalogueGainMode
> -            or ExposureTimeMode is set to auto, and the AE algorithm has
> -            converged.
> +            This state is returned if at least one of AnalogueGainMode
> +            or ExposureTimeMode is set to Auto, and the AEGC algorithm has
> +            converged to stable value.
> +
> +            The AEGC algorithm might spontaneously re-initiate an AE scan, in
> +            which case the state is moved to AeStateSearching.
>  
>    # AeMeteringMode needs further attention:
>    # - Auto-generate max enum value.
> -- 
> 2.36.1
>