[libcamera-devel] [PATCH v2 05/13] libcamera: controls: Generate a vector of enumerated values

[Jacopo Mondi]

Hi Kieran,

On Wed, Oct 21, 2020 at 01:58:17PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 20/10/2020 19:05, 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.
> >
>
> Personally I think this is a good way to handle this data.
>
>
> > 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>
> >
> >  #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 93cb3885c3da..87036fe7dec1 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}Values {''')
> >      enum_value_template = string.Template('''\t${name} = ${value},''')
> > +    enum_list_start = string.Template('''static const std::vector<ControlValue> ${name}List = {''')
>
> 'List' appended to the name feels a bit odd.
>
> Seeing the usage in the next patch,:
>
> &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeList)
> &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeList)
> &controls::AeExposureMode, ControlInfo(controls::AeExposureModeList)
>
> Throwing alternatives in to the bikeshed, because that's all it is, this
> could be 'Values' or 'Enum' on the end.
>

'Values' is taken as you can see a few lines above
I'm not excited by 'Enum' to be honest

What if we rename the value enumeration with 'Value' and the vector
'Values' :

enum ControlXYZValue {
        ControlXYZOne,
        ControlXYZTwo,
        ControlXYZThree,
};
static const std::vector<ControlValue>  ControlXYZValues = {
        static_cast<int32_t>(ControlXYZOne),
        static_cast<int32_t>(ControlXYZTwo),
        static_cast<int32_t>(ControlXYZThree),
};

> To me, List could end up being a list of strings, or floats, or anything.
>
> Values, could also be ... arbitrary numerical values, but matches the
> name in the ControlInfo
>
> Enum ... will be integer values ...
>
>
> Bikeshed colours aside:
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>

Thanks
  j

> > +    enum_list_values = string.Template('''\tstatic_cast<int32_t>(${name}),''')
> >      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("};")
> > +
> >          target_ctrls.append(template.substitute(info))
> >          id_value += 1
> >
> >
>
> --
> Regards
> --
> Kieran