Skip to content

boards: shields: add suport for st_b_cams_omv_mb1683 #76143

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

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

CharlesDias
Copy link
Contributor

@CharlesDias CharlesDias commented Jul 21, 2024

Add support for ST B-CAMS-OMV MB1683 shield.

Required PR:

Testing on STM32H7B3I-DK

west build -p -b stm32h7b3i_dk --shield st_b_cams_omv_mb1683 samples/drivers/video/capture_to_lvgl/

@CharlesDias CharlesDias force-pushed the st_b_cams_omv_mb1683 branch from 74f10a5 to f9b29d6 Compare August 4, 2024 10:49
@CharlesDias CharlesDias changed the title boards: shields: add suport for st_b_cams_omv_mb1683 [DNM] boards: shields: add suport for st_b_cams_omv_mb1683 Aug 5, 2024
@CharlesDias CharlesDias marked this pull request as ready for review September 14, 2024 21:49
@zephyrbot zephyrbot added the area: Samples Samples label Sep 14, 2024
@CharlesDias CharlesDias changed the title [DNM] boards: shields: add suport for st_b_cams_omv_mb1683 boards: shields: add suport for st_b_cams_omv_mb1683 Sep 14, 2024
@CharlesDias
Copy link
Contributor Author

Rebase.

@zephyrbot zephyrbot requested a review from ngphibang October 12, 2024 18:20
@CharlesDias
Copy link
Contributor Author

Updated endpoint configuration according to #74415.

@kartben
Copy link
Collaborator

kartben commented Nov 26, 2024

@CharlesDias will you be able to revisit this PR?

@CharlesDias
Copy link
Contributor Author

Rebase

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.

Some optional proposals to avoid easy mistakes.
Besides this, I do not see anything else missing, and LGTM!

Comment on lines 98 to 100
``_st_b_cams_omv_mb1683`` and adding the necessary device tree properties.

Set ``--shield "_st_b_cams_omv_mb1683"`` when you invoke ``west build``. For example:
Copy link
Collaborator

Choose a reason for hiding this comment

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

_st_b_cams_omv_mb1683 -> st_b_cams_omv_mb1683 ?
Is it a copy-paste typo from the title or is the --shield flag works with the leading underscore?
Thanks!

Copy link
Contributor Author

@CharlesDias CharlesDias Feb 4, 2025

Choose a reason for hiding this comment

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

My bad! Thanks for catching it! I'll fix!

:alt: B-CAMS-OMV-MB1683

B-CAMS-OMV MB1683 Image (Credit: STMicroelectronics.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am tempted to add a short sentence reminding what this is documenting, as it is easily mistaken by the connector that is in the devicetree (different pinout). Like this or any other wording:

The camera signals go into the shield from one of the supported input connectors (CN1, CN2, CN4), and out of the shield towards Zephyr through the output 30-pin ZIF connector CN5.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In addition, how about something like:

Suggested change
refer to the User manual for CN1 and CN2 pinout


B-CAMS-OMV MB1683 Image (Credit: STMicroelectronics.)

Waveshare camera board connector CN4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Waveshare camera board connector CN4
Waveshare camera board connector CN4 (camera input)

Copy link
Collaborator

Choose a reason for hiding this comment

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

| 17 | PWR_EN / LED1 | 18 | RESET# |
+------------+-----------------+------------+--------------+

ZIF connector CN5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ZIF connector CN5
ZIF connector CN5 (camera output)

Copy link
Collaborator

Choose a reason for hiding this comment

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

For reference:
scrot_20250204_150841_761x677

Comment on lines 55 to 87
+------------+--------------+------------+--------------+
| Pin number | Description | Pin number | Description |
+============+==============+============+==============+
| 1 | 3V3 | 16 | DCMI_PIXCK |
+------------+--------------+------------+--------------+
| 2 | GND | 17 | GND |
+------------+--------------+------------+--------------+
| 3 | I2C_SCL | 18 | SPI_MISO |
+------------+--------------+------------+--------------+
| 4 | I2C_SDA | 19 | SPI_CS |
+------------+--------------+------------+--------------+
| 5 | RESET# | 20 | DCMI_D7 |
+------------+--------------+------------+--------------+
| 6 | PWR_EN / LED1| 21 | DCMI_D6 |
+------------+--------------+------------+--------------+
| 7 | SHUTTER | 22 | DCMI_D5 |
+------------+--------------+------------+--------------+
| 8 | GND | 23 | DCMI_D4 |
+------------+--------------+------------+--------------+
| 9 | PULLDOWN / LED2 | 24 | DCMI_D3 |
+------------+--------------+------------+--------------+
| 10 | Camera_CLK | 25 | DCMI_D2 |
+------------+--------------+------------+--------------+
| 11 | 3V3 | 26 | DCMI_D1 |
+------------+--------------+------------+--------------+
| 12 | DCMI_VSYNC | 27 | DCMI_D0 |
+------------+--------------+------------+--------------+
| 13 | 5V (RSU) | 28 | SPI_MOSI |
+------------+--------------+------------+--------------+
| 14 | DCMI_HSYNC | 29 | SPI_CLK |
+------------+--------------+------------+--------------+
| 15 | GND | 30 | GND |
+------------+--------------+------------+--------------+
Copy link
Collaborator

Choose a reason for hiding this comment

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

ST used a 2-columns table for aesthetical reasons, but we could consider preserving the layout of the 30-pin connector for clarity:

Suggested change
+------------+--------------+------------+--------------+
| Pin number | Description | Pin number | Description |
+============+==============+============+==============+
| 1 | 3V3 | 16 | DCMI_PIXCK |
+------------+--------------+------------+--------------+
| 2 | GND | 17 | GND |
+------------+--------------+------------+--------------+
| 3 | I2C_SCL | 18 | SPI_MISO |
+------------+--------------+------------+--------------+
| 4 | I2C_SDA | 19 | SPI_CS |
+------------+--------------+------------+--------------+
| 5 | RESET# | 20 | DCMI_D7 |
+------------+--------------+------------+--------------+
| 6 | PWR_EN / LED1| 21 | DCMI_D6 |
+------------+--------------+------------+--------------+
| 7 | SHUTTER | 22 | DCMI_D5 |
+------------+--------------+------------+--------------+
| 8 | GND | 23 | DCMI_D4 |
+------------+--------------+------------+--------------+
| 9 | PULLDOWN / LED2 | 24 | DCMI_D3 |
+------------+--------------+------------+--------------+
| 10 | Camera_CLK | 25 | DCMI_D2 |
+------------+--------------+------------+--------------+
| 11 | 3V3 | 26 | DCMI_D1 |
+------------+--------------+------------+--------------+
| 12 | DCMI_VSYNC | 27 | DCMI_D0 |
+------------+--------------+------------+--------------+
| 13 | 5V (RSU) | 28 | SPI_MOSI |
+------------+--------------+------------+--------------+
| 14 | DCMI_HSYNC | 29 | SPI_CLK |
+------------+--------------+------------+--------------+
| 15 | GND | 30 | GND |
+------------+--------------+------------+--------------+
+------------+-----------------+
| Pin number | Description |
+============+=================+
| 1 | 3V3 |
+------------+-----------------+
| 2 | GND |
+------------+-----------------+
| 3 | I2C_SCL |
+------------+-----------------+
| 4 | I2C_SDA |
+------------+-----------------+
| 5 | RESET# |
+------------+-----------------+
| 6 | PWR_EN / LED1 |
+------------+-----------------+
| 7 | SHUTTER |
+------------+-----------------+
| 8 | GND |
+------------+-----------------+
| 9 | PULLDOWN / LED2 |
+------------+-----------------+
| 10 | Camera_CLK |
+------------+-----------------+
| 11 | 3V3 |
+------------+-----------------+
| 12 | DCMI_VSYNC |
+------------+-----------------+
| 13 | 5V (RSU) |
+------------+-----------------+
| 14 | DCMI_HSYNC |
+------------+-----------------+
| 15 | GND |
+------------+-----------------+
| 16 | DCMI_PIXCK |
+------------+-----------------+
| 17 | GND |
+------------+-----------------+
| 18 | SPI_MISO |
+------------+-----------------+
| 19 | SPI_CS |
+------------+-----------------+
| 20 | DCMI_D7 |
+------------+-----------------+
| 21 | DCMI_D6 |
+------------+-----------------+
| 22 | DCMI_D5 |
+------------+-----------------+
| 23 | DCMI_D4 |
+------------+-----------------+
| 24 | DCMI_D3 |
+------------+-----------------+
| 25 | DCMI_D2 |
+------------+-----------------+
| 26 | DCMI_D1 |
+------------+-----------------+
| 27 | DCMI_D0 |
+------------+-----------------+
| 28 | SPI_MOSI |
+------------+-----------------+
| 29 | SPI_CLK |
+------------+-----------------+
| 30 | GND |
+------------+-----------------+

Comment on lines 7 to 21
(1) 3V3 (2) GND
(3) I2C_SCL (4) I2C_SDA
(5) RESET# (6) PWR_EN / LED1
(7) SHUTTER (8) GND
(9) PULLDOWN / LED2 (10) Camera_CLK
(11) 3V3 (12) DCMI_VSYNC
(13) 5V (RSU) (14) DCMI_HSYNC
(15) GND (16) DCMI_PIXCK
(17) GND (18) SPI_MISO
(19) SPI_CS (20) DCMI_D7
(21) DCMI_D6 (22) DCMI_D5
(23) DCMI_D4 (24) DCMI_D3
(25) DCMI_D2 (26) DCMI_D1
(27) DCMI_D0 (28) SPI_MOSI
(29) SPI_CLK (30) GND
Copy link
Collaborator

Choose a reason for hiding this comment

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

At your disposition, a more verbose but less ambiguous alternative:

Suggested change
(1) 3V3 (2) GND
(3) I2C_SCL (4) I2C_SDA
(5) RESET# (6) PWR_EN / LED1
(7) SHUTTER (8) GND
(9) PULLDOWN / LED2 (10) Camera_CLK
(11) 3V3 (12) DCMI_VSYNC
(13) 5V (RSU) (14) DCMI_HSYNC
(15) GND (16) DCMI_PIXCK
(17) GND (18) SPI_MISO
(19) SPI_CS (20) DCMI_D7
(21) DCMI_D6 (22) DCMI_D5
(23) DCMI_D4 (24) DCMI_D3
(25) DCMI_D2 (26) DCMI_D1
(27) DCMI_D0 (28) SPI_MOSI
(29) SPI_CLK (30) GND
(1) 3V3
(2) GND
(3) I2C_SCL
(4) I2C_SDA
(5) RESET#
(6) PWR_EN / LED1
(7) SHUTTER
(8) GND
(9) PULLDOWN / LED2
(10) Camera_CLK
(11) 3V3
(12) DCMI_VSYNC
(13) 5V (RSU)
(14) DCMI_HSYNC
(15) GND
(16) DCMI_PIXCK
(17) GND
(18) SPI_MISO
(19) SPI_CS
(20) DCMI_D7
(21) DCMI_D6
(22) DCMI_D5
(23) DCMI_D4
(24) DCMI_D3
(25) DCMI_D2
(26) DCMI_D1
(27) DCMI_D0
(28) SPI_MOSI
(29) SPI_CLK
(30) GND

Copy link
Collaborator

Choose a reason for hiding this comment

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

If preserving the original "2-column" layout, it could help to transpose the table so that it looks like the one in the doc and PDF.

@CharlesDias
Copy link
Contributor Author

CharlesDias commented Feb 4, 2025

Implemented previous suggestions, except this one #76143 (comment).

@erwango and @ngphibang, do you mean?

config VIDEO_STM32_DCMI
	default y
	depends on DT_HAS_ST_STM32_DCMI_ENABLED

@CharlesDias
Copy link
Contributor Author

CharlesDias commented Feb 5, 2025

Implemented suggestion #76143 (comment).

config VIDEO_STM32_DCMI
	default y
	depends on DT_HAS_ST_STM32_DCMI_ENABLED

@CharlesDias
Copy link
Contributor Author

Delete boards/shields/st_b_cams_omv_mb1683/Kconfig.defconfig once VIDEO_STM32_DCMI config is already defined in the driver's Kconfig.

Copy link
Collaborator

@ngphibang ngphibang left a comment

Choose a reason for hiding this comment

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

In general, I think we should clarify if the shield is specific only to ST boards or generic enough for any 30-pin dvp camera interface, then the shield name, readme, node labels will be made according to it. I learned some lessons from @kartben 's comments in these shield PRs :-)

Add support for ST B-CAMS-OMV MB1683 shield.

Signed-off-by: Charles Dias <[email protected]>
@JarmouniA JarmouniA added this to the v4.1.0 milestone Feb 5, 2025
@JarmouniA JarmouniA requested a review from kartben February 5, 2025 19:50
@erwango
Copy link
Member

erwango commented Feb 6, 2025

@ngphibang For your next round of review, would you mind closing the discussion on comments that have been addressed ? This will be helpful for other reviewers. Thanks!

Copy link
Collaborator

@ngphibang ngphibang left a comment

Choose a reason for hiding this comment

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

LGTM, but other reviewers may have some findings

@ngphibang
Copy link
Collaborator

@erwango Yes, done. Sorry !

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

LGTM.

@josuah PTAL

@kartben kartben merged commit 435b6f6 into zephyrproject-rtos:main Feb 6, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: camera area: GPIO area: Samples Samples area: Shields Shields (add-on boards) area: Video Video subsystem platform: STM32 ST Micro STM32
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants