<div dir="ltr"><div dir="ltr">Hi Kieran and David,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 28 Jun 2022 at 10:36, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@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>
Quoting David Plowman (2022-06-28 09:35:00)<br>
> Hi Naush<br>
> <br>
> Thanks for this, I think it's a good improvement!<br>
> <br>
> On Fri, 24 Jun 2022 at 13:34, Kieran Bingham via libcamera-devel<br>
> <<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a>> wrote:<br>
> ><br>
> > Quoting Naushir Patuck via libcamera-devel (2022-06-24 08:35:28)<br>
> > > Switch the aperture, lens_position, and flash_intensity fields in the<br>
> > > DeviceStatus structure to use std::optional instead of using invalid default<br>
> > > values.<br>
> ><br>
> > \o/<br>
> ><br>
> ><br>
> > Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> ><br>
> > ><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> <br>
> I suppose the only thing I wonder about is that once upon a time we<br>
> had a convention that .h files could be included in a C program, and<br>
> .hpp was for C++ only headers. But obviously libcamera doesn't do that<br>
> so I guess it is indeed time to consign that one to the dustbin of<br>
> history. Is it a bit weird now that we have both? I don't think I'm<br>
> bothered. So it's a \o/ from me too.<br>
<br>
Oh ... we have .hpp files ? - Oh - we do in controller indeed.<br>
<br>
We should probably make these more consitent, but that can be a patch on<br>
top anyway.<br></blockquote><div><br></div><div>Yes we do. There is a long (long) outstanding task of libcamerifying our controller</div><div>source files to match the required style. This includes *.hpp -> *.h and converting</div><div>from snake_case to camelCase. As always, it's on my to-do list :-)</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>
Another comment below too...<br>
<br>
> <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>
> > > src/ipa/raspberrypi/controller/device_status.cpp | 14 ++++++++++----<br>
> > > src/ipa/raspberrypi/controller/device_status.h | 9 ++++-----<br>
> > > src/ipa/raspberrypi/controller/rpi/lux.cpp | 5 ++---<br>
> > > 3 files changed, 16 insertions(+), 12 deletions(-)<br>
> > ><br>
> > > diff --git a/src/ipa/raspberrypi/controller/device_status.cpp b/src/ipa/raspberrypi/controller/device_status.cpp<br>
> > > index 05897fc15b50..a389c40dafed 100644<br>
> > > --- a/src/ipa/raspberrypi/controller/device_status.cpp<br>
> > > +++ b/src/ipa/raspberrypi/controller/device_status.cpp<br>
> > > @@ -12,10 +12,16 @@ std::ostream &operator<<(std::ostream &out, const DeviceStatus &d)<br>
> > > {<br>
> > > out << "Exposure: " << d.shutter_speed<br>
> > > << " Frame length: " << d.frame_length<br>
> > > - << " Gain: " << d.analogue_gain<br>
> > > - << " Aperture: " << d.aperture<br>
> > > - << " Lens: " << d.lens_position<br>
> > > - << " Flash: " << d.flash_intensity;<br>
> > > + << " Gain: " << d.analogue_gain;<br>
> > > +<br>
> > > + if (d.aperture)<br>
> > > + out << " Aperture: " << *d.aperture;<br>
> > > +<br>
> > > + if (d.lens_position)<br>
> > > + out << " Lens: " << *d.lens_position;<br>
> > > +<br>
> > > + if (d.flash_intensity)<br>
> > > + out << " Flash: " << *d.flash_intensity;<br>
> > ><br>
> > > if (d.sensor_temperature)<br>
> > > out << " Temperature: " << *d.sensor_temperature;<br>
> > > diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h<br>
> > > index eca3bf4b042e..b33f0d093ff3 100644<br>
> > > --- a/src/ipa/raspberrypi/controller/device_status.h<br>
> > > +++ b/src/ipa/raspberrypi/controller/device_status.h<br>
> > > @@ -19,8 +19,7 @@<br>
> > > struct DeviceStatus {<br>
> > > DeviceStatus()<br>
> > > : shutter_speed(std::chrono::seconds(0)), frame_length(0),<br>
> > > - analogue_gain(0.0), lens_position(0.0), aperture(0.0),<br>
> > > - flash_intensity(0.0)<br>
> > > + analogue_gain(0.0)<br>
> > > {<br>
> > > }<br>
> > ><br>
> > > @@ -32,11 +31,11 @@ struct DeviceStatus {<br>
> > > uint32_t frame_length;<br>
> > > double analogue_gain;<br>
> > > /* 1.0/distance-in-metres, or 0 if unknown */<br>
> > > - double lens_position;<br>
> > > + std::optional<double> lens_position;<br>
> > > /* 1/f so that brightness quadruples when this doubles, or 0 if unknown */<br>
> > > - double aperture;<br>
> > > + std::optional<double> aperture;<br>
> > > /* proportional to brightness with 0 = no flash, 1 = maximum flash */<br>
> > > - double flash_intensity;<br>
> > > + std::optional<double> flash_intensity;<br>
> > > /* Sensor reported temperature value (in degrees) */<br>
> > > std::optional<double> sensor_temperature;<br>
> > > };<br>
> > > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>
> > > index f77e9140ac10..643fb8fbaac6 100644<br>
> > > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>
> > > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>
> > > @@ -63,9 +63,8 @@ void Lux::Process(StatisticsPtr &stats, Metadata *image_metadata)<br>
> > > DeviceStatus device_status;<br>
> > > if (image_metadata->Get("device.status", device_status) == 0) {<br>
> > > double current_gain = device_status.analogue_gain;<br>
> > > - double current_aperture = device_status.aperture;<br>
> > > - if (current_aperture == 0)<br>
> > > - current_aperture = current_aperture_;<br>
> > > + double current_aperture = device_status.aperture ? *device_status.aperture<br>
> > > + : current_aperture_;<br>
<br>
Aha, there's actually one small thing here.<br>
<br>
With std::optional, we have value_or(), [0] so this could be:<br>
double current_aperture = device_status.aperture.value_or(current_aperture_);<br>
<br>
though that comes in at 94 characters long...<br>
I don't mind either way though, it's only really syntactic sugar.<br>
<br>
[0] <a href="https://en.cppreference.com/w/cpp/utility/optional/value_or" rel="noreferrer" target="_blank">https://en.cppreference.com/w/cpp/utility/optional/value_or</a><br>
<br>
Let me know what you think/prefer and I'll get these through the matrix<br>
and merged soon now we have David's review.<br></blockquote><div><br></div><div>Happy to change to value_or() here, it does seem more concise.</div><div>Would you be able to change that while merging, or should I post an update?</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>
--<br>
Kieran<br>
<br>
> > > uint64_t sum = 0;<br>
> > > uint32_t num = 0;<br>
> > > uint32_t *bin = stats->hist[0].g_hist;<br>
> > > --<br>
> > > 2.25.1<br>
> > ><br>
</blockquote></div></div>