| Message ID | 20241025153948.1213629-2-chenghaoyang@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey On Fri, Oct 25, 2024 at 03:38:50PM +0000, Harvey Yang wrote: > From: Yudhistira Erlandinata <yerlandinata@chromium.org> > > V4L2 Controls support a wide variety of types not yet supported by the > ControlValue type system. > > Extend the libcamera ControlValue types to support an explicit 32 bit > unsigned integer type, and map that to the corresponding > V4L2_CTRL_TYPE_U32 type within the v4l2_device support class. > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > --- > include/libcamera/controls.h | 7 ++++++ > src/libcamera/controls.cpp | 12 ++++++++-- > src/libcamera/v4l2_device.cpp | 14 ++++++++++++ > test/controls/control_value.cpp | 40 +++++++++++++++++++++++++++++++++ > 4 files changed, 71 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index ca60bbaca..6da8ad2c3 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -29,6 +29,7 @@ enum ControlType { > ControlTypeNone, > ControlTypeBool, > ControlTypeByte, > + ControlTypeUnsigned32, > ControlTypeInteger32, > ControlTypeInteger64, > ControlTypeFloat, > @@ -62,6 +63,12 @@ struct control_type<uint8_t> { > static constexpr std::size_t size = 0; > }; > > +template<> > +struct control_type<uint32_t> { > + static constexpr ControlType value = ControlTypeUnsigned32; > + static constexpr std::size_t size = 0; > +}; > + > template<> > struct control_type<int32_t> { > static constexpr ControlType value = ControlTypeInteger32; > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 62185d643..8ae295191 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -54,6 +54,7 @@ static constexpr size_t ControlValueSize[] = { > [ControlTypeNone] = 0, > [ControlTypeBool] = sizeof(bool), > [ControlTypeByte] = sizeof(uint8_t), > + [ControlTypeUnsigned32] = sizeof(uint32_t), > [ControlTypeInteger32] = sizeof(int32_t), > [ControlTypeInteger64] = sizeof(int64_t), > [ControlTypeFloat] = sizeof(float), > @@ -74,10 +75,12 @@ static constexpr size_t ControlValueSize[] = { > * The control stores a boolean value > * \var ControlTypeByte > * The control stores a byte value as an unsigned 8-bit integer > + * \var ControlTypeUnsigned32 > + * The control stores an unsigned 32-bit integer value > * \var ControlTypeInteger32 > - * The control stores a 32-bit integer value > + * The control stores a signed 32-bit integer value > * \var ControlTypeInteger64 > - * The control stores a 64-bit integer value > + * The control stores a signed 64-bit integer value > * \var ControlTypeFloat > * The control stores a 32-bit floating point value > * \var ControlTypeString > @@ -230,6 +233,11 @@ std::string ControlValue::toString() const > str += std::to_string(*value); > break; > } > + case ControlTypeUnsigned32: { > + const uint32_t *value = reinterpret_cast<const uint32_t *>(data); > + str += std::to_string(*value); > + break; > + } > case ControlTypeInteger32: { > const int32_t *value = reinterpret_cast<const int32_t *>(data); > str += std::to_string(*value); > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 68add4f2e..f39ec3760 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -9,6 +9,7 @@ > > #include <fcntl.h> > #include <map> > +#include <stdint.h> > #include <stdlib.h> > #include <string.h> > #include <sys/ioctl.h> > @@ -210,6 +211,10 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > type = ControlTypeByte; > break; > > + case V4L2_CTRL_TYPE_U32: > + type = ControlTypeUnsigned32; > + break; > + This is not enough. After the switch we have ControlValue &value = ctrl.second; value.reserve(type, true, info.elems); Span<uint8_t> data = value.data(); v4l2Ctrl.p_u8 = data.data(); v4l2Ctrl.size = data.size(); However controls of type V4L2_CTRL_TYPE_U32 use the p_u32 union member of 'struct v4l2_ext_control' https://www.kernel.org/doc/html/v4.9/media/uapi/v4l/vidioc-g-ext-ctrls.html#id1 Same for U16 control types. Be careful about one thing. v4l2Ctrl.p_u32 is of type __u32* so it would feel natural to Span<uint32_t> data = value.data(); v4l2Ctrl.p_u32 = data.data(); as data.data() returns a T* However v4l2Ctrl.size wants the size in bytes while data.size() returns the number of elements, so you should probably use data.size_bytes(). Or you can do as we do in setControls and cast the data.data() pointer. Span<uint8_t> data = value.data(); v4l2Ctrl.p_u32 = reinterpret_cast<uint32_t *>(data.data()); v4l2Ctrl.size = data.size(); Also, you need to handle U32 and U16 also setControl(). If you can test this by reading and writing u16 and u32 controls this would make me way more confident this patch is correct. Thanks j > default: > LOG(V4L2, Error) > << "Unsupported payload control type " > @@ -488,6 +493,9 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType) > case V4L2_CTRL_TYPE_BOOLEAN: > return ControlTypeBool; > > + case V4L2_CTRL_TYPE_U32: > + return ControlTypeUnsigned32; > + > case V4L2_CTRL_TYPE_INTEGER: > return ControlTypeInteger32; > > @@ -536,6 +544,11 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl > static_cast<uint8_t>(ctrl.maximum), > static_cast<uint8_t>(ctrl.default_value)); > > + case V4L2_CTRL_TYPE_U32: > + return ControlInfo(static_cast<uint32_t>(ctrl.minimum), > + static_cast<uint32_t>(ctrl.maximum), > + static_cast<uint32_t>(ctrl.default_value)); > + > case V4L2_CTRL_TYPE_BOOLEAN: > return ControlInfo(static_cast<bool>(ctrl.minimum), > static_cast<bool>(ctrl.maximum), > @@ -622,6 +635,7 @@ void V4L2Device::listControls() > case V4L2_CTRL_TYPE_BITMASK: > case V4L2_CTRL_TYPE_INTEGER_MENU: > case V4L2_CTRL_TYPE_U8: > + case V4L2_CTRL_TYPE_U32: > break; > /* \todo Support other control types. */ > default: > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > index 344107fae..6ca85b739 100644 > --- a/test/controls/control_value.cpp > +++ b/test/controls/control_value.cpp > @@ -109,6 +109,46 @@ protected: > return TestFail; > } > > + /* > + * Unsigned Integer32 type. > + */ > + value.set(static_cast<uint32_t>(42)); > + if (value.isNone() || value.isArray() || > + value.type() != ControlTypeUnsigned32) { > + cerr << "Control type mismatch after setting to uint32_t" << endl; > + return TestFail; > + } > + > + if (value.get<uint32_t>() != 42) { > + cerr << "Control value mismatch after setting to uint32_t" << endl; > + return TestFail; > + } > + > + if (value.toString() != "42") { > + cerr << "Control string mismatch after setting to uint32_t" << endl; > + return TestFail; > + } > + > + std::array<uint32_t, 4> uint32s{ 3, 14, 15, 9 }; > + value.set(Span<uint32_t>(uint32s)); > + if (value.isNone() || !value.isArray() || > + value.type() != ControlTypeUnsigned32) { > + cerr << "Control type mismatch after setting to uint32_t array" << endl; > + return TestFail; > + } > + > + Span<const uint32_t> uint32sResult = value.get<Span<const uint32_t>>(); > + if (uint32s.size() != uint32sResult.size() || > + !std::equal(uint32s.begin(), uint32s.end(), uint32sResult.begin())) { > + cerr << "Control value mismatch after setting to uint32_t array" << endl; > + return TestFail; > + } > + > + if (value.toString() != "[ 3, 14, 15, 9 ]") { > + cerr << "Control string mismatch after setting to uint32_t array" << endl; > + return TestFail; > + } > + > /* > * Integer32 type. > */ > -- > 2.47.0.163.g1226f6d8fa-goog >
Hi Jacopo, On Mon, Oct 28, 2024 at 6:36 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Harvey > > On Fri, Oct 25, 2024 at 03:38:50PM +0000, Harvey Yang wrote: > > From: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > > V4L2 Controls support a wide variety of types not yet supported by the > > ControlValue type system. > > > > Extend the libcamera ControlValue types to support an explicit 32 bit > > unsigned integer type, and map that to the corresponding > > V4L2_CTRL_TYPE_U32 type within the v4l2_device support class. > > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > --- > > include/libcamera/controls.h | 7 ++++++ > > src/libcamera/controls.cpp | 12 ++++++++-- > > src/libcamera/v4l2_device.cpp | 14 ++++++++++++ > > test/controls/control_value.cpp | 40 +++++++++++++++++++++++++++++++++ > > 4 files changed, 71 insertions(+), 2 deletions(-) > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > index ca60bbaca..6da8ad2c3 100644 > > --- a/include/libcamera/controls.h > > +++ b/include/libcamera/controls.h > > @@ -29,6 +29,7 @@ enum ControlType { > > ControlTypeNone, > > ControlTypeBool, > > ControlTypeByte, > > + ControlTypeUnsigned32, > > ControlTypeInteger32, > > ControlTypeInteger64, > > ControlTypeFloat, > > @@ -62,6 +63,12 @@ struct control_type<uint8_t> { > > static constexpr std::size_t size = 0; > > }; > > > > +template<> > > +struct control_type<uint32_t> { > > + static constexpr ControlType value = ControlTypeUnsigned32; > > + static constexpr std::size_t size = 0; > > +}; > > + > > template<> > > struct control_type<int32_t> { > > static constexpr ControlType value = ControlTypeInteger32; > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > index 62185d643..8ae295191 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -54,6 +54,7 @@ static constexpr size_t ControlValueSize[] = { > > [ControlTypeNone] = 0, > > [ControlTypeBool] = sizeof(bool), > > [ControlTypeByte] = sizeof(uint8_t), > > + [ControlTypeUnsigned32] = sizeof(uint32_t), > > [ControlTypeInteger32] = sizeof(int32_t), > > [ControlTypeInteger64] = sizeof(int64_t), > > [ControlTypeFloat] = sizeof(float), > > @@ -74,10 +75,12 @@ static constexpr size_t ControlValueSize[] = { > > * The control stores a boolean value > > * \var ControlTypeByte > > * The control stores a byte value as an unsigned 8-bit integer > > + * \var ControlTypeUnsigned32 > > + * The control stores an unsigned 32-bit integer value > > * \var ControlTypeInteger32 > > - * The control stores a 32-bit integer value > > + * The control stores a signed 32-bit integer value > > * \var ControlTypeInteger64 > > - * The control stores a 64-bit integer value > > + * The control stores a signed 64-bit integer value > > * \var ControlTypeFloat > > * The control stores a 32-bit floating point value > > * \var ControlTypeString > > @@ -230,6 +233,11 @@ std::string ControlValue::toString() const > > str += std::to_string(*value); > > break; > > } > > + case ControlTypeUnsigned32: { > > + const uint32_t *value = reinterpret_cast<const uint32_t *>(data); > > + str += std::to_string(*value); > > + break; > > + } > > case ControlTypeInteger32: { > > const int32_t *value = reinterpret_cast<const int32_t *>(data); > > str += std::to_string(*value); > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 68add4f2e..f39ec3760 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -9,6 +9,7 @@ > > > > #include <fcntl.h> > > #include <map> > > +#include <stdint.h> > > #include <stdlib.h> > > #include <string.h> > > #include <sys/ioctl.h> > > @@ -210,6 +211,10 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > type = ControlTypeByte; > > break; > > > > + case V4L2_CTRL_TYPE_U32: > > + type = ControlTypeUnsigned32; > > + break; > > + > > This is not enough. > > After the switch we have > > ControlValue &value = ctrl.second; > value.reserve(type, true, info.elems); > Span<uint8_t> data = value.data(); > > v4l2Ctrl.p_u8 = data.data(); > v4l2Ctrl.size = data.size(); > > However controls of type V4L2_CTRL_TYPE_U32 use the p_u32 union member > of 'struct v4l2_ext_control' > https://www.kernel.org/doc/html/v4.9/media/uapi/v4l/vidioc-g-ext-ctrls.html#id1 > > Same for U16 control types. > > Be careful about one thing. v4l2Ctrl.p_u32 is of type __u32* so it > would feel natural to > > Span<uint32_t> data = value.data(); > v4l2Ctrl.p_u32 = data.data(); > > as data.data() returns a T* > > However v4l2Ctrl.size wants the size in bytes while data.size() > returns the number of elements, so you should probably use > data.size_bytes(). > > Or you can do as we do in setControls and cast the data.data() > pointer. > > Span<uint8_t> data = value.data(); > v4l2Ctrl.p_u32 = reinterpret_cast<uint32_t *>(data.data()); > v4l2Ctrl.size = data.size(); > > Also, you need to handle U32 and U16 also setControl(). Thanks for the catch! Please help take a look at my WIP patch: https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/commit/faec47f605881062e58718371183a0c2d3ffe772 , before I upload another version. Let me know if you think I should merge the two `switch`es and duplicate the code in the middle. > > If you can test this by reading and writing u16 and u32 controls this > would make me way more confident this patch is correct. Hmm, I couldn't find such a use case. Can you suggest how we could test it? BR, Harvey > > Thanks > j > > > default: > > LOG(V4L2, Error) > > << "Unsupported payload control type " > > @@ -488,6 +493,9 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType) > > case V4L2_CTRL_TYPE_BOOLEAN: > > return ControlTypeBool; > > > > + case V4L2_CTRL_TYPE_U32: > > + return ControlTypeUnsigned32; > > + > > case V4L2_CTRL_TYPE_INTEGER: > > return ControlTypeInteger32; > > > > @@ -536,6 +544,11 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl > > static_cast<uint8_t>(ctrl.maximum), > > static_cast<uint8_t>(ctrl.default_value)); > > > > + case V4L2_CTRL_TYPE_U32: > > + return ControlInfo(static_cast<uint32_t>(ctrl.minimum), > > + static_cast<uint32_t>(ctrl.maximum), > > + static_cast<uint32_t>(ctrl.default_value)); > > + > > case V4L2_CTRL_TYPE_BOOLEAN: > > return ControlInfo(static_cast<bool>(ctrl.minimum), > > static_cast<bool>(ctrl.maximum), > > @@ -622,6 +635,7 @@ void V4L2Device::listControls() > > case V4L2_CTRL_TYPE_BITMASK: > > case V4L2_CTRL_TYPE_INTEGER_MENU: > > case V4L2_CTRL_TYPE_U8: > > + case V4L2_CTRL_TYPE_U32: > > break; > > /* \todo Support other control types. */ > > default: > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > > index 344107fae..6ca85b739 100644 > > --- a/test/controls/control_value.cpp > > +++ b/test/controls/control_value.cpp > > @@ -109,6 +109,46 @@ protected: > > return TestFail; > > } > > > > + /* > > + * Unsigned Integer32 type. > > + */ > > + value.set(static_cast<uint32_t>(42)); > > + if (value.isNone() || value.isArray() || > > + value.type() != ControlTypeUnsigned32) { > > + cerr << "Control type mismatch after setting to uint32_t" << endl; > > + return TestFail; > > + } > > + > > + if (value.get<uint32_t>() != 42) { > > + cerr << "Control value mismatch after setting to uint32_t" << endl; > > + return TestFail; > > + } > > + > > + if (value.toString() != "42") { > > + cerr << "Control string mismatch after setting to uint32_t" << endl; > > + return TestFail; > > + } > > + > > + std::array<uint32_t, 4> uint32s{ 3, 14, 15, 9 }; > > + value.set(Span<uint32_t>(uint32s)); > > + if (value.isNone() || !value.isArray() || > > + value.type() != ControlTypeUnsigned32) { > > + cerr << "Control type mismatch after setting to uint32_t array" << endl; > > + return TestFail; > > + } > > + > > + Span<const uint32_t> uint32sResult = value.get<Span<const uint32_t>>(); > > + if (uint32s.size() != uint32sResult.size() || > > + !std::equal(uint32s.begin(), uint32s.end(), uint32sResult.begin())) { > > + cerr << "Control value mismatch after setting to uint32_t array" << endl; > > + return TestFail; > > + } > > + > > + if (value.toString() != "[ 3, 14, 15, 9 ]") { > > + cerr << "Control string mismatch after setting to uint32_t array" << endl; > > + return TestFail; > > + } > > + > > /* > > * Integer32 type. > > */ > > -- > > 2.47.0.163.g1226f6d8fa-goog > >
On Tue, Oct 29, 2024 at 06:08:44PM +0800, Cheng-Hao Yang wrote: > Hi Jacopo, > > On Mon, Oct 28, 2024 at 6:36 PM Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Harvey > > > > On Fri, Oct 25, 2024 at 03:38:50PM +0000, Harvey Yang wrote: > > > From: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > > > > V4L2 Controls support a wide variety of types not yet supported by the > > > ControlValue type system. > > > > > > Extend the libcamera ControlValue types to support an explicit 32 bit > > > unsigned integer type, and map that to the corresponding > > > V4L2_CTRL_TYPE_U32 type within the v4l2_device support class. > > > > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > --- > > > include/libcamera/controls.h | 7 ++++++ > > > src/libcamera/controls.cpp | 12 ++++++++-- > > > src/libcamera/v4l2_device.cpp | 14 ++++++++++++ > > > test/controls/control_value.cpp | 40 +++++++++++++++++++++++++++++++++ > > > 4 files changed, 71 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > index ca60bbaca..6da8ad2c3 100644 > > > --- a/include/libcamera/controls.h > > > +++ b/include/libcamera/controls.h > > > @@ -29,6 +29,7 @@ enum ControlType { > > > ControlTypeNone, > > > ControlTypeBool, > > > ControlTypeByte, > > > + ControlTypeUnsigned32, > > > ControlTypeInteger32, > > > ControlTypeInteger64, > > > ControlTypeFloat, > > > @@ -62,6 +63,12 @@ struct control_type<uint8_t> { > > > static constexpr std::size_t size = 0; > > > }; > > > > > > +template<> > > > +struct control_type<uint32_t> { > > > + static constexpr ControlType value = ControlTypeUnsigned32; > > > + static constexpr std::size_t size = 0; > > > +}; > > > + > > > template<> > > > struct control_type<int32_t> { > > > static constexpr ControlType value = ControlTypeInteger32; > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > index 62185d643..8ae295191 100644 > > > --- a/src/libcamera/controls.cpp > > > +++ b/src/libcamera/controls.cpp > > > @@ -54,6 +54,7 @@ static constexpr size_t ControlValueSize[] = { > > > [ControlTypeNone] = 0, > > > [ControlTypeBool] = sizeof(bool), > > > [ControlTypeByte] = sizeof(uint8_t), > > > + [ControlTypeUnsigned32] = sizeof(uint32_t), > > > [ControlTypeInteger32] = sizeof(int32_t), > > > [ControlTypeInteger64] = sizeof(int64_t), > > > [ControlTypeFloat] = sizeof(float), > > > @@ -74,10 +75,12 @@ static constexpr size_t ControlValueSize[] = { > > > * The control stores a boolean value > > > * \var ControlTypeByte > > > * The control stores a byte value as an unsigned 8-bit integer > > > + * \var ControlTypeUnsigned32 > > > + * The control stores an unsigned 32-bit integer value > > > * \var ControlTypeInteger32 > > > - * The control stores a 32-bit integer value > > > + * The control stores a signed 32-bit integer value > > > * \var ControlTypeInteger64 > > > - * The control stores a 64-bit integer value > > > + * The control stores a signed 64-bit integer value > > > * \var ControlTypeFloat > > > * The control stores a 32-bit floating point value > > > * \var ControlTypeString > > > @@ -230,6 +233,11 @@ std::string ControlValue::toString() const > > > str += std::to_string(*value); > > > break; > > > } > > > + case ControlTypeUnsigned32: { > > > + const uint32_t *value = reinterpret_cast<const uint32_t *>(data); > > > + str += std::to_string(*value); > > > + break; > > > + } > > > case ControlTypeInteger32: { > > > const int32_t *value = reinterpret_cast<const int32_t *>(data); > > > str += std::to_string(*value); > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > index 68add4f2e..f39ec3760 100644 > > > --- a/src/libcamera/v4l2_device.cpp > > > +++ b/src/libcamera/v4l2_device.cpp > > > @@ -9,6 +9,7 @@ > > > > > > #include <fcntl.h> > > > #include <map> > > > +#include <stdint.h> > > > #include <stdlib.h> > > > #include <string.h> > > > #include <sys/ioctl.h> > > > @@ -210,6 +211,10 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > type = ControlTypeByte; > > > break; > > > > > > + case V4L2_CTRL_TYPE_U32: > > > + type = ControlTypeUnsigned32; > > > + break; > > > + > > > > This is not enough. > > > > After the switch we have > > > > ControlValue &value = ctrl.second; > > value.reserve(type, true, info.elems); > > Span<uint8_t> data = value.data(); > > > > v4l2Ctrl.p_u8 = data.data(); > > v4l2Ctrl.size = data.size(); > > > > However controls of type V4L2_CTRL_TYPE_U32 use the p_u32 union member > > of 'struct v4l2_ext_control' > > https://www.kernel.org/doc/html/v4.9/media/uapi/v4l/vidioc-g-ext-ctrls.html#id1 > > > > Same for U16 control types. > > > > Be careful about one thing. v4l2Ctrl.p_u32 is of type __u32* so it > > would feel natural to > > > > Span<uint32_t> data = value.data(); > > v4l2Ctrl.p_u32 = data.data(); > > > > as data.data() returns a T* > > > > However v4l2Ctrl.size wants the size in bytes while data.size() > > returns the number of elements, so you should probably use > > data.size_bytes(). > > > > Or you can do as we do in setControls and cast the data.data() > > pointer. > > > > Span<uint8_t> data = value.data(); > > v4l2Ctrl.p_u32 = reinterpret_cast<uint32_t *>(data.data()); > > v4l2Ctrl.size = data.size(); > > > > Also, you need to handle U32 and U16 also setControl(). > > Thanks for the catch! > Please help take a look at my WIP patch: > https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/commit/faec47f605881062e58718371183a0c2d3ffe772 > , before I upload another version. > Let me know if you think I should merge the two `switch`es and duplicate > the code in the middle. You would have to copy value.reserve(type, true, info.elems); Span<uint8_t> data = value.data(); In the two switch branches but you can keep v4l2Ctrl.size = data.size(); After the switch. Up to you, I would prefer a single switch but I don't mind too much > > > > > If you can test this by reading and writing u16 and u32 controls this > > would make me way more confident this patch is correct. > > Hmm, I couldn't find such a use case. Can you suggest how we could > test it? Well, I presume you have a device with a u16/u32 control if you sent this :) Just calling getControls(), trying to set them to some value and reading them back would validate the implementation. Or have I missed your question ? Thanks j > > BR, > Harvey > > > > > Thanks > > j > > > > > default: > > > LOG(V4L2, Error) > > > << "Unsupported payload control type " > > > @@ -488,6 +493,9 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType) > > > case V4L2_CTRL_TYPE_BOOLEAN: > > > return ControlTypeBool; > > > > > > + case V4L2_CTRL_TYPE_U32: > > > + return ControlTypeUnsigned32; > > > + > > > case V4L2_CTRL_TYPE_INTEGER: > > > return ControlTypeInteger32; > > > > > > @@ -536,6 +544,11 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl > > > static_cast<uint8_t>(ctrl.maximum), > > > static_cast<uint8_t>(ctrl.default_value)); > > > > > > + case V4L2_CTRL_TYPE_U32: > > > + return ControlInfo(static_cast<uint32_t>(ctrl.minimum), > > > + static_cast<uint32_t>(ctrl.maximum), > > > + static_cast<uint32_t>(ctrl.default_value)); > > > + > > > case V4L2_CTRL_TYPE_BOOLEAN: > > > return ControlInfo(static_cast<bool>(ctrl.minimum), > > > static_cast<bool>(ctrl.maximum), > > > @@ -622,6 +635,7 @@ void V4L2Device::listControls() > > > case V4L2_CTRL_TYPE_BITMASK: > > > case V4L2_CTRL_TYPE_INTEGER_MENU: > > > case V4L2_CTRL_TYPE_U8: > > > + case V4L2_CTRL_TYPE_U32: > > > break; > > > /* \todo Support other control types. */ > > > default: > > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > > > index 344107fae..6ca85b739 100644 > > > --- a/test/controls/control_value.cpp > > > +++ b/test/controls/control_value.cpp > > > @@ -109,6 +109,46 @@ protected: > > > return TestFail; > > > } > > > > > > + /* > > > + * Unsigned Integer32 type. > > > + */ > > > + value.set(static_cast<uint32_t>(42)); > > > + if (value.isNone() || value.isArray() || > > > + value.type() != ControlTypeUnsigned32) { > > > + cerr << "Control type mismatch after setting to uint32_t" << endl; > > > + return TestFail; > > > + } > > > + > > > + if (value.get<uint32_t>() != 42) { > > > + cerr << "Control value mismatch after setting to uint32_t" << endl; > > > + return TestFail; > > > + } > > > + > > > + if (value.toString() != "42") { > > > + cerr << "Control string mismatch after setting to uint32_t" << endl; > > > + return TestFail; > > > + } > > > + > > > + std::array<uint32_t, 4> uint32s{ 3, 14, 15, 9 }; > > > + value.set(Span<uint32_t>(uint32s)); > > > + if (value.isNone() || !value.isArray() || > > > + value.type() != ControlTypeUnsigned32) { > > > + cerr << "Control type mismatch after setting to uint32_t array" << endl; > > > + return TestFail; > > > + } > > > + > > > + Span<const uint32_t> uint32sResult = value.get<Span<const uint32_t>>(); > > > + if (uint32s.size() != uint32sResult.size() || > > > + !std::equal(uint32s.begin(), uint32s.end(), uint32sResult.begin())) { > > > + cerr << "Control value mismatch after setting to uint32_t array" << endl; > > > + return TestFail; > > > + } > > > + > > > + if (value.toString() != "[ 3, 14, 15, 9 ]") { > > > + cerr << "Control string mismatch after setting to uint32_t array" << endl; > > > + return TestFail; > > > + } > > > + > > > /* > > > * Integer32 type. > > > */ > > > -- > > > 2.47.0.163.g1226f6d8fa-goog > > >
Hi Jacopo, On Tue, Oct 29, 2024 at 6:33 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > > On Tue, Oct 29, 2024 at 06:08:44PM +0800, Cheng-Hao Yang wrote: > > Hi Jacopo, > > > > On Mon, Oct 28, 2024 at 6:36 PM Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Harvey > > > > > > On Fri, Oct 25, 2024 at 03:38:50PM +0000, Harvey Yang wrote: > > > > From: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > > > > > > V4L2 Controls support a wide variety of types not yet supported by the > > > > ControlValue type system. > > > > > > > > Extend the libcamera ControlValue types to support an explicit 32 bit > > > > unsigned integer type, and map that to the corresponding > > > > V4L2_CTRL_TYPE_U32 type within the v4l2_device support class. > > > > > > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > > --- > > > > include/libcamera/controls.h | 7 ++++++ > > > > src/libcamera/controls.cpp | 12 ++++++++-- > > > > src/libcamera/v4l2_device.cpp | 14 ++++++++++++ > > > > test/controls/control_value.cpp | 40 +++++++++++++++++++++++++++++++++ > > > > 4 files changed, 71 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > > index ca60bbaca..6da8ad2c3 100644 > > > > --- a/include/libcamera/controls.h > > > > +++ b/include/libcamera/controls.h > > > > @@ -29,6 +29,7 @@ enum ControlType { > > > > ControlTypeNone, > > > > ControlTypeBool, > > > > ControlTypeByte, > > > > + ControlTypeUnsigned32, > > > > ControlTypeInteger32, > > > > ControlTypeInteger64, > > > > ControlTypeFloat, > > > > @@ -62,6 +63,12 @@ struct control_type<uint8_t> { > > > > static constexpr std::size_t size = 0; > > > > }; > > > > > > > > +template<> > > > > +struct control_type<uint32_t> { > > > > + static constexpr ControlType value = ControlTypeUnsigned32; > > > > + static constexpr std::size_t size = 0; > > > > +}; > > > > + > > > > template<> > > > > struct control_type<int32_t> { > > > > static constexpr ControlType value = ControlTypeInteger32; > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > > index 62185d643..8ae295191 100644 > > > > --- a/src/libcamera/controls.cpp > > > > +++ b/src/libcamera/controls.cpp > > > > @@ -54,6 +54,7 @@ static constexpr size_t ControlValueSize[] = { > > > > [ControlTypeNone] = 0, > > > > [ControlTypeBool] = sizeof(bool), > > > > [ControlTypeByte] = sizeof(uint8_t), > > > > + [ControlTypeUnsigned32] = sizeof(uint32_t), > > > > [ControlTypeInteger32] = sizeof(int32_t), > > > > [ControlTypeInteger64] = sizeof(int64_t), > > > > [ControlTypeFloat] = sizeof(float), > > > > @@ -74,10 +75,12 @@ static constexpr size_t ControlValueSize[] = { > > > > * The control stores a boolean value > > > > * \var ControlTypeByte > > > > * The control stores a byte value as an unsigned 8-bit integer > > > > + * \var ControlTypeUnsigned32 > > > > + * The control stores an unsigned 32-bit integer value > > > > * \var ControlTypeInteger32 > > > > - * The control stores a 32-bit integer value > > > > + * The control stores a signed 32-bit integer value > > > > * \var ControlTypeInteger64 > > > > - * The control stores a 64-bit integer value > > > > + * The control stores a signed 64-bit integer value > > > > * \var ControlTypeFloat > > > > * The control stores a 32-bit floating point value > > > > * \var ControlTypeString > > > > @@ -230,6 +233,11 @@ std::string ControlValue::toString() const > > > > str += std::to_string(*value); > > > > break; > > > > } > > > > + case ControlTypeUnsigned32: { > > > > + const uint32_t *value = reinterpret_cast<const uint32_t *>(data); > > > > + str += std::to_string(*value); > > > > + break; > > > > + } > > > > case ControlTypeInteger32: { > > > > const int32_t *value = reinterpret_cast<const int32_t *>(data); > > > > str += std::to_string(*value); > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > > index 68add4f2e..f39ec3760 100644 > > > > --- a/src/libcamera/v4l2_device.cpp > > > > +++ b/src/libcamera/v4l2_device.cpp > > > > @@ -9,6 +9,7 @@ > > > > > > > > #include <fcntl.h> > > > > #include <map> > > > > +#include <stdint.h> > > > > #include <stdlib.h> > > > > #include <string.h> > > > > #include <sys/ioctl.h> > > > > @@ -210,6 +211,10 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > > type = ControlTypeByte; > > > > break; > > > > > > > > + case V4L2_CTRL_TYPE_U32: > > > > + type = ControlTypeUnsigned32; > > > > + break; > > > > + > > > > > > This is not enough. > > > > > > After the switch we have > > > > > > ControlValue &value = ctrl.second; > > > value.reserve(type, true, info.elems); > > > Span<uint8_t> data = value.data(); > > > > > > v4l2Ctrl.p_u8 = data.data(); > > > v4l2Ctrl.size = data.size(); > > > > > > However controls of type V4L2_CTRL_TYPE_U32 use the p_u32 union member > > > of 'struct v4l2_ext_control' > > > https://www.kernel.org/doc/html/v4.9/media/uapi/v4l/vidioc-g-ext-ctrls.html#id1 > > > > > > Same for U16 control types. > > > > > > Be careful about one thing. v4l2Ctrl.p_u32 is of type __u32* so it > > > would feel natural to > > > > > > Span<uint32_t> data = value.data(); > > > v4l2Ctrl.p_u32 = data.data(); > > > > > > as data.data() returns a T* > > > > > > However v4l2Ctrl.size wants the size in bytes while data.size() > > > returns the number of elements, so you should probably use > > > data.size_bytes(). > > > > > > Or you can do as we do in setControls and cast the data.data() > > > pointer. > > > > > > Span<uint8_t> data = value.data(); > > > v4l2Ctrl.p_u32 = reinterpret_cast<uint32_t *>(data.data()); > > > v4l2Ctrl.size = data.size(); > > > > > > Also, you need to handle U32 and U16 also setControl(). > > > > Thanks for the catch! > > Please help take a look at my WIP patch: > > https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/commit/faec47f605881062e58718371183a0c2d3ffe772 > > , before I upload another version. > > Let me know if you think I should merge the two `switch`es and duplicate > > the code in the middle. > > > You would have to copy > > value.reserve(type, true, info.elems); > Span<uint8_t> data = value.data(); > > In the two switch branches but you can keep > > v4l2Ctrl.size = data.size(); > > After the switch. Done. > > Up to you, I would prefer a single switch but I don't mind too much > > > > > > > > > If you can test this by reading and writing u16 and u32 controls this > > > would make me way more confident this patch is correct. > > > > Hmm, I couldn't find such a use case. Can you suggest how we could > > test it? > > Well, I presume you have a device with a u16/u32 control if you sent > this :) Just calling getControls(), trying to set them to some value > and reading them back would validate the implementation. Or have I > missed your question ? Actually, the two new types are not being used in setControls() / getControls(). u32, for example, is only used for listControls with VIDIOC_QUERY_EXT_CTRL [1], and V4L2Device needs to recognize u32. That's also why I didn't have the changes in setControls() / getControls(). BR, Harvey [1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/v4l2_device.cpp;l=681 > > Thanks > j > > > > > BR, > > Harvey > > > > > > > > Thanks > > > j > > > > > > > default: > > > > LOG(V4L2, Error) > > > > << "Unsupported payload control type " > > > > @@ -488,6 +493,9 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType) > > > > case V4L2_CTRL_TYPE_BOOLEAN: > > > > return ControlTypeBool; > > > > > > > > + case V4L2_CTRL_TYPE_U32: > > > > + return ControlTypeUnsigned32; > > > > + > > > > case V4L2_CTRL_TYPE_INTEGER: > > > > return ControlTypeInteger32; > > > > > > > > @@ -536,6 +544,11 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl > > > > static_cast<uint8_t>(ctrl.maximum), > > > > static_cast<uint8_t>(ctrl.default_value)); > > > > > > > > + case V4L2_CTRL_TYPE_U32: > > > > + return ControlInfo(static_cast<uint32_t>(ctrl.minimum), > > > > + static_cast<uint32_t>(ctrl.maximum), > > > > + static_cast<uint32_t>(ctrl.default_value)); > > > > + > > > > case V4L2_CTRL_TYPE_BOOLEAN: > > > > return ControlInfo(static_cast<bool>(ctrl.minimum), > > > > static_cast<bool>(ctrl.maximum), > > > > @@ -622,6 +635,7 @@ void V4L2Device::listControls() > > > > case V4L2_CTRL_TYPE_BITMASK: > > > > case V4L2_CTRL_TYPE_INTEGER_MENU: > > > > case V4L2_CTRL_TYPE_U8: > > > > + case V4L2_CTRL_TYPE_U32: > > > > break; > > > > /* \todo Support other control types. */ > > > > default: > > > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > > > > index 344107fae..6ca85b739 100644 > > > > --- a/test/controls/control_value.cpp > > > > +++ b/test/controls/control_value.cpp > > > > @@ -109,6 +109,46 @@ protected: > > > > return TestFail; > > > > } > > > > > > > > + /* > > > > + * Unsigned Integer32 type. > > > > + */ > > > > + value.set(static_cast<uint32_t>(42)); > > > > + if (value.isNone() || value.isArray() || > > > > + value.type() != ControlTypeUnsigned32) { > > > > + cerr << "Control type mismatch after setting to uint32_t" << endl; > > > > + return TestFail; > > > > + } > > > > + > > > > + if (value.get<uint32_t>() != 42) { > > > > + cerr << "Control value mismatch after setting to uint32_t" << endl; > > > > + return TestFail; > > > > + } > > > > + > > > > + if (value.toString() != "42") { > > > > + cerr << "Control string mismatch after setting to uint32_t" << endl; > > > > + return TestFail; > > > > + } > > > > + > > > > + std::array<uint32_t, 4> uint32s{ 3, 14, 15, 9 }; > > > > + value.set(Span<uint32_t>(uint32s)); > > > > + if (value.isNone() || !value.isArray() || > > > > + value.type() != ControlTypeUnsigned32) { > > > > + cerr << "Control type mismatch after setting to uint32_t array" << endl; > > > > + return TestFail; > > > > + } > > > > + > > > > + Span<const uint32_t> uint32sResult = value.get<Span<const uint32_t>>(); > > > > + if (uint32s.size() != uint32sResult.size() || > > > > + !std::equal(uint32s.begin(), uint32s.end(), uint32sResult.begin())) { > > > > + cerr << "Control value mismatch after setting to uint32_t array" << endl; > > > > + return TestFail; > > > > + } > > > > + > > > > + if (value.toString() != "[ 3, 14, 15, 9 ]") { > > > > + cerr << "Control string mismatch after setting to uint32_t array" << endl; > > > > + return TestFail; > > > > + } > > > > + > > > > /* > > > > * Integer32 type. > > > > */ > > > > -- > > > > 2.47.0.163.g1226f6d8fa-goog > > > >
Hi Harvey, On Wed, Oct 30, 2024 at 12:11:12AM +0800, Cheng-Hao Yang wrote: > On Tue, Oct 29, 2024 at 6:33 PM Jacopo Mondi wrote: > > On Tue, Oct 29, 2024 at 06:08:44PM +0800, Cheng-Hao Yang wrote: > > > On Mon, Oct 28, 2024 at 6:36 PM Jacopo Mondi wrote: > > > > On Fri, Oct 25, 2024 at 03:38:50PM +0000, Harvey Yang wrote: > > > > > From: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > > > > > > > > V4L2 Controls support a wide variety of types not yet supported by the > > > > > ControlValue type system. > > > > > > > > > > Extend the libcamera ControlValue types to support an explicit 32 bit > > > > > unsigned integer type, and map that to the corresponding > > > > > V4L2_CTRL_TYPE_U32 type within the v4l2_device support class. > > > > > > > > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > > > --- > > > > > include/libcamera/controls.h | 7 ++++++ > > > > > src/libcamera/controls.cpp | 12 ++++++++-- > > > > > src/libcamera/v4l2_device.cpp | 14 ++++++++++++ > > > > > test/controls/control_value.cpp | 40 +++++++++++++++++++++++++++++++++ > > > > > 4 files changed, 71 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > > > index ca60bbaca..6da8ad2c3 100644 > > > > > --- a/include/libcamera/controls.h > > > > > +++ b/include/libcamera/controls.h > > > > > @@ -29,6 +29,7 @@ enum ControlType { > > > > > ControlTypeNone, > > > > > ControlTypeBool, > > > > > ControlTypeByte, > > > > > + ControlTypeUnsigned32, > > > > > ControlTypeInteger32, > > > > > ControlTypeInteger64, > > > > > ControlTypeFloat, > > > > > @@ -62,6 +63,12 @@ struct control_type<uint8_t> { > > > > > static constexpr std::size_t size = 0; > > > > > }; > > > > > > > > > > +template<> > > > > > +struct control_type<uint32_t> { > > > > > + static constexpr ControlType value = ControlTypeUnsigned32; > > > > > + static constexpr std::size_t size = 0; > > > > > +}; > > > > > + > > > > > template<> > > > > > struct control_type<int32_t> { > > > > > static constexpr ControlType value = ControlTypeInteger32; > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > > > index 62185d643..8ae295191 100644 > > > > > --- a/src/libcamera/controls.cpp > > > > > +++ b/src/libcamera/controls.cpp > > > > > @@ -54,6 +54,7 @@ static constexpr size_t ControlValueSize[] = { > > > > > [ControlTypeNone] = 0, > > > > > [ControlTypeBool] = sizeof(bool), > > > > > [ControlTypeByte] = sizeof(uint8_t), > > > > > + [ControlTypeUnsigned32] = sizeof(uint32_t), > > > > > [ControlTypeInteger32] = sizeof(int32_t), > > > > > [ControlTypeInteger64] = sizeof(int64_t), > > > > > [ControlTypeFloat] = sizeof(float), > > > > > @@ -74,10 +75,12 @@ static constexpr size_t ControlValueSize[] = { > > > > > * The control stores a boolean value > > > > > * \var ControlTypeByte > > > > > * The control stores a byte value as an unsigned 8-bit integer > > > > > + * \var ControlTypeUnsigned32 > > > > > + * The control stores an unsigned 32-bit integer value > > > > > * \var ControlTypeInteger32 > > > > > - * The control stores a 32-bit integer value > > > > > + * The control stores a signed 32-bit integer value > > > > > * \var ControlTypeInteger64 > > > > > - * The control stores a 64-bit integer value > > > > > + * The control stores a signed 64-bit integer value > > > > > * \var ControlTypeFloat > > > > > * The control stores a 32-bit floating point value > > > > > * \var ControlTypeString > > > > > @@ -230,6 +233,11 @@ std::string ControlValue::toString() const > > > > > str += std::to_string(*value); > > > > > break; > > > > > } > > > > > + case ControlTypeUnsigned32: { > > > > > + const uint32_t *value = reinterpret_cast<const uint32_t *>(data); > > > > > + str += std::to_string(*value); > > > > > + break; > > > > > + } > > > > > case ControlTypeInteger32: { > > > > > const int32_t *value = reinterpret_cast<const int32_t *>(data); > > > > > str += std::to_string(*value); > > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > > > index 68add4f2e..f39ec3760 100644 > > > > > --- a/src/libcamera/v4l2_device.cpp > > > > > +++ b/src/libcamera/v4l2_device.cpp > > > > > @@ -9,6 +9,7 @@ > > > > > > > > > > #include <fcntl.h> > > > > > #include <map> > > > > > +#include <stdint.h> > > > > > #include <stdlib.h> > > > > > #include <string.h> > > > > > #include <sys/ioctl.h> > > > > > @@ -210,6 +211,10 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > > > type = ControlTypeByte; > > > > > break; > > > > > > > > > > + case V4L2_CTRL_TYPE_U32: > > > > > + type = ControlTypeUnsigned32; > > > > > + break; > > > > > + > > > > > > > > This is not enough. > > > > > > > > After the switch we have > > > > > > > > ControlValue &value = ctrl.second; > > > > value.reserve(type, true, info.elems); > > > > Span<uint8_t> data = value.data(); > > > > > > > > v4l2Ctrl.p_u8 = data.data(); > > > > v4l2Ctrl.size = data.size(); > > > > > > > > However controls of type V4L2_CTRL_TYPE_U32 use the p_u32 union member > > > > of 'struct v4l2_ext_control' > > > > https://www.kernel.org/doc/html/v4.9/media/uapi/v4l/vidioc-g-ext-ctrls.html#id1 > > > > > > > > Same for U16 control types. > > > > > > > > Be careful about one thing. v4l2Ctrl.p_u32 is of type __u32* so it > > > > would feel natural to > > > > > > > > Span<uint32_t> data = value.data(); > > > > v4l2Ctrl.p_u32 = data.data(); > > > > > > > > as data.data() returns a T* > > > > > > > > However v4l2Ctrl.size wants the size in bytes while data.size() > > > > returns the number of elements, so you should probably use > > > > data.size_bytes(). > > > > > > > > Or you can do as we do in setControls and cast the data.data() > > > > pointer. > > > > > > > > Span<uint8_t> data = value.data(); > > > > v4l2Ctrl.p_u32 = reinterpret_cast<uint32_t *>(data.data()); > > > > v4l2Ctrl.size = data.size(); > > > > > > > > Also, you need to handle U32 and U16 also setControl(). > > > > > > Thanks for the catch! > > > Please help take a look at my WIP patch: > > > https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/commit/faec47f605881062e58718371183a0c2d3ffe772 > > > , before I upload another version. > > > Let me know if you think I should merge the two `switch`es and duplicate > > > the code in the middle. > > > > > > You would have to copy > > > > value.reserve(type, true, info.elems); > > Span<uint8_t> data = value.data(); > > > > In the two switch branches but you can keep > > > > v4l2Ctrl.size = data.size(); > > > > After the switch. > > Done. > > > Up to you, I would prefer a single switch but I don't mind too much > > > > > > If you can test this by reading and writing u16 and u32 controls this > > > > would make me way more confident this patch is correct. > > > > > > Hmm, I couldn't find such a use case. Can you suggest how we could > > > test it? > > > > Well, I presume you have a device with a u16/u32 control if you sent > > this :) Just calling getControls(), trying to set them to some value > > and reading them back would validate the implementation. Or have I > > missed your question ? > > Actually, the two new types are not being used in setControls() / getControls(). > u32, for example, is only used for listControls with VIDIOC_QUERY_EXT_CTRL [1], > and V4L2Device needs to recognize u32. But surely that's used with a device ? Can you point to the driver that exposes these controls ? > That's also why I didn't have the changes in setControls() / getControls(). > > BR, > Harvey > > [1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/v4l2_device.cpp;l=681 > > > > > > default: > > > > > LOG(V4L2, Error) > > > > > << "Unsupported payload control type " > > > > > @@ -488,6 +493,9 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType) > > > > > case V4L2_CTRL_TYPE_BOOLEAN: > > > > > return ControlTypeBool; > > > > > > > > > > + case V4L2_CTRL_TYPE_U32: > > > > > + return ControlTypeUnsigned32; > > > > > + > > > > > case V4L2_CTRL_TYPE_INTEGER: > > > > > return ControlTypeInteger32; > > > > > > > > > > @@ -536,6 +544,11 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl > > > > > static_cast<uint8_t>(ctrl.maximum), > > > > > static_cast<uint8_t>(ctrl.default_value)); > > > > > > > > > > + case V4L2_CTRL_TYPE_U32: > > > > > + return ControlInfo(static_cast<uint32_t>(ctrl.minimum), > > > > > + static_cast<uint32_t>(ctrl.maximum), > > > > > + static_cast<uint32_t>(ctrl.default_value)); > > > > > + > > > > > case V4L2_CTRL_TYPE_BOOLEAN: > > > > > return ControlInfo(static_cast<bool>(ctrl.minimum), > > > > > static_cast<bool>(ctrl.maximum), > > > > > @@ -622,6 +635,7 @@ void V4L2Device::listControls() > > > > > case V4L2_CTRL_TYPE_BITMASK: > > > > > case V4L2_CTRL_TYPE_INTEGER_MENU: > > > > > case V4L2_CTRL_TYPE_U8: > > > > > + case V4L2_CTRL_TYPE_U32: > > > > > break; > > > > > /* \todo Support other control types. */ > > > > > default: > > > > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > > > > > index 344107fae..6ca85b739 100644 > > > > > --- a/test/controls/control_value.cpp > > > > > +++ b/test/controls/control_value.cpp > > > > > @@ -109,6 +109,46 @@ protected: > > > > > return TestFail; > > > > > } > > > > > > > > > > + /* > > > > > + * Unsigned Integer32 type. > > > > > + */ > > > > > + value.set(static_cast<uint32_t>(42)); > > > > > + if (value.isNone() || value.isArray() || > > > > > + value.type() != ControlTypeUnsigned32) { > > > > > + cerr << "Control type mismatch after setting to uint32_t" << endl; > > > > > + return TestFail; > > > > > + } > > > > > + > > > > > + if (value.get<uint32_t>() != 42) { > > > > > + cerr << "Control value mismatch after setting to uint32_t" << endl; > > > > > + return TestFail; > > > > > + } > > > > > + > > > > > + if (value.toString() != "42") { > > > > > + cerr << "Control string mismatch after setting to uint32_t" << endl; > > > > > + return TestFail; > > > > > + } > > > > > + > > > > > + std::array<uint32_t, 4> uint32s{ 3, 14, 15, 9 }; > > > > > + value.set(Span<uint32_t>(uint32s)); > > > > > + if (value.isNone() || !value.isArray() || > > > > > + value.type() != ControlTypeUnsigned32) { > > > > > + cerr << "Control type mismatch after setting to uint32_t array" << endl; > > > > > + return TestFail; > > > > > + } > > > > > + > > > > > + Span<const uint32_t> uint32sResult = value.get<Span<const uint32_t>>(); > > > > > + if (uint32s.size() != uint32sResult.size() || > > > > > + !std::equal(uint32s.begin(), uint32s.end(), uint32sResult.begin())) { > > > > > + cerr << "Control value mismatch after setting to uint32_t array" << endl; > > > > > + return TestFail; > > > > > + } > > > > > + > > > > > + if (value.toString() != "[ 3, 14, 15, 9 ]") { > > > > > + cerr << "Control string mismatch after setting to uint32_t array" << endl; > > > > > + return TestFail; > > > > > + } > > > > > + > > > > > /* > > > > > * Integer32 type. > > > > > */
Hi Laurent, On Wed, Oct 30, 2024 at 12:14 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Harvey, > > On Wed, Oct 30, 2024 at 12:11:12AM +0800, Cheng-Hao Yang wrote: > > On Tue, Oct 29, 2024 at 6:33 PM Jacopo Mondi wrote: > > > On Tue, Oct 29, 2024 at 06:08:44PM +0800, Cheng-Hao Yang wrote: > > > > On Mon, Oct 28, 2024 at 6:36 PM Jacopo Mondi wrote: > > > > > On Fri, Oct 25, 2024 at 03:38:50PM +0000, Harvey Yang wrote: > > > > > > From: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > > > > > > > > > > V4L2 Controls support a wide variety of types not yet supported by the > > > > > > ControlValue type system. > > > > > > > > > > > > Extend the libcamera ControlValue types to support an explicit 32 bit > > > > > > unsigned integer type, and map that to the corresponding > > > > > > V4L2_CTRL_TYPE_U32 type within the v4l2_device support class. > > > > > > > > > > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > > > > --- > > > > > > include/libcamera/controls.h | 7 ++++++ > > > > > > src/libcamera/controls.cpp | 12 ++++++++-- > > > > > > src/libcamera/v4l2_device.cpp | 14 ++++++++++++ > > > > > > test/controls/control_value.cpp | 40 +++++++++++++++++++++++++++++++++ > > > > > > 4 files changed, 71 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > > > > index ca60bbaca..6da8ad2c3 100644 > > > > > > --- a/include/libcamera/controls.h > > > > > > +++ b/include/libcamera/controls.h > > > > > > @@ -29,6 +29,7 @@ enum ControlType { > > > > > > ControlTypeNone, > > > > > > ControlTypeBool, > > > > > > ControlTypeByte, > > > > > > + ControlTypeUnsigned32, > > > > > > ControlTypeInteger32, > > > > > > ControlTypeInteger64, > > > > > > ControlTypeFloat, > > > > > > @@ -62,6 +63,12 @@ struct control_type<uint8_t> { > > > > > > static constexpr std::size_t size = 0; > > > > > > }; > > > > > > > > > > > > +template<> > > > > > > +struct control_type<uint32_t> { > > > > > > + static constexpr ControlType value = ControlTypeUnsigned32; > > > > > > + static constexpr std::size_t size = 0; > > > > > > +}; > > > > > > + > > > > > > template<> > > > > > > struct control_type<int32_t> { > > > > > > static constexpr ControlType value = ControlTypeInteger32; > > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > > > > index 62185d643..8ae295191 100644 > > > > > > --- a/src/libcamera/controls.cpp > > > > > > +++ b/src/libcamera/controls.cpp > > > > > > @@ -54,6 +54,7 @@ static constexpr size_t ControlValueSize[] = { > > > > > > [ControlTypeNone] = 0, > > > > > > [ControlTypeBool] = sizeof(bool), > > > > > > [ControlTypeByte] = sizeof(uint8_t), > > > > > > + [ControlTypeUnsigned32] = sizeof(uint32_t), > > > > > > [ControlTypeInteger32] = sizeof(int32_t), > > > > > > [ControlTypeInteger64] = sizeof(int64_t), > > > > > > [ControlTypeFloat] = sizeof(float), > > > > > > @@ -74,10 +75,12 @@ static constexpr size_t ControlValueSize[] = { > > > > > > * The control stores a boolean value > > > > > > * \var ControlTypeByte > > > > > > * The control stores a byte value as an unsigned 8-bit integer > > > > > > + * \var ControlTypeUnsigned32 > > > > > > + * The control stores an unsigned 32-bit integer value > > > > > > * \var ControlTypeInteger32 > > > > > > - * The control stores a 32-bit integer value > > > > > > + * The control stores a signed 32-bit integer value > > > > > > * \var ControlTypeInteger64 > > > > > > - * The control stores a 64-bit integer value > > > > > > + * The control stores a signed 64-bit integer value > > > > > > * \var ControlTypeFloat > > > > > > * The control stores a 32-bit floating point value > > > > > > * \var ControlTypeString > > > > > > @@ -230,6 +233,11 @@ std::string ControlValue::toString() const > > > > > > str += std::to_string(*value); > > > > > > break; > > > > > > } > > > > > > + case ControlTypeUnsigned32: { > > > > > > + const uint32_t *value = reinterpret_cast<const uint32_t *>(data); > > > > > > + str += std::to_string(*value); > > > > > > + break; > > > > > > + } > > > > > > case ControlTypeInteger32: { > > > > > > const int32_t *value = reinterpret_cast<const int32_t *>(data); > > > > > > str += std::to_string(*value); > > > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > > > > index 68add4f2e..f39ec3760 100644 > > > > > > --- a/src/libcamera/v4l2_device.cpp > > > > > > +++ b/src/libcamera/v4l2_device.cpp > > > > > > @@ -9,6 +9,7 @@ > > > > > > > > > > > > #include <fcntl.h> > > > > > > #include <map> > > > > > > +#include <stdint.h> > > > > > > #include <stdlib.h> > > > > > > #include <string.h> > > > > > > #include <sys/ioctl.h> > > > > > > @@ -210,6 +211,10 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > > > > type = ControlTypeByte; > > > > > > break; > > > > > > > > > > > > + case V4L2_CTRL_TYPE_U32: > > > > > > + type = ControlTypeUnsigned32; > > > > > > + break; > > > > > > + > > > > > > > > > > This is not enough. > > > > > > > > > > After the switch we have > > > > > > > > > > ControlValue &value = ctrl.second; > > > > > value.reserve(type, true, info.elems); > > > > > Span<uint8_t> data = value.data(); > > > > > > > > > > v4l2Ctrl.p_u8 = data.data(); > > > > > v4l2Ctrl.size = data.size(); > > > > > > > > > > However controls of type V4L2_CTRL_TYPE_U32 use the p_u32 union member > > > > > of 'struct v4l2_ext_control' > > > > > https://www.kernel.org/doc/html/v4.9/media/uapi/v4l/vidioc-g-ext-ctrls.html#id1 > > > > > > > > > > Same for U16 control types. > > > > > > > > > > Be careful about one thing. v4l2Ctrl.p_u32 is of type __u32* so it > > > > > would feel natural to > > > > > > > > > > Span<uint32_t> data = value.data(); > > > > > v4l2Ctrl.p_u32 = data.data(); > > > > > > > > > > as data.data() returns a T* > > > > > > > > > > However v4l2Ctrl.size wants the size in bytes while data.size() > > > > > returns the number of elements, so you should probably use > > > > > data.size_bytes(). > > > > > > > > > > Or you can do as we do in setControls and cast the data.data() > > > > > pointer. > > > > > > > > > > Span<uint8_t> data = value.data(); > > > > > v4l2Ctrl.p_u32 = reinterpret_cast<uint32_t *>(data.data()); > > > > > v4l2Ctrl.size = data.size(); > > > > > > > > > > Also, you need to handle U32 and U16 also setControl(). > > > > > > > > Thanks for the catch! > > > > Please help take a look at my WIP patch: > > > > https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/commit/faec47f605881062e58718371183a0c2d3ffe772 > > > > , before I upload another version. > > > > Let me know if you think I should merge the two `switch`es and duplicate > > > > the code in the middle. > > > > > > > > > You would have to copy > > > > > > value.reserve(type, true, info.elems); > > > Span<uint8_t> data = value.data(); > > > > > > In the two switch branches but you can keep > > > > > > v4l2Ctrl.size = data.size(); > > > > > > After the switch. > > > > Done. > > > > > Up to you, I would prefer a single switch but I don't mind too much > > > > > > > > If you can test this by reading and writing u16 and u32 controls this > > > > > would make me way more confident this patch is correct. > > > > > > > > Hmm, I couldn't find such a use case. Can you suggest how we could > > > > test it? > > > > > > Well, I presume you have a device with a u16/u32 control if you sent > > > this :) Just calling getControls(), trying to set them to some value > > > and reading them back would validate the implementation. Or have I > > > missed your question ? > > > > Actually, the two new types are not being used in setControls() / getControls(). > > u32, for example, is only used for listControls with VIDIOC_QUERY_EXT_CTRL [1], > > and V4L2Device needs to recognize u32. > > But surely that's used with a device ? Can you point to the driver that > exposes these controls ? Yes, the failure happens in Aie [1] , and `sourceVideo_` should be "mtk-aie-5.3-source" [2]. Let me know if I missed anything. BR, Harvey [1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/face_detect/aie.cpp;l=156 [2]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/face_detect/aie.cpp;l=121 > > > That's also why I didn't have the changes in setControls() / getControls(). > > > > BR, > > Harvey > > > > [1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/v4l2_device.cpp;l=681 > > > > > > > > default: > > > > > > LOG(V4L2, Error) > > > > > > << "Unsupported payload control type " > > > > > > @@ -488,6 +493,9 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType) > > > > > > case V4L2_CTRL_TYPE_BOOLEAN: > > > > > > return ControlTypeBool; > > > > > > > > > > > > + case V4L2_CTRL_TYPE_U32: > > > > > > + return ControlTypeUnsigned32; > > > > > > + > > > > > > case V4L2_CTRL_TYPE_INTEGER: > > > > > > return ControlTypeInteger32; > > > > > > > > > > > > @@ -536,6 +544,11 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl > > > > > > static_cast<uint8_t>(ctrl.maximum), > > > > > > static_cast<uint8_t>(ctrl.default_value)); > > > > > > > > > > > > + case V4L2_CTRL_TYPE_U32: > > > > > > + return ControlInfo(static_cast<uint32_t>(ctrl.minimum), > > > > > > + static_cast<uint32_t>(ctrl.maximum), > > > > > > + static_cast<uint32_t>(ctrl.default_value)); > > > > > > + > > > > > > case V4L2_CTRL_TYPE_BOOLEAN: > > > > > > return ControlInfo(static_cast<bool>(ctrl.minimum), > > > > > > static_cast<bool>(ctrl.maximum), > > > > > > @@ -622,6 +635,7 @@ void V4L2Device::listControls() > > > > > > case V4L2_CTRL_TYPE_BITMASK: > > > > > > case V4L2_CTRL_TYPE_INTEGER_MENU: > > > > > > case V4L2_CTRL_TYPE_U8: > > > > > > + case V4L2_CTRL_TYPE_U32: > > > > > > break; > > > > > > /* \todo Support other control types. */ > > > > > > default: > > > > > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > > > > > > index 344107fae..6ca85b739 100644 > > > > > > --- a/test/controls/control_value.cpp > > > > > > +++ b/test/controls/control_value.cpp > > > > > > @@ -109,6 +109,46 @@ protected: > > > > > > return TestFail; > > > > > > } > > > > > > > > > > > > + /* > > > > > > + * Unsigned Integer32 type. > > > > > > + */ > > > > > > + value.set(static_cast<uint32_t>(42)); > > > > > > + if (value.isNone() || value.isArray() || > > > > > > + value.type() != ControlTypeUnsigned32) { > > > > > > + cerr << "Control type mismatch after setting to uint32_t" << endl; > > > > > > + return TestFail; > > > > > > + } > > > > > > + > > > > > > + if (value.get<uint32_t>() != 42) { > > > > > > + cerr << "Control value mismatch after setting to uint32_t" << endl; > > > > > > + return TestFail; > > > > > > + } > > > > > > + > > > > > > + if (value.toString() != "42") { > > > > > > + cerr << "Control string mismatch after setting to uint32_t" << endl; > > > > > > + return TestFail; > > > > > > + } > > > > > > + > > > > > > + std::array<uint32_t, 4> uint32s{ 3, 14, 15, 9 }; > > > > > > + value.set(Span<uint32_t>(uint32s)); > > > > > > + if (value.isNone() || !value.isArray() || > > > > > > + value.type() != ControlTypeUnsigned32) { > > > > > > + cerr << "Control type mismatch after setting to uint32_t array" << endl; > > > > > > + return TestFail; > > > > > > + } > > > > > > + > > > > > > + Span<const uint32_t> uint32sResult = value.get<Span<const uint32_t>>(); > > > > > > + if (uint32s.size() != uint32sResult.size() || > > > > > > + !std::equal(uint32s.begin(), uint32s.end(), uint32sResult.begin())) { > > > > > > + cerr << "Control value mismatch after setting to uint32_t array" << endl; > > > > > > + return TestFail; > > > > > > + } > > > > > > + > > > > > > + if (value.toString() != "[ 3, 14, 15, 9 ]") { > > > > > > + cerr << "Control string mismatch after setting to uint32_t array" << endl; > > > > > > + return TestFail; > > > > > > + } > > > > > > + > > > > > > /* > > > > > > * Integer32 type. > > > > > > */ > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index ca60bbaca..6da8ad2c3 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -29,6 +29,7 @@ enum ControlType { ControlTypeNone, ControlTypeBool, ControlTypeByte, + ControlTypeUnsigned32, ControlTypeInteger32, ControlTypeInteger64, ControlTypeFloat, @@ -62,6 +63,12 @@ struct control_type<uint8_t> { static constexpr std::size_t size = 0; }; +template<> +struct control_type<uint32_t> { + static constexpr ControlType value = ControlTypeUnsigned32; + static constexpr std::size_t size = 0; +}; + template<> struct control_type<int32_t> { static constexpr ControlType value = ControlTypeInteger32; diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 62185d643..8ae295191 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -54,6 +54,7 @@ static constexpr size_t ControlValueSize[] = { [ControlTypeNone] = 0, [ControlTypeBool] = sizeof(bool), [ControlTypeByte] = sizeof(uint8_t), + [ControlTypeUnsigned32] = sizeof(uint32_t), [ControlTypeInteger32] = sizeof(int32_t), [ControlTypeInteger64] = sizeof(int64_t), [ControlTypeFloat] = sizeof(float), @@ -74,10 +75,12 @@ static constexpr size_t ControlValueSize[] = { * The control stores a boolean value * \var ControlTypeByte * The control stores a byte value as an unsigned 8-bit integer + * \var ControlTypeUnsigned32 + * The control stores an unsigned 32-bit integer value * \var ControlTypeInteger32 - * The control stores a 32-bit integer value + * The control stores a signed 32-bit integer value * \var ControlTypeInteger64 - * The control stores a 64-bit integer value + * The control stores a signed 64-bit integer value * \var ControlTypeFloat * The control stores a 32-bit floating point value * \var ControlTypeString @@ -230,6 +233,11 @@ std::string ControlValue::toString() const str += std::to_string(*value); break; } + case ControlTypeUnsigned32: { + const uint32_t *value = reinterpret_cast<const uint32_t *>(data); + str += std::to_string(*value); + break; + } case ControlTypeInteger32: { const int32_t *value = reinterpret_cast<const int32_t *>(data); str += std::to_string(*value); diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 68add4f2e..f39ec3760 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -9,6 +9,7 @@ #include <fcntl.h> #include <map> +#include <stdint.h> #include <stdlib.h> #include <string.h> #include <sys/ioctl.h> @@ -210,6 +211,10 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) type = ControlTypeByte; break; + case V4L2_CTRL_TYPE_U32: + type = ControlTypeUnsigned32; + break; + default: LOG(V4L2, Error) << "Unsupported payload control type " @@ -488,6 +493,9 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType) case V4L2_CTRL_TYPE_BOOLEAN: return ControlTypeBool; + case V4L2_CTRL_TYPE_U32: + return ControlTypeUnsigned32; + case V4L2_CTRL_TYPE_INTEGER: return ControlTypeInteger32; @@ -536,6 +544,11 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl static_cast<uint8_t>(ctrl.maximum), static_cast<uint8_t>(ctrl.default_value)); + case V4L2_CTRL_TYPE_U32: + return ControlInfo(static_cast<uint32_t>(ctrl.minimum), + static_cast<uint32_t>(ctrl.maximum), + static_cast<uint32_t>(ctrl.default_value)); + case V4L2_CTRL_TYPE_BOOLEAN: return ControlInfo(static_cast<bool>(ctrl.minimum), static_cast<bool>(ctrl.maximum), @@ -622,6 +635,7 @@ void V4L2Device::listControls() case V4L2_CTRL_TYPE_BITMASK: case V4L2_CTRL_TYPE_INTEGER_MENU: case V4L2_CTRL_TYPE_U8: + case V4L2_CTRL_TYPE_U32: break; /* \todo Support other control types. */ default: diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp index 344107fae..6ca85b739 100644 --- a/test/controls/control_value.cpp +++ b/test/controls/control_value.cpp @@ -109,6 +109,46 @@ protected: return TestFail; } + /* + * Unsigned Integer32 type. + */ + value.set(static_cast<uint32_t>(42)); + if (value.isNone() || value.isArray() || + value.type() != ControlTypeUnsigned32) { + cerr << "Control type mismatch after setting to uint32_t" << endl; + return TestFail; + } + + if (value.get<uint32_t>() != 42) { + cerr << "Control value mismatch after setting to uint32_t" << endl; + return TestFail; + } + + if (value.toString() != "42") { + cerr << "Control string mismatch after setting to uint32_t" << endl; + return TestFail; + } + + std::array<uint32_t, 4> uint32s{ 3, 14, 15, 9 }; + value.set(Span<uint32_t>(uint32s)); + if (value.isNone() || !value.isArray() || + value.type() != ControlTypeUnsigned32) { + cerr << "Control type mismatch after setting to uint32_t array" << endl; + return TestFail; + } + + Span<const uint32_t> uint32sResult = value.get<Span<const uint32_t>>(); + if (uint32s.size() != uint32sResult.size() || + !std::equal(uint32s.begin(), uint32s.end(), uint32sResult.begin())) { + cerr << "Control value mismatch after setting to uint32_t array" << endl; + return TestFail; + } + + if (value.toString() != "[ 3, 14, 15, 9 ]") { + cerr << "Control string mismatch after setting to uint32_t array" << endl; + return TestFail; + } + /* * Integer32 type. */