| Message ID | 20221011105859.457567-10-paul.elder@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. 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@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. We should either find a suitable enum that IPA modules (will) really need, or try to test this feature using test.mojom and/or vimc.mojom. > [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 {
Hi Laurent, On Tue, Oct 11, 2022 at 05:01:53PM +0300, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > 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@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? :) > 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. So we need a suitable enum that IPA modules will really need. Paul > > > [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 {
Hi Paul, On Wed, Oct 12, 2022 at 11:30:02AM +0900, paul.elder@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@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 {
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 {}; + [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 {
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@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(+)