[PATCH] test: controls: control_info: Add test for enum controls

[Paul Elder]

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 at 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;
> >   	}
> >   };
>