Message ID | 20191108205409.18845-12-laurent.pinchart@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Laurent, On Fri, Nov 08, 2019 at 10:53:56PM +0200, 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@ideasonboard.com> indeed! Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > 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))); > + 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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, Thanks for your patch. On 2019-11-08 22:53:56 +0200, 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@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > 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))); > + 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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, 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@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)); Otherwise, Reviewed-by: Kieran Bingham <kieran.bingham@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 */ >
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@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@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 */ > >
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))); + 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 */
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@ideasonboard.com> --- src/libcamera/v4l2_controls.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)