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

[Kieran Bingham]

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 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?

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