[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:25:51 CET 2019
Hi Laurent,
On 09/12/2019 13:20, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Mon, Dec 09, 2019 at 01:16:08PM +0000, Kieran Bingham wrote:
>> On 09/12/2019 12:54, Laurent Pinchart wrote:
>>> 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:
>>>>> 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.
>
> Yes, it's a typo, sorry.
>
>>> 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.
>
> I've thought about that, but it can be a bit error-prone as inserting a
> new value in the middle may not be caught during review. Furthermore, I
> think having the explicit values in the documentation is useful for
> binary IPAs that don't want to link to libcamera. For those reasons I
> think there's more cons than pros for automatic values.
Good, we're in agreement. That was my point! :-)
We need to ensure the numbers are consistent.
--
Kieran
>
>> Otherwise, +1 on the YAML style Laurent has posted. It seems concise and
>> fills the requirements I can see.
>>
>>>>> 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