Initial CAM implementation
Hello all, wanted to start this PR as a basis of the initial CAM implementation. Before adding doc comments I wanted to get eyes on the simple wrapper functions. The only function not implemented is CAMU_GetSuitableY2rStandardCoefficient.
I want to use this PR sort of as a starting point of helping out the project and learning the preferred design and style for further contributions.
Changes in other files are a result of cargo fmt,
I think the API looks okay, but we could also consider something like we did for Screen, which is to have a trait/struct that implements common functionality and can be obtained from the Cam service, logically representing a single camera (or maybe also multiple?). A rough sketch of what I'm thinking:
let cam = Cam::init().unwrap():
let mut rear_cams = cam.both_outward();
rear_cams.set_size(CamSize::CTR_TOP_LCD).unwrap();
rear_cams.set_output_format(CamOutputFormat::RGB_565).unwrap();
cam.activate(rear_cams).unwrap();
cam.start_capture(CamPort::BOTH).unwrap();
// etc...
I haven't look in great detail at the libctru C API, so maybe this doesn't make a ton of sense, but I think abstracting over the hardware like this to prevent accidentally crossing wires or mutating the wrong device state can be helpful, so just a suggestion. Feel free to ignore if you think it doesn't make sense or introduces too much complexity to the service (or, it could be implemented in another PR too).
let cam = Cam::init().unwrap(): let mut rear_cams = cam.both_outward(); rear_cams.set_size(CamSize::CTR_TOP_LCD).unwrap(); rear_cams.set_output_format(CamOutputFormat::RGB_565).unwrap(); cam.activate(rear_cams).unwrap(); cam.start_capture(CamPort::BOTH).unwrap(); // etc...
Something like this is what I was thinking as well in the comment I mentioned earlier. We could change each cam variant to a struct with their own implementation of a Camera trait.
The three singular cameras could use the default implementation and then the two outer cameras together can have its own. This would also reduce all the enums visible to the user to just the different camera settings.
Also something to consider is using something like num_enum for non-bitflag enums. I think it would help a lot with removing all the From implementation boilerplate. I didn't want to add this without asking though because it's another dependency to tack on
Working on an idea of this and was curious, is there a reason the screens are wrapped in a RefCell within the gfx module?
Working on an idea of this and was curious, is there a reason the screens are wrapped in a
RefCellwithin thegfxmodule?
There's some context here: https://github.com/Meziu/ctru-rs/pull/22
(I haven't caught up on this PR yet, but will take a look soon)
Pushed an initial version of this to my local. Didn't want to commit to the current PR just in case. The initial idea can be found here. I can also open a separate PR for that branch if necessary.
Also something to consider is using something like
num_enumfor non-bitflag enums. I think it would help a lot with removing all theFromimplementation boilerplate. I didn't want to add this without asking though because it's another dependency to tack on
I'm ok with this, it looks like it will cut down on the boilerplate by a lot.
I decided to merge the structs branch into this one as it seems it would be a favored starting point to refactor what was originally there. I think it's a better idea to wrap the hardware with structs as @ian-h-chamberlain mentioned.
There are definitely changes to be made for what functions to safely expose to the user and I have a way to do that but I want to ask first what everyone believes should be safe to provide. Currently I believe the only things that should be exported are functions related to camera settings (this excludes Handle results and other svc related functions such as vsync timings and synching handles).
We should still expose some way of waiting for the photo to be taken. The details of how that waiting happens can be hidden behind a simple function, like @Meziu suggested.
We should still expose some way of waiting for the photo to be taken. The details of how that waiting happens can be hidden behind a simple function, like @Meziu suggested.
In the current version, take_picture starts and waits for the image under the hood. The returned result contains the image bytes.
Other than documentation, I don’t know if there is anything else to check. @ian-h-chamberlain @AzureMarker could you re-review it?
I have a final that I'm studying for to take tomorrow morning. Afterwards I can work on documentation.
LGTM
Apologies for the delay, I finally had the time today to add probably the most basic comments.