[libcamera-devel] [PATCH 5/8] libcamera: camera: add state machine to control access from applications
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Feb 27 17:43:02 CET 2019
Hi Niklas,
Thank you for the patch.
On Tue, Feb 26, 2019 at 03:18:54AM +0100, Niklas Söderlund wrote:
> There is a need to better control the order of operations an application
> perform on a camera for it to function correctly. Add a basic state
s/perform/performs/
> machine to ensure applications perform operations on the camera in good
> order.
>
> Internal to the Camera four states are added; Disconnected, Free,
> Acquired 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 can perform.
>
> * Disconnected
> Is the lowest state a camera can be in. It indicates that the camera
> have been disconnected from the system and the only operations an
s/have/has/
> application shall do at this point is clean up and release the camera so
> it can be removed from libcamera as well.
I think you'll have to keep the connection status separate from the
state, otherwise an application won't be able to free buffers for a
disconnected camera. Disconnection should be separate from the state
machine related to the operations performed by the application, but of
course needs to be taken into account when checking whether a function
can be called or not.
I know that the current implementation doesn't handle this correctly
either, but I think we should fix it instead of moving the incorrect
behaviour to the new state machine implementation.
> * Free
> Is the base state of a camera, an application can inspect the properties
> of the camera to determine if it wish to use it or not. If an
s/wish/wishes/
> application wish to use a camera it should acquire it to proceed to the
s/wish/wishes/
> next state.
>
> * Acquired
> When an application have acquired a camera it have exclusive access to
s/have/has/g
> it and can modify the cameras parameters to prepare it for capturing.
s/cameras/camera's/
> Once the application is done configure the camera it may be started to
s/configure/configuring/
> progress to the running state. Once the camera is started it can not be
> reconfigured until it's stopped.
>
> * Running
> In this state the camera is running and able to process requests queued
> to it by the application. Once an application finish capturing the
s/finish/finishes/
> camera shall be stopped to change the state to acquired where it can be
> reconfigured or released.
All this should be captured in the documentation, with a state diagram
(probably as part of the Camera \class documentation). You can find an
example of how to embed dot diagrams in Doxygen at
http://www.doxygen.nl/manual/commands.html#cmddot.
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> include/libcamera/camera.h | 14 ++++-
> src/libcamera/camera.cpp | 110 +++++++++++++++++++++++--------------
> 2 files changed, 80 insertions(+), 44 deletions(-)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index bf70255a6a5ea364..8c8545b074e8ae13 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -55,20 +55,28 @@ public:
> int stop();
>
> private:
> + enum State {
> + Disconnected,
> + Free,
> + Acquired,
> + Running,
That's pretty generic and could lead to namespace collision. How about
prefixing these state names with Camera ?
> + };
> +
> Camera(PipelineHandler *pipe, const std::string &name);
> ~Camera();
>
> + bool stateIs(State state) const;
> + bool stateIsAtleast(State state) const;
> +
> friend class PipelineHandler;
> void disconnect();
> - int exclusiveAccess();
>
> std::shared_ptr<PipelineHandler> pipe_;
> std::string name_;
> std::vector<Stream *> streams_;
> std::vector<Stream *> activeStreams_;
>
> - bool acquired_;
> - bool disconnected_;
> + State state_;
> };
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index d4258fe3c7551af3..c50b14bbd904fc1c 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -116,17 +116,47 @@ 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), state_(Free)
> {
> }
>
> Camera::~Camera()
> {
> - if (acquired_)
> + if (state_ > Free)
> LOG(Camera, Error) << "Removing camera while still in use";
> }
>
> +static const std::string stateNames[] = {
> + "Disconnected",
> + "Free",
> + "Acquired",
> + "Running",
> +};
> +
> +bool Camera::stateIs(State state) const
> +{
> + if (state_ == state)
> + return true;
> +
> + LOG(Camera, Error) << "Camera in " << stateNames[state_]
> + << " state trying operation requiering "
s/requiering/requiring/
Is this an error, or could it be a warning ? Or possibly even a debug
mesage ? Are there cases when an application could try to call an
operation to check whether access is possible during the course of
normal operation ? I think that would be the case of the acquire()
function in particular. Same comment for the next function.
It would make sense to make the camera Loggable to print its name, but
that would require exposing the log.h header, which is likely not a good
idea. I think we can postpone this until we implement a Camera facade
object.
> + << stateNames[state] << " state";
> +
> + return false;
> +}
> +
> +bool Camera::stateIsAtleast(State state) const
> +{
> + if (state_ >= state)
> + return true;
> +
> + LOG(Camera, Error) << "Camera in " << stateNames[state_]
> + << " state trying operation requiering at least "
s/requiering/requiring/
> + << stateNames[state] << " state";
> +
> + return false;
> +}
> +
> /**
> * \brief Notify camera disconnection
> *
> @@ -140,7 +170,7 @@ void Camera::disconnect()
> {
> LOG(Camera, Debug) << "Disconnecting camera " << name_;
>
> - disconnected_ = true;
> + state_ = Disconnected;
> disconnected.emit(this);
> }
>
> @@ -162,10 +192,11 @@ void Camera::disconnect()
> */
> int Camera::acquire()
> {
> - if (acquired_)
> + if (!stateIs(Free))
> return -EBUSY;
>
> - acquired_ = true;
> + state_ = Acquired;
> +
> return 0;
> }
>
> @@ -177,7 +208,10 @@ int Camera::acquire()
> */
> void Camera::release()
> {
> - acquired_ = false;
> + if (!stateIs(Acquired))
> + return;
> +
> + state_ = Free;
Would it make sense to allow release() to be called on a Free camera, to
simplify error handling in the application ?
> }
>
> /**
> @@ -191,6 +225,9 @@ void Camera::release()
> */
> const std::vector<Stream *> &Camera::streams() const
> {
> + if (!stateIsAtleast(Free))
> + std::vector<Stream *>{};
Missing return ? This will lead you to another problem, which is that
you return a reference to a local object. There are multiple ways to
solve this, but I think you can drop the state check for this function,
as streams have the same lifetime as the camera, there's no need to
restrict the stream() function.
> +
> return streams_;
> }
>
> @@ -213,7 +250,7 @@ const std::vector<Stream *> &Camera::streams() const
> std::map<Stream *, StreamConfiguration>
> Camera::streamConfiguration(std::vector<Stream *> &streams)
> {
> - if (disconnected_ || !streams.size())
> + if (!stateIsAtleast(Free) || !streams.size())
> return std::map<Stream *, StreamConfiguration>{};
>
> return pipe_->streamConfiguration(this, streams);
> @@ -244,9 +281,8 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> {
> int ret;
>
> - ret = exclusiveAccess();
> - if (ret)
> - return ret;
> + if (!stateIs(Acquired))
> + return -EACCES;
>
> if (!config.size()) {
> LOG(Camera, Error)
> @@ -284,11 +320,8 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> */
> int Camera::allocateBuffers()
> {
> - int ret;
> -
> - ret = exclusiveAccess();
> - if (ret)
> - return ret;
> + if (!stateIs(Acquired))
> + return -EACCES;
I agree with Jacopo that a Configured state is likely needed.
>
> if (activeStreams_.empty()) {
> LOG(Camera, Error)
> @@ -297,7 +330,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();
> @@ -313,6 +346,9 @@ int Camera::allocateBuffers()
> */
> void Camera::freeBuffers()
> {
> + if (!stateIs(Acquired))
> + return;
> +
This allows calling freeBuffers() even when no buffers have been
allocated. I think it's a good idea, but it calls for allowing release()
on a released camera, as mentioned above.
> for (Stream *stream : activeStreams_) {
> if (!stream->bufferPool().count())
> continue;
> @@ -336,7 +372,7 @@ void Camera::freeBuffers()
> */
> Request *Camera::createRequest()
> {
> - if (exclusiveAccess())
> + if (!stateIsAtleast(Acquired))
> return nullptr;
>
> return new Request(this);
> @@ -358,13 +394,10 @@ Request *Camera::createRequest()
> */
> int Camera::queueRequest(Request *request)
> {
> - int ret;
> + if (!stateIs(Running))
> + return -EACCES;
>
> - ret = exclusiveAccess();
> - if (ret)
> - return ret;
> -
> - ret = request->prepare();
> + int ret = request->prepare();
> if (ret) {
> LOG(Camera, Error) << "Failed to prepare request";
> return ret;
> @@ -385,13 +418,18 @@ int Camera::queueRequest(Request *request)
> */
> int Camera::start()
> {
> - int ret = exclusiveAccess();
> - if (ret)
> - return ret;
> + if (!stateIs(Acquired))
> + return -EACCES;
>
> LOG(Camera, Debug) << "Starting capture";
>
> - return pipe_->start(this);
> + int ret = pipe_->start(this);
> + if (ret)
> + return ret;
> +
> + state_ = Running;
> +
> + return 0;
> }
>
> /**
> @@ -405,24 +443,14 @@ int Camera::start()
> */
> int Camera::stop()
> {
> - int ret = exclusiveAccess();
> - if (ret)
> - return ret;
> + if (!stateIs(Running))
> + return -EACCES;
>
> LOG(Camera, Debug) << "Stopping capture";
>
> pipe_->stop(this);
>
> - return 0;
> -}
> -
> -int Camera::exclusiveAccess()
> -{
> - if (disconnected_)
> - return -ENODEV;
> -
> - if (!acquired_)
> - return -EACCES;
> + state_ = Acquired;
>
> return 0;
> }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list