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

[Umang Jain]

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 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).

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