| Message ID | 20240910133130.1374459-1-paul.elder@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul On 10/09/24 7:01 pm, Paul Elder wrote: > Add a test for enum controls to test that the ranges and values are set > properly. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > test/controls/control_info.cpp | 39 ++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp > index e1bb43f0e..460aaa345 100644 > --- a/test/controls/control_info.cpp > +++ b/test/controls/control_info.cpp > @@ -6,6 +6,7 @@ > */ > > #include <iostream> > +#include <vector> > > #include <libcamera/control_ids.h> > #include <libcamera/controls.h> > @@ -79,6 +80,44 @@ protected: > return TestFail; > } > > + /* > + * Test information retrieval from an enum control. > + */ > + ControlInfo awbMode(static_cast<int32_t>(controls::AwbTungsten), > + static_cast<int32_t>(controls::AwbDaylight)); > + if (awbMode.min().get<int32_t>() != controls::AwbTungsten || > + awbMode.max().get<int32_t>() != controls::AwbDaylight) { > + cout << "Invalid control range for AwbMode" << endl; > + return TestFail; > + } > + > + std::vector<ControlValue> modes = { > + static_cast<int32_t>(controls::AwbTungsten), > + static_cast<int32_t>(controls::AwbFluorescent), > + static_cast<int32_t>(controls::AwbDaylight), > + }; > + ControlInfo awbModes(Span<const ControlValue>{ modes }); > + > + if (awbModes.min() != modes.front() || > + awbModes.def() != modes.front() || > + awbModes.max() != modes.back()) { > + cout << "Invalid control range for AwbModes" << endl; > + return TestFail; > + } > + > + if (awbModes.values().size() != modes.size()) { Are we really comparing the awbModes with the modes vector ? Shouldn't we be comparing awbModes with awbMode ? > + cout << "Invalid size for AwbModes" << endl; > + return TestFail; > + } > + > + unsigned int i = 0; > + for (const auto &value : awbModes.values()) { > + if (value != modes.at(i++)) { Similar comment here: Shouldn't we be comparing enum values : ControlInfo awbModes vs ControlInfo awbMode My idea of the test would be to equate the values of the range vs values from modes vector. Does it make sense? > + cout << "Invalid control values for AwbModes" << endl; > + return TestFail; > + } > + } > + > return TestPass; > } > };
Hi Umang, On Wed, Sep 11, 2024 at 10:04:24AM +0530, Umang Jain wrote: > Hi Paul > > On 10/09/24 7:01 pm, Paul Elder wrote: > > Add a test for enum controls to test that the ranges and values are set > > properly. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > test/controls/control_info.cpp | 39 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp > > index e1bb43f0e..460aaa345 100644 > > --- a/test/controls/control_info.cpp > > +++ b/test/controls/control_info.cpp > > @@ -6,6 +6,7 @@ > > */ > > #include <iostream> > > +#include <vector> > > #include <libcamera/control_ids.h> > > #include <libcamera/controls.h> > > @@ -79,6 +80,44 @@ protected: > > return TestFail; > > } > > + /* > > + * Test information retrieval from an enum control. > > + */ > > + ControlInfo awbMode(static_cast<int32_t>(controls::AwbTungsten), > > + static_cast<int32_t>(controls::AwbDaylight)); > > + if (awbMode.min().get<int32_t>() != controls::AwbTungsten || > > + awbMode.max().get<int32_t>() != controls::AwbDaylight) { > > + cout << "Invalid control range for AwbMode" << endl; > > + return TestFail; > > + } > > + > > + std::vector<ControlValue> modes = { > > + static_cast<int32_t>(controls::AwbTungsten), > > + static_cast<int32_t>(controls::AwbFluorescent), > > + static_cast<int32_t>(controls::AwbDaylight), > > + }; > > + ControlInfo awbModes(Span<const ControlValue>{ modes }); > > + > > + if (awbModes.min() != modes.front() || > > + awbModes.def() != modes.front() || > > + awbModes.max() != modes.back()) { > > + cout << "Invalid control range for AwbModes" << endl; > > + return TestFail; > > + } > > + > > + if (awbModes.values().size() != modes.size()) { > > Are we really comparing the awbModes with the modes vector ? > > Shouldn't we be comparing awbModes with awbMode ? Ok I think this is caused by a mismatch of what you imagine should be tested and what I wanted to test. I wanted to test/confirm that if you create an enum ControlInfo from a list of values that it would store those values properly (which was confusing because ControlInfo::toString() only shows "[0..2]" so it looks like 0, 1, and 2 are supported even if you created the ControlInfo with only 0 and 2). And in fact, we shouldn't actually construct enum ControlInfo like awbMode is, as we've decided (so far) that the discriminator between an enum control and a non-enum control is if ControlInfo::values() is non-null. So by that definition awbMode wouldn't actually be treated as an enum ControlInfo. But enum ControlInfos shouldn't be created like that anyway so maybe I should get rid of that actually? > > + cout << "Invalid size for AwbModes" << endl; > > + return TestFail; > > + } > > + > > + unsigned int i = 0; > > + for (const auto &value : awbModes.values()) { > > + if (value != modes.at(i++)) { > > Similar comment here: > > Shouldn't we be comparing enum values : > > ControlInfo awbModes > > vs > > ControlInfo awbMode > > My idea of the test would be to equate the values of the range vs values > from modes vector. Does it make sense? I suppose we could extend ControlInfo to populate values_ when an enum ControlInfo is constructed from a range? Now that enum information is stored in ControlId we could do that. But imo it doesn't make sense to construct an enum ControlInfo from a range; you should be declaring which specific values you support from the enum. That's the point of enum controls right? Maybe I should just delete the awbMode test. Thanks, Paul > > + cout << "Invalid control values for AwbModes" << endl; > > + return TestFail; > > + } > > + } > > + > > return TestPass; > > } > > }; >
Quoting Paul Elder (2024-09-11 10:06:16) > Hi Umang, > > On Wed, Sep 11, 2024 at 10:04:24AM +0530, Umang Jain wrote: > > Hi Paul > > > > On 10/09/24 7:01 pm, Paul Elder wrote: > > > Add a test for enum controls to test that the ranges and values are set > > > properly. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > --- > > > test/controls/control_info.cpp | 39 ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 39 insertions(+) > > > > > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp > > > index e1bb43f0e..460aaa345 100644 > > > --- a/test/controls/control_info.cpp > > > +++ b/test/controls/control_info.cpp > > > @@ -6,6 +6,7 @@ > > > */ > > > #include <iostream> > > > +#include <vector> > > > #include <libcamera/control_ids.h> > > > #include <libcamera/controls.h> > > > @@ -79,6 +80,44 @@ protected: > > > return TestFail; > > > } > > > + /* > > > + * Test information retrieval from an enum control. > > > + */ > > > + ControlInfo awbMode(static_cast<int32_t>(controls::AwbTungsten), > > > + static_cast<int32_t>(controls::AwbDaylight)); > > > + if (awbMode.min().get<int32_t>() != controls::AwbTungsten || > > > + awbMode.max().get<int32_t>() != controls::AwbDaylight) { > > > + cout << "Invalid control range for AwbMode" << endl; > > > + return TestFail; > > > + } > > > + > > > + std::vector<ControlValue> modes = { > > > + static_cast<int32_t>(controls::AwbTungsten), > > > + static_cast<int32_t>(controls::AwbFluorescent), > > > + static_cast<int32_t>(controls::AwbDaylight), > > > + }; > > > + ControlInfo awbModes(Span<const ControlValue>{ modes }); > > > + > > > + if (awbModes.min() != modes.front() || > > > + awbModes.def() != modes.front() || > > > + awbModes.max() != modes.back()) { > > > + cout << "Invalid control range for AwbModes" << endl; > > > + return TestFail; > > > + } > > > + > > > + if (awbModes.values().size() != modes.size()) { > > > > Are we really comparing the awbModes with the modes vector ? > > > > Shouldn't we be comparing awbModes with awbMode ? > > Ok I think this is caused by a mismatch of what you imagine should be > tested and what I wanted to test. > > I wanted to test/confirm that if you create an enum ControlInfo from a > list of values that it would store those values properly (which was > confusing because ControlInfo::toString() only shows "[0..2]" so it > looks like 0, 1, and 2 are supported even if you created the ControlInfo > with only 0 and 2). > > And in fact, we shouldn't actually construct enum ControlInfo like > awbMode is, as we've decided (so far) that the discriminator between an > enum control and a non-enum control is if ControlInfo::values() is > non-null. So by that definition awbMode wouldn't actually be treated as > an enum ControlInfo. But enum ControlInfos shouldn't be created like > that anyway so maybe I should get rid of that actually? > > > > + cout << "Invalid size for AwbModes" << endl; > > > + return TestFail; > > > + } > > > + > > > + unsigned int i = 0; > > > + for (const auto &value : awbModes.values()) { > > > + if (value != modes.at(i++)) { > > > > Similar comment here: > > > > Shouldn't we be comparing enum values : > > > > ControlInfo awbModes > > > > vs > > > > ControlInfo awbMode > > > > My idea of the test would be to equate the values of the range vs values > > from modes vector. Does it make sense? > > I suppose we could extend ControlInfo to populate values_ when an enum > ControlInfo is constructed from a range? Now that enum information is > stored in ControlId we could do that. > > But imo it doesn't make sense to construct an enum ControlInfo from a > range; you should be declaring which specific values you support from > the enum. That's the point of enum controls right? Would we expect any shortcuts for adding a control that supports 'all' available enum 's ? or would they have to be explicitly mentioned when being registered? -- Kieran > > Maybe I should just delete the awbMode test. > > > Thanks, > > Paul > > > > + cout << "Invalid control values for AwbModes" << endl; > > > + return TestFail; > > > + } > > > + } > > > + > > > return TestPass; > > > } > > > }; > >
Hi Paul, On 11/09/24 2:36 pm, Paul Elder wrote: > Hi Umang, > > On Wed, Sep 11, 2024 at 10:04:24AM +0530, Umang Jain wrote: >> Hi Paul >> >> On 10/09/24 7:01 pm, Paul Elder wrote: >>> Add a test for enum controls to test that the ranges and values are set >>> properly. >>> >>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> >>> --- >>> test/controls/control_info.cpp | 39 ++++++++++++++++++++++++++++++++++ >>> 1 file changed, 39 insertions(+) >>> >>> diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp >>> index e1bb43f0e..460aaa345 100644 >>> --- a/test/controls/control_info.cpp >>> +++ b/test/controls/control_info.cpp >>> @@ -6,6 +6,7 @@ >>> */ >>> #include <iostream> >>> +#include <vector> >>> #include <libcamera/control_ids.h> >>> #include <libcamera/controls.h> >>> @@ -79,6 +80,44 @@ protected: >>> return TestFail; >>> } >>> + /* >>> + * Test information retrieval from an enum control. >>> + */ >>> + ControlInfo awbMode(static_cast<int32_t>(controls::AwbTungsten), >>> + static_cast<int32_t>(controls::AwbDaylight)); >>> + if (awbMode.min().get<int32_t>() != controls::AwbTungsten || >>> + awbMode.max().get<int32_t>() != controls::AwbDaylight) { >>> + cout << "Invalid control range for AwbMode" << endl; >>> + return TestFail; >>> + } >>> + >>> + std::vector<ControlValue> modes = { >>> + static_cast<int32_t>(controls::AwbTungsten), >>> + static_cast<int32_t>(controls::AwbFluorescent), >>> + static_cast<int32_t>(controls::AwbDaylight), >>> + }; >>> + ControlInfo awbModes(Span<const ControlValue>{ modes }); >>> + >>> + if (awbModes.min() != modes.front() || >>> + awbModes.def() != modes.front() || >>> + awbModes.max() != modes.back()) { >>> + cout << "Invalid control range for AwbModes" << endl; >>> + return TestFail; >>> + } >>> + >>> + if (awbModes.values().size() != modes.size()) { >> Are we really comparing the awbModes with the modes vector ? >> >> Shouldn't we be comparing awbModes with awbMode ? > Ok I think this is caused by a mismatch of what you imagine should be > tested and what I wanted to test. > > I wanted to test/confirm that if you create an enum ControlInfo from a > list of values that it would store those values properly (which was > confusing because ControlInfo::toString() only shows "[0..2]" so it > looks like 0, 1, and 2 are supported even if you created the ControlInfo > with only 0 and 2). That seems fine to test. > > And in fact, we shouldn't actually construct enum ControlInfo like > awbMode is, as we've decided (so far) that the discriminator between an > enum control and a non-enum control is if ControlInfo::values() is > non-null. So by that definition awbMode wouldn't actually be treated as > an enum ControlInfo. But enum ControlInfos shouldn't be created like > that anyway so maybe I should get rid of that actually? Yes, if its not expected to be a range, then we should get rid of 'awbMode' Furthermore possibility, we can decide and go one step further, to prevent enum ControlInfos being defined as ranges, in controls handling logic. > >>> + cout << "Invalid size for AwbModes" << endl; >>> + return TestFail; >>> + } >>> + >>> + unsigned int i = 0; >>> + for (const auto &value : awbModes.values()) { >>> + if (value != modes.at(i++)) { >> Similar comment here: >> >> Shouldn't we be comparing enum values : >> >> ControlInfo awbModes >> >> vs >> >> ControlInfo awbMode >> >> My idea of the test would be to equate the values of the range vs values >> from modes vector. Does it make sense? > I suppose we could extend ControlInfo to populate values_ when an enum > ControlInfo is constructed from a range? Now that enum information is > stored in ControlId we could do that. > > But imo it doesn't make sense to construct an enum ControlInfo from a > range; you should be declaring which specific values you support from > the enum. That's the point of enum controls right? > > Maybe I should just delete the awbMode test. > > > Thanks, > > Paul > >>> + cout << "Invalid control values for AwbModes" << endl; >>> + return TestFail; >>> + } >>> + } >>> + >>> return TestPass; >>> } >>> };
diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp index e1bb43f0e..460aaa345 100644 --- a/test/controls/control_info.cpp +++ b/test/controls/control_info.cpp @@ -6,6 +6,7 @@ */ #include <iostream> +#include <vector> #include <libcamera/control_ids.h> #include <libcamera/controls.h> @@ -79,6 +80,44 @@ protected: return TestFail; } + /* + * Test information retrieval from an enum control. + */ + ControlInfo awbMode(static_cast<int32_t>(controls::AwbTungsten), + static_cast<int32_t>(controls::AwbDaylight)); + if (awbMode.min().get<int32_t>() != controls::AwbTungsten || + awbMode.max().get<int32_t>() != controls::AwbDaylight) { + cout << "Invalid control range for AwbMode" << endl; + return TestFail; + } + + std::vector<ControlValue> modes = { + static_cast<int32_t>(controls::AwbTungsten), + static_cast<int32_t>(controls::AwbFluorescent), + static_cast<int32_t>(controls::AwbDaylight), + }; + ControlInfo awbModes(Span<const ControlValue>{ modes }); + + if (awbModes.min() != modes.front() || + awbModes.def() != modes.front() || + awbModes.max() != modes.back()) { + cout << "Invalid control range for AwbModes" << endl; + return TestFail; + } + + if (awbModes.values().size() != modes.size()) { + cout << "Invalid size for AwbModes" << endl; + return TestFail; + } + + unsigned int i = 0; + for (const auto &value : awbModes.values()) { + if (value != modes.at(i++)) { + cout << "Invalid control values for AwbModes" << endl; + return TestFail; + } + } + return TestPass; } };
Add a test for enum controls to test that the ranges and values are set properly. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- test/controls/control_info.cpp | 39 ++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)