| Message ID | 20221006221506.16948-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | b5ef421e0320d4a0578dee0109049637a7ad7a0d |
| Headers | show |
| Series |
|
| Related | show |
On Fri, Oct 07, 2022 at 01:15:06AM +0300, Laurent Pinchart via libcamera-devel wrote: > Extend the ControlInfoMap test to verify the behaviour of the default > 'def' argument to the ControlInfoMap constructor. s/ControlInfoMap/ControlInfo/ > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > test/controls/control_info.cpp | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp > index 56b4101f72fe..1176a5024b3a 100644 > --- a/test/controls/control_info.cpp > +++ b/test/controls/control_info.cpp > @@ -27,19 +27,21 @@ protected: > ControlInfo brightness; > > if (brightness.min().type() != ControlType::ControlTypeNone || > - brightness.max().type() != ControlType::ControlTypeNone) { > + brightness.max().type() != ControlType::ControlTypeNone || > + brightness.def().type() != ControlType::ControlTypeNone) { > cout << "Invalid control range for Brightness" << endl; > return TestFail; > } > > /* > * Test information retrieval from a control with a minimum and > - * a maximum value. > + * a maximum value, and an implicit default value. > */ > ControlInfo contrast(10, 200); > > if (contrast.min().get<int32_t>() != 10 || > - contrast.max().get<int32_t>() != 200) { > + contrast.max().get<int32_t>() != 200 || > + !contrast.def().isNone()) { > cout << "Invalid control range for Contrast" << endl; > return TestFail; > }
Quoting Laurent Pinchart via libcamera-devel (2022-10-06 23:17:16) > On Fri, Oct 07, 2022 at 01:15:06AM +0300, Laurent Pinchart via libcamera-devel wrote: > > Extend the ControlInfoMap test to verify the behaviour of the default > > 'def' argument to the ControlInfoMap constructor. > > s/ControlInfoMap/ControlInfo/ > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > test/controls/control_info.cpp | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp > > index 56b4101f72fe..1176a5024b3a 100644 > > --- a/test/controls/control_info.cpp > > +++ b/test/controls/control_info.cpp > > @@ -27,19 +27,21 @@ protected: > > ControlInfo brightness; > > > > if (brightness.min().type() != ControlType::ControlTypeNone || > > - brightness.max().type() != ControlType::ControlTypeNone) { > > + brightness.max().type() != ControlType::ControlTypeNone || > > + brightness.def().type() != ControlType::ControlTypeNone) { Should these use the .isNone() helper? Otherwise, or either way: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > cout << "Invalid control range for Brightness" << endl; > > return TestFail; > > } > > > > /* > > * Test information retrieval from a control with a minimum and > > - * a maximum value. > > + * a maximum value, and an implicit default value. Except the 'implicit' default value is 'not a value' ? > > */ > > ControlInfo contrast(10, 200); > > > > if (contrast.min().get<int32_t>() != 10 || > > - contrast.max().get<int32_t>() != 200) { > > + contrast.max().get<int32_t>() != 200 || > > + !contrast.def().isNone()) { > > cout << "Invalid control range for Contrast" << endl; > > return TestFail; > > } > > -- > Regards, > > Laurent Pinchart
Hi On 10/7/22 3:47 AM, Laurent Pinchart via libcamera-devel wrote: > On Fri, Oct 07, 2022 at 01:15:06AM +0300, Laurent Pinchart via libcamera-devel wrote: >> Extend the ControlInfoMap test to verify the behaviour of the default >> 'def' argument to the ControlInfoMap constructor. > s/ControlInfoMap/ControlInfo/ > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I thought I reviewed this but might have forgotten to send r-b, here it is: Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> test/controls/control_info.cpp | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp >> index 56b4101f72fe..1176a5024b3a 100644 >> --- a/test/controls/control_info.cpp >> +++ b/test/controls/control_info.cpp >> @@ -27,19 +27,21 @@ protected: >> ControlInfo brightness; >> >> if (brightness.min().type() != ControlType::ControlTypeNone || >> - brightness.max().type() != ControlType::ControlTypeNone) { >> + brightness.max().type() != ControlType::ControlTypeNone || >> + brightness.def().type() != ControlType::ControlTypeNone) { >> cout << "Invalid control range for Brightness" << endl; >> return TestFail; >> } >> >> /* >> * Test information retrieval from a control with a minimum and >> - * a maximum value. >> + * a maximum value, and an implicit default value. >> */ >> ControlInfo contrast(10, 200); >> >> if (contrast.min().get<int32_t>() != 10 || >> - contrast.max().get<int32_t>() != 200) { >> + contrast.max().get<int32_t>() != 200 || >> + !contrast.def().isNone()) { >> cout << "Invalid control range for Contrast" << endl; >> return TestFail; >> }
On Wed, Oct 12, 2022 at 11:34:08AM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart via libcamera-devel (2022-10-06 23:17:16) > > On Fri, Oct 07, 2022 at 01:15:06AM +0300, Laurent Pinchart via libcamera-devel wrote: > > > Extend the ControlInfoMap test to verify the behaviour of the default > > > 'def' argument to the ControlInfoMap constructor. > > > > s/ControlInfoMap/ControlInfo/ > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > test/controls/control_info.cpp | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp > > > index 56b4101f72fe..1176a5024b3a 100644 > > > --- a/test/controls/control_info.cpp > > > +++ b/test/controls/control_info.cpp > > > @@ -27,19 +27,21 @@ protected: > > > ControlInfo brightness; > > > > > > if (brightness.min().type() != ControlType::ControlTypeNone || > > > - brightness.max().type() != ControlType::ControlTypeNone) { > > > + brightness.max().type() != ControlType::ControlTypeNone || > > > + brightness.def().type() != ControlType::ControlTypeNone) { > > Should these use the .isNone() helper? Yes they should. Should I do so in this patch or would you prefer a separate one ? > Otherwise, or either way: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > cout << "Invalid control range for Brightness" << endl; > > > return TestFail; > > > } > > > > > > /* > > > * Test information retrieval from a control with a minimum and > > > - * a maximum value. > > > + * a maximum value, and an implicit default value. > > Except the 'implicit' default value is 'not a value' ? I'm not sure to follow you. > > > */ > > > ControlInfo contrast(10, 200); > > > > > > if (contrast.min().get<int32_t>() != 10 || > > > - contrast.max().get<int32_t>() != 200) { > > > + contrast.max().get<int32_t>() != 200 || > > > + !contrast.def().isNone()) { > > > cout << "Invalid control range for Contrast" << endl; > > > return TestFail; > > > }
Quoting Laurent Pinchart (2022-10-16 03:13:12) > On Wed, Oct 12, 2022 at 11:34:08AM +0100, Kieran Bingham wrote: > > Quoting Laurent Pinchart via libcamera-devel (2022-10-06 23:17:16) > > > On Fri, Oct 07, 2022 at 01:15:06AM +0300, Laurent Pinchart via libcamera-devel wrote: > > > > Extend the ControlInfoMap test to verify the behaviour of the default > > > > 'def' argument to the ControlInfoMap constructor. > > > > > > s/ControlInfoMap/ControlInfo/ > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > test/controls/control_info.cpp | 8 +++++--- > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp > > > > index 56b4101f72fe..1176a5024b3a 100644 > > > > --- a/test/controls/control_info.cpp > > > > +++ b/test/controls/control_info.cpp > > > > @@ -27,19 +27,21 @@ protected: > > > > ControlInfo brightness; > > > > > > > > if (brightness.min().type() != ControlType::ControlTypeNone || > > > > - brightness.max().type() != ControlType::ControlTypeNone) { > > > > + brightness.max().type() != ControlType::ControlTypeNone || > > > > + brightness.def().type() != ControlType::ControlTypeNone) { > > > > Should these use the .isNone() helper? > > Yes they should. Should I do so in this patch or would you prefer a > separate one ? Up to you. > > > Otherwise, or either way: > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > cout << "Invalid control range for Brightness" << endl; > > > > return TestFail; > > > > } > > > > > > > > /* > > > > * Test information retrieval from a control with a minimum and > > > > - * a maximum value. > > > > + * a maximum value, and an implicit default value. > > > > Except the 'implicit' default value is 'not a value' ? > > I'm not sure to follow you. It doesn't matter, re-reading it now it's ok. But I just found it confusing that you're extending this to state that there will be an implicit default value. But the default value is 'None'. ... as in not a value - but that's an explicit state for our controls. So it *is* the default value. > > > > > */ > > > > ControlInfo contrast(10, 200); > > > > > > > > if (contrast.min().get<int32_t>() != 10 || > > > > - contrast.max().get<int32_t>() != 200) { > > > > + contrast.max().get<int32_t>() != 200 || > > > > + !contrast.def().isNone()) { > > > > cout << "Invalid control range for Contrast" << endl; > > > > return TestFail; > > > > } > > -- > Regards, > > Laurent Pinchart
On Wed, Oct 19, 2022 at 10:38:07AM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2022-10-16 03:13:12) > > On Wed, Oct 12, 2022 at 11:34:08AM +0100, Kieran Bingham wrote: > > > Quoting Laurent Pinchart via libcamera-devel (2022-10-06 23:17:16) > > > > On Fri, Oct 07, 2022 at 01:15:06AM +0300, Laurent Pinchart via libcamera-devel wrote: > > > > > Extend the ControlInfoMap test to verify the behaviour of the default > > > > > 'def' argument to the ControlInfoMap constructor. > > > > > > > > s/ControlInfoMap/ControlInfo/ > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > test/controls/control_info.cpp | 8 +++++--- > > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp > > > > > index 56b4101f72fe..1176a5024b3a 100644 > > > > > --- a/test/controls/control_info.cpp > > > > > +++ b/test/controls/control_info.cpp > > > > > @@ -27,19 +27,21 @@ protected: > > > > > ControlInfo brightness; > > > > > > > > > > if (brightness.min().type() != ControlType::ControlTypeNone || > > > > > - brightness.max().type() != ControlType::ControlTypeNone) { > > > > > + brightness.max().type() != ControlType::ControlTypeNone || > > > > > + brightness.def().type() != ControlType::ControlTypeNone) { > > > > > > Should these use the .isNone() helper? > > > > Yes they should. Should I do so in this patch or would you prefer a > > separate one ? > > Up to you. I'll cook a patch on top. > > > Otherwise, or either way: > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > cout << "Invalid control range for Brightness" << endl; > > > > > return TestFail; > > > > > } > > > > > > > > > > /* > > > > > * Test information retrieval from a control with a minimum and > > > > > - * a maximum value. > > > > > + * a maximum value, and an implicit default value. > > > > > > Except the 'implicit' default value is 'not a value' ? > > > > I'm not sure to follow you. > > It doesn't matter, re-reading it now it's ok. > > But I just found it confusing that you're extending this to state that > there will be an implicit default value. But the default value is > 'None'. ... as in not a value - but that's an explicit state for our > controls. So it *is* the default value. > > > > > > */ > > > > > ControlInfo contrast(10, 200); > > > > > > > > > > if (contrast.min().get<int32_t>() != 10 || > > > > > - contrast.max().get<int32_t>() != 200) { > > > > > + contrast.max().get<int32_t>() != 200 || > > > > > + !contrast.def().isNone()) { > > > > > cout << "Invalid control range for Contrast" << endl; > > > > > return TestFail; > > > > > }
diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp index 56b4101f72fe..1176a5024b3a 100644 --- a/test/controls/control_info.cpp +++ b/test/controls/control_info.cpp @@ -27,19 +27,21 @@ protected: ControlInfo brightness; if (brightness.min().type() != ControlType::ControlTypeNone || - brightness.max().type() != ControlType::ControlTypeNone) { + brightness.max().type() != ControlType::ControlTypeNone || + brightness.def().type() != ControlType::ControlTypeNone) { cout << "Invalid control range for Brightness" << endl; return TestFail; } /* * Test information retrieval from a control with a minimum and - * a maximum value. + * a maximum value, and an implicit default value. */ ControlInfo contrast(10, 200); if (contrast.min().get<int32_t>() != 10 || - contrast.max().get<int32_t>() != 200) { + contrast.max().get<int32_t>() != 200 || + !contrast.def().isNone()) { cout << "Invalid control range for Contrast" << endl; return TestFail; }
Extend the ControlInfoMap test to verify the behaviour of the default 'def' argument to the ControlInfoMap constructor. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- test/controls/control_info.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)