lit-robotics / libcamera-rs

Experimental Rust bindings for libcamera
Apache License 2.0
51 stars 16 forks source link

Add error handling for queuing functions #12

Closed mfreeborn closed 8 months ago

mfreeborn commented 1 year ago

Following on from discussions in #10, this PR demonstrates a possible implementation of improved error handling, focusing just on one group of errors at present - those returned by libcamera_camera_queue_request.

There's a couple of ways of doing this, depending on how much one wants to read into the libcamera code.

The simplest solution is just to map all error numbers to a rust enum, e.g.

#[derive(Debug, thiserror::Error)]
pub enum LibcameraError {
    #[error("No device found")]
    ENODEV,
    #[error("Access error")]
    EACCES,
    #[error(transparent)]
    Unexpected(#[from] io::Error),
    // etc
}

// then map return codes to rust errors

let ret = libcamera_camera_queue_request(req);
match ret {
    0 => Ok(()),
    v if v == -libc::ENODEV => Err(LibcameraError::ENODEV),
    v if c == -libc......
    // catch all
    v => Err(LibcameraError::Unexpected(io::Error::from_raw_os_error(v)))
}

That would still be better than just returning an io::Error which would need to be interrogated with io::Error.kind() to yield the particular type.

In the pull request, I've demonstrated a fuller approach which gives specific reasons for why the code produced an error \but\ at the expense of reading through the libcamera C code and following all the error paths.

I've be interested to hear thoughts on how I've implemented it in the PR versus any other preferred way.

mfreeborn commented 1 year ago

Just to add, I've added a basic test which relies upon a camera device being present... I'm not sure how to emulate that on CI.

mfreeborn commented 1 year ago

Yes, adding sudo ldconfig after building libcamera is required lest we get the error: error while loading shared libraries: libcamera-base.so.0.0.4: cannot open shared object file: No such file or directory.

Apparently this should have been fixed, so I've re-raised the question: https://github.com/kbingham/libcamera/issues/17

mfreeborn commented 1 year ago

I've just added an example for CameraManager, using a slightly different API (impl'ing from_raw_os_error directly on CameraManagerError to move error handling logic closer to the error type itself). This works when there are no clashes between libcamera error codes and their meanings, as is the case here.

chemicstry commented 1 year ago

In the pull request, I've demonstrated a fuller approach which gives specific reasons for why the code produced an error \but\ at the expense of reading through the libcamera C code and following all the error paths.

I've be interested to hear thoughts on how I've implemented it in the PR versus any other preferred way.

This is exactly how I think it should be done. I don't think there is much use of just re-writting io::Error like in the example.