[libcamera-devel] [PATCH v5 9/9] [RFC] test: generated_serializer: Test skipHeader enums and flags

[Laurent Pinchart]

Hi Paul,

On Wed, Oct 12, 2022 at 11:30:02AM +0900, paul.elder at ideasonboard.com wrote:
> On Tue, Oct 11, 2022 at 05:01:53PM +0300, Laurent Pinchart wrote:
> > On Tue, Oct 11, 2022 at 07:58:59PM +0900, Paul Elder via libcamera-devel wrote:
> > > Add fields to the test struct to test serialization/deserialization of
> > > scoped enums and flags of said scoped enums that are defined in a C++
> > > header, which are thus designated by [skipHeader] in mojom.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > 
> > > ---
> > > New in v5
> > > 
> > > RFC because this touches core stuff. This can be detached from the rest
> > > of the series.
> > > ---
> > >  include/libcamera/ipa/core.mojom                      |  2 ++
> > >  include/libcamera/ipa/ipa_interface.h                 |  7 +++++++
> > >  .../generated_serializer_test.cpp                     | 11 +++++++++++
> > >  .../include/libcamera/ipa/test.mojom                  |  4 ++++
> > >  4 files changed, 24 insertions(+)
> > > 
> > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> > > index 1ff674b0..3438af93 100644
> > > --- a/include/libcamera/ipa/core.mojom
> > > +++ b/include/libcamera/ipa/core.mojom
> > > @@ -84,6 +84,8 @@ module libcamera;
> > >  [skipSerdes, skipHeader] struct ControlList {};
> > >  [skipSerdes, skipHeader] struct SharedFD {};
> > >  
> > > +[skipHeader, scopedEnum] enum TestEnum {};
> > > +
> > 
> > This particular part isn't very nice. Same for the change to
> > ipa_interface.h below.
> 
> Yeah that's what I thought :/
> 
> > We should either find a suitable enum that IPA modules (will) really need,
> 
> Any ideas? :)

Well... :-)

> > or try to test this feature using test.mojom and/or vimc.mojom.
> 
> That won't work (that's why this part of the series was delayed). The
> enums that are skipHeader-ed need to be accessbile from
> {pipeline}_ipa_interface.h, but those are generated from
> {pipeline}.mojom. Which means that either 1) the headers that defined
> those enums need to be #included in the global ipa_interface.h (so *all*
> IPAs have access to them) or 2) we add a new header that will
> automatically be imcluded in the generated {pipeline}_ipa_interface.h,
> which imo defeats the whole purpose of {pipeline}.mojom.

Indeed, neither are nice.

Could we use the mojo import directive ? It's meant to import other
.mojom files, but maybe we could use it to specify .h files that need to
be included ?

> So we need a suitable enum that IPA modules will really need.
> 
> > >  [skipHeader] struct Point {
> > >  	int32 x;
> > >  	int32 y;
> > > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> > > index 8884f0ed..96fb8344 100644
> > > --- a/include/libcamera/ipa/ipa_interface.h
> > > +++ b/include/libcamera/ipa/ipa_interface.h
> > > @@ -27,6 +27,13 @@ namespace libcamera {
> > >   * tag must be #included here.
> > >   */
> > >  
> > > +enum class TestEnum {
> > > +	TestEnumValueA,
> > > +	TestEnumValueB,
> > > +	TestEnumValueC,
> > > +	TestEnumValueD,
> > > +};
> > > +
> > >  class IPAInterface
> > >  {
> > >  public:
> > > diff --git a/test/serialization/generated_serializer/generated_serializer_test.cpp b/test/serialization/generated_serializer/generated_serializer_test.cpp
> > > index 4670fe46..6f01c3d4 100644
> > > --- a/test/serialization/generated_serializer/generated_serializer_test.cpp
> > > +++ b/test/serialization/generated_serializer/generated_serializer_test.cpp
> > > @@ -11,6 +11,8 @@
> > >  
> > >  #include "test.h"
> > >  
> > > +#include <libcamera/ipa/ipa_interface.h>
> > > +
> > >  #include "test_ipa_interface.h"
> > >  #include "test_ipa_serializer.h"
> > >  
> > > @@ -59,6 +61,9 @@ if (struct1.field != struct2.field) {				\
> > >  		t.s3 = "lorem ipsum";
> > >  		t.i  = 58527;
> > >  		t.c = ipa::test::IPAOperationInit;
> > > +		t.ex = TestEnum::TestEnumValueA;
> > > +		t.exf |= TestEnum::TestEnumValueB;
> > > +
> > >  		t.e = ipa::test::ErrorFlags::Error1;
> > >  
> > >  		Flags<ipa::test::ErrorFlags> flags;
> > > @@ -87,6 +92,8 @@ if (struct1.field != struct2.field) {				\
> > >  
> > >  		TEST_SCOPED_ENUM_EQUALITY(t, u, e);
> > >  		TEST_SCOPED_ENUM_EQUALITY(t, u, f);
> > > +		TEST_SCOPED_ENUM_EQUALITY(t, u, ex);
> > > +		TEST_SCOPED_ENUM_EQUALITY(t, u, exf);
> > >  
> > >  		/* Test vector of generated structs */
> > >  		std::vector<ipa::test::TestStruct> v = { t, u };
> > > @@ -113,6 +120,8 @@ if (struct1.field != struct2.field) {				\
> > >  
> > >  		TEST_SCOPED_ENUM_EQUALITY(v[0], w[0], e);
> > >  		TEST_SCOPED_ENUM_EQUALITY(v[0], w[0], f);
> > > +		TEST_SCOPED_ENUM_EQUALITY(v[0], w[0], ex);
> > > +		TEST_SCOPED_ENUM_EQUALITY(v[0], w[0], exf);
> > >  
> > >  		TEST_FIELD_EQUALITY(v[1], w[1], s1);
> > >  		TEST_FIELD_EQUALITY(v[1], w[1], s2);
> > > @@ -122,6 +131,8 @@ if (struct1.field != struct2.field) {				\
> > >  
> > >  		TEST_SCOPED_ENUM_EQUALITY(v[1], w[1], e);
> > >  		TEST_SCOPED_ENUM_EQUALITY(v[1], w[1], f);
> > > +		TEST_SCOPED_ENUM_EQUALITY(v[1], w[1], ex);
> > > +		TEST_SCOPED_ENUM_EQUALITY(v[1], w[1], exf);
> > >  
> > >  		return TestPass;
> > >  	}
> > > diff --git a/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom b/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> > > index 91c31642..1f1ba8dc 100644
> > > --- a/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> > > +++ b/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> > > @@ -2,6 +2,8 @@
> > >  
> > >  module ipa.test;
> > >  
> > > +import "include/libcamera/ipa/core.mojom";
> > > +
> > >  enum IPAOperationCode {
> > >  	IPAOperationNone,
> > >  	IPAOperationInit,
> > > @@ -28,6 +30,8 @@ struct TestStruct {
> > >  	IPAOperationCode c;
> > >  	ErrorFlags e;
> > >  	[flags] ErrorFlags f;
> > > +	libcamera.TestEnum ex;
> > > +	[flags] libcamera.TestEnum exf;
> > >  };
> > >  
> > >  interface IPATestInterface {

-- 
Regards,

Laurent Pinchart