| Message ID | 20220903114803.951682-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Sat, Sep 03, 2022 at 12:48:03PM +0100, Kieran Bingham via libcamera-devel wrote: > If the CameraSensor fails to identify the VCM specified, it could still > be possible to continue to operate the sensor. Autofocus support will be > disabled, but this would be no different to operating a camera with a > fixed focus. While of course the fixed focus position may not be > suitable, it would provide a better user experience to be able to > continue to operate the camera, while still reporting that the lens is > disabled. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=146 > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > > The Surface devices now have a VCM registered, but they don't > successfully identify the device. While that should also be fixed, > failing to operate a camera because the VCM isn't operating seems to be > a bad user experience. I think that's reasonable, while there's clearly a problem on the kernel side, this degraded mode is implemented without much overhead here, so I'm fine with it. > src/libcamera/camera_sensor.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index d055c16a4885..dcee7b9a0210 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -467,8 +467,8 @@ int CameraSensor::discoverAncillaryDevices() > ret = focusLens_->init(); > if (ret) { > LOG(CameraSensor, Error) > - << "CameraLens initialisation failed"; > - return ret; > + << "CameraLens initialisation failed: Lens disabled"; I'd reset focusLens_ here to avoid exposin a non-working CameraLens through focusLens(). > + break; And you can drop the break, there's one just after. The message could also be shortened slightly, up to you. ret = focusLens_->init(); if (ret) { LOG(CameraSensor, Error) << "Lens initialisation failed, lens disabled"; focusLens_.reset(); } break; Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > } > break; >
Hello, On 9/3/22 7:23 PM, Laurent Pinchart via libcamera-devel wrote: > Hi Kieran, > > Thank you for the patch. > > On Sat, Sep 03, 2022 at 12:48:03PM +0100, Kieran Bingham via libcamera-devel wrote: >> If the CameraSensor fails to identify the VCM specified, it could still >> be possible to continue to operate the sensor. Autofocus support will be >> disabled, but this would be no different to operating a camera with a >> fixed focus. While of course the fixed focus position may not be >> suitable, it would provide a better user experience to be able to >> continue to operate the camera, while still reporting that the lens is >> disabled. >> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=146 >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> >> The Surface devices now have a VCM registered, but they don't >> successfully identify the device. While that should also be fixed, >> failing to operate a camera because the VCM isn't operating seems to be >> a bad user experience. > I think that's reasonable, while there's clearly a problem on the kernel > side, this degraded mode is implemented without much overhead here, so > I'm fine with it. > >> src/libcamera/camera_sensor.cpp | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp >> index d055c16a4885..dcee7b9a0210 100644 >> --- a/src/libcamera/camera_sensor.cpp >> +++ b/src/libcamera/camera_sensor.cpp >> @@ -467,8 +467,8 @@ int CameraSensor::discoverAncillaryDevices() >> ret = focusLens_->init(); >> if (ret) { >> LOG(CameraSensor, Error) >> - << "CameraLens initialisation failed"; >> - return ret; >> + << "CameraLens initialisation failed: Lens disabled"; > I'd reset focusLens_ here to avoid exposin a non-working CameraLens > through focusLens(). > >> + break; > And you can drop the break, there's one just after. The message could > also be shortened slightly, up to you. > > ret = focusLens_->init(); > if (ret) { > LOG(CameraSensor, Error) > << "Lens initialisation failed, lens disabled"; > focusLens_.reset(); > } > break; > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > >> } >> break; >>
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index d055c16a4885..dcee7b9a0210 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -467,8 +467,8 @@ int CameraSensor::discoverAncillaryDevices() ret = focusLens_->init(); if (ret) { LOG(CameraSensor, Error) - << "CameraLens initialisation failed"; - return ret; + << "CameraLens initialisation failed: Lens disabled"; + break; } break;
If the CameraSensor fails to identify the VCM specified, it could still be possible to continue to operate the sensor. Autofocus support will be disabled, but this would be no different to operating a camera with a fixed focus. While of course the fixed focus position may not be suitable, it would provide a better user experience to be able to continue to operate the camera, while still reporting that the lens is disabled. Bug: https://bugs.libcamera.org/show_bug.cgi?id=146 Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- The Surface devices now have a VCM registered, but they don't successfully identify the device. While that should also be fixed, failing to operate a camera because the VCM isn't operating seems to be a bad user experience. src/libcamera/camera_sensor.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)