| Message ID | 20190226021857.28255-9-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Tue, Feb 26, 2019 at 03:18:57AM +0100, Niklas Söderlund wrote: > If the camera is not configured there is little use to start it. Add a > check to make sure the camera is configured before it's started. > As commented on the previous patch, shouldn't this (and more..) be caught by the state machine? Thanks j > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/camera.cpp | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index ba8638009992170f..cbc34599d25e5ed5 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -423,12 +423,19 @@ int Camera::queueRequest(Request *request) > * > * \return 0 on success or a negative error code > * \retval -EACCES The camera is not in a state where it can be started. > + * \retval -EINVAL The camera is not configured. > */ > int Camera::start() > { > if (!stateIs(Acquired)) > return -EACCES; > > + if (activeStreams_.empty()) { > + LOG(Camera, Error) > + << "Can't start camera without configuration"; > + return -EINVAL; > + } > + > LOG(Camera, Debug) << "Starting capture"; > > int ret = pipe_->start(this); > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thanks for your feedback. On 2019-02-26 18:21:26 +0100, Jacopo Mondi wrote: > Hi Niklas, > > On Tue, Feb 26, 2019 at 03:18:57AM +0100, Niklas Söderlund wrote: > > If the camera is not configured there is little use to start it. Add a > > check to make sure the camera is configured before it's started. > > > > As commented on the previous patch, shouldn't this (and more..) > be caught by the state machine? If we introduce a Configured state sometime in the future this could be replaced with such a check. As I replied earlier, this series aim is to enforce the current behavior and what have been discussed. Currently there is no way for an application to un-configure a camera so adding such a state now is not possible. > > Thanks > j > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/camera.cpp | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index ba8638009992170f..cbc34599d25e5ed5 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -423,12 +423,19 @@ int Camera::queueRequest(Request *request) > > * > > * \return 0 on success or a negative error code > > * \retval -EACCES The camera is not in a state where it can be started. > > + * \retval -EINVAL The camera is not configured. > > */ > > int Camera::start() > > { > > if (!stateIs(Acquired)) > > return -EACCES; > > > > + if (activeStreams_.empty()) { > > + LOG(Camera, Error) > > + << "Can't start camera without configuration"; > > + return -EINVAL; > > + } > > + > > LOG(Camera, Debug) << "Starting capture"; > > > > int ret = pipe_->start(this); > > -- > > 2.20.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Tue, Feb 26, 2019 at 08:08:54PM +0100, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your feedback. > > On 2019-02-26 18:21:26 +0100, Jacopo Mondi wrote: > > Hi Niklas, > > > > On Tue, Feb 26, 2019 at 03:18:57AM +0100, Niklas Söderlund wrote: > > > If the camera is not configured there is little use to start it. Add a > > > check to make sure the camera is configured before it's started. > > > > > > > As commented on the previous patch, shouldn't this (and more..) > > be caught by the state machine? > > If we introduce a Configured state sometime in the future this could be > replaced with such a check. As I replied earlier, this series aim is to > enforce the current behavior and what have been discussed. Currently > there is no way for an application to un-configure a camera so adding > such a state now is not possible. > It was my intention to start discussing if we want an explicit 'unconfigure' step, that would make sure 'allocate' and 'free' buffers could only be called on a configured Camera (and possible hide 'free' (and allocate?) completely from the application). If you do consider this is not the right time to do so, please add my Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Thanks > > j > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > src/libcamera/camera.cpp | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index ba8638009992170f..cbc34599d25e5ed5 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -423,12 +423,19 @@ int Camera::queueRequest(Request *request) > > > * > > > * \return 0 on success or a negative error code > > > * \retval -EACCES The camera is not in a state where it can be started. > > > + * \retval -EINVAL The camera is not configured. > > > */ > > > int Camera::start() > > > { > > > if (!stateIs(Acquired)) > > > return -EACCES; > > > > > > + if (activeStreams_.empty()) { > > > + LOG(Camera, Error) > > > + << "Can't start camera without configuration"; > > > + return -EINVAL; > > > + } > > > + > > > LOG(Camera, Debug) << "Starting capture"; > > > > > > int ret = pipe_->start(this); > > > -- > > > 2.20.1 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards, > Niklas Söderlund
Hi Niklas, Thank you for the patch. On Tue, Feb 26, 2019 at 03:18:57AM +0100, Niklas Söderlund wrote: > If the camera is not configured there is little use to start it. Add a > check to make sure the camera is configured before it's started. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/camera.cpp | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index ba8638009992170f..cbc34599d25e5ed5 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -423,12 +423,19 @@ int Camera::queueRequest(Request *request) > * > * \return 0 on success or a negative error code > * \retval -EACCES The camera is not in a state where it can be started. > + * \retval -EINVAL The camera is not configured. > */ > int Camera::start() > { > if (!stateIs(Acquired)) > return -EACCES; > > + if (activeStreams_.empty()) { > + LOG(Camera, Error) > + << "Can't start camera without configuration"; > + return -EINVAL; > + } So this essentially creates a Configured state, without naming it. I think the implementation may be simpler if you added an explicit state, but there's no urgency, as the check is in place here. I however wonder if -EINVAL is the right error code, given that you use -EACCESS to denote state-related errors. > + > LOG(Camera, Debug) << "Starting capture"; > > int ret = pipe_->start(this);
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index ba8638009992170f..cbc34599d25e5ed5 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -423,12 +423,19 @@ int Camera::queueRequest(Request *request) * * \return 0 on success or a negative error code * \retval -EACCES The camera is not in a state where it can be started. + * \retval -EINVAL The camera is not configured. */ int Camera::start() { if (!stateIs(Acquired)) return -EACCES; + if (activeStreams_.empty()) { + LOG(Camera, Error) + << "Can't start camera without configuration"; + return -EINVAL; + } + LOG(Camera, Debug) << "Starting capture"; int ret = pipe_->start(this);
If the camera is not configured there is little use to start it. Add a check to make sure the camera is configured before it's started. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/camera.cpp | 7 +++++++ 1 file changed, 7 insertions(+)