[libcamera-devel,2/3] ipa: raspberrypi: Add sensor temperature to DeviceStatus
diff mbox series

Message ID 20220623144410.20725-3-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Add SensorTemperature control
Related show

Commit Message

Naushir Patuck June 23, 2022, 2:44 p.m. UTC 
Add a sensor_temperature field to the DeviceStatus structure. The default value
of -300.0 indicates a temperature measurement is unavilable. If a temperature
measurement is available for a frame, the value is returned out through the
SensorTemperature control in the Request metadata.

Additionally, provide the sensor temperature value from the std::ostream &operator<<
overload.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/raspberrypi/cam_helper.cpp               | 7 ++++---
 src/ipa/raspberrypi/controller/device_status.cpp | 3 +++
 src/ipa/raspberrypi/controller/device_status.h   | 4 +++-
 src/ipa/raspberrypi/raspberrypi.cpp              | 2 ++
 4 files changed, 12 insertions(+), 4 deletions(-)

Comments

Kieran Bingham June 23, 2022, 4:18 p.m. UTC | #1 
Quoting Naushir Patuck via libcamera-devel (2022-06-23 15:44:09)
> Add a sensor_temperature field to the DeviceStatus structure. The default value
> of -300.0 indicates a temperature measurement is unavilable. If a temperature

/unavilable/unavailable/

> measurement is available for a frame, the value is returned out through the
> SensorTemperature control in the Request metadata.

Rather than a 'fake default' - I think std::optional<double> could come
to the rescue here.

> 
> Additionally, provide the sensor temperature value from the std::ostream &operator<<
> overload.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp               | 7 ++++---
>  src/ipa/raspberrypi/controller/device_status.cpp | 3 +++
>  src/ipa/raspberrypi/controller/device_status.h   | 4 +++-
>  src/ipa/raspberrypi/raspberrypi.cpp              | 2 ++
>  4 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 74179399e86a..046428de8545 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -185,9 +185,9 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
>         metadata.Merge(parsedMetadata);
>  
>         /*
> -        * Overwrite the exposure/gain values in the existing DeviceStatus with
> -        * values from the parsed embedded buffer. Fetch it first in case any
> -        * other fields were set meaningfully.
> +        * Overwrite the exposure/gain, frame length and sensor temperature values
> +        * in the existing DeviceStatus with values from the parsed embedded buffer.
> +        * Fetch it first in case any other fields were set meaningfully.
>          */
>         DeviceStatus deviceStatus, parsedDeviceStatus;
>         if (metadata.Get("device.status", deviceStatus) ||
> @@ -199,6 +199,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
>         deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
>         deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
>         deviceStatus.frame_length = parsedDeviceStatus.frame_length;
> +       deviceStatus.sensor_temperature = parsedDeviceStatus.sensor_temperature;
>  
>         LOG(IPARPI, Debug) << "Metadata updated - " << deviceStatus;
>  
> diff --git a/src/ipa/raspberrypi/controller/device_status.cpp b/src/ipa/raspberrypi/controller/device_status.cpp
> index f052ea8b7bed..1a376eecb1c3 100644
> --- a/src/ipa/raspberrypi/controller/device_status.cpp
> +++ b/src/ipa/raspberrypi/controller/device_status.cpp
> @@ -17,5 +17,8 @@ std::ostream &operator<<(std::ostream &out, const DeviceStatus &d)
>             << " Lens: " << d.lens_position
>             << " Flash: " << d.flash_intensity;
>  
> +       if (d.sensor_temperature != -300)
> +               out << " Temperature: " << d.sensor_temperature;
> +
>         return out;
>  }
> diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h
> index c4a5d9c8e8c7..53cfeedc2956 100644
> --- a/src/ipa/raspberrypi/controller/device_status.h
> +++ b/src/ipa/raspberrypi/controller/device_status.h
> @@ -19,7 +19,7 @@ struct DeviceStatus {
>         DeviceStatus()
>                 : shutter_speed(std::chrono::seconds(0)), frame_length(0),
>                   analogue_gain(0.0), lens_position(0.0), aperture(0.0),
> -                 flash_intensity(0.0)
> +                 flash_intensity(0.0), sensor_temperature(-300.0)
>         {
>         }
>  
> @@ -36,4 +36,6 @@ struct DeviceStatus {
>         double aperture;
>         /* proportional to brightness with 0 = no flash, 1 = maximum flash */
>         double flash_intensity;
> +       /* Sensor reported temperature value (in degrees) */
> +       double sensor_temperature;
>  };
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 3b126bb5175e..fb91c8d11f6c 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -491,6 +491,8 @@ void IPARPi::reportMetadata()
>                 libcameraMetadata_.set(controls::AnalogueGain, deviceStatus->analogue_gain);
>                 libcameraMetadata_.set(controls::FrameDuration,
>                                        helper_->Exposure(deviceStatus->frame_length).get<std::micro>());
> +               if (deviceStatus->sensor_temperature != -300)
> +                       libcameraMetadata_.set(controls::SensorTemperature, deviceStatus->sensor_temperature);
>         }
>  
>         AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>("agc.status");
> -- 
> 2.25.1
>
Naushir Patuck June 24, 2022, 7:12 a.m. UTC | #2 
Hi Kieran,

Thank you for your feedback.

On Thu, 23 Jun 2022 at 17:18, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Quoting Naushir Patuck via libcamera-devel (2022-06-23 15:44:09)
> > Add a sensor_temperature field to the DeviceStatus structure. The
> default value
> > of -300.0 indicates a temperature measurement is unavilable. If a
> temperature
>
> /unavilable/unavailable/
>
> > measurement is available for a frame, the value is returned out through
> the
> > SensorTemperature control in the Request metadata.
>
> Rather than a 'fake default' - I think std::optional<double> could come
> to the rescue here.
>

I did consider std::optional, but decided against it to keep in the
"spirit" of the
DeviceStatus structure.  We have lens_position, aperture and
flash_intensity fields
that are optional but set with a default invalid value.

Having said that, I much prefer the std::optional approach as well, and I
will change
to use it.  As a separate patch, I will also switch the other optional
fields to use std::optional.

Regards,
Naush



>
> >
> > Additionally, provide the sensor temperature value from the std::ostream
> &operator<<
> > overload.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/cam_helper.cpp               | 7 ++++---
> >  src/ipa/raspberrypi/controller/device_status.cpp | 3 +++
> >  src/ipa/raspberrypi/controller/device_status.h   | 4 +++-
> >  src/ipa/raspberrypi/raspberrypi.cpp              | 2 ++
> >  4 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > index 74179399e86a..046428de8545 100644
> > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > @@ -185,9 +185,9 @@ void CamHelper::parseEmbeddedData(Span<const
> uint8_t> buffer,
> >         metadata.Merge(parsedMetadata);
> >
> >         /*
> > -        * Overwrite the exposure/gain values in the existing
> DeviceStatus with
> > -        * values from the parsed embedded buffer. Fetch it first in
> case any
> > -        * other fields were set meaningfully.
> > +        * Overwrite the exposure/gain, frame length and sensor
> temperature values
> > +        * in the existing DeviceStatus with values from the parsed
> embedded buffer.
> > +        * Fetch it first in case any other fields were set meaningfully.
> >          */
> >         DeviceStatus deviceStatus, parsedDeviceStatus;
> >         if (metadata.Get("device.status", deviceStatus) ||
> > @@ -199,6 +199,7 @@ void CamHelper::parseEmbeddedData(Span<const
> uint8_t> buffer,
> >         deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
> >         deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
> >         deviceStatus.frame_length = parsedDeviceStatus.frame_length;
> > +       deviceStatus.sensor_temperature =
> parsedDeviceStatus.sensor_temperature;
> >
> >         LOG(IPARPI, Debug) << "Metadata updated - " << deviceStatus;
> >
> > diff --git a/src/ipa/raspberrypi/controller/device_status.cpp
> b/src/ipa/raspberrypi/controller/device_status.cpp
> > index f052ea8b7bed..1a376eecb1c3 100644
> > --- a/src/ipa/raspberrypi/controller/device_status.cpp
> > +++ b/src/ipa/raspberrypi/controller/device_status.cpp
> > @@ -17,5 +17,8 @@ std::ostream &operator<<(std::ostream &out, const
> DeviceStatus &d)
> >             << " Lens: " << d.lens_position
> >             << " Flash: " << d.flash_intensity;
> >
> > +       if (d.sensor_temperature != -300)
> > +               out << " Temperature: " << d.sensor_temperature;
> > +
> >         return out;
> >  }
> > diff --git a/src/ipa/raspberrypi/controller/device_status.h
> b/src/ipa/raspberrypi/controller/device_status.h
> > index c4a5d9c8e8c7..53cfeedc2956 100644
> > --- a/src/ipa/raspberrypi/controller/device_status.h
> > +++ b/src/ipa/raspberrypi/controller/device_status.h
> > @@ -19,7 +19,7 @@ struct DeviceStatus {
> >         DeviceStatus()
> >                 : shutter_speed(std::chrono::seconds(0)),
> frame_length(0),
> >                   analogue_gain(0.0), lens_position(0.0), aperture(0.0),
> > -                 flash_intensity(0.0)
> > +                 flash_intensity(0.0), sensor_temperature(-300.0)
> >         {
> >         }
> >
> > @@ -36,4 +36,6 @@ struct DeviceStatus {
> >         double aperture;
> >         /* proportional to brightness with 0 = no flash, 1 = maximum
> flash */
> >         double flash_intensity;
> > +       /* Sensor reported temperature value (in degrees) */
> > +       double sensor_temperature;
> >  };
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 3b126bb5175e..fb91c8d11f6c 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -491,6 +491,8 @@ void IPARPi::reportMetadata()
> >                 libcameraMetadata_.set(controls::AnalogueGain,
> deviceStatus->analogue_gain);
> >                 libcameraMetadata_.set(controls::FrameDuration,
> >
> helper_->Exposure(deviceStatus->frame_length).get<std::micro>());
> > +               if (deviceStatus->sensor_temperature != -300)
> > +
>  libcameraMetadata_.set(controls::SensorTemperature,
> deviceStatus->sensor_temperature);
> >         }
> >
> >         AgcStatus *agcStatus =
> rpiMetadata_.GetLocked<AgcStatus>("agc.status");
> > --
> > 2.25.1
> >
>

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
index 74179399e86a..046428de8545 100644
--- a/src/ipa/raspberrypi/cam_helper.cpp
+++ b/src/ipa/raspberrypi/cam_helper.cpp
@@ -185,9 +185,9 @@  void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
 	metadata.Merge(parsedMetadata);
 
 	/*
-	 * Overwrite the exposure/gain values in the existing DeviceStatus with
-	 * values from the parsed embedded buffer. Fetch it first in case any
-	 * other fields were set meaningfully.
+	 * Overwrite the exposure/gain, frame length and sensor temperature values
+	 * in the existing DeviceStatus with values from the parsed embedded buffer.
+	 * Fetch it first in case any other fields were set meaningfully.
 	 */
 	DeviceStatus deviceStatus, parsedDeviceStatus;
 	if (metadata.Get("device.status", deviceStatus) ||
@@ -199,6 +199,7 @@  void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
 	deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
 	deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
 	deviceStatus.frame_length = parsedDeviceStatus.frame_length;
+	deviceStatus.sensor_temperature = parsedDeviceStatus.sensor_temperature;
 
 	LOG(IPARPI, Debug) << "Metadata updated - " << deviceStatus;
 
diff --git a/src/ipa/raspberrypi/controller/device_status.cpp b/src/ipa/raspberrypi/controller/device_status.cpp
index f052ea8b7bed..1a376eecb1c3 100644
--- a/src/ipa/raspberrypi/controller/device_status.cpp
+++ b/src/ipa/raspberrypi/controller/device_status.cpp
@@ -17,5 +17,8 @@  std::ostream &operator<<(std::ostream &out, const DeviceStatus &d)
 	    << " Lens: " << d.lens_position
 	    << " Flash: " << d.flash_intensity;
 
+	if (d.sensor_temperature != -300)
+		out << " Temperature: " << d.sensor_temperature;
+
 	return out;
 }
diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h
index c4a5d9c8e8c7..53cfeedc2956 100644
--- a/src/ipa/raspberrypi/controller/device_status.h
+++ b/src/ipa/raspberrypi/controller/device_status.h
@@ -19,7 +19,7 @@  struct DeviceStatus {
 	DeviceStatus()
 		: shutter_speed(std::chrono::seconds(0)), frame_length(0),
 		  analogue_gain(0.0), lens_position(0.0), aperture(0.0),
-		  flash_intensity(0.0)
+		  flash_intensity(0.0), sensor_temperature(-300.0)
 	{
 	}
 
@@ -36,4 +36,6 @@  struct DeviceStatus {
 	double aperture;
 	/* proportional to brightness with 0 = no flash, 1 = maximum flash */
 	double flash_intensity;
+	/* Sensor reported temperature value (in degrees) */
+	double sensor_temperature;
 };
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 3b126bb5175e..fb91c8d11f6c 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -491,6 +491,8 @@  void IPARPi::reportMetadata()
 		libcameraMetadata_.set(controls::AnalogueGain, deviceStatus->analogue_gain);
 		libcameraMetadata_.set(controls::FrameDuration,
 				       helper_->Exposure(deviceStatus->frame_length).get<std::micro>());
+		if (deviceStatus->sensor_temperature != -300)
+			libcameraMetadata_.set(controls::SensorTemperature, deviceStatus->sensor_temperature);
 	}
 
 	AgcStatus *agcStatus = rpiMetadata_.GetLocked<AgcStatus>("agc.status");