Make use of the typestate pattern

[mfreeborn]

There are several places in the library where different types need to be in specific states before they can be called, otherwise a panic! or similar behaviour will ensue.

For example, ActiveCamera::start() needs to be called before ActiveCamera::queue_request() and Camera::queue_request() can only be called with a Request that has had Request::add_buffer.

I think there is potential for the typestate pattern to be applied here.

I've created PR #9 which is a very very proof of concept implementation just focusing on the Request type, because it is small and self contained. There's a few issues with it which I would be happy to discuss further if this is a direction that people wish to go in, as I say, I only really mean to produce a proof of concept at this stage.

Notes: My implementation is based on this article An example of a codebase which uses it very elegantly is the Encoder struct here

[chemicstry]

Ideally functions should not panic and return an error instead, but I'm sure there are cases where it panics or fails assert in libcamera. These should be reported and fixed.

Regarding typestate pattern, I tried to use it where it makes sense (i.e. Camera::acquire() -> ActiveCamera), however in some cases it just makes the API unergonomic and too complicated to understand, especially when you add generics. The problem with proposed Request<S> is that it does not represent the entire state and some streams can still have missing buffers, while adding a lot of unnecessary mental overhead to the API.

TLDR, it depends

[mfreeborn]

A particular difficulty with Request is with libcamera_request_reuse. Depending on the flags passed to it, it either transitions to a WithoutBuffers state or a WithBuffers state, at which point I think the only way to handle that in a reasonable way in rust is to split that into two methods like I've done with Request::reuse and Request::reset. The more flags there are, the more complex this becomes.

Perhaps in the meantime, it would be worth implementing good runtime error handling at least, if it's not practical to implement compile time guarantees.

For example, calling ActiveCamera::queue_request returns the error Os { code: -22, kind: Uncategorized, message: "Unknown error -22" } if called with a request that has no buffers. libcamera itself does at least print out [10:17:35.531738771] [100656] ERROR Camera camera.cpp:1136 Request contains no buffers to the shell. I might have a look to see if I can handle that better...

[chemicstry]

For example, calling ActiveCamera::queue_request returns the error Os { code: -22, kind: Uncategorized, message: "Unknown error -22" } if called with a request that has no buffers. libcamera itself does at least print out [10:17:35.531738771] [100656] ERROR Camera camera.cpp:1136 Request contains no buffers to the shell. I might have a look to see if I can handle that better...

Yeah, I think we should map all instances of io::Result to a rust error enum

[kbingham]

Having just come across this - indeed - if you've hit a panic/assert in libcamera - please report it to us. That shouldn't happen. The assertions mean something is wrong. And applications shouldn't be able to trigger assertions - they should be to prevent us making development mistakes.

Of course the states are there to ensure that applications can only make valid calls when the internal state is correct too.

All negative error numbers from libcamera are errno values.

$ errno 22
EINVAL 22 Invalid argument