[PATCH] libcamera: controls: Handle enum values without a cast

[Laurent Pinchart]

On Thu, Sep 26, 2024 at 11:28:41AM +0100, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2024-09-26 10:13:44)
> > On Thu, Sep 26, 2024 at 03:24:27AM GMT, Laurent Pinchart wrote:
> > > When constructing a ControlValue from an enum value, an explicit cast to
> > > int32_t is needed as we use int32_t as the underlying type for all
> > > enumerated controls. This makes users of ControlValue more complex. To
> > > simplify them, specialize the control_type template for enum types, to
> > > support construction of ControlValue directly without a cast.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > brilliant, less things to type in!
> > 
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > 
> > Thanks
> >   j
> > 
> > > ---
> > >  include/libcamera/controls.h         | 6 +++++-
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
> > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > index 25f67ed948a3..c5131870d402 100644
> > > --- a/include/libcamera/controls.h
> > > +++ b/include/libcamera/controls.h
> > > @@ -39,7 +39,7 @@ enum ControlType {
> > >
> > >  namespace details {
> > >
> > > -template<typename T>
> > > +template<typename T, typename = std::void_t<>>
> > >  struct control_type {
> > >  };
> > >
> > > @@ -102,6 +102,10 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
> > >       static constexpr std::size_t size = N;
> > >  };
> > >
> > > +template<typename T>
> > > +struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> {
> > > +};
> > > +
> 
> I have no idea how you get this black magic to work ... :-)

Sometimes I'm not sure myself :-)

> But if it works and is reducing boilerplate casts which impact readability
> then \o/
> 
> So I think this makes a new templated control_type of the given enum
> type, which is derived from an int32_t ...

It introduces a new specialization for the control_type structure
template, which is enabled only when T is a enumeration type. The
specialization inherits from control_type<int32_t> to make it explicit
that we're storing the value as an int32_t. I could have copied

	static constexpr ControlType value = ControlTypeInteger32;
	static constexpr std::size_t size = 0;

from struct control_type<int32_t> instead, but inheriting expresses the
goal better I think.

I had to add a second template parameter to the base template, as I
needed a place to introduce the std::enable_if_t<> in the template
specialization. Writing

template<typename T, std::enable_if_t<std::is_enum_v<T>>> * = nullptr>
struct control_type<T> : public control_type<int32_t> {
};

is not valid, as it's not a specialization of the base template.

As the second template parameter of the base template has a default
value, none of the other template specialization need to be modified.

> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > >  } /* namespace details */
> > >
> > >  class ControlValue
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 430aa902eec8..29d3c6c194c1 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -958,7 +958,7 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)
> > >               values.reserve(testPatternModes.size());
> > >
> > >               for (auto pattern : testPatternModes)
> > > -                     values.emplace_back(static_cast<int32_t>(pattern));
> > > +                     values.emplace_back(pattern);
> > >
> > >               controls[&controls::draft::TestPatternMode] = ControlInfo(values);
> > >       }
> > >
> > > base-commit: f2088eb91fd6477b152233b9031cb115ca1ae824

-- 
Regards,

Laurent Pinchart