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

652 diff mbox series

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@ragnatech.se>
---
 include/libcamera/camera.h |  18 ++-
 src/libcamera/camera.cpp   | 230 ++++++++++++++++++++++++++++++-------
 2 files changed, 203 insertions(+), 45 deletions(-)

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@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;
>  }
>

Hi Kieran,

Thanks for your feedback.

On 2019-02-28 13:21:02 +0000, Kieran Bingham wrote:
> 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@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?

I added the prefixes in v2 after a comment from Laurent that they where 
very generic and might conflict with applications. As they are only used 
internally in the camera object I do not feel strongly one way or the 
other.

> 
> 
> > +	};
> > +
> >  	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?

Yes I wanted to somehow highlight the default state. I'm open to change 
or remove the double circle.

> 
> > + *   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.

The enum is defined in the header file and I don't think this would fit 
well in the header file. In v1 I had two functions checking states and 
then this was a global lookup table which I thought was kind of ugly. So 
in v2 where I opted to only have one function actually checking state I 
made ut a static local variable.

> 
> 
> > +
> > +	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.

Me neither :-) If this changes we can modify this later easily as it 
should only be used inside the camera object and not visible to the 
application.

> 
> > +
> > +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.

I had this in v1 but though it was ugly, I think this is a bit of a test 
issue, I will yield to popular demand. If someone else agrees with you I 
will turn this into its own implementation similar to v1.

> 
> > +}
> > +
> >  /**
> >   * \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)

I don't like this very much, that is why I tried to describe all of this 
in the class documentation. The idea was to collect all state machine 
information in one place and document the functions on there own. I fear 
adding this information in the class documentation and in the function 
documentation would just be copying the same sentences around adding 
little value. I'm open to discuss this however.

> 
> 
> >   */
> > -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.

Same comment as above.

> 
> 
> > +
> >  	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

On 2019-02-28 03:16:25 +0100, Niklas Söderlund wrote:

[snip]

> 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

[snip]

> @@ -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;

After a discussion on IRC it is agreed that the order of these 
operations should be swapped for next version,

    state_ = CameraPrepared;
    pipe_->stop(this)

Instead of how this patch changes it,

    pipe_->stop(this)
    state_ = CameraPrepared;

>  
>  	return 0;
>  }

Hello Niklas,

Thank you for the patch.

On Thu, Feb 28, 2019 at 05:27:07PM +0100, Niklas Söderlund wrote:
> On 2019-02-28 13:21:02 +0000, Kieran Bingham wrote:
> > 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@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();

I have a feeling applications won't check the return value of release().
I don't think we can do much about it though.

> >>  
> >>  	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();

Same here.

> >>  	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?
> 
> I added the prefixes in v2 after a comment from Laurent that they where 
> very generic and might conflict with applications. As they are only used 
> internally in the camera object I do not feel strongly one way or the 
> other.

I might have been overcautious there, if you think we should get rid of
the prefixes globally, I won't complain (that is unless I find a reason
to that would escape my mind right now :-)).

> >> +	};
> >> +
> >>  	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

s/camera/Camera/

> >> + *
> >> + * An application needs to preform a sequence of operations on a camera before

s/preform/perform/

> >> + * 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

s/import/imported/
s/it for start/the camera for capture/

> >> + * camera can process requests until it's stopped. When an application is done

s/it's/it is/

> >> + * with a camera all resources allocated to process requests needs to be freed

Maybe s/to process requests //

> >> + * and the camera released.
> >> + *
> >> + * An application may start and stop a camera multiple times as long as it's

s/it's/it is/

> >> + * 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

s/operation need/operations needed/

> >> + * of states are defined. Each state describes which operations may be preformed

s/preformed/performed/

> >> + * 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?
> 
> Yes I wanted to somehow highlight the default state. I'm open to change 
> or remove the double circle.

I recall that being a standard notation, but I couldn't quickly find a
document about it.

> >> + *   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

I'm tempted to leave the streams() and streamConfiguration() functions
out for clarity as they're allowed in all states. Maybe with a text
explaining that "methods not listed in the state diagram are allowed in
all states" ?

> >> + *
> >> + * \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.

s/acquire/acquire()/
s/acquired/Acquired/

> >> + *
> >> + * \subsubsection Acquired
> >> + * In the acquired state an application have exclusive access to the camera and

s/have/has/

> >> + * may modify the camera's parameters to configure it and proceed to the
> >> + * configured state.

s/configured/Configured/

> >> + *
> >> + * \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.

s/progressed/progresses/ ?
s/provided/Prepared/

> >> + *
> >> + * \subsubsection Prepared
> >> + * Camera have been configured and provided with resource and is ready to be

s/Camera have/The camera has/
s/resource/resources/

> >> + * 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.

s/configured/Configured/
s/running/Running/

> >> + *
> >> + * \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

s/it's/it is/

> >> + * to the prepared state.

s/prepared/Prepared/

> >>   */
> >>  
> >>  /**
> >> @@ -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.
> 
> The enum is defined in the header file and I don't think this would fit 
> well in the header file. In v1 I had two functions checking states and 
> then this was a global lookup table which I thought was kind of ugly. So 
> in v2 where I opted to only have one function actually checking state I 
> made ut a static local variable.

You could make a global camera_state_name() function and store the array
in it, just like we do in log.cpp.

> >> +
> >> +	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];

Should the caller also pass the operation name for debug purpose ? It
may not be worth it.

> >> +
> >> +	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.
> 
> Me neither :-) If this changes we can modify this later easily as it 
> should only be used inside the camera object and not visible to the 
> application.
> 
> >> +
> >> +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.
> 
> I had this in v1 but though it was ugly, I think this is a bit of a test 
> issue, I will yield to popular demand. If someone else agrees with you I 
> will turn this into its own implementation similar to v1.

I think I agree with Kieran :-)

> >> +}
> >> +
> >>  /**
> >>   * \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.

Seems that hot-unplug handling will require more work to complete
pending requests, right ?

> >> +	 *
> >> +	 * \todo: Update comment when importing buffers as well as allocating

As this isn't a doxygen comment (and rightfully so), you could use TODO
instead of \todo, but standardizing on \todo can also make sense.

> >> +	 * 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

The wording sounds a bit weird. How about "The camera has been
disconnected from the system" ?

> >> + * \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)
> 
> I don't like this very much, that is why I tried to describe all of this 
> in the class documentation. The idea was to collect all state machine 
> information in one place and document the functions on there own. I fear 
> adding this information in the class documentation and in the function 
> documentation would just be copying the same sentences around adding 
> little value. I'm open to discuss this however.

I like how the information is centralized. I wonder if we could just
link to the state machine though.

> >>   */
> >> -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

Should we add -ENOMEM, as that can be returned by
pipe_->allocateBuffers() ?

> >>   */
> >>  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.
> 
> Same comment as above.
> 
> >> +
> >>  	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;
> >>  }

With all the small issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Hi Laurent,

Thanks for your feedback.

On 2019-02-28 19:12:22 +0200, Laurent Pinchart wrote:
> Hello Niklas,
> 
> Thank you for the patch.
> 
> On Thu, Feb 28, 2019 at 05:27:07PM +0100, Niklas Söderlund wrote:
> > On 2019-02-28 13:21:02 +0000, Kieran Bingham wrote:
> > > 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@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();
> 
> I have a feeling applications won't check the return value of release().
> I don't think we can do much about it though.

I agree but I feel it's nicer to keep them for applications that do wish 
to check.

> 
> > >>  
> > >>  	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();
> 
> Same here.
> 
> > >>  	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?
> > 
> > I added the prefixes in v2 after a comment from Laurent that they where 
> > very generic and might conflict with applications. As they are only used 
> > internally in the camera object I do not feel strongly one way or the 
> > other.
> 
> I might have been overcautious there, if you think we should get rid of
> the prefixes globally, I won't complain (that is unless I find a reason
> to that would escape my mind right now :-)).

I like prefixing them, makes it clear where they come from. If we go for 
removing the prefixes I think it should be globally and then we can fix 
this one at the same go. I will keep them for now.

> 
> > >> +	};
> > >> +
> > >>  	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
> 
> s/camera/Camera/
> 
> > >> + *
> > >> + * An application needs to preform a sequence of operations on a camera before
> 
> s/preform/perform/
> 
> > >> + * 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
> 
> s/import/imported/
> s/it for start/the camera for capture/
> 
> > >> + * camera can process requests until it's stopped. When an application is done
> 
> s/it's/it is/
> 
> > >> + * with a camera all resources allocated to process requests needs to be freed
> 
> Maybe s/to process requests //
> 
> > >> + * and the camera released.
> > >> + *
> > >> + * An application may start and stop a camera multiple times as long as it's
> 
> s/it's/it is/
> 
> > >> + * 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
> 
> s/operation need/operations needed/
> 
> > >> + * of states are defined. Each state describes which operations may be preformed
> 
> s/preformed/performed/
> 

Thanks for this, I really appreciate your hard work reviewing my 
documentation work.

> > >> + * 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?
> > 
> > Yes I wanted to somehow highlight the default state. I'm open to change 
> > or remove the double circle.
> 
> I recall that being a standard notation, but I couldn't quickly find a
> document about it.

I had the same notion in my head but can't point to any reference. Will 
keep it as double circle for now.

> 
> > >> + *   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
> 
> I'm tempted to leave the streams() and streamConfiguration() functions
> out for clarity as they're allowed in all states. Maybe with a text
> explaining that "methods not listed in the state diagram are allowed in
> all states" ?

I tired it and the graph looks better, will act on your suggestion.

> 
> > >> + *
> > >> + * \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.
> 
> s/acquire/acquire()/
> s/acquired/Acquired/
> 
> > >> + *
> > >> + * \subsubsection Acquired
> > >> + * In the acquired state an application have exclusive access to the camera and
> 
> s/have/has/
> 
> > >> + * may modify the camera's parameters to configure it and proceed to the
> > >> + * configured state.
> 
> s/configured/Configured/
> 
> > >> + *
> > >> + * \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.
> 
> s/progressed/progresses/ ?
> s/provided/Prepared/
> 
> > >> + *
> > >> + * \subsubsection Prepared
> > >> + * Camera have been configured and provided with resource and is ready to be
> 
> s/Camera have/The camera has/
> s/resource/resources/
> 
> > >> + * 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.
> 
> s/configured/Configured/
> s/running/Running/
> 
> > >> + *
> > >> + * \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
> 
> s/it's/it is/
> 
> > >> + * to the prepared state.
> 
> s/prepared/Prepared/
> 
> > >>   */
> > >>  
> > >>  /**
> > >> @@ -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.
> > 
> > The enum is defined in the header file and I don't think this would fit 
> > well in the header file. In v1 I had two functions checking states and 
> > then this was a global lookup table which I thought was kind of ugly. So 
> > in v2 where I opted to only have one function actually checking state I 
> > made ut a static local variable.
> 
> You could make a global camera_state_name() function and store the array
> in it, just like we do in log.cpp.

I could but Camera::State is a private member and I would rather not 
expose that to applicants. I opted for this:

static const char *const camera_state_name[] = {
    ...
}

bool Camera::stateIs(State state) const
{
    ...

    ASSERT(static_cast<unsigned int>(state) < ARRAY_SIZE(camera_state_name));

    LOG(...) << ... << camera_state_name[state];

    ...
}

> 
> > >> +
> > >> +	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];
> 
> Should the caller also pass the operation name for debug purpose ? It
> may not be worth it.

I can see how it could be useful but I don't think it would be useful 
enough to add the plumbing for it. If we find out later it is it's 
trivial to add.

> 
> > >> +
> > >> +	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.
> > 
> > Me neither :-) If this changes we can modify this later easily as it 
> > should only be used inside the camera object and not visible to the 
> > application.
> > 
> > >> +
> > >> +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.
> > 
> > I had this in v1 but though it was ugly, I think this is a bit of a test 
> > issue, I will yield to popular demand. If someone else agrees with you I 
> > will turn this into its own implementation similar to v1.
> 
> I think I agree with Kieran :-)

The people have spoken, I shall make it so.

> 
> > >> +}
> > >> +
> > >>  /**
> > >>   * \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.
> 
> Seems that hot-unplug handling will require more work to complete
> pending requests, right ?

You are correct. I will add a todo so we won't forget.

> 
> > >> +	 *
> > >> +	 * \todo: Update comment when importing buffers as well as allocating
> 
> As this isn't a doxygen comment (and rightfully so), you could use TODO
> instead of \todo, but standardizing on \todo can also make sense.

I like the \todo as it shows up in the Documentation displaying our 
shame and forcing us to deal with the issues :-) You are however correct 
that this one will not show up in doxygen so I will move it so it do.

> 
> > >> +	 * 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
> 
> The wording sounds a bit weird. How about "The camera has been
> disconnected from the system" ?
> 
> > >> + * \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)
> > 
> > I don't like this very much, that is why I tried to describe all of this 
> > in the class documentation. The idea was to collect all state machine 
> > information in one place and document the functions on there own. I fear 
> > adding this information in the class documentation and in the function 
> > documentation would just be copying the same sentences around adding 
> > little value. I'm open to discuss this however.
> 
> I like how the information is centralized. I wonder if we could just
> link to the state machine though.

I will add the following to all functions capable of changing the 
cameras state,

    This function effects the state of the camera, see \ref 
    camera_operation.

> 
> > >>   */
> > >> -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
> 
> Should we add -ENOMEM, as that can be returned by
> pipe_->allocateBuffers() ?

I contemplated this as well but opted not do do so now and instead aim 
to add it to our task list to look over this for all methods of the 
Camera object.

> 
> > >>   */
> > >>  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.
> > 
> > Same comment as above.
> > 
> > >> +
> > >>  	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;
> > >>  }
> 
> With all the small issues fixed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks, so much changed due to your review so I don't feel right 
collecting your tag. I'm thinking of how the state name lookup was 
solved.

> 
> -- 
> Regards,
> 
> Laurent Pinchart