[libcamera-devel] [PATCH v2] ipa: core.mojom: Rework core file documentation

[Jacopo Mondi]

Hi Paul,
  thanks for your comment

On Mon, Jul 19, 2021 at 03:09:20PM +0900, paul.elder at ideasonboard.com wrote:
> Hi Jacopo,
>
> On Fri, Jul 16, 2021 at 02:19:02PM +0200, Jacopo Mondi wrote:
> > The comment block at the beginning of the core.mojom file is meant to
> > provide an overview of how to use libcamera defined types in the definition
> > of mojom interfaces.
> >
> > As the IPA/IPC interface definition mechanism evolved, the documentation
> > has not been updated accordingly.
> >
> > Update the file comments to match the most recent IPA/IPC
> > interface definition and generation mechanism.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > v1->v2:
> > - Address Paul's comment and clarify points that were not clear to me when I
> >   wrote v1 :)
> >   - (de)serializers implementations go in ipa_data_serializer.cpp
> >   - types used as array/map members do not require a mojom definition
> >   - remove duplications
> >   - s/the library/libcamera
> > - While at it, make all statements start with a capital letter and end without a
> >   full-stop.
> > ---
> >  include/libcamera/ipa/core.mojom | 67 +++++++++++++++++++-------------
> >  1 file changed, 40 insertions(+), 27 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> > index b32f30939454..39fac624658a 100644
> > --- a/include/libcamera/ipa/core.mojom
> > +++ b/include/libcamera/ipa/core.mojom
> > @@ -15,40 +15,53 @@ module libcamera;
> >   *
> >   * Attributes:
> >   * - skipHeader - structs only, and only in core.mojom
> > - *   - designate that this struct shall not have a C++ header definition
> > - *     generated
> > + *   - Do not generate a C++ definition for the structure
> > + *   - Any type used in a mojom interface definition must have a corresponding
> > + *     definition in a mojo file for the Mojo core to accept it, except for
>
> Here you say "Mojo core" and below you say "build system"... idk which
> one is better, though. We don't really have a "Mojo core" (we only use
> the mojo lexer and parser; the code generator and certainly the IPC
> system is custom), but also "build system" sounds too ambiguous?
>

The "code generator" ?

> > + *     types used as map/array members for which a definition is not required
>
> If the type is *only* used as a map/array member, then the type doesn't
> need to be defined in mojom. It still needs to be defined in C++, and a
> (de)serializer implemented, though. I think you point it out below under
> "Rules", but it might be good to point out the caveat here too?

Yes, I missed the fact that is should be *only* used as a container
member.

>
> > + *   - This attribute allows defining a symbol for the Mojo core that
> > + *     corresponds to a libcamera type without duplicating its definition in the
> > + *     generated C++ headers
> >   * - skipSerdes - structs only, and only in core.mojom
> > - *   - designate that this struct shall not have a (de)serializer generated
> > - *   - all fields need a (de)serializer to be defined, either hand-written
> > - *     in ipa_data_serializer.h
> > + *   - Do not generate a (de)serializer for the structure
> > + *     - This attribute instructs the build system that a (de)serializer is
> > + *       available for the type and there's no need to generate one
> > + *   - All types need a (de)serializer to be defined in order to be transported
> > + *     over the IPA protocol. The (de)serializer can be:
> > + *     - Manually implemented as a template specialization in
> > + *       ipa_data_serializer.cpp in the libcamera sources
> > + *     - Generated at build time for types defined in a mojo interfaces
>
> Maybe clarify that the second situation is when skipSerdes is *not*
> specified?
>

I just said above "Do not generate a (de)serializer..."

I'll swap points, to make this one the first one and give a general
introduction and then introduce skipSerdes

> >   * - hasFd - struct fields or empty structs only
> > - *   - designate that this field or empty struct contains a FileDescriptor
> > + *   - Designate that this field or empty struct contains a FileDescriptor
> >   *
> >   * Rules:
> > - * - Any struct that is used in a struct definition in mojom must also be
> > - *   defined in mojom
> > - *   - If the struct has both a definition in a C++ header and a (de)serializer
> > - *     in ipa_data_serializer.h, then the struct shall be declared as empty,
> > - *     with both the [skipHeader] and [skipSerdes] attributes
> > - *   - If the struct only has a definition in a C++ header, but no
> > - *     (de)serializer, then the struct definition should have the [skipHeader]
> > - *     attribute
> > - * - Nested structures (e.g. FrameBuffer::Plane) cannot be defined in mojom.
> > - *   - Avoid them, by defining them in a header in C++ and a (de)serializer in
> > - *     ipa_data_serializer.h
> > - * - If a struct is in an array/map inside a struct, then the struct that is
> > - *   the member of the array/map does not need a mojom definition if it is
> > - *   defined in a C++ header.
> > - *   - This can be used to embed nested structures. The C++ double colon is
> > - *     replaced with a dot (e.g. FrameBuffer::Plane -> FrameBuffer.Plane)
> > - *   - The struct must still be defined in a header in C++ and a (de)serializer
> > - *     implemented in ipa_data_serializer.h, as it cannot be defined in mojom
> > + * - If the type is defined in a libcamera C++ header *and* a (de)serializer is
> > + *   available then the type shall be declared as empty with both attributes
> > + *   associated and specified as: [skipHeader, skipSerdes]
> > + *   - Example: [skipHeader, skipSerdes] ControlList {};
> > + * - If the type is defined in libcamera but no (de)serializer is available
> > + *   then the type definition in the core.mojom file should have the
> > + *   [skipHeader] attribute only
> > + *   - A (de)serializer will be generated for the type
> > + * - If a type definition has [skipHeader], then the header where the type is
> > + *   defined must be included in ipa_interface.h
> > + * - Types that are contained in an array/map do not require a mojom definition
>
> Again, types that are *only* used as array/map members.
>
> > + *   if one exists in libcamera
>
> Yes that is correct, if there is no definition of the type in libcamera,
> then you can define it in mojom and let the code generator generate the
> C++ definition and the (de)serializer. (technically it's if-and-only-if
> but whatever)
>
> > + * - Nested types (e.g. FrameBuffer::Plane) cannot be directly used in mojom
>
> The original says cannot be *defined*.
>
> > + *   - If used directly in mojom, the nested type shall be defined in a C++
>
> "cannot be directly used" "if used directly" :D
>
> > + *     header and a (de)serializer shall be provided
>
> This is the condition for using nested types. The restriction is that
> they can only be used as map/array members. I think you mention that
> below but bringing it up here might be better?
>
> > + *   - The C++ namespace separator :: is replaced with a dot
> > + *   - In example, to directly use the FrameBuffer::Plane type:
> > + *     - Provide a definition of the Plane type in a C++ header
>
> s/Plane/FrameBuffer::Plane/ ?
>
> > + *     - Include the header in ipa_interface.h
> > + *     - Provide a (de)serializer implementation ipa_data_serializer.cpp
> > + *     - In mojom, reference the type as FrameBuffer.Plane
>
> This can only be referenced as a map/array member.
>
> > + *   - Nested types can be used without an associated C++ definition if used
>
> s/can be/can only be/ because nested types can't be defined.
>
> s/C++ definition/mojom definition/
>
> The C++ definition + (de)serializer is certainly required.

This now looks like

 * - Nested types (e.g. FrameBuffer::Plane) cannot be defined in mojom
 *   - If used in mojom, the nested type shall be defined in a C++ header
 *     and a (de)serializer shall be provided
 *   - Nested types can only be used as part of array/map containers
 *   - When using the type, the C++ namespace separator :: is replaced with a
 *     dot
 *   - In example, to use the FrameBuffer::Plane type in mojom:
 *     - Provide a definition of the FrameBuffer::Plane type in a C++ header
 *     - Include the header in ipa_interface.h
 *     - Provide a (de)serializer implementation ipa_data_serializer.cpp
 *     - In mojom, reference the type as FrameBuffer.Plane and only as map/array
 *       member

>
>
> Sorry this is so complicated :(

Nah, don't worry, not easy to express stuff, thank you for sticking with it and
rectify my understanding.
v3 out soon

Thanks
  j

>
>
> Paul
>
> > + *     as part of an array/map container
> >   * - [skipHeader] and [skipSerdes] only work here in core.mojom.
> > - * - If a struct definition has [skipHeader], then the header where the
> > - *   struct is defined must be #included in ipa_interface.h
> >   * - If a field in a struct has a FileDescriptor, but is not explicitly
> >   *   defined so in mojom, then the field must be marked with the [hasFd]
> > - *   attribute.
> > + *   attribute
> >   *
> >   * \todo Generate documentation from Doxygen comments in .mojom files
> >   * \todo Figure out how to keep the skipHeader structs in sync with their
> > --
> > 2.32.0
> >