[libcamera-devel] [PATCH v2 11/24] libcamera: v4l2_controls: Fix control range construction for bool

[Laurent Pinchart]

Hi Kieran,

On Tue, Nov 19, 2019 at 04:26:04PM +0000, Kieran Bingham wrote:
> On 08/11/2019 20:53, Laurent Pinchart wrote:
> > V4L2 controls of type V4L2_CTRL_TYPE_BOOLEAN are incorrectly described
> > with a ControlRange of int32_t values. Fix it.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/v4l2_controls.cpp | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > index 2462c3e2c264..b6547a7c627c 100644
> > --- a/src/libcamera/v4l2_controls.cpp
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -118,12 +118,22 @@ V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)
> >   */
> >  V4L2ControlRange::V4L2ControlRange(const struct v4l2_query_ext_ctrl &ctrl)
> >  {
> > -	if (ctrl.type == V4L2_CTRL_TYPE_INTEGER64)
> > +	switch (ctrl.type) {
> > +	case V4L2_CTRL_TYPE_BOOLEAN:
> > +		ControlRange::operator=(ControlRange(static_cast<bool>(ctrl.minimum),
> > +						     static_cast<bool>(ctrl.maximum)));
> 
> Hrm shouldn't the minimum and maximum of a bool control be fairly const?
> 
> If so would this be better written as:
> 
>  ControlRange::operator=(ControlRange(false, true));

A V4L2 device could decide to expose a control but only support a single
value. The main use case would be read-only controls that report a
device configuration, but it could also be useful to show that a feature
is only partly implemented/supported.

> Otherwise,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > +		break;
> > +
> > +	case V4L2_CTRL_TYPE_INTEGER64:
> >  		ControlRange::operator=(ControlRange(static_cast<int64_t>(ctrl.minimum),
> >  						     static_cast<int64_t>(ctrl.maximum)));
> > -	else
> > +		break;
> > +
> > +	default:
> >  		ControlRange::operator=(ControlRange(static_cast<int32_t>(ctrl.minimum),
> >  						     static_cast<int32_t>(ctrl.maximum)));
> > +		break;
> > +	}
> >  }
> >  
> >  } /* namespace libcamera */
> > 

-- 
Regards,

Laurent Pinchart