[libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse 'values' in gen-controls.py
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Dec 9 14:16:08 CET 2019
Hi Jacopo, and all,
On 09/12/2019 12:54, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Sun, Dec 08, 2019 at 07:08:00PM +0100, Jacopo Mondi wrote:
>> On Fri, Dec 06, 2019 at 08:09:04PM +0100, Niklas Söderlund wrote:
>>> Hi Jacopo,
>>>
>>> Thanks for your work.
>>>
>>> On 2019-12-05 21:43:42 +0100, Jacopo Mondi wrote:
>>>> In preparation to add libcamera Camera definition by re-using the
>>>
>>> First sentence is hard for me to parse.
>>
>> Indeed.. I've left the subject behind :)
>>
>> In preparation to add libcamera Camera -properties- definition by re-using the
>>
>>>> control generation framework, augment the gen_controls.py script to
>>>> support parsing the 'values' yaml tag and generate documentation and
>>>> definition of possible values associated with a Control or a Property.
>>>>
>>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>>>> ---
>>>> src/libcamera/gen-controls.py | 34 ++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 34 insertions(+)
>>>>
>>>> diff --git a/src/libcamera/gen-controls.py b/src/libcamera/gen-controls.py
>>>> index 940386cc68c8..f86f01f6759b 100755
>>>> --- a/src/libcamera/gen-controls.py
>>>> +++ b/src/libcamera/gen-controls.py
>>>> @@ -17,10 +17,15 @@ def snake_case(s):
>>>>
>>>>
>>>> def generate_cpp(controls):
>>>> + value_doc_template = string.Template('''/**
>>>> + * \\def ${name}
>>>> + * \\brief ${description} */''')
>>>> +
>>>> doc_template = string.Template('''/**
>>>> * \\var extern const Control<${type}> ${name}
>>>> ${description}
>>>> */''')
>>>> +
>>>> def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, "${name}");')
>>>>
>>>> ctrls_doc = []
>>>> @@ -35,13 +40,27 @@ ${description}
>>>> description[0] = '\\brief ' + description[0]
>>>> description = '\n'.join([(line and ' * ' or ' *') + line for line in description])
>>>>
>>>> + try:
>>>> + values = ctrl['values']
>>>> + except KeyError:
>>>> + values = ""
>>>> +
>>>> info = {
>>>> 'name': name,
>>>> 'type': ctrl['type'],
>>>> 'description': description,
>>>> 'id_name': id_name,
>>>> + 'values' : values,
>>>> }
>>>>
>>>> + for value in values:
>>>> + value_info = {
>>>> + 'name': list(value.keys())[0],
>>>> + 'value': value['value'],
>>>> + 'description': value['description']
>>>> + }
>>>> + ctrls_doc.append(value_doc_template.substitute(value_info))
>>>> +
>>>> ctrls_doc.append(doc_template.substitute(info))
>>>> ctrls_def.append(def_template.substitute(info))
>>>> ctrls_map.append('\t{ ' + id_name + ', &' + name + ' },')
>>>> @@ -54,6 +73,7 @@ ${description}
>>>>
>>>>
>>>> def generate_h(controls):
>>>> + value_template = string.Template('''#define ${name} ${value}''')
>>>
>>> Defines are probably the way we want to play this but the notion of
>>> using enums jumped into my head, is that something you think could be
>>> used or is it more trouble then it's worth?
>>
>> I admit I have considered enums as well... I think defines are fine
>> for now, but we can add support for enums on top if we think it's
>> better. The only thing I would consider from the very beginning is how
>> to do so.
>>
>> I would keep the 'values' tag for defines and add an 'enum' tag which
>> might look like this
>>
>> controls:
>> - CONTROL_NAME:
>> type: int32_t
>> desription: -|
>> blah blah
>> enum:
>> - NAME_OF_THE_ENUM:
>> values:
>> - FIRST_ENUM_ELEMENT:
>> value: x
>> description: -|
>> blah blah
>> - SECOND_ENUM_ELEMENT:
>> value: x
>> description: -|
>> blah blah
>>
>> The only reason why we should think how enum definition should look
>> like is to make it plays nice with the current use of values, which,
>> for reference is something like
>>
>> controls:
>> - CONTROL_NAME:
>> type: int32_t
>> desription: -|
>> blah blah
>> values:
>> - NAME_OF_THE_DEFINE:
>> value: x
>> description: -|
>> blah blah
>>
>> I'm not sure I like it.
>>
>> I would rather introduce a 'values' section, which could contains
>> (alternatively) a set of definition or an enumeration
>>
>> controls:
>> - CONTROL_NAME:
>> type: int32_t
>> desription: -|
>> blah blah
>> values:
>> defines:
>> - NAME_OF_THE_DEFINE:
>> value: x
>> description: -|
>> blah blah
>> - NAME_OF_ANOHER_DEFINE:
>> value: y
>> description: -|
>> blah blah
>>
>> Or
>>
>> controls:
>> - CONTROL_NAME:
>> type: int32_t
>> desription: -|
>> blah blah
>> values:
>> enum:
>> - NAME_OF_THE_ENUM:
>> values:
>> - FIRST_ENUM_ELEMENT:
>> value: x
>> description: -|
>> blah blah
>> - SECOND_ENUM_ELEMENT:
>> value: y
>> description: -|
>> blah blah
>>
>> So that the only part that changes is the content of the 'values'
>> section.
>>
>> Congratulation, I've just proposed myself more work
>>
>> What do you think, should we consider enum from the beginning, is this
>> an overkill ?
>
> I think we should only have enums, not defines, and I think it shouldn't
> be controlled explicitly from yaml. The NAME_OF_THE_ENUM isn't needed,
> you can name it enum ControlNameValues.
>
> ------- control_ids.yaml ------
> controls:
> - WhiteBalanceMode:
> type: int32_t
> desription: -|
> blah blah
> enum:
> - WhiteBalanceModeAuto:
> value: 0
> description: -|
> blah blah
> - WhiteBalanceModeSunny:
> value: 1
> description: -|
> blah blah
> - WhiteBalanceModeCloudy:
> value: 1
I suspect you mean value: 2 here, but I think that's just a typo in the
psuedo code.
> description: -|
> blah blah
> -------------------------------
>
> should generate
>
> -------- control_ids.h --------
> namespace controls {
>
> enum WhiteBalanceModeValues {
> WhiteBalanceModeAuto = 0,
> WhiteBalanceModeSunny = 1,
> WhiteBalanceModeCloudy = 2,
> ...
> };
>
> extern const Control<int32_t> WhiteBalanceMode;
>
> }
> -------------------------------
Would we support automatic values in enums?
I prefer using enums here over defines, as it scopes the values and will
give the compiler/debug-info more information as to their context too,
which means we will be able to perform more context validation (such as
ensuring all enum values are correctly identified within a switch)
Once defined, the numbers become part of the ABI, so they'd have to be
constant ... So we probably / possibly need to either document or define
some rule that any new additions are only appended, and not added in any
sort-order.
Otherwise, +1 on the YAML style Laurent has posted. It seems concise and
fills the requirements I can see.
--
Kieran
>>> With or without enums and but with an updated commit message,
>>>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>>>
>>>> template = string.Template('''extern const Control<${type}> ${name};''')
>>>>
>>>> ctrls = []
>>>> @@ -66,11 +86,25 @@ def generate_h(controls):
>>>>
>>>> ids.append('\t' + id_name + ' = ' + str(id_value) + ',')
>>>>
>>>> + try:
>>>> + values = ctrl['values']
>>>> + except KeyError:
>>>> + values = ""
>>>> +
>>>> info = {
>>>> 'name': name,
>>>> 'type': ctrl['type'],
>>>> + 'values' : values,
>>>> }
>>>>
>>>> + for value in values:
>>>> + value_info = {
>>>> + 'name': list(value.keys())[0],
>>>> + 'value': value['value'],
>>>> + 'description': value['description']
>>>> + }
>>>> + ctrls.append(value_template.substitute(value_info))
>>>> +
>>>> ctrls.append(template.substitute(info))
>>>> id_value += 1
>>>>
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list