[v1] ipa: rpi: Initialize enum controls with a list of values
diff mbox series

Message ID 20250609151624.278754-1-barnabas.pocze@ideasonboard.com
State Superseded
Headers show
Series
  • [v1] ipa: rpi: Initialize enum controls with a list of values
Related show

Commit Message

Barnabás Pőcze June 9, 2025, 3:16 p.m. UTC
This is how uvcvideo and rkisp1 do it. See ee918b370a08b
("ipa: rkisp1: agc: Initialize enum controls with a list of values")
for the motivation. In summary, having a list of values is used as a sign
that the control is an enum in multiple places (e.g. `cam`, `camshark`).

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/rpi/common/ipa_base.cpp | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

Comments

Laurent Pinchart June 11, 2025, 2:08 p.m. UTC | #1
Hi Barnabás,

Thank you for the patch.

On Mon, Jun 09, 2025 at 05:16:24PM +0200, Barnabás Pőcze wrote:
> This is how uvcvideo and rkisp1 do it. See ee918b370a08b
> ("ipa: rkisp1: agc: Initialize enum controls with a list of values")
> for the motivation. In summary, having a list of values is used as a sign
> that the control is an enum in multiple places (e.g. `cam`, `camshark`).
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/rpi/common/ipa_base.cpp | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index e0f8b7e78..39e2db86e 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -57,24 +57,18 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
>  const ControlInfoMap::Map ipaControls{
>  	/* \todo Move this to the Camera class */
>  	{ &controls::AeEnable, ControlInfo(false, true, true) },
> -	{ &controls::ExposureTimeMode,
> -	  ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> -		      static_cast<int32_t>(controls::ExposureTimeModeManual),
> -		      static_cast<int32_t>(controls::ExposureTimeModeAuto)) },
> +	{ &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) },
>  	{ &controls::ExposureTime,
>  	  ControlInfo(1, 66666, static_cast<int32_t>(defaultExposureTime.get<std::micro>())) },
> -	{ &controls::AnalogueGainMode,
> -	  ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> -		      static_cast<int32_t>(controls::AnalogueGainModeManual),
> -		      static_cast<int32_t>(controls::AnalogueGainModeAuto)) },
> +	{ &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) },

Could we list the values explicitly here, in case new values get added
to the controls in the future ?

>  	{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) },
>  	{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
>  	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
>  	{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
>  	{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
> -	{ &controls::AeFlickerMode, ControlInfo(static_cast<int>(controls::FlickerOff),
> -						static_cast<int>(controls::FlickerManual),
> -						static_cast<int>(controls::FlickerOff)) },
> +	{ &controls::AeFlickerMode,
> +	  ControlInfo({ { ControlValue(controls::FlickerOff), ControlValue(controls::FlickerManual) } },
> +		      controls::FlickerOff) },
>  	{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },
>  	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
>  	{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
Barnabás Pőcze June 11, 2025, 2:11 p.m. UTC | #2
2025. 06. 11. 16:08 keltezéssel, Laurent Pinchart írta:
> Hi Barnabás,
> 
> Thank you for the patch.
> 
> On Mon, Jun 09, 2025 at 05:16:24PM +0200, Barnabás Pőcze wrote:
>> This is how uvcvideo and rkisp1 do it. See ee918b370a08b
>> ("ipa: rkisp1: agc: Initialize enum controls with a list of values")
>> for the motivation. In summary, having a list of values is used as a sign
>> that the control is an enum in multiple places (e.g. `cam`, `camshark`).
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/ipa/rpi/common/ipa_base.cpp | 16 +++++-----------
>>   1 file changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
>> index e0f8b7e78..39e2db86e 100644
>> --- a/src/ipa/rpi/common/ipa_base.cpp
>> +++ b/src/ipa/rpi/common/ipa_base.cpp
>> @@ -57,24 +57,18 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
>>   const ControlInfoMap::Map ipaControls{
>>   	/* \todo Move this to the Camera class */
>>   	{ &controls::AeEnable, ControlInfo(false, true, true) },
>> -	{ &controls::ExposureTimeMode,
>> -	  ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
>> -		      static_cast<int32_t>(controls::ExposureTimeModeManual),
>> -		      static_cast<int32_t>(controls::ExposureTimeModeAuto)) },
>> +	{ &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) },
>>   	{ &controls::ExposureTime,
>>   	  ControlInfo(1, 66666, static_cast<int32_t>(defaultExposureTime.get<std::micro>())) },
>> -	{ &controls::AnalogueGainMode,
>> -	  ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
>> -		      static_cast<int32_t>(controls::AnalogueGainModeManual),
>> -		      static_cast<int32_t>(controls::AnalogueGainModeAuto)) },
>> +	{ &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) },
> 
> Could we list the values explicitly here, in case new values get added
> to the controls in the future ?

Most other `ControlInfo`s also specify all values, that's why I did the same here.
Do these two deserve special treatment? Or maybe all of the should be modified?


Regards,
Barnabás Pőcze

> 
>>   	{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) },
>>   	{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
>>   	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
>>   	{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
>>   	{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
>> -	{ &controls::AeFlickerMode, ControlInfo(static_cast<int>(controls::FlickerOff),
>> -						static_cast<int>(controls::FlickerManual),
>> -						static_cast<int>(controls::FlickerOff)) },
>> +	{ &controls::AeFlickerMode,
>> +	  ControlInfo({ { ControlValue(controls::FlickerOff), ControlValue(controls::FlickerManual) } },
>> +		      controls::FlickerOff) },
>>   	{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },
>>   	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
>>   	{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
>
Laurent Pinchart June 11, 2025, 2:22 p.m. UTC | #3
On Wed, Jun 11, 2025 at 04:11:36PM +0200, Barnabás Pőcze wrote:
> 2025. 06. 11. 16:08 keltezéssel, Laurent Pinchart írta:
> > On Mon, Jun 09, 2025 at 05:16:24PM +0200, Barnabás Pőcze wrote:
> >> This is how uvcvideo and rkisp1 do it. See ee918b370a08b
> >> ("ipa: rkisp1: agc: Initialize enum controls with a list of values")
> >> for the motivation. In summary, having a list of values is used as a sign
> >> that the control is an enum in multiple places (e.g. `cam`, `camshark`).
> >>
> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >> ---
> >>   src/ipa/rpi/common/ipa_base.cpp | 16 +++++-----------
> >>   1 file changed, 5 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> >> index e0f8b7e78..39e2db86e 100644
> >> --- a/src/ipa/rpi/common/ipa_base.cpp
> >> +++ b/src/ipa/rpi/common/ipa_base.cpp
> >> @@ -57,24 +57,18 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
> >>   const ControlInfoMap::Map ipaControls{
> >>   	/* \todo Move this to the Camera class */
> >>   	{ &controls::AeEnable, ControlInfo(false, true, true) },
> >> -	{ &controls::ExposureTimeMode,
> >> -	  ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> >> -		      static_cast<int32_t>(controls::ExposureTimeModeManual),
> >> -		      static_cast<int32_t>(controls::ExposureTimeModeAuto)) },
> >> +	{ &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) },
> >>   	{ &controls::ExposureTime,
> >>   	  ControlInfo(1, 66666, static_cast<int32_t>(defaultExposureTime.get<std::micro>())) },
> >> -	{ &controls::AnalogueGainMode,
> >> -	  ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> >> -		      static_cast<int32_t>(controls::AnalogueGainModeManual),
> >> -		      static_cast<int32_t>(controls::AnalogueGainModeAuto)) },
> >> +	{ &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) },
> > 
> > Could we list the values explicitly here, in case new values get added
> > to the controls in the future ?
> 
> Most other `ControlInfo`s also specify all values, that's why I did the same here.
> Do these two deserve special treatment? Or maybe all of the should be modified?

I'm tempted to modify them all, or at least to use controls::*Values
only in cases where we expect there will never be any new values, or
where adding a new value would require implementing it in all IPA
modules due to the nature of the control. Does that make sense ?

> >>   	{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) },
> >>   	{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> >>   	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> >>   	{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> >>   	{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
> >> -	{ &controls::AeFlickerMode, ControlInfo(static_cast<int>(controls::FlickerOff),
> >> -						static_cast<int>(controls::FlickerManual),
> >> -						static_cast<int>(controls::FlickerOff)) },
> >> +	{ &controls::AeFlickerMode,
> >> +	  ControlInfo({ { ControlValue(controls::FlickerOff), ControlValue(controls::FlickerManual) } },
> >> +		      controls::FlickerOff) },
> >>   	{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },
> >>   	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
> >>   	{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },

Patch
diff mbox series

diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
index e0f8b7e78..39e2db86e 100644
--- a/src/ipa/rpi/common/ipa_base.cpp
+++ b/src/ipa/rpi/common/ipa_base.cpp
@@ -57,24 +57,18 @@  constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
 const ControlInfoMap::Map ipaControls{
 	/* \todo Move this to the Camera class */
 	{ &controls::AeEnable, ControlInfo(false, true, true) },
-	{ &controls::ExposureTimeMode,
-	  ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
-		      static_cast<int32_t>(controls::ExposureTimeModeManual),
-		      static_cast<int32_t>(controls::ExposureTimeModeAuto)) },
+	{ &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) },
 	{ &controls::ExposureTime,
 	  ControlInfo(1, 66666, static_cast<int32_t>(defaultExposureTime.get<std::micro>())) },
-	{ &controls::AnalogueGainMode,
-	  ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
-		      static_cast<int32_t>(controls::AnalogueGainModeManual),
-		      static_cast<int32_t>(controls::AnalogueGainModeAuto)) },
+	{ &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) },
 	{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) },
 	{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
 	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
 	{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
 	{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
-	{ &controls::AeFlickerMode, ControlInfo(static_cast<int>(controls::FlickerOff),
-						static_cast<int>(controls::FlickerManual),
-						static_cast<int>(controls::FlickerOff)) },
+	{ &controls::AeFlickerMode,
+	  ControlInfo({ { ControlValue(controls::FlickerOff), ControlValue(controls::FlickerManual) } },
+		      controls::FlickerOff) },
 	{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },
 	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
 	{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },