Message ID | 20221011105859.457567-5-paul.elder@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
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@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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@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 >
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) + 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,