[libcamera-devel,v2,4/5] libcamera: camera_sensor: Add controls() method

10764 diff mbox series

Add a controls() method to retrieve the map of libcamera controls
registered by the CameraSensor class.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/internal/camera_sensor.h | 1 +
 src/libcamera/camera_sensor.cpp            | 6 ++++++
 2 files changed, 7 insertions(+)

Hi Jacopo,

Thank you for the patch.

On Mon, Dec 28, 2020 at 05:55:59PM +0100, Jacopo Mondi wrote:
> Add a controls() method to retrieve the map of libcamera controls
> registered by the CameraSensor class.
> 
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/internal/camera_sensor.h | 1 +
>  src/libcamera/camera_sensor.cpp            | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 841c7f4bef0f..10877b9d1d9d 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -62,6 +62,7 @@ public:
>  	int setControls(ControlList *ctrls);
>  
>  	const ControlList &properties() const { return properties_; }
> +	const ControlInfoMap &controls() const { return controls_; }
>  	int sensorInfo(CameraSensorInfo *info) const;
>  
>  protected:
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 337d73c524bf..ea57943647b1 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -653,6 +653,12 @@ ControlList CameraSensor::getControls(const std::vector<uint32_t> &ids)
>   * \return The list of camera sensor properties
>   */
>  
> +/**
> + * \fn CameraSensor::controls()
> + * \brief Retrieve the camera sensor controls info

This needs to describe what those controls are. You're using libcamera
controls at the moment, which is OK as an interim measure, but we'll
need to define sensor-specific controls sooner than latter. This should
be mentioned, with a warning stating that those controls must not be
copied verbatim to the controls exposed by the Camera to applications.
Similarly, the subdevControls() documentation should state that the
function is deprecated.

This brings a second question, which is whether using controls for this
is actually the right API. There's little that the camera sensor needs
to expose, and a specific API may make more sense.

> + * \return The map of camera sensor controls info
> + */
> +
>  /**
>   * \brief Write controls to the sensor
>   * \param[in] ctrls The list of controls to write