Add Arducam mega camera driver#96234
Add Arducam mega camera driver#96234kristosb wants to merge 3 commits intozephyrproject-rtos:mainfrom
Conversation
|
Hello @kristosb, and thank you very much for your first pull request to the Zephyr project! |
kristosb
force-pushed
the
update_arducam_mega_driver
branch
from
5606ed3 to
5fa321b
Compare
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
Sorry for such a long silence.
Here is another round of review to react to some of your comment as well as follow-up modifications to reduce the custom APIs that this driver requires.
Thank you for your previous changes!
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
Here is some other things I noticed that Arducam left along the way.
I wish I did catch these few extra things on previous review.
|
Thank you for all the changes @kristosb! Note that there were a few PRs merged and you might want to |
Thanks for all the feedback and suggestions. There is still some work required. I rebased not long ago but will do that again soon. |
|
There has been a hurricane of PR merges just before the 4.3.0 release RC1, probably why so many conflicts in such short time. https://github.com/zephyrproject-rtos/zephyr/wiki/Release-Management No rush of course! |
kristosb
force-pushed
the
update_arducam_mega_driver
branch
from
f63f0ea to
d7ab999
Compare
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
Thank you for these changes!
This time, some focus on error handling to make a "try/catch" replacement in C appear, to help proof reading driver code (which almost always look the same).
I have annotated them all so that you have some kind of to-do list.
Last but not least, we can reorganize the commits into just a few commits.
You can "squash" all the commits into just 3 (or more if you want to segment more):
vendor-prefixes.txtchangesvideo-controls.hchanges- everything else in just one commit
Past this, I think it would be ready for being upstreamed!
Thanks again for this long refactoring!
|
This looks like one of the most complete/featureful camera driver of Zephyr so far. Congrats. :) |
zephyrbot
requested review from
avolmat-st,
decsny,
loicpoulain,
mbolivar,
ngphibang and
rruuaanng
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
Thanks for all these changes, that was a large batch of small things.
So here is a small batch for one last thing and I think this would be ready. :)
| struct video_buffer *vbuf; | ||
| int ret = 0; | ||
|
|
||
| vbuf = k_fifo_get(&drv_data->fifo_in, K_NO_WAIT); |
There was a problem hiding this comment.
Reminder to add error handling in case vbuf == NULL.
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
I did spot a few more details, but I think we are almost there!
kristosb
force-pushed
the
update_arducam_mega_driver
branch
from
1a77fe1 to
f1a979f
Compare
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
I was starting a review, and realized this was on an older commit.
This is a good example why it's helping to not have "fixup commits" in a same patch: commits that modify issues introduced by the previous other commits.
It seems like what reviewers look at is the "File changed" tab, but this is the biggest fallacy of Github ever, as reviewers are expected to exclusively review the "Commits" tab.
If you switch to the Commits tab, and see code that is not looking ideal (even though it looks ideal on the "files changed" tab), then likely this will not be good for review...
So this is fine to drop the commits from arducam. You can eventually add the original authors as Co-authored-by: User Name <email> in a line after/before Signed-off-by:.
Keeping the original comments below to outline the issues, but they don't need to be fixed I think as you already did all the fixups in latter commits (which you can then merge with the original commits).
Thank you! Getting really close!
Made the changes for completeness. |
kristosb
force-pushed
the
update_arducam_mega_driver
branch
from
98a6f76 to
9a9c8ad
Compare
I squashed the commits again and included original co-author in a commit message. |
kristosb
force-pushed
the
update_arducam_mega_driver
branch
from
9a9c8ad to
175eb26
Compare
There was a missing first commit and wrong file under commit for prefixes. Should be fine now. I will rebase now to sync with upstream. |
kristosb
force-pushed
the
update_arducam_mega_driver
branch
from
175eb26 to
9c0ccee
Compare
|
Branch rebased and in sync with upstream now. |
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
I think we enter in the polishing phase now really.
Only the commit message, order of commits are mandatory, and the others are more a recommendation and less blocking.
Thank you again for this very long fine-tuning session!
kristosb
force-pushed
the
update_arducam_mega_driver
branch
from
5cd2705 to
0fe7102
Compare
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
Thank you for the changes! I think we are at the last mile now.
There was a problem hiding this comment.
The driver needs to be at least built in CI, see tests/drivers/build_all/video.
Also, the last commit's message is not that useful, it's just an advertisement of the hardware. What could be useful is pointing out for ex. any known limitations of the current driver, justifying design choices... in general info that will be helpful for someone debugging the driver or trying to improve it.
kristosb
force-pushed
the
update_arducam_mega_driver
branch
from
0fe7102 to
2079bdd
Compare
|
drivers/video/video_arducam_mega.c
Outdated
| static const struct arducam_mega_config arducam_mega_cfg_##inst = { \ | ||
| .bus = SPI_DT_SPEC_INST_GET(inst, \ | ||
| SPI_OP_MODE_MASTER | SPI_WORD_SET(8) | \ | ||
| SPI_CS_ACTIVE_HIGH | SPI_LINES_SINGLE | \ |
There was a problem hiding this comment.
I'm questioning this SPI_CS_ACTIVE_HIGH here. The camera datasheet shows that the SS line to the camera is active low, as is typical for SPI devices.
I've been working with this camera for the last couple of days on a platform that does not support active high CS lines (and returns an error when you try to use it that way), so I had to remove this setting. Seems like it would make more sense to be in the device tree if this was needed on a particular platform.
There was a problem hiding this comment.
Thanks for the report, and sorry for the long wait.
Could you give a hint about which platform that was if it is public information?
kristosb
force-pushed
the
update_arducam_mega_driver
branch
from
2079bdd to
e260a90
Compare
zephyrbot
added
the
area: Devicetree Binding
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
Thank you for coming back to this driver!
There does not seem to be a lot of changes in the video API since last time luckily, so very few adjustments (which I missed) but otherwise I see it ready for being merged!
kristosb
force-pushed
the
update_arducam_mega_driver
branch
from
e260a90 to
bf46e5e
Compare
Added automatic exposure compensation control under base camera class. Signed-off-by: Krystian Balicki <kristos_b@wp.pl>
Added arducam company name to vendor name list. Signed-off-by: Krystian Balicki <kristos_b@wp.pl>
The Arducam mega is a low power, rolling shutter camera, supports connecting one or more cameras to 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. In current implementation connecting multiple instances of the same camera system is problematic. Co-authored-by: Lee Jackson <lee.jackson@arducam.com> Signed-off-by: Krystian Balicki <kristos_b@wp.pl>
kristosb
force-pushed
the
update_arducam_mega_driver
branch
from
bf46e5e to
d854cd4
Compare
|
6 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.