<div dir="ltr"><div dir="ltr">Hi Jacopo,<div><br></div><div>Thank you for your feedback!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 12 Dec 2022 at 09:50, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
On Fri, Dec 02, 2022 at 12:40:04PM +0000, Naushir Patuck via libcamera-devel wrote:<br>
> Add a read-only flag to the ControlInfo flag to indicate whether a V4L2 control<br>
> is read-only or not. This flag only makes sense for V2L2 controls at preset, so<br>
> only update the ControlInfo constructor signature used by the V4L2Device class<br>
> to set the value of the flag.<br>
><br>
<br>
It feels a bit the three constructors are mis-aligned now :/<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
I would have gone for a default parameter for all three of them to be<br>
honest... What do others think ?<br></blockquote><div><br></div><div><div>I do kind of agree with this, the constructor signatures were the one bit</div><div>where I was unsure what to do.</div></div><div><br></div><div>Happy to change all the constructors to have a readOnly param if others</div><div>agree!</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> Add a ControlInfo::readOnly() member function to retrive this flag.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> include/libcamera/controls.h | 5 ++++-<br>
> src/libcamera/controls.cpp | 17 +++++++++++++----<br>
> src/libcamera/v4l2_device.cpp | 12 ++++++++----<br>
> 3 files changed, 25 insertions(+), 9 deletions(-)<br>
><br>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h<br>
> index cf94205577a5..488663a7ba04 100644<br>
> --- a/include/libcamera/controls.h<br>
> +++ b/include/libcamera/controls.h<br>
> @@ -270,7 +270,8 @@ class ControlInfo<br>
> public:<br>
> explicit ControlInfo(const ControlValue &min = {},<br>
> const ControlValue &max = {},<br>
> - const ControlValue &def = {});<br>
> + const ControlValue &def = {},<br>
> + bool readOnly = false);<br>
> explicit ControlInfo(Span<const ControlValue> values,<br>
> const ControlValue &def = {});<br>
> explicit ControlInfo(std::set<bool> values, bool def);<br>
> @@ -279,6 +280,7 @@ public:<br>
> const ControlValue &min() const { return min_; }<br>
> const ControlValue &max() const { return max_; }<br>
> const ControlValue &def() const { return def_; }<br>
> + bool readOnly() const { return readOnly_; }<br>
> const std::vector<ControlValue> &values() const { return values_; }<br>
><br>
> std::string toString() const;<br>
> @@ -297,6 +299,7 @@ private:<br>
> ControlValue min_;<br>
> ControlValue max_;<br>
> ControlValue def_;<br>
> + bool readOnly_;<br>
> std::vector<ControlValue> values_;<br>
> };<br>
><br>
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp<br>
> index 6dbf9b348709..fc66abad600d 100644<br>
> --- a/src/libcamera/controls.cpp<br>
> +++ b/src/libcamera/controls.cpp<br>
> @@ -484,11 +484,13 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen<br>
> * \param[in] min The control minimum value<br>
> * \param[in] max The control maximum value<br>
> * \param[in] def The control default value<br>
> + * \param[in] readOnly Read-only status of a V4L2 control<br>
> */<br>
> ControlInfo::ControlInfo(const ControlValue &min,<br>
> const ControlValue &max,<br>
> - const ControlValue &def)<br>
> - : min_(min), max_(max), def_(def)<br>
> + const ControlValue &def,<br>
> + bool readOnly)<br>
> + : min_(min), max_(max), def_(def), readOnly_(readOnly)<br>
> {<br>
> }<br>
><br>
> @@ -525,7 +527,8 @@ ControlInfo::ControlInfo(Span<const ControlValue> values,<br>
> * default value is \a def.<br>
> */<br>
> ControlInfo::ControlInfo(std::set<bool> values, bool def)<br>
> - : min_(false), max_(true), def_(def), values_({ false, true })<br>
> + : min_(false), max_(true), def_(def), readOnly_(false),<br>
> + values_({ false, true })<br>
> {<br>
> ASSERT(values.count(def) && values.size() == 2);<br>
> }<br>
> @@ -538,7 +541,7 @@ ControlInfo::ControlInfo(std::set<bool> values, bool def)<br>
> * value. The minimum, maximum, and default values will all be \a value.<br>
> */<br>
> ControlInfo::ControlInfo(bool value)<br>
> - : min_(value), max_(value), def_(value)<br>
> + : min_(value), max_(value), def_(value), readOnly_(false)<br>
> {<br>
> values_ = { value };<br>
> }<br>
> @@ -571,6 +574,12 @@ ControlInfo::ControlInfo(bool value)<br>
> * \return A ControlValue with the default value for the control<br>
> */<br>
><br>
> +/**<br>
> + * \fn ControlInfo::readOnly()<br>
> + * \brief Identifies if a V4L2 control is flagged as read-only<br>
> + * \return True if the V4L2 control is read-only, false otherwise<br>
<br>
I would not mention V4L2 here and keep the documentation generic<br>
<br>
> + */<br>
> +<br>
> /**<br>
> * \fn ControlInfo::values()<br>
> * \brief Retrieve the list of valid values<br>
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp<br>
> index 57a88d96b12c..9018f1b0b9e1 100644<br>
> --- a/src/libcamera/v4l2_device.cpp<br>
> +++ b/src/libcamera/v4l2_device.cpp<br>
> @@ -535,17 +535,20 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl<br>
> case V4L2_CTRL_TYPE_U8:<br>
> return ControlInfo(static_cast<uint8_t>(ctrl.minimum),<br>
> static_cast<uint8_t>(ctrl.maximum),<br>
> - static_cast<uint8_t>(ctrl.default_value));<br>
> + static_cast<uint8_t>(ctrl.default_value),<br>
> + !!(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY));<br>
><br>
> case V4L2_CTRL_TYPE_BOOLEAN:<br>
> return ControlInfo(static_cast<bool>(ctrl.minimum),<br>
> static_cast<bool>(ctrl.maximum),<br>
> - static_cast<bool>(ctrl.default_value));<br>
> + static_cast<bool>(ctrl.default_value),<br>
> + !!(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY));<br>
><br>
> case V4L2_CTRL_TYPE_INTEGER64:<br>
> return ControlInfo(static_cast<int64_t>(ctrl.minimum),<br>
> static_cast<int64_t>(ctrl.maximum),<br>
> - static_cast<int64_t>(ctrl.default_value));<br>
> + static_cast<int64_t>(ctrl.default_value),<br>
> + !!(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY));<br>
><br>
> case V4L2_CTRL_TYPE_INTEGER_MENU:<br>
> case V4L2_CTRL_TYPE_MENU:<br>
> @@ -554,7 +557,8 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl<br>
> default:<br>
> return ControlInfo(static_cast<int32_t>(ctrl.minimum),<br>
> static_cast<int32_t>(ctrl.maximum),<br>
> - static_cast<int32_t>(ctrl.default_value));<br>
> + static_cast<int32_t>(ctrl.default_value),<br>
> + !!(ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY));<br>
> }<br>
> }<br>
><br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>