[libcamera-devel] [PATCH v2 02/10] libcamera: controls: Parse 'values' in gen-controls.py
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Dec 9 14:20:16 CET 2019
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.
> 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,
Laurent Pinchart
More information about the libcamera-devel
mailing list