<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 11 Jul 2021 at 19:54, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.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>
Thank you for the patch.<br>
<br>
On Fri, Jul 09, 2021 at 03:58:20PM +0100, Naushir Patuck wrote:<br>
> Add an operator<< overload to log all fields in DeviceStatus, and remove the<br>
> manual logging statements in the IPA and CamHelper.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
>  src/ipa/raspberrypi/cam_helper.cpp             |  5 +----<br>
>  src/ipa/raspberrypi/controller/device_status.h | 15 +++++++++++++++<br>
>  src/ipa/raspberrypi/raspberrypi.cpp            |  5 +----<br>
>  3 files changed, 17 insertions(+), 8 deletions(-)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp<br>
> index e6d2258c66d7..1ec3f03e1aa3 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
> @@ -188,10 +188,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,<br>
>       deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;<br>
>       deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;<br>
>  <br>
> -     LOG(IPARPI, Debug) << "Metadata updated - Exposure : "<br>
> -                        << deviceStatus.shutter_speed<br>
> -                        << " Gain : "<br>
> -                        << deviceStatus.analogue_gain;<br>
> +     LOG(IPARPI, Debug) << "Metadata updated - " << deviceStatus;<br>
>  <br>
>       metadata.Set("device.status", deviceStatus);<br>
>  }<br>
> diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h<br>
> index 73df7ce228dd..e2511d19b96d 100644<br>
> --- a/src/ipa/raspberrypi/controller/device_status.h<br>
> +++ b/src/ipa/raspberrypi/controller/device_status.h<br>
> @@ -6,6 +6,8 @@<br>
>   */<br>
>  #pragma once<br>
>  <br>
> +#include <iostream><br>
> +<br>
>  #include <libcamera/base/utils.h><br>
>  <br>
>  /*<br>
> @@ -20,6 +22,19 @@ struct DeviceStatus {<br>
>       {<br>
>       }<br>
>  <br>
> +     friend std::ostream &operator<<(std::ostream &out, const DeviceStatus &d)<br>
> +     {<br>
> +             using namespace libcamera; /* for the Duration operator<< overload */<br>
> +<br>
> +             out << "Exposure: " << d.shutter_speed<br>
> +                 << " Gain: " << d.analogue_gain<br>
> +                 << " Aperture: " << d.aperture<br>
> +                 << " Lens: " << d.lens_position<br>
> +                 << " Flash: " << d.flash_intensity;<br>
> +<br>
> +             return out;<br>
> +     }<br>
<br>
Does this function need to be inline ? Moving it to a new<br>
device_status.cpp file would avoid duplicating the implementation in<br>
multiple object files.<br></blockquote><div><br></div><div>Not necessary to be inline, but it was small enough that I did not think it would</div><div>matter.</div><div><br></div><div>I will send an update with this in its own cpp file.  Will also address your other</div><div>feedback comments in the next update.</div><div><br></div><div>Regards,</div><div>Naush </div><div><br></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>
>       /* time shutter is open */<br>
>       libcamera::utils::Duration shutter_speed;<br>
>       double analogue_gain;<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 4d09a84f6532..f51c970befb5 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -1019,10 +1019,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)<br>
>       deviceStatus.shutter_speed = helper_->Exposure(exposureLines);<br>
>       deviceStatus.analogue_gain = helper_->Gain(gainCode);<br>
>  <br>
> -     LOG(IPARPI, Debug) << "Metadata - Exposure : "<br>
> -                        << deviceStatus.shutter_speed<br>
> -                        << " Gain : "<br>
> -                        << deviceStatus.analogue_gain;<br>
> +     LOG(IPARPI, Debug) << "Metadata - " << deviceStatus;<br>
>  <br>
>       rpiMetadata_.Set("device.status", deviceStatus);<br>
>  }<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>