[libcamera-devel] [PATCH v2 3/4] libcamera: camera: add state machine to control access from applications
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Feb 28 18:12:22 CET 2019
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 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();
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 at ideasonboard.com>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list