Skip to content

boards: shields: add suport for weact_ov2640_cam_module #77949

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 25, 2024
Merged

boards: shields: add suport for weact_ov2640_cam_module #77949

merged 2 commits into from
Oct 25, 2024

Conversation

CharlesDias
Copy link
Contributor

@CharlesDias CharlesDias commented Sep 3, 2024 •
edited
Loading

Add support for OV2640 camera sensor to use with WeAct Studio MiniSTM32H7xx boards.

PR dependency:

@zephyrbot zephyrbot added the area: Shields Shields (add-on boards) label Sep 3, 2024
@CharlesDias CharlesDias mentioned this pull request Sep 3, 2024
Merged
@JarmouniA
Copy link
Collaborator

I think we should define a binding for the connector (similar to dsi_lcd_qsh_030 display connector for example) since it's apparently used by many vendors (#71463 (comment)) and use that binding to specify the pins in ov2640 node, in a board-independent way.

Also, the I2C, DCMI (except port child node, and board-independent properties like sensor), and DMA nodes should be moved to a board-specific overlay (#77854 does the same for display shield) & then be referenced by generic aliases (like "cam_interface" for DCMI so the sensor can be used with a board that has a cam interface other than DCMI).

BTW, should the shield be renamed to "ov2640_cam_module" for example?

@CharlesDias
Copy link
Contributor Author

I think we should define a binding for the connector (similar to dsi_lcd_qsh_030 display connector for example) since it's apparently used by many vendors (#71463 (comment)) and use that binding to specify the pins in ov2640 node, in a board-independent way.

Also, the I2C, DCMI (except port child node, and board-independent properties like sensor), and DMA nodes should be moved to a board-specific overlay (#77854 does the same for display shield) & then be referenced by generic aliases (like "cam_interface" for DCMI so the sensor can be used with a board that has a cam interface other than DCMI).

BTW, should the shield be renamed to "ov2640_cam_module" for example?

Thanks for your review, @JarmouniA!

Since I'm not familiar with the connector binding approach, I'll take a look at it carefully.

@CharlesDias CharlesDias force-pushed the weact_ministm32h7xx_ov2640 branch from d73a2d8 to 76759c8 Compare September 7, 2024 20:29
@CharlesDias
Copy link
Contributor Author

CharlesDias commented Sep 7, 2024 •
edited
Loading

Hi, @JarmouniA.

I think we should define a binding for the connector (similar to dsi_lcd_qsh_030 display connector for example) since it's apparently used by many vendors (#71463 (comment)) and use that binding to specify the pins in ov2640 node, in a board-independent way.

I think it is done! Let me know if I got your suggestion right.

BTW, should the shield be renamed to "ov2640_cam_module" for example?

About the shield name, could it be "weact_ov2640_cam_module"? To make it less generic.

@JarmouniA
Copy link
Collaborator

Hi @CharlesDias, thanks for considering my suggestions.

I think it is done! Let me know if I got your suggestion right.

Yeah, it looks good!

About the shield name, could it be "weact_ov2640_cam_module"? To make it less generic.

My point was that the connector should't be considered a WeAct one, it's used on a lot of boards from other vendors as well (like ESP32-CAM https://components101.com/sites/default/files/inline-images/ESP32-CAM-Schematics.png), so if we are going to make a compatible for it, it should not have WeAct in its name. Same thing for the camera module (https://components101.com/sites/default/files/components/ESP32-CAM-Camera-Module.jpg).

@CharlesDias
Copy link
Contributor Author

My point was that the connector should't be considered a WeAct one, it's used on a lot of boards from other vendors as well (like ESP32-CAM https://components101.com/sites/default/files/inline-images/ESP32-CAM-Schematics.png), so if we are going to make a compatible for it, it should not have WeAct in its name. Same thing for the camera module (https://components101.com/sites/default/files/components/ESP32-CAM-Camera-Module.jpg).

Good observation, @JarmouniA.

There is one point here to consider: even though the connector is similar, the position of the pins is different from the WeAct schematic https://github.com/WeActStudio/MiniSTM32H7xx/blob/master/HDK/STM32H7xx%20SchDoc%20V11.pdf. I believe this is a problem when creating a generic OV2640 shield version. What you think?

@CharlesDias
Copy link
Contributor Author

CharlesDias commented Sep 7, 2024 •
edited
Loading

I placed the connectors next to each other and noticed that the pins are upside down. Additionally, the ESP32-CAM doesn't map the PWDN pin, while the WeAct doesn't map the RESET pin (which is linked to the reset system).

image

@JarmouniA
Copy link
Collaborator

JarmouniA commented Sep 7, 2024 •
edited
Loading

the position of the pins is different from the WeAct schematic

Ah, good catch, didn't pay attention to this.

In this case, would it make sense to have a generic compatible for the connector, then specific ones for each vendor, depending on the pins mapping, that include the common one? Or would it be simpler to just keep the current implementation? The same for the cam module.

@CharlesDias
Copy link
Contributor Author

the position of the pins is different from the WeAct schematic

Ah, good catch, didn't pay attention to this.

In this case, would it make sense to have a generic compatible for the connector, then specific ones for each vendor, depending on the pins mapping, that include the common one? Or would it be simpler to just keep the current implementation? The same for the cam module.

I think we would keep it simpler as in the current implementation. Is it ok for you?

@JarmouniA
Copy link
Collaborator

I think we would keep it simpler as in the current implementation. Is it ok for you?

Yeah I agree.

@CharlesDias
Copy link
Contributor Author

Thank you @JarmouniA for your valuable review!

@CharlesDias CharlesDias force-pushed the weact_ministm32h7xx_ov2640 branch from 76759c8 to d8c2dd2 Compare September 8, 2024 10:26
@CharlesDias
Copy link
Contributor Author

Change the shield name to weact_ov2640_cam_module.

@CharlesDias CharlesDias changed the title boards: shields: add suport for weact_ministm32h7xx_ov2640 boards: shields: add suport for weact_ov2640_cam_module Sep 8, 2024
@josuah josuah added the area: Video Video subsystem label Sep 11, 2024
@henrikbrixandersen henrikbrixandersen removed their request for review September 12, 2024 09:34
@CharlesDias CharlesDias force-pushed the weact_ministm32h7xx_ov2640 branch 3 times, most recently from 3a1d31f to 222d51f Compare September 17, 2024 14:02
@josuah josuah mentioned this pull request Sep 22, 2024
Merged
@zephyrbot zephyrbot requested a review from ngphibang October 13, 2024 09:56
josuah
josuah previously approved these changes Oct 13, 2024
View reviewed changes
Copy link
Collaborator

@josuah josuah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks in good condition to be merged.
The last little details that might have been missed will be a lot easier to fix once an alternative camera module is to be supported for this board, or once an alternative board tries to use that camera module.

Thank you for this integration!

@CharlesDias CharlesDias force-pushed the weact_ministm32h7xx_ov2640 branch from 00bf49e to 8967d41 Compare October 13, 2024 10:22
@CharlesDias
Copy link
Contributor Author

CharlesDias commented Oct 13, 2024 •
edited
Loading

Hi, everyone.

What you think about the use of CONFIG_VIDEO_HFLIP=y and CONFIG_VIDEO_VFLIP=ysymbols inside of boards/shields/weact_ov2640_cam_module/boards/mini_stm32h743.conf? In this way, the shield depends on these symbols that are relevant only for sample/drivers/video/capture_to_lvgl code. Does this make sense? Is there another way? Thanks!

@CharlesDias CharlesDias force-pushed the weact_ministm32h7xx_ov2640 branch from 8967d41 to 0e5f9b9 Compare October 13, 2024 10:40
@CharlesDias
Copy link
Contributor Author

Change port label from dcmi_ep_in to zephyr_camera_dvp_in.

josuah
josuah previously approved these changes Oct 13, 2024
View reviewed changes
@CharlesDias CharlesDias requested a review from erwango October 13, 2024 11:30
@josuah
Copy link
Collaborator

josuah commented Oct 13, 2024 •
edited
Loading

What you think about the use of CONFIG_VIDEO_HFLIP=y and CONFIG_VIDEO_VFLIP=y symbols inside of boards/shields/weact_ov2640_cam_module/boards/mini_stm32h743.conf? In this way, the shield depends on these symbols that are relevant only for sample/drivers/video/capture_to_lvgl code. Does this make sense? Is there another way? Thanks!

The FPC can eventually be folded to rotate the camera by 180°, but then that can still be overridden by the user.
This reminds me of LIBV4LCONTROL_FLAGS that permits to rotate a misoriented camera at the very last moment. Ideally, there need to be a database of orientation for modules and displays, and having it in the shield's board definition makes sense to me.

kartben
kartben previously approved these changes Oct 18, 2024
View reviewed changes
boards/shields/weact_ov2640_cam_module/boards/mini_stm32h743.overlay Outdated
@@ -0,0 +1,59 @@
/*
* Copyright (c) Copyright (c) 2024 Charles Dias
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

boards/shields/weact_ov2640_cam_module/weact_ov2640_cam_module.overlay Outdated
@@ -0,0 +1,44 @@
/*
* Copyright (c) Copyright (c) 2024 Charles Dias
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@CharlesDias CharlesDias dismissed stale reviews from kartben and josuah via ada2cec October 20, 2024 15:25
@CharlesDias CharlesDias force-pushed the weact_ministm32h7xx_ov2640 branch from 0e5f9b9 to ada2cec Compare October 20, 2024 15:25
@JarmouniA JarmouniA requested review from kartben and josuah October 22, 2024 16:47
@JarmouniA JarmouniA mentioned this pull request Oct 22, 2024
Closed
josuah
josuah previously approved these changes Oct 22, 2024
View reviewed changes
kartben
kartben requested changes Oct 24, 2024
View reviewed changes
Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small comment, looks good otherwise!

samples/drivers/video/capture_to_lvgl/Kconfig Outdated
Copy link
Collaborator

@kartben kartben Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you put the change to this file in a different commit please?

@CharlesDias
boards: shields: add suport for weact_ov2640_cam_module
9b93ffb 
Add support for WeAct Studio MiniSTM32H7xx OV2640 camera sensor.

Signed-off-by: Charles Dias <[email protected]>
@CharlesDias CharlesDias force-pushed the weact_ministm32h7xx_ov2640 branch from ada2cec to 3988225 Compare October 24, 2024 18:43
@CharlesDias
samples: drivers: video: capture_to_lvgl: add menu and update the YAML
36bde7e 
Add the Kconfig menu to the sample configuration.
Update sample.yaml by adding the shield configuration.

Signed-off-by: Charles Dias <[email protected]>
@CharlesDias CharlesDias force-pushed the weact_ministm32h7xx_ov2640 branch from 3988225 to 36bde7e Compare October 24, 2024 18:46
@dleach02 dleach02 merged commit 7128e33 into zephyrproject-rtos:main Oct 25, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kartben kartben kartben approved these changes

@JarmouniA JarmouniA JarmouniA approved these changes

@josuah josuah josuah approved these changes

@avisconti avisconti Awaiting requested review from avisconti

@jfischer-no jfischer-no Awaiting requested review from jfischer-no

@mnkp mnkp Awaiting requested review from mnkp

@loicpoulain loicpoulain Awaiting requested review from loicpoulain

@nashif nashif Awaiting requested review from nashif

@ngphibang ngphibang Awaiting requested review from ngphibang

@erwango erwango Awaiting requested review from erwango

Assignees

@kartben kartben

Labels
area: GPIO area: Samples Samples area: Shields Shields (add-on boards) area: Video Video subsystem
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@CharlesDias @JarmouniA @josuah @erwango @kartben @zephyrbot @dleach02