[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 19:41:38 CET 2019
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 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.
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 at 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list