[libcamera-devel,v2,1/6] libcamera: controls: Specify manual gain units and change exposure units

3059 diff mbox series

Use micro-seconds for ManualExposure. This is changed from milli-
seconds. The latter would not allow very low exposure times.

ManualGain switch to use a float to allow fractional gain adjustments.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/libcamera/control_ids.yaml | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

On 09/03/2020 12:33, Naushir Patuck wrote:
> Use micro-seconds for ManualExposure. This is changed from milli-
> seconds. The latter would not allow very low exposure times.
> 
> ManualGain switch to use a float to allow fractional gain adjustments.

"Switch ManualGain to use a float" ...?

> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/libcamera/control_ids.yaml | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 4befec74..5bbe65ae 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -44,10 +44,17 @@ controls:
>  
>    - ManualExposure:
>        type: int32_t

Not directly against this patch, but I can't imagine having a negative
exposure time - so should this be uint32_t?

> -      description: Specify a fixed exposure time in milli-seconds
> +      description: |
> +        Specify a fixed exposure time in micro-seconds to be applied in the
> +        sensor device.
> +
> +        \sa ManualGain
>  
>    - ManualGain:
> -      type: int32_t
> -      description: Specify a fixed gain parameter
> +      type: float
> +      description: |
> +        Specify a fixed gain value to be applied in the pipeline.  This gain is
> +        applied to all colour channels.
>  
> +        \sa ManualExposure
>  ...
> 

Otherwise, this sounds good to me.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Hi Kieran,

On Fri, Mar 20, 2020 at 01:38:02PM +0000, Kieran Bingham wrote:
> On 09/03/2020 12:33, Naushir Patuck wrote:
> > Use micro-seconds for ManualExposure. This is changed from milli-
> > seconds. The latter would not allow very low exposure times.
> > 
> > ManualGain switch to use a float to allow fractional gain adjustments.
> 
> "Switch ManualGain to use a float" ...?
> 
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/libcamera/control_ids.yaml | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index 4befec74..5bbe65ae 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -44,10 +44,17 @@ controls:
> >  
> >    - ManualExposure:
> >        type: int32_t
> 
> Not directly against this patch, but I can't imagine having a negative
> exposure time - so should this be uint32_t?

We don't have unsigned int controls :-)

> > -      description: Specify a fixed exposure time in milli-seconds
> > +      description: |
> > +        Specify a fixed exposure time in micro-seconds to be applied in the
> > +        sensor device.
> > +
> > +        \sa ManualGain
> >  
> >    - ManualGain:
> > -      type: int32_t
> > -      description: Specify a fixed gain parameter
> > +      type: float
> > +      description: |
> > +        Specify a fixed gain value to be applied in the pipeline.  This gain is
> > +        applied to all colour channels.
> >  
> > +        \sa ManualExposure
> >  ...
> > 
> 
> Otherwise, this sounds good to me.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

On 20/03/2020 14:05, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Fri, Mar 20, 2020 at 01:38:02PM +0000, Kieran Bingham wrote:
>> On 09/03/2020 12:33, Naushir Patuck wrote:
>>> Use micro-seconds for ManualExposure. This is changed from milli-
>>> seconds. The latter would not allow very low exposure times.
>>>
>>> ManualGain switch to use a float to allow fractional gain adjustments.
>>
>> "Switch ManualGain to use a float" ...?
>>
>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>>> ---
>>>  src/libcamera/control_ids.yaml | 13 ++++++++++---
>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
>>> index 4befec74..5bbe65ae 100644
>>> --- a/src/libcamera/control_ids.yaml
>>> +++ b/src/libcamera/control_ids.yaml
>>> @@ -44,10 +44,17 @@ controls:
>>>  
>>>    - ManualExposure:
>>>        type: int32_t
>>
>> Not directly against this patch, but I can't imagine having a negative
>> exposure time - so should this be uint32_t?
> 
> We don't have unsigned int controls :-)

Ah well then I guess we can't use a uint32_t ...

Is that just a restriction due to ControlValue templates?
Presumably we 'could' have them - but maybe it's not needed.

> 
>>> -      description: Specify a fixed exposure time in milli-seconds
>>> +      description: |
>>> +        Specify a fixed exposure time in micro-seconds to be applied in the
>>> +        sensor device.
>>> +
>>> +        \sa ManualGain
>>>  
>>>    - ManualGain:
>>> -      type: int32_t
>>> -      description: Specify a fixed gain parameter
>>> +      type: float
>>> +      description: |
>>> +        Specify a fixed gain value to be applied in the pipeline.  This gain is
>>> +        applied to all colour channels.
>>>  
>>> +        \sa ManualExposure
>>>  ...
>>>
>>
>> Otherwise, this sounds good to me.
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>

Hi Kieran,

On Fri, Mar 20, 2020 at 02:35:36PM +0000, Kieran Bingham wrote:
> On 20/03/2020 14:05, Laurent Pinchart wrote:
> > On Fri, Mar 20, 2020 at 01:38:02PM +0000, Kieran Bingham wrote:
> >> On 09/03/2020 12:33, Naushir Patuck wrote:
> >>> Use micro-seconds for ManualExposure. This is changed from milli-
> >>> seconds. The latter would not allow very low exposure times.
> >>>
> >>> ManualGain switch to use a float to allow fractional gain adjustments.
> >>
> >> "Switch ManualGain to use a float" ...?
> >>
> >>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> >>> ---
> >>>  src/libcamera/control_ids.yaml | 13 ++++++++++---
> >>>  1 file changed, 10 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> >>> index 4befec74..5bbe65ae 100644
> >>> --- a/src/libcamera/control_ids.yaml
> >>> +++ b/src/libcamera/control_ids.yaml
> >>> @@ -44,10 +44,17 @@ controls:
> >>>  
> >>>    - ManualExposure:
> >>>        type: int32_t
> >>
> >> Not directly against this patch, but I can't imagine having a negative
> >> exposure time - so should this be uint32_t?
> > 
> > We don't have unsigned int controls :-)
> 
> Ah well then I guess we can't use a uint32_t ...
> 
> Is that just a restriction due to ControlValue templates?
> Presumably we 'could' have them - but maybe it's not needed.

We could, but I think it would complexify the API, especially for V4L2
controls, and you'll have to be very, very careful to add a U suffix to
your integer literal when calling some of the functions, otherwise
you'll get undefined behaviour without a compiler warning. 32-bit vs.
64-bit is already painful enough :-)

> >>> -      description: Specify a fixed exposure time in milli-seconds
> >>> +      description: |
> >>> +        Specify a fixed exposure time in micro-seconds to be applied in the
> >>> +        sensor device.
> >>> +
> >>> +        \sa ManualGain
> >>>  
> >>>    - ManualGain:
> >>> -      type: int32_t
> >>> -      description: Specify a fixed gain parameter
> >>> +      type: float
> >>> +      description: |
> >>> +        Specify a fixed gain value to be applied in the pipeline.  This gain is
> >>> +        applied to all colour channels.
> >>>  
> >>> +        \sa ManualExposure
> >>>  ...
> >>>
> >>
> >> Otherwise, this sounds good to me.
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>