-
Notifications
You must be signed in to change notification settings - Fork 6.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
drivers: video: Add support for arducam mega #66994
base: main
Are you sure you want to change the base?
Conversation
Hello @ArduCAM, and thank you very much for your first pull request to the Zephyr project! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial impressions are good :)
|
||
#define DT_DRV_COMPAT arducam_mega | ||
|
||
#include <zephyr/drivers/video/arducam_mega.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need an header file? as far as I see everything can be in the C file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@loicpoulain
Thank you for your detail check.
Due to our arducam mega driver need some specific controls and other parameters and we need provide them to user.
We have not see them in the video.h file. So we add a arducam_mega.h file. If not, users can't control our mega's full features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but look like some of them, like exposure, brightness, etc, are quite common for camera, so it would be worth to add new generic CId for them, instead of having custom ones for each vendor. For sure that new control values should be in known device-agnostic unit like us
or %
..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@loicpoulain
Hello, thanks for your fantastic sugguestions. In fact, in the video-controls.h file there are exist some common camera controls, and we have use it,we can add some other common controls(such as exposure mode) to video-controls.h and send you a new PR.
But,we have some specific control just used for arducam mega. so we can't make sure all controls are common.
Do you think so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's fine, you can also have private controls. But it makes sense to make the generic ones common.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@loicpoulain
OK, I have modified it.
Hello, could you please rebase and squash. |
8ba42f5
to
8775fb0
Compare
@loicpoulain |
@galak |
You still need to use the Apache 2.0 license, and you have a few CI issues to fix before this can be merged :) |
10e5c85
to
eed8f09
Compare
@bjarki-trackunit |
1484bea
to
145fb29
Compare
dismissing my review as comments to date re: typos etc have been addressed but I lack the domain knowledge to provide an actual +1
05f138a
to
d847472
Compare
#define VIDEO_CTRL_CLASS_CAMERA 0x00010000 /**< Camera class controls */ | ||
#define VIDEO_CTRL_CLASS_MPEG 0x00020000 /**< MPEG-compression controls */ | ||
#define VIDEO_CTRL_CLASS_JPEG 0x00030000 /**< JPEG-compression controls */ | ||
#define VIDEO_CTRL_CLASS_VENDOR 0xFFFF0000 /**< Vendor-specific class controls */ | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No functional change above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move generic video changes to dedicated commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@loicpoulain OK, I have send a new PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comments have been addressed :)
d847472
to
b44ed2f
Compare
@ArduCAM just curious if you'd be interested in contributing a code sample as a follow-up pull request? It would be really great to have a sample combining video and display APIs to actually display captured images on an LCD display. A sample saving captured image to file system would also be nice :) Thanks again for this PR, and thanks in advance if you happen to be interested in adding code samples :) |
@ArduCAM looks quite ok, I understand you have a custom sample to show off all the camera features. But could you confirm or test that the driver also works with the simple video |
Thanks for your reply. I will test it and reply you later. |
@loicpoulain |
Add some common camera's control: VIDEO_CID_CAMERA_EXPOSURE_AUTO, VIDEO_CID_CAMERA_GAIN_AUTO, VIDEO_CID_CAMERA_WHITE_BAL_AUTO, VIDEO_CID_JPEG_COMPRESSION_QUALITY Signed-off-by: Lee Jackson <[email protected]>
Adding frame length and fragmented frame status to indicate certain information about the frame when the video buffer is smaller than the frame length. Signed-off-by: Lee Jackson <[email protected]>
681bf99
to
779062f
Compare
@loicpoulain Have tested the capture using the samples/subsys/video/capture code. and do some necessary modification in order to fit mega spi camera.
|
The Arducam mega is an low power, rolling shutter camera, support connect one or more cameras any Microcontroller. It provides high-quality image capture and processing capabilities, making it highly suitable for various application fields, including machine vision, image recognition, and robotics, among others. Signed-off-by: Lee Jackson <[email protected]>
This example is a full-featured sample for the Arducam Mega camera driver. By using serial communication with a PC software, it can capture images, display them, and also control camera settings such as exposure and gain. Signed-off-by: Lee Jackson <[email protected]>
779062f
to
60de7c4
Compare
Looking at this API, I'm unclear how the consumer determines what size of framebuffer they should provide to the video device. In the case of the arducam, is the size essentially arbitrary? IE the application could provided 10 1KiB byte buffers, or 1 10KiB buffer, and the camera would work either way? The other use case I'd like to try to cover with a "partial frame" API is one where the camera driver "slices" the frame into segments. For example, a 320x240 frame might be divided into 8 320x30 pixel buffers, which would allow a low memory device to stream QVGA frames in chunks. The issue here is how to tell the application what size of framebuffer is required, since in this case the size of each framebuffer is fixed. In #72827, the approach I used was to provide a @loicpoulain do you have any thoughts here? Maybe we add a special value to |
Yes,For arducam spi camera, the the size of each segments is flexible. the camera have a hardware frambuffer, so read image data is very flexible. it support read one by one or block read. Driver gets each frame size and send to user space through multi segments, due to some platform's memory limitation, each segments is flexible, even one byte. For compressed data such as jpeg format, the frame size is flexible and not stable. so for the last vbuf,it will contains some supplementary data, if the driver can accept variably sized video buffers, i think it‘s great. |
@loicpoulain and @ArduCAM, I've updated #72827 with commits to demonstrate how I think we might merge these two methods of handling partial video frames. Essentially, a driver would set the I'm interested to hear any feedback on this approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding Zephyr support for this camera!
ret |= arducam_mega_await_bus_idle(&cfg->bus, 3); | ||
ret |= arducam_mega_write_reg(&cfg->bus, CAM_REG_MANUAL_GAIN_BIT_9_8, (value >> 8) & 0xff); | ||
ret |= arducam_mega_await_bus_idle(&cfg->bus, 10); | ||
ret |= arducam_mega_write_reg(&cfg->bus, CAM_REG_MANUAL_GAIN_BIT_7_0, value & 0xff); | ||
ret |= arducam_mega_await_bus_idle(&cfg->bus, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am personally a fan with this compact style of error checking, although in Zephyr, the return values are negatives of values from errno.h
. This means that OR-ing them would scramble the value, and eventually give a bad error code.
The best would be to convert it to something like this this, even though this takes-up 4 lines instead of 1:
ret = arducam_mega_await_bus_idle(&cfg->bus, 3);
if (ret < 0) {
return ret;
}
#define RESET_CAMERA 0XFF | ||
#define SET_PICTURE_RESOLUTION 0X01 | ||
#define SET_VIDEO_RESOLUTION 0X02 | ||
#define SET_BRIGHTNESS 0X03 | ||
#define SET_CONTRAST 0X04 | ||
#define SET_SATURATION 0X05 | ||
#define SET_EV 0X06 | ||
#define SET_WHITEBALANCE 0X07 | ||
#define SET_SPECIAL_EFFECTS 0X08 | ||
#define SET_FOCUS_ENABLE 0X09 | ||
#define SET_EXPOSURE_GAIN_ENABLE 0X0A | ||
#define SET_WHITE_BALANCE_ENABLE 0X0C | ||
#define SET_MANUAL_GAIN 0X0D | ||
#define SET_MANUAL_EXPOSURE 0X0E | ||
#define GET_CAMERA_INFO 0X0F | ||
#define TAKE_PICTURE 0X10 | ||
#define SET_SHARPNESS 0X11 | ||
#define DEBUG_WRITE_REGISTER 0X12 | ||
#define STOP_STREAM 0X21 | ||
#define GET_FRM_VER_INFO 0X30 | ||
#define GET_SDK_VER_INFO 0X40 | ||
#define SET_IMAGE_QUALITY 0X50 | ||
#define SET_LOWPOWER_MODE 0X60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting implementation, but looks too complex for a sample: expected to be the simplest possible way to implement a driver.
This gives the impression that the Ardcuam driver does not work with the generic Zephyr API, which is false as you properly integrated everything quite nicely!
How about trying to get it to work with the existing sample here?
https://github.com/zephyrproject-rtos/zephyr/tree/main/samples/subsys/video/capture
This could also help promoting this camera by showing that no extra application needs to be written to control the Arucam.
This would also allow shrinking the amount of code to review as part of this PR which is in review since too long already (sorry about this!).
{ | ||
while ((arducam_mega_read_reg(spec, CAM_REG_SENSOR_STATE) & 0x03) != SENSOR_STATE_IDLE) { | ||
if (tries-- == 0) { | ||
return -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Often Zephyr uses -errno
as return value as a hint of where the error comes from.
Maybe here it is return -EAGAIN
as it is a timeout? Or maybe return -EIO;
for it is related to communication...
|
||
ret |= arducam_mega_write_reg(&cfg->bus, CAM_REG_SHARPNESS_CONTROL, level); | ||
|
||
if (ret == -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the -errno
being returned for errors, I suppose this would need to be converted to ret < 0
drv_data->info = &mega_infos[0]; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like missing a case
, or might need to be removed.
It sounds like it is possible to merge this PR without introducing the extra capability immediately:
But it looks like Arducam does not need any
|
enum video_frame_fragmented_status { | ||
VIDEO_BUF_FRAG, | ||
VIDEO_BUF_EOF, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enum video_frame_fragmented_status { | |
VIDEO_BUF_FRAG, | |
VIDEO_BUF_EOF, | |
}; | |
#define VIDEO_BUF_FRAG BIT(0) | |
#define VIDEO_BUF_EOF BIT(1) |
I think this is what @loicpoulain suggested here.
Video buffers did not precise whether they contained a whole frame, or a fragment of a frame. Adding a flag struct as well as a total frame field allows video devices to tell whether they produced - a complete frame (with VIDEO_BUF_EOF and no VIDEO_BUF_FRAG), - a partial frame (with VIDEO_BUF_FRAG), - a last frame (with VIDEO_BUF_EOF), The video devices that do not support fragmentation currently ignore this flag. As discussed in zephyrproject-rtos#66994 and zephyrproject-rtos#72827. Co-authored-by: Lee <[email protected]> Co-authored-by: Daniel DeGrasse <[email protected]> Signed-off-by: Josuah Demangeon <[email protected]>
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
@ArduCAM -- I know this has been open for a really long time, but as a lot has happened around video recently, I feel like addressing the remaining comments should help us get this really close to be mergeable. Thanks! |
The Arducam mega is an low power, rolling shutter camera, support connect one or more cameras any Microcontroller. It provides high-quality image capture and processing capabilities, making it highly suitable for various application fields, including machine vision, image recognition, and robotics, among others.
This commit adds arducam mega driver and bindings file.