Add Arducam mega camera driver #96234
Conversation
|
Hello @kristosb, and thank you very much for your first pull request to the Zephyr project! |
| #endif | ||
|
|
||
| /* Arducam specific camera controls */ | ||
| #define VIDEO_CID_ARDUCAM_EV (VIDEO_CID_PRIVATE_BASE + 1) |
There was a problem hiding this comment.
We can introduce the standard control V4L2_CID_AUTO_EXPOSURE_BIAS from Linux (for both the description and the CID number) inside video-controls.h
https://docs.kernel.org/userspace-api/media/v4l/ext-ctrls-camera.html
| #define VIDEO_CID_ARDUCAM_SUPP_RES (VIDEO_CID_PRIVATE_BASE + 7) | ||
| #define VIDEO_CID_ARDUCAM_SUPP_SP_EFF (VIDEO_CID_PRIVATE_BASE + 8) | ||
| #define VIDEO_CID_ARDUCAM_EN_FOCUS (VIDEO_CID_PRIVATE_BASE + 9) | ||
| #define VIDEO_CID_ARDUCAM_EN_SHARPNESS (VIDEO_CID_PRIVATE_BASE + 10) |
There was a problem hiding this comment.
It seems like VIDEO_CID_SHARPNESS would be a good match.
There was a problem hiding this comment.
On Arducam there is Adjust sharpness register (0x28) which I mapped to video driver VIDEO_CID_SHARPNESS control and goes up to max 8. Then there is internally stored information (right now in the driver) weather sharpness is available on the device and is mapped to custom VIDEO_CID_ARDUCAM_EN_SHARPNESS control (this control is read only). We could drop the VIDEO_CID_ARDUCAM_EN_SHARPNESS since the information is used in the driver on an attempt to access VIDEO_CID_SHARPNESS and throw an errror log if its not supported?
https://blog.arducam.com/downloads/datasheet/Arducam_MEGA_SPI_Camera_Application_Note.pdf
There was a problem hiding this comment.
Removed VIDEO_CID_ARDUCAM_EN_SHARPNESS as information on availability of sharpness control should be known by application.
There was a problem hiding this comment.
This makes sense, I understand better now that you explained it.
When sharpness is supported, the sharpness control is added.
When sharpness is not supported, the sharpness control is not added.
This way, there is indeed no need for _EN_SHARPNESS.
There was a problem hiding this comment.
This is now solved with resource flags.
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]>
… driver changes Driver files updated to synchronize with changes upstream. Removed any non generic private controls from sample code. Signed-off-by: Krystian Balicki <[email protected]>
Partial expression handling is not accepted. The controlling expression of an if statement shall be boolean type. Signed-off-by: Krystian Balicki <[email protected]>
Buffer count is by default set to one. For capture mode only one buffer is needed. Minimum and maximum line count are set to minus one which means camera provides full frame. Signed-off-by: Krystian Balicki <[email protected]>
Removed remaining partial exception handling. Wrapped wait and write functions in one with appropriate exception handling. Signed-off-by: Krystian Balicki <[email protected]>
Replaced vendor color fx CID with existing one from video driver. Mapped vendor fx values to video fx values. Signed-off-by: Krystian Balicki <[email protected]>
Linted whitespaces to conform to zephyr guidelines. Signed-off-by: Krystian Balicki <[email protected]>
Added missing return on error in special effects function. Signed-off-by: Krystian Balicki <[email protected]>
Changed all enum names to lowercase underscore delimited and removed unnecessary typedef. Signed-off-by: Krystian Balicki <[email protected]>
Removed camera address and ID from controls as they are internal details. Signed-off-by: Krystian Balicki <[email protected]>
Added missing private functions prefixes. Signed-off-by: Krystian Balicki <[email protected]>
Rebased with upstream main and updated latest video driver changes. Signed-off-by: Krystian Balicki <[email protected]>
Arducam sample code using uart is not necessary since other samples should work with the driver. It has been removed. Signed-off-by: Krystian Balicki <[email protected]>
Fixed MISRA issues regarding commenting style, unused variables and duplicated switch case. Signed-off-by: Krystian Balicki <[email protected]>
Added video prefix to driver file names reflecting the fact that the device is a video receiver. Signed-off-by: Krystian Balicki <[email protected]>
Removed version information read from camera from cached driver data. Signed-off-by: Krystian Balicki <[email protected]>
…n name Changed name of the function initializing controls to avoid confusion. Signed-off-by: Krystian Balicki <[email protected]>
…ition Fx control was already changed to standard definition but old specific one was not removed. Signed-off-by: Krystian Balicki <[email protected]>
Replaced custom focus control definition with standard VIDEO_CID_FOCUS_AUTO. Added missing auto focus setting function. Signed-off-by: Krystian Balicki <[email protected]>
Removed label argument from register write function. Having register address in the error logs is enough information. Signed-off-by: Krystian Balicki <[email protected]>
Removed unused custom controls definitions. Signed-off-by: Krystian Balicki <[email protected]>
Add Zephyr Project Contributors to license header. Signed-off-by: Krystian Balicki <[email protected]>
kristosb
force-pushed
the
update_arducam_mega_driver
branch
from
5606ed3 to
5fa321b
Compare
drivers/video/video_arducam_mega.c
Outdated
| struct video_ctrl lowpower; | ||
| struct video_ctrl whitebalauto; | ||
| struct { | ||
| struct video_ctrl enable_sharpness; |
There was a problem hiding this comment.
You may remove this control completely.
Auto-sharpness is also supported but is a different feature, and can be added through another PR to help merging this one faster.
There was a problem hiding this comment.
Removed enable_sharpness and replaced with resource flag. Control is initialized based on the flag value. Application will throw an error on corresponding register access if not enabled.
drivers/video/video_arducam_mega.c
Outdated
| struct video_ctrl gain_auto; | ||
| }; | ||
| struct { | ||
| struct video_ctrl enable_focus; |
There was a problem hiding this comment.
It is possible to remove this control. As seen below, it will be possible to selectively exposes or not the focus control depending on whether it is supported on the camera plugged.
There was a problem hiding this comment.
Removed enable_focus and replaced with resource flag. Control is initialized based on the flag value. Application will throw an error on corresponding register access if not enabled.
drivers/video/video_arducam_mega.c
Outdated
| ret = video_init_ctrl(&ctrls->sharpness, dev, VIDEO_CID_SHARPNESS, | ||
| (struct video_ctrl_range){.min = 0, .max = 8, .step = 1, .def = 0}); | ||
| if (ret < 0) { | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
As discussed above, this control can be enabled only when it is supported.
That way, the application will only see controls that can be used.
Thanks for raising this, I did not understood it at first.
There was a problem hiding this comment.
One way to implement this, for instance, is to have a arducam_mega_init_controls(dev, &features); that is filled from arducam_mega_check_connection(dev, &features);, and then if (features & ARDUCAM_MEGA_HAS_SHARPNESS) { vidoe_init_ctrl(...sharpness...); }, for instance.
Or any software strategy that works. Would you like to go this extra mile?
There was a problem hiding this comment.
Removed enable_sharpness and enable_focus and replaced with resource flags. Control is initialized based on the flag value. Application will throw an error on corresponding register access if not enabled.
| MEGA_RESOLUTION_14 = 0x0f, /**<Reserve*/ | ||
| MEGA_RESOLUTION_15 = 0x10, /**<Reserve*/ | ||
| MEGA_RESOLUTION_NONE, | ||
| }; |
There was a problem hiding this comment.
Thanks to your previous integration efforts, it is now possible to move all these hardware-specific APIs to video_arducam_mega.c, or even delete them altogether:
-
Macros mapping range values (one-by-one, min/max, low/medium/high) are not needed as Zephyr video controls already provide range information. They might in some case need to be mapped though (for instance -2 => 0x04, +2 => 0x03 in
EV_LEVEL). -
Supported resolutions are given through the video APIs.
After moving them, you can also remove all the /** and @keywords as driver implementation details do not have public-facing documentation.
There was a problem hiding this comment.
Addressed EV not matching with mapping function. Removed documentation tags from device specific defines.
I am not sure what do you mean by saying supported resolutions are given through api. I thought these MEGA_RESOLUTION_.. are arducam specific values. Moved these to source. Should we move all the device specific enum defs into source?
Removed enable type controls and replaced with resource flags. Based on resource flags control is added to the system. Application will throw an error on attempt to use uninitialized controls. Signed-off-by: Krystian Balicki <[email protected]>
| if (!spi_write_dt(spec, &tx_bufs)) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Like in algebra, let's transform this expression to make the typical error handling idiom appear:
| if (!spi_write_dt(spec, &tx_bufs)) { | |
| return 0; | |
| } | |
| ret = spi_write_dt(spec, &tx_bufs) | |
| if (ret < 0) { | |
| return ret; | |
| } |
for (int tries = 3;; tries--) {
ret = spi_write_dt(spec, &tx_bufs)
if (ret < 0 && tries == 0) {
LOG_ERR("failed to write 0x%x to 0x%x", value, reg_addr);
return ret;
}
if (ret == 0) {
break;
}
/* If writing failed wait 5ms before next attempt */
k_msleep(5);
}
return 0;Other suggestions welcome!
My bad I did not notice this at previous review...
| } | ||
| LOG_ERR("failed to read 0x%x register", reg_addr); | ||
|
|
||
| return -1; |
There was a problem hiding this comment.
This needs to be returning the original ret to propagate the error, helpful for debugging or ignoring selectively some errors (i.e. retry on -EAGAIN, or -EIO from the application).
| if (tries-- == 0) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
There is already a number of retries in arducam_mega_read_reg(), if there need a variable number of attempts, maybe it's worth having a tries parameter to this function?
There was a problem hiding this comment.
This mean a lot of places of the code would need to be replaced, but that's probably the way to go for Zephyr code standard, precisely to avoid ambiguous situations like here above...
Very glad if you wish to do the conversion!
|
|
||
| static int arducam_mega_await_bus_idle(const struct spi_dt_spec *spec, uint8_t tries) | ||
| { | ||
| while ((arducam_mega_read_reg(spec, CAM_REG_SENSOR_STATE) & 0x03) != SENSOR_STATE_IDLE) { |
There was a problem hiding this comment.
Given this is also a function that might fail, could be useful to have the error handling idiom appear here too. And for this modifying arducam_mega_read_reg().
For instance, when returning -1, this is 0xffffffff in unsigned... could randomly break if other things are refactored.
| do { | ||
| if (tries-- == 0) { | ||
| LOG_ERR("Capture timeout!"); | ||
| return -1; | ||
| } | ||
| k_msleep(2); |
There was a problem hiding this comment.
Here too, the number of attempts could be a parmeter to arducam_mega_..._reg() functions to remove the need for a loop, simplifying the driver at a few places.
| if (vbuf == NULL) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
If there is K_FOREVER, vbuf is guaranteed to be non-NULL, otherwise the function simply blocks forever. This mean no error checking required here.
| if (vbuf == NULL) { | |
| return; | |
| } |
| vbuf->timestamp = f_timestamp; | ||
| k_fifo_put(&drv_data->fifo_out, vbuf); | ||
|
|
||
| k_yield(); |
There was a problem hiding this comment.
No need for k_yield(), this was probably copy-pasted from a video-sw-generator, and the video-sw-generator removed the call to k_yield() as well.
This would have been needed if the thread was doing cooperative (manual) scheduling.
|
|
||
| *vbuf = k_fifo_get(&data->fifo_out, timeout); | ||
|
|
||
| LOG_DBG("dequeue buffer %p", (*vbuf)->buffer); |
There was a problem hiding this comment.
If (*vbuf) is NULL, (*vbuf)->buffer will cause a fault, and this can be moved below the error checking.
(this is why the error checking idiom is useful, it raises awareness whenever it's not visible)
As alternative, LOG_WRN("") in the if (*vbuf == NULL) if wanting to help debugging...
| f_timestamp = k_uptime_get_32(); | ||
| } | ||
|
|
||
| arducam_mega_fifo_read(drv_data->dev, vbuf); |
There was a problem hiding this comment.
For every function call that can fail, we might want to add a LOG_ERR(); or LOG_WRN(); to it, even if it already prints the error inside the function.
| arducam_mega_fifo_read(drv_data->dev, vbuf); | |
| ret = arducam_mega_fifo_read(drv_data->dev, vbuf); | |
| if (ret < 0) { | |
| LOG_ERR("failed to read a buffer (%d)", ret); | |
| } |
| int ret = arducam_mega_write_reg(&cfg->bus, CAM_REG_SENSOR_RESET, SENSOR_RESET_ENABLE); | ||
| k_msleep(1000); | ||
|
|
||
| return ret; |
There was a problem hiding this comment.
It is possible to use the same error handling idiom as usual here too for failing without waiting 1 whole second in case of error:
int ret;
...
ret = arducam_mega_write_reg(&cfg->bus, CAM_REG_SENSOR_RESET, SENSOR_RESET_ENABLE);
if (ret < 0) {
LOG_ERR("Failed to reset the sensor (%d)", ret);
return ret;
}
k_msleep(1000);
return 0;
| while (fmts[i].pixelformat) { | ||
| if (fmts[i].width_min == width && fmts[i].height_min == height && | ||
| fmts[i].pixelformat == fmt->pixelformat) { | ||
| /* Set output format */ |
There was a problem hiding this comment.
Since this driver was written, there is now an utility that automates this, so no more need for a loop:
| /* Set output format */ | |
| ret = video_format_caps_index(fmts, fmt, &i); | |
| if (ret) { | |
| LOG_ERR("Unsupported pixel format or resolution %s %ux%u", | |
| VIDEO_FOURCC_TO_STR(fmt->pixelformat), fmt->width, fmt->height); | |
| return ret; | |
| } |
| /* We only support RGB565, JPEG, and YUYV pixel formats */ | ||
| if (fmt->pixelformat != VIDEO_PIX_FMT_RGB565 && fmt->pixelformat != VIDEO_PIX_FMT_JPEG && | ||
| fmt->pixelformat != VIDEO_PIX_FMT_YUYV) { | ||
| LOG_ERR("Arducam Mega camera only supports RGB565, JPG, and YUYV pixel " | ||
| "formats!"); | ||
| return -ENOTSUP; | ||
| } |
There was a problem hiding this comment.
By using the video_format_caps_index() function below, this can be skipped altogether.
| /* We only support RGB565, JPEG, and YUYV pixel formats */ | |
| if (fmt->pixelformat != VIDEO_PIX_FMT_RGB565 && fmt->pixelformat != VIDEO_PIX_FMT_JPEG && | |
| fmt->pixelformat != VIDEO_PIX_FMT_YUYV) { | |
| LOG_ERR("Arducam Mega camera only supports RGB565, JPG, and YUYV pixel " | |
| "formats!"); | |
| return -ENOTSUP; | |
| } |
Removed definitions for control limits. Introduced mapping for not matching specific control values in EV. Removed documentation tags from device specific definitions. Signed-off-by: Krystian Balicki <[email protected]>
|
2 participants
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.