Add controls ov5640 #76144

ngphibang · 2024-07-21T22:28:56Z

This PR adds to the ov5640 camera driver

some controls (hue, saturation, etc.)
support for changing frame rate which depends on this PR which add frame rate changing APIs

decsny · 2024-07-22T17:17:16Z

drivers/video/ov5640.c

maybe these features should be optional? maybe not every platform can afford to link in math?

I saw many drivers including math.h without constraints, can you please explain why it might be necessary?

dleach02 · 2024-08-14T19:37:44Z

@ngphibang can you resolve the CI errors?

dleach02 · 2024-08-20T03:04:54Z

@ngphibang can you address the CI failures

ngphibang · 2024-08-20T09:25:02Z

@dleach02 CI failures are due to the dependency on this PR which is not merged yet

dleach02 · 2024-08-22T01:40:53Z

@dleach02 CI failures are due to the dependency on this PR which is not merged yet

Okay, looks like you have a comment 10 hrs ago then in the other PR from the assignee. Please work with the assignee to get their approval.

trunghieulenxp · 2024-09-11T08:44:19Z

Rebase because the PR #76393 is merged

josuah

Grateful for this addition! OV5640 is a complex sensor with a lot of features.

Just waiting the dependency PR to get validated before it: the comments below are informational.

drivers/video/ov5640.c

include/zephyr/drivers/video-controls.h

josuah

Adding "DNM" to allow starting the review now and remove it once dependencies are met.

dleach02 · 2024-10-01T09:19:23Z

@loicpoulain @josuah, Can I get your reviews on this PR so we can move it along

loicpoulain

@ngphibang you should check out-of-bound values in set_ctrl... as nothing is preventing bad values.

josuah

I missed the rebase from @trunghieulenxp. Thank you for reminding us.
I also did not notice that controls could be done out of bounds.
This could be interesting to introduce in a generic way (i.e. in another PR) for all sensors, once the MIN/MAX can be communicated from the driver to the API.

loicpoulain · 2024-10-01T14:13:52Z

@josuah : https://github.com/zephyrproject-rtos/zephyr/blob/84f22d8a2dcce38a71eec029818fe8113b876161/drivers/video/ov5640.c#L604 ?

ngphibang · 2024-10-01T14:20:19Z

@loicpoulain @josuah : Yes, the ctrl value should be checked. The bigger problem is all control values in the API is defined as (void *) which causes some difficulties for drivers and applications. I am working on defining control types for each CID, will try to find time to finalize and push the PR soon.

josuah · 2024-10-01T15:07:33Z

@loicpoulain: you are right, I gave my +1 too early. That was counting on this modification to be made but I should be patient.

Run clang format before making any changes. Signed-off-by: Phi Bang Nguyen <[email protected]>

Add some control IDs: - VIDEO_CID_CAMERA_HUE - VIDEO_CID_POWER_LINE_FREQUENCY - VIDEO_CID_PIXEL_RATE which is needed for changing frame rate Signed-off-by: Phi Bang Nguyen <[email protected]>

Color bar is one type of test patterns. Rename it to VIDEO_CID_CAMERA_TEST_PATTERN to be more generic. Signed-off-by: Phi Bang Nguyen <[email protected]>

Add some minor fixes and optimizations: - Fix coding style - Rename some variables - Use the array variable size directly instead of defining a macro Signed-off-by: Phi Bang Nguyen <[email protected]> Signed-off-by: Trung Hieu Le <[email protected]>

Add support for 4 test pattern modes: - Color bar - Color bar rolling - Square - Square rolling Signed-off-by: Phi Bang Nguyen <[email protected]>

Add some controls: - hue - saturation - brightness - contrast - gain - hflip - vflip - power line frequency filter - pixel rate (read-only) which is needed for changing frame rate Signed-off-by: Farah Fliss <[email protected]> Signed-off-by: Phi Bang Nguyen <[email protected]>

Add support for changing frame rate Signed-off-by: Trung Hieu Le <[email protected]> Signed-off-by: Phi Bang Nguyen <[email protected]>

josuah

I thought having the variable initialization at the top was a requirement in Linux, but I might have been mixing with KNF.

It is apparently used in the kernel too.

Very glad to learn about IN_RANGE()!

drivers/video/ov5640.c

ngphibang · 2024-10-03T15:47:57Z

Hi @loicpoulain , could you revisit this ?
Thanks

ngphibang requested review from dleach02 and mmahadevan108 as code owners July 21, 2024 22:28

zephyrbot added area: Video Video subsystem platform: NXP Drivers NXP Semiconductors, drivers labels Jul 21, 2024

zephyrbot requested review from danieldegrasse, dbaluta, decsny, loicpoulain and manuargue July 21, 2024 22:29

zephyrbot assigned dleach02 Jul 21, 2024

josuah mentioned this pull request Jul 22, 2024

Add parallel interface (DVP) support for ov5640 driver #76124

Merged

decsny reviewed Jul 22, 2024

View reviewed changes

ngphibang force-pushed the add_controls_ov5640 branch 5 times, most recently from 18a90b2 to bd3f125 Compare July 24, 2024 16:03

josuah mentioned this pull request Jul 27, 2024

drivers: video: image sensor list (from Linux, others) #76378

Open

ngphibang mentioned this pull request Jul 30, 2024

Improve NXP CSI and MIPI_CSI2Rx drivers #76475

Merged

ngphibang mentioned this pull request Aug 20, 2024

Video : Add frame interval APIs #72254

Merged

trunghieulenxp force-pushed the add_controls_ov5640 branch from bd3f125 to 65eb6a8 Compare September 11, 2024 08:24

josuah reviewed Sep 16, 2024

View reviewed changes

include/zephyr/drivers/video-controls.h Outdated Show resolved Hide resolved

josuah added the DNM This PR should not be merged (Do Not Merge) label Sep 16, 2024

josuah previously approved these changes Sep 16, 2024

View reviewed changes

josuah removed the DNM This PR should not be merged (Do Not Merge) label Sep 24, 2024

trunghieulenxp dismissed josuah’s stale review via 0289426 September 25, 2024 12:27

trunghieulenxp force-pushed the add_controls_ov5640 branch from 65eb6a8 to 0289426 Compare September 25, 2024 12:27

loicpoulain reviewed Oct 1, 2024

View reviewed changes

josuah previously approved these changes Oct 1, 2024

View reviewed changes

josuah self-requested a review October 1, 2024 15:05

ngphibang and others added 7 commits October 2, 2024 14:47

drivers: video-controls: Run clang-format

0df9aba

Run clang format before making any changes. Signed-off-by: Phi Bang Nguyen <[email protected]>

drivers: video-controls: Add some control IDs

a2dff86

Add some control IDs: - VIDEO_CID_CAMERA_HUE - VIDEO_CID_POWER_LINE_FREQUENCY - VIDEO_CID_PIXEL_RATE which is needed for changing frame rate Signed-off-by: Phi Bang Nguyen <[email protected]>

drivers: video-controls: Rename colorbar to test pattern

2287ad3

Color bar is one type of test patterns. Rename it to VIDEO_CID_CAMERA_TEST_PATTERN to be more generic. Signed-off-by: Phi Bang Nguyen <[email protected]>

drivers: video: ov5640: Add support for test patterns

8919a0b

Add support for 4 test pattern modes: - Color bar - Color bar rolling - Square - Square rolling Signed-off-by: Phi Bang Nguyen <[email protected]>

drivers: video: ov5640: Add support for changing frame rate

721ab6b

Add support for changing frame rate Signed-off-by: Trung Hieu Le <[email protected]> Signed-off-by: Phi Bang Nguyen <[email protected]>

trunghieulenxp dismissed josuah’s stale review via 721ab6b October 3, 2024 14:00

trunghieulenxp force-pushed the add_controls_ov5640 branch from 0289426 to 721ab6b Compare October 3, 2024 14:00

josuah approved these changes Oct 3, 2024

View reviewed changes

drivers/video/ov5640.c Show resolved Hide resolved

ngphibang requested a review from loicpoulain October 3, 2024 15:44

dleach02 approved these changes Oct 9, 2024

View reviewed changes

carlescufi merged commit fc90c9f into zephyrproject-rtos:main Oct 10, 2024
23 checks passed

ngphibang deleted the add_controls_ov5640 branch October 10, 2024 08:19

This was referenced Oct 10, 2024

Remove video control include #79595

Merged

Enhance camera fixture test #79337

Merged

drivers: video: controls: rename all CIDs to Linux counterpart #78802

Merged

josuah mentioned this pull request Nov 5, 2024

doc: release: 4.0: Add video driver and video API updates #80945

Merged

Add controls ov5640 #76144

Add controls ov5640 #76144

Uh oh!

Conversation

ngphibang commented Jul 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

decsny Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

0xFarahFl Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

dleach02 commented Aug 14, 2024

Uh oh!

dleach02 commented Aug 20, 2024

Uh oh!

ngphibang commented Aug 20, 2024

Uh oh!

dleach02 commented Aug 22, 2024

Uh oh!

trunghieulenxp commented Sep 11, 2024

Uh oh!

josuah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josuah left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dleach02 commented Oct 1, 2024

Uh oh!

loicpoulain left a comment

Choose a reason for hiding this comment

Uh oh!

josuah left a comment

Choose a reason for hiding this comment

Uh oh!

loicpoulain commented Oct 1, 2024

Uh oh!

ngphibang commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josuah commented Oct 1, 2024

Uh oh!

josuah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ngphibang commented Oct 3, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ngphibang commented Jul 21, 2024 •

edited

Loading

josuah left a comment •

edited

Loading

ngphibang commented Oct 1, 2024 •

edited

Loading