[libcamera-devel] [PATCH v3 06/14] libcamera: controls: Generate a vector of enumerated values

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Oct 21, 2020 at 04:36:27PM +0200, Jacopo Mondi wrote:
> For each Control that support enumerated values generate a vector
> of ControlValues which contains the full list of values.
> 
> At the expense of a slight increase in memory occupation this change
> allows the construction of the ControlInfo associated with a Control
> from the values list, defaulting the minimum and maximum values
> reported by the ControlInfo.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/control_ids.cpp.in |  2 ++
>  utils/gen-controls.py            | 10 ++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> index 056645cfbdfb..ca0b5b22f899 100644
> --- a/src/libcamera/control_ids.cpp.in
> +++ b/src/libcamera/control_ids.cpp.in
> @@ -6,8 +6,10 @@
>   *
>   * This file is auto-generated. Do not edit.
>   */
> +#include <vector>

This should go to control_ids.h.in as that's where you add
std::vector<>.

>  #include <libcamera/control_ids.h>
> +#include <libcamera/controls.h>
>  
>  /**
>   * \file control_ids.h
> diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> index bf681503f86a..23aace50f666 100755
> --- a/utils/gen-controls.py
> +++ b/utils/gen-controls.py
> @@ -100,6 +100,8 @@ ${description}
>  def generate_h(controls):
>      enum_template_start = string.Template('''enum ${name}Enum {''')
>      enum_value_template = string.Template('''\t${name} = ${value},''')
> +    enum_list_start = string.Template('''static const std::vector<ControlValue> ${name}Values = {''')
> +    enum_list_values = string.Template('''\tstatic_cast<int32_t>(${name}),''')

Shouldn't you declare the vector as extern const here, and define it in
control_ids.cpp ? Otherwise you'll duplicate the values in every
compilation unit that includes control_ids.h.

I would also make it an std::array, to avoid global objects that have
non-trivial constructors. But the ControlValue stored in the vector has
a non-trivial constructor :-/ Do you expect enumerated controls with
other types than Integer32 ? If not we could make the API more specific,
but maybe that's not a very good idea in case we need to extend it
later.

std::array may be best anyway, even with ControlValue, as it will be
simpler. Given that we know here how many enumerated values we generate,
setting the number of elements in the std::array template parameter
shouldn't be a problem. Then, you would need to modify the ControlInfo
constructor to take a Span<ControlValue> instead of a std::vector<>.

>      template = string.Template('''extern const Control<${type}> ${name};''')
>  
>      ctrls = []
> @@ -140,6 +142,14 @@ def generate_h(controls):
>                  target_ctrls.append(enum_value_template.substitute(value_info))
>              target_ctrls.append("};")
>  
> +            target_ctrls.append(enum_list_start.substitute(info))
> +            for entry in enum:
> +                value_info = {
> +                    'name': entry['name'],
> +                }
> +                target_ctrls.append(enum_list_values.substitute(value_info))
> +            target_ctrls.append("};")
> +

Not a candidate for this series, but given that we will soon depend on
jinja at build time (for IPA IPC and for tracing), I think we could
upgrade the control and property templates to jinja templates, it should
simplify this code.

>          target_ctrls.append(template.substitute(info))
>          id_value += 1
>  

-- 
Regards,

Laurent Pinchart