[libcamera-devel,v13] libcamera: controls: Generate and use fixed-sized Span types
diff mbox series

Message ID 20220802210324.115893-1-Rauch.Christian@gmx.de
State Accepted
Headers show
Series
  • [libcamera-devel,v13] libcamera: controls: Generate and use fixed-sized Span types
Related show

Commit Message

Christian Rauch Aug. 2, 2022, 9:03 p.m. UTC 
Define Span types explicitly as either variable- or fixed-sized. This
introduces a new convention for defining Span dimensions in the property
and control value definitions and generates Span types as variable-sized
Span<T> or as fixed-sized Span<T,N>.

Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/controls.h        |  2 +-
 src/ipa/raspberrypi/raspberrypi.cpp | 19 +++++++++--------
 src/libcamera/control_ids.yaml      |  4 ++--
 src/libcamera/property_ids.yaml     |  4 ++--
 src/qcam/dng_writer.cpp             |  2 +-
 utils/gen-controls.py               | 32 ++++++++++++++++++++---------
 6 files changed, 38 insertions(+), 25 deletions(-)

--
2.34.1

Comments

Jacopo Mondi Aug. 3, 2022, 9:44 a.m. UTC | #1 
Hi Christian,

On Tue, Aug 02, 2022 at 11:03:24PM +0200, Christian Rauch wrote:
> Define Span types explicitly as either variable- or fixed-sized. This
> introduces a new convention for defining Span dimensions in the property
> and control value definitions and generates Span types as variable-sized
> Span<T> or as fixed-sized Span<T,N>.
>
> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

The issue with gcc8 seems now solved.

One more tag and I hope we can collect the patch ?

Thanks
  j

> ---
>  include/libcamera/controls.h        |  2 +-
>  src/ipa/raspberrypi/raspberrypi.cpp | 19 +++++++++--------
>  src/libcamera/control_ids.yaml      |  4 ++--
>  src/libcamera/property_ids.yaml     |  4 ++--
>  src/qcam/dng_writer.cpp             |  2 +-
>  utils/gen-controls.py               | 32 ++++++++++++++++++++---------
>  6 files changed, 38 insertions(+), 25 deletions(-)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index b1b52acb..ebc168fc 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -168,7 +168,7 @@ public:
>
>  		using V = typename T::value_type;
>  		const V *value = reinterpret_cast<const V *>(data().data());
> -		return { value, numElements_ };
> +		return T{ value, numElements_ };
>  	}
>
>  #ifndef __DOXYGEN__
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 6befdd71..69c73f8c 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -567,18 +567,19 @@ void IPARPi::reportMetadata()
>
>  	AwbStatus *awbStatus = rpiMetadata_.getLocked<AwbStatus>("awb.status");
>  	if (awbStatus) {
> -		libcameraMetadata_.set(controls::ColourGains, { static_cast<float>(awbStatus->gainR),
> -								static_cast<float>(awbStatus->gainB) });
> +		libcameraMetadata_.set(controls::ColourGains,
> +				       Span<const float, 2>({ static_cast<float>(awbStatus->gainR),
> +							      static_cast<float>(awbStatus->gainB) }));
>  		libcameraMetadata_.set(controls::ColourTemperature, awbStatus->temperatureK);
>  	}
>
>  	BlackLevelStatus *blackLevelStatus = rpiMetadata_.getLocked<BlackLevelStatus>("black_level.status");
>  	if (blackLevelStatus)
>  		libcameraMetadata_.set(controls::SensorBlackLevels,
> -				       { static_cast<int32_t>(blackLevelStatus->blackLevelR),
> -					 static_cast<int32_t>(blackLevelStatus->blackLevelG),
> -					 static_cast<int32_t>(blackLevelStatus->blackLevelG),
> -					 static_cast<int32_t>(blackLevelStatus->blackLevelB) });
> +				       Span<const int32_t, 4>({ static_cast<int32_t>(blackLevelStatus->blackLevelR),
> +								static_cast<int32_t>(blackLevelStatus->blackLevelG),
> +								static_cast<int32_t>(blackLevelStatus->blackLevelG),
> +								static_cast<int32_t>(blackLevelStatus->blackLevelB) }));
>
>  	FocusStatus *focusStatus = rpiMetadata_.getLocked<FocusStatus>("focus.status");
>  	if (focusStatus && focusStatus->num == 12) {
> @@ -883,7 +884,7 @@ void IPARPi::queueRequest(const ControlList &controls)
>  			if (gains[0] != 0.0f && gains[1] != 0.0f)
>  				/* A gain of 0.0f will switch back to auto mode. */
>  				libcameraMetadata_.set(controls::ColourGains,
> -						       { gains[0], gains[1] });
> +						       Span<const float, 2>({ gains[0], gains[1] }));
>  			break;
>  		}
>
> @@ -1167,8 +1168,8 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur
>
>  	/* Return the validated limits via metadata. */
>  	libcameraMetadata_.set(controls::FrameDurationLimits,
> -			       { static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
> -				 static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) });
> +			       Span<const int64_t, 2>({ static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
> +							static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) }));
>
>  	/*
>  	 * Calculate the maximum exposure time possible for the AGC to use.
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index ecab3ae9..5fa168c6 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -291,7 +291,7 @@ controls:
>          transformation. The 3x3 matrix is stored in conventional reading
>          order in an array of 9 floating point values.
>
> -      size: [3x3]
> +      size: [3,3]
>
>    - ScalerCrop:
>        type: Rectangle
> @@ -525,7 +525,7 @@ controls:
>          the window where the focal distance for the objects shown in that part
>          of the image are closest to the camera.
>
> -      size: [n]
> +      size: []
>
>    - AfTrigger:
>        type: int32_t
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> index 11b7ebdc..a87485d7 100644
> --- a/src/libcamera/property_ids.yaml
> +++ b/src/libcamera/property_ids.yaml
> @@ -497,7 +497,7 @@ controls:
>
>    - PixelArrayOpticalBlackRectangles:
>        type: Rectangle
> -      size: [n]
> +      size: []
>        description: |
>          The pixel array region(s) which contain optical black pixels
>          considered valid for calibration purposes.
> @@ -592,7 +592,7 @@ controls:
>
>    - PixelArrayActiveAreas:
>        type: Rectangle
> -      size: [n]
> +      size: []
>        description: |
>          The PixelArrayActiveAreas property defines the (possibly multiple and
>          overlapping) portions of the camera sensor readable pixel matrix
> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
> index 5b94b3f2..b4362537 100644
> --- a/src/qcam/dng_writer.cpp
> +++ b/src/qcam/dng_writer.cpp
> @@ -517,7 +517,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>
>  	const auto &blackLevels = metadata.get(controls::SensorBlackLevels);
>  	if (blackLevels) {
> -		Span<const int32_t> levels = *blackLevels;
> +		Span<const int32_t, 4> levels = *blackLevels;
>
>  		/*
>  		 * The black levels control is specified in R, Gr, Gb, B order.
> diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> index 3f99b5e2..46ba4394 100755
> --- a/utils/gen-controls.py
> +++ b/utils/gen-controls.py
> @@ -7,6 +7,8 @@
>  # gen-controls.py - Generate control definitions from YAML
>
>  import argparse
> +from functools import reduce
> +import operator
>  import string
>  import sys
>  import yaml
> @@ -22,6 +24,24 @@ def format_description(description):
>      return '\n'.join([(line and ' * ' or ' *') + line for line in description])
>
>
> +def get_ctrl_type(ctrl):
> +    ctrl_type = ctrl['type']
> +    ctrl_size_arr = ctrl.get('size')
> +
> +    if ctrl_type == 'string':
> +        return 'std::string'
> +    elif ctrl_size_arr is not None:
> +        if len(ctrl_size_arr) > 0:
> +            # fixed-sized Span
> +            ctrl_span_size = reduce(operator.mul, ctrl_size_arr)
> +            return f"Span<const {ctrl_type}, {ctrl_span_size}>"
> +        else:
> +            # variable-sized Span
> +            return f"Span<const {ctrl_type}>"
> +    else:
> +        return ctrl_type
> +
> +
>  def generate_cpp(controls):
>      enum_doc_start_template = string.Template('''/**
>   * \\enum ${name}Enum
> @@ -50,11 +70,7 @@ ${description}
>          name, ctrl = ctrl.popitem()
>          id_name = snake_case(name).upper()
>
> -        ctrl_type = ctrl['type']
> -        if ctrl_type == 'string':
> -            ctrl_type = 'std::string'
> -        elif ctrl.get('size'):
> -            ctrl_type = 'Span<const %s>' % ctrl_type
> +        ctrl_type = get_ctrl_type(ctrl)
>
>          info = {
>              'name': name,
> @@ -135,11 +151,7 @@ def generate_h(controls):
>
>          ids.append('\t' + id_name + ' = ' + str(id_value) + ',')
>
> -        ctrl_type = ctrl['type']
> -        if ctrl_type == 'string':
> -            ctrl_type = 'std::string'
> -        elif ctrl.get('size'):
> -            ctrl_type = 'Span<const %s>' % ctrl_type
> +        ctrl_type = get_ctrl_type(ctrl)
>
>          info = {
>              'name': name,
> --
> 2.34.1
>
Kieran Bingham Aug. 3, 2022, 12:17 p.m. UTC | #2 
Quoting Jacopo Mondi via libcamera-devel (2022-08-03 10:44:01)
> Hi Christian,
> 
> On Tue, Aug 02, 2022 at 11:03:24PM +0200, Christian Rauch wrote:
> > Define Span types explicitly as either variable- or fixed-sized. This
> > introduces a new convention for defining Span dimensions in the property
> > and control value definitions and generates Span types as variable-sized
> > Span<T> or as fixed-sized Span<T,N>.
> >
> > Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> The issue with gcc8 seems now solved.
> 
> One more tag and I hope we can collect the patch ?

What are the user facing API changes here? (i.e. how much does this
break applications?)

I'm surprised there are no updates to the python bindings in this patch
?

Is that all handled gracefully? (Is it tested?)

--
Kieran


> 
> Thanks
>   j
> 
> > ---
> >  include/libcamera/controls.h        |  2 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp | 19 +++++++++--------
> >  src/libcamera/control_ids.yaml      |  4 ++--
> >  src/libcamera/property_ids.yaml     |  4 ++--
> >  src/qcam/dng_writer.cpp             |  2 +-
> >  utils/gen-controls.py               | 32 ++++++++++++++++++++---------
> >  6 files changed, 38 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index b1b52acb..ebc168fc 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -168,7 +168,7 @@ public:
> >
> >               using V = typename T::value_type;
> >               const V *value = reinterpret_cast<const V *>(data().data());
> > -             return { value, numElements_ };
> > +             return T{ value, numElements_ };
> >       }
> >
> >  #ifndef __DOXYGEN__
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 6befdd71..69c73f8c 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -567,18 +567,19 @@ void IPARPi::reportMetadata()
> >
> >       AwbStatus *awbStatus = rpiMetadata_.getLocked<AwbStatus>("awb.status");
> >       if (awbStatus) {
> > -             libcameraMetadata_.set(controls::ColourGains, { static_cast<float>(awbStatus->gainR),
> > -                                                             static_cast<float>(awbStatus->gainB) });
> > +             libcameraMetadata_.set(controls::ColourGains,
> > +                                    Span<const float, 2>({ static_cast<float>(awbStatus->gainR),
> > +                                                           static_cast<float>(awbStatus->gainB) }));
> >               libcameraMetadata_.set(controls::ColourTemperature, awbStatus->temperatureK);
> >       }
> >
> >       BlackLevelStatus *blackLevelStatus = rpiMetadata_.getLocked<BlackLevelStatus>("black_level.status");
> >       if (blackLevelStatus)
> >               libcameraMetadata_.set(controls::SensorBlackLevels,
> > -                                    { static_cast<int32_t>(blackLevelStatus->blackLevelR),
> > -                                      static_cast<int32_t>(blackLevelStatus->blackLevelG),
> > -                                      static_cast<int32_t>(blackLevelStatus->blackLevelG),
> > -                                      static_cast<int32_t>(blackLevelStatus->blackLevelB) });
> > +                                    Span<const int32_t, 4>({ static_cast<int32_t>(blackLevelStatus->blackLevelR),
> > +                                                             static_cast<int32_t>(blackLevelStatus->blackLevelG),
> > +                                                             static_cast<int32_t>(blackLevelStatus->blackLevelG),
> > +                                                             static_cast<int32_t>(blackLevelStatus->blackLevelB) }));
> >
> >       FocusStatus *focusStatus = rpiMetadata_.getLocked<FocusStatus>("focus.status");
> >       if (focusStatus && focusStatus->num == 12) {
> > @@ -883,7 +884,7 @@ void IPARPi::queueRequest(const ControlList &controls)
> >                       if (gains[0] != 0.0f && gains[1] != 0.0f)
> >                               /* A gain of 0.0f will switch back to auto mode. */
> >                               libcameraMetadata_.set(controls::ColourGains,
> > -                                                    { gains[0], gains[1] });
> > +                                                    Span<const float, 2>({ gains[0], gains[1] }));
> >                       break;
> >               }
> >
> > @@ -1167,8 +1168,8 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur
> >
> >       /* Return the validated limits via metadata. */
> >       libcameraMetadata_.set(controls::FrameDurationLimits,
> > -                            { static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
> > -                              static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) });
> > +                            Span<const int64_t, 2>({ static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
> > +                                                     static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) }));
> >
> >       /*
> >        * Calculate the maximum exposure time possible for the AGC to use.
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index ecab3ae9..5fa168c6 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -291,7 +291,7 @@ controls:
> >          transformation. The 3x3 matrix is stored in conventional reading
> >          order in an array of 9 floating point values.
> >
> > -      size: [3x3]
> > +      size: [3,3]
> >
> >    - ScalerCrop:
> >        type: Rectangle
> > @@ -525,7 +525,7 @@ controls:
> >          the window where the focal distance for the objects shown in that part
> >          of the image are closest to the camera.
> >
> > -      size: [n]
> > +      size: []
> >
> >    - AfTrigger:
> >        type: int32_t
> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > index 11b7ebdc..a87485d7 100644
> > --- a/src/libcamera/property_ids.yaml
> > +++ b/src/libcamera/property_ids.yaml
> > @@ -497,7 +497,7 @@ controls:
> >
> >    - PixelArrayOpticalBlackRectangles:
> >        type: Rectangle
> > -      size: [n]
> > +      size: []
> >        description: |
> >          The pixel array region(s) which contain optical black pixels
> >          considered valid for calibration purposes.
> > @@ -592,7 +592,7 @@ controls:
> >
> >    - PixelArrayActiveAreas:
> >        type: Rectangle
> > -      size: [n]
> > +      size: []
> >        description: |
> >          The PixelArrayActiveAreas property defines the (possibly multiple and
> >          overlapping) portions of the camera sensor readable pixel matrix
> > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
> > index 5b94b3f2..b4362537 100644
> > --- a/src/qcam/dng_writer.cpp
> > +++ b/src/qcam/dng_writer.cpp
> > @@ -517,7 +517,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> >
> >       const auto &blackLevels = metadata.get(controls::SensorBlackLevels);
> >       if (blackLevels) {
> > -             Span<const int32_t> levels = *blackLevels;
> > +             Span<const int32_t, 4> levels = *blackLevels;
> >
> >               /*
> >                * The black levels control is specified in R, Gr, Gb, B order.
> > diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> > index 3f99b5e2..46ba4394 100755
> > --- a/utils/gen-controls.py
> > +++ b/utils/gen-controls.py
> > @@ -7,6 +7,8 @@
> >  # gen-controls.py - Generate control definitions from YAML
> >
> >  import argparse
> > +from functools import reduce
> > +import operator
> >  import string
> >  import sys
> >  import yaml
> > @@ -22,6 +24,24 @@ def format_description(description):
> >      return '\n'.join([(line and ' * ' or ' *') + line for line in description])
> >
> >
> > +def get_ctrl_type(ctrl):
> > +    ctrl_type = ctrl['type']
> > +    ctrl_size_arr = ctrl.get('size')
> > +
> > +    if ctrl_type == 'string':
> > +        return 'std::string'
> > +    elif ctrl_size_arr is not None:
> > +        if len(ctrl_size_arr) > 0:
> > +            # fixed-sized Span
> > +            ctrl_span_size = reduce(operator.mul, ctrl_size_arr)
> > +            return f"Span<const {ctrl_type}, {ctrl_span_size}>"
> > +        else:
> > +            # variable-sized Span
> > +            return f"Span<const {ctrl_type}>"
> > +    else:
> > +        return ctrl_type
> > +
> > +
> >  def generate_cpp(controls):
> >      enum_doc_start_template = string.Template('''/**
> >   * \\enum ${name}Enum
> > @@ -50,11 +70,7 @@ ${description}
> >          name, ctrl = ctrl.popitem()
> >          id_name = snake_case(name).upper()
> >
> > -        ctrl_type = ctrl['type']
> > -        if ctrl_type == 'string':
> > -            ctrl_type = 'std::string'
> > -        elif ctrl.get('size'):
> > -            ctrl_type = 'Span<const %s>' % ctrl_type
> > +        ctrl_type = get_ctrl_type(ctrl)
> >
> >          info = {
> >              'name': name,
> > @@ -135,11 +151,7 @@ def generate_h(controls):
> >
> >          ids.append('\t' + id_name + ' = ' + str(id_value) + ',')
> >
> > -        ctrl_type = ctrl['type']
> > -        if ctrl_type == 'string':
> > -            ctrl_type = 'std::string'
> > -        elif ctrl.get('size'):
> > -            ctrl_type = 'Span<const %s>' % ctrl_type
> > +        ctrl_type = get_ctrl_type(ctrl)
> >
> >          info = {
> >              'name': name,
> > --
> > 2.34.1
> >
Christian Rauch Aug. 3, 2022, 8:13 p.m. UTC | #3 
Hi Kieran,

Am 03.08.22 um 14:17 schrieb Kieran Bingham:
> Quoting Jacopo Mondi via libcamera-devel (2022-08-03 10:44:01)
>> Hi Christian,
>>
>> On Tue, Aug 02, 2022 at 11:03:24PM +0200, Christian Rauch wrote:
>>> Define Span types explicitly as either variable- or fixed-sized. This
>>> introduces a new convention for defining Span dimensions in the property
>>> and control value definitions and generates Span types as variable-sized
>>> Span<T> or as fixed-sized Span<T,N>.
>>>
>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>>
>> The issue with gcc8 seems now solved.
>>
>> One more tag and I hope we can collect the patch ?
>
> What are the user facing API changes here? (i.e. how much does this
> break applications?)

Everything that touches fixed-sized Spans with explicit types (i.e. no
auto) need a change that defines the size. variable-sized spans should
behave as before. E.g. in the diff you can see that something like
"Span<const int32_t> levels = *blackLevels;"
needs to be set the Span size as template paremter:
"Span<const int32_t, 4> levels = *blackLevels;"

But overall, I only had to change this in 5 places in the entire
codebase to make this compile. So I don't think the impact will be that big.

>
> I'm surprised there are no updates to the python bindings in this patch
> ?
>
> Is that all handled gracefully? (Is it tested?)

I compiled ("meson build_py -Dpycamera=enabled") and ran the tests
("ninja -C build_py/ test") without failures. Is there another way to
test that the python bindings work?

A quick glimpse into "py_controls_generated.cpp" tells me that Spans are
not used there.

>
> --
> Kieran
>
>
>>
>> Thanks
>>   j
>>
>>> ---
>>>  include/libcamera/controls.h        |  2 +-
>>>  src/ipa/raspberrypi/raspberrypi.cpp | 19 +++++++++--------
>>>  src/libcamera/control_ids.yaml      |  4 ++--
>>>  src/libcamera/property_ids.yaml     |  4 ++--
>>>  src/qcam/dng_writer.cpp             |  2 +-
>>>  utils/gen-controls.py               | 32 ++++++++++++++++++++---------
>>>  6 files changed, 38 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>>> index b1b52acb..ebc168fc 100644
>>> --- a/include/libcamera/controls.h
>>> +++ b/include/libcamera/controls.h
>>> @@ -168,7 +168,7 @@ public:
>>>
>>>               using V = typename T::value_type;
>>>               const V *value = reinterpret_cast<const V *>(data().data());
>>> -             return { value, numElements_ };
>>> +             return T{ value, numElements_ };
>>>       }
>>>
>>>  #ifndef __DOXYGEN__
>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>>> index 6befdd71..69c73f8c 100644
>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>>> @@ -567,18 +567,19 @@ void IPARPi::reportMetadata()
>>>
>>>       AwbStatus *awbStatus = rpiMetadata_.getLocked<AwbStatus>("awb.status");
>>>       if (awbStatus) {
>>> -             libcameraMetadata_.set(controls::ColourGains, { static_cast<float>(awbStatus->gainR),
>>> -                                                             static_cast<float>(awbStatus->gainB) });
>>> +             libcameraMetadata_.set(controls::ColourGains,
>>> +                                    Span<const float, 2>({ static_cast<float>(awbStatus->gainR),
>>> +                                                           static_cast<float>(awbStatus->gainB) }));
>>>               libcameraMetadata_.set(controls::ColourTemperature, awbStatus->temperatureK);
>>>       }
>>>
>>>       BlackLevelStatus *blackLevelStatus = rpiMetadata_.getLocked<BlackLevelStatus>("black_level.status");
>>>       if (blackLevelStatus)
>>>               libcameraMetadata_.set(controls::SensorBlackLevels,
>>> -                                    { static_cast<int32_t>(blackLevelStatus->blackLevelR),
>>> -                                      static_cast<int32_t>(blackLevelStatus->blackLevelG),
>>> -                                      static_cast<int32_t>(blackLevelStatus->blackLevelG),
>>> -                                      static_cast<int32_t>(blackLevelStatus->blackLevelB) });
>>> +                                    Span<const int32_t, 4>({ static_cast<int32_t>(blackLevelStatus->blackLevelR),
>>> +                                                             static_cast<int32_t>(blackLevelStatus->blackLevelG),
>>> +                                                             static_cast<int32_t>(blackLevelStatus->blackLevelG),
>>> +                                                             static_cast<int32_t>(blackLevelStatus->blackLevelB) }));
>>>
>>>       FocusStatus *focusStatus = rpiMetadata_.getLocked<FocusStatus>("focus.status");
>>>       if (focusStatus && focusStatus->num == 12) {
>>> @@ -883,7 +884,7 @@ void IPARPi::queueRequest(const ControlList &controls)
>>>                       if (gains[0] != 0.0f && gains[1] != 0.0f)
>>>                               /* A gain of 0.0f will switch back to auto mode. */
>>>                               libcameraMetadata_.set(controls::ColourGains,
>>> -                                                    { gains[0], gains[1] });
>>> +                                                    Span<const float, 2>({ gains[0], gains[1] }));
>>>                       break;
>>>               }
>>>
>>> @@ -1167,8 +1168,8 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur
>>>
>>>       /* Return the validated limits via metadata. */
>>>       libcameraMetadata_.set(controls::FrameDurationLimits,
>>> -                            { static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
>>> -                              static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) });
>>> +                            Span<const int64_t, 2>({ static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
>>> +                                                     static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) }));
>>>
>>>       /*
>>>        * Calculate the maximum exposure time possible for the AGC to use.
>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
>>> index ecab3ae9..5fa168c6 100644
>>> --- a/src/libcamera/control_ids.yaml
>>> +++ b/src/libcamera/control_ids.yaml
>>> @@ -291,7 +291,7 @@ controls:
>>>          transformation. The 3x3 matrix is stored in conventional reading
>>>          order in an array of 9 floating point values.
>>>
>>> -      size: [3x3]
>>> +      size: [3,3]
>>>
>>>    - ScalerCrop:
>>>        type: Rectangle
>>> @@ -525,7 +525,7 @@ controls:
>>>          the window where the focal distance for the objects shown in that part
>>>          of the image are closest to the camera.
>>>
>>> -      size: [n]
>>> +      size: []
>>>
>>>    - AfTrigger:
>>>        type: int32_t
>>> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
>>> index 11b7ebdc..a87485d7 100644
>>> --- a/src/libcamera/property_ids.yaml
>>> +++ b/src/libcamera/property_ids.yaml
>>> @@ -497,7 +497,7 @@ controls:
>>>
>>>    - PixelArrayOpticalBlackRectangles:
>>>        type: Rectangle
>>> -      size: [n]
>>> +      size: []
>>>        description: |
>>>          The pixel array region(s) which contain optical black pixels
>>>          considered valid for calibration purposes.
>>> @@ -592,7 +592,7 @@ controls:
>>>
>>>    - PixelArrayActiveAreas:
>>>        type: Rectangle
>>> -      size: [n]
>>> +      size: []
>>>        description: |
>>>          The PixelArrayActiveAreas property defines the (possibly multiple and
>>>          overlapping) portions of the camera sensor readable pixel matrix
>>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
>>> index 5b94b3f2..b4362537 100644
>>> --- a/src/qcam/dng_writer.cpp
>>> +++ b/src/qcam/dng_writer.cpp
>>> @@ -517,7 +517,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,
>>>
>>>       const auto &blackLevels = metadata.get(controls::SensorBlackLevels);
>>>       if (blackLevels) {
>>> -             Span<const int32_t> levels = *blackLevels;
>>> +             Span<const int32_t, 4> levels = *blackLevels;
>>>
>>>               /*
>>>                * The black levels control is specified in R, Gr, Gb, B order.
>>> diff --git a/utils/gen-controls.py b/utils/gen-controls.py
>>> index 3f99b5e2..46ba4394 100755
>>> --- a/utils/gen-controls.py
>>> +++ b/utils/gen-controls.py
>>> @@ -7,6 +7,8 @@
>>>  # gen-controls.py - Generate control definitions from YAML
>>>
>>>  import argparse
>>> +from functools import reduce
>>> +import operator
>>>  import string
>>>  import sys
>>>  import yaml
>>> @@ -22,6 +24,24 @@ def format_description(description):
>>>      return '\n'.join([(line and ' * ' or ' *') + line for line in description])
>>>
>>>
>>> +def get_ctrl_type(ctrl):
>>> +    ctrl_type = ctrl['type']
>>> +    ctrl_size_arr = ctrl.get('size')
>>> +
>>> +    if ctrl_type == 'string':
>>> +        return 'std::string'
>>> +    elif ctrl_size_arr is not None:
>>> +        if len(ctrl_size_arr) > 0:
>>> +            # fixed-sized Span
>>> +            ctrl_span_size = reduce(operator.mul, ctrl_size_arr)
>>> +            return f"Span<const {ctrl_type}, {ctrl_span_size}>"
>>> +        else:
>>> +            # variable-sized Span
>>> +            return f"Span<const {ctrl_type}>"
>>> +    else:
>>> +        return ctrl_type
>>> +
>>> +
>>>  def generate_cpp(controls):
>>>      enum_doc_start_template = string.Template('''/**
>>>   * \\enum ${name}Enum
>>> @@ -50,11 +70,7 @@ ${description}
>>>          name, ctrl = ctrl.popitem()
>>>          id_name = snake_case(name).upper()
>>>
>>> -        ctrl_type = ctrl['type']
>>> -        if ctrl_type == 'string':
>>> -            ctrl_type = 'std::string'
>>> -        elif ctrl.get('size'):
>>> -            ctrl_type = 'Span<const %s>' % ctrl_type
>>> +        ctrl_type = get_ctrl_type(ctrl)
>>>
>>>          info = {
>>>              'name': name,
>>> @@ -135,11 +151,7 @@ def generate_h(controls):
>>>
>>>          ids.append('\t' + id_name + ' = ' + str(id_value) + ',')
>>>
>>> -        ctrl_type = ctrl['type']
>>> -        if ctrl_type == 'string':
>>> -            ctrl_type = 'std::string'
>>> -        elif ctrl.get('size'):
>>> -            ctrl_type = 'Span<const %s>' % ctrl_type
>>> +        ctrl_type = get_ctrl_type(ctrl)
>>>
>>>          info = {
>>>              'name': name,
>>> --
>>> 2.34.1
>>>
Laurent Pinchart Aug. 9, 2022, 10:53 a.m. UTC | #4 
On Wed, Aug 03, 2022 at 11:44:01AM +0200, Jacopo Mondi via libcamera-devel wrote:
> Hi Christian,
> 
> On Tue, Aug 02, 2022 at 11:03:24PM +0200, Christian Rauch wrote:
> > Define Span types explicitly as either variable- or fixed-sized. This
> > introduces a new convention for defining Span dimensions in the property
> > and control value definitions and generates Span types as variable-sized
> > Span<T> or as fixed-sized Span<T,N>.
> >
> > Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> The issue with gcc8 seems now solved.
> 
> One more tag and I hope we can collect the patch ?

There are still a few things I'd like to change, but I'll do so by
submitting patches on top, it will be more efficient.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > ---
> >  include/libcamera/controls.h        |  2 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp | 19 +++++++++--------
> >  src/libcamera/control_ids.yaml      |  4 ++--
> >  src/libcamera/property_ids.yaml     |  4 ++--
> >  src/qcam/dng_writer.cpp             |  2 +-
> >  utils/gen-controls.py               | 32 ++++++++++++++++++++---------
> >  6 files changed, 38 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index b1b52acb..ebc168fc 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -168,7 +168,7 @@ public:
> >
> >  		using V = typename T::value_type;
> >  		const V *value = reinterpret_cast<const V *>(data().data());
> > -		return { value, numElements_ };
> > +		return T{ value, numElements_ };
> >  	}
> >
> >  #ifndef __DOXYGEN__
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 6befdd71..69c73f8c 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -567,18 +567,19 @@ void IPARPi::reportMetadata()
> >
> >  	AwbStatus *awbStatus = rpiMetadata_.getLocked<AwbStatus>("awb.status");
> >  	if (awbStatus) {
> > -		libcameraMetadata_.set(controls::ColourGains, { static_cast<float>(awbStatus->gainR),
> > -								static_cast<float>(awbStatus->gainB) });
> > +		libcameraMetadata_.set(controls::ColourGains,
> > +				       Span<const float, 2>({ static_cast<float>(awbStatus->gainR),
> > +							      static_cast<float>(awbStatus->gainB) }));
> >  		libcameraMetadata_.set(controls::ColourTemperature, awbStatus->temperatureK);
> >  	}
> >
> >  	BlackLevelStatus *blackLevelStatus = rpiMetadata_.getLocked<BlackLevelStatus>("black_level.status");
> >  	if (blackLevelStatus)
> >  		libcameraMetadata_.set(controls::SensorBlackLevels,
> > -				       { static_cast<int32_t>(blackLevelStatus->blackLevelR),
> > -					 static_cast<int32_t>(blackLevelStatus->blackLevelG),
> > -					 static_cast<int32_t>(blackLevelStatus->blackLevelG),
> > -					 static_cast<int32_t>(blackLevelStatus->blackLevelB) });
> > +				       Span<const int32_t, 4>({ static_cast<int32_t>(blackLevelStatus->blackLevelR),
> > +								static_cast<int32_t>(blackLevelStatus->blackLevelG),
> > +								static_cast<int32_t>(blackLevelStatus->blackLevelG),
> > +								static_cast<int32_t>(blackLevelStatus->blackLevelB) }));
> >
> >  	FocusStatus *focusStatus = rpiMetadata_.getLocked<FocusStatus>("focus.status");
> >  	if (focusStatus && focusStatus->num == 12) {
> > @@ -883,7 +884,7 @@ void IPARPi::queueRequest(const ControlList &controls)
> >  			if (gains[0] != 0.0f && gains[1] != 0.0f)
> >  				/* A gain of 0.0f will switch back to auto mode. */
> >  				libcameraMetadata_.set(controls::ColourGains,
> > -						       { gains[0], gains[1] });
> > +						       Span<const float, 2>({ gains[0], gains[1] }));
> >  			break;
> >  		}
> >
> > @@ -1167,8 +1168,8 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur
> >
> >  	/* Return the validated limits via metadata. */
> >  	libcameraMetadata_.set(controls::FrameDurationLimits,
> > -			       { static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
> > -				 static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) });
> > +			       Span<const int64_t, 2>({ static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
> > +							static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) }));
> >
> >  	/*
> >  	 * Calculate the maximum exposure time possible for the AGC to use.
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index ecab3ae9..5fa168c6 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -291,7 +291,7 @@ controls:
> >          transformation. The 3x3 matrix is stored in conventional reading
> >          order in an array of 9 floating point values.
> >
> > -      size: [3x3]
> > +      size: [3,3]
> >
> >    - ScalerCrop:
> >        type: Rectangle
> > @@ -525,7 +525,7 @@ controls:
> >          the window where the focal distance for the objects shown in that part
> >          of the image are closest to the camera.
> >
> > -      size: [n]
> > +      size: []
> >
> >    - AfTrigger:
> >        type: int32_t
> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > index 11b7ebdc..a87485d7 100644
> > --- a/src/libcamera/property_ids.yaml
> > +++ b/src/libcamera/property_ids.yaml
> > @@ -497,7 +497,7 @@ controls:
> >
> >    - PixelArrayOpticalBlackRectangles:
> >        type: Rectangle
> > -      size: [n]
> > +      size: []
> >        description: |
> >          The pixel array region(s) which contain optical black pixels
> >          considered valid for calibration purposes.
> > @@ -592,7 +592,7 @@ controls:
> >
> >    - PixelArrayActiveAreas:
> >        type: Rectangle
> > -      size: [n]
> > +      size: []
> >        description: |
> >          The PixelArrayActiveAreas property defines the (possibly multiple and
> >          overlapping) portions of the camera sensor readable pixel matrix
> > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
> > index 5b94b3f2..b4362537 100644
> > --- a/src/qcam/dng_writer.cpp
> > +++ b/src/qcam/dng_writer.cpp
> > @@ -517,7 +517,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> >
> >  	const auto &blackLevels = metadata.get(controls::SensorBlackLevels);
> >  	if (blackLevels) {
> > -		Span<const int32_t> levels = *blackLevels;
> > +		Span<const int32_t, 4> levels = *blackLevels;
> >
> >  		/*
> >  		 * The black levels control is specified in R, Gr, Gb, B order.
> > diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> > index 3f99b5e2..46ba4394 100755
> > --- a/utils/gen-controls.py
> > +++ b/utils/gen-controls.py
> > @@ -7,6 +7,8 @@
> >  # gen-controls.py - Generate control definitions from YAML
> >
> >  import argparse
> > +from functools import reduce
> > +import operator
> >  import string
> >  import sys
> >  import yaml
> > @@ -22,6 +24,24 @@ def format_description(description):
> >      return '\n'.join([(line and ' * ' or ' *') + line for line in description])
> >
> >
> > +def get_ctrl_type(ctrl):
> > +    ctrl_type = ctrl['type']
> > +    ctrl_size_arr = ctrl.get('size')
> > +
> > +    if ctrl_type == 'string':
> > +        return 'std::string'
> > +    elif ctrl_size_arr is not None:
> > +        if len(ctrl_size_arr) > 0:
> > +            # fixed-sized Span
> > +            ctrl_span_size = reduce(operator.mul, ctrl_size_arr)
> > +            return f"Span<const {ctrl_type}, {ctrl_span_size}>"
> > +        else:
> > +            # variable-sized Span
> > +            return f"Span<const {ctrl_type}>"
> > +    else:
> > +        return ctrl_type
> > +
> > +
> >  def generate_cpp(controls):
> >      enum_doc_start_template = string.Template('''/**
> >   * \\enum ${name}Enum
> > @@ -50,11 +70,7 @@ ${description}
> >          name, ctrl = ctrl.popitem()
> >          id_name = snake_case(name).upper()
> >
> > -        ctrl_type = ctrl['type']
> > -        if ctrl_type == 'string':
> > -            ctrl_type = 'std::string'
> > -        elif ctrl.get('size'):
> > -            ctrl_type = 'Span<const %s>' % ctrl_type
> > +        ctrl_type = get_ctrl_type(ctrl)
> >
> >          info = {
> >              'name': name,
> > @@ -135,11 +151,7 @@ def generate_h(controls):
> >
> >          ids.append('\t' + id_name + ' = ' + str(id_value) + ',')
> >
> > -        ctrl_type = ctrl['type']
> > -        if ctrl_type == 'string':
> > -            ctrl_type = 'std::string'
> > -        elif ctrl.get('size'):
> > -            ctrl_type = 'Span<const %s>' % ctrl_type
> > +        ctrl_type = get_ctrl_type(ctrl)
> >
> >          info = {
> >              'name': name,

Patch
diff mbox series 

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index b1b52acb..ebc168fc 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -168,7 +168,7 @@  public:

 		using V = typename T::value_type;
 		const V *value = reinterpret_cast<const V *>(data().data());
-		return { value, numElements_ };
+		return T{ value, numElements_ };
 	}

 #ifndef __DOXYGEN__
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 6befdd71..69c73f8c 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -567,18 +567,19 @@  void IPARPi::reportMetadata()

 	AwbStatus *awbStatus = rpiMetadata_.getLocked<AwbStatus>("awb.status");
 	if (awbStatus) {
-		libcameraMetadata_.set(controls::ColourGains, { static_cast<float>(awbStatus->gainR),
-								static_cast<float>(awbStatus->gainB) });
+		libcameraMetadata_.set(controls::ColourGains,
+				       Span<const float, 2>({ static_cast<float>(awbStatus->gainR),
+							      static_cast<float>(awbStatus->gainB) }));
 		libcameraMetadata_.set(controls::ColourTemperature, awbStatus->temperatureK);
 	}

 	BlackLevelStatus *blackLevelStatus = rpiMetadata_.getLocked<BlackLevelStatus>("black_level.status");
 	if (blackLevelStatus)
 		libcameraMetadata_.set(controls::SensorBlackLevels,
-				       { static_cast<int32_t>(blackLevelStatus->blackLevelR),
-					 static_cast<int32_t>(blackLevelStatus->blackLevelG),
-					 static_cast<int32_t>(blackLevelStatus->blackLevelG),
-					 static_cast<int32_t>(blackLevelStatus->blackLevelB) });
+				       Span<const int32_t, 4>({ static_cast<int32_t>(blackLevelStatus->blackLevelR),
+								static_cast<int32_t>(blackLevelStatus->blackLevelG),
+								static_cast<int32_t>(blackLevelStatus->blackLevelG),
+								static_cast<int32_t>(blackLevelStatus->blackLevelB) }));

 	FocusStatus *focusStatus = rpiMetadata_.getLocked<FocusStatus>("focus.status");
 	if (focusStatus && focusStatus->num == 12) {
@@ -883,7 +884,7 @@  void IPARPi::queueRequest(const ControlList &controls)
 			if (gains[0] != 0.0f && gains[1] != 0.0f)
 				/* A gain of 0.0f will switch back to auto mode. */
 				libcameraMetadata_.set(controls::ColourGains,
-						       { gains[0], gains[1] });
+						       Span<const float, 2>({ gains[0], gains[1] }));
 			break;
 		}

@@ -1167,8 +1168,8 @@  void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur

 	/* Return the validated limits via metadata. */
 	libcameraMetadata_.set(controls::FrameDurationLimits,
-			       { static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
-				 static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) });
+			       Span<const int64_t, 2>({ static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
+							static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) }));

 	/*
 	 * Calculate the maximum exposure time possible for the AGC to use.
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
index ecab3ae9..5fa168c6 100644
--- a/src/libcamera/control_ids.yaml
+++ b/src/libcamera/control_ids.yaml
@@ -291,7 +291,7 @@  controls:
         transformation. The 3x3 matrix is stored in conventional reading
         order in an array of 9 floating point values.

-      size: [3x3]
+      size: [3,3]

   - ScalerCrop:
       type: Rectangle
@@ -525,7 +525,7 @@  controls:
         the window where the focal distance for the objects shown in that part
         of the image are closest to the camera.

-      size: [n]
+      size: []

   - AfTrigger:
       type: int32_t
diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
index 11b7ebdc..a87485d7 100644
--- a/src/libcamera/property_ids.yaml
+++ b/src/libcamera/property_ids.yaml
@@ -497,7 +497,7 @@  controls:

   - PixelArrayOpticalBlackRectangles:
       type: Rectangle
-      size: [n]
+      size: []
       description: |
         The pixel array region(s) which contain optical black pixels
         considered valid for calibration purposes.
@@ -592,7 +592,7 @@  controls:

   - PixelArrayActiveAreas:
       type: Rectangle
-      size: [n]
+      size: []
       description: |
         The PixelArrayActiveAreas property defines the (possibly multiple and
         overlapping) portions of the camera sensor readable pixel matrix
diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
index 5b94b3f2..b4362537 100644
--- a/src/qcam/dng_writer.cpp
+++ b/src/qcam/dng_writer.cpp
@@ -517,7 +517,7 @@  int DNGWriter::write(const char *filename, const Camera *camera,

 	const auto &blackLevels = metadata.get(controls::SensorBlackLevels);
 	if (blackLevels) {
-		Span<const int32_t> levels = *blackLevels;
+		Span<const int32_t, 4> levels = *blackLevels;

 		/*
 		 * The black levels control is specified in R, Gr, Gb, B order.
diff --git a/utils/gen-controls.py b/utils/gen-controls.py
index 3f99b5e2..46ba4394 100755
--- a/utils/gen-controls.py
+++ b/utils/gen-controls.py
@@ -7,6 +7,8 @@ 
 # gen-controls.py - Generate control definitions from YAML

 import argparse
+from functools import reduce
+import operator
 import string
 import sys
 import yaml
@@ -22,6 +24,24 @@  def format_description(description):
     return '\n'.join([(line and ' * ' or ' *') + line for line in description])


+def get_ctrl_type(ctrl):
+    ctrl_type = ctrl['type']
+    ctrl_size_arr = ctrl.get('size')
+
+    if ctrl_type == 'string':
+        return 'std::string'
+    elif ctrl_size_arr is not None:
+        if len(ctrl_size_arr) > 0:
+            # fixed-sized Span
+            ctrl_span_size = reduce(operator.mul, ctrl_size_arr)
+            return f"Span<const {ctrl_type}, {ctrl_span_size}>"
+        else:
+            # variable-sized Span
+            return f"Span<const {ctrl_type}>"
+    else:
+        return ctrl_type
+
+
 def generate_cpp(controls):
     enum_doc_start_template = string.Template('''/**
  * \\enum ${name}Enum
@@ -50,11 +70,7 @@  ${description}
         name, ctrl = ctrl.popitem()
         id_name = snake_case(name).upper()

-        ctrl_type = ctrl['type']
-        if ctrl_type == 'string':
-            ctrl_type = 'std::string'
-        elif ctrl.get('size'):
-            ctrl_type = 'Span<const %s>' % ctrl_type
+        ctrl_type = get_ctrl_type(ctrl)

         info = {
             'name': name,
@@ -135,11 +151,7 @@  def generate_h(controls):

         ids.append('\t' + id_name + ' = ' + str(id_value) + ',')

-        ctrl_type = ctrl['type']
-        if ctrl_type == 'string':
-            ctrl_type = 'std::string'
-        elif ctrl.get('size'):
-            ctrl_type = 'Span<const %s>' % ctrl_type
+        ctrl_type = get_ctrl_type(ctrl)

         info = {
             'name': name,