| Message ID | 20221122083005.5239-1-jacopo@jmondi.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 11/22/22 2:00 PM, Jacopo Mondi via libcamera-devel wrote: > Since the very beginning the camera sensor class has validated the > controls available from the camera sensor driver, and complained > accordingly when any of them was missing. > > The complaint message reported however only the numerical identifier of > the V4L2 control, making debugging harder. I had similar pain recently but the printing control-id made things a bit better in my case :D ed591e705c (libcamera: v4l2_device: Log control id instead of errorIdx) > > Associate to each control a human readable identifier and use it in > debug messages. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/camera_sensor.cpp | 38 ++++++++++++++++----------------- > 1 file changed, 19 insertions(+), 19 deletions(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index ea373458a164..0780ce5a7007 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -213,31 +213,31 @@ int CameraSensor::validateSensorDriver() > * Optional controls are used to register optional sensor properties. If > * not present, some values will be defaulted. > */ > - static constexpr uint32_t optionalControls[] = { > - V4L2_CID_CAMERA_SENSOR_ROTATION, > + static const std::map<uint32_t, std::string> optionalControls = { > + { V4L2_CID_CAMERA_SENSOR_ROTATION, "Rotation" }, > }; > > const ControlIdMap &controls = subdev_->controls().idmap(); > - for (uint32_t ctrl : optionalControls) { > + for (const auto &[ctrl, name] : optionalControls) { > if (!controls.count(ctrl)) > LOG(CameraSensor, Debug) > - << "Optional V4L2 control " << utils::hex(ctrl) > - << " not supported"; > + << "Optional V4L2 control '" << name > + << "' not supported"; > } > > /* > * Recommended controls are similar to optional controls, but will > * become mandatory in the near future. Be loud if they're missing. > */ > - static constexpr uint32_t recommendedControls[] = { > - V4L2_CID_CAMERA_ORIENTATION, > + static const std::map<uint32_t, std::string> recommendedControls = { > + { V4L2_CID_CAMERA_ORIENTATION, "Orientation" }, > }; > > - for (uint32_t ctrl : recommendedControls) { > + for (const auto &[ctrl, name] : recommendedControls) { > if (!controls.count(ctrl)) { > LOG(CameraSensor, Warning) > - << "Recommended V4L2 control " << utils::hex(ctrl) > - << " not supported"; > + << "Recommended V4L2 control '" << name > + << "' not supported"; > err = -EINVAL; > } > } > @@ -300,20 +300,20 @@ int CameraSensor::validateSensorDriver() > * For raw sensors, make sure the sensor driver supports the controls > * required by the CameraSensor class. > */ > - static constexpr uint32_t mandatoryControls[] = { > - V4L2_CID_ANALOGUE_GAIN, > - V4L2_CID_EXPOSURE, > - V4L2_CID_HBLANK, > - V4L2_CID_PIXEL_RATE, > - V4L2_CID_VBLANK, > + static const std::map<uint32_t, std::string> mandatoryControls = { > + { V4L2_CID_ANALOGUE_GAIN, "Analogue gain" }, > + { V4L2_CID_EXPOSURE, "Exposure" }, > + { V4L2_CID_HBLANK, "Horizontal blanking" }, > + { V4L2_CID_PIXEL_RATE, "Pixel Rate" }, > + { V4L2_CID_VBLANK, "Vertical blanking" } > }; > > err = 0; > - for (uint32_t ctrl : mandatoryControls) { > + for (const auto &[ctrl, name] : mandatoryControls) { > if (!controls.count(ctrl)) { > LOG(CameraSensor, Error) > - << "Mandatory V4L2 control " << utils::hex(ctrl) > - << " not available"; > + << "Mandatory V4L2 control '" << name > + << "' not available"; > err = -EINVAL; > } > }
Quoting Jacopo Mondi via libcamera-devel (2022-11-22 08:30:05) > Since the very beginning the camera sensor class has validated the > controls available from the camera sensor driver, and complained > accordingly when any of them was missing. > > The complaint message reported however only the numerical identifier of > the V4L2 control, making debugging harder. > > Associate to each control a human readable identifier and use it in > debug messages. > Screams in 'YES PLEASE' [0] [1] [0] https://patchwork.libcamera.org/patch/13346/ (13/08/2021) [1] https://patchwork.libcamera.org/patch/11178/ (05/02/2021) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/camera_sensor.cpp | 38 ++++++++++++++++----------------- > 1 file changed, 19 insertions(+), 19 deletions(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index ea373458a164..0780ce5a7007 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -213,31 +213,31 @@ int CameraSensor::validateSensorDriver() > * Optional controls are used to register optional sensor properties. If > * not present, some values will be defaulted. > */ > - static constexpr uint32_t optionalControls[] = { > - V4L2_CID_CAMERA_SENSOR_ROTATION, > + static const std::map<uint32_t, std::string> optionalControls = { > + { V4L2_CID_CAMERA_SENSOR_ROTATION, "Rotation" }, > }; > > const ControlIdMap &controls = subdev_->controls().idmap(); > - for (uint32_t ctrl : optionalControls) { > + for (const auto &[ctrl, name] : optionalControls) { > if (!controls.count(ctrl)) > LOG(CameraSensor, Debug) > - << "Optional V4L2 control " << utils::hex(ctrl) > - << " not supported"; > + << "Optional V4L2 control '" << name > + << "' not supported"; > } > > /* > * Recommended controls are similar to optional controls, but will > * become mandatory in the near future. Be loud if they're missing. > */ > - static constexpr uint32_t recommendedControls[] = { > - V4L2_CID_CAMERA_ORIENTATION, > + static const std::map<uint32_t, std::string> recommendedControls = { > + { V4L2_CID_CAMERA_ORIENTATION, "Orientation" }, > }; > > - for (uint32_t ctrl : recommendedControls) { > + for (const auto &[ctrl, name] : recommendedControls) { > if (!controls.count(ctrl)) { > LOG(CameraSensor, Warning) > - << "Recommended V4L2 control " << utils::hex(ctrl) > - << " not supported"; > + << "Recommended V4L2 control '" << name > + << "' not supported"; > err = -EINVAL; > } > } > @@ -300,20 +300,20 @@ int CameraSensor::validateSensorDriver() > * For raw sensors, make sure the sensor driver supports the controls > * required by the CameraSensor class. > */ > - static constexpr uint32_t mandatoryControls[] = { > - V4L2_CID_ANALOGUE_GAIN, > - V4L2_CID_EXPOSURE, > - V4L2_CID_HBLANK, > - V4L2_CID_PIXEL_RATE, > - V4L2_CID_VBLANK, > + static const std::map<uint32_t, std::string> mandatoryControls = { > + { V4L2_CID_ANALOGUE_GAIN, "Analogue gain" }, > + { V4L2_CID_EXPOSURE, "Exposure" }, > + { V4L2_CID_HBLANK, "Horizontal blanking" }, > + { V4L2_CID_PIXEL_RATE, "Pixel Rate" }, > + { V4L2_CID_VBLANK, "Vertical blanking" } > }; > > err = 0; > - for (uint32_t ctrl : mandatoryControls) { > + for (const auto &[ctrl, name] : mandatoryControls) { > if (!controls.count(ctrl)) { > LOG(CameraSensor, Error) > - << "Mandatory V4L2 control " << utils::hex(ctrl) > - << " not available"; > + << "Mandatory V4L2 control '" << name > + << "' not available"; > err = -EINVAL; > } > } > -- > 2.38.1 >
Hi Jacopo, Thank you for the patch. On Tue, Nov 22, 2022 at 09:30:05AM +0100, Jacopo Mondi via libcamera-devel wrote: > Since the very beginning the camera sensor class has validated the > controls available from the camera sensor driver, and complained > accordingly when any of them was missing. > > The complaint message reported however only the numerical identifier of > the V4L2 control, making debugging harder. > > Associate to each control a human readable identifier and use it in > debug messages. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/camera_sensor.cpp | 38 ++++++++++++++++----------------- > 1 file changed, 19 insertions(+), 19 deletions(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index ea373458a164..0780ce5a7007 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -213,31 +213,31 @@ int CameraSensor::validateSensorDriver() > * Optional controls are used to register optional sensor properties. If > * not present, some values will be defaulted. > */ > - static constexpr uint32_t optionalControls[] = { > - V4L2_CID_CAMERA_SENSOR_ROTATION, > + static const std::map<uint32_t, std::string> optionalControls = { > + { V4L2_CID_CAMERA_SENSOR_ROTATION, "Rotation" }, Wouldn't it be more explicit if the name was "CAMERA_SENSOR_ROTATION", or even "V4L2_CID_CAMERA_SENSOR_ROTATION" ? We should centralize the control names somewhere, but that's a topic for later. > }; > > const ControlIdMap &controls = subdev_->controls().idmap(); > - for (uint32_t ctrl : optionalControls) { > + for (const auto &[ctrl, name] : optionalControls) { > if (!controls.count(ctrl)) > LOG(CameraSensor, Debug) > - << "Optional V4L2 control " << utils::hex(ctrl) > - << " not supported"; > + << "Optional V4L2 control '" << name > + << "' not supported"; > } > > /* > * Recommended controls are similar to optional controls, but will > * become mandatory in the near future. Be loud if they're missing. > */ > - static constexpr uint32_t recommendedControls[] = { > - V4L2_CID_CAMERA_ORIENTATION, > + static const std::map<uint32_t, std::string> recommendedControls = { > + { V4L2_CID_CAMERA_ORIENTATION, "Orientation" }, > }; > > - for (uint32_t ctrl : recommendedControls) { > + for (const auto &[ctrl, name] : recommendedControls) { > if (!controls.count(ctrl)) { > LOG(CameraSensor, Warning) > - << "Recommended V4L2 control " << utils::hex(ctrl) > - << " not supported"; > + << "Recommended V4L2 control '" << name > + << "' not supported"; > err = -EINVAL; > } > } > @@ -300,20 +300,20 @@ int CameraSensor::validateSensorDriver() > * For raw sensors, make sure the sensor driver supports the controls > * required by the CameraSensor class. > */ > - static constexpr uint32_t mandatoryControls[] = { > - V4L2_CID_ANALOGUE_GAIN, > - V4L2_CID_EXPOSURE, > - V4L2_CID_HBLANK, > - V4L2_CID_PIXEL_RATE, > - V4L2_CID_VBLANK, > + static const std::map<uint32_t, std::string> mandatoryControls = { > + { V4L2_CID_ANALOGUE_GAIN, "Analogue gain" }, > + { V4L2_CID_EXPOSURE, "Exposure" }, > + { V4L2_CID_HBLANK, "Horizontal blanking" }, > + { V4L2_CID_PIXEL_RATE, "Pixel Rate" }, > + { V4L2_CID_VBLANK, "Vertical blanking" } > }; > > err = 0; > - for (uint32_t ctrl : mandatoryControls) { > + for (const auto &[ctrl, name] : mandatoryControls) { > if (!controls.count(ctrl)) { > LOG(CameraSensor, Error) > - << "Mandatory V4L2 control " << utils::hex(ctrl) > - << " not available"; > + << "Mandatory V4L2 control '" << name > + << "' not available"; > err = -EINVAL; > } > }
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index ea373458a164..0780ce5a7007 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -213,31 +213,31 @@ int CameraSensor::validateSensorDriver() * Optional controls are used to register optional sensor properties. If * not present, some values will be defaulted. */ - static constexpr uint32_t optionalControls[] = { - V4L2_CID_CAMERA_SENSOR_ROTATION, + static const std::map<uint32_t, std::string> optionalControls = { + { V4L2_CID_CAMERA_SENSOR_ROTATION, "Rotation" }, }; const ControlIdMap &controls = subdev_->controls().idmap(); - for (uint32_t ctrl : optionalControls) { + for (const auto &[ctrl, name] : optionalControls) { if (!controls.count(ctrl)) LOG(CameraSensor, Debug) - << "Optional V4L2 control " << utils::hex(ctrl) - << " not supported"; + << "Optional V4L2 control '" << name + << "' not supported"; } /* * Recommended controls are similar to optional controls, but will * become mandatory in the near future. Be loud if they're missing. */ - static constexpr uint32_t recommendedControls[] = { - V4L2_CID_CAMERA_ORIENTATION, + static const std::map<uint32_t, std::string> recommendedControls = { + { V4L2_CID_CAMERA_ORIENTATION, "Orientation" }, }; - for (uint32_t ctrl : recommendedControls) { + for (const auto &[ctrl, name] : recommendedControls) { if (!controls.count(ctrl)) { LOG(CameraSensor, Warning) - << "Recommended V4L2 control " << utils::hex(ctrl) - << " not supported"; + << "Recommended V4L2 control '" << name + << "' not supported"; err = -EINVAL; } } @@ -300,20 +300,20 @@ int CameraSensor::validateSensorDriver() * For raw sensors, make sure the sensor driver supports the controls * required by the CameraSensor class. */ - static constexpr uint32_t mandatoryControls[] = { - V4L2_CID_ANALOGUE_GAIN, - V4L2_CID_EXPOSURE, - V4L2_CID_HBLANK, - V4L2_CID_PIXEL_RATE, - V4L2_CID_VBLANK, + static const std::map<uint32_t, std::string> mandatoryControls = { + { V4L2_CID_ANALOGUE_GAIN, "Analogue gain" }, + { V4L2_CID_EXPOSURE, "Exposure" }, + { V4L2_CID_HBLANK, "Horizontal blanking" }, + { V4L2_CID_PIXEL_RATE, "Pixel Rate" }, + { V4L2_CID_VBLANK, "Vertical blanking" } }; err = 0; - for (uint32_t ctrl : mandatoryControls) { + for (const auto &[ctrl, name] : mandatoryControls) { if (!controls.count(ctrl)) { LOG(CameraSensor, Error) - << "Mandatory V4L2 control " << utils::hex(ctrl) - << " not available"; + << "Mandatory V4L2 control '" << name + << "' not available"; err = -EINVAL; } }
Since the very beginning the camera sensor class has validated the controls available from the camera sensor driver, and complained accordingly when any of them was missing. The complaint message reported however only the numerical identifier of the V4L2 control, making debugging harder. Associate to each control a human readable identifier and use it in debug messages. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/camera_sensor.cpp | 38 ++++++++++++++++----------------- 1 file changed, 19 insertions(+), 19 deletions(-)