[{"id":16531,"web_url":"https://patchwork.libcamera.org/comment/16531/","msgid":"<b77ccf3c-7dee-4410-dd53-73d81e866ab5@ideasonboard.com>","date":"2021-04-23T15:10:19","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Make stop()\n\tidempotent","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn 23/04/2021 15:24, Nícolas F. R. A. Prado wrote:\n> Make Camera::stop() idempotent so that it can be called in any state and\n> consecutive times. When called in any state other than CameraRunning, it\n> is a no-op. This simplifies the cleanup path for applications.\n> \n> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>\n> ---\n> This will be used to simplify the cleanup path in lc-compliance without having\n> error messages [1].\n> \n> Also, I'm not sure if I should add the silent parameter to the other\n> isAccessAllowed variant as well. It would make sense for consistency's sake, but\n> it's not needed currently.\n> \n> [1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019703.html\n> \n>  src/libcamera/camera.cpp | 29 ++++++++++++++++++-----------\n>  1 file changed, 18 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 763f3b9926fd..baffdafc8146 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -350,7 +350,7 @@ public:\n>  \tint isAccessAllowed(State state, bool allowDisconnected = false,\n>  \t\t\t    const char *from = __builtin_FUNCTION()) const;\n>  \tint isAccessAllowed(State low, State high,\n> -\t\t\t    bool allowDisconnected = false,\n> +\t\t\t    bool allowDisconnected = false, bool silent = false,\n>  \t\t\t    const char *from = __builtin_FUNCTION()) const;\n>  \n>  \tvoid disconnect();\n> @@ -408,7 +408,7 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected,\n>  }\n>  \n>  int Camera::Private::isAccessAllowed(State low, State high,\n> -\t\t\t\t     bool allowDisconnected,\n> +\t\t\t\t     bool allowDisconnected, bool silent,\n>  \t\t\t\t     const char *from) const\n>  {\n>  \tif (!allowDisconnected && disconnected_)\n> @@ -421,11 +421,12 @@ int Camera::Private::isAccessAllowed(State low, State high,\n>  \tASSERT(static_cast<unsigned int>(low) < std::size(camera_state_names) &&\n>  \t       static_cast<unsigned int>(high) < std::size(camera_state_names));\n>  \n> -\tLOG(Camera, Error) << \"Camera in \" << camera_state_names[currentState]\n> -\t\t\t   << \" state trying \" << from\n> -\t\t\t   << \"() requiring state between \"\n> -\t\t\t   << camera_state_names[low] << \" and \"\n> -\t\t\t   << camera_state_names[high];\n> +\tif (!silent)\n> +\t\tLOG(Camera, Error) << \"Camera in \" << camera_state_names[currentState]\n> +\t\t\t\t   << \" state trying \" << from\n> +\t\t\t\t   << \"() requiring state between \"\n> +\t\t\t\t   << camera_state_names[low] << \" and \"\n> +\t\t\t\t   << camera_state_names[high];\n>  \n>  \treturn -EACCES;\n>  }\n> @@ -1061,9 +1062,10 @@ int Camera::start(const ControlList *controls)\n>   * This method stops capturing and processing requests immediately. All pending\n>   * requests are cancelled and complete synchronously in an error state.\n>   *\n> - * \\context This function may only be called when the camera is in the Running\n> - * state as defined in \\ref camera_operation, and shall be synchronized by the\n> - * caller with other functions that affect the camera state.\n> + * \\context This function may be called even if the camera isn't in the Running\n> + * state as defined in \\ref camera_operation, in which cases it is a no-op. It\n> + * shall be synchronized by the caller with other functions that affect the\n> + * camera state.\n>   *\n>   * \\return 0 on success or a negative error code otherwise\n>   * \\retval -ENODEV The camera has been disconnected from the system\n> @@ -1073,7 +1075,12 @@ int Camera::stop()\n>  {\n>  \tPrivate *const d = LIBCAMERA_D_PTR();\n>  \n> -\tint ret = d->isAccessAllowed(Private::CameraRunning);\n> +\tint ret = d->isAccessAllowed(Private::CameraAvailable,\n> +\t\t\tPrivate::CameraStopping, false, true);\n> +\tif (!ret)\n> +\t\treturn 0;\n\nThis seems like a lot of complexity to add to be able to return out of\nthis function silently if the state is not running.\n\nEffectively we're saying that this function 'Is allowed' in any state,\nbut it simply returns early if state < CameraRunning. That makes me\nthink using a function called 'isAccessAllowed()' isn't quite right -\nIt's always allowed.\n\nWould it be better to provide an accessor on the private state to better\nrepresent that?\n\nLike isRunning()?\n\nThen this simply becomes\n\n\tif (!d->isRunning())\n\t\treturn 0;\n\n\n\n\n> +\n> +\tret = d->isAccessAllowed(Private::CameraRunning);\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>  \n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7B660BDB15\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 23 Apr 2021 15:10:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F0B306887C;\n\tFri, 23 Apr 2021 17:10:24 +0200 (CEST)","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 86E366885A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 23 Apr 2021 17:10:23 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DB912332;\n\tFri, 23 Apr 2021 17:10:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"wGsX4XmD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619190623;\n\tbh=A+s1bjXXp7gQxZO+eC4ysvKKqXflUBEkypQlBbaUUqs=;\n\th=Reply-To:To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=wGsX4XmDjyQSZIZ9iPXuJ0WLF6umWs/nxPEi5D/jrLwmUJxrmdLJcqzGfjYFSyXv/\n\tODnFWodmn4sqokU87cTY6bm29Wvo101sUwGL73Igrmvv2sQpuXpP4FPPmzU90Ib5VE\n\tLp0TNOllukK9NSwXmM869m27kT0KGx1ijCRJ/mNo=","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= <nfraprado@collabora.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210423142412.460460-1-nfraprado@collabora.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Organization":"Ideas on Board","Message-ID":"<b77ccf3c-7dee-4410-dd53-73d81e866ab5@ideasonboard.com>","Date":"Fri, 23 Apr 2021 16:10:19 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"<20210423142412.460460-1-nfraprado@collabora.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Make stop()\n\tidempotent","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"kernel@collabora.com, =?utf-8?q?Andr=C3=A9_Almeida?=\n\t<andrealmeid@collabora.com>","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16532,"web_url":"https://patchwork.libcamera.org/comment/16532/","msgid":"<CAV8V006VDX8.FX7CVOXTAU6P@notapiano>","date":"2021-04-23T16:29:47","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Make stop()\n\tidempotent","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Em 2021-04-23 12:10, Kieran Bingham escreveu:\n\n> Hi Nicolas,\n>\n> On 23/04/2021 15:24, Nícolas F. R. A. Prado wrote:\n> > Make Camera::stop() idempotent so that it can be called in any state and\n> > consecutive times. When called in any state other than CameraRunning, it\n> > is a no-op. This simplifies the cleanup path for applications.\n> > \n> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>\n> > ---\n> > This will be used to simplify the cleanup path in lc-compliance without having\n> > error messages [1].\n> > \n> > Also, I'm not sure if I should add the silent parameter to the other\n> > isAccessAllowed variant as well. It would make sense for consistency's sake, but\n> > it's not needed currently.\n> > \n> > [1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019703.html\n> > \n> >  src/libcamera/camera.cpp | 29 ++++++++++++++++++-----------\n> >  1 file changed, 18 insertions(+), 11 deletions(-)\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 763f3b9926fd..baffdafc8146 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -350,7 +350,7 @@ public:\n> >  \tint isAccessAllowed(State state, bool allowDisconnected = false,\n> >  \t\t\t    const char *from = __builtin_FUNCTION()) const;\n> >  \tint isAccessAllowed(State low, State high,\n> > -\t\t\t    bool allowDisconnected = false,\n> > +\t\t\t    bool allowDisconnected = false, bool silent = false,\n> >  \t\t\t    const char *from = __builtin_FUNCTION()) const;\n> >  \n> >  \tvoid disconnect();\n> > @@ -408,7 +408,7 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected,\n> >  }\n> >  \n> >  int Camera::Private::isAccessAllowed(State low, State high,\n> > -\t\t\t\t     bool allowDisconnected,\n> > +\t\t\t\t     bool allowDisconnected, bool silent,\n> >  \t\t\t\t     const char *from) const\n> >  {\n> >  \tif (!allowDisconnected && disconnected_)\n> > @@ -421,11 +421,12 @@ int Camera::Private::isAccessAllowed(State low, State high,\n> >  \tASSERT(static_cast<unsigned int>(low) < std::size(camera_state_names) &&\n> >  \t       static_cast<unsigned int>(high) < std::size(camera_state_names));\n> >  \n> > -\tLOG(Camera, Error) << \"Camera in \" << camera_state_names[currentState]\n> > -\t\t\t   << \" state trying \" << from\n> > -\t\t\t   << \"() requiring state between \"\n> > -\t\t\t   << camera_state_names[low] << \" and \"\n> > -\t\t\t   << camera_state_names[high];\n> > +\tif (!silent)\n> > +\t\tLOG(Camera, Error) << \"Camera in \" << camera_state_names[currentState]\n> > +\t\t\t\t   << \" state trying \" << from\n> > +\t\t\t\t   << \"() requiring state between \"\n> > +\t\t\t\t   << camera_state_names[low] << \" and \"\n> > +\t\t\t\t   << camera_state_names[high];\n> >  \n> >  \treturn -EACCES;\n> >  }\n> > @@ -1061,9 +1062,10 @@ int Camera::start(const ControlList *controls)\n> >   * This method stops capturing and processing requests immediately. All pending\n> >   * requests are cancelled and complete synchronously in an error state.\n> >   *\n> > - * \\context This function may only be called when the camera is in the Running\n> > - * state as defined in \\ref camera_operation, and shall be synchronized by the\n> > - * caller with other functions that affect the camera state.\n> > + * \\context This function may be called even if the camera isn't in the Running\n> > + * state as defined in \\ref camera_operation, in which cases it is a no-op. It\n> > + * shall be synchronized by the caller with other functions that affect the\n> > + * camera state.\n> >   *\n> >   * \\return 0 on success or a negative error code otherwise\n> >   * \\retval -ENODEV The camera has been disconnected from the system\n> > @@ -1073,7 +1075,12 @@ int Camera::stop()\n> >  {\n> >  \tPrivate *const d = LIBCAMERA_D_PTR();\n> >  \n> > -\tint ret = d->isAccessAllowed(Private::CameraRunning);\n> > +\tint ret = d->isAccessAllowed(Private::CameraAvailable,\n> > +\t\t\tPrivate::CameraStopping, false, true);\n> > +\tif (!ret)\n> > +\t\treturn 0;\n>\n> This seems like a lot of complexity to add to be able to return out of\n> this function silently if the state is not running.\n>\n> Effectively we're saying that this function 'Is allowed' in any state,\n> but it simply returns early if state < CameraRunning. That makes me\n> think using a function called 'isAccessAllowed()' isn't quite right -\n> It's always allowed.\n>\n> Would it be better to provide an accessor on the private state to better\n> represent that?\n>\n> Like isRunning()?\n>\n> Then this simply becomes\n>\n> if (!d->isRunning())\n> return 0;\n\nYep, that sounds a lot better. I'll do that instead for v2.\n\nThanks,\nNícolas","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9C407BDB8F\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 23 Apr 2021 16:33:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 72FF168879;\n\tFri, 23 Apr 2021 18:33:05 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 755C768878\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 23 Apr 2021 18:33:04 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nfraprado) with ESMTPSA id 1A2081F43D52"],"Mime-Version":"1.0","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= <nfraprado@collabora.com>","To":"<kieran.bingham@ideasonboard.com>, <libcamera-devel@lists.libcamera.org>","Date":"Fri, 23 Apr 2021 13:29:47 -0300","Message-Id":"<CAV8V006VDX8.FX7CVOXTAU6P@notapiano>","In-Reply-To":"<b77ccf3c-7dee-4410-dd53-73d81e866ab5@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Make stop()\n\tidempotent","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"kernel@collabora.com, =?utf-8?q?Andr=C3=A9_Almeida?=\n\t<andrealmeid@collabora.com>","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16533,"web_url":"https://patchwork.libcamera.org/comment/16533/","msgid":"<YIXPLop6dWcySuvk@pendragon.ideasonboard.com>","date":"2021-04-25T20:21:02","subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Make stop()\n\tidempotent","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nícolas,\n\nOn Fri, Apr 23, 2021 at 01:29:47PM -0300, Nícolas F. R. A. Prado wrote:\n> Em 2021-04-23 12:10, Kieran Bingham escreveu:\n> > On 23/04/2021 15:24, Nícolas F. R. A. Prado wrote:\n> > > Make Camera::stop() idempotent so that it can be called in any state and\n> > > consecutive times. When called in any state other than CameraRunning, it\n> > > is a no-op. This simplifies the cleanup path for applications.\n> > > \n> > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>\n> > > ---\n> > > This will be used to simplify the cleanup path in lc-compliance without having\n> > > error messages [1].\n> > > \n> > > Also, I'm not sure if I should add the silent parameter to the other\n> > > isAccessAllowed variant as well. It would make sense for consistency's sake, but\n> > > it's not needed currently.\n> > > \n> > > [1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019703.html\n> > > \n> > >  src/libcamera/camera.cpp | 29 ++++++++++++++++++-----------\n> > >  1 file changed, 18 insertions(+), 11 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 763f3b9926fd..baffdafc8146 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -350,7 +350,7 @@ public:\n> > >  \tint isAccessAllowed(State state, bool allowDisconnected = false,\n> > >  \t\t\t    const char *from = __builtin_FUNCTION()) const;\n> > >  \tint isAccessAllowed(State low, State high,\n> > > -\t\t\t    bool allowDisconnected = false,\n> > > +\t\t\t    bool allowDisconnected = false, bool silent = false,\n\nI know this will change in v2, but on a general note, bool parameters\nare not great (this applies to the existing allowDisconnected parameter\ntoo). When reading the caller\n\n\tint ret = d->isAccessAllowed(Private::CameraAvailable,\n\t\t\tPrivate::CameraStopping, false, true);\n\nit's hard to tell what false and true refer to. Instead, the following\nwould be better:\n\n\tint ret = d->isAccessAllowed(Private::CameraAvailable,\n\t\t\t\t     Private::CameraStopping,\n\t\t\t\t     Private::AccessCheck::NoLogging);\n\nwith a new\n\n\tenum AccessCbeck {\n\t\tAllowDisconnected = 0x1,\n\t\tNoLogging = 0x2,\n\t};\n\nin the Private class. An enum class would be even better, but that\nwouldn't allow us to | multiple flags.\nhttps://patchwork.libcamera.org/cover/8991/ could be a solution.\n\n> > >  \t\t\t    const char *from = __builtin_FUNCTION()) const;\n> > >  \n> > >  \tvoid disconnect();\n> > > @@ -408,7 +408,7 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected,\n> > >  }\n> > >  \n> > >  int Camera::Private::isAccessAllowed(State low, State high,\n> > > -\t\t\t\t     bool allowDisconnected,\n> > > +\t\t\t\t     bool allowDisconnected, bool silent,\n> > >  \t\t\t\t     const char *from) const\n> > >  {\n> > >  \tif (!allowDisconnected && disconnected_)\n> > > @@ -421,11 +421,12 @@ int Camera::Private::isAccessAllowed(State low, State high,\n> > >  \tASSERT(static_cast<unsigned int>(low) < std::size(camera_state_names) &&\n> > >  \t       static_cast<unsigned int>(high) < std::size(camera_state_names));\n> > >  \n> > > -\tLOG(Camera, Error) << \"Camera in \" << camera_state_names[currentState]\n> > > -\t\t\t   << \" state trying \" << from\n> > > -\t\t\t   << \"() requiring state between \"\n> > > -\t\t\t   << camera_state_names[low] << \" and \"\n> > > -\t\t\t   << camera_state_names[high];\n> > > +\tif (!silent)\n> > > +\t\tLOG(Camera, Error) << \"Camera in \" << camera_state_names[currentState]\n> > > +\t\t\t\t   << \" state trying \" << from\n> > > +\t\t\t\t   << \"() requiring state between \"\n> > > +\t\t\t\t   << camera_state_names[low] << \" and \"\n> > > +\t\t\t\t   << camera_state_names[high];\n> > >  \n> > >  \treturn -EACCES;\n> > >  }\n> > > @@ -1061,9 +1062,10 @@ int Camera::start(const ControlList *controls)\n> > >   * This method stops capturing and processing requests immediately. All pending\n> > >   * requests are cancelled and complete synchronously in an error state.\n> > >   *\n> > > - * \\context This function may only be called when the camera is in the Running\n> > > - * state as defined in \\ref camera_operation, and shall be synchronized by the\n> > > - * caller with other functions that affect the camera state.\n> > > + * \\context This function may be called even if the camera isn't in the Running\n> > > + * state as defined in \\ref camera_operation, in which cases it is a no-op. It\n> > > + * shall be synchronized by the caller with other functions that affect the\n> > > + * camera state.\n\nHow about stating that the function may be called in any camera state ?\n\n> > >   *\n> > >   * \\return 0 on success or a negative error code otherwise\n> > >   * \\retval -ENODEV The camera has been disconnected from the system\n> > > @@ -1073,7 +1075,12 @@ int Camera::stop()\n> > >  {\n> > >  \tPrivate *const d = LIBCAMERA_D_PTR();\n> > >  \n> > > -\tint ret = d->isAccessAllowed(Private::CameraRunning);\n> > > +\tint ret = d->isAccessAllowed(Private::CameraAvailable,\n> > > +\t\t\tPrivate::CameraStopping, false, true);\n> > > +\tif (!ret)\n> > > +\t\treturn 0;\n> >\n> > This seems like a lot of complexity to add to be able to return out of\n> > this function silently if the state is not running.\n> >\n> > Effectively we're saying that this function 'Is allowed' in any state,\n> > but it simply returns early if state < CameraRunning. That makes me\n> > think using a function called 'isAccessAllowed()' isn't quite right -\n> > It's always allowed.\n> >\n> > Would it be better to provide an accessor on the private state to better\n> > represent that?\n> >\n> > Like isRunning()?\n> >\n> > Then this simply becomes\n> >\n> > if (!d->isRunning())\n> > return 0;\n> \n> Yep, that sounds a lot better. I'll do that instead for v2.","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BA79CBDC91\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 25 Apr 2021 20:21:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 099B26887C;\n\tSun, 25 Apr 2021 22:21:12 +0200 (CEST)","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 7951D68878\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 25 Apr 2021 22:21:08 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C12F988F;\n\tSun, 25 Apr 2021 22:21:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YBv3kH5A\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619382067;\n\tbh=AwAHjBWvkqSB7m7MLv1aVneW2JhB1N0VxUmJfAhQ9+w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YBv3kH5AXxD+P6WFjkpW82NiCZ+qdE+UxhSeX+dppXwhORTZVFqXLvl7aMunSl7Vt\n\tcEP7NZJz9C8ZDKHkFpq2xzz8yT+CauKUjnb+/GiggNivXsKskaYk1ubpbOsaxYRdTG\n\t5UEztzzAoZ5GPgVUfB23HKgk/Ikq2kG+grSt1ATQ=","Date":"Sun, 25 Apr 2021 23:21:02 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado <nfraprado@collabora.com>","Message-ID":"<YIXPLop6dWcySuvk@pendragon.ideasonboard.com>","References":"<b77ccf3c-7dee-4410-dd53-73d81e866ab5@ideasonboard.com>\n\t<CAV8V006VDX8.FX7CVOXTAU6P@notapiano>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAV8V006VDX8.FX7CVOXTAU6P@notapiano>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: camera: Make stop()\n\tidempotent","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida <andrealmeid@collabora.com>","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]