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

[Kieran Bingham]

Hi Jacopo,


On 21/10/2020 14:40, Jacopo Mondi wrote:
> 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' :

Or rename the enum to Enum,

> 
> enum ControlXYZValue {

enum ControlXYZEnum {

?
--
Kieran

>         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

-- 
Regards
--
Kieran