[libcamera-devel] [PATCH v2 3/4] libcamera: camera: add state machine to control access from applications

[Kieran Bingham]

Hi Niklas,

A few quick comments, not necessarily a full review - I havent' been
through it all but was skim-reading.


On 28/02/2019 02:16, Niklas Söderlund wrote:
> There is a need to better control the order of operations an application
> performs on a camera for it to function correctly. Add a basic state
> machine to ensure applications perform operations on the camera in good
> order.
> 
> Internal to the Camera states are added; Available, Acquired,
> Configured, Prepared and Running. Each state represents a higher state
> of configuration of the camera ultimately leading to the highest state
> where the camera is capturing frames. Each state supports a subset of
> operations the application may perform.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  include/libcamera/camera.h |  18 ++-
>  src/libcamera/camera.cpp   | 230 ++++++++++++++++++++++++++++++-------
>  2 files changed, 203 insertions(+), 45 deletions(-)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 9c8ae01ed5c607f1..e5212cf05d221279 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -39,7 +39,7 @@ public:
>  	Signal<Camera *> disconnected;
>  
>  	int acquire();
> -	void release();
> +	int release();
>  
>  	const std::set<Stream *> &streams() const;
>  	std::map<Stream *, StreamConfiguration>
> @@ -47,7 +47,7 @@ public:
>  	int configureStreams(std::map<Stream *, StreamConfiguration> &config);
>  
>  	int allocateBuffers();
> -	void freeBuffers();
> +	int freeBuffers();
>  
>  	Request *createRequest();
>  	int queueRequest(Request *request);
> @@ -56,20 +56,30 @@ public:
>  	int stop();
>  
>  private:
> +	enum State {
> +		CameraAvailable,
> +		CameraAcquired,
> +		CameraConfigured,
> +		CameraPrepared,
> +		CameraRunning,

Do these need to be prefixed with Camera?

The enum is already scoped to the Camera class so the common prefix
feels a bit redundant?


> +	};
> +
>  	Camera(PipelineHandler *pipe, const std::string &name);
>  	~Camera();
>  
> +	bool stateBetween(State low, State high) const;
> +	bool stateIs(State state) const;
> +
>  	friend class PipelineHandler;
>  	void disconnect();
> -	int exclusiveAccess();
>  
>  	std::shared_ptr<PipelineHandler> pipe_;
>  	std::string name_;
>  	std::set<Stream *> streams_;
>  	std::set<Stream *> activeStreams_;
>  
> -	bool acquired_;
>  	bool disconnected_;
> +	State state_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 84b97b5c2ce94ecf..4f0833300b9b7ffc 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -52,6 +52,78 @@ LOG_DECLARE_CATEGORY(Camera)
>   * created with the create() function which returns a shared pointer. The
>   * Camera constructors and destructor are private, to prevent instances from
>   * being constructed and destroyed manually.
> + *
> + * \section camera_operation Operating the camera
> + *
> + * An application needs to preform a sequence of operations on a camera before
> + * it is ready to process requests. The camera needs to be acquired, configured
> + * and resources allocated or import to prepare it for start. Once started the
> + * camera can process requests until it's stopped. When an application is done
> + * with a camera all resources allocated to process requests needs to be freed
> + * and the camera released.
> + *
> + * An application may start and stop a camera multiple times as long as it's
> + * not released. The camera may also be reconfigured provided that all
> + * resources allocated are freed prior to the reconfiguration.
> + *
> + * \subsection Camera States
> + *
> + * To help manage the sequence of operation need to control the camera a set
> + * of states are defined. Each state describes which operations may be preformed
> + * on the camera.
> + *
> + * \dot
> + * digraph camera_state_machine {
> + *   node [shape = doublecircle ]; Available;

Out of interest, why do you double circle this state? Because it's the
initial state or such?

> + *   node [shape = circle ]; Acquired;
> + *   node [shape = circle ]; Configured;
> + *   node [shape = circle ]; Prepared;
> + *   node [shape = circle ]; Running;
> + *
> + *   Available -> Available [label = "release(), streams(), streamConfiguration()"];
> + *   Available -> Acquired [label = "acquire()"];
> + *
> + *   Acquired -> Available [label = "release()"];
> + *   Acquired -> Acquired [label = "streams(), streamConfiguration()"];
> + *   Acquired -> Configured [label = "configureStreams()"];
> + *
> + *   Configured -> Available [label = "release()"];
> + *   Configured -> Configured [label = "streams(), streamConfiguration(), configureStreams()"];
> + *   Configured -> Prepared [label = "allocateBuffers()"];
> + *
> + *   Prepared -> Configured [label = "freeBuffers()"];
> + *   Prepared -> Prepared [label = "streams(), streamConfiguration(), createRequest()"];
> + *   Prepared -> Running [label = "start()"];
> + *
> + *   Running -> Prepared [label = "stop()"];
> + *   Running -> Running [label = "streams(), streamConfiguration(), createRequest(), queueRequest()"];
> + * }
> + * \enddot
> + *
> + * \subsubsection Available
> + * The base state of a camera, an application can inspect the properties of the
> + * camera to determine if it wishes to use it. If an application wishes to use
> + * a camera it should acquire it to proceed to the acquired state.
> + *
> + * \subsubsection Acquired
> + * In the acquired state an application have exclusive access to the camera and
> + * may modify the camera's parameters to configure it and proceed to the
> + * configured state.
> + *
> + * \subsubsection Configured
> + * The camera is configured and ready for the application to prepare it with
> + * resources. The camera may be reconfigured multiple times until resources
> + * are provided and the state progressed to provided.
> + *
> + * \subsubsection Prepared
> + * Camera have been configured and provided with resource and is ready to be
> + * started. The application may free the camera's resources to get back to the
> + * configured state or start it to progress to the running state.
> + *
> + * \subsubsection Running
> + * The camera is running and ready to process requests queued by the
> + * application. The camera remains in this state until it's stopped and moved
> + * to the prepared state.
>   */
>  
>  /**
> @@ -116,17 +188,42 @@ const std::string &Camera::name() const
>   */
>  
>  Camera::Camera(PipelineHandler *pipe, const std::string &name)
> -	: pipe_(pipe->shared_from_this()), name_(name), acquired_(false),
> -	  disconnected_(false)
> +	: pipe_(pipe->shared_from_this()), name_(name), disconnected_(false),
> +	  state_(CameraAvailable)
>  {
>  }
>  
>  Camera::~Camera()
>  {
> -	if (acquired_)
> +	if (!stateIs(CameraAvailable))
>  		LOG(Camera, Error) << "Removing camera while still in use";
>  }
>  
> +bool Camera::stateBetween(State low, State high) const
> +{
> +	static const std::string stateNames[] = {
> +		"Available",
> +		"Acquired",
> +		"Configured",
> +		"Prepared",
> +		"Running",
> +	};

Should this be moved nearer to the enum so that it can be easily updated
if the states ever change? Perhaps not a big deal.


> +
> +	if (state_ >= low && state_ <= high)
> +		return true;
> +
> +	LOG(Camera, Debug) << "Camera in " << stateNames[state_]
> +			   << " state trying operation requiring state between "
> +			   << stateNames[low] << " and " << stateNames[high];
> +
> +	return false;
> +}

I wondered if any calls would be available in discontinuous states,
(like Configured and Running, but not prepared) but I can't imaging any
such use case - so I like this 'stateBetween' concept.

> +
> +bool Camera::stateIs(State state) const
> +{
> +	return stateBetween(state, state);

I suspect this may as well be it's own implementation.

If the state is correct, then it has to do two comparisons instead of
one to validate that, and if it's incorrect - then the LOG() message
will print saying it requires the state between RUNNING and RUNNING or
such ... which also seems a bit wierd.

*if* you choose to move stateNames out of stateBetween, a simple
implementation here would be clearer I think.

> +}
> +
>  /**
>   * \brief Notify camera disconnection
>   *
> @@ -140,6 +237,17 @@ void Camera::disconnect()
>  {
>  	LOG(Camera, Debug) << "Disconnecting camera " << name_;
>  
> +	/*
> +	 * If the camera was running when the hardware was removed force the
> +	 * state to prepared to allow applications to call freeBuffers() and
> +	 * release() before deleting the camera.
> +	 *
> +	 * \todo: Update comment when importing buffers as well as allocating
> +	 * them are supported.
> +	 */
> +	if (state_ == CameraRunning)
> +		state_ = CameraPrepared;
> +
>  	disconnected_ = true;
>  	disconnected.emit(this);
>  }
> @@ -158,13 +266,19 @@ void Camera::disconnect()
>   * \todo Implement exclusive access across processes.
>   *
>   * \return 0 on success or a negative error code otherwise
> + * \retval -ENODEV The camera is no longer connected to the hardware
> + * \retval -EBUSY The camera is not free and can't be acquired by the caller
>   */
>  int Camera::acquire()
>  {
> -	if (acquired_)
> +	if (disconnected_)
> +		return -ENODEV;
> +
> +	if (!stateIs(CameraAvailable))
>  		return -EBUSY;
>  
> -	acquired_ = true;
> +	state_ = CameraAcquired;
> +
>  	return 0;
>  }
>  
> @@ -173,10 +287,18 @@ int Camera::acquire()
>   *
>   * Releasing the camera device allows other users to acquire exclusive access
>   * with the acquire() function.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EBUSY The camera is running and can't be released

Perhaps we should add the state restrictions to the function API
documentation?

(That comment applies to all functions in Camera of course)


>   */
> -void Camera::release()
> +int Camera::release()
>  {
> -	acquired_ = false;
> +	if (!stateBetween(CameraAvailable, CameraConfigured))
> +		return -EBUSY;
> +
> +	state_ = CameraAvailable;
> +
> +	return 0;
>  }
>  
>  /**
> @@ -236,17 +358,19 @@ Camera::streamConfiguration(std::set<Stream *> &streams)
>   * to calling this function, otherwise an -EACCES error will be returned.
>   *
>   * \return 0 on success or a negative error code otherwise
> - * \retval -ENODEV The camera is not connected to any hardware
> - * \retval -EACCES The user has not acquired exclusive access to the camera
> + * \retval -ENODEV The camera is no longer connected to the hardware
> + * \retval -EACCES The camera is not in a state where it can be configured
>   * \retval -EINVAL The configuration is not valid
>   */
>  int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
>  {
>  	int ret;
>  
> -	ret = exclusiveAccess();
> -	if (ret)
> -		return ret;
> +	if (disconnected_)
> +		return -ENODEV;
> +
> +	if (!stateBetween(CameraAvailable, CameraConfigured))
> +		return -EACCES;
>  
>  	if (!config.size()) {
>  		LOG(Camera, Error)
> @@ -273,20 +397,25 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
>  		stream->bufferPool().createBuffers(cfg.bufferCount);
>  	}
>  
> +	state_ = CameraConfigured;
> +
>  	return 0;
>  }
>  
>  /**
>   * \brief Allocate buffers for all configured streams
>   * \return 0 on success or a negative error code otherwise
> + * \retval -ENODEV The camera is no longer connected to the hardware
> + * \retval -EACCES The camera is not in a state where buffers can be allocated
> + * \retval -EINVAL The configuration is not valid
>   */
>  int Camera::allocateBuffers()
>  {
> -	int ret;
> +	if (disconnected_)
> +		return -ENODEV;
>  
> -	ret = exclusiveAccess();
> -	if (ret)
> -		return ret;
> +	if (!stateIs(CameraConfigured))
> +		return -EACCES;
>  
>  	if (activeStreams_.empty()) {
>  		LOG(Camera, Error)
> @@ -295,7 +424,7 @@ int Camera::allocateBuffers()
>  	}
>  
>  	for (Stream *stream : activeStreams_) {
> -		ret = pipe_->allocateBuffers(this, stream);
> +		int ret = pipe_->allocateBuffers(this, stream);
>  		if (ret) {
>  			LOG(Camera, Error) << "Failed to allocate buffers";
>  			freeBuffers();
> @@ -303,14 +432,22 @@ int Camera::allocateBuffers()
>  		}
>  	}
>  
> +	state_ = CameraPrepared;

And a function which causes a state transition (if it succeeds) should
also document that in it's function documentation I think.


> +
>  	return 0;
>  }
>  
>  /**
>   * \brief Release all buffers from allocated pools in each stream
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EACCES The camera is not in a state where buffers can be freed
>   */
> -void Camera::freeBuffers()
> +int Camera::freeBuffers()
>  {
> +	if (!stateIs(CameraPrepared))
> +		return -EACCES;
> +
>  	for (Stream *stream : activeStreams_) {
>  		if (!stream->bufferPool().count())
>  			continue;
> @@ -318,6 +455,10 @@ void Camera::freeBuffers()
>  		pipe_->freeBuffers(this, stream);
>  		stream->bufferPool().destroyBuffers();
>  	}
> +
> +	state_ = CameraConfigured;
> +
> +	return 0;
>  }
>  
>  /**
> @@ -333,7 +474,7 @@ void Camera::freeBuffers()
>   */
>  Request *Camera::createRequest()
>  {
> -	if (exclusiveAccess())
> +	if (disconnected_ || !stateBetween(CameraPrepared, CameraRunning))>  		return nullptr;
>  
>  	return new Request(this);
> @@ -351,16 +492,18 @@ Request *Camera::createRequest()
>   * automatically after it completes.
>   *
>   * \return 0 on success or a negative error code otherwise
> + * \retval -ENODEV The camera is no longer connected to the hardware
> + * \retval -EACCES The camera is not running so requests can't be queued
>   */
>  int Camera::queueRequest(Request *request)
>  {
> -	int ret;
> +	if (disconnected_)
> +		return -ENODEV;
>  
> -	ret = exclusiveAccess();
> -	if (ret)
> -		return ret;
> +	if (!stateIs(CameraRunning))
> +		return -EACCES;
>  
> -	ret = request->prepare();
> +	int ret = request->prepare();
>  	if (ret) {
>  		LOG(Camera, Error) << "Failed to prepare request";
>  		return ret;
> @@ -377,16 +520,26 @@ int Camera::queueRequest(Request *request)
>   * until the capture session is terminated with \a stop().
>   *
>   * \return 0 on success or a negative error code otherwise
> + * \retval -ENODEV The camera is no longer connected to the hardware
> + * \retval -EACCES The camera is not in a state where it can be started
>   */
>  int Camera::start()
>  {
> -	int ret = exclusiveAccess();
> -	if (ret)
> -		return ret;
> +	if (disconnected_)
> +		return -ENODEV;
> +
> +	if (!stateIs(CameraPrepared))
> +		return -EACCES;
>  
>  	LOG(Camera, Debug) << "Starting capture";
>  
> -	return pipe_->start(this);
> +	int ret = pipe_->start(this);
> +	if (ret)
> +		return ret;
> +
> +	state_ = CameraRunning;
> +
> +	return 0;
>  }
>  
>  /**
> @@ -396,27 +549,22 @@ int Camera::start()
>   * requests are cancelled and complete synchronously in an error state.
>   *
>   * \return 0 on success or a negative error code otherwise
> + * \retval -ENODEV The camera is no longer connected to the hardware
> + * \retval -EACCES The camera is not running so can't be stopped
>   */
>  int Camera::stop()
>  {
> -	int ret = exclusiveAccess();
> -	if (ret)
> -		return ret;
> +	if (disconnected_)
> +		return -ENODEV;
> +
> +	if (!stateIs(CameraRunning))
> +		return -EACCES;
>  
>  	LOG(Camera, Debug) << "Stopping capture";
>  
>  	pipe_->stop(this);
>  
> -	return 0;
> -}
> -
> -int Camera::exclusiveAccess()
> -{
> -	if (disconnected_)
> -		return -ENODEV;
> -
> -	if (!acquired_)
> -		return -EACCES;
> +	state_ = CameraPrepared;
>  
>  	return 0;
>  }
> 

-- 
Regards
--
Kieran