Message ID | 20250128121352.494582-1-pobrn@protonmail.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Quoting Barnabás Pőcze (2025-01-28 12:13:55) > It is expected that the min/max/default/etc. values in a `ControlInfo` > have the same type. So add assertions to check that. > This patch looks like a very good addition, which could have helped catch the issue Naush faced yesterday (https://patchwork.libcamera.org/patch/22778/) Is there anyway we can make this a compile time assertion instead of a runtime check? -- Kieran > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > src/libcamera/controls.cpp | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 70f6f6092..07f276b35 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -580,6 +580,19 @@ ControlId::ControlId(unsigned int id, const std::string &name, > * \brief The Control template type T > */ > > +namespace { > + > +bool sameShape(const ControlValue &a, const ControlValue &b) > +{ > + /** > + * \todo This is a best effort approach. Without the `ControlId` > + * there is no way to check if the sizes of fixed size arrays match. > + */ > + return a.type() == b.type() && a.isArray() == b.isArray(); > +} > + > +} > + > /** > * \class ControlInfo > * \brief Describe the limits of valid values for a Control > @@ -601,6 +614,7 @@ ControlInfo::ControlInfo(const ControlValue &min, > const ControlValue &def) > : min_(min), max_(max), def_(def) > { > + ASSERT(sameShape(min_, max_) && (def_.isNone() || sameShape(max_, def_))); > } > > /** > @@ -616,13 +630,19 @@ ControlInfo::ControlInfo(const ControlValue &min, > ControlInfo::ControlInfo(Span<const ControlValue> values, > const ControlValue &def) > { > + ASSERT(!values.empty()); > + > min_ = values.front(); > max_ = values.back(); > def_ = !def.isNone() ? def : values.front(); > > + ASSERT(sameShape(min_, max_) && sameShape(max_, def_)); > + > values_.reserve(values.size()); > - for (const ControlValue &value : values) > + for (const ControlValue &value : values) { > + ASSERT(sameShape(def_, value)); > values_.push_back(value); > + } > } > > /** > -- > 2.48.1 > >
Hi 2025. február 13., csütörtök 11:57 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > Quoting Barnabás Pőcze (2025-01-28 12:13:55) > > It is expected that the min/max/default/etc. values in a `ControlInfo` > > have the same type. So add assertions to check that. > > > > This patch looks like a very good addition, which could have helped > catch the issue Naush faced yesterday > (https://patchwork.libcamera.org/patch/22778/) > > Is there anyway we can make this a compile time assertion instead of a > runtime check? I think it is definitely possible, at least to some degree. But that would be an appreciably more intrusive change. Regards, Barnabás Pőcze > > -- > Kieran > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > --- > > src/libcamera/controls.cpp | 22 +++++++++++++++++++++- > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > index 70f6f6092..07f276b35 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -580,6 +580,19 @@ ControlId::ControlId(unsigned int id, const std::string &name, > > * \brief The Control template type T > > */ > > > > +namespace { > > + > > +bool sameShape(const ControlValue &a, const ControlValue &b) > > +{ > > + /** > > + * \todo This is a best effort approach. Without the `ControlId` > > + * there is no way to check if the sizes of fixed size arrays match. > > + */ > > + return a.type() == b.type() && a.isArray() == b.isArray(); > > +} > > + > > +} > > + > > /** > > * \class ControlInfo > > * \brief Describe the limits of valid values for a Control > > @@ -601,6 +614,7 @@ ControlInfo::ControlInfo(const ControlValue &min, > > const ControlValue &def) > > : min_(min), max_(max), def_(def) > > { > > + ASSERT(sameShape(min_, max_) && (def_.isNone() || sameShape(max_, def_))); > > } > > > > /** > > @@ -616,13 +630,19 @@ ControlInfo::ControlInfo(const ControlValue &min, > > ControlInfo::ControlInfo(Span<const ControlValue> values, > > const ControlValue &def) > > { > > + ASSERT(!values.empty()); > > + > > min_ = values.front(); > > max_ = values.back(); > > def_ = !def.isNone() ? def : values.front(); > > > > + ASSERT(sameShape(min_, max_) && sameShape(max_, def_)); > > + > > values_.reserve(values.size()); > > - for (const ControlValue &value : values) > > + for (const ControlValue &value : values) { > > + ASSERT(sameShape(def_, value)); > > values_.push_back(value); > > + } > > } > > > > /** > > -- > > 2.48.1 > > > > >
On Thu, Feb 13, 2025 at 04:21:39PM +0000, Barnabás Pőcze wrote: > Hi > > > 2025. február 13., csütörtök 11:57 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > > > Quoting Barnabás Pőcze (2025-01-28 12:13:55) > > > It is expected that the min/max/default/etc. values in a `ControlInfo` > > > have the same type. So add assertions to check that. > > > > > > > This patch looks like a very good addition, which could have helped > > catch the issue Naush faced yesterday > > (https://patchwork.libcamera.org/patch/22778/) > > > > Is there anyway we can make this a compile time assertion instead of a > > runtime check? > > I think it is definitely possible, at least to some degree. But that would be an > appreciably more intrusive change. I wonder how instrusive it would be. We would need to pass a const Control<T> reference to the ControlInfo constructor, and therefore make the constructors inline (part of the constructors could be deferred to a separate private init function, I'm thinking in particular about the Span-based constructor). One possibly upside it that we may be able to replace the ControlValue arguments with const T &, potentially simplifying the callers. I'm sure I'm missing some of the impact this would have. > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > --- > > > src/libcamera/controls.cpp | 22 +++++++++++++++++++++- > > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > index 70f6f6092..07f276b35 100644 > > > --- a/src/libcamera/controls.cpp > > > +++ b/src/libcamera/controls.cpp > > > @@ -580,6 +580,19 @@ ControlId::ControlId(unsigned int id, const std::string &name, > > > * \brief The Control template type T > > > */ > > > > > > +namespace { > > > + > > > +bool sameShape(const ControlValue &a, const ControlValue &b) > > > +{ > > > + /** > > > + * \todo This is a best effort approach. Without the `ControlId` > > > + * there is no way to check if the sizes of fixed size arrays match. > > > + */ > > > + return a.type() == b.type() && a.isArray() == b.isArray(); > > > +} > > > + > > > +} > > > + > > > /** > > > * \class ControlInfo > > > * \brief Describe the limits of valid values for a Control > > > @@ -601,6 +614,7 @@ ControlInfo::ControlInfo(const ControlValue &min, > > > const ControlValue &def) > > > : min_(min), max_(max), def_(def) > > > { > > > + ASSERT(sameShape(min_, max_) && (def_.isNone() || sameShape(max_, def_))); > > > } > > > > > > /** > > > @@ -616,13 +630,19 @@ ControlInfo::ControlInfo(const ControlValue &min, > > > ControlInfo::ControlInfo(Span<const ControlValue> values, > > > const ControlValue &def) > > > { > > > + ASSERT(!values.empty()); > > > + > > > min_ = values.front(); > > > max_ = values.back(); > > > def_ = !def.isNone() ? def : values.front(); > > > > > > + ASSERT(sameShape(min_, max_) && sameShape(max_, def_)); > > > + > > > values_.reserve(values.size()); > > > - for (const ControlValue &value : values) > > > + for (const ControlValue &value : values) { > > > + ASSERT(sameShape(def_, value)); > > > values_.push_back(value); > > > + } > > > } > > > > > > /**
Hi 2025. február 13., csütörtök 21:57 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > On Thu, Feb 13, 2025 at 04:21:39PM +0000, Barnabás Pőcze wrote: > > Hi > > > > > > 2025. február 13., csütörtök 11:57 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > > > > > Quoting Barnabás Pőcze (2025-01-28 12:13:55) > > > > It is expected that the min/max/default/etc. values in a `ControlInfo` > > > > have the same type. So add assertions to check that. > > > > > > > > > > This patch looks like a very good addition, which could have helped > > > catch the issue Naush faced yesterday > > > (https://patchwork.libcamera.org/patch/22778/) > > > > > > Is there anyway we can make this a compile time assertion instead of a > > > runtime check? > > > > I think it is definitely possible, at least to some degree. But that would be an > > appreciably more intrusive change. > > I wonder how instrusive it would be. We would need to pass a const > Control<T> reference to the ControlInfo constructor, and therefore make > the constructors inline (part of the constructors could be deferred to a > separate private init function, I'm thinking in particular about the > Span-based constructor). One possibly upside it that we may be able to > replace the ControlValue arguments with const T &, potentially > simplifying the callers. I'm sure I'm missing some of the impact this > would have. Maybe something like this: template<typename T, typename... Args> ControlInfo(const Control<T>&, std::optional<typename Control<T>::type> def, const Args&... values) { static_assert(sizeof...(values) > 0); static_assert(((details::control_type<T>::value == details::control_type<Args>::value) && ...)); static_assert(((details::control_type<T>::size == details::control_type<Args>::size) && ...)); (values_.emplace_back(values), ...); min_ = values_.front(); max_ = values_.back(); def_ = def ? ControlValue(*def) : values_.front(); } But there are still some things: 1) strings, i.e. handling `const char (&)[N]`, `const char *`, (`std::string_view`), and `std::string` (although there are no string controls at the moment); 2) arrays, i.e. the handling of `vector<T>`, `T (&)[N]`, `span<T>`, etc; 3) the type information about enums is lost, which is not ideal in my opinion. I am wondering if there is anything preventing (3) from being addressed? Regards, Barnabás Pőcze > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > > --- > > > > src/libcamera/controls.cpp | 22 +++++++++++++++++++++- > > > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > > index 70f6f6092..07f276b35 100644 > > > > --- a/src/libcamera/controls.cpp > > > > +++ b/src/libcamera/controls.cpp > > > > @@ -580,6 +580,19 @@ ControlId::ControlId(unsigned int id, const std::string &name, > > > > * \brief The Control template type T > > > > */ > > > > > > > > +namespace { > > > > + > > > > +bool sameShape(const ControlValue &a, const ControlValue &b) > > > > +{ > > > > + /** > > > > + * \todo This is a best effort approach. Without the `ControlId` > > > > + * there is no way to check if the sizes of fixed size arrays match. > > > > + */ > > > > + return a.type() == b.type() && a.isArray() == b.isArray(); > > > > +} > > > > + > > > > +} > > > > + > > > > /** > > > > * \class ControlInfo > > > > * \brief Describe the limits of valid values for a Control > > > > @@ -601,6 +614,7 @@ ControlInfo::ControlInfo(const ControlValue &min, > > > > const ControlValue &def) > > > > : min_(min), max_(max), def_(def) > > > > { > > > > + ASSERT(sameShape(min_, max_) && (def_.isNone() || sameShape(max_, def_))); > > > > } > > > > > > > > /** > > > > @@ -616,13 +630,19 @@ ControlInfo::ControlInfo(const ControlValue &min, > > > > ControlInfo::ControlInfo(Span<const ControlValue> values, > > > > const ControlValue &def) > > > > { > > > > + ASSERT(!values.empty()); > > > > + > > > > min_ = values.front(); > > > > max_ = values.back(); > > > > def_ = !def.isNone() ? def : values.front(); > > > > > > > > + ASSERT(sameShape(min_, max_) && sameShape(max_, def_)); > > > > + > > > > values_.reserve(values.size()); > > > > - for (const ControlValue &value : values) > > > > + for (const ControlValue &value : values) { > > > > + ASSERT(sameShape(def_, value)); > > > > values_.push_back(value); > > > > + } > > > > } > > > > > > > > /** > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 70f6f6092..07f276b35 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -580,6 +580,19 @@ ControlId::ControlId(unsigned int id, const std::string &name, * \brief The Control template type T */ +namespace { + +bool sameShape(const ControlValue &a, const ControlValue &b) +{ + /** + * \todo This is a best effort approach. Without the `ControlId` + * there is no way to check if the sizes of fixed size arrays match. + */ + return a.type() == b.type() && a.isArray() == b.isArray(); +} + +} + /** * \class ControlInfo * \brief Describe the limits of valid values for a Control @@ -601,6 +614,7 @@ ControlInfo::ControlInfo(const ControlValue &min, const ControlValue &def) : min_(min), max_(max), def_(def) { + ASSERT(sameShape(min_, max_) && (def_.isNone() || sameShape(max_, def_))); } /** @@ -616,13 +630,19 @@ ControlInfo::ControlInfo(const ControlValue &min, ControlInfo::ControlInfo(Span<const ControlValue> values, const ControlValue &def) { + ASSERT(!values.empty()); + min_ = values.front(); max_ = values.back(); def_ = !def.isNone() ? def : values.front(); + ASSERT(sameShape(min_, max_) && sameShape(max_, def_)); + values_.reserve(values.size()); - for (const ControlValue &value : values) + for (const ControlValue &value : values) { + ASSERT(sameShape(def_, value)); values_.push_back(value); + } } /**
It is expected that the min/max/default/etc. values in a `ControlInfo` have the same type. So add assertions to check that. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/libcamera/controls.cpp | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)