[libcamera-devel,v2,4/7] utils: ipc: Add support for Flags

17150 diff mbox series

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.

The [Flags] attribute can also 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>

---
Question for reviewers: Should we use a different attribute name to
specify that an enum should be an enum class?

My rationale here was that if you want to use an enum for Flags then you
put the [Flags] attribute on it, then it'll be generated as an enum
class, and just like how in C++ you have to wrap E in Flags<>, the
analogous form in mojom is the [Flags] attribute.

Also, I did try converting all enums to enum classes; it broke a lot of
raspberrypi stuff because they expect enums to be usable as both flags
and ints (as far as I could understand)...

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/ipa_interface.h         |  1 +
 .../definition_functions.tmpl                 |  2 +-
 .../libcamera_templates/proxy_functions.tmpl  | 10 +++++---
 .../libcamera_templates/serializer.tmpl       |  4 ++++
 .../generators/mojom_libcamera_generator.py   | 23 ++++++++++++++++++-
 5 files changed, 35 insertions(+), 5 deletions(-)

On Thu, Aug 18, 2022 at 03:49:19PM +0900, Paul Elder 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.
> 
> The [Flags] attribute can also 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>
> 
> ---
> Question for reviewers: Should we use a different attribute name to
> specify that an enum should be an enum class?
> 
> My rationale here was that if you want to use an enum for Flags then you
> put the [Flags] attribute on it, then it'll be generated as an enum
> class, and just like how in C++ you have to wrap E in Flags<>, the
> analogous form in mojom is the [Flags] attribute.

It turns out that since you can make Flags out of an enum that isn't a
class, maybe this isn't even necessary? I just noticed in the
generated_serializer test (next patch) I forgot to put [Flags] in the
enum definition but in the [TEST] patch (at the end of the series) I
did.

Not sure which is better.

Paul

> 
> Also, I did try converting all enums to enum classes; it broke a lot of
> raspberrypi stuff because they expect enums to be usable as both flags
> and ints (as far as I could understand)...
> 
> 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/ipa_interface.h         |  1 +
>  .../definition_functions.tmpl                 |  2 +-
>  .../libcamera_templates/proxy_functions.tmpl  | 10 +++++---
>  .../libcamera_templates/serializer.tmpl       |  4 ++++
>  .../generators/mojom_libcamera_generator.py   | 23 ++++++++++++++++++-
>  5 files changed, 35 insertions(+), 5 deletions(-)
> 
> 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..fa185177 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_flags}} {{enum.mojom_name}} {
>  {%- for field in enum.fields %}
>  	{{field.mojom_name}} = {{field.numeric_value}},
>  {%- endfor %}
> 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..eec75211 100644
> --- a/utils/ipc/generators/libcamera_templates/serializer.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl
> @@ -34,6 +34,8 @@
>  		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 %}
>  			IPADataSerializer<uint{{field|bit_width}}_t>::serialize(data.{{field.mojom_name}});
>  	{%- endif %}
> @@ -96,6 +98,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..5534b24e 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 \
> @@ -223,6 +225,15 @@ def IsEnum(element):
>  def IsFd(element):
>      return mojom.IsStructKind(element.kind) and element.kind.mojom_name == "SharedFD"
>  
> +def IsFlags(element):
> +    if not hasattr(element, 'attributes'):
> +        return False
> +    if element.attributes is None:
> +        return False
> +    if 'Flags' in element.attributes:
> +        return True
> +    return False
> +
>  def IsMap(element):
>      return mojom.IsMapKind(element.kind)
>  
> @@ -251,9 +262,13 @@ 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):
> +        if mojom.IsEnumKind(element):
> +            return element.mojom_name
> +        return f'Flags<{GetFullNameForElement(element.kind)}>'
>      # structs
>      if (mojom.IsEnumKind(element) or
>          mojom.IsInterfaceKind(element) or
> @@ -311,6 +326,11 @@ def GetFullNameForElement(element):
>  
>      if namespace_str == '':
>          return name
> +
> +    if IsFlags(element):
> +        name = re.match(r'^Flags<(.*)>$', name).group(1)
> +        return f'Flags<{namespace_str}::{name}>'
> +
>      return f'{namespace_str}::{name}'
>  
>  def ValidateZeroLength(l, s, cap=True):
> @@ -408,6 +428,7 @@ class Generator(generator.Generator):
>              'is_controls': IsControls,
>              'is_enum': IsEnum,
>              'is_fd': IsFd,
> +            'is_flags': IsFlags,
>              'is_map': IsMap,
>              'is_plain_struct': IsPlainStruct,
>              'is_pod': IsPod,
> -- 
> 2.30.2
>

Hi Paul,

Thank you for the patch.

On Thu, Aug 18, 2022 at 03:56:26PM +0900, Paul Elder via libcamera-devel wrote:
> On Thu, Aug 18, 2022 at 03:49:19PM +0900, Paul Elder 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.
> > 
> > The [Flags] attribute can also 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>
> > 
> > ---
> > Question for reviewers: Should we use a different attribute name to
> > specify that an enum should be an enum class?

I think so, as a scoped enumeration (a.k.a. enum class, but enum struct
works too) can be used as non-flags. I propose the scopedEnum attribute.

> > My rationale here was that if you want to use an enum for Flags then you
> > put the [Flags] attribute on it, then it'll be generated as an enum
> > class, and just like how in C++ you have to wrap E in Flags<>, the
> > analogous form in mojom is the [Flags] attribute.
> 
> It turns out that since you can make Flags out of an enum that isn't a
> class, maybe this isn't even necessary? I just noticed in the
> generated_serializer test (next patch) I forgot to put [Flags] in the
> enum definition but in the [TEST] patch (at the end of the series) I
> did.
> 
> Not sure which is better.

Flags<> work with both scoped and unscoped enumerations indeed, but it
only makes sense with scoped enumerations. With an unscoped enumeration
you can use logical operators on the enumerators directly, without the
help of the Flag class.

> > Also, I did try converting all enums to enum classes; it broke a lot of
> > raspberrypi stuff because they expect enums to be usable as both flags
> > and ints (as far as I could understand)...

We could possibly handle some low-hanging fruits already, but some
enumerations would be header to convert indeed.

Additional discussion point: I think we'll to support the skipHeader
attribute for enums.

> > 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/ipa_interface.h         |  1 +
> >  .../definition_functions.tmpl                 |  2 +-
> >  .../libcamera_templates/proxy_functions.tmpl  | 10 +++++---
> >  .../libcamera_templates/serializer.tmpl       |  4 ++++
> >  .../generators/mojom_libcamera_generator.py   | 23 ++++++++++++++++++-
> >  5 files changed, 35 insertions(+), 5 deletions(-)
> > 
> > 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..fa185177 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_flags}} {{enum.mojom_name}} {

I suppose is_flags will be turned into is_scoped

> >  {%- for field in enum.fields %}
> >  	{{field.mojom_name}} = {{field.numeric_value}},
> >  {%- endfor %}
> > 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..eec75211 100644
> > --- a/utils/ipc/generators/libcamera_templates/serializer.tmpl
> > +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl
> > @@ -34,6 +34,8 @@
> >  		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 %}
> >  			IPADataSerializer<uint{{field|bit_width}}_t>::serialize(data.{{field.mojom_name}});
> >  	{%- endif %}
> > @@ -96,6 +98,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..5534b24e 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 \
> > @@ -223,6 +225,15 @@ def IsEnum(element):
> >  def IsFd(element):
> >      return mojom.IsStructKind(element.kind) and element.kind.mojom_name == "SharedFD"
> >  
> > +def IsFlags(element):
> > +    if not hasattr(element, 'attributes'):
> > +        return False
> > +    if element.attributes is None:
> > +        return False
> > +    if 'Flags' in element.attributes:
> > +        return True
> > +    return False

This could be simplified to

    attributes = getattr(element, 'attributes', None)
    if not attributes:
        return False
    return 'Flags' in attributes

Up to you.

> > +
> >  def IsMap(element):
> >      return mojom.IsMapKind(element.kind)
> >  
> > @@ -251,9 +262,13 @@ 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):
> > +        if mojom.IsEnumKind(element):
> > +            return element.mojom_name
> > +        return f'Flags<{GetFullNameForElement(element.kind)}>'
> >      # structs
> >      if (mojom.IsEnumKind(element) or
> >          mojom.IsInterfaceKind(element) or
> > @@ -311,6 +326,11 @@ def GetFullNameForElement(element):
> >  
> >      if namespace_str == '':
> >          return name
> > +
> > +    if IsFlags(element):
> > +        name = re.match(r'^Flags<(.*)>$', name).group(1)
> > +        return f'Flags<{namespace_str}::{name}>'

I may be missing something, but for flags, isn't the full name identical
to the name, given that GetNameForElement() qualifies the Flags template
argument with the namespace ? If so, you could write

        return GetNameForElement(element)

or

        return f'Flags<GetFullNameForElement(element.kind)>'

> > +
> >      return f'{namespace_str}::{name}'
> >  
> >  def ValidateZeroLength(l, s, cap=True):
> > @@ -408,6 +428,7 @@ class Generator(generator.Generator):
> >              'is_controls': IsControls,
> >              'is_enum': IsEnum,
> >              'is_fd': IsFd,
> > +            'is_flags': IsFlags,
> >              'is_map': IsMap,
> >              'is_plain_struct': IsPlainStruct,
> >              'is_pod': IsPod,