[libcamera-devel] [PATCH v5 4/9] utils: ipc: Add support for Flags
Jacopo Mondi
jacopo at jmondi.org
Wed Oct 12 09:32:16 CEST 2022
Not sure I fully get why this patch does two things (scoped enums +
flags) , but I guess it's fine
On Tue, Oct 11, 2022 at 07:58:54PM +0900, Paul Elder via libcamera-devel wrote:
> Add Flags<E> as a supported type in the IPA interface.
>
> It is used in mojom with the [flags] attribute. Any field or parameter
> type E that is prefixed with the [flags] attribute will direct the code
> generator to generate the type name "Flags<E>" and appropriate
> serialization/deserialization code for Flags<E> instead of for E.
>
> It is usable and has been tested in struct members, function input and
> output parameters, and Signal parameters. This does not add support for
> returning Flags as direct return values.
>
> Additionally, the [scopedEnum] attribute can be used on enum
> definitions, which will instruct the code generator to convert it to an
> enum class instead of a raw enum.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> ---
> Changes in v5:
> - fix serialization of scoped enums that are struct members
> - IsScoped doesn't catch enums that are struct members; only the enum
> definition itself, so we have a separate IsEnumScoped now for struct
> members
>
> Changes in v4:
> - make flags also passed directly (as opposed to const references)
>
> Changes in v3.1:
> - add documentation of [scopedEnum] and [flags] to core.mojom
> - it's not the best place, but better somewhere than nowhere for now
>
> Changes in v3:
> - change flags attribute from [Flags] to [flags]
> - make it so that enum input parameters are passed directly (as opposed
> to const references)
> - to turn enum definitions in mojom to enum classes, use the
> [scopedEnum] attribute (as opposed to the [Flags] attribute in v2)
> - correspondingly, the function in the mojom generator python script
> is separated out from IsFlags() into IsScoped()
> - clean up IsFlags()
> - simplify GetFullNameForElement() for flags
>
> Question for reviewers (in v2): Should we use a different attribute name to
> specify that an enum should be an enum class?
>
> Answer (in v3): Yes; that attribute name is [scopedEnum]
>
> Changes in v2:
> - Use mojom attribute to specify that a field should be Flags<E>,
> instead of using a magic type name format
> ---
> include/libcamera/ipa/core.mojom | 9 +++++
> include/libcamera/ipa/ipa_interface.h | 1 +
> .../definition_functions.tmpl | 2 +-
> .../module_ipa_interface.h.tmpl | 2 +-
> .../module_ipa_proxy.h.tmpl | 2 +-
> .../libcamera_templates/proxy_functions.tmpl | 10 +++--
> .../libcamera_templates/serializer.tmpl | 6 +++
> .../generators/mojom_libcamera_generator.py | 37 +++++++++++++++++--
> 8 files changed, 60 insertions(+), 9 deletions(-)
>
> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> index 74f3339e..ef28ff2d 100644
> --- a/include/libcamera/ipa/core.mojom
> +++ b/include/libcamera/ipa/core.mojom
> @@ -33,6 +33,15 @@ module libcamera;
> * available for the type and there's no need to generate one
> * - hasFd - struct fields or empty structs only
> * - Designate that this field or empty struct contains a SharedFD
> + * - scopedEnum - enum definitions
> + * - Designate that this enum should be an enum class, as opposed to a pure
> + * enum
> + * - flags - struct fields or function parameters that are enums
> + * - Designate that this enum type E should be Flags<E> in the generated C++
> + * code
> + * - For example, if a struct field is defined as `[flags] ErrorFlag f;`
> + * (where ErrorFlag is defined as an enum elsewhere in mojom), then the
> + * generated code for this field will be `Flags<ErrorFlag> f`
> *
> * Rules:
> * - If the type is defined in a libcamera C++ header *and* a (de)serializer is
> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> index 50ca0e7b..8afcfe21 100644
> --- a/include/libcamera/ipa/ipa_interface.h
> +++ b/include/libcamera/ipa/ipa_interface.h
> @@ -13,6 +13,7 @@
> #include <map>
> #include <vector>
>
> +#include <libcamera/base/flags.h>
> #include <libcamera/base/signal.h>
>
> #include <libcamera/controls.h>
> diff --git a/utils/ipc/generators/libcamera_templates/definition_functions.tmpl b/utils/ipc/generators/libcamera_templates/definition_functions.tmpl
> index 94bb4918..8b8509f3 100644
> --- a/utils/ipc/generators/libcamera_templates/definition_functions.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/definition_functions.tmpl
> @@ -9,7 +9,7 @@
> # \param enum Enum object whose definition is to be generated
> #}
> {%- macro define_enum(enum) -%}
> -enum {{enum.mojom_name}} {
> +enum{{" class" if enum|is_scoped}} {{enum.mojom_name}} {
> {%- for field in enum.fields %}
> {{field.mojom_name}} = {{field.numeric_value}},
> {%- endfor %}
> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_interface.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_interface.h.tmpl
> index 415ec283..160601f7 100644
> --- a/utils/ipc/generators/libcamera_templates/module_ipa_interface.h.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_interface.h.tmpl
> @@ -69,7 +69,7 @@ public:
> {%- for method in interface_event.methods %}
> Signal<
> {%- for param in method.parameters -%}
> - {{"const " if not param|is_pod}}{{param|name}}{{" &" if not param|is_pod}}
> + {{"const " if not param|is_pod}}{{param|name}}{{" &" if not param|is_pod and not param|is_enum}}
> {{- ", " if not loop.last}}
> {%- endfor -%}
> > {{method.mojom_name}};
> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> index c308dd10..ed270f5c 100644
> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> @@ -46,7 +46,7 @@ public:
> {%- for method in interface_event.methods %}
> Signal<
> {%- for param in method.parameters -%}
> - {{"const " if not param|is_pod}}{{param|name}}{{" &" if not param|is_pod}}
> + {{"const " if not param|is_pod}}{{param|name}}{{" &" if not param|is_pod and not param|is_enum}}
> {{- ", " if not loop.last}}
> {%- endfor -%}
> > {{method.mojom_name}};
> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> index cbcfb64a..2be65d43 100644
> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> @@ -62,7 +62,9 @@
> {%- else %}
> std::tie({{param.mojom_name}}Buf, std::ignore) =
> {%- endif %}
> -{%- if param|is_enum %}
> +{%- if param|is_flags %}
> + IPADataSerializer<{{param|name_full}}>::serialize({{param.mojom_name}}
> +{%- elif param|is_enum %}
> IPADataSerializer<uint32_t>::serialize(static_cast<uint32_t>({{param.mojom_name}})
> {%- else %}
> IPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}
> @@ -105,7 +107,9 @@
> #}
> {%- macro deserialize_param(param, pointer, loop, buf, fds, iter, data_size) -%}
> {{"*" if pointer}}{{param.mojom_name}} =
> -{%- if param|is_enum %}
> +{%- if param|is_flags %}
> +IPADataSerializer<{{param|name_full}}>::deserialize(
> +{%- elif param|is_enum %}
> static_cast<{{param|name_full}}>(IPADataSerializer<uint32_t>::deserialize(
> {%- else %}
> IPADataSerializer<{{param|name}}>::deserialize(
> @@ -133,7 +137,7 @@ IPADataSerializer<{{param|name}}>::deserialize(
> {%- if param|needs_control_serializer %}
> &controlSerializer_
> {%- endif -%}
> -){{")" if param|is_enum}};
> +){{")" if param|is_enum and not param|is_flags}};
> {%- endmacro -%}
>
>
> diff --git a/utils/ipc/generators/libcamera_templates/serializer.tmpl b/utils/ipc/generators/libcamera_templates/serializer.tmpl
> index 77bae36f..323e1293 100644
> --- a/utils/ipc/generators/libcamera_templates/serializer.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl
> @@ -34,6 +34,10 @@
> std::tie({{field.mojom_name}}, std::ignore) =
> {%- if field|is_pod %}
> IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}});
> + {%- elif field|is_flags %}
> + IPADataSerializer<{{field|name_full}}>::serialize(data.{{field.mojom_name}});
> + {%- elif field|is_enum_scoped %}
> + IPADataSerializer<uint{{field|bit_width}}_t>::serialize(static_cast<uint{{field|bit_width}}_t>(data.{{field.mojom_name}}));
> {%- elif field|is_enum %}
> IPADataSerializer<uint{{field|bit_width}}_t>::serialize(data.{{field.mojom_name}});
> {%- endif %}
> @@ -96,6 +100,8 @@
> {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}
> {%- if field|is_pod %}
> ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}});
> + {%- elif field|is_flags %}
> + ret.{{field.mojom_name}} = IPADataSerializer<{{field|name_full}}>::deserialize(m, m + {{field_size}});
> {%- else %}
> ret.{{field.mojom_name}} = static_cast<{{field|name_full}}>(IPADataSerializer<uint{{field|bit_width}}_t>::deserialize(m, m + {{field_size}}));
> {%- endif %}
> diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py
> index 753bfc73..6c176aba 100644
> --- a/utils/ipc/generators/mojom_libcamera_generator.py
> +++ b/utils/ipc/generators/mojom_libcamera_generator.py
> @@ -74,6 +74,8 @@ def GetDefaultValue(element):
> return element.default
> if type(element.kind) == mojom.Kind:
> return '0'
> + if IsFlags(element):
> + return ''
> if mojom.IsEnumKind(element.kind):
> return f'static_cast<{element.kind.mojom_name}>(0)'
> if isinstance(element.kind, mojom.Struct) and \
> @@ -184,7 +186,7 @@ def MethodParameters(method):
> params = []
> for param in method.parameters:
> params.append('const %s %s%s' % (GetNameForElement(param),
> - '&' if not IsPod(param) else '',
> + '' if IsPod(param) or IsEnum(param) else '&',
> param.mojom_name))
> for param in MethodParamOutputs(method):
> params.append(f'{GetNameForElement(param)} *{param.mojom_name}')
> @@ -220,9 +222,28 @@ def IsControls(element):
> def IsEnum(element):
> return mojom.IsEnumKind(element.kind)
>
> +# Only works the enum definition, not types
> +def IsScoped(element):
> + attributes = getattr(element, 'attributes', None)
> + if not attributes:
> + return False
> + return 'scopedEnum' in attributes
> +
> +
> +def IsEnumScoped(element):
> + if not IsEnum(element):
> + return False
> + return IsScoped(element.kind)
> +
I guess Python coding style wants two empy lines between function
declarations ? At least this is how the current implementation looks
like
Clearly a minor:
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> def IsFd(element):
> return mojom.IsStructKind(element.kind) and element.kind.mojom_name == "SharedFD"
>
> +def IsFlags(element):
> + attributes = getattr(element, 'attributes', None)
> + if not attributes:
> + return False
> + return 'flags' in attributes
> +
> def IsMap(element):
> return mojom.IsMapKind(element.kind)
>
> @@ -251,9 +272,11 @@ def ByteWidthFromCppType(t):
> raise Exception('invalid type')
> return str(int(_bit_widths[key]) // 8)
>
> -
> # Get the type name for a given element
> def GetNameForElement(element):
> + # Flags
> + if IsFlags(element):
> + return f'Flags<{GetFullNameForElement(element.kind)}>'
> # structs
> if (mojom.IsEnumKind(element) or
> mojom.IsInterfaceKind(element) or
> @@ -302,7 +325,8 @@ def GetNameForElement(element):
> def GetFullNameForElement(element):
> name = GetNameForElement(element)
> namespace_str = ''
> - if mojom.IsStructKind(element):
> + if (mojom.IsStructKind(element) or
> + mojom.IsEnumKind(element)):
> namespace_str = element.module.mojom_namespace.replace('.', '::')
> elif (hasattr(element, 'kind') and
> (mojom.IsStructKind(element.kind) or
> @@ -311,6 +335,10 @@ def GetFullNameForElement(element):
>
> if namespace_str == '':
> return name
> +
> + if IsFlags(element):
> + return GetNameForElement(element)
> +
> return f'{namespace_str}::{name}'
>
> def ValidateZeroLength(l, s, cap=True):
> @@ -407,10 +435,13 @@ class Generator(generator.Generator):
> 'is_array': IsArray,
> 'is_controls': IsControls,
> 'is_enum': IsEnum,
> + 'is_enum_scoped': IsEnumScoped,
> 'is_fd': IsFd,
> + 'is_flags': IsFlags,
> 'is_map': IsMap,
> 'is_plain_struct': IsPlainStruct,
> 'is_pod': IsPod,
> + 'is_scoped': IsScoped,
> 'is_str': IsStr,
> 'method_input_has_fd': MethodInputHasFd,
> 'method_output_has_fd': MethodOutputHasFd,
> --
> 2.30.2
>
More information about the libcamera-devel
mailing list