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

13028 diff mbox series

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@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(-)

--
2.32.0

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@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?

> + *     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?

> + *   - 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?

>   * - 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.


Sorry this is so complicated :(


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
>

Hi Paul,
  thanks for your comment

On Mon, Jul 19, 2021 at 03:09:20PM +0900, paul.elder@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@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
> >