| Message ID | 20191108205409.18845-11-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi LAurent On Fri, Nov 08, 2019 at 10:53:55PM +0200, Laurent Pinchart wrote: > ControlInfoMap requires the ControlId and ControlRange of each entry to > have identical types. Check for this and log an error if a mismatch is > detected. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/controls.cpp | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index eae0250a92e3..c488b2e4eb3f 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -571,8 +571,19 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const > void ControlInfoMap::generateIdmap() > { > idmap_.clear(); > - for (const auto &ctrl : *this) > + > + for (const auto &ctrl : *this) { > + if (ctrl.first->type() != ctrl.second.min().type()) { > + LOG(Controls, Error) > + << "Control " << utils::hex(ctrl.first->id()) > + << " type and range type mismatch"; > + idmap_.clear(); > + clear(); > + return; > + } > + Should we just skip the control or actually abort the whole id map generation ? > idmap_[ctrl.first->id()] = ctrl.first; > + } > } > > /** > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Fri, Nov 15, 2019 at 05:21:29PM +0100, Jacopo Mondi wrote: > On Fri, Nov 08, 2019 at 10:53:55PM +0200, Laurent Pinchart wrote: > > ControlInfoMap requires the ControlId and ControlRange of each entry to > > have identical types. Check for this and log an error if a mismatch is > > detected. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/controls.cpp | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > index eae0250a92e3..c488b2e4eb3f 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -571,8 +571,19 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const > > void ControlInfoMap::generateIdmap() > > { > > idmap_.clear(); > > - for (const auto &ctrl : *this) > > + > > + for (const auto &ctrl : *this) { > > + if (ctrl.first->type() != ctrl.second.min().type()) { > > + LOG(Controls, Error) > > + << "Control " << utils::hex(ctrl.first->id()) > > + << " type and range type mismatch"; > > + idmap_.clear(); > > + clear(); > > + return; > > + } > > + > > Should we just skip the control or actually abort the whole id map > generation ? As this is a serious error, I think we should abort it completely, in order to show that something went wrong. If you think we should just skip the control, we can discuss that. > > idmap_[ctrl.first->id()] = ctrl.first; > > + } > > } > > > > /**
Hi Laurent, Thanks for your work. On 2019-11-08 22:53:55 +0200, Laurent Pinchart wrote: > ControlInfoMap requires the ControlId and ControlRange of each entry to > have identical types. Check for this and log an error if a mismatch is > detected. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/controls.cpp | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index eae0250a92e3..c488b2e4eb3f 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -571,8 +571,19 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const > void ControlInfoMap::generateIdmap() > { > idmap_.clear(); > - for (const auto &ctrl : *this) > + > + for (const auto &ctrl : *this) { > + if (ctrl.first->type() != ctrl.second.min().type()) { > + LOG(Controls, Error) > + << "Control " << utils::hex(ctrl.first->id()) > + << " type and range type mismatch"; > + idmap_.clear(); > + clear(); > + return; > + } > + > idmap_[ctrl.first->id()] = ctrl.first; > + } > } > > /** > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On Mon, Nov 18, 2019 at 03:01:33AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Fri, Nov 15, 2019 at 05:21:29PM +0100, Jacopo Mondi wrote: > > On Fri, Nov 08, 2019 at 10:53:55PM +0200, Laurent Pinchart wrote: > > > ControlInfoMap requires the ControlId and ControlRange of each entry to > > > have identical types. Check for this and log an error if a mismatch is > > > detected. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/libcamera/controls.cpp | 13 ++++++++++++- > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > index eae0250a92e3..c488b2e4eb3f 100644 > > > --- a/src/libcamera/controls.cpp > > > +++ b/src/libcamera/controls.cpp > > > @@ -571,8 +571,19 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const > > > void ControlInfoMap::generateIdmap() > > > { > > > idmap_.clear(); > > > - for (const auto &ctrl : *this) > > > + > > > + for (const auto &ctrl : *this) { > > > + if (ctrl.first->type() != ctrl.second.min().type()) { > > > + LOG(Controls, Error) > > > + << "Control " << utils::hex(ctrl.first->id()) > > > + << " type and range type mismatch"; > > > + idmap_.clear(); > > > + clear(); > > > + return; > > > + } > > > + > > > > Should we just skip the control or actually abort the whole id map > > generation ? > > As this is a serious error, I think we should abort it completely, in > order to show that something went wrong. If you think we should just > skip the control, we can discuss that. > No, it's fine.. If this happens there has been a problem during development, is not something a user could trigger.. In this case it's better to fail loudly Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > > idmap_[ctrl.first->id()] = ctrl.first; > > > + } > > > } > > > > > > /** > > -- > Regards, > > Laurent Pinchart
Hi Laurent, On 18/11/2019 01:01, Laurent Pinchart wrote: > Hi Jacopo, > > On Fri, Nov 15, 2019 at 05:21:29PM +0100, Jacopo Mondi wrote: >> On Fri, Nov 08, 2019 at 10:53:55PM +0200, Laurent Pinchart wrote: >>> ControlInfoMap requires the ControlId and ControlRange of each entry to >>> have identical types. Check for this and log an error if a mismatch is >>> detected. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> src/libcamera/controls.cpp | 13 ++++++++++++- >>> 1 file changed, 12 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp >>> index eae0250a92e3..c488b2e4eb3f 100644 >>> --- a/src/libcamera/controls.cpp >>> +++ b/src/libcamera/controls.cpp >>> @@ -571,8 +571,19 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const >>> void ControlInfoMap::generateIdmap() >>> { >>> idmap_.clear(); >>> - for (const auto &ctrl : *this) >>> + >>> + for (const auto &ctrl : *this) { >>> + if (ctrl.first->type() != ctrl.second.min().type()) { >>> + LOG(Controls, Error) >>> + << "Control " << utils::hex(ctrl.first->id()) >>> + << " type and range type mismatch"; >>> + idmap_.clear(); >>> + clear(); >>> + return; >>> + } >>> + >> >> Should we just skip the control or actually abort the whole id map >> generation ? > > As this is a serious error, I think we should abort it completely, in > order to show that something went wrong. If you think we should just > skip the control, we can discuss that. Is this serious enough to be Fatal? (to be really noisy?) (Bearing in mind we still need to look at if Fatal messages are non-fatal when NDEBUG is defined). -- Kieran > >>> idmap_[ctrl.first->id()] = ctrl.first; >>> + } >>> } >>> >>> /** >
Hi Kieran, On Tue, Nov 19, 2019 at 04:19:45PM +0000, Kieran Bingham wrote: > On 18/11/2019 01:01, Laurent Pinchart wrote: > > On Fri, Nov 15, 2019 at 05:21:29PM +0100, Jacopo Mondi wrote: > >> On Fri, Nov 08, 2019 at 10:53:55PM +0200, Laurent Pinchart wrote: > >>> ControlInfoMap requires the ControlId and ControlRange of each entry to > >>> have identical types. Check for this and log an error if a mismatch is > >>> detected. > >>> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> src/libcamera/controls.cpp | 13 ++++++++++++- > >>> 1 file changed, 12 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > >>> index eae0250a92e3..c488b2e4eb3f 100644 > >>> --- a/src/libcamera/controls.cpp > >>> +++ b/src/libcamera/controls.cpp > >>> @@ -571,8 +571,19 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const > >>> void ControlInfoMap::generateIdmap() > >>> { > >>> idmap_.clear(); > >>> - for (const auto &ctrl : *this) > >>> + > >>> + for (const auto &ctrl : *this) { > >>> + if (ctrl.first->type() != ctrl.second.min().type()) { > >>> + LOG(Controls, Error) > >>> + << "Control " << utils::hex(ctrl.first->id()) > >>> + << " type and range type mismatch"; > >>> + idmap_.clear(); > >>> + clear(); > >>> + return; > >>> + } > >>> + > >> > >> Should we just skip the control or actually abort the whole id map > >> generation ? > > > > As this is a serious error, I think we should abort it completely, in > > order to show that something went wrong. If you think we should just > > skip the control, we can discuss that. > > Is this serious enough to be Fatal? (to be really noisy?) > > (Bearing in mind we still need to look at if Fatal messages are > non-fatal when NDEBUG is defined). I'm in two minds about this, for this exact reason, we still need to define how to handle Fatal errors. I think I would prefer avoiding an abort() here until the code gets tested a bit more. > >>> idmap_[ctrl.first->id()] = ctrl.first; > >>> + } > >>> } > >>> > >>> /**
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index eae0250a92e3..c488b2e4eb3f 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -571,8 +571,19 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const void ControlInfoMap::generateIdmap() { idmap_.clear(); - for (const auto &ctrl : *this) + + for (const auto &ctrl : *this) { + if (ctrl.first->type() != ctrl.second.min().type()) { + LOG(Controls, Error) + << "Control " << utils::hex(ctrl.first->id()) + << " type and range type mismatch"; + idmap_.clear(); + clear(); + return; + } + idmap_[ctrl.first->id()] = ctrl.first; + } } /**
ControlInfoMap requires the ControlId and ControlRange of each entry to have identical types. Check for this and log an error if a mismatch is detected. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/controls.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)