[libcamera-devel] [PATCH v5 8/9] utils: ipc: Allow the skipHeader attribute on enums

[paul.elder at ideasonboard.com]

On Wed, Oct 12, 2022 at 03:55:15PM +0200, Jacopo Mondi wrote:
> Hi Paul
> 
> On Tue, Oct 11, 2022 at 07:58:58PM +0900, Paul Elder via libcamera-devel wrote:
> > Currently, enums that are passed between pipeline handlers and their IPA
> > must be defined in a mojom file. However, there is a use case for
> > enum/flags to be defined in a C++ header, such that the enum can be used
> > in a component other than the pipeline handler and its IPA.
> >
> > To support this, add support for the skipHeader attribute for enums.
> > Like structs, it is only allowed in core.mojom.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > New in v5
> > ---
> >  include/libcamera/ipa/core.mojom                              | 4 +++-
> >  include/libcamera/ipa/ipa_interface.h                         | 4 ++--
> >  .../generators/libcamera_templates/core_ipa_interface.h.tmpl  | 2 +-
> >  utils/ipc/generators/mojom_libcamera_generator.py             | 2 +-
> >  4 files changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> > index ef28ff2d..1ff674b0 100644
> > --- a/include/libcamera/ipa/core.mojom
> > +++ b/include/libcamera/ipa/core.mojom
> > @@ -14,7 +14,7 @@ module libcamera;
> >   * - structs
> >   *
> >   * Attributes:
> > - * - skipHeader - structs only, and only in core.mojom
> > + * - skipHeader - allowed only for structs and enums in core.mojom
> >   *   - Do not generate a C++ definition for the structure
> 
> s/structure/type ?
> 
> >   *   - Any type used in a mojom interface definition must have a corresponding
> >   *     definition in a mojom file for the code generator to accept it, except
> 
> I guess we don't want [skipSerdes] for enums ? Or do we ?

enums, like PODs, already get skipSerdes-ed.

> 
> > @@ -52,6 +52,8 @@ module libcamera;
> >   *   then the type definition in the core.mojom file should have the
> >   *   [skipHeader] attribute only
> >   *   - A (de)serializer will be generated for the type
> > + *   - enums that are defined in a libcamera C++ header also fall in this
> > + *     category
> 
> Is this necessary, as we already claimed the attribute applies to
> struct and enums ?
>
> 
> >   * - If a type definition has [skipHeader], then the header where the type is
> >   *   defined must be included in ipa_interface.h
> >   * - Types that are solely used as array/map members do not require a mojom
> > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> > index 8afcfe21..8884f0ed 100644
> > --- a/include/libcamera/ipa/ipa_interface.h
> > +++ b/include/libcamera/ipa/ipa_interface.h
> > @@ -23,8 +23,8 @@
> >  namespace libcamera {
> >
> >  /*
> > - * Structs that are defined in core.mojom and have the skipHeader tag must be
> > - * #included here.
> > + * Structs and enums that are defined in core.mojom that have the skipHeader
> > + * tag must be #included here.
> 
> I would take the occasion to re-phrase this:
> 
>     * The header file where symbols used in core.mojom are defined
>     * must be #included here
> 
> If my understanding is correct

It's not wrong, I just think it's less precise.


Paul

> 
> >   */
> >
> >  class IPAInterface
> > diff --git a/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl b/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl
> > index a565b59a..c60b99b8 100644
> > --- a/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl
> > +++ b/utils/ipc/generators/libcamera_templates/core_ipa_interface.h.tmpl
> > @@ -26,7 +26,7 @@ namespace libcamera {
> >  static const {{const.kind|name}} {{const.mojom_name}} = {{const.value}};
> >  {% endfor %}
> >
> > -{% for enum in enums %}
> > +{% for enum in enums_gen_header %}
> >  {{funcs.define_enum(enum)}}
> >  {% endfor %}
> >
> > diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py
> > index 6c176aba..64987ccd 100644
> > --- a/utils/ipc/generators/mojom_libcamera_generator.py
> > +++ b/utils/ipc/generators/mojom_libcamera_generator.py
> > @@ -483,7 +483,7 @@ class Generator(generator.Generator):
> >      def _GetJinjaExportsForCore(self):
> >          return {
> >              'consts': self.module.constants,
> > -            'enums': self.module.enums,
> > +            'enums_gen_header': [x for x in self.module.enums if x.attributes is None or 'skipHeader' not in x.attributes],
> >              '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,
> >              'structs_gen_header': [x for x in self.module.structs if x.attributes is None or 'skipHeader' not in x.attributes],
> > --
> > 2.30.2
> >