test: controls: control_info: Add test for enum controls
diff mbox series

Message ID 20240910133130.1374459-1-paul.elder@ideasonboard.com
State New
Headers show
Series
  • test: controls: control_info: Add test for enum controls
Related show

Commit Message

Paul Elder Sept. 10, 2024, 1:31 p.m. UTC
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(+)

Comments

Umang Jain Sept. 11, 2024, 4:34 a.m. UTC | #1
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;
>   	}
>   };
Paul Elder Sept. 11, 2024, 9:06 a.m. UTC | #2
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;
> >   	}
> >   };
>
Kieran Bingham Sept. 11, 2024, 4:12 p.m. UTC | #3
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;
> > >     }
> > >   };
> >
Umang Jain Sept. 12, 2024, 3:31 a.m. UTC | #4
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;
>>>    	}
>>>    };

Patch
diff mbox series

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