Skip to content

Conversation

@sylvioalves
Copy link
Contributor

@sylvioalves sylvioalves commented May 17, 2025

  1. Remove endpoint ID entry in esp32 video driver needed after API changes.
  2. Add build_all/video tests for ESP32-S3 video driver.

Remove endpoint ID entry in esp32 video driver needed after
API changes.

Signed-off-by: Sylvio Alves <[email protected]>
kartben
kartben previously approved these changes May 17, 2025
ngphibang
ngphibang previously approved these changes May 17, 2025
Copy link
Contributor

@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.

Thanks !

Make sure esp32s3 video driver is built during CI tests.

Signed-off-by: Sylvio Alves <[email protected]>
@sylvioalves sylvioalves dismissed stale reviews from ngphibang and kartben via ac3cc76 May 17, 2025 10:11
@sylvioalves sylvioalves requested review from kartben and ngphibang May 17, 2025 10:12
@sylvioalves
Copy link
Contributor Author

sylvioalves commented May 17, 2025

Updated the PR to include esp32s3_eye board video test.

@sonarqubecloud
Copy link

Comment on lines +31 to +33
drivers.video.esp32_dvp.build:
platform_allow:
- esp32s3_eye/esp32s3/procpu
Copy link
Contributor

@ngphibang ngphibang May 17, 2025

Choose a reason for hiding this comment

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

I am not sure if we need to add the extra_args with an esp32 shield to get this tested in twister (because the device node is usually disabled by default and enabled in a separate shield). If it's the case, you can look at the ST platform example just above:

  drivers.video.stm32_dcmi.build:
    platform_allow:
      - stm32h7b3i_dk/stm32h7b3xx
      - arduino_nicla_vision/stm32h747xx/m7
    extra_args:
      - platform:stm32h7b3i_dk/stm32h7b3xx:SHIELD=st_b_cams_omv_mb1683

We can test this by making some wrong code in the driver and run twister to see if it can detect the error:

west twister -i -T tests/drivers/build_all/video

If it can, so that's fine.

Otherwise, if it takes time to do, we can merge the hot fix first then adding the test later in another PR. What do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need for shield or extra_args as I see. esp32s3_eye/esp32s3/procpu board is stand-alone. For testing porpuses with the device it would need extra .conf file though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it is building successfully:
INFO - 617/954 esp32s3_eye/esp32s3/procpu drivers.video.esp32_dvp.build

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, tha'ts fine then. Thanks again !

@ngphibang ngphibang linked an issue May 17, 2025 that may be closed by this pull request
Copy link
Contributor

@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.

[EDIT: I missed the "eye" in esp32s3_eye, all good to go!]

@ngphibang ngphibang added the Hotfix Fix for issues blocking development, i.e. upstream CI issues, tests failing in upstream CI , etc. label May 19, 2025
@kartben kartben merged commit b78d1a1 into zephyrproject-rtos:main May 19, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Video Video subsystem Hotfix Fix for issues blocking development, i.e. upstream CI issues, tests failing in upstream CI , etc. platform: ESP32 Espressif ESP32 size: XS A PR changing only a single line of code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Some video drivers lack of CI tests

7 participants