[libcamera-devel] [PATCH 0/3] Add SensorTemperature control

[Kieran Bingham]

Quoting Naushir Patuck via libcamera-devel (2022-06-23 16:16:12)
> On Thu, 23 Jun 2022 at 16:12, Dave Stevenson <dave.stevenson at raspberrypi.com>
> wrote:
> 
> > Hi Naush and Jean-Michel
> >
> > On Thu, 23 Jun 2022 at 15:56, Naushir Patuck via libcamera-devel
> > <libcamera-devel at lists.libcamera.org> wrote:
> > >
> > > Hi Jean-Michel,
> > >
> > > On Thu, 23 Jun 2022 at 15:51, Jean-Michel Hautbois <
> > jeanmichel.hautbois at yoseli.org> wrote:
> > >>
> > >>
> > >>
> > >> On 23/06/2022 16:50, Jean-Michel Hautbois via libcamera-devel wrote:
> > >> > Hi Naushir,
> > >> >
> > >> > Thanks for the series !
> > >> >
> > >> > On 23/06/2022 16:44, Naushir Patuck via libcamera-devel wrote:
> > >> >> Hi,
> > >> >>
> > >> >> This series adds a sensor temperature metadata control to libcamera.
> > >> >> This
> > >> >> control is returned out through Request metadata when available.
> > >> >> There is an
> > >> >> open PR [1] to enable the temperature sensor on the imx477 on the
> > >> >> Raspberry Pi
> > >> >> platforms.
> > >> >>
> > >> >> At present, there is no mechanism in V4L2 to return the temperature
> > >> >> from the
> > >> >> sensor device driver, so we rely on the embedded data to fetch the
> > >> >> values every
> > >> >> frame.
> > >> >
> > >> > I think this PR along with a V4L2 control introduction
> > >> > (V4L2_CID_TEMPERATURE :-) ?) could be interesting to be sent to the
> > >> > linux-media ML at the same time ?
> > >
> > >
> > > I was hoping to decouple this with V4L2 changes for now.  From what I
> > can tell,
> > > the topic of V4L2_CID_TEMPERATURE is a long ongoing discussion, and my
> > > change here does not rely on it.  So our users can get this feature
> > earlier!
> >
> > I've flagged it for discussion at the proposed Media Summit alongside
> > ELCE in September -
> > https://www.spinics.net/lists/linux-media/msg212772.html
> >
> > Laurent has responded in support of V4L2_CID_TEMPERATURE in preference
> > to hwmon API, but that still doesn't make it a done deal.
> > When Benjamin brought it up in
> >
> > https://lore.kernel.org/linux-media/20220415111845.27130-3-benjamin.mugnier@foss.st.com/
> > ,
> > it became quite a long debate with no resolution, leading to the patch
> > just being dropped.
> >
> > As Naush says, with imx477 (and others) carrying this value in
> > embedded metadata, we may as well extract it from there and avoid the
> > complication. It'll actually be a more up to date value than reading
> > it on demand over I2C.
> >
> 
> That's a good point - embedded data values will always be guaranteed to be
> synced with the
> frame in question.  But sync granularity might not be such a big deal for a
> temperature measurement...

Not critical no, - but it helps to know that it's at the right frame.

To me - reading this from the embedded metadata is 'more' correct
anyway. (At least from our libcamera perspective where we can access
that data). If we need a V4L2 control to be able to provide arbitrary
reads of the value - perhaps there's merit in still exposing it as a
control - but to me - having this available in the metadata is entirely
suitable for use.

And we don't have to wait for any kernel interface updates!

Perhaps there might be sensors that don't send the temperature value in
the embedded metadata, which might need the v4l2 control interface - but
that's not the case here so to me this seems like the right approach
overall.

Now to the details in the patches ...
--
Kieran


> Naush
> 
> 
> 
> >
> >   Dave
> >
> > >>
> > >>
> > >> I forgot to ask the use case though ? :-)
> > >
> > >
> > > We have had a few requests for this feature, see
> > https://github.com/raspberrypi/libcamera-apps/issues/306
> > > for the most recent one where the temperature value is useful for long
> > exposures to
> > > help with dark frame subtraction in post processing for astrophotography.
> > >
> > > Regards,
> > > Naush
> > >
> > >>
> > >>
> > >> >
> > >> > JM
> > >> >
> > >> >>
> > >> >> Thanks,
> > >> >> Naush
> > >> >>
> > >> >> [1] https://github.com/raspberrypi/linux/pull/5067
> > >> >>
> > >> >> Naushir Patuck (3):
> > >> >>    libcamera: controls: Add SensorTemperature control
> > >> >>    ipa: raspberrypi: Add sensor temperature to DeviceStatus
> > >> >>    ipa: raspberrypi: imx477: Get sensor temperature from embedded
> > data
> > >> >>
> > >> >>   src/ipa/raspberrypi/cam_helper.cpp               |  7 ++++---
> > >> >>   src/ipa/raspberrypi/cam_helper_imx477.cpp        |  5 ++++-
> > >> >>   src/ipa/raspberrypi/controller/device_status.cpp |  3 +++
> > >> >>   src/ipa/raspberrypi/controller/device_status.h   |  4 +++-
> > >> >>   src/ipa/raspberrypi/raspberrypi.cpp              |  2 ++
> > >> >>   src/libcamera/control_ids.yaml                   | 10 ++++++++++
> > >> >>   6 files changed, 26 insertions(+), 5 deletions(-)
> > >> >>
> >