<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 19 May 2021 at 15:21, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
Thanks for this work!<br>
<br>
On Tue, 18 May 2021 at 11:09, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Switch the AgcAlgorithm API functions to use RPiController::Duration<br>
> for all time based variables.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/controller/agc_algorithm.hpp | 7 ++++---<br>
> src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 ++++++------<br>
> src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 +++---<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 6 +++---<br>
> 4 files changed, 16 insertions(+), 15 deletions(-)<br>
><br>
> diff --git a/src/ipa/raspberrypi/controller/agc_algorithm.hpp b/src/ipa/raspberrypi/controller/agc_algorithm.hpp<br>
> index f97deb0fca59..668912c47388 100644<br>
> --- a/src/ipa/raspberrypi/controller/agc_algorithm.hpp<br>
> +++ b/src/ipa/raspberrypi/controller/agc_algorithm.hpp<br>
> @@ -7,6 +7,7 @@<br>
> #pragma once<br>
><br>
> #include "algorithm.hpp"<br>
> +#include "duration.hpp"<br>
><br>
> namespace RPiController {<br>
><br>
> @@ -17,9 +18,9 @@ public:<br>
> // An AGC algorithm must provide the following:<br>
> virtual unsigned int GetConvergenceFrames() const = 0;<br>
> virtual void SetEv(double ev) = 0;<br>
> - virtual void SetFlickerPeriod(double flicker_period) = 0;<br>
> - virtual void SetFixedShutter(double fixed_shutter) = 0; // microseconds<br>
> - virtual void SetMaxShutter(double max_shutter) = 0; // microseconds<br>
> + virtual void SetFlickerPeriod(Duration flicker_period) = 0;<br>
> + virtual void SetFixedShutter(Duration fixed_shutter) = 0;<br>
> + virtual void SetMaxShutter(Duration max_shutter) = 0;<br>
> virtual void SetFixedAnalogueGain(double fixed_analogue_gain) = 0;<br>
> virtual void SetMeteringMode(std::string const &metering_mode_name) = 0;<br>
> virtual void SetExposureMode(std::string const &exposure_mode_name) = 0;<br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> index f4cd5d26fb4e..1cb4472b2691 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> @@ -222,19 +222,19 @@ void Agc::SetEv(double ev)<br>
> ev_ = ev;<br>
> }<br>
><br>
> -void Agc::SetFlickerPeriod(double flicker_period)<br>
> +void Agc::SetFlickerPeriod(Duration flicker_period)<br>
> {<br>
> - flicker_period_ = flicker_period;<br>
> + flicker_period_ = DurationValue<std::micro>(flicker_period);<br>
<br>
I assume these will all get nicer again in the next patch...!<br></blockquote><div><br></div><div>Yes!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> }<br>
><br>
> -void Agc::SetMaxShutter(double max_shutter)<br>
> +void Agc::SetMaxShutter(Duration max_shutter)<br>
> {<br>
> - max_shutter_ = max_shutter;<br>
> + max_shutter_ = DurationValue<std::micro>(max_shutter);<br>
> }<br>
><br>
> -void Agc::SetFixedShutter(double fixed_shutter)<br>
> +void Agc::SetFixedShutter(Duration fixed_shutter)<br>
> {<br>
> - fixed_shutter_ = fixed_shutter;<br>
> + fixed_shutter_ = DurationValue<std::micro>(fixed_shutter);<br>
> // Set this in case someone calls Pause() straight after.<br>
> status_.shutter_time = clipShutter(fixed_shutter_);<br>
> }<br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp<br>
> index 0427fb59ec1b..cb79bf61ba42 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp<br>
> @@ -77,9 +77,9 @@ public:<br>
> void Resume() override;<br>
> unsigned int GetConvergenceFrames() const override;<br>
> void SetEv(double ev) override;<br>
> - void SetFlickerPeriod(double flicker_period) override;<br>
> - void SetMaxShutter(double max_shutter) override; // microseconds<br>
> - void SetFixedShutter(double fixed_shutter) override; // microseconds<br>
> + void SetFlickerPeriod(Duration flicker_period) override;<br>
> + void SetMaxShutter(Duration max_shutter) override;<br>
> + void SetFixedShutter(Duration fixed_shutter) override;<br>
> void SetFixedAnalogueGain(double fixed_analogue_gain) override;<br>
> void SetMeteringMode(std::string const &metering_mode_name) override;<br>
> void SetExposureMode(std::string const &exposure_mode_name) override;<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 994fb7e057a9..f080f2e53bac 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -643,8 +643,8 @@ void IPARPi::queueRequest(const ControlList &controls)<br>
> break;<br>
> }<br>
><br>
> - /* This expects units of micro-seconds. */<br>
> - agc->SetFixedShutter(ctrl.second.get<int32_t>());<br>
> + /* The control provides units of microseconds. */<br>
> + agc->SetFixedShutter(ctrl.second.get<int32_t>() * 1.0us);<br>
<br>
I suppose I just wanted to ask if this is the "proper" way to convert<br>
to a Duration - it certainly seems quite clear and tidy.<br></blockquote><div><br></div><div>You can either use the above or the constructor form Duration(x). I find the</div><div>above a bit more intuitive, as you do not need to know the time units of the</div><div>Duration object.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
<br>
Thanks<br>
David<br>
<br>
><br>
> libcameraMetadata_.set(controls::ExposureTime, ctrl.second.get<int32_t>());<br>
> break;<br>
> @@ -1097,7 +1097,7 @@ void IPARPi::applyFrameDurations(const Duration &minFrameDuration,<br>
><br>
> RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(<br>
> controller_.GetAlgorithm("agc"));<br>
> - agc->SetMaxShutter(DurationValue<std::micro>(maxShutter));<br>
> + agc->SetMaxShutter(maxShutter);<br>
> }<br>
><br>
> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)<br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>