[{"id":945,"web_url":"https://patchwork.libcamera.org/comment/945/","msgid":"<74ce331d-afba-62b7-d5a9-531b6729b5cc@ideasonboard.com>","date":"2019-02-28T13:21:02","subject":"Re: [libcamera-devel] [PATCH v2 3/4] libcamera: camera: add state\n\tmachine to control access from applications","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nA few quick comments, not necessarily a full review - I havent' been\nthrough it all but was skim-reading.\n\n\nOn 28/02/2019 02:16, Niklas Söderlund wrote:\n> There is a need to better control the order of operations an application\n> performs on a camera for it to function correctly. Add a basic state\n> machine to ensure applications perform operations on the camera in good\n> order.\n> \n> Internal to the Camera states are added; Available, Acquired,\n> Configured, Prepared and Running. Each state represents a higher state\n> of configuration of the camera ultimately leading to the highest state\n> where the camera is capturing frames. Each state supports a subset of\n> operations the application may perform.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/camera.h | 18 ++-\n> src/libcamera/camera.cpp | 230 ++++++++++++++++++++++++++++++-------\n> 2 files changed, 203 insertions(+), 45 deletions(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 9c8ae01ed5c607f1..e5212cf05d221279 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -39,7 +39,7 @@ public:\n> \tSignal disconnected;\n> \n> \tint acquire();\n> -\tvoid release();\n> +\tint release();\n> \n> \tconst std::set &streams() const;\n> \tstd::map\n> @@ -47,7 +47,7 @@ public:\n> \tint configureStreams(std::map &config);\n> \n> \tint allocateBuffers();\n> -\tvoid freeBuffers();\n> +\tint freeBuffers();\n> \n> \tRequest *createRequest();\n> \tint queueRequest(Request *request);\n> @@ -56,20 +56,30 @@ public:\n> \tint stop();\n> \n> private:\n> +\tenum State {\n> +\t\tCameraAvailable,\n> +\t\tCameraAcquired,\n> +\t\tCameraConfigured,\n> +\t\tCameraPrepared,\n> +\t\tCameraRunning,\n\nDo these need to be prefixed with Camera?\n\nThe enum is already scoped to the Camera class so the common prefix\nfeels a bit redundant?\n\n\n> +\t};\n> +\n> \tCamera(PipelineHandler *pipe, const std::string &name);\n> \t~Camera();\n> \n> +\tbool stateBetween(State low, State high) const;\n> +\tbool stateIs(State state) const;\n> +\n> \tfriend class PipelineHandler;\n> \tvoid disconnect();\n> -\tint exclusiveAccess();\n> \n> \tstd::shared_ptr pipe_;\n> \tstd::string name_;\n> \tstd::set streams_;\n> \tstd::set activeStreams_;\n> \n> -\tbool acquired_;\n> \tbool disconnected_;\n> +\tState state_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 84b97b5c2ce94ecf..4f0833300b9b7ffc 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -52,6 +52,78 @@ LOG_DECLARE_CATEGORY(Camera)\n> * created with the create() function which returns a shared pointer. The\n> * Camera constructors and destructor are private, to prevent instances from\n> * being constructed and destroyed manually.\n> + *\n> + * \\section camera_operation Operating the camera\n> + *\n> + * An application needs to preform a sequence of operations on a camera before\n> + * it is ready to process requests. The camera needs to be acquired, configured\n> + * and resources allocated or import to prepare it for start. Once started the\n> + * camera can process requests until it's stopped. When an application is done\n> + * with a camera all resources allocated to process requests needs to be freed\n> + * and the camera released.\n> + *\n> + * An application may start and stop a camera multiple times as long as it's\n> + * not released. The camera may also be reconfigured provided that all\n> + * resources allocated are freed prior to the reconfiguration.\n> + *\n> + * \\subsection Camera States\n> + *\n> + * To help manage the sequence of operation need to control the camera a set\n> + * of states are defined. Each state describes which operations may be preformed\n> + * on the camera.\n> + *\n> + * \\dot\n> + * digraph camera_state_machine {\n> + * node [shape = doublecircle ]; Available;\n\nOut of interest, why do you double circle this state? Because it's the\ninitial state or such?\n\n> + * node [shape = circle ]; Acquired;\n> + * node [shape = circle ]; Configured;\n> + * node [shape = circle ]; Prepared;\n> + * node [shape = circle ]; Running;\n> + *\n> + * Available -> Available [label = \"release(), streams(), streamConfiguration()\"];\n> + * Available -> Acquired [label = \"acquire()\"];\n> + *\n> + * Acquired -> Available [label = \"release()\"];\n> + * Acquired -> Acquired [label = \"streams(), streamConfiguration()\"];\n> + * Acquired -> Configured [label = \"configureStreams()\"];\n> + *\n> + * Configured -> Available [label = \"release()\"];\n> + * Configured -> Configured [label = \"streams(), streamConfiguration(), configureStreams()\"];\n> + * Configured -> Prepared [label = \"allocateBuffers()\"];\n> + *\n> + * Prepared -> Configured [label = \"freeBuffers()\"];\n> + * Prepared -> Prepared [label = \"streams(), streamConfiguration(), createRequest()\"];\n> + * Prepared -> Running [label = \"start()\"];\n> + *\n> + * Running -> Prepared [label = \"stop()\"];\n> + * Running -> Running [label = \"streams(), streamConfiguration(), createRequest(), queueRequest()\"];\n> + * }\n> + * \\enddot\n> + *\n> + * \\subsubsection Available\n> + * The base state of a camera, an application can inspect the properties of the\n> + * camera to determine if it wishes to use it. If an application wishes to use\n> + * a camera it should acquire it to proceed to the acquired state.\n> + *\n> + * \\subsubsection Acquired\n> + * In the acquired state an application have exclusive access to the camera and\n> + * may modify the camera's parameters to configure it and proceed to the\n> + * configured state.\n> + *\n> + * \\subsubsection Configured\n> + * The camera is configured and ready for the application to prepare it with\n> + * resources. The camera may be reconfigured multiple times until resources\n> + * are provided and the state progressed to provided.\n> + *\n> + * \\subsubsection Prepared\n> + * Camera have been configured and provided with resource and is ready to be\n> + * started. The application may free the camera's resources to get back to the\n> + * configured state or start it to progress to the running state.\n> + *\n> + * \\subsubsection Running\n> + * The camera is running and ready to process requests queued by the\n> + * application. The camera remains in this state until it's stopped and moved\n> + * to the prepared state.\n> */\n> \n> /**\n> @@ -116,17 +188,42 @@ const std::string &Camera::name() const\n> */\n> \n> Camera::Camera(PipelineHandler *pipe, const std::string &name)\n> -\t: pipe_(pipe->shared_from_this()), name_(name), acquired_(false),\n> -\t disconnected_(false)\n> +\t: pipe_(pipe->shared_from_this()), name_(name), disconnected_(false),\n> +\t state_(CameraAvailable)\n> {\n> }\n> \n> Camera::~Camera()\n> {\n> -\tif (acquired_)\n> +\tif (!stateIs(CameraAvailable))\n> \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> }\n> \n> +bool Camera::stateBetween(State low, State high) const\n> +{\n> +\tstatic const std::string stateNames[] = {\n> +\t\t\"Available\",\n> +\t\t\"Acquired\",\n> +\t\t\"Configured\",\n> +\t\t\"Prepared\",\n> +\t\t\"Running\",\n> +\t};\n\nShould this be moved nearer to the enum so that it can be easily updated\nif the states ever change? Perhaps not a big deal.\n\n\n> +\n> +\tif (state_ >= low && state_ <= high)\n> +\t\treturn true;\n> +\n> +\tLOG(Camera, Debug) << \"Camera in \" << stateNames[state_]\n> +\t\t\t << \" state trying operation requiring state between \"\n> +\t\t\t << stateNames[low] << \" and \" << stateNames[high];\n> +\n> +\treturn false;\n> +}\n\nI wondered if any calls would be available in discontinuous states,\n(like Configured and Running, but not prepared) but I can't imaging any\nsuch use case - so I like this 'stateBetween' concept.\n\n> +\n> +bool Camera::stateIs(State state) const\n> +{\n> +\treturn stateBetween(state, state);\n\nI suspect this may as well be it's own implementation.\n\nIf the state is correct, then it has to do two comparisons instead of\none to validate that, and if it's incorrect - then the LOG() message\nwill print saying it requires the state between RUNNING and RUNNING or\nsuch ... which also seems a bit wierd.\n\n*if* you choose to move stateNames out of stateBetween, a simple\nimplementation here would be clearer I think.\n\n> +}\n> +\n> /**\n> * \\brief Notify camera disconnection\n> *\n> @@ -140,6 +237,17 @@ void Camera::disconnect()\n> {\n> \tLOG(Camera, Debug) << \"Disconnecting camera \" << name_;\n> \n> +\t/*\n> +\t * If the camera was running when the hardware was removed force the\n> +\t * state to prepared to allow applications to call freeBuffers() and\n> +\t * release() before deleting the camera.\n> +\t *\n> +\t * \\todo: Update comment when importing buffers as well as allocating\n> +\t * them are supported.\n> +\t */\n> +\tif (state_ == CameraRunning)\n> +\t\tstate_ = CameraPrepared;\n> +\n> \tdisconnected_ = true;\n> \tdisconnected.emit(this);\n> }\n> @@ -158,13 +266,19 @@ void Camera::disconnect()\n> * \\todo Implement exclusive access across processes.\n> *\n> * \\return 0 on success or a negative error code otherwise\n> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> + * \\retval -EBUSY The camera is not free and can't be acquired by the caller\n> */\n> int Camera::acquire()\n> {\n> -\tif (acquired_)\n> +\tif (disconnected_)\n> +\t\treturn -ENODEV;\n> +\n> +\tif (!stateIs(CameraAvailable))\n> \t\treturn -EBUSY;\n> \n> -\tacquired_ = true;\n> +\tstate_ = CameraAcquired;\n> +\n> \treturn 0;\n> }\n> \n> @@ -173,10 +287,18 @@ int Camera::acquire()\n> *\n> * Releasing the camera device allows other users to acquire exclusive access\n> * with the acquire() function.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EBUSY The camera is running and can't be released\n\nPerhaps we should add the state restrictions to the function API\ndocumentation?\n\n(That comment applies to all functions in Camera of course)\n\n\n> */\n> -void Camera::release()\n> +int Camera::release()\n> {\n> -\tacquired_ = false;\n> +\tif (!stateBetween(CameraAvailable, CameraConfigured))\n> +\t\treturn -EBUSY;\n> +\n> +\tstate_ = CameraAvailable;\n> +\n> +\treturn 0;\n> }\n> \n> /**\n> @@ -236,17 +358,19 @@ Camera::streamConfiguration(std::set &streams)\n> * to calling this function, otherwise an -EACCES error will be returned.\n> *\n> * \\return 0 on success or a negative error code otherwise\n> - * \\retval -ENODEV The camera is not connected to any hardware\n> - * \\retval -EACCES The user has not acquired exclusive access to the camera\n> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> + * \\retval -EACCES The camera is not in a state where it can be configured\n> * \\retval -EINVAL The configuration is not valid\n> */\n> int Camera::configureStreams(std::map &config)\n> {\n> \tint ret;\n> \n> -\tret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (disconnected_)\n> +\t\treturn -ENODEV;\n> +\n> +\tif (!stateBetween(CameraAvailable, CameraConfigured))\n> +\t\treturn -EACCES;\n> \n> \tif (!config.size()) {\n> \t\tLOG(Camera, Error)\n> @@ -273,20 +397,25 @@ int Camera::configureStreams(std::map &config)\n> \t\tstream->bufferPool().createBuffers(cfg.bufferCount);\n> \t}\n> \n> +\tstate_ = CameraConfigured;\n> +\n> \treturn 0;\n> }\n> \n> /**\n> * \\brief Allocate buffers for all configured streams\n> * \\return 0 on success or a negative error code otherwise\n> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> + * \\retval -EACCES The camera is not in a state where buffers can be allocated\n> + * \\retval -EINVAL The configuration is not valid\n> */\n> int Camera::allocateBuffers()\n> {\n> -\tint ret;\n> +\tif (disconnected_)\n> +\t\treturn -ENODEV;\n> \n> -\tret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (!stateIs(CameraConfigured))\n> +\t\treturn -EACCES;\n> \n> \tif (activeStreams_.empty()) {\n> \t\tLOG(Camera, Error)\n> @@ -295,7 +424,7 @@ int Camera::allocateBuffers()\n> \t}\n> \n> \tfor (Stream *stream : activeStreams_) {\n> -\t\tret = pipe_->allocateBuffers(this, stream);\n> +\t\tint ret = pipe_->allocateBuffers(this, stream);\n> \t\tif (ret) {\n> \t\t\tLOG(Camera, Error) << \"Failed to allocate buffers\";\n> \t\t\tfreeBuffers();\n> @@ -303,14 +432,22 @@ int Camera::allocateBuffers()\n> \t\t}\n> \t}\n> \n> +\tstate_ = CameraPrepared;\n\nAnd a function which causes a state transition (if it succeeds) should\nalso document that in it's function documentation I think.\n\n\n> +\n> \treturn 0;\n> }\n> \n> /**\n> * \\brief Release all buffers from allocated pools in each stream\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EACCES The camera is not in a state where buffers can be freed\n> */\n> -void Camera::freeBuffers()\n> +int Camera::freeBuffers()\n> {\n> +\tif (!stateIs(CameraPrepared))\n> +\t\treturn -EACCES;\n> +\n> \tfor (Stream *stream : activeStreams_) {\n> \t\tif (!stream->bufferPool().count())\n> \t\t\tcontinue;\n> @@ -318,6 +455,10 @@ void Camera::freeBuffers()\n> \t\tpipe_->freeBuffers(this, stream);\n> \t\tstream->bufferPool().destroyBuffers();\n> \t}\n> +\n> +\tstate_ = CameraConfigured;\n> +\n> +\treturn 0;\n> }\n> \n> /**\n> @@ -333,7 +474,7 @@ void Camera::freeBuffers()\n> */\n> Request *Camera::createRequest()\n> {\n> -\tif (exclusiveAccess())\n> +\tif (disconnected_ || !stateBetween(CameraPrepared, CameraRunning))> \t\treturn nullptr;\n> \n> \treturn new Request(this);\n> @@ -351,16 +492,18 @@ Request *Camera::createRequest()\n> * automatically after it completes.\n> *\n> * \\return 0 on success or a negative error code otherwise\n> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> + * \\retval -EACCES The camera is not running so requests can't be queued\n> */\n> int Camera::queueRequest(Request *request)\n> {\n> -\tint ret;\n> +\tif (disconnected_)\n> +\t\treturn -ENODEV;\n> \n> -\tret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (!stateIs(CameraRunning))\n> +\t\treturn -EACCES;\n> \n> -\tret = request->prepare();\n> +\tint ret = request->prepare();\n> \tif (ret) {\n> \t\tLOG(Camera, Error) << \"Failed to prepare request\";\n> \t\treturn ret;\n> @@ -377,16 +520,26 @@ int Camera::queueRequest(Request *request)\n> * until the capture session is terminated with \\a stop().\n> *\n> * \\return 0 on success or a negative error code otherwise\n> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> + * \\retval -EACCES The camera is not in a state where it can be started\n> */\n> int Camera::start()\n> {\n> -\tint ret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (disconnected_)\n> +\t\treturn -ENODEV;\n> +\n> +\tif (!stateIs(CameraPrepared))\n> +\t\treturn -EACCES;\n> \n> \tLOG(Camera, Debug) << \"Starting capture\";\n> \n> -\treturn pipe_->start(this);\n> +\tint ret = pipe_->start(this);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tstate_ = CameraRunning;\n> +\n> +\treturn 0;\n> }\n> \n> /**\n> @@ -396,27 +549,22 @@ int Camera::start()\n> * requests are cancelled and complete synchronously in an error state.\n> *\n> * \\return 0 on success or a negative error code otherwise\n> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> + * \\retval -EACCES The camera is not running so can't be stopped\n> */\n> int Camera::stop()\n> {\n> -\tint ret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (disconnected_)\n> +\t\treturn -ENODEV;\n> +\n> +\tif (!stateIs(CameraRunning))\n> +\t\treturn -EACCES;\n> \n> \tLOG(Camera, Debug) << \"Stopping capture\";\n> \n> \tpipe_->stop(this);\n> \n> -\treturn 0;\n> -}\n> -\n> -int Camera::exclusiveAccess()\n> -{\n> -\tif (disconnected_)\n> -\t\treturn -ENODEV;\n> -\n> -\tif (!acquired_)\n> -\t\treturn -EACCES;\n> +\tstate_ = CameraPrepared;\n> \n> \treturn 0;\n> }\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7959E601E2\n\tfor ;\n\tThu, 28 Feb 2019 14:21:05 +0100 (CET)","from [192.168.0.21]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CDF6149;\n\tThu, 28 Feb 2019 14:21:04 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551360065;\n\tbh=KKJ8arJ68EKBUDfb1l6bpS2Cx27GWZ/QysRhH4Dmk4A=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=nk3wRDQFbXsdAF7xjy1CXFHPHLUEoREE9ZWJ6PF0h1Fj2ZTubKmHDvMxv1aGM9phY\n\tLhXqoB862j0LDhHUYqKu2HJUY80SNtXhZUYaVBq5WlsnjoddoDbWtXPMRzCrcRL83y\n\tkkrGNPKfuwNkdHh3dmt5ntwJ32e8cWcCR5A7WAqs=","Reply-To":"kieran.bingham@ideasonboard.com","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20190228021626.15062-1-niklas.soderlund@ragnatech.se>\n\t<20190228021626.15062-4-niklas.soderlund@ragnatech.se>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<74ce331d-afba-62b7-d5a9-531b6729b5cc@ideasonboard.com>","Date":"Thu, 28 Feb 2019 13:21:02 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.4.0","MIME-Version":"1.0","In-Reply-To":"<20190228021626.15062-4-niklas.soderlund@ragnatech.se>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] libcamera: camera: add state\n\tmachine to control access from applications","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 28 Feb 2019 13:21:05 -0000"}},{"id":946,"web_url":"https://patchwork.libcamera.org/comment/946/","msgid":"<20190228162707.GB899@bigcity.dyn.berto.se>","date":"2019-02-28T16:27:07","subject":"Re: [libcamera-devel] [PATCH v2 3/4] libcamera: camera: add state\n\tmachine to control access from applications","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Kieran,\n\nThanks for your feedback.\n\nOn 2019-02-28 13:21:02 +0000, Kieran Bingham wrote:\n> Hi Niklas,\n> \n> A few quick comments, not necessarily a full review - I havent' been\n> through it all but was skim-reading.\n> \n> \n> On 28/02/2019 02:16, Niklas Söderlund wrote:\n> > There is a need to better control the order of operations an application\n> > performs on a camera for it to function correctly. Add a basic state\n> > machine to ensure applications perform operations on the camera in good\n> > order.\n> > \n> > Internal to the Camera states are added; Available, Acquired,\n> > Configured, Prepared and Running. Each state represents a higher state\n> > of configuration of the camera ultimately leading to the highest state\n> > where the camera is capturing frames. Each state supports a subset of\n> > operations the application may perform.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/camera.h | 18 ++-\n> > src/libcamera/camera.cpp | 230 ++++++++++++++++++++++++++++++-------\n> > 2 files changed, 203 insertions(+), 45 deletions(-)\n> > \n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 9c8ae01ed5c607f1..e5212cf05d221279 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -39,7 +39,7 @@ public:\n> > \tSignal disconnected;\n> > \n> > \tint acquire();\n> > -\tvoid release();\n> > +\tint release();\n> > \n> > \tconst std::set &streams() const;\n> > \tstd::map\n> > @@ -47,7 +47,7 @@ public:\n> > \tint configureStreams(std::map &config);\n> > \n> > \tint allocateBuffers();\n> > -\tvoid freeBuffers();\n> > +\tint freeBuffers();\n> > \n> > \tRequest *createRequest();\n> > \tint queueRequest(Request *request);\n> > @@ -56,20 +56,30 @@ public:\n> > \tint stop();\n> > \n> > private:\n> > +\tenum State {\n> > +\t\tCameraAvailable,\n> > +\t\tCameraAcquired,\n> > +\t\tCameraConfigured,\n> > +\t\tCameraPrepared,\n> > +\t\tCameraRunning,\n> \n> Do these need to be prefixed with Camera?\n> \n> The enum is already scoped to the Camera class so the common prefix\n> feels a bit redundant?\n\nI added the prefixes in v2 after a comment from Laurent that they where \nvery generic and might conflict with applications. As they are only used \ninternally in the camera object I do not feel strongly one way or the \nother.\n\n> \n> \n> > +\t};\n> > +\n> > \tCamera(PipelineHandler *pipe, const std::string &name);\n> > \t~Camera();\n> > \n> > +\tbool stateBetween(State low, State high) const;\n> > +\tbool stateIs(State state) const;\n> > +\n> > \tfriend class PipelineHandler;\n> > \tvoid disconnect();\n> > -\tint exclusiveAccess();\n> > \n> > \tstd::shared_ptr pipe_;\n> > \tstd::string name_;\n> > \tstd::set streams_;\n> > \tstd::set activeStreams_;\n> > \n> > -\tbool acquired_;\n> > \tbool disconnected_;\n> > +\tState state_;\n> > };\n> > \n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 84b97b5c2ce94ecf..4f0833300b9b7ffc 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -52,6 +52,78 @@ LOG_DECLARE_CATEGORY(Camera)\n> > * created with the create() function which returns a shared pointer. The\n> > * Camera constructors and destructor are private, to prevent instances from\n> > * being constructed and destroyed manually.\n> > + *\n> > + * \\section camera_operation Operating the camera\n> > + *\n> > + * An application needs to preform a sequence of operations on a camera before\n> > + * it is ready to process requests. The camera needs to be acquired, configured\n> > + * and resources allocated or import to prepare it for start. Once started the\n> > + * camera can process requests until it's stopped. When an application is done\n> > + * with a camera all resources allocated to process requests needs to be freed\n> > + * and the camera released.\n> > + *\n> > + * An application may start and stop a camera multiple times as long as it's\n> > + * not released. The camera may also be reconfigured provided that all\n> > + * resources allocated are freed prior to the reconfiguration.\n> > + *\n> > + * \\subsection Camera States\n> > + *\n> > + * To help manage the sequence of operation need to control the camera a set\n> > + * of states are defined. Each state describes which operations may be preformed\n> > + * on the camera.\n> > + *\n> > + * \\dot\n> > + * digraph camera_state_machine {\n> > + * node [shape = doublecircle ]; Available;\n> \n> Out of interest, why do you double circle this state? Because it's the\n> initial state or such?\n\nYes I wanted to somehow highlight the default state. I'm open to change \nor remove the double circle.\n\n> \n> > + * node [shape = circle ]; Acquired;\n> > + * node [shape = circle ]; Configured;\n> > + * node [shape = circle ]; Prepared;\n> > + * node [shape = circle ]; Running;\n> > + *\n> > + * Available -> Available [label = \"release(), streams(), streamConfiguration()\"];\n> > + * Available -> Acquired [label = \"acquire()\"];\n> > + *\n> > + * Acquired -> Available [label = \"release()\"];\n> > + * Acquired -> Acquired [label = \"streams(), streamConfiguration()\"];\n> > + * Acquired -> Configured [label = \"configureStreams()\"];\n> > + *\n> > + * Configured -> Available [label = \"release()\"];\n> > + * Configured -> Configured [label = \"streams(), streamConfiguration(), configureStreams()\"];\n> > + * Configured -> Prepared [label = \"allocateBuffers()\"];\n> > + *\n> > + * Prepared -> Configured [label = \"freeBuffers()\"];\n> > + * Prepared -> Prepared [label = \"streams(), streamConfiguration(), createRequest()\"];\n> > + * Prepared -> Running [label = \"start()\"];\n> > + *\n> > + * Running -> Prepared [label = \"stop()\"];\n> > + * Running -> Running [label = \"streams(), streamConfiguration(), createRequest(), queueRequest()\"];\n> > + * }\n> > + * \\enddot\n> > + *\n> > + * \\subsubsection Available\n> > + * The base state of a camera, an application can inspect the properties of the\n> > + * camera to determine if it wishes to use it. If an application wishes to use\n> > + * a camera it should acquire it to proceed to the acquired state.\n> > + *\n> > + * \\subsubsection Acquired\n> > + * In the acquired state an application have exclusive access to the camera and\n> > + * may modify the camera's parameters to configure it and proceed to the\n> > + * configured state.\n> > + *\n> > + * \\subsubsection Configured\n> > + * The camera is configured and ready for the application to prepare it with\n> > + * resources. The camera may be reconfigured multiple times until resources\n> > + * are provided and the state progressed to provided.\n> > + *\n> > + * \\subsubsection Prepared\n> > + * Camera have been configured and provided with resource and is ready to be\n> > + * started. The application may free the camera's resources to get back to the\n> > + * configured state or start it to progress to the running state.\n> > + *\n> > + * \\subsubsection Running\n> > + * The camera is running and ready to process requests queued by the\n> > + * application. The camera remains in this state until it's stopped and moved\n> > + * to the prepared state.\n> > */\n> > \n> > /**\n> > @@ -116,17 +188,42 @@ const std::string &Camera::name() const\n> > */\n> > \n> > Camera::Camera(PipelineHandler *pipe, const std::string &name)\n> > -\t: pipe_(pipe->shared_from_this()), name_(name), acquired_(false),\n> > -\t disconnected_(false)\n> > +\t: pipe_(pipe->shared_from_this()), name_(name), disconnected_(false),\n> > +\t state_(CameraAvailable)\n> > {\n> > }\n> > \n> > Camera::~Camera()\n> > {\n> > -\tif (acquired_)\n> > +\tif (!stateIs(CameraAvailable))\n> > \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> > }\n> > \n> > +bool Camera::stateBetween(State low, State high) const\n> > +{\n> > +\tstatic const std::string stateNames[] = {\n> > +\t\t\"Available\",\n> > +\t\t\"Acquired\",\n> > +\t\t\"Configured\",\n> > +\t\t\"Prepared\",\n> > +\t\t\"Running\",\n> > +\t};\n> \n> Should this be moved nearer to the enum so that it can be easily updated\n> if the states ever change? Perhaps not a big deal.\n\nThe enum is defined in the header file and I don't think this would fit \nwell in the header file. In v1 I had two functions checking states and \nthen this was a global lookup table which I thought was kind of ugly. So \nin v2 where I opted to only have one function actually checking state I \nmade ut a static local variable.\n\n> \n> \n> > +\n> > +\tif (state_ >= low && state_ <= high)\n> > +\t\treturn true;\n> > +\n> > +\tLOG(Camera, Debug) << \"Camera in \" << stateNames[state_]\n> > +\t\t\t << \" state trying operation requiring state between \"\n> > +\t\t\t << stateNames[low] << \" and \" << stateNames[high];\n> > +\n> > +\treturn false;\n> > +}\n> \n> I wondered if any calls would be available in discontinuous states,\n> (like Configured and Running, but not prepared) but I can't imaging any\n> such use case - so I like this 'stateBetween' concept.\n\nMe neither :-) If this changes we can modify this later easily as it \nshould only be used inside the camera object and not visible to the \napplication.\n\n> \n> > +\n> > +bool Camera::stateIs(State state) const\n> > +{\n> > +\treturn stateBetween(state, state);\n> \n> I suspect this may as well be it's own implementation.\n> \n> If the state is correct, then it has to do two comparisons instead of\n> one to validate that, and if it's incorrect - then the LOG() message\n> will print saying it requires the state between RUNNING and RUNNING or\n> such ... which also seems a bit wierd.\n> \n> *if* you choose to move stateNames out of stateBetween, a simple\n> implementation here would be clearer I think.\n\nI had this in v1 but though it was ugly, I think this is a bit of a test \nissue, I will yield to popular demand. If someone else agrees with you I \nwill turn this into its own implementation similar to v1.\n\n> \n> > +}\n> > +\n> > /**\n> > * \\brief Notify camera disconnection\n> > *\n> > @@ -140,6 +237,17 @@ void Camera::disconnect()\n> > {\n> > \tLOG(Camera, Debug) << \"Disconnecting camera \" << name_;\n> > \n> > +\t/*\n> > +\t * If the camera was running when the hardware was removed force the\n> > +\t * state to prepared to allow applications to call freeBuffers() and\n> > +\t * release() before deleting the camera.\n> > +\t *\n> > +\t * \\todo: Update comment when importing buffers as well as allocating\n> > +\t * them are supported.\n> > +\t */\n> > +\tif (state_ == CameraRunning)\n> > +\t\tstate_ = CameraPrepared;\n> > +\n> > \tdisconnected_ = true;\n> > \tdisconnected.emit(this);\n> > }\n> > @@ -158,13 +266,19 @@ void Camera::disconnect()\n> > * \\todo Implement exclusive access across processes.\n> > *\n> > * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -ENODEV The camera is no longer connected to the hardware\n> > + * \\retval -EBUSY The camera is not free and can't be acquired by the caller\n> > */\n> > int Camera::acquire()\n> > {\n> > -\tif (acquired_)\n> > +\tif (disconnected_)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\tif (!stateIs(CameraAvailable))\n> > \t\treturn -EBUSY;\n> > \n> > -\tacquired_ = true;\n> > +\tstate_ = CameraAcquired;\n> > +\n> > \treturn 0;\n> > }\n> > \n> > @@ -173,10 +287,18 @@ int Camera::acquire()\n> > *\n> > * Releasing the camera device allows other users to acquire exclusive access\n> > * with the acquire() function.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -EBUSY The camera is running and can't be released\n> \n> Perhaps we should add the state restrictions to the function API\n> documentation?\n> \n> (That comment applies to all functions in Camera of course)\n\nI don't like this very much, that is why I tried to describe all of this \nin the class documentation. The idea was to collect all state machine \ninformation in one place and document the functions on there own. I fear \nadding this information in the class documentation and in the function \ndocumentation would just be copying the same sentences around adding \nlittle value. I'm open to discuss this however.\n\n> \n> \n> > */\n> > -void Camera::release()\n> > +int Camera::release()\n> > {\n> > -\tacquired_ = false;\n> > +\tif (!stateBetween(CameraAvailable, CameraConfigured))\n> > +\t\treturn -EBUSY;\n> > +\n> > +\tstate_ = CameraAvailable;\n> > +\n> > +\treturn 0;\n> > }\n> > \n> > /**\n> > @@ -236,17 +358,19 @@ Camera::streamConfiguration(std::set &streams)\n> > * to calling this function, otherwise an -EACCES error will be returned.\n> > *\n> > * \\return 0 on success or a negative error code otherwise\n> > - * \\retval -ENODEV The camera is not connected to any hardware\n> > - * \\retval -EACCES The user has not acquired exclusive access to the camera\n> > + * \\retval -ENODEV The camera is no longer connected to the hardware\n> > + * \\retval -EACCES The camera is not in a state where it can be configured\n> > * \\retval -EINVAL The configuration is not valid\n> > */\n> > int Camera::configureStreams(std::map &config)\n> > {\n> > \tint ret;\n> > \n> > -\tret = exclusiveAccess();\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\tif (disconnected_)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\tif (!stateBetween(CameraAvailable, CameraConfigured))\n> > +\t\treturn -EACCES;\n> > \n> > \tif (!config.size()) {\n> > \t\tLOG(Camera, Error)\n> > @@ -273,20 +397,25 @@ int Camera::configureStreams(std::map &config)\n> > \t\tstream->bufferPool().createBuffers(cfg.bufferCount);\n> > \t}\n> > \n> > +\tstate_ = CameraConfigured;\n> > +\n> > \treturn 0;\n> > }\n> > \n> > /**\n> > * \\brief Allocate buffers for all configured streams\n> > * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -ENODEV The camera is no longer connected to the hardware\n> > + * \\retval -EACCES The camera is not in a state where buffers can be allocated\n> > + * \\retval -EINVAL The configuration is not valid\n> > */\n> > int Camera::allocateBuffers()\n> > {\n> > -\tint ret;\n> > +\tif (disconnected_)\n> > +\t\treturn -ENODEV;\n> > \n> > -\tret = exclusiveAccess();\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\tif (!stateIs(CameraConfigured))\n> > +\t\treturn -EACCES;\n> > \n> > \tif (activeStreams_.empty()) {\n> > \t\tLOG(Camera, Error)\n> > @@ -295,7 +424,7 @@ int Camera::allocateBuffers()\n> > \t}\n> > \n> > \tfor (Stream *stream : activeStreams_) {\n> > -\t\tret = pipe_->allocateBuffers(this, stream);\n> > +\t\tint ret = pipe_->allocateBuffers(this, stream);\n> > \t\tif (ret) {\n> > \t\t\tLOG(Camera, Error) << \"Failed to allocate buffers\";\n> > \t\t\tfreeBuffers();\n> > @@ -303,14 +432,22 @@ int Camera::allocateBuffers()\n> > \t\t}\n> > \t}\n> > \n> > +\tstate_ = CameraPrepared;\n> \n> And a function which causes a state transition (if it succeeds) should\n> also document that in it's function documentation I think.\n\nSame comment as above.\n\n> \n> \n> > +\n> > \treturn 0;\n> > }\n> > \n> > /**\n> > * \\brief Release all buffers from allocated pools in each stream\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -EACCES The camera is not in a state where buffers can be freed\n> > */\n> > -void Camera::freeBuffers()\n> > +int Camera::freeBuffers()\n> > {\n> > +\tif (!stateIs(CameraPrepared))\n> > +\t\treturn -EACCES;\n> > +\n> > \tfor (Stream *stream : activeStreams_) {\n> > \t\tif (!stream->bufferPool().count())\n> > \t\t\tcontinue;\n> > @@ -318,6 +455,10 @@ void Camera::freeBuffers()\n> > \t\tpipe_->freeBuffers(this, stream);\n> > \t\tstream->bufferPool().destroyBuffers();\n> > \t}\n> > +\n> > +\tstate_ = CameraConfigured;\n> > +\n> > +\treturn 0;\n> > }\n> > \n> > /**\n> > @@ -333,7 +474,7 @@ void Camera::freeBuffers()\n> > */\n> > Request *Camera::createRequest()\n> > {\n> > -\tif (exclusiveAccess())\n> > +\tif (disconnected_ || !stateBetween(CameraPrepared, CameraRunning))> \t\treturn nullptr;\n> > \n> > \treturn new Request(this);\n> > @@ -351,16 +492,18 @@ Request *Camera::createRequest()\n> > * automatically after it completes.\n> > *\n> > * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -ENODEV The camera is no longer connected to the hardware\n> > + * \\retval -EACCES The camera is not running so requests can't be queued\n> > */\n> > int Camera::queueRequest(Request *request)\n> > {\n> > -\tint ret;\n> > +\tif (disconnected_)\n> > +\t\treturn -ENODEV;\n> > \n> > -\tret = exclusiveAccess();\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\tif (!stateIs(CameraRunning))\n> > +\t\treturn -EACCES;\n> > \n> > -\tret = request->prepare();\n> > +\tint ret = request->prepare();\n> > \tif (ret) {\n> > \t\tLOG(Camera, Error) << \"Failed to prepare request\";\n> > \t\treturn ret;\n> > @@ -377,16 +520,26 @@ int Camera::queueRequest(Request *request)\n> > * until the capture session is terminated with \\a stop().\n> > *\n> > * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -ENODEV The camera is no longer connected to the hardware\n> > + * \\retval -EACCES The camera is not in a state where it can be started\n> > */\n> > int Camera::start()\n> > {\n> > -\tint ret = exclusiveAccess();\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\tif (disconnected_)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\tif (!stateIs(CameraPrepared))\n> > +\t\treturn -EACCES;\n> > \n> > \tLOG(Camera, Debug) << \"Starting capture\";\n> > \n> > -\treturn pipe_->start(this);\n> > +\tint ret = pipe_->start(this);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tstate_ = CameraRunning;\n> > +\n> > +\treturn 0;\n> > }\n> > \n> > /**\n> > @@ -396,27 +549,22 @@ int Camera::start()\n> > * requests are cancelled and complete synchronously in an error state.\n> > *\n> > * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -ENODEV The camera is no longer connected to the hardware\n> > + * \\retval -EACCES The camera is not running so can't be stopped\n> > */\n> > int Camera::stop()\n> > {\n> > -\tint ret = exclusiveAccess();\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > +\tif (disconnected_)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\tif (!stateIs(CameraRunning))\n> > +\t\treturn -EACCES;\n> > \n> > \tLOG(Camera, Debug) << \"Stopping capture\";\n> > \n> > \tpipe_->stop(this);\n> > \n> > -\treturn 0;\n> > -}\n> > -\n> > -int Camera::exclusiveAccess()\n> > -{\n> > -\tif (disconnected_)\n> > -\t\treturn -ENODEV;\n> > -\n> > -\tif (!acquired_)\n> > -\t\treturn -EACCES;\n> > +\tstate_ = CameraPrepared;\n> > \n> > \treturn 0;\n> > }\n> > \n> \n> -- \n> Regards\n> --\n> Kieran","headers":{"Return-Path":"","Received":["from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 665F4610B6\n\tfor ;\n\tThu, 28 Feb 2019 17:27:09 +0100 (CET)","by mail-lf1-x142.google.com with SMTP id r123so7336737lff.6\n\tfor ;\n\tThu, 28 Feb 2019 08:27:09 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tg11sm923388lfh.15.2019.02.28.08.27.07\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 28 Feb 2019 08:27:07 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=kPwH68mKw0Z48r8iFZwIxTZn7J0MpcnhHIHAjgCA48Q=;\n\tb=wsxlf7oJgQN2Ysol5eIoFRgeEEN1PugEq+VO+1RaB7BMSmFyuQde1ljI/bV9EzP4U4\n\t+SLSdDa4OWIkRsBZ3QzWijbQvpK0YXAC5uwUFXggYTI0+LQvQOj/HFEMw0wIGtZ6D/c6\n\t3fTfAkmvUTCcVuWyN8lP1+78rP0iDSihpaR8R2PLPmzstWBwXYXwM/fXhf36f9dTi/dc\n\t0l1aQIf1Ang6r6EkhBVZUBRLdSzlyUAFU60V20Dn3SFth3cdi7npONUz1MRjZ7Ycw7EJ\n\tn4FfFSrPwhk7X/8C2gw9xCqqhRxVtoDqNdCShWKkegwHQODLjEMJJxbVi2UfsngFU4Rq\n\tOu2g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=kPwH68mKw0Z48r8iFZwIxTZn7J0MpcnhHIHAjgCA48Q=;\n\tb=uUUyNj25PJWvr2RVOQd54y7dVT/JeMC1LJyYNE8vb3EcaPyG9n7P3h2XfbDEBAurJH\n\tAe7+i5vSdyJeuSma/NOOV36Y1T/Eu9rZVerIyppGRYlL075BdtikaG0vnR3pFfBtNNOP\n\t8yIZIaOXtHYiD70deWVA3RGJx0C6nBVdlfxoaL/3nXzi+qxlAw8apwvAPsOZNK9Ij9f1\n\tNM6435o318EYTwAVpBaz5zNgJTTYvgzsovcpSE9t99+OYFn0EQg90IJ+41p1+ydfJ8O7\n\tCEw1InkOGrUb7v9+rdaJhwRae5I/86INo12ER/BQ8lYIaxDvTNmfklBA8nUQIweby+bl\n\tj0ig==","X-Gm-Message-State":"APjAAAVkcH422XDqEsFgqsKrblGT2Loyss3UEMUNaJ3pP/KlIRMYy2QC\n\tTqg/bEwjXVf0YgUNIoBM7qNRVOxwqeI=","X-Google-Smtp-Source":"APXvYqwz1GykdnurpM3wFmdcPFfBboxGUUusr8qsF/Ua17OJmj4/kfJyd0yLnGIBrn2G38dFF5vtkQ==","X-Received":"by 2002:ac2:446c:: with SMTP id y12mr209434lfl.44.1551371228346; \n\tThu, 28 Feb 2019 08:27:08 -0800 (PST)","Date":"Thu, 28 Feb 2019 17:27:07 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190228162707.GB899@bigcity.dyn.berto.se>","References":"<20190228021626.15062-1-niklas.soderlund@ragnatech.se>\n\t<20190228021626.15062-4-niklas.soderlund@ragnatech.se>\n\t<74ce331d-afba-62b7-d5a9-531b6729b5cc@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<74ce331d-afba-62b7-d5a9-531b6729b5cc@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] libcamera: camera: add state\n\tmachine to control access from applications","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 28 Feb 2019 16:27:09 -0000"}},{"id":947,"web_url":"https://patchwork.libcamera.org/comment/947/","msgid":"<20190228163029.GC899@bigcity.dyn.berto.se>","date":"2019-02-28T16:30:29","subject":"Re: [libcamera-devel] [PATCH v2 3/4] libcamera: camera: add state\n\tmachine to control access from applications","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"On 2019-02-28 03:16:25 +0100, Niklas Söderlund wrote:\n\n[snip]\n\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 84b97b5c2ce94ecf..4f0833300b9b7ffc 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n\n[snip]\n\n> @@ -396,27 +549,22 @@ int Camera::start()\n> * requests are cancelled and complete synchronously in an error state.\n> *\n> * \\return 0 on success or a negative error code otherwise\n> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> + * \\retval -EACCES The camera is not running so can't be stopped\n> */\n> int Camera::stop()\n> {\n> -\tint ret = exclusiveAccess();\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tif (disconnected_)\n> +\t\treturn -ENODEV;\n> +\n> +\tif (!stateIs(CameraRunning))\n> +\t\treturn -EACCES;\n> \n> \tLOG(Camera, Debug) << \"Stopping capture\";\n> \n> \tpipe_->stop(this);\n> \n> -\treturn 0;\n> -}\n> -\n> -int Camera::exclusiveAccess()\n> -{\n> -\tif (disconnected_)\n> -\t\treturn -ENODEV;\n> -\n> -\tif (!acquired_)\n> -\t\treturn -EACCES;\n> +\tstate_ = CameraPrepared;\n\nAfter a discussion on IRC it is agreed that the order of these \noperations should be swapped for next version,\n\n state_ = CameraPrepared;\n pipe_->stop(this)\n\nInstead of how this patch changes it,\n\n pipe_->stop(this)\n state_ = CameraPrepared;\n\n> \n> \treturn 0;\n> }","headers":{"Return-Path":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B4CD610B6\n\tfor ;\n\tThu, 28 Feb 2019 17:30:31 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id g80so17690921ljg.6\n\tfor ;\n\tThu, 28 Feb 2019 08:30:31 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tf9sm4128413ljc.56.2019.02.28.08.30.29\n\tfor \n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 28 Feb 2019 08:30:29 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=zlnqKcx6eBXjrLp2Hymtg/4AvvdWFYQrWTF8MlfKAsk=;\n\tb=00WI4iiB0jkQIYUjKsk4E3yI2RSS96TZeq43wftUJ0isdIynnXntx+khwO9xl2kBgu\n\tQQdjhJ59Hq92euWefO2rLlyzg/CMLb7L1yWG57DWzbiA+rucIH8VKdzzaSesn93g2/UR\n\tGo7zRsQDXu7zK9b7/7938f7ywYNoekuixMYgOAY6bleYzCY1+z+2WO7z9b7M8v/vCjJ9\n\tHnQ76MXB/kehYbkhM/yRVqShn6dEck01vmA63I0DyUV+ta9+sDcRtV/MR3IYGvXKBxCn\n\tx0sg9JZVDbMZ/WnVqw/wDgaRiwMJvbuJoQKE6kCZw1JRF4hUzsj48Q+3gMKHpIrYiKkh\n\tdgsQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=zlnqKcx6eBXjrLp2Hymtg/4AvvdWFYQrWTF8MlfKAsk=;\n\tb=gjTo5lPsLva7xLcqfM0YebHhNKtWI/WnzRxuKLf0QqUiyidA9COCmA0YS+XKPUDX1+\n\tY3R4Htrd0/dgkH2CbUdc1c3ZhC0XssqUu6akJZY5HfAYFXy49GTXSKiVefiAFWEuC6mi\n\tS+gtKVbX7QdGni5O+u0JaDJ9PVgSh2e2sNf58F+QjtpOH55aflTDSp0LKDNyJfIJhZW/\n\tPlteDICOgwdvbq86u9ZzSCJ4/OEGR5JMNH3UlOoo90prDgXxUT8eDyd9WuuF+UaUx8Nf\n\tTXcg6ihO4c1oTozXAoJUai+mhfTw0FOtfzu0/NXgJU3Ebc4XGkjiNu29k1VB6PZ3nEq4\n\tUv0w==","X-Gm-Message-State":"APjAAAVrfLQR0vk3E1hVusPEB1qyjQi59eZ2R87o7AxPUcIQ4g4wcHcp\n\tDzfZx92E8S6ampKV0kqX+9oakPtCGQo=","X-Google-Smtp-Source":"AHgI3IavWkfvWMv27vQQQ/pX+q/DqeF7sfEXcTQPUJwt8FKNP5sDkuvqkig1ntqsl8taBhJekjftKw==","X-Received":"by 2002:a2e:7e0f:: with SMTP id\n\tz15mr5258651ljc.145.1551371430393; \n\tThu, 28 Feb 2019 08:30:30 -0800 (PST)","Date":"Thu, 28 Feb 2019 17:30:29 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190228163029.GC899@bigcity.dyn.berto.se>","References":"<20190228021626.15062-1-niklas.soderlund@ragnatech.se>\n\t<20190228021626.15062-4-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190228021626.15062-4-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] libcamera: camera: add state\n\tmachine to control access from applications","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 28 Feb 2019 16:30:31 -0000"}},{"id":952,"web_url":"https://patchwork.libcamera.org/comment/952/","msgid":"<20190228171222.GB14557@pendragon.ideasonboard.com>","date":"2019-02-28T17:12:22","subject":"Re: [libcamera-devel] [PATCH v2 3/4] libcamera: camera: add state\n\tmachine to control access from applications","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello Niklas,\n\nThank you for the patch.\n\nOn Thu, Feb 28, 2019 at 05:27:07PM +0100, Niklas Söderlund wrote:\n> On 2019-02-28 13:21:02 +0000, Kieran Bingham wrote:\n> > On 28/02/2019 02:16, Niklas Söderlund wrote:\n> >> There is a need to better control the order of operations an application\n> >> performs on a camera for it to function correctly. Add a basic state\n> >> machine to ensure applications perform operations on the camera in good\n> >> order.\n> >> \n> >> Internal to the Camera states are added; Available, Acquired,\n> >> Configured, Prepared and Running. Each state represents a higher state\n> >> of configuration of the camera ultimately leading to the highest state\n> >> where the camera is capturing frames. Each state supports a subset of\n> >> operations the application may perform.\n> >> \n> >> Signed-off-by: Niklas Söderlund \n> >> ---\n> >> include/libcamera/camera.h | 18 ++-\n> >> src/libcamera/camera.cpp | 230 ++++++++++++++++++++++++++++++-------\n> >> 2 files changed, 203 insertions(+), 45 deletions(-)\n> >> \n> >> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> >> index 9c8ae01ed5c607f1..e5212cf05d221279 100644\n> >> --- a/include/libcamera/camera.h\n> >> +++ b/include/libcamera/camera.h\n> >> @@ -39,7 +39,7 @@ public:\n> >> \tSignal disconnected;\n> >> \n> >> \tint acquire();\n> >> -\tvoid release();\n> >> +\tint release();\n\nI have a feeling applications won't check the return value of release().\nI don't think we can do much about it though.\n\n> >> \n> >> \tconst std::set &streams() const;\n> >> \tstd::map\n> >> @@ -47,7 +47,7 @@ public:\n> >> \tint configureStreams(std::map &config);\n> >> \n> >> \tint allocateBuffers();\n> >> -\tvoid freeBuffers();\n> >> +\tint freeBuffers();\n\nSame here.\n\n> >> \tRequest *createRequest();\n> >> \tint queueRequest(Request *request);\n> >> @@ -56,20 +56,30 @@ public:\n> >> \tint stop();\n> >> \n> >> private:\n> >> +\tenum State {\n> >> +\t\tCameraAvailable,\n> >> +\t\tCameraAcquired,\n> >> +\t\tCameraConfigured,\n> >> +\t\tCameraPrepared,\n> >> +\t\tCameraRunning,\n> > \n> > Do these need to be prefixed with Camera?\n> > \n> > The enum is already scoped to the Camera class so the common prefix\n> > feels a bit redundant?\n> \n> I added the prefixes in v2 after a comment from Laurent that they where \n> very generic and might conflict with applications. As they are only used \n> internally in the camera object I do not feel strongly one way or the \n> other.\n\nI might have been overcautious there, if you think we should get rid of\nthe prefixes globally, I won't complain (that is unless I find a reason\nto that would escape my mind right now :-)).\n\n> >> +\t};\n> >> +\n> >> \tCamera(PipelineHandler *pipe, const std::string &name);\n> >> \t~Camera();\n> >> \n> >> +\tbool stateBetween(State low, State high) const;\n> >> +\tbool stateIs(State state) const;\n> >> +\n> >> \tfriend class PipelineHandler;\n> >> \tvoid disconnect();\n> >> -\tint exclusiveAccess();\n> >> \n> >> \tstd::shared_ptr pipe_;\n> >> \tstd::string name_;\n> >> \tstd::set streams_;\n> >> \tstd::set activeStreams_;\n> >> \n> >> -\tbool acquired_;\n> >> \tbool disconnected_;\n> >> +\tState state_;\n> >> };\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> >> index 84b97b5c2ce94ecf..4f0833300b9b7ffc 100644\n> >> --- a/src/libcamera/camera.cpp\n> >> +++ b/src/libcamera/camera.cpp\n> >> @@ -52,6 +52,78 @@ LOG_DECLARE_CATEGORY(Camera)\n> >> * created with the create() function which returns a shared pointer. The\n> >> * Camera constructors and destructor are private, to prevent instances from\n> >> * being constructed and destroyed manually.\n> >> + *\n> >> + * \\section camera_operation Operating the camera\n\ns/camera/Camera/\n\n> >> + *\n> >> + * An application needs to preform a sequence of operations on a camera before\n\ns/preform/perform/\n\n> >> + * it is ready to process requests. The camera needs to be acquired, configured\n> >> + * and resources allocated or import to prepare it for start. Once started the\n\ns/import/imported/\ns/it for start/the camera for capture/\n\n> >> + * camera can process requests until it's stopped. When an application is done\n\ns/it's/it is/\n\n> >> + * with a camera all resources allocated to process requests needs to be freed\n\nMaybe s/to process requests //\n\n> >> + * and the camera released.\n> >> + *\n> >> + * An application may start and stop a camera multiple times as long as it's\n\ns/it's/it is/\n\n> >> + * not released. The camera may also be reconfigured provided that all\n> >> + * resources allocated are freed prior to the reconfiguration.\n> >> + *\n> >> + * \\subsection Camera States\n> >> + *\n> >> + * To help manage the sequence of operation need to control the camera a set\n\ns/operation need/operations needed/\n\n> >> + * of states are defined. Each state describes which operations may be preformed\n\ns/preformed/performed/\n\n> >> + * on the camera.\n> >> + *\n> >> + * \\dot\n> >> + * digraph camera_state_machine {\n> >> + * node [shape = doublecircle ]; Available;\n> > \n> > Out of interest, why do you double circle this state? Because it's the\n> > initial state or such?\n> \n> Yes I wanted to somehow highlight the default state. I'm open to change \n> or remove the double circle.\n\nI recall that being a standard notation, but I couldn't quickly find a\ndocument about it.\n\n> >> + * node [shape = circle ]; Acquired;\n> >> + * node [shape = circle ]; Configured;\n> >> + * node [shape = circle ]; Prepared;\n> >> + * node [shape = circle ]; Running;\n> >> + *\n> >> + * Available -> Available [label = \"release(), streams(), streamConfiguration()\"];\n> >> + * Available -> Acquired [label = \"acquire()\"];\n> >> + *\n> >> + * Acquired -> Available [label = \"release()\"];\n> >> + * Acquired -> Acquired [label = \"streams(), streamConfiguration()\"];\n> >> + * Acquired -> Configured [label = \"configureStreams()\"];\n> >> + *\n> >> + * Configured -> Available [label = \"release()\"];\n> >> + * Configured -> Configured [label = \"streams(), streamConfiguration(), configureStreams()\"];\n> >> + * Configured -> Prepared [label = \"allocateBuffers()\"];\n> >> + *\n> >> + * Prepared -> Configured [label = \"freeBuffers()\"];\n> >> + * Prepared -> Prepared [label = \"streams(), streamConfiguration(), createRequest()\"];\n> >> + * Prepared -> Running [label = \"start()\"];\n> >> + *\n> >> + * Running -> Prepared [label = \"stop()\"];\n> >> + * Running -> Running [label = \"streams(), streamConfiguration(), createRequest(), queueRequest()\"];\n> >> + * }\n> >> + * \\enddot\n\nI'm tempted to leave the streams() and streamConfiguration() functions\nout for clarity as they're allowed in all states. Maybe with a text\nexplaining that \"methods not listed in the state diagram are allowed in\nall states\" ?\n\n> >> + *\n> >> + * \\subsubsection Available\n> >> + * The base state of a camera, an application can inspect the properties of the\n> >> + * camera to determine if it wishes to use it. If an application wishes to use\n> >> + * a camera it should acquire it to proceed to the acquired state.\n\ns/acquire/acquire()/\ns/acquired/Acquired/\n\n> >> + *\n> >> + * \\subsubsection Acquired\n> >> + * In the acquired state an application have exclusive access to the camera and\n\ns/have/has/\n\n> >> + * may modify the camera's parameters to configure it and proceed to the\n> >> + * configured state.\n\ns/configured/Configured/\n\n> >> + *\n> >> + * \\subsubsection Configured\n> >> + * The camera is configured and ready for the application to prepare it with\n> >> + * resources. The camera may be reconfigured multiple times until resources\n> >> + * are provided and the state progressed to provided.\n\ns/progressed/progresses/ ?\ns/provided/Prepared/\n\n> >> + *\n> >> + * \\subsubsection Prepared\n> >> + * Camera have been configured and provided with resource and is ready to be\n\ns/Camera have/The camera has/\ns/resource/resources/\n\n> >> + * started. The application may free the camera's resources to get back to the\n> >> + * configured state or start it to progress to the running state.\n\ns/configured/Configured/\ns/running/Running/\n\n> >> + *\n> >> + * \\subsubsection Running\n> >> + * The camera is running and ready to process requests queued by the\n> >> + * application. The camera remains in this state until it's stopped and moved\n\ns/it's/it is/\n\n> >> + * to the prepared state.\n\ns/prepared/Prepared/\n\n> >> */\n> >> \n> >> /**\n> >> @@ -116,17 +188,42 @@ const std::string &Camera::name() const\n> >> */\n> >> \n> >> Camera::Camera(PipelineHandler *pipe, const std::string &name)\n> >> -\t: pipe_(pipe->shared_from_this()), name_(name), acquired_(false),\n> >> -\t disconnected_(false)\n> >> +\t: pipe_(pipe->shared_from_this()), name_(name), disconnected_(false),\n> >> +\t state_(CameraAvailable)\n> >> {\n> >> }\n> >> \n> >> Camera::~Camera()\n> >> {\n> >> -\tif (acquired_)\n> >> +\tif (!stateIs(CameraAvailable))\n> >> \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> >> }\n> >> \n> >> +bool Camera::stateBetween(State low, State high) const\n> >> +{\n> >> +\tstatic const std::string stateNames[] = {\n> >> +\t\t\"Available\",\n> >> +\t\t\"Acquired\",\n> >> +\t\t\"Configured\",\n> >> +\t\t\"Prepared\",\n> >> +\t\t\"Running\",\n> >> +\t};\n> > \n> > Should this be moved nearer to the enum so that it can be easily updated\n> > if the states ever change? Perhaps not a big deal.\n> \n> The enum is defined in the header file and I don't think this would fit \n> well in the header file. In v1 I had two functions checking states and \n> then this was a global lookup table which I thought was kind of ugly. So \n> in v2 where I opted to only have one function actually checking state I \n> made ut a static local variable.\n\nYou could make a global camera_state_name() function and store the array\nin it, just like we do in log.cpp.\n\n> >> +\n> >> +\tif (state_ >= low && state_ <= high)\n> >> +\t\treturn true;\n> >> +\n> >> +\tLOG(Camera, Debug) << \"Camera in \" << stateNames[state_]\n> >> +\t\t\t << \" state trying operation requiring state between \"\n> >> +\t\t\t << stateNames[low] << \" and \" << stateNames[high];\n\nShould the caller also pass the operation name for debug purpose ? It\nmay not be worth it.\n\n> >> +\n> >> +\treturn false;\n> >> +}\n> > \n> > I wondered if any calls would be available in discontinuous states,\n> > (like Configured and Running, but not prepared) but I can't imaging any\n> > such use case - so I like this 'stateBetween' concept.\n> \n> Me neither :-) If this changes we can modify this later easily as it \n> should only be used inside the camera object and not visible to the \n> application.\n> \n> >> +\n> >> +bool Camera::stateIs(State state) const\n> >> +{\n> >> +\treturn stateBetween(state, state);\n> > \n> > I suspect this may as well be it's own implementation.\n> > \n> > If the state is correct, then it has to do two comparisons instead of\n> > one to validate that, and if it's incorrect - then the LOG() message\n> > will print saying it requires the state between RUNNING and RUNNING or\n> > such ... which also seems a bit wierd.\n> > \n> > *if* you choose to move stateNames out of stateBetween, a simple\n> > implementation here would be clearer I think.\n> \n> I had this in v1 but though it was ugly, I think this is a bit of a test \n> issue, I will yield to popular demand. If someone else agrees with you I \n> will turn this into its own implementation similar to v1.\n\nI think I agree with Kieran :-)\n\n> >> +}\n> >> +\n> >> /**\n> >> * \\brief Notify camera disconnection\n> >> *\n> >> @@ -140,6 +237,17 @@ void Camera::disconnect()\n> >> {\n> >> \tLOG(Camera, Debug) << \"Disconnecting camera \" << name_;\n> >> \n> >> +\t/*\n> >> +\t * If the camera was running when the hardware was removed force the\n> >> +\t * state to prepared to allow applications to call freeBuffers() and\n> >> +\t * release() before deleting the camera.\n\nSeems that hot-unplug handling will require more work to complete\npending requests, right ?\n\n> >> +\t *\n> >> +\t * \\todo: Update comment when importing buffers as well as allocating\n\nAs this isn't a doxygen comment (and rightfully so), you could use TODO\ninstead of \\todo, but standardizing on \\todo can also make sense.\n\n> >> +\t * them are supported.\n> >> +\t */\n> >> +\tif (state_ == CameraRunning)\n> >> +\t\tstate_ = CameraPrepared;\n> >> +\n> >> \tdisconnected_ = true;\n> >> \tdisconnected.emit(this);\n> >> }\n> >> @@ -158,13 +266,19 @@ void Camera::disconnect()\n> >> * \\todo Implement exclusive access across processes.\n> >> *\n> >> * \\return 0 on success or a negative error code otherwise\n> >> + * \\retval -ENODEV The camera is no longer connected to the hardware\n\nThe wording sounds a bit weird. How about \"The camera has been\ndisconnected from the system\" ?\n\n> >> + * \\retval -EBUSY The camera is not free and can't be acquired by the caller\n> >> */\n> >> int Camera::acquire()\n> >> {\n> >> -\tif (acquired_)\n> >> +\tif (disconnected_)\n> >> +\t\treturn -ENODEV;\n> >> +\n> >> +\tif (!stateIs(CameraAvailable))\n> >> \t\treturn -EBUSY;\n> >> \n> >> -\tacquired_ = true;\n> >> +\tstate_ = CameraAcquired;\n> >> +\n> >> \treturn 0;\n> >> }\n> >> \n> >> @@ -173,10 +287,18 @@ int Camera::acquire()\n> >> *\n> >> * Releasing the camera device allows other users to acquire exclusive access\n> >> * with the acquire() function.\n> >> + *\n> >> + * \\return 0 on success or a negative error code otherwise\n> >> + * \\retval -EBUSY The camera is running and can't be released\n> > \n> > Perhaps we should add the state restrictions to the function API\n> > documentation?\n> > \n> > (That comment applies to all functions in Camera of course)\n> \n> I don't like this very much, that is why I tried to describe all of this \n> in the class documentation. The idea was to collect all state machine \n> information in one place and document the functions on there own. I fear \n> adding this information in the class documentation and in the function \n> documentation would just be copying the same sentences around adding \n> little value. I'm open to discuss this however.\n\nI like how the information is centralized. I wonder if we could just\nlink to the state machine though.\n\n> >> */\n> >> -void Camera::release()\n> >> +int Camera::release()\n> >> {\n> >> -\tacquired_ = false;\n> >> +\tif (!stateBetween(CameraAvailable, CameraConfigured))\n> >> +\t\treturn -EBUSY;\n> >> +\n> >> +\tstate_ = CameraAvailable;\n> >> +\n> >> +\treturn 0;\n> >> }\n> >> \n> >> /**\n> >> @@ -236,17 +358,19 @@ Camera::streamConfiguration(std::set &streams)\n> >> * to calling this function, otherwise an -EACCES error will be returned.\n> >> *\n> >> * \\return 0 on success or a negative error code otherwise\n> >> - * \\retval -ENODEV The camera is not connected to any hardware\n> >> - * \\retval -EACCES The user has not acquired exclusive access to the camera\n> >> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> >> + * \\retval -EACCES The camera is not in a state where it can be configured\n> >> * \\retval -EINVAL The configuration is not valid\n> >> */\n> >> int Camera::configureStreams(std::map &config)\n> >> {\n> >> \tint ret;\n> >> \n> >> -\tret = exclusiveAccess();\n> >> -\tif (ret)\n> >> -\t\treturn ret;\n> >> +\tif (disconnected_)\n> >> +\t\treturn -ENODEV;\n> >> +\n> >> +\tif (!stateBetween(CameraAvailable, CameraConfigured))\n> >> +\t\treturn -EACCES;\n> >> \n> >> \tif (!config.size()) {\n> >> \t\tLOG(Camera, Error)\n> >> @@ -273,20 +397,25 @@ int Camera::configureStreams(std::map &config)\n> >> \t\tstream->bufferPool().createBuffers(cfg.bufferCount);\n> >> \t}\n> >> \n> >> +\tstate_ = CameraConfigured;\n> >> +\n> >> \treturn 0;\n> >> }\n> >> \n> >> /**\n> >> * \\brief Allocate buffers for all configured streams\n> >> * \\return 0 on success or a negative error code otherwise\n> >> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> >> + * \\retval -EACCES The camera is not in a state where buffers can be allocated\n> >> + * \\retval -EINVAL The configuration is not valid\n\nShould we add -ENOMEM, as that can be returned by\npipe_->allocateBuffers() ?\n\n> >> */\n> >> int Camera::allocateBuffers()\n> >> {\n> >> -\tint ret;\n> >> +\tif (disconnected_)\n> >> +\t\treturn -ENODEV;\n> >> \n> >> -\tret = exclusiveAccess();\n> >> -\tif (ret)\n> >> -\t\treturn ret;\n> >> +\tif (!stateIs(CameraConfigured))\n> >> +\t\treturn -EACCES;\n> >> \n> >> \tif (activeStreams_.empty()) {\n> >> \t\tLOG(Camera, Error)\n> >> @@ -295,7 +424,7 @@ int Camera::allocateBuffers()\n> >> \t}\n> >> \n> >> \tfor (Stream *stream : activeStreams_) {\n> >> -\t\tret = pipe_->allocateBuffers(this, stream);\n> >> +\t\tint ret = pipe_->allocateBuffers(this, stream);\n> >> \t\tif (ret) {\n> >> \t\t\tLOG(Camera, Error) << \"Failed to allocate buffers\";\n> >> \t\t\tfreeBuffers();\n> >> @@ -303,14 +432,22 @@ int Camera::allocateBuffers()\n> >> \t\t}\n> >> \t}\n> >> \n> >> +\tstate_ = CameraPrepared;\n> > \n> > And a function which causes a state transition (if it succeeds) should\n> > also document that in it's function documentation I think.\n> \n> Same comment as above.\n> \n> >> +\n> >> \treturn 0;\n> >> }\n> >> \n> >> /**\n> >> * \\brief Release all buffers from allocated pools in each stream\n> >> + *\n> >> + * \\return 0 on success or a negative error code otherwise\n> >> + * \\retval -EACCES The camera is not in a state where buffers can be freed\n> >> */\n> >> -void Camera::freeBuffers()\n> >> +int Camera::freeBuffers()\n> >> {\n> >> +\tif (!stateIs(CameraPrepared))\n> >> +\t\treturn -EACCES;\n> >> +\n> >> \tfor (Stream *stream : activeStreams_) {\n> >> \t\tif (!stream->bufferPool().count())\n> >> \t\t\tcontinue;\n> >> @@ -318,6 +455,10 @@ void Camera::freeBuffers()\n> >> \t\tpipe_->freeBuffers(this, stream);\n> >> \t\tstream->bufferPool().destroyBuffers();\n> >> \t}\n> >> +\n> >> +\tstate_ = CameraConfigured;\n> >> +\n> >> +\treturn 0;\n> >> }\n> >> \n> >> /**\n> >> @@ -333,7 +474,7 @@ void Camera::freeBuffers()\n> >> */\n> >> Request *Camera::createRequest()\n> >> {\n> >> -\tif (exclusiveAccess())\n> >> +\tif (disconnected_ || !stateBetween(CameraPrepared, CameraRunning))\n> >> \t\treturn nullptr;\n> >> \n> >> \treturn new Request(this);\n> >> @@ -351,16 +492,18 @@ Request *Camera::createRequest()\n> >> * automatically after it completes.\n> >> *\n> >> * \\return 0 on success or a negative error code otherwise\n> >> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> >> + * \\retval -EACCES The camera is not running so requests can't be queued\n> >> */\n> >> int Camera::queueRequest(Request *request)\n> >> {\n> >> -\tint ret;\n> >> +\tif (disconnected_)\n> >> +\t\treturn -ENODEV;\n> >> \n> >> -\tret = exclusiveAccess();\n> >> -\tif (ret)\n> >> -\t\treturn ret;\n> >> +\tif (!stateIs(CameraRunning))\n> >> +\t\treturn -EACCES;\n> >> \n> >> -\tret = request->prepare();\n> >> +\tint ret = request->prepare();\n> >> \tif (ret) {\n> >> \t\tLOG(Camera, Error) << \"Failed to prepare request\";\n> >> \t\treturn ret;\n> >> @@ -377,16 +520,26 @@ int Camera::queueRequest(Request *request)\n> >> * until the capture session is terminated with \\a stop().\n> >> *\n> >> * \\return 0 on success or a negative error code otherwise\n> >> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> >> + * \\retval -EACCES The camera is not in a state where it can be started\n> >> */\n> >> int Camera::start()\n> >> {\n> >> -\tint ret = exclusiveAccess();\n> >> -\tif (ret)\n> >> -\t\treturn ret;\n> >> +\tif (disconnected_)\n> >> +\t\treturn -ENODEV;\n> >> +\n> >> +\tif (!stateIs(CameraPrepared))\n> >> +\t\treturn -EACCES;\n> >> \n> >> \tLOG(Camera, Debug) << \"Starting capture\";\n> >> \n> >> -\treturn pipe_->start(this);\n> >> +\tint ret = pipe_->start(this);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tstate_ = CameraRunning;\n> >> +\n> >> +\treturn 0;\n> >> }\n> >> \n> >> /**\n> >> @@ -396,27 +549,22 @@ int Camera::start()\n> >> * requests are cancelled and complete synchronously in an error state.\n> >> *\n> >> * \\return 0 on success or a negative error code otherwise\n> >> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> >> + * \\retval -EACCES The camera is not running so can't be stopped\n> >> */\n> >> int Camera::stop()\n> >> {\n> >> -\tint ret = exclusiveAccess();\n> >> -\tif (ret)\n> >> -\t\treturn ret;\n> >> +\tif (disconnected_)\n> >> +\t\treturn -ENODEV;\n> >> +\n> >> +\tif (!stateIs(CameraRunning))\n> >> +\t\treturn -EACCES;\n> >> \n> >> \tLOG(Camera, Debug) << \"Stopping capture\";\n> >> \n> >> \tpipe_->stop(this);\n> >> \n> >> -\treturn 0;\n> >> -}\n> >> -\n> >> -int Camera::exclusiveAccess()\n> >> -{\n> >> -\tif (disconnected_)\n> >> -\t\treturn -ENODEV;\n> >> -\n> >> -\tif (!acquired_)\n> >> -\t\treturn -EACCES;\n> >> +\tstate_ = CameraPrepared;\n> >> \n> >> \treturn 0;\n> >> }\n\nWith all the small issues fixed,\n\nReviewed-by: Laurent Pinchart ","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F07C1610B6\n\tfor ;\n\tThu, 28 Feb 2019 18:12:27 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 58E5249;\n\tThu, 28 Feb 2019 18:12:27 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551373947;\n\tbh=JF2s1HPbVdbryWIzt3DyY+nc+GZinsss1TzPQLrDFZs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=I3lDrxniZUU9SsGHlDUdCS8C+Dkgv77bkyORAHeOdX8jk3UF1ii7WZ5DZtAePvTVd\n\tONBGiXza0zGPU01MKbiLEWq+5iiRG3s5NcjwCCuiMBXdfQzSHHQKKllSQCRgnfJym6\n\t7DtCCEVuEEW29+wvknenes1UnsKjrn6k9bDH3KuA=","Date":"Thu, 28 Feb 2019 19:12:22 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190228171222.GB14557@pendragon.ideasonboard.com>","References":"<20190228021626.15062-1-niklas.soderlund@ragnatech.se>\n\t<20190228021626.15062-4-niklas.soderlund@ragnatech.se>\n\t<74ce331d-afba-62b7-d5a9-531b6729b5cc@ideasonboard.com>\n\t<20190228162707.GB899@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190228162707.GB899@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] libcamera: camera: add state\n\tmachine to control access from applications","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 28 Feb 2019 17:12:28 -0000"}},{"id":964,"web_url":"https://patchwork.libcamera.org/comment/964/","msgid":"<20190228184138.GL899@bigcity.dyn.berto.se>","date":"2019-02-28T18:41:38","subject":"Re: [libcamera-devel] [PATCH v2 3/4] libcamera: camera: add state\n\tmachine to control access from applications","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2019-02-28 19:12:22 +0200, Laurent Pinchart wrote:\n> Hello Niklas,\n> \n> Thank you for the patch.\n> \n> On Thu, Feb 28, 2019 at 05:27:07PM +0100, Niklas Söderlund wrote:\n> > On 2019-02-28 13:21:02 +0000, Kieran Bingham wrote:\n> > > On 28/02/2019 02:16, Niklas Söderlund wrote:\n> > >> There is a need to better control the order of operations an application\n> > >> performs on a camera for it to function correctly. Add a basic state\n> > >> machine to ensure applications perform operations on the camera in good\n> > >> order.\n> > >> \n> > >> Internal to the Camera states are added; Available, Acquired,\n> > >> Configured, Prepared and Running. Each state represents a higher state\n> > >> of configuration of the camera ultimately leading to the highest state\n> > >> where the camera is capturing frames. Each state supports a subset of\n> > >> operations the application may perform.\n> > >> \n> > >> Signed-off-by: Niklas Söderlund \n> > >> ---\n> > >> include/libcamera/camera.h | 18 ++-\n> > >> src/libcamera/camera.cpp | 230 ++++++++++++++++++++++++++++++-------\n> > >> 2 files changed, 203 insertions(+), 45 deletions(-)\n> > >> \n> > >> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > >> index 9c8ae01ed5c607f1..e5212cf05d221279 100644\n> > >> --- a/include/libcamera/camera.h\n> > >> +++ b/include/libcamera/camera.h\n> > >> @@ -39,7 +39,7 @@ public:\n> > >> \tSignal disconnected;\n> > >> \n> > >> \tint acquire();\n> > >> -\tvoid release();\n> > >> +\tint release();\n> \n> I have a feeling applications won't check the return value of release().\n> I don't think we can do much about it though.\n\nI agree but I feel it's nicer to keep them for applications that do wish \nto check.\n\n> \n> > >> \n> > >> \tconst std::set &streams() const;\n> > >> \tstd::map\n> > >> @@ -47,7 +47,7 @@ public:\n> > >> \tint configureStreams(std::map &config);\n> > >> \n> > >> \tint allocateBuffers();\n> > >> -\tvoid freeBuffers();\n> > >> +\tint freeBuffers();\n> \n> Same here.\n> \n> > >> \tRequest *createRequest();\n> > >> \tint queueRequest(Request *request);\n> > >> @@ -56,20 +56,30 @@ public:\n> > >> \tint stop();\n> > >> \n> > >> private:\n> > >> +\tenum State {\n> > >> +\t\tCameraAvailable,\n> > >> +\t\tCameraAcquired,\n> > >> +\t\tCameraConfigured,\n> > >> +\t\tCameraPrepared,\n> > >> +\t\tCameraRunning,\n> > > \n> > > Do these need to be prefixed with Camera?\n> > > \n> > > The enum is already scoped to the Camera class so the common prefix\n> > > feels a bit redundant?\n> > \n> > I added the prefixes in v2 after a comment from Laurent that they where \n> > very generic and might conflict with applications. As they are only used \n> > internally in the camera object I do not feel strongly one way or the \n> > other.\n> \n> I might have been overcautious there, if you think we should get rid of\n> the prefixes globally, I won't complain (that is unless I find a reason\n> to that would escape my mind right now :-)).\n\nI like prefixing them, makes it clear where they come from. If we go for \nremoving the prefixes I think it should be globally and then we can fix \nthis one at the same go. I will keep them for now.\n\n> \n> > >> +\t};\n> > >> +\n> > >> \tCamera(PipelineHandler *pipe, const std::string &name);\n> > >> \t~Camera();\n> > >> \n> > >> +\tbool stateBetween(State low, State high) const;\n> > >> +\tbool stateIs(State state) const;\n> > >> +\n> > >> \tfriend class PipelineHandler;\n> > >> \tvoid disconnect();\n> > >> -\tint exclusiveAccess();\n> > >> \n> > >> \tstd::shared_ptr pipe_;\n> > >> \tstd::string name_;\n> > >> \tstd::set streams_;\n> > >> \tstd::set activeStreams_;\n> > >> \n> > >> -\tbool acquired_;\n> > >> \tbool disconnected_;\n> > >> +\tState state_;\n> > >> };\n> > >> \n> > >> } /* namespace libcamera */\n> > >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > >> index 84b97b5c2ce94ecf..4f0833300b9b7ffc 100644\n> > >> --- a/src/libcamera/camera.cpp\n> > >> +++ b/src/libcamera/camera.cpp\n> > >> @@ -52,6 +52,78 @@ LOG_DECLARE_CATEGORY(Camera)\n> > >> * created with the create() function which returns a shared pointer. The\n> > >> * Camera constructors and destructor are private, to prevent instances from\n> > >> * being constructed and destroyed manually.\n> > >> + *\n> > >> + * \\section camera_operation Operating the camera\n> \n> s/camera/Camera/\n> \n> > >> + *\n> > >> + * An application needs to preform a sequence of operations on a camera before\n> \n> s/preform/perform/\n> \n> > >> + * it is ready to process requests. The camera needs to be acquired, configured\n> > >> + * and resources allocated or import to prepare it for start. Once started the\n> \n> s/import/imported/\n> s/it for start/the camera for capture/\n> \n> > >> + * camera can process requests until it's stopped. When an application is done\n> \n> s/it's/it is/\n> \n> > >> + * with a camera all resources allocated to process requests needs to be freed\n> \n> Maybe s/to process requests //\n> \n> > >> + * and the camera released.\n> > >> + *\n> > >> + * An application may start and stop a camera multiple times as long as it's\n> \n> s/it's/it is/\n> \n> > >> + * not released. The camera may also be reconfigured provided that all\n> > >> + * resources allocated are freed prior to the reconfiguration.\n> > >> + *\n> > >> + * \\subsection Camera States\n> > >> + *\n> > >> + * To help manage the sequence of operation need to control the camera a set\n> \n> s/operation need/operations needed/\n> \n> > >> + * of states are defined. Each state describes which operations may be preformed\n> \n> s/preformed/performed/\n> \n\nThanks for this, I really appreciate your hard work reviewing my \ndocumentation work.\n\n> > >> + * on the camera.\n> > >> + *\n> > >> + * \\dot\n> > >> + * digraph camera_state_machine {\n> > >> + * node [shape = doublecircle ]; Available;\n> > > \n> > > Out of interest, why do you double circle this state? Because it's the\n> > > initial state or such?\n> > \n> > Yes I wanted to somehow highlight the default state. I'm open to change \n> > or remove the double circle.\n> \n> I recall that being a standard notation, but I couldn't quickly find a\n> document about it.\n\nI had the same notion in my head but can't point to any reference. Will \nkeep it as double circle for now.\n\n> \n> > >> + * node [shape = circle ]; Acquired;\n> > >> + * node [shape = circle ]; Configured;\n> > >> + * node [shape = circle ]; Prepared;\n> > >> + * node [shape = circle ]; Running;\n> > >> + *\n> > >> + * Available -> Available [label = \"release(), streams(), streamConfiguration()\"];\n> > >> + * Available -> Acquired [label = \"acquire()\"];\n> > >> + *\n> > >> + * Acquired -> Available [label = \"release()\"];\n> > >> + * Acquired -> Acquired [label = \"streams(), streamConfiguration()\"];\n> > >> + * Acquired -> Configured [label = \"configureStreams()\"];\n> > >> + *\n> > >> + * Configured -> Available [label = \"release()\"];\n> > >> + * Configured -> Configured [label = \"streams(), streamConfiguration(), configureStreams()\"];\n> > >> + * Configured -> Prepared [label = \"allocateBuffers()\"];\n> > >> + *\n> > >> + * Prepared -> Configured [label = \"freeBuffers()\"];\n> > >> + * Prepared -> Prepared [label = \"streams(), streamConfiguration(), createRequest()\"];\n> > >> + * Prepared -> Running [label = \"start()\"];\n> > >> + *\n> > >> + * Running -> Prepared [label = \"stop()\"];\n> > >> + * Running -> Running [label = \"streams(), streamConfiguration(), createRequest(), queueRequest()\"];\n> > >> + * }\n> > >> + * \\enddot\n> \n> I'm tempted to leave the streams() and streamConfiguration() functions\n> out for clarity as they're allowed in all states. Maybe with a text\n> explaining that \"methods not listed in the state diagram are allowed in\n> all states\" ?\n\nI tired it and the graph looks better, will act on your suggestion.\n\n> \n> > >> + *\n> > >> + * \\subsubsection Available\n> > >> + * The base state of a camera, an application can inspect the properties of the\n> > >> + * camera to determine if it wishes to use it. If an application wishes to use\n> > >> + * a camera it should acquire it to proceed to the acquired state.\n> \n> s/acquire/acquire()/\n> s/acquired/Acquired/\n> \n> > >> + *\n> > >> + * \\subsubsection Acquired\n> > >> + * In the acquired state an application have exclusive access to the camera and\n> \n> s/have/has/\n> \n> > >> + * may modify the camera's parameters to configure it and proceed to the\n> > >> + * configured state.\n> \n> s/configured/Configured/\n> \n> > >> + *\n> > >> + * \\subsubsection Configured\n> > >> + * The camera is configured and ready for the application to prepare it with\n> > >> + * resources. The camera may be reconfigured multiple times until resources\n> > >> + * are provided and the state progressed to provided.\n> \n> s/progressed/progresses/ ?\n> s/provided/Prepared/\n> \n> > >> + *\n> > >> + * \\subsubsection Prepared\n> > >> + * Camera have been configured and provided with resource and is ready to be\n> \n> s/Camera have/The camera has/\n> s/resource/resources/\n> \n> > >> + * started. The application may free the camera's resources to get back to the\n> > >> + * configured state or start it to progress to the running state.\n> \n> s/configured/Configured/\n> s/running/Running/\n> \n> > >> + *\n> > >> + * \\subsubsection Running\n> > >> + * The camera is running and ready to process requests queued by the\n> > >> + * application. The camera remains in this state until it's stopped and moved\n> \n> s/it's/it is/\n> \n> > >> + * to the prepared state.\n> \n> s/prepared/Prepared/\n> \n> > >> */\n> > >> \n> > >> /**\n> > >> @@ -116,17 +188,42 @@ const std::string &Camera::name() const\n> > >> */\n> > >> \n> > >> Camera::Camera(PipelineHandler *pipe, const std::string &name)\n> > >> -\t: pipe_(pipe->shared_from_this()), name_(name), acquired_(false),\n> > >> -\t disconnected_(false)\n> > >> +\t: pipe_(pipe->shared_from_this()), name_(name), disconnected_(false),\n> > >> +\t state_(CameraAvailable)\n> > >> {\n> > >> }\n> > >> \n> > >> Camera::~Camera()\n> > >> {\n> > >> -\tif (acquired_)\n> > >> +\tif (!stateIs(CameraAvailable))\n> > >> \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> > >> }\n> > >> \n> > >> +bool Camera::stateBetween(State low, State high) const\n> > >> +{\n> > >> +\tstatic const std::string stateNames[] = {\n> > >> +\t\t\"Available\",\n> > >> +\t\t\"Acquired\",\n> > >> +\t\t\"Configured\",\n> > >> +\t\t\"Prepared\",\n> > >> +\t\t\"Running\",\n> > >> +\t};\n> > > \n> > > Should this be moved nearer to the enum so that it can be easily updated\n> > > if the states ever change? Perhaps not a big deal.\n> > \n> > The enum is defined in the header file and I don't think this would fit \n> > well in the header file. In v1 I had two functions checking states and \n> > then this was a global lookup table which I thought was kind of ugly. So \n> > in v2 where I opted to only have one function actually checking state I \n> > made ut a static local variable.\n> \n> You could make a global camera_state_name() function and store the array\n> in it, just like we do in log.cpp.\n\nI could but Camera::State is a private member and I would rather not \nexpose that to applicants. I opted for this:\n\nstatic const char *const camera_state_name[] = {\n ...\n}\n\nbool Camera::stateIs(State state) const\n{\n ...\n\n ASSERT(static_cast(state) < ARRAY_SIZE(camera_state_name));\n\n LOG(...) << ... << camera_state_name[state];\n\n ...\n}\n\n> \n> > >> +\n> > >> +\tif (state_ >= low && state_ <= high)\n> > >> +\t\treturn true;\n> > >> +\n> > >> +\tLOG(Camera, Debug) << \"Camera in \" << stateNames[state_]\n> > >> +\t\t\t << \" state trying operation requiring state between \"\n> > >> +\t\t\t << stateNames[low] << \" and \" << stateNames[high];\n> \n> Should the caller also pass the operation name for debug purpose ? It\n> may not be worth it.\n\nI can see how it could be useful but I don't think it would be useful \nenough to add the plumbing for it. If we find out later it is it's \ntrivial to add.\n\n> \n> > >> +\n> > >> +\treturn false;\n> > >> +}\n> > > \n> > > I wondered if any calls would be available in discontinuous states,\n> > > (like Configured and Running, but not prepared) but I can't imaging any\n> > > such use case - so I like this 'stateBetween' concept.\n> > \n> > Me neither :-) If this changes we can modify this later easily as it \n> > should only be used inside the camera object and not visible to the \n> > application.\n> > \n> > >> +\n> > >> +bool Camera::stateIs(State state) const\n> > >> +{\n> > >> +\treturn stateBetween(state, state);\n> > > \n> > > I suspect this may as well be it's own implementation.\n> > > \n> > > If the state is correct, then it has to do two comparisons instead of\n> > > one to validate that, and if it's incorrect - then the LOG() message\n> > > will print saying it requires the state between RUNNING and RUNNING or\n> > > such ... which also seems a bit wierd.\n> > > \n> > > *if* you choose to move stateNames out of stateBetween, a simple\n> > > implementation here would be clearer I think.\n> > \n> > I had this in v1 but though it was ugly, I think this is a bit of a test \n> > issue, I will yield to popular demand. If someone else agrees with you I \n> > will turn this into its own implementation similar to v1.\n> \n> I think I agree with Kieran :-)\n\nThe people have spoken, I shall make it so.\n\n> \n> > >> +}\n> > >> +\n> > >> /**\n> > >> * \\brief Notify camera disconnection\n> > >> *\n> > >> @@ -140,6 +237,17 @@ void Camera::disconnect()\n> > >> {\n> > >> \tLOG(Camera, Debug) << \"Disconnecting camera \" << name_;\n> > >> \n> > >> +\t/*\n> > >> +\t * If the camera was running when the hardware was removed force the\n> > >> +\t * state to prepared to allow applications to call freeBuffers() and\n> > >> +\t * release() before deleting the camera.\n> \n> Seems that hot-unplug handling will require more work to complete\n> pending requests, right ?\n\nYou are correct. I will add a todo so we won't forget.\n\n> \n> > >> +\t *\n> > >> +\t * \\todo: Update comment when importing buffers as well as allocating\n> \n> As this isn't a doxygen comment (and rightfully so), you could use TODO\n> instead of \\todo, but standardizing on \\todo can also make sense.\n\nI like the \\todo as it shows up in the Documentation displaying our \nshame and forcing us to deal with the issues :-) You are however correct \nthat this one will not show up in doxygen so I will move it so it do.\n\n> \n> > >> +\t * them are supported.\n> > >> +\t */\n> > >> +\tif (state_ == CameraRunning)\n> > >> +\t\tstate_ = CameraPrepared;\n> > >> +\n> > >> \tdisconnected_ = true;\n> > >> \tdisconnected.emit(this);\n> > >> }\n> > >> @@ -158,13 +266,19 @@ void Camera::disconnect()\n> > >> * \\todo Implement exclusive access across processes.\n> > >> *\n> > >> * \\return 0 on success or a negative error code otherwise\n> > >> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> \n> The wording sounds a bit weird. How about \"The camera has been\n> disconnected from the system\" ?\n> \n> > >> + * \\retval -EBUSY The camera is not free and can't be acquired by the caller\n> > >> */\n> > >> int Camera::acquire()\n> > >> {\n> > >> -\tif (acquired_)\n> > >> +\tif (disconnected_)\n> > >> +\t\treturn -ENODEV;\n> > >> +\n> > >> +\tif (!stateIs(CameraAvailable))\n> > >> \t\treturn -EBUSY;\n> > >> \n> > >> -\tacquired_ = true;\n> > >> +\tstate_ = CameraAcquired;\n> > >> +\n> > >> \treturn 0;\n> > >> }\n> > >> \n> > >> @@ -173,10 +287,18 @@ int Camera::acquire()\n> > >> *\n> > >> * Releasing the camera device allows other users to acquire exclusive access\n> > >> * with the acquire() function.\n> > >> + *\n> > >> + * \\return 0 on success or a negative error code otherwise\n> > >> + * \\retval -EBUSY The camera is running and can't be released\n> > > \n> > > Perhaps we should add the state restrictions to the function API\n> > > documentation?\n> > > \n> > > (That comment applies to all functions in Camera of course)\n> > \n> > I don't like this very much, that is why I tried to describe all of this \n> > in the class documentation. The idea was to collect all state machine \n> > information in one place and document the functions on there own. I fear \n> > adding this information in the class documentation and in the function \n> > documentation would just be copying the same sentences around adding \n> > little value. I'm open to discuss this however.\n> \n> I like how the information is centralized. I wonder if we could just\n> link to the state machine though.\n\nI will add the following to all functions capable of changing the \ncameras state,\n\n This function effects the state of the camera, see \\ref \n camera_operation.\n\n> \n> > >> */\n> > >> -void Camera::release()\n> > >> +int Camera::release()\n> > >> {\n> > >> -\tacquired_ = false;\n> > >> +\tif (!stateBetween(CameraAvailable, CameraConfigured))\n> > >> +\t\treturn -EBUSY;\n> > >> +\n> > >> +\tstate_ = CameraAvailable;\n> > >> +\n> > >> +\treturn 0;\n> > >> }\n> > >> \n> > >> /**\n> > >> @@ -236,17 +358,19 @@ Camera::streamConfiguration(std::set &streams)\n> > >> * to calling this function, otherwise an -EACCES error will be returned.\n> > >> *\n> > >> * \\return 0 on success or a negative error code otherwise\n> > >> - * \\retval -ENODEV The camera is not connected to any hardware\n> > >> - * \\retval -EACCES The user has not acquired exclusive access to the camera\n> > >> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> > >> + * \\retval -EACCES The camera is not in a state where it can be configured\n> > >> * \\retval -EINVAL The configuration is not valid\n> > >> */\n> > >> int Camera::configureStreams(std::map &config)\n> > >> {\n> > >> \tint ret;\n> > >> \n> > >> -\tret = exclusiveAccess();\n> > >> -\tif (ret)\n> > >> -\t\treturn ret;\n> > >> +\tif (disconnected_)\n> > >> +\t\treturn -ENODEV;\n> > >> +\n> > >> +\tif (!stateBetween(CameraAvailable, CameraConfigured))\n> > >> +\t\treturn -EACCES;\n> > >> \n> > >> \tif (!config.size()) {\n> > >> \t\tLOG(Camera, Error)\n> > >> @@ -273,20 +397,25 @@ int Camera::configureStreams(std::map &config)\n> > >> \t\tstream->bufferPool().createBuffers(cfg.bufferCount);\n> > >> \t}\n> > >> \n> > >> +\tstate_ = CameraConfigured;\n> > >> +\n> > >> \treturn 0;\n> > >> }\n> > >> \n> > >> /**\n> > >> * \\brief Allocate buffers for all configured streams\n> > >> * \\return 0 on success or a negative error code otherwise\n> > >> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> > >> + * \\retval -EACCES The camera is not in a state where buffers can be allocated\n> > >> + * \\retval -EINVAL The configuration is not valid\n> \n> Should we add -ENOMEM, as that can be returned by\n> pipe_->allocateBuffers() ?\n\nI contemplated this as well but opted not do do so now and instead aim \nto add it to our task list to look over this for all methods of the \nCamera object.\n\n> \n> > >> */\n> > >> int Camera::allocateBuffers()\n> > >> {\n> > >> -\tint ret;\n> > >> +\tif (disconnected_)\n> > >> +\t\treturn -ENODEV;\n> > >> \n> > >> -\tret = exclusiveAccess();\n> > >> -\tif (ret)\n> > >> -\t\treturn ret;\n> > >> +\tif (!stateIs(CameraConfigured))\n> > >> +\t\treturn -EACCES;\n> > >> \n> > >> \tif (activeStreams_.empty()) {\n> > >> \t\tLOG(Camera, Error)\n> > >> @@ -295,7 +424,7 @@ int Camera::allocateBuffers()\n> > >> \t}\n> > >> \n> > >> \tfor (Stream *stream : activeStreams_) {\n> > >> -\t\tret = pipe_->allocateBuffers(this, stream);\n> > >> +\t\tint ret = pipe_->allocateBuffers(this, stream);\n> > >> \t\tif (ret) {\n> > >> \t\t\tLOG(Camera, Error) << \"Failed to allocate buffers\";\n> > >> \t\t\tfreeBuffers();\n> > >> @@ -303,14 +432,22 @@ int Camera::allocateBuffers()\n> > >> \t\t}\n> > >> \t}\n> > >> \n> > >> +\tstate_ = CameraPrepared;\n> > > \n> > > And a function which causes a state transition (if it succeeds) should\n> > > also document that in it's function documentation I think.\n> > \n> > Same comment as above.\n> > \n> > >> +\n> > >> \treturn 0;\n> > >> }\n> > >> \n> > >> /**\n> > >> * \\brief Release all buffers from allocated pools in each stream\n> > >> + *\n> > >> + * \\return 0 on success or a negative error code otherwise\n> > >> + * \\retval -EACCES The camera is not in a state where buffers can be freed\n> > >> */\n> > >> -void Camera::freeBuffers()\n> > >> +int Camera::freeBuffers()\n> > >> {\n> > >> +\tif (!stateIs(CameraPrepared))\n> > >> +\t\treturn -EACCES;\n> > >> +\n> > >> \tfor (Stream *stream : activeStreams_) {\n> > >> \t\tif (!stream->bufferPool().count())\n> > >> \t\t\tcontinue;\n> > >> @@ -318,6 +455,10 @@ void Camera::freeBuffers()\n> > >> \t\tpipe_->freeBuffers(this, stream);\n> > >> \t\tstream->bufferPool().destroyBuffers();\n> > >> \t}\n> > >> +\n> > >> +\tstate_ = CameraConfigured;\n> > >> +\n> > >> +\treturn 0;\n> > >> }\n> > >> \n> > >> /**\n> > >> @@ -333,7 +474,7 @@ void Camera::freeBuffers()\n> > >> */\n> > >> Request *Camera::createRequest()\n> > >> {\n> > >> -\tif (exclusiveAccess())\n> > >> +\tif (disconnected_ || !stateBetween(CameraPrepared, CameraRunning))\n> > >> \t\treturn nullptr;\n> > >> \n> > >> \treturn new Request(this);\n> > >> @@ -351,16 +492,18 @@ Request *Camera::createRequest()\n> > >> * automatically after it completes.\n> > >> *\n> > >> * \\return 0 on success or a negative error code otherwise\n> > >> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> > >> + * \\retval -EACCES The camera is not running so requests can't be queued\n> > >> */\n> > >> int Camera::queueRequest(Request *request)\n> > >> {\n> > >> -\tint ret;\n> > >> +\tif (disconnected_)\n> > >> +\t\treturn -ENODEV;\n> > >> \n> > >> -\tret = exclusiveAccess();\n> > >> -\tif (ret)\n> > >> -\t\treturn ret;\n> > >> +\tif (!stateIs(CameraRunning))\n> > >> +\t\treturn -EACCES;\n> > >> \n> > >> -\tret = request->prepare();\n> > >> +\tint ret = request->prepare();\n> > >> \tif (ret) {\n> > >> \t\tLOG(Camera, Error) << \"Failed to prepare request\";\n> > >> \t\treturn ret;\n> > >> @@ -377,16 +520,26 @@ int Camera::queueRequest(Request *request)\n> > >> * until the capture session is terminated with \\a stop().\n> > >> *\n> > >> * \\return 0 on success or a negative error code otherwise\n> > >> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> > >> + * \\retval -EACCES The camera is not in a state where it can be started\n> > >> */\n> > >> int Camera::start()\n> > >> {\n> > >> -\tint ret = exclusiveAccess();\n> > >> -\tif (ret)\n> > >> -\t\treturn ret;\n> > >> +\tif (disconnected_)\n> > >> +\t\treturn -ENODEV;\n> > >> +\n> > >> +\tif (!stateIs(CameraPrepared))\n> > >> +\t\treturn -EACCES;\n> > >> \n> > >> \tLOG(Camera, Debug) << \"Starting capture\";\n> > >> \n> > >> -\treturn pipe_->start(this);\n> > >> +\tint ret = pipe_->start(this);\n> > >> +\tif (ret)\n> > >> +\t\treturn ret;\n> > >> +\n> > >> +\tstate_ = CameraRunning;\n> > >> +\n> > >> +\treturn 0;\n> > >> }\n> > >> \n> > >> /**\n> > >> @@ -396,27 +549,22 @@ int Camera::start()\n> > >> * requests are cancelled and complete synchronously in an error state.\n> > >> *\n> > >> * \\return 0 on success or a negative error code otherwise\n> > >> + * \\retval -ENODEV The camera is no longer connected to the hardware\n> > >> + * \\retval -EACCES The camera is not running so can't be stopped\n> > >> */\n> > >> int Camera::stop()\n> > >> {\n> > >> -\tint ret = exclusiveAccess();\n> > >> -\tif (ret)\n> > >> -\t\treturn ret;\n> > >> +\tif (disconnected_)\n> > >> +\t\treturn -ENODEV;\n> > >> +\n> > >> +\tif (!stateIs(CameraRunning))\n> > >> +\t\treturn -EACCES;\n> > >> \n> > >> \tLOG(Camera, Debug) << \"Stopping capture\";\n> > >> \n> > >> \tpipe_->stop(this);\n> > >> \n> > >> -\treturn 0;\n> > >> -}\n> > >> -\n> > >> -int Camera::exclusiveAccess()\n> > >> -{\n> > >> -\tif (disconnected_)\n> > >> -\t\treturn -ENODEV;\n> > >> -\n> > >> -\tif (!acquired_)\n> > >> -\t\treturn -EACCES;\n> > >> +\tstate_ = CameraPrepared;\n> > >> \n> > >> \treturn 0;\n> > >> }\n> \n> With all the small issues fixed,\n> \n> Reviewed-by: Laurent Pinchart \n\nThanks, so much changed due to your review so I don't feel right \ncollecting your tag. I'm thinking of how the state name lookup was \nsolved.\n\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AEAEC610B6\n\tfor ;\n\tThu, 28 Feb 2019 19:41:40 +0100 (CET)","by mail-lf1-x143.google.com with SMTP id a8so10247161lfi.7\n\tfor ;\n\tThu, 28 Feb 2019 10:41:40 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\ts129sm4195752lja.49.2019.02.28.10.41.38\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 28 Feb 2019 10:41:38 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=G5oUOOh5no5upJ/ijZmUb/1vMPaaS28f7DHgMCtzr2Y=;\n\tb=rYXmxxvOOewUhS84EPvRZnoW/wabF7w+hNJIf/TMP73VXB/NxbcgxpwuD5vVBOWO3b\n\tGGBCe3GjmKCAByDGEMfYeClFOTQRpZ5Pmk4OmthlzAy8o/79/BkZGFX+ZieXLTTxsHOW\n\tjG6EF2sEWLbhwUfCeaqzGPF21QLLKy8iPLi6xG7qWFoabwMI59NqK5K6LmsRm9ALODX3\n\twSuT8Thzmq1uYli4KvcLVG4/Ri7P3/FnJKTiwhaOa/DTrBtDahDYe31EectSGv1VrLpP\n\tNNv0fl7E0DXURFIKf1J3rZ9CdfPcMiDRkgvXok7Fz1g2zcD1yA4MvTA7x4rNR1NsRODA\n\tbx1g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=G5oUOOh5no5upJ/ijZmUb/1vMPaaS28f7DHgMCtzr2Y=;\n\tb=JrmXYronaAqDeZx9Y+escuGfEGCTROIa1MDyyKATQFl5HYbnJBTOJ8N7z4ohWC5gTc\n\thL8jfUId2e5UgEDYAFyeXz76WdXwjmhUUmjV6tx66XatSKEjjtE232bW2IdD/ZKAx/A7\n\tVL7S7vsQIWT7xEGWm1AeKyDomGIoKmu4EIpUMfERz/IN79faQ3hqoJbMIE2BufcppZS7\n\tzqspL61FVVWpsOLGvSQro7D13WM2xJyPvB2Pzp/nFh6GHI7maEyHP90XmJxfFcVkSjNk\n\t/SgGcRLCzURnMgIlBz4Z7XvxfbRCww2gbmkOpwJxJUMJGxkv9JPcgK+LIA4WsNdhS4K+\n\trfbA==","X-Gm-Message-State":"APjAAAWTO5Wh/GauYxbt3hS4A1B4DAkqClQBg7M+Q+Jz0iCSMhqR35pr\n\tx+OuGtacF249kAtF7LKMFUA2bhZ/6Dc=","X-Google-Smtp-Source":"APXvYqzvQfJM2LZtoipWZcrQQu2c4s9cZ8SqfS7CFEe1XnCNMyAyATa02AEkQqy8J7fj3UVeQjBYNg==","X-Received":"by 2002:a19:c1d4:: with SMTP id r203mr536293lff.60.1551379299809;\n\tThu, 28 Feb 2019 10:41:39 -0800 (PST)","Date":"Thu, 28 Feb 2019 19:41:38 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190228184138.GL899@bigcity.dyn.berto.se>","References":"<20190228021626.15062-1-niklas.soderlund@ragnatech.se>\n\t<20190228021626.15062-4-niklas.soderlund@ragnatech.se>\n\t<74ce331d-afba-62b7-d5a9-531b6729b5cc@ideasonboard.com>\n\t<20190228162707.GB899@bigcity.dyn.berto.se>\n\t<20190228171222.GB14557@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190228171222.GB14557@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] libcamera: camera: add state\n\tmachine to control access from applications","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 28 Feb 2019 18:41:41 -0000"}}]