[libcamera-devel] [PATCH v4 04/37] utils: ipc: add templates for code generation for IPC mechanism
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Mon Nov 23 09:11:56 CET 2020
Hi Laurent,
Thank you for the review.
On Thu, Nov 19, 2020 at 02:39:17PM +0200, Laurent Pinchart wrote:
> Hi Paul,
>
> Thank you for the patch.
>
> 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.
> > + 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.
> > + 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'
> > + '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.
> > + '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.
Thanks,
Paul
More information about the libcamera-devel
mailing list