| Message ID | 20220212233407.3309708-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for your patch. On Sat, 12 Feb 2022 at 23:34, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > After the CameraSensor class has initialised properties, a default > setting is applied to ensure that test pattern modes are disabled unless > explicitly requested. > > This does not currently check to see if test patterns are supported on > the CameraSensor and the applyTestPatternMode call will report a failure > if it attempts to set a mode when not supported. > > Move the initialisation of the test pattern mode to the implementation > of the control mappings to ensure that it is correctly reset at start > up, while the code path will have already completed early if not > supported. > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/249 > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > This does reduce panic caused by the error message on startup for our imx477 users :-) Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/camera_sensor.cpp | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/camera_sensor.cpp > b/src/libcamera/camera_sensor.cpp > index 345b4a170d47..3a6a1a6bca77 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -172,7 +172,7 @@ int CameraSensor::init() > if (ret) > return ret; > > - return > applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > + return 0; > } > > int CameraSensor::validateSensorDriver() > @@ -372,6 +372,9 @@ void CameraSensor::initTestPatternModes() > > testPatternModes_.push_back(it->second); > } > + > + /* Initialise the sensor with test patterns disabled. */ > + > applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > } > > int CameraSensor::initProperties() > -- > 2.32.0 > >
Hi Kieran, Thank you for the patch. On Sat, Feb 12, 2022 at 11:34:07PM +0000, Kieran Bingham wrote: > After the CameraSensor class has initialised properties, a default > setting is applied to ensure that test pattern modes are disabled unless > explicitly requested. > > This does not currently check to see if test patterns are supported on > the CameraSensor and the applyTestPatternMode call will report a failure > if it attempts to set a mode when not supported. > > Move the initialisation of the test pattern mode to the implementation > of the control mappings to ensure that it is correctly reset at start > up, while the code path will have already completed early if not > supported. > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/249 > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/camera_sensor.cpp | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 345b4a170d47..3a6a1a6bca77 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -172,7 +172,7 @@ int CameraSensor::init() > if (ret) > return ret; > > - return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > + return 0; > } > > int CameraSensor::validateSensorDriver() > @@ -372,6 +372,9 @@ void CameraSensor::initTestPatternModes() > > testPatternModes_.push_back(it->second); > } > + > + /* Initialise the sensor with test patterns disabled. */ > + applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); Errors when applyting the test pattern are not propagated up anymore. A different approach if you don't want to turn all void functions in the call stack to return an int is to move the error message when testPatternModes_.empty() from CameraSensor::applyTestPatternMode() to CameraSensor::setTestPatternMode(). You will need to keep the check in CameraSensor::applyTestPatternMode() in order to return 0, but CameraSensor::setTestPatternMode() could return a proper error code. > } > > int CameraSensor::initProperties()
Hi all, On Tue, 22 Feb 2022 at 06:11, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Kieran, > > Thank you for the patch. > > On Sat, Feb 12, 2022 at 11:34:07PM +0000, Kieran Bingham wrote: > > After the CameraSensor class has initialised properties, a default > > setting is applied to ensure that test pattern modes are disabled unless > > explicitly requested. > > > > This does not currently check to see if test patterns are supported on > > the CameraSensor and the applyTestPatternMode call will report a failure > > if it attempts to set a mode when not supported. > > > > Move the initialisation of the test pattern mode to the implementation > > of the control mappings to ensure that it is correctly reset at start > > up, while the code path will have already completed early if not > > supported. > > > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/249 > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/libcamera/camera_sensor.cpp | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/camera_sensor.cpp > b/src/libcamera/camera_sensor.cpp > > index 345b4a170d47..3a6a1a6bca77 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -172,7 +172,7 @@ int CameraSensor::init() > > if (ret) > > return ret; > > > > - return > applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > > + return 0; > > } > > > > int CameraSensor::validateSensorDriver() > > @@ -372,6 +372,9 @@ void CameraSensor::initTestPatternModes() > > > > testPatternModes_.push_back(it->second); > > } > > + > > + /* Initialise the sensor with test patterns disabled. */ > > + > applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > > Errors when applyting the test pattern are not propagated up anymore. > > A different approach if you don't want to turn all void functions in the > call stack to return an int is to move the error message when > testPatternModes_.empty() from CameraSensor::applyTestPatternMode() to > CameraSensor::setTestPatternMode(). You will need to keep the check in > CameraSensor::applyTestPatternMode() in order to return 0, but > CameraSensor::setTestPatternMode() could return a proper error code. > Have we come to a resolution on this? It's winding up some of our users, and we could do with suppressing these warnings :-) Regards, Naush > > > } > > > > int CameraSensor::initProperties() > > -- > Regards, > > Laurent Pinchart >
Quoting Naushir Patuck (2022-05-25 12:05:49) > Hi all, > > On Tue, 22 Feb 2022 at 06:11, Laurent Pinchart < > laurent.pinchart@ideasonboard.com> wrote: > > > Hi Kieran, > > > > Thank you for the patch. > > > > On Sat, Feb 12, 2022 at 11:34:07PM +0000, Kieran Bingham wrote: > > > After the CameraSensor class has initialised properties, a default > > > setting is applied to ensure that test pattern modes are disabled unless > > > explicitly requested. > > > > > > This does not currently check to see if test patterns are supported on > > > the CameraSensor and the applyTestPatternMode call will report a failure > > > if it attempts to set a mode when not supported. > > > > > > Move the initialisation of the test pattern mode to the implementation > > > of the control mappings to ensure that it is correctly reset at start > > > up, while the code path will have already completed early if not > > > supported. > > > > > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/249 > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > > src/libcamera/camera_sensor.cpp | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/camera_sensor.cpp > > b/src/libcamera/camera_sensor.cpp > > > index 345b4a170d47..3a6a1a6bca77 100644 > > > --- a/src/libcamera/camera_sensor.cpp > > > +++ b/src/libcamera/camera_sensor.cpp > > > @@ -172,7 +172,7 @@ int CameraSensor::init() > > > if (ret) > > > return ret; > > > > > > - return > > applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > > > + return 0; > > > } > > > > > > int CameraSensor::validateSensorDriver() > > > @@ -372,6 +372,9 @@ void CameraSensor::initTestPatternModes() > > > > > > testPatternModes_.push_back(it->second); > > > } > > > + > > > + /* Initialise the sensor with test patterns disabled. */ > > > + > > applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > > > > Errors when applyting the test pattern are not propagated up anymore. > > > > A different approach if you don't want to turn all void functions in the > > call stack to return an int is to move the error message when > > testPatternModes_.empty() from CameraSensor::applyTestPatternMode() to > > CameraSensor::setTestPatternMode(). You will need to keep the check in > > CameraSensor::applyTestPatternMode() in order to return 0, but > > CameraSensor::setTestPatternMode() could return a proper error code. > > > > Have we come to a resolution on this? It's winding up some of our users, > and we could do with suppressing these warnings :-) I have pretty low availabilty for the next couple of weeks with holiday. Would you be able to send a patch with Laurents suggestion above? Hopefully that could be merged quicker. Of course adding the test pattern controls to the IMX477 is another option too? -- Regards Kieran > > Regards, > Naush > > > > > > > > } > > > > > > int CameraSensor::initProperties() > > > > -- > > Regards, > > > > Laurent Pinchart > >
Hi Kieran, On Sat, 28 May 2022 at 23:52, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck (2022-05-25 12:05:49) > > Hi all, > > > > On Tue, 22 Feb 2022 at 06:11, Laurent Pinchart < > > laurent.pinchart@ideasonboard.com> wrote: > > > > > Hi Kieran, > > > > > > Thank you for the patch. > > > > > > On Sat, Feb 12, 2022 at 11:34:07PM +0000, Kieran Bingham wrote: > > > > After the CameraSensor class has initialised properties, a default > > > > setting is applied to ensure that test pattern modes are disabled > unless > > > > explicitly requested. > > > > > > > > This does not currently check to see if test patterns are supported > on > > > > the CameraSensor and the applyTestPatternMode call will report a > failure > > > > if it attempts to set a mode when not supported. > > > > > > > > Move the initialisation of the test pattern mode to the > implementation > > > > of the control mappings to ensure that it is correctly reset at start > > > > up, while the code path will have already completed early if not > > > > supported. > > > > > > > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/249 > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > --- > > > > src/libcamera/camera_sensor.cpp | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/libcamera/camera_sensor.cpp > > > b/src/libcamera/camera_sensor.cpp > > > > index 345b4a170d47..3a6a1a6bca77 100644 > > > > --- a/src/libcamera/camera_sensor.cpp > > > > +++ b/src/libcamera/camera_sensor.cpp > > > > @@ -172,7 +172,7 @@ int CameraSensor::init() > > > > if (ret) > > > > return ret; > > > > > > > > - return > > > > applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > > > > + return 0; > > > > } > > > > > > > > int CameraSensor::validateSensorDriver() > > > > @@ -372,6 +372,9 @@ void CameraSensor::initTestPatternModes() > > > > > > > > testPatternModes_.push_back(it->second); > > > > } > > > > + > > > > + /* Initialise the sensor with test patterns disabled. */ > > > > + > > > > applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > > > > > > Errors when applyting the test pattern are not propagated up anymore. > > > > > > A different approach if you don't want to turn all void functions in > the > > > call stack to return an int is to move the error message when > > > testPatternModes_.empty() from CameraSensor::applyTestPatternMode() to > > > CameraSensor::setTestPatternMode(). You will need to keep the check in > > > CameraSensor::applyTestPatternMode() in order to return 0, but > > > CameraSensor::setTestPatternMode() could return a proper error code. > > > > > > > Have we come to a resolution on this? It's winding up some of our users, > > and we could do with suppressing these warnings :-) > > I have pretty low availabilty for the next couple of weeks with > holiday. > > Would you be able to send a patch with Laurents suggestion above? > Hopefully that could be merged quicker. > Sure, I can give it a go. It will probably have to wait for next week as I am off for the week after today. I'll post a patch when it's done. Regards, Naush > > Of course adding the test pattern controls to the IMX477 is another > option too? > -- > Regards > > Kieran > > > > > > Regards, > > Naush > > > > > > > > > > > > > } > > > > > > > > int CameraSensor::initProperties() > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart > > > >
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 345b4a170d47..3a6a1a6bca77 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -172,7 +172,7 @@ int CameraSensor::init() if (ret) return ret; - return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); + return 0; } int CameraSensor::validateSensorDriver() @@ -372,6 +372,9 @@ void CameraSensor::initTestPatternModes() testPatternModes_.push_back(it->second); } + + /* Initialise the sensor with test patterns disabled. */ + applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); } int CameraSensor::initProperties()
After the CameraSensor class has initialised properties, a default setting is applied to ensure that test pattern modes are disabled unless explicitly requested. This does not currently check to see if test patterns are supported on the CameraSensor and the applyTestPatternMode call will report a failure if it attempts to set a mode when not supported. Move the initialisation of the test pattern mode to the implementation of the control mappings to ensure that it is correctly reset at start up, while the code path will have already completed early if not supported. Bug: https://github.com/raspberrypi/libcamera-apps/issues/249 Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/camera_sensor.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)