[libcamera-devel] [PATCH 3/8] libcamera: camera: document return values for application facing functions

[Jacopo Mondi]

Hi Niklas,

On Tue, Feb 26, 2019 at 07:53:50PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2019-02-26 17:56:09 +0100, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Tue, Feb 26, 2019 at 03:18:52AM +0100, Niklas Söderlund wrote:
> > > In preparation for adding a state machine to control access to the
> > > functions facing the application document the error codes which will be
> > > returned once it's in place.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > >  src/libcamera/camera.cpp | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index e2372fea31e2921a..d4258fe3c7551af3 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -158,6 +158,7 @@ void Camera::disconnect()
> > >   * \todo Implement exclusive access across processes.
> > >   *
> > >   * \return 0 on success or a negative error code
> > > + * \retval -EBUSY The camera is not free and can't be acquired by the caller.
> > >   */
> > >  int Camera::acquire()
> > >  {
> > > @@ -236,9 +237,8 @@ Camera::streamConfiguration(std::vector<Stream *> &streams)
> > >   * to calling this function, otherwise an -EACCES error will be returned.
> > >   *
> > >   * \return 0 on success or a negative error code
> > > - * \retval -ENODEV The camera is not connected to any hardware
> > > - * \retval -EACCES The user has not acquired exclusive access to the camera
> > > - * \retval -EINVAL The configuration is not valid
> > > + * \retval -EACCES The camera is not in a state where it can be configured.
> >
> > I'm not sure I like "the camera is not in a state where", here and in other
> > places, but I got nothing better to offer, so feel free to keep it there :)
> >
> >
> > > + * \retval -EINVAL The configuration is not valid.
> > >   */
> > >  int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> > >  {
> > > @@ -279,6 +279,8 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> > >  /**
> > >   * \brief Allocate buffers for all configured streams
> > >   * \return 0 on success or a negative error code
> > > + * \retval -EACCES The camera is not in a state where it can allocate buffers.
> > > + * \retval -EINVAL The configuration is not valid.
> > >   */
> > >  int Camera::allocateBuffers()
> > >  {
> > > @@ -330,6 +332,7 @@ void Camera::freeBuffers()
> > >   * responsible for either queueing the request or deleting it.
> > >   *
> > >   * \return A pointer to the newly created request, or nullptr on error
> > > + * \retval -EACCES The camera is not in a state where requests can be created.
> >
> > This returns nullptr, am I wrong?
>
> Correct, my bad will drop this change for v2.
>
> >
> > >   */
> > >  Request *Camera::createRequest()
> > >  {
> > > @@ -351,6 +354,7 @@ Request *Camera::createRequest()
> > >   * automatically after it completes.
> > >   *
> > >   * \return 0 on success or a negative error code
> > > + * \retval -EACCES The camera is not running so requests can't be queued.
> >
> > I think this returns EBUSY, right?
> >
> > Well, I assume this patch should come after you have changed the code,
> > which I assume happens later in the series, otherwise it is difficult
> > to make sure the comments are correct.
>
> As the commit message says this updates the documentation only for the
> new return codes which are introduced in 5/8. I contemplated squashing
> the two but opted to keep the separate.
>
> As the cam tool needs to be updated (patch 4/5) before the state machine
> is added I judged it to be easier to review this in these three parts;
> documentation, cam tool, state machine. As it the cam tool change would
> then make sens. If this is confusing I can squash this in with 5/8. Let
> me know what you think.
>

No need to squash if you don't want to, I was just pointing out that
to make sure documentation is updated properly, I had to look at this
after 5/8 is applied. I would have just re-sorted them, but that's
fine.

> >
> >
> > >   */
> > >  int Camera::queueRequest(Request *request)
> > >  {
> > > @@ -377,6 +381,7 @@ int Camera::queueRequest(Request *request)
> > >   * until the capture session is terminated with \a stop().
> > >   *
> > >   * \return 0 on success or a negative error code
> > > + * \retval -EACCES The camera is not in a state where it can be started.
> > >   */
> > >  int Camera::start()
> > >  {
> > > @@ -396,6 +401,7 @@ int Camera::start()
> > >   * requests are cancelled and complete synchronously in an error state.
> > >   *
> > >   * \return 0 on success or a negative error code
> > > + * \retval -EACCES The camera is not running so can't be stopped.
> > >   */
> > >  int Camera::stop()
> > >  {
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
>
>
> --
> Regards,
> Niklas Söderlund
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190227/98418a4e/attachment.sig>