[libcamera-devel] [PATCH v4 04/37] utils: ipc: add templates for code generation for IPC mechanism
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Nov 26 17:00:47 CET 2020
Hi Paul,
On Mon, Nov 23, 2020 at 05:11:56PM +0900, paul.elder at ideasonboard.com wrote:
> On Thu, Nov 19, 2020 at 02:39:17PM +0200, Laurent Pinchart wrote:
> > On Fri, Nov 06, 2020 at 07:36:34PM +0900, Paul Elder wrote:
> > > Add templates to mojo to generate code for the IPC mechanism. These
> > > templates generate:
> > > - module header
> > > - module serializer
> > > - IPA proxy cpp, header, and worker
> > >
> > > Given an input data definition mojom file for a pipeline.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > Changes in v4:
> > > For the non-template files:
> > > - rename IPA{pipeline_name}CallbackInterface to
> > > IPA{pipeline_name}EventInterface
> > > - to avoid the notion of "callback" and emphasize that it's an event
> > > - add support for strings in custom structs
> > > - add validation, that async methods must not have return values
> > > - it throws exception and isn't very clear though...?
> > > - rename controls to libcamera::{pipeline_name}::controls (controls is
> > > now lowercase)
> > > - rename {pipeline_name}_generated.h to {pipeline_name}_ipa_interface.h,
> > > and {pipeline_name}_serializer.h to {pipeline_name}_ipa_serializer.h
> > > - same for their corresponding template files
> > > For the template files:
> > > - fix spacing (now it's all {{var}} instead of some {{ var }})
> > > - except if it's code, so code is still {{ code }}
> > > - move inclusion of corresponding header to first in the inclusion list
> > > - fix copy&paste errors
> > > - change snake_case to camelCase in the generated code
> > > - template code still uses snake_case
> > > - change the generated command enums to an enum class, and make it
> > > capitalized (instead of allcaps)
> > > - add length checks to recvIPC (in proxy)
> > > - fix some template spacing
> > > - don't use const for PODs in function/signal parameters
> > > - add the proper length checks to readPOD/appendPOD
> > > - the helper functions for reading and writing PODs to and from
> > > serialized data
> > > - rename readUInt/appendUInt to readPOD/appendPOD
> > > - add support for strings in custom structs
> > >
> > > Changes in v3:
> > > - add support for namespaces
> > > - fix enum assignment (used to have +1 for CMD applied to all enums)
> > > - use readHeader, writeHeader, and eraseHeader as static class functions
> > > of IPAIPCUnixSocket (in the proxy worker)
> > > - add requirement that base controls *must* be defined in
> > > libcamera::{pipeline_name}::Controls
> > >
> > > Changes in v2:
> > > - mandate the main and callback interfaces, and init(), start(), stop()
> > > and their parameters
> > > - fix returning single pod value from IPC-called function
> > > - add licenses
> > > - improve auto-generated message
> > > - other fixes related to serdes
> > > ---
> > > .../module_ipa_interface.h.tmpl | 113 ++++
> > > .../module_ipa_proxy.cpp.tmpl | 238 +++++++++
> > > .../module_ipa_proxy.h.tmpl | 118 +++++
> > > .../module_ipa_proxy_worker.cpp.tmpl | 187 +++++++
> > > .../module_ipa_serializer.h.tmpl | 44 ++
> > > .../libcamera_templates/proxy_functions.tmpl | 205 ++++++++
> > > .../libcamera_templates/serializer.tmpl | 280 ++++++++++
> > > .../generators/mojom_libcamera_generator.py | 488 ++++++++++++++++++
> > > 8 files changed, 1673 insertions(+)
> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_interface.h.tmpl
> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_serializer.h.tmpl
> > > create mode 100644 utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > create mode 100644 utils/ipc/generators/libcamera_templates/serializer.tmpl
> > > create mode 100644 utils/ipc/generators/mojom_libcamera_generator.py
> >
> > Let's start with the generator.
> >
> > [snip]
> >
> > > diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py
> > > new file mode 100644
> > > index 00000000..c4fbf9b3
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/mojom_libcamera_generator.py
> > > @@ -0,0 +1,488 @@
> >
> > Missing SPDX header and copyright notice.
> >
> > > +'''Generates libcamera files from a mojom.Module.'''
> >
> > Comments in Python use # :-)
> >
> > > +
> > > +import argparse
> > > +import ast
> > > +import datetime
> > > +import contextlib
> > > +import os
> > > +import re
> > > +import shutil
> > > +import sys
> > > +import tempfile
> >
> > ast, contextlib, shutil, sys and tempfile don't seem to be used.
> >
> > > +
> > > +from jinja2 import contextfilter
> >
> > contextfilter isn't used either.
> >
> > > +
> > > +import mojom.fileutil as fileutil
> > > +import mojom.generate.generator as generator
> > > +import mojom.generate.module as mojom
> > > +from mojom.generate.template_expander import UseJinja
> > > +
> > > +
> > > +GENERATOR_PREFIX = 'libcamera'
> > > +
> > > +_kind_to_cpp_type = {
> > > + mojom.BOOL: 'bool',
> > > + mojom.INT8: 'int8_t',
> > > + mojom.UINT8: 'uint8_t',
> > > + mojom.INT16: 'int16_t',
> > > + mojom.UINT16: 'uint16_t',
> > > + mojom.INT32: 'int32_t',
> > > + mojom.UINT32: 'uint32_t',
> > > + mojom.FLOAT: 'float',
> > > + mojom.INT64: 'int64_t',
> > > + mojom.UINT64: 'uint64_t',
> > > + mojom.DOUBLE: 'double',
> > > +}
> > > +
> > > +_bit_widths = {
> > > + mojom.BOOL: '8',
> > > + mojom.INT8: '8',
> > > + mojom.UINT8: '8',
> > > + mojom.INT16: '16',
> > > + mojom.UINT16: '16',
> > > + mojom.INT32: '32',
> > > + mojom.UINT32: '32',
> > > + mojom.FLOAT: '32',
> > > + mojom.INT64: '64',
> > > + mojom.UINT64: '64',
> > > + mojom.DOUBLE: '64',
> > > +}
> > > +
> > > +def ModuleName(path):
> > > + return path.split('/')[-1].split('.')[0]
> > > +
> > > +def ModuleClassName(module):
> > > + s = re.sub(r'^IPA', '', module.interfaces[0].mojom_name)
> > > + return re.sub(r'Interface$', '', s)
> >
> > You could combine the two with
> >
> > return re.sub(r'^IPA(.*)Interface$', lambda match: match.group(1), module)
> >
> > and possibly optimize it further by pre-compiling the regular
> > expression. I'll let you decide if it's worth it.
> >
> > > +
> > > +def UpperCamelCase(name):
> > > + return ''.join([x.capitalize() for x in generator.SplitCamelCase(name)])
> > > +
> > > +def CamelCase(name):
> > > + uccc = UpperCamelCase(name)
> > > + return uccc[0].lower() + uccc[1:]
> >
> > These two functions don't seem to be used.
> >
> > > +
> > > +def Capitalize(name):
> > > + return name[0].upper() + name[1:]
> > > +
> > > +def ConstantStyle(name):
> > > + return generator.ToUpperSnakeCase(name)
> > > +
> > > +def Choose(cond, t, f):
> > > + return t if cond else f
> > > +
> > > +def CommaSep(l):
> > > + return ', '.join([m for m in l])
> > > +
> > > +def ParamsCommaSep(l):
> > > + return ', '.join([m.mojom_name for m in l])
> > > +
> > > +def GetDefaultValue(element):
> > > + if element.default is not None:
> > > + return element.default
> > > + if type(element.kind) == mojom.Kind:
> > > + return '0'
> > > + if mojom.IsEnumKind(element.kind):
> > > + return f'static_cast<{element.kind.mojom_name}>(0)'
> > > + if (isinstance(element.kind, mojom.Struct) and
> > > + element.kind.mojom_name == 'FileDescriptor'):
> >
> > Is there a need for the outer parentheses ? Or is this dictated by the
> > mojom coding style ? Same comment for several locations below.
> >
> > > + return '-1'
> > > + return ''
> > > +
> > > +def WithDefaultValues(element):
> > > + return [x for x in element if HasDefaultValue(x)]
> > > +
> > > +def WithFds(element):
> > > + return [x for x in element if HasFd(x)]
> >
> > Should these be moved a little bit further down, after the functions
> > they call ? The code works fine, but it would create an easier to read
> > flow.
> >
> > > +
> > > +def HasDefaultValue(element):
> > > + return GetDefaultValue(element) != ''
> > > +
> > > +def HasDefaultFields(element):
> > > + return True in [HasDefaultValue(x) for x in element.fields]
> > > +
> > > +def GetAllTypes(element):
> > > + if mojom.IsArrayKind(element):
> > > + return GetAllTypes(element.kind)
> > > + if mojom.IsMapKind(element):
> > > + return GetAllTypes(element.key_kind) + GetAllTypes(element.value_kind)
> > > + if isinstance(element, mojom.Parameter):
> > > + return GetAllTypes(element.kind)
> > > + if mojom.IsEnumKind(element):
> > > + return [element.mojom_name]
> > > + if not mojom.IsStructKind(element):
> > > + return [element.spec]
> > > + if len(element.fields) == 0:
> > > + return [element.mojom_name]
> > > + ret = [GetAllTypes(x.kind) for x in element.fields]
> > > + ret = [x for sublist in ret for x in sublist]
> > > + return list(set(ret))
> > > +
> > > +def GetAllAttrs(element):
> > > + if mojom.IsArrayKind(element):
> > > + return GetAllAttrs(element.kind)
> > > + if mojom.IsMapKind(element):
> > > + return {**GetAllAttrs(element.key_kind), **GetAllAttrs(element.value_kind)}
> > > + if isinstance(element, mojom.Parameter):
> > > + return GetAllAttrs(element.kind)
> > > + if mojom.IsEnumKind(element):
> > > + return element.attributes if element.attributes is not None else {}
> > > + if mojom.IsStructKind(element) and len(element.fields) == 0:
> > > + return element.attributes if element.attributes is not None else {}
> > > + if not mojom.IsStructKind(element):
> > > + return {}
> > > + attrs = [(x.attributes) for x in element.fields]
> > > + ret = {}
> > > + for d in attrs:
> > > + if d is not None:
> > > + ret = {**ret, **d}
> >
> > Maybe
> >
> > ret.update(d)
> >
> > > + return ret
> > > +
> > > +def NeedsControlSerializer(element):
> > > + types = GetAllTypes(element)
> > > + return "ControlList" in types or "ControlInfoMap" in types
> > > +
> > > +def HasFd(element):
> > > + attrs = GetAllAttrs(element)
> > > + if isinstance(element, mojom.Kind):
> > > + types = GetAllTypes(element)
> > > + else:
> > > + types = GetAllTypes(element.kind)
> > > + return "FileDescriptor" in types or (attrs is not None and "hasFd" in attrs)
> > > +
> > > +def MethodInputHasFd(method):
> > > + if len([x for x in method.parameters if HasFd(x)]) > 0:
> >
> > You can drop the len() call, [] evaluates to False.
>
> What do you mean? It evaluates to something like this:
>
> [<mojom.generate.module.Parameter object at 0x7f232bdbb280>]
>
> So it checks how many parameters have fd.
My bad. I thought python would make it a list, not a generator, and
empty lists evaluate to False. Please ignore this comment.
It would be nice if python had a syntax similar to
if one of HasFd(x) for x in method.parameters:
:-)
> > > + return True
> > > + return False
> > > +
> > > +def MethodOutputHasFd(method):
> > > + if (MethodReturnValue(method) != 'void' or
> > > + method.response_parameters is None):
> > > + return False
> > > + if len([x for x in method.parameters if HasFd(x)]) > 0:
> >
> > Same here.
> >
> > Shouldn't it be method.response_parameters instead of method.parameters
> > ?
>
> Yeah it should be.
>
> > > + return True
> > > + return False
> > > +
> > > +def MethodParamInputs(method):
> > > + return method.parameters
> > > +
> > > +def MethodParamOutputs(method):
> > > + if (MethodReturnValue(method) != 'void' or
> > > + method.response_parameters is None):
> > > + return []
> > > + return method.response_parameters
> >
> > The increase code reuse, you could implement these functions as follows.
> >
> >
> > def MethodParamInputs(method):
> > return method.parameters
> >
> > def MethodParamOutputs(method):
> > if (MethodReturnValue(method) != 'void' or
> > method.response_parameters is None):
> > return []
> > return method.response_parameters
> >
> > def MethodParamHasFd(parameters):
> > if [x for x in parameters if HasFd(x)]:
> > return True
> > return False
> >
> > def MethodInputHasFd(method):
> > return MethodParamHasFd(MethodParamInputs(method))
> >
> > def MethodOutputHasFd(method):
> > return MethodParamHasFd(MethodParamOutputs(method))
> >
> > > +
> > > +def MethodParamNames(method):
> > > + params = []
> > > + for param in method.parameters:
> > > + params.append(param.mojom_name)
> > > + if MethodReturnValue(method) == 'void':
> > > + if method.response_parameters is None:
> > > + return params
> > > + for param in method.response_parameters:
> > > + params.append(param.mojom_name)
> > > + return params
> > > +
> > > +def MethodParameters(method):
> > > + params = []
> > > + for param in method.parameters:
> > > + params.append('const %s %s%s' % (GetNameForElement(param),
> > > + ('&' if not IsPod(param) else ''),
> >
> > No need for the outer parentheses.
> >
> > > + param.mojom_name))
> > > + if MethodReturnValue(method) == 'void':
> > > + if method.response_parameters is None:
> > > + return params
> > > + for param in method.response_parameters:
> > > + params.append(f'{GetNameForElement(param)} *{param.mojom_name}')
> > > + return params
> > > +
> > > +def MethodReturnValue(method):
> > > + if method.response_parameters is None:
> > > + return 'void'
> > > + if len(method.response_parameters) == 1 and IsPod(method.response_parameters[0]):
> > > + return GetNameForElement(method.response_parameters[0])
> > > + return 'void'
> > > +
> > > +def IsAsync(method):
> > > + # callbacks are always async
> >
> > s/callbacks/Events/ ?
> >
> > > + if re.match("^IPA.*EventInterface$", method.interface.mojom_name):
> > > + return True
> > > + elif re.match("^IPA.*Interface$", method.interface.mojom_name):
> > > + if method.attributes is None:
> > > + return False
> > > + elif 'async' in method.attributes and method.attributes['async']:
> > > + return True
> > > + return False
> > > +
> > > +def IsArray(element):
> > > + return mojom.IsArrayKind(element.kind)
> > > +
> > > +def IsControls(element):
> > > + return mojom.IsStructKind(element.kind) and (element.kind.mojom_name == "ControlList" or
> > > + element.kind.mojom_name == "ControlInfoMap")
> > > +
> > > +def IsEnum(element):
> > > + return mojom.IsEnumKind(element.kind)
> > > +
> > > +def IsFd(element):
> > > + return mojom.IsStructKind(element.kind) and element.kind.mojom_name == "FileDescriptor"
> > > +
> > > +def IsMap(element):
> > > + return mojom.IsMapKind(element.kind)
> > > +
> > > +def IsPlainStruct(element):
> > > + return mojom.IsStructKind(element.kind) and not IsControls(element) and not IsFd(element)
> > > +
> > > +def IsPod(element):
> > > + return element.kind in _kind_to_cpp_type
> > > +
> > > +def IsStr(element):
> > > + return element.kind.spec == 's'
> > > +
> > > +def BitWidth(element):
> > > + if element.kind in _bit_widths:
> > > + return _bit_widths[element.kind]
> > > + if mojom.IsEnumKind(element.kind):
> > > + return '32'
> > > + return ''
> > > +
> > > +def GetNameForElement(element):
> > > + if (mojom.IsEnumKind(element) or
> > > + mojom.IsInterfaceKind(element) or
> > > + mojom.IsStructKind(element)):
> > > + return element.mojom_name
> > > + if (mojom.IsArrayKind(element)):
> > > + elem_name = GetNameForElement(element.kind)
> > > + return f'std::vector<{elem_name}>'
> > > + if (mojom.IsMapKind(element)):
> > > + key_name = GetNameForElement(element.key_kind)
> > > + value_name = GetNameForElement(element.value_kind)
> > > + return f'std::map<{key_name}, {value_name}>'
> > > + if isinstance(element, (mojom.Field, mojom.Method, mojom.Parameter)):
> > > + if (mojom.IsArrayKind(element.kind) or mojom.IsMapKind(element.kind)):
> > > + return GetNameForElement(element.kind)
> > > + if (mojom.IsReferenceKind(element.kind) and element.kind.spec == 's'):
> > > + return 'std::string'
> > > + if (hasattr(element, 'kind') and element.kind in _kind_to_cpp_type):
> >
> > Is the hasattr() check needed ? If element has no kind attribute, I
> > would expect the previous two if's to raise an exception.
>
> I've added comments for each section, because this confuses even me...
>
> So the upper-level if is to catch struct fields and function parameters.
> This if catches PODs.
>
> And you're right, the hasattr() check isn't needed. I think it was a
> remnant of fiddling with it and python complaining that element has no
> attribute kind.
>
> > > + return _kind_to_cpp_type[element.kind]
> > > + return element.kind.mojom_name
> > > + if isinstance(element, mojom.EnumValue):
> >
> > Extra space after comma.
> >
> > > + return (GetNameForElement(element.enum) + '.' +
> > > + ConstantStyle(element.name))
> > > + if isinstance(element, (mojom.NamedValue,
> > > + mojom.Constant,
> > > + mojom.EnumField)):
> > > + return ConstantStyle(element.name)
> > > + if (hasattr(element, '__hash__') and element in _kind_to_cpp_type):
> > > + return _kind_to_cpp_type[element]
> >
> > For my own education, what type of elements does this cover ?
>
> This covers PODs that are members of vectors/maps.
>
> I've tested each case, and removed the ones that I can't remember what
> they do.
Thank you, that will make the code easier to maintain.
> > > + if (hasattr(element, 'kind') and element.kind in _kind_to_cpp_type):
> > > + return _kind_to_cpp_type[element.kind]
> > > + if (hasattr(element, 'spec')):
> > > + if (element.spec == 's'):
> > > + return 'std::string'
> > > + return element.spec.split(':')[-1]
> > > + if (mojom.IsInterfaceRequestKind(element) or
> > > + mojom.IsAssociatedKind(element) or
> > > + mojom.IsPendingRemoteKind(element) or
> > > + mojom.IsPendingReceiverKind(element) or
> > > + mojom.IsUnionKind(element)):
> > > + raise Exception('Unsupported element: %s' % element)
> > > + raise Exception('Unexpected element: %s' % element)
> > > +
> > > +def GetFullNameForElement(element, namespace_str):
> > > + name = GetNameForElement(element)
> > > + if namespace_str == '':
> > > + return name
> > > + return f'{namespace_str}::{name}'
> > > +
> > > +def ValidateZeroLength(l, s, cap=True):
> > > + if l is None:
> > > + return
> > > + if len(l) > 0:
> > > + raise Exception(f'{s.capitalize() if cap else s} should be empty')
> > > +
> > > +def ValidateSingleLength(l, s, cap=True):
> > > + if len(l) > 1:
> > > + raise Exception(f'Only one {s} allowed')
> > > + if len(l) < 1:
> > > + raise Exception(f'{s.capitalize() if cap else s} is required')
> > > +
> > > +def ValidateInterfaces(interfaces):
> > > + # Validate presence of main interface
> > > + intf = [x for x in interfaces
> > > + if re.match("^IPA.*Interface", x.mojom_name) and
> > > + not re.match("^IPA.*EventInterface", x.mojom_name)]
> > > + ValidateSingleLength(intf, 'main interface')
> > > + intf = intf[0]
> > > +
> > > + # Validate presence of callback interface
> >
> > s/callback/event/
> >
> > > + cb = [x for x in interfaces if re.match("^IPA.*EventInterface", x.mojom_name)]
> >
> > s/cb/event/ ?
> >
> > > + ValidateSingleLength(cb, 'event interface')
> > > + cb = cb[0]
> > > +
> > > + # Validate required main interface functions
> > > + f_init = [x for x in intf.methods if x.mojom_name == 'init']
> > > + f_start = [x for x in intf.methods if x.mojom_name == 'start']
> > > + f_stop = [x for x in intf.methods if x.mojom_name == 'stop']
> > > +
> > > + ValidateSingleLength(f_init, 'init()', False)
> > > + ValidateSingleLength(f_start, 'start()', False)
> > > + ValidateSingleLength(f_stop, 'stop()', False)
> > > +
> > > + f_init = f_init[0]
> > > + f_start = f_start[0]
> > > + f_stop = f_stop[0]
> > > +
> > > + # Validate parameters to init()
> > > + ValidateSingleLength(f_init.parameters, 'input parameter to init()')
> > > + ValidateSingleLength(f_init.response_parameters, 'output parameter from init()')
> > > + if f_init.parameters[0].kind.mojom_name != 'IPASettings':
> > > + raise Exception('init() must have single IPASettings input parameter')
> > > + if f_init.response_parameters[0].kind.spec != 'i32':
> > > + raise Exception('init() must have single int32 output parameter')
> > > +
> > > + # Validate parameters to start()
> > > + ValidateZeroLength(f_start.parameters, 'input parameter to start()')
> > > + ValidateSingleLength(f_start.response_parameters, 'output parameter from start()')
> > > + if f_start.response_parameters[0].kind.spec != 'i32':
> > > + raise Exception('start() must have single int32 output parameter')
> > > +
> > > + # Validate parameters to stop()
> > > + ValidateZeroLength(f_stop.parameters, 'input parameter to stop()')
> > > + ValidateZeroLength(f_stop.parameters, 'output parameter from stop()')
> > > +
> > > + # Validate that all async methods don't have return values
> > > + intf_methods_async = [x for x in intf.methods if IsAsync(x)]
> > > + for method in intf_methods_async:
> > > + ValidateZeroLength(method.response_parameters,
> > > + f'{method.mojom_name} response parameters', False)
> > > +
> > > + cb_methods_async = [x for x in cb.methods if IsAsync(x)]
> >
> > s/cb_method_async/event_methods_async/
> >
> > > + for method in cb_methods_async:
> > > + ValidateZeroLength(method.response_parameters,
> > > + f'{method.mojom_name} response parameters', False)
> > > +
> > > +class Generator(generator.Generator):
> > > +
> > > + @staticmethod
> > > + def GetTemplatePrefix():
> > > + return 'libcamera_templates'
> > > +
> > > + def GetFilters(self):
> > > + libcamera_filters = {
> > > + 'all_types': GetAllTypes,
> > > + 'bit_width': BitWidth,
> > > + 'cap': Capitalize,
> >
> > 'cap': str.capitalize,
> >
> > and drop the custom Capitalize function.
>
> The built-in capitalize function doesn't preserve camel-casing so it's
> not sufficient:
>
> >>> "camelCaseClass".capitalize()
> 'Camelcaseclass'
> >>> Capitalize("camelCaseClass")
> 'CamelCaseClass'
Oops.
> > > + 'choose': Choose,
> > > + 'comma_sep': CommaSep,
> > > + 'default_value': GetDefaultValue,
> > > + 'has_default_fields': HasDefaultFields,
> > > + 'has_fd': HasFd,
> > > + 'is_async': IsAsync,
> > > + 'is_array': IsArray,
> > > + 'is_controls': IsControls,
> > > + 'is_enum': IsEnum,
> > > + 'is_fd': IsFd,
> > > + 'is_map': IsMap,
> > > + 'is_plain_struct': IsPlainStruct,
> > > + 'is_pod': IsPod,
> > > + 'is_str': IsStr,
> > > + 'method_input_has_fd': MethodInputHasFd,
> > > + 'method_output_has_fd': MethodOutputHasFd,
> > > + 'method_param_names': MethodParamNames,
> > > + 'method_param_inputs': MethodParamInputs,
> > > + 'method_param_outputs': MethodParamOutputs,
> > > + 'method_parameters': MethodParameters,
> > > + 'method_return_value': MethodReturnValue,
> > > + 'name': GetNameForElement,
> > > + 'name_full': GetFullNameForElement,
> > > + 'needs_control_serializer': NeedsControlSerializer,
> > > + 'params_comma_sep': ParamsCommaSep,
> > > + 'with_default_values': WithDefaultValues,
> > > + 'with_fds': WithFds,
> > > + }
> > > + return libcamera_filters
> > > +
> > > + def _GetJinjaExports(self):
> >
> > Would it make sense to store the return value of
> > ModuleClassName(self.module) in a local variable instead of calling the
> > function several times below ? If self.module is constant during the
> > lifetime of the Generator (I think it is), you could even compute it in
> > the constructor and store it in a member variable.
>
> Yeah I did think that was a good idea.
>
> Hm, it doesn't work in the constructor, but it works in GenerateFiles.
>
> > > + return {
> > > + 'base_controls': '%s::controls' % ModuleClassName(self.module),
> > > + 'cmd_enum_name': '_%sCmd' % ModuleClassName(self.module),
> > > + 'cmd_event_enum_name': '_%sEventCmd' % ModuleClassName(self.module),
> > > + 'enums': self.module.enums,
> > > + 'has_array': len([x for x in self.module.kinds.keys() if x[0] == 'a']) > 0,
> > > + 'has_map': len([x for x in self.module.kinds.keys() if x[0] == 'm']) > 0,
> > > + 'has_namespace': self.module.mojom_namespace != '',
> > > + 'imports': self.module.imports,
> >
> > I don't see any mention of 'imports' in the templates. Is this used
> > internally by mojom and/or jinja, or is it unused ?
>
> It was probably copied from the mojom C++ generator. I think I was going
> to use it for something... but then importing ended up working in such a
> way where this is no longer needed.
>
> > > + 'interface_cb': self.module.interfaces[1],
> >
> > interface_event ?
> >
> > > + 'interface_main': self.module.interfaces[0],
> >
> > Is there a guarantee the interfaces will always be specified in this
> > order ? The code in ValidateInterfaces() finds interfaces based on their
> > name.
>
> Ah, there is indeed no such guarantee, just a undocumented convention.
Could we then either avoid relying on this, or document the convention ?
:-)
> > > + 'interface_name': 'IPA%sInterface' % ModuleClassName(self.module),
> > > + 'ipc_name': 'IPAIPCUnixSocket',
> >
> > This seems unused.
>
> It will be used when we support different IPC mechanisms :)
>
> Wait nvm, it won't. I'll remove it.
>
> > > + 'kinds': self.module.kinds,
> > > + 'module': self.module,
> >
> > Same for those two (unless they are used elsewhere than in the
> > templates).
> >
> > > + 'module_name': ModuleName(self.module.path),
> > > + 'module_class_name': ModuleClassName(self.module),
> >
> > Ditto.
> >
> > > + 'namespace': self.module.mojom_namespace.split('.'),
> > > + 'namespace_str': self.module.mojom_namespace.replace('.', '::') if
> > > + self.module.mojom_namespace is not None else '',
> > > + 'proxy_name': 'IPAProxy%s' % ModuleClassName(self.module),
> > > + 'proxy_worker_name': 'IPAProxy%sWorker' % ModuleClassName(self.module),
> >
> > Same here.
> >
> > > + 'structs_nonempty': [x for x in self.module.structs if len(x.fields) > 0],
> > > + 'year': datetime.datetime.now().year,
> > > + }
> > > +
> > > +
> > > + @UseJinja('module_ipa_interface.h.tmpl')
> > > + def _GenerateDataHeader(self):
> > > + return self._GetJinjaExports()
> > > +
> > > + @UseJinja('module_ipa_serializer.h.tmpl')
> > > + def _GenerateSerializer(self):
> > > + return self._GetJinjaExports()
> > > +
> > > + @UseJinja('module_ipa_proxy.cpp.tmpl')
> > > + def _GenerateProxyCpp(self):
> > > + return self._GetJinjaExports()
> > > +
> > > + @UseJinja('module_ipa_proxy.h.tmpl')
> > > + def _GenerateProxyHeader(self):
> > > + return self._GetJinjaExports()
> > > +
> > > + @UseJinja('module_ipa_proxy_worker.cpp.tmpl')
> > > + def _GenerateProxyWorker(self):
> > > + return self._GetJinjaExports()
> > > +
> > > + def GenerateFiles(self, unparsed_args):
> > > + parser = argparse.ArgumentParser()
> > > + parser.add_argument('--libcamera_generate_header', action='store_true')
> > > + parser.add_argument('--libcamera_generate_serializer', action='store_true')
> > > + parser.add_argument('--libcamera_generate_proxy_cpp', action='store_true')
> > > + parser.add_argument('--libcamera_generate_proxy_h', action='store_true')
> > > + parser.add_argument('--libcamera_generate_proxy_worker', action='store_true')
> > > + parser.add_argument('--libcamera_output_path')
> > > + args = parser.parse_args(unparsed_args)
> > > +
> > > + ValidateInterfaces(self.module.interfaces)
> > > +
> > > + fileutil.EnsureDirectoryExists(os.path.dirname(args.libcamera_output_path))
> > > +
> > > + module_name = ModuleName(self.module.path)
> >
> > This seems unused.
> >
> > > +
> > > + if args.libcamera_generate_header:
> > > + self.Write(self._GenerateDataHeader(),
> > > + args.libcamera_output_path)
> > > +
> > > + if args.libcamera_generate_serializer:
> > > + self.Write(self._GenerateSerializer(),
> > > + args.libcamera_output_path)
> > > +
> > > + if args.libcamera_generate_proxy_cpp:
> > > + self.Write(self._GenerateProxyCpp(),
> > > + args.libcamera_output_path)
> > > +
> > > + if args.libcamera_generate_proxy_h:
> > > + self.Write(self._GenerateProxyHeader(),
> > > + args.libcamera_output_path)
> > > +
> > > + if args.libcamera_generate_proxy_worker:
> > > + self.Write(self._GenerateProxyWorker(),
> > > + args.libcamera_output_path)
> >
> > I'm sure we could avoid the manual unrolling of all this, using a
> > dictionary to store the template files and iterating over it to add
> > arguments to the parser and replacing the above code, but it's probably
> > not worth it as I don't expect this will change a lot.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list