[libcamera-devel] [PATCH v2 3/4] libcamera: camera: add state machine to control access from applications
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Feb 28 17:27:07 CET 2019
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 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?
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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list