[{"id":889,"web_url":"https://patchwork.libcamera.org/comment/889/","msgid":"<20190226165609.2ulu64fzegy7scut@uno.localdomain>","date":"2019-02-26T16:56:09","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: camera: document\n\treturn values for application facing functions","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Tue, Feb 26, 2019 at 03:18:52AM +0100, Niklas Söderlund wrote:\n> In preparation for adding a state machine to control access to the\n> functions facing the application document the error codes which will be\n> returned once it's in place.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/camera.cpp | 12 +++++++++---\n> 1 file changed, 9 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index e2372fea31e2921a..d4258fe3c7551af3 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -158,6 +158,7 @@ void Camera::disconnect()\n> * \\todo Implement exclusive access across processes.\n> *\n> * \\return 0 on success or a negative error code\n> + * \\retval -EBUSY The camera is not free and can't be acquired by the caller.\n> */\n> int Camera::acquire()\n> {\n> @@ -236,9 +237,8 @@ Camera::streamConfiguration(std::vector &streams)\n> * to calling this function, otherwise an -EACCES error will be returned.\n> *\n> * \\return 0 on success or a negative error code\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 -EINVAL The configuration is not valid\n> + * \\retval -EACCES The camera is not in a state where it can be configured.\n\nI'm not sure I like \"the camera is not in a state where\", here and in other\nplaces, but I got nothing better to offer, so feel free to keep it there :)\n\n\n> + * \\retval -EINVAL The configuration is not valid.\n> */\n> int Camera::configureStreams(std::map &config)\n> {\n> @@ -279,6 +279,8 @@ int Camera::configureStreams(std::map &config)\n> /**\n> * \\brief Allocate buffers for all configured streams\n> * \\return 0 on success or a negative error code\n> + * \\retval -EACCES The camera is not in a state where it can allocate buffers.\n> + * \\retval -EINVAL The configuration is not valid.\n> */\n> int Camera::allocateBuffers()\n> {\n> @@ -330,6 +332,7 @@ void Camera::freeBuffers()\n> * responsible for either queueing the request or deleting it.\n> *\n> * \\return A pointer to the newly created request, or nullptr on error\n> + * \\retval -EACCES The camera is not in a state where requests can be created.\n\nThis returns nullptr, am I wrong?\n\n> */\n> Request *Camera::createRequest()\n> {\n> @@ -351,6 +354,7 @@ Request *Camera::createRequest()\n> * automatically after it completes.\n> *\n> * \\return 0 on success or a negative error code\n> + * \\retval -EACCES The camera is not running so requests can't be queued.\n\nI think this returns EBUSY, right?\n\nWell, I assume this patch should come after you have changed the code,\nwhich I assume happens later in the series, otherwise it is difficult\nto make sure the comments are correct.\n\n\n> */\n> int Camera::queueRequest(Request *request)\n> {\n> @@ -377,6 +381,7 @@ 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\n> + * \\retval -EACCES The camera is not in a state where it can be started.\n> */\n> int Camera::start()\n> {\n> @@ -396,6 +401,7 @@ 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\n> + * \\retval -EACCES The camera is not running so can't be stopped.\n> */\n> int Camera::stop()\n> {\n> --\n> 2.20.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CEBE3610B2\n\tfor ;\n\tTue, 26 Feb 2019 17:55:40 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 46B6220006;\n\tTue, 26 Feb 2019 16:55:40 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 26 Feb 2019 17:56:09 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190226165609.2ulu64fzegy7scut@uno.localdomain>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-4-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"2grted3aqsn2ctgl\"","Content-Disposition":"inline","In-Reply-To":"<20190226021857.28255-4-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: camera: document\n\treturn values for application facing functions","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":"Tue, 26 Feb 2019 16:55:41 -0000"}},{"id":894,"web_url":"https://patchwork.libcamera.org/comment/894/","msgid":"<20190226185350.GK899@bigcity.dyn.berto.se>","date":"2019-02-26T18:53:50","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: camera: document\n\treturn values for application facing functions","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your feedback.\n\nOn 2019-02-26 17:56:09 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Tue, Feb 26, 2019 at 03:18:52AM +0100, Niklas Söderlund wrote:\n> > In preparation for adding a state machine to control access to the\n> > functions facing the application document the error codes which will be\n> > returned once it's in place.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/camera.cpp | 12 +++++++++---\n> > 1 file changed, 9 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index e2372fea31e2921a..d4258fe3c7551af3 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -158,6 +158,7 @@ void Camera::disconnect()\n> > * \\todo Implement exclusive access across processes.\n> > *\n> > * \\return 0 on success or a negative error code\n> > + * \\retval -EBUSY The camera is not free and can't be acquired by the caller.\n> > */\n> > int Camera::acquire()\n> > {\n> > @@ -236,9 +237,8 @@ Camera::streamConfiguration(std::vector &streams)\n> > * to calling this function, otherwise an -EACCES error will be returned.\n> > *\n> > * \\return 0 on success or a negative error code\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 -EINVAL The configuration is not valid\n> > + * \\retval -EACCES The camera is not in a state where it can be configured.\n> \n> I'm not sure I like \"the camera is not in a state where\", here and in other\n> places, but I got nothing better to offer, so feel free to keep it there :)\n> \n> \n> > + * \\retval -EINVAL The configuration is not valid.\n> > */\n> > int Camera::configureStreams(std::map &config)\n> > {\n> > @@ -279,6 +279,8 @@ int Camera::configureStreams(std::map &config)\n> > /**\n> > * \\brief Allocate buffers for all configured streams\n> > * \\return 0 on success or a negative error code\n> > + * \\retval -EACCES The camera is not in a state where it can allocate buffers.\n> > + * \\retval -EINVAL The configuration is not valid.\n> > */\n> > int Camera::allocateBuffers()\n> > {\n> > @@ -330,6 +332,7 @@ void Camera::freeBuffers()\n> > * responsible for either queueing the request or deleting it.\n> > *\n> > * \\return A pointer to the newly created request, or nullptr on error\n> > + * \\retval -EACCES The camera is not in a state where requests can be created.\n> \n> This returns nullptr, am I wrong?\n\nCorrect, my bad will drop this change for v2.\n\n> \n> > */\n> > Request *Camera::createRequest()\n> > {\n> > @@ -351,6 +354,7 @@ Request *Camera::createRequest()\n> > * automatically after it completes.\n> > *\n> > * \\return 0 on success or a negative error code\n> > + * \\retval -EACCES The camera is not running so requests can't be queued.\n> \n> I think this returns EBUSY, right?\n> \n> Well, I assume this patch should come after you have changed the code,\n> which I assume happens later in the series, otherwise it is difficult\n> to make sure the comments are correct.\n\nAs the commit message says this updates the documentation only for the \nnew return codes which are introduced in 5/8. I contemplated squashing \nthe two but opted to keep the separate.\n\nAs the cam tool needs to be updated (patch 4/5) before the state machine \nis added I judged it to be easier to review this in these three parts; \ndocumentation, cam tool, state machine. As it the cam tool change would \nthen make sens. If this is confusing I can squash this in with 5/8. Let \nme know what you think.\n\n> \n> \n> > */\n> > int Camera::queueRequest(Request *request)\n> > {\n> > @@ -377,6 +381,7 @@ 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\n> > + * \\retval -EACCES The camera is not in a state where it can be started.\n> > */\n> > int Camera::start()\n> > {\n> > @@ -396,6 +401,7 @@ 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\n> > + * \\retval -EACCES The camera is not running so can't be stopped.\n> > */\n> > int Camera::stop()\n> > {\n> > --\n> > 2.20.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C450F610B2\n\tfor ;\n\tTue, 26 Feb 2019 19:53:52 +0100 (CET)","by mail-lj1-x241.google.com with SMTP id z25so11723359ljk.8\n\tfor ;\n\tTue, 26 Feb 2019 10:53:52 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tp22sm3052175ljb.93.2019.02.26.10.53.51\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tTue, 26 Feb 2019 10:53:51 -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=3XPGgwWPDGE8KVafDJNwVDgqOzA9SXt1r+E9lW++txg=;\n\tb=VBITmHh6jpSoN/JephRclMlcdkxKxkk2CbihwPLwzvXyJ3duieJwG+1DzfHFXBwE4H\n\tv847rWcsebYdrkUd7d/cicLTJRB3Vkd3H/oi9j7pOLh/4RqZ80MtYSzN+lAmbTWtcvmh\n\tJK0jeulf/jgaql6SkvhB85xrW19r+5Lsryyiu9tQDzaZEiUXOMjoslXwWX0VS5F24u0Z\n\tu53ewJkY9WFgEcjRWC8MPyn87aZl9cPmLufEozjKLR+4rkm1Opa70TFhssfrs9zJ0b3n\n\t8eUtqUd2HJWkt3Ixxaj8HOEEsQpk+ppugJx0fIc/5SCWdIuyyuLEGpHgmtChclfjFl2Z\n\tul4A==","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=3XPGgwWPDGE8KVafDJNwVDgqOzA9SXt1r+E9lW++txg=;\n\tb=KaU3LAgGVGiIpIpA0IBXOzVQF6t8sVi1KS3q3sBNKx8fZ03wpXxJsBrsAxmAEOdWuI\n\tYrMuehOYYH6adp/lQUKU0MNXtfyqZLbSvdqRK8pjkMIg2uUsZwk+3vInfZX7HdfxFPak\n\tRpgUxe5+o9jpBr3OnGGGszARuM0DxNuBwJp+HAVLhldUg4Q4tHtjGdWAgrVK/9D1+8sG\n\t+IWz9JVLzDfSFu1ARShjuPLJDB6Pm4A3nzURmkJGRgVgyCzbY+kU7JMCoWZjyqCJOg6k\n\tc6Nv2V59aQ8lmUfTVKXlRsN3Nlm56gzH7S0uj7+jhVYywvQsHZL14KgMRqXi27TJPYqi\n\tsJBw==","X-Gm-Message-State":"AHQUAubINtN6tkfjgOwjASYC01oM4e0fd6QpRo01pD5dPeLWsBGrPcoQ\n\t6iDSR5TKt/NaNL1dtFWEwMHMgg==","X-Google-Smtp-Source":"AHgI3IZRqS0zvh+WBrccsaT6ypYu4zl+sMCyvjXR00DGouLAeIm62XqQNSeANW3Ur5+RZf+P8ICcXw==","X-Received":"by 2002:a2e:20cf:: with SMTP id\n\tg76mr14116238lji.36.1551207231782; \n\tTue, 26 Feb 2019 10:53:51 -0800 (PST)","Date":"Tue, 26 Feb 2019 19:53:50 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190226185350.GK899@bigcity.dyn.berto.se>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-4-niklas.soderlund@ragnatech.se>\n\t<20190226165609.2ulu64fzegy7scut@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190226165609.2ulu64fzegy7scut@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: camera: document\n\treturn values for application facing functions","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":"Tue, 26 Feb 2019 18:53:53 -0000"}},{"id":913,"web_url":"https://patchwork.libcamera.org/comment/913/","msgid":"<20190227094829.wn4jdifscie2t6jj@uno.localdomain>","date":"2019-02-27T09:48:29","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: camera: document\n\treturn values for application facing functions","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Tue, Feb 26, 2019 at 07:53:50PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your feedback.\n>\n> On 2019-02-26 17:56:09 +0100, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Tue, Feb 26, 2019 at 03:18:52AM +0100, Niklas Söderlund wrote:\n> > > In preparation for adding a state machine to control access to the\n> > > functions facing the application document the error codes which will be\n> > > returned once it's in place.\n> > >\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/camera.cpp | 12 +++++++++---\n> > > 1 file changed, 9 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index e2372fea31e2921a..d4258fe3c7551af3 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -158,6 +158,7 @@ void Camera::disconnect()\n> > > * \\todo Implement exclusive access across processes.\n> > > *\n> > > * \\return 0 on success or a negative error code\n> > > + * \\retval -EBUSY The camera is not free and can't be acquired by the caller.\n> > > */\n> > > int Camera::acquire()\n> > > {\n> > > @@ -236,9 +237,8 @@ Camera::streamConfiguration(std::vector &streams)\n> > > * to calling this function, otherwise an -EACCES error will be returned.\n> > > *\n> > > * \\return 0 on success or a negative error code\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 -EINVAL The configuration is not valid\n> > > + * \\retval -EACCES The camera is not in a state where it can be configured.\n> >\n> > I'm not sure I like \"the camera is not in a state where\", here and in other\n> > places, but I got nothing better to offer, so feel free to keep it there :)\n> >\n> >\n> > > + * \\retval -EINVAL The configuration is not valid.\n> > > */\n> > > int Camera::configureStreams(std::map &config)\n> > > {\n> > > @@ -279,6 +279,8 @@ int Camera::configureStreams(std::map &config)\n> > > /**\n> > > * \\brief Allocate buffers for all configured streams\n> > > * \\return 0 on success or a negative error code\n> > > + * \\retval -EACCES The camera is not in a state where it can allocate buffers.\n> > > + * \\retval -EINVAL The configuration is not valid.\n> > > */\n> > > int Camera::allocateBuffers()\n> > > {\n> > > @@ -330,6 +332,7 @@ void Camera::freeBuffers()\n> > > * responsible for either queueing the request or deleting it.\n> > > *\n> > > * \\return A pointer to the newly created request, or nullptr on error\n> > > + * \\retval -EACCES The camera is not in a state where requests can be created.\n> >\n> > This returns nullptr, am I wrong?\n>\n> Correct, my bad will drop this change for v2.\n>\n> >\n> > > */\n> > > Request *Camera::createRequest()\n> > > {\n> > > @@ -351,6 +354,7 @@ Request *Camera::createRequest()\n> > > * automatically after it completes.\n> > > *\n> > > * \\return 0 on success or a negative error code\n> > > + * \\retval -EACCES The camera is not running so requests can't be queued.\n> >\n> > I think this returns EBUSY, right?\n> >\n> > Well, I assume this patch should come after you have changed the code,\n> > which I assume happens later in the series, otherwise it is difficult\n> > to make sure the comments are correct.\n>\n> As the commit message says this updates the documentation only for the\n> new return codes which are introduced in 5/8. I contemplated squashing\n> the two but opted to keep the separate.\n>\n> As the cam tool needs to be updated (patch 4/5) before the state machine\n> is added I judged it to be easier to review this in these three parts;\n> documentation, cam tool, state machine. As it the cam tool change would\n> then make sens. If this is confusing I can squash this in with 5/8. Let\n> me know what you think.\n>\n\nNo need to squash if you don't want to, I was just pointing out that\nto make sure documentation is updated properly, I had to look at this\nafter 5/8 is applied. I would have just re-sorted them, but that's\nfine.\n\n> >\n> >\n> > > */\n> > > int Camera::queueRequest(Request *request)\n> > > {\n> > > @@ -377,6 +381,7 @@ 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\n> > > + * \\retval -EACCES The camera is not in a state where it can be started.\n> > > */\n> > > int Camera::start()\n> > > {\n> > > @@ -396,6 +401,7 @@ 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\n> > > + * \\retval -EACCES The camera is not running so can't be stopped.\n> > > */\n> > > int Camera::stop()\n> > > {\n> > > --\n> > > 2.20.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n>\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0679D601E2\n\tfor ;\n\tWed, 27 Feb 2019 10:48:01 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 792CE240006;\n\tWed, 27 Feb 2019 09:48:00 +0000 (UTC)"],"Date":"Wed, 27 Feb 2019 10:48:29 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190227094829.wn4jdifscie2t6jj@uno.localdomain>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-4-niklas.soderlund@ragnatech.se>\n\t<20190226165609.2ulu64fzegy7scut@uno.localdomain>\n\t<20190226185350.GK899@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"zynqyz4jxnperinu\"","Content-Disposition":"inline","In-Reply-To":"<20190226185350.GK899@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: camera: document\n\treturn values for application facing functions","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":"Wed, 27 Feb 2019 09:48:01 -0000"}},{"id":919,"web_url":"https://patchwork.libcamera.org/comment/919/","msgid":"<20190227130107.GD4813@pendragon.ideasonboard.com>","date":"2019-02-27T13:01:07","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: camera: document\n\treturn values for application facing functions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Feb 27, 2019 at 10:48:29AM +0100, Jacopo Mondi wrote:\n> On Tue, Feb 26, 2019 at 07:53:50PM +0100, Niklas Söderlund wrote:\n> > On 2019-02-26 17:56:09 +0100, Jacopo Mondi wrote:\n> >> On Tue, Feb 26, 2019 at 03:18:52AM +0100, Niklas Söderlund wrote:\n> >>> In preparation for adding a state machine to control access to the\n> >>> functions facing the application document the error codes which will be\n> >>> returned once it's in place.\n> >>>\n> >>> Signed-off-by: Niklas Söderlund \n> >>> ---\n> >>> src/libcamera/camera.cpp | 12 +++++++++---\n> >>> 1 file changed, 9 insertions(+), 3 deletions(-)\n> >>>\n> >>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> >>> index e2372fea31e2921a..d4258fe3c7551af3 100644\n> >>> --- a/src/libcamera/camera.cpp\n> >>> +++ b/src/libcamera/camera.cpp\n> >>> @@ -158,6 +158,7 @@ void Camera::disconnect()\n> >>> * \\todo Implement exclusive access across processes.\n> >>> *\n> >>> * \\return 0 on success or a negative error code\n> >>> + * \\retval -EBUSY The camera is not free and can't be acquired by the caller.\n> >>> */\n> >>> int Camera::acquire()\n> >>> {\n> >>> @@ -236,9 +237,8 @@ Camera::streamConfiguration(std::vector &streams)\n> >>> * to calling this function, otherwise an -EACCES error will be returned.\n> >>> *\n> >>> * \\return 0 on success or a negative error code\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 -EINVAL The configuration is not valid\n> >>> + * \\retval -EACCES The camera is not in a state where it can be configured.\n\nDid we standardize on periods at the end of \\retval statements ?\n\n> >>\n> >> I'm not sure I like \"the camera is not in a state where\", here and in other\n> >> places, but I got nothing better to offer, so feel free to keep it there :)\n\nWouldn't it make sense to keep a different return code to report camera\ndisconnection, to simplify hot-unplug handling ?\n\n> >>> + * \\retval -EINVAL The configuration is not valid.\n> >>> */\n> >>> int Camera::configureStreams(std::map &config)\n> >>> {\n> >>> @@ -279,6 +279,8 @@ int Camera::configureStreams(std::map &config)\n> >>> /**\n> >>> * \\brief Allocate buffers for all configured streams\n> >>> * \\return 0 on success or a negative error code\n> >>> + * \\retval -EACCES The camera is not in a state where it can allocate buffers.\n> >>> + * \\retval -EINVAL The configuration is not valid.\n> >>> */\n> >>> int Camera::allocateBuffers()\n> >>> {\n> >>> @@ -330,6 +332,7 @@ void Camera::freeBuffers()\n> >>> * responsible for either queueing the request or deleting it.\n> >>> *\n> >>> * \\return A pointer to the newly created request, or nullptr on error\n> >>> + * \\retval -EACCES The camera is not in a state where requests can be created.\n> >>\n> >> This returns nullptr, am I wrong?\n> >\n> > Correct, my bad will drop this change for v2.\n> >\n> >>> */\n> >>> Request *Camera::createRequest()\n> >>> {\n> >>> @@ -351,6 +354,7 @@ Request *Camera::createRequest()\n> >>> * automatically after it completes.\n> >>> *\n> >>> * \\return 0 on success or a negative error code\n> >>> + * \\retval -EACCES The camera is not running so requests can't be queued.\n> >>\n> >> I think this returns EBUSY, right?\n> >>\n> >> Well, I assume this patch should come after you have changed the code,\n> >> which I assume happens later in the series, otherwise it is difficult\n> >> to make sure the comments are correct.\n> >\n> > As the commit message says this updates the documentation only for the\n> > new return codes which are introduced in 5/8. I contemplated squashing\n> > the two but opted to keep the separate.\n> >\n> > As the cam tool needs to be updated (patch 4/5) before the state machine\n> > is added I judged it to be easier to review this in these three parts;\n> > documentation, cam tool, state machine. As it the cam tool change would\n> > then make sens. If this is confusing I can squash this in with 5/8. Let\n> > me know what you think.\n> \n> No need to squash if you don't want to, I was just pointing out that\n> to make sure documentation is updated properly, I had to look at this\n> after 5/8 is applied. I would have just re-sorted them, but that's\n> fine.\n\nI would have squashed them too. If this was about code you'd create a\nbisection error, and it's more difficult to review this way. No big deal\nthough.\n\n> >>> */\n> >>> int Camera::queueRequest(Request *request)\n> >>> {\n> >>> @@ -377,6 +381,7 @@ 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\n> >>> + * \\retval -EACCES The camera is not in a state where it can be started.\n> >>> */\n> >>> int Camera::start()\n> >>> {\n> >>> @@ -396,6 +401,7 @@ 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\n> >>> + * \\retval -EACCES The camera is not running so can't be stopped.\n> >>> */\n> >>> int Camera::stop()\n> >>> {","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 099D1600FD\n\tfor ;\n\tWed, 27 Feb 2019 14:01:13 +0100 (CET)","from pendragon.ideasonboard.com (unknown [185.25.200.246])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 780D249;\n\tWed, 27 Feb 2019 14:01:12 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551272472;\n\tbh=vzSR/pH2sqC7uvXP5J1mipQbEX+CIQc6pTZvrErBwlI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Dq2kHEku7Rzh5VTsAKPLxpThVkzYU64Yr3Za6JvrWft6Wfem9ztAsNH9d+HxLo2pP\n\tghakH/du0WrLnZ+oeQ376WB6Gxr2AvRU+SX4bS6hv/ALdjO997KrCj7pJq+sGFNHiw\n\taahShK/+aUhJGWX61mAK9E3YiezRJ5w5p+IURc5c=","Date":"Wed, 27 Feb 2019 15:01:07 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190227130107.GD4813@pendragon.ideasonboard.com>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-4-niklas.soderlund@ragnatech.se>\n\t<20190226165609.2ulu64fzegy7scut@uno.localdomain>\n\t<20190226185350.GK899@bigcity.dyn.berto.se>\n\t<20190227094829.wn4jdifscie2t6jj@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190227094829.wn4jdifscie2t6jj@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: camera: document\n\treturn values for application facing functions","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":"Wed, 27 Feb 2019 13:01:13 -0000"}},{"id":937,"web_url":"https://patchwork.libcamera.org/comment/937/","msgid":"<20190227190245.GW899@bigcity.dyn.berto.se>","date":"2019-02-27T19:02:45","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: camera: document\n\treturn values for application facing functions","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-27 15:01:07 +0200, Laurent Pinchart wrote:\n> Hello,\n> \n> On Wed, Feb 27, 2019 at 10:48:29AM +0100, Jacopo Mondi wrote:\n> > On Tue, Feb 26, 2019 at 07:53:50PM +0100, Niklas Söderlund wrote:\n> > > On 2019-02-26 17:56:09 +0100, Jacopo Mondi wrote:\n> > >> On Tue, Feb 26, 2019 at 03:18:52AM +0100, Niklas Söderlund wrote:\n> > >>> In preparation for adding a state machine to control access to the\n> > >>> functions facing the application document the error codes which will be\n> > >>> returned once it's in place.\n> > >>>\n> > >>> Signed-off-by: Niklas Söderlund \n> > >>> ---\n> > >>> src/libcamera/camera.cpp | 12 +++++++++---\n> > >>> 1 file changed, 9 insertions(+), 3 deletions(-)\n> > >>>\n> > >>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > >>> index e2372fea31e2921a..d4258fe3c7551af3 100644\n> > >>> --- a/src/libcamera/camera.cpp\n> > >>> +++ b/src/libcamera/camera.cpp\n> > >>> @@ -158,6 +158,7 @@ void Camera::disconnect()\n> > >>> * \\todo Implement exclusive access across processes.\n> > >>> *\n> > >>> * \\return 0 on success or a negative error code\n> > >>> + * \\retval -EBUSY The camera is not free and can't be acquired by the caller.\n> > >>> */\n> > >>> int Camera::acquire()\n> > >>> {\n> > >>> @@ -236,9 +237,8 @@ Camera::streamConfiguration(std::vector &streams)\n> > >>> * to calling this function, otherwise an -EACCES error will be returned.\n> > >>> *\n> > >>> * \\return 0 on success or a negative error code\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 -EINVAL The configuration is not valid\n> > >>> + * \\retval -EACCES The camera is not in a state where it can be configured.\n> \n> Did we standardize on periods at the end of \\retval statements ?\n\nWoops, I think we standardize on not having . at the end of \\retval \nstatement. At least that what the rest of the code base does, will \nremove it for v2.\n\n> \n> > >>\n> > >> I'm not sure I like \"the camera is not in a state where\", here and in other\n> > >> places, but I got nothing better to offer, so feel free to keep it there :)\n> \n> Wouldn't it make sense to keep a different return code to report camera\n> disconnection, to simplify hot-unplug handling ?\n\nI wrestled with they question myself when writing this but abandoned to \ntry and keep the logic simple. But as you now suggest it I will give it \nanother try and see if I can add it and still keep it simple.\n\n> \n> > >>> + * \\retval -EINVAL The configuration is not valid.\n> > >>> */\n> > >>> int Camera::configureStreams(std::map &config)\n> > >>> {\n> > >>> @@ -279,6 +279,8 @@ int Camera::configureStreams(std::map &config)\n> > >>> /**\n> > >>> * \\brief Allocate buffers for all configured streams\n> > >>> * \\return 0 on success or a negative error code\n> > >>> + * \\retval -EACCES The camera is not in a state where it can allocate buffers.\n> > >>> + * \\retval -EINVAL The configuration is not valid.\n> > >>> */\n> > >>> int Camera::allocateBuffers()\n> > >>> {\n> > >>> @@ -330,6 +332,7 @@ void Camera::freeBuffers()\n> > >>> * responsible for either queueing the request or deleting it.\n> > >>> *\n> > >>> * \\return A pointer to the newly created request, or nullptr on error\n> > >>> + * \\retval -EACCES The camera is not in a state where requests can be created.\n> > >>\n> > >> This returns nullptr, am I wrong?\n> > >\n> > > Correct, my bad will drop this change for v2.\n> > >\n> > >>> */\n> > >>> Request *Camera::createRequest()\n> > >>> {\n> > >>> @@ -351,6 +354,7 @@ Request *Camera::createRequest()\n> > >>> * automatically after it completes.\n> > >>> *\n> > >>> * \\return 0 on success or a negative error code\n> > >>> + * \\retval -EACCES The camera is not running so requests can't be queued.\n> > >>\n> > >> I think this returns EBUSY, right?\n> > >>\n> > >> Well, I assume this patch should come after you have changed the code,\n> > >> which I assume happens later in the series, otherwise it is difficult\n> > >> to make sure the comments are correct.\n> > >\n> > > As the commit message says this updates the documentation only for the\n> > > new return codes which are introduced in 5/8. I contemplated squashing\n> > > the two but opted to keep the separate.\n> > >\n> > > As the cam tool needs to be updated (patch 4/5) before the state machine\n> > > is added I judged it to be easier to review this in these three parts;\n> > > documentation, cam tool, state machine. As it the cam tool change would\n> > > then make sens. If this is confusing I can squash this in with 5/8. Let\n> > > me know what you think.\n> > \n> > No need to squash if you don't want to, I was just pointing out that\n> > to make sure documentation is updated properly, I had to look at this\n> > after 5/8 is applied. I would have just re-sorted them, but that's\n> > fine.\n> \n> I would have squashed them too. If this was about code you'd create a\n> bisection error, and it's more difficult to review this way. No big deal\n> though.\n\nI will squash it in v2 as it's now served it's purpose to to spur \ndiscussion :-)\n\n> \n> > >>> */\n> > >>> int Camera::queueRequest(Request *request)\n> > >>> {\n> > >>> @@ -377,6 +381,7 @@ 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\n> > >>> + * \\retval -EACCES The camera is not in a state where it can be started.\n> > >>> */\n> > >>> int Camera::start()\n> > >>> {\n> > >>> @@ -396,6 +401,7 @@ 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\n> > >>> + * \\retval -EACCES The camera is not running so can't be stopped.\n> > >>> */\n> > >>> int Camera::stop()\n> > >>> {\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 17C2C600FD\n\tfor ;\n\tWed, 27 Feb 2019 20:02:48 +0100 (CET)","by mail-lf1-x135.google.com with SMTP id 131so4067008lfa.5\n\tfor ;\n\tWed, 27 Feb 2019 11:02:48 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tg24sm3014481ljj.87.2019.02.27.11.02.46\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tWed, 27 Feb 2019 11:02:46 -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=aW7QUSDh18aCsJuJu+BnfW4OXRX+oiCpQ0MF5x5Sjc8=;\n\tb=S2fFbweqqdiTOO8AZneq/rs/m2wwx22xGBJp6VNw37S5fCg21DPzAwGVfy1ZDg67Bm\n\tMMol+mniPw3CCcFZlZoPkobZmm24xlAMu/PxiY1m472hK1Ya7O07s/p0B+eTtg4wvJ5l\n\tCIOcwvmwCEdlL8WJj5dZ/1dPYoZjUAkRY+gEcHvMucFWBGCDw1ph55U1uKzW0DUc2d2C\n\twBBOrS6f0Nj5czg/sXMsZNIFCoVPC/K1X7B3t+k7xxeX2yNAfDiEgI2dW+Jayfpxgb/a\n\tZ2ca3x7MlBdooqfj3JSkufXWw0QXdhre+cnN+KZojDvm88tp2wov09MuLv/hLnGUrTlg\n\t2yfA==","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=aW7QUSDh18aCsJuJu+BnfW4OXRX+oiCpQ0MF5x5Sjc8=;\n\tb=sMmUrMAphDZ3xOnABADJWfnNOOgv893dNGYEFLkQpa6fhP5dMvfwewHCfA0Z60EAP3\n\t9gRi1ibdt0ysHlPf27zFHdAzhrLrV5upW7/uGdRW6hk6XAOKWcIZX4KWd6CSq6V1xgT4\n\t66pGSxaj/WmGH2ueGRB1D6bkyKYZKFWOaniDrad6k6TJGmJXBmdbrc2bNYZTC5oIrE8q\n\tLRQybYupCR5St0Kmo6s/ChQ9nKRC2U/TBga42vEXqkDLUHoRwoKOtEhdglHSDsgauyDW\n\tdj1kq9CwYDtBswW2TssZYqwyqL3FnPUitjh4MNe5pifzz2EL+b/1o4RUG0V/0tgEHJTz\n\tpo0g==","X-Gm-Message-State":"AHQUAuZrq85Aa57iSf79B2hK7kaD7dTpeVMjDQ6wtpofTuI/GMxRztzD\n\ttLjP1qhlwpQc5y6KeEnRHycpeg==","X-Google-Smtp-Source":"AHgI3IYuLHgKgc0h+wBRZrM5jATYx+JRzpU7GnwPwrSxdF3Vq/5TESy7jtZajlPfEvzZOMmspF4mKw==","X-Received":"by 2002:ac2:510e:: with SMTP id\n\tq14mr1735536lfb.151.1551294167265; \n\tWed, 27 Feb 2019 11:02:47 -0800 (PST)","Date":"Wed, 27 Feb 2019 20:02:45 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20190227190245.GW899@bigcity.dyn.berto.se>","References":"<20190226021857.28255-1-niklas.soderlund@ragnatech.se>\n\t<20190226021857.28255-4-niklas.soderlund@ragnatech.se>\n\t<20190226165609.2ulu64fzegy7scut@uno.localdomain>\n\t<20190226185350.GK899@bigcity.dyn.berto.se>\n\t<20190227094829.wn4jdifscie2t6jj@uno.localdomain>\n\t<20190227130107.GD4813@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":"<20190227130107.GD4813@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: camera: document\n\treturn values for application facing functions","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":"Wed, 27 Feb 2019 19:02:48 -0000"}}]