Camera interface esp32s3 #75331

epc-ake · 2024-07-02T15:27:10Z

Pull Request Summary

This pull request introduces video capturing functionality for the ESP32-S3 LCD_CAM interface.
~~To use this feature, you must first merge pull request #74974.~~
It depends on the hal espressif being merged and added to the zephyr manifest.
Alternatively drop the last commit so the driver writes directly into the registers and skipping the hal.
Using the xiao_esp32s3 board and the OV2640 camera sensor, I successfully captured low resolution images in RGB565 format.

Open Topics for Discussion

Although the driver is functional, several topics need further discussion:

1. Using External Memory

~~Due to issues with memory reallocation on the ESP32-S3, testing was limited to internal memory, requiring the imager to be set to the lowest resolution (160x120px).~~
#76982 enables to choose the video buffer location and to put it into to external ram. This enables this driver also to capture high resolution images.

2. ESP32-HAL

Currently, the driver directly writes to the LCD_CAM register values using the SOC register structs.
It may be more appropriate to move these calls to the hal_espressif.

3. Devicetree structure:

The LCD_CAM module involves four different devices (cam-clk, lcd-clk, lcd-interface, cam-interface).
Initially, I attempted to combine the cam-clk and cam-interface into one device and set the cam-clk rate based on a devicetree property.
This approach failed because the cam-interface references the sensor (OV2640) in the device tree, requiring the sensor to be initialized before the cam-interface, which isn't possible without first initializing the camera clock.

A proposed structure is:

lcd_cam: lcd_cam@60041000 {
    compatible = "espressif,esp32-lcd_cam";
    reg = <0x60041000 DT_SIZE_K(4)>;
    status = "disabled";
    clocks = <&rtc ESP32_LCD_CAM_MODULE>;

    cam-xclk: cam-xclk {
        compatible = "espressif,esp32-cam-xclk";
        clock-rate = <1000000>;
    };

    cam-interface: cam-interface {
        compatible = "espressif,esp32-cam";
        dmas = <&dma 2>;
        dma-names = "rx";
    };

    lcd-clk: lcd-clk {
        compatible = "espressif,esp32-lcd-clk";
        clock-rate = <1000000>;
    };

    lcd-interface: lcd-interface {
        compatible = "espressif,esp32-lcd";
        dmas = <&dma 2>;
        dma-names = "tx";
    };
};

However, this might be overly complex, especially since espressif,esp32-lcd_cam would only enable the peripheral without much additional functionality.
If you have a better idea or see an alternative way to implement this, please leave a comment!

4. Testing

I have some tests locally that only run on the xiao_esp32s3 with the ov2640 sensor. I could add them to the zephyr test folder but I don't know where to place them exactly.
It would also be nice to add support for the sample/subsys/video/ samples.
This PR adds an overlay for the xiao_esp32s3 to the samples/drivers/video/capture/.

josuah

Thank you for this driver! I tried to spot a few things that I thought you'd be interested in adjusting. Glad to see more device families part of the Video driver collection.

drivers/video/video_esp32_dvp.c

dts/bindings/clock/espressif,esp32-cam-clk.yaml

drivers/video/video_esp32_dvp.c

dts/bindings/video/espressif,esp32_cam.yaml

drivers/video/video_esp32_dvp.c

drivers/video/Kconfig.esp32_dvp

ASerbinski · 2024-07-03T16:31:46Z

I'm no expert with cameras or the differences between successive generations of esp32's, but is this fundamentally bound to the "s3" version of esp32? Or would it also be compatible with older esp32's as well?

epc-ake · 2024-07-04T06:23:40Z

@ASerbinski The s3 is the only esp32 model with a dedicated video interface. The other esp32 variants use the I2S interface to "emulate" a video interface. Therefore, this driver is only compatible with the esp32s3.

havoc-dlt · 2024-08-04T10:01:44Z

Concerning the tests/samples, I can't seem to find your sample code anywhere. I'd really like to contribute with testing, so can you point me in the right direction?

josuah · 2024-08-04T10:26:19Z

<@epc-ake> I could add them to the zephyr test folder but I don't know where to place them exactly

The best could be to have the upstream examples supported, making modifications to the example only if needed, and using a shield, a bit like how it was done for this OV5640 module.

Maybe one way to figure out how to compose the example together would be to add another example here on this PR, just for the sake of publishing the source and reviewing it, even if it is removed afterward... Or just paste the snippets of code/config if there are just a few.

Thanks again!

epc-ake · 2024-08-05T15:23:37Z

Concerning the tests/samples, I can't seem to find your sample code anywhere. I'd really like to contribute with testing, so can you point me in the right direction?

I made a new commit and added the overlays for the xiao_esp32s3 to support for the sample/subsys/video/capture.
I made some temporary changes to the sample it self, since I still didn't figure out how to store the image in external ram.
This should enable you to capture images on the ov2640 and store the image in internal ram.
Remember that you also have to checkout #74974 for this PR to work.

havoc-dlt · 2024-08-06T05:44:36Z

Concerning the tests/samples, I can't seem to find your sample code anywhere. I'd really like to contribute with testing, so can you point me in the right direction?

I made a new commit and added the overlays for the xiao_esp32s3 to support for the sample/subsys/video/capture. I made some temporary changes to the sample it self, since I still didn't figure out how to store the image in external ram. This should enable you to capture images on the ov2640 and store the image in internal ram. Remember that you also have to checkout #74974 for this PR to work.

Thanks for the reminder, but I already have a working setup with your PRs patched in and been tinkering with it and my own zephyr projects a bit. I'm looking forward to test your code in my spare time!

uLipe

Great work! Please check some comments and minor concerns I placed across this patchset

drivers/clock_control/clock_control_esp32_cam.c

dts/bindings/clock/espressif,esp32-cam-clk.yaml

drivers/video/video_esp32_dvp.c

uLipe · 2024-08-06T12:34:43Z

drivers/video/video_esp32_dvp.c

+	}
+
+	if (dma_block_iter->next_block) {
+		LOG_ERR("Not enough descriptors available. Increase DMA_ESP32_DESCRIPTOR_NUM");


I noticed there is neither irq_unlock, not goto_unlock which may cause irq state conflict.

uLipe · 2024-08-06T12:35:22Z

drivers/video/video_esp32_dvp.c

+	LCD_CAM.cam_ctrl.cam_update = 0;
+	LCD_CAM.cam_ctrl1.cam_start = 1;
+
+	data->is_streaming = true;


Question, is it a must to place a giant lock inside of this function?

probably not, since the interrupts are inactive when you start the stream any how...
I'll remove it and add a check to make sure we are not already streaming on stream_start().

This commit creates a new node for the lcd_cam interface. It enables the lcd_cam pheripheral clock and adds the possibiliy to controll the camera clock frequency trough the `cam-clk` property. This is its own device for two reasons: 1. It enables to use the camera clock as a standalone clock. 2. It enables to initialize xclk before the sensor driver. Signed-off-by: Armin Kessler <[email protected]>

Adding support for the esp32s3 camera interface. Signed-off-by: Armin Kessler <[email protected]>

Remove the VSync interrupt handle, as DMA captures VSync through the in_suc_eof event. Signed-off-by: Armin Kessler [email protected]

adding xiao_esp32s3 support to video capture sample Signed-off-by: Armin Kessler [email protected]

The driver now calls the `<hal/cam_hal.h>` API instead of writing directly into the registers. Signed-off-by: Armin Kessler [email protected]

epc-ake · 2024-08-15T13:16:03Z

@LucasTambor I've added a new commit to so that the driver now uses the espressif hal instead of writing directly into the registers.

kartben · 2024-08-15T13:17:50Z

drivers/clock_control/Kconfig.esp32_cam

@@ -0,0 +1,4 @@
+config CLOCK_CONTROL_ESP32_CAM


This is wrong, it should default to y and have a "depends on DT_HAS...". See Kconfig files for the other clock control drivers

kartben · 2024-08-15T13:18:35Z

drivers/clock_control/Kconfig.esp32_cam

+config CLOCK_CONTROL_ESP32_CAM
+	bool "master clock for esp32 camera interface"
+	help
+	  This option enables the pheriphery and cam clock for the lcd_cam module.


kartben · 2024-08-15T13:20:20Z

drivers/video/Kconfig.esp32_dvp

@@ -0,0 +1,6 @@
+config VIDEO_ESP32S3


Ditto: default y / DT_HAS_...

kartben · 2024-08-15T13:21:14Z

drivers/video/video_esp32_dvp.c

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#define DT_DRV_COMPAT espressif_esp32_cam


Would be better to keep driver file name and compatible consistent. Go for either cam or dvp everywhere (I personally prefer cam, I'd say)

kartben · 2024-08-15T13:22:23Z

samples/drivers/video/capture/boards/xiao_esp32s3_procpu.conf

+CONFIG_VIDEO_ESP32S3=y
+CONFIG_VIDEO_BUFFER_POOL_SZ_MAX=40000
+CONFIG_VIDEO_BUFFER_POOL_NUM_MAX=3
+CONFIG_CLOCK_CONTROL_ESP32_CAM=y


Drop. Not needed when you will have fixed the kconfigs

kartben · 2024-08-15T13:22:36Z

samples/drivers/video/capture/boards/xiao_esp32s3_procpu.conf

@@ -0,0 +1,5 @@
+CONFIG_DMA=y
+CONFIG_VIDEO_ESP32S3=y


kartben · 2024-08-15T13:24:14Z

samples/drivers/video/capture/src/main.c

+        fmt.width = 160;
+	fmt.height = 120;
+	fmt.pitch = fmt.width * 2;
+	fmt.pixelformat = VIDEO_PIX_FMT_RGB565;
+
+	if(video_set_format(video_dev, VIDEO_EP_OUT, &fmt)){
+                LOG_ERR("Unable to set format");
+                return 0;
+        }
+
+        printk("- Set format: %c%c%c%c %ux%u\n", (char)fmt.pixelformat,
+	       (char)(fmt.pixelformat >> 8), (char)(fmt.pixelformat >> 16),
+	       (char)(fmt.pixelformat >> 24), fmt.width, fmt.height);
+


What is this change?

This is just a workaround to be able to fit the captured frame into internal ram.
It should and will be removed once #76982 is merged.

Ah I see. But since this impacts (negatively) users using other drivers, I would not consider this workaround acceptable. If you wanted, I guess you could add Kconfigs to the sample to allow setting a max resolution though, and based on the supported resolutions you get from the call to video_get_capabilities you could set to whatever resolutions is the closest?

LucasTambor · 2024-08-15T13:47:12Z

drivers/clock_control/clock_control_esp32_cam.c

+	int ret = 0;
+
+	if (0 == cam_clk) {
+		LCD_CAM.cam_ctrl.cam_clk_sel =


Can you use HAL API here as well?

I left a comment in the hal/cam PR.

LucasTambor · 2024-08-15T13:48:09Z

drivers/video/Kconfig.esp32_dvp

@@ -0,0 +1,6 @@
+config VIDEO_ESP32S3


Let's use just VIDEO_ESP32, it's more generic for later SoCs

epc-ake · 2024-08-15T16:04:12Z

Thanks for all the reviews.
I will be on vacation for the next two weeks and won't be able to work on this.
If you need to expedite it, feel free to create a new PR based on this one.
Otherwise, I will implement your suggestions after I return.

epc-ake · 2024-09-02T14:29:59Z

Since @LucasTambor took over this at #77709 and #77879 I close this PR

epc-ake requested a review from sylvioalves as a code owner July 2, 2024 15:27

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding platform: ESP32 Espressif ESP32 area: Xtensa Xtensa Architecture area: Video Video subsystem area: Clock Control labels Jul 2, 2024

zephyrbot requested review from andyross, ceolin, dcpleung, decsny, galak, loicpoulain, LucasTambor, marekmatej, nashif, nordic-krch and uLipe July 2, 2024 15:27

zephyrbot assigned nordic-krch Jul 2, 2024

epc-ake marked this pull request as draft July 2, 2024 15:28

epc-ake mentioned this pull request Jul 3, 2024

[Page Add][Enhanced Function]Support the camera interface for XIAO ESP32S3 Sense, by Zephyr Seeed-Studio/wiki-documents#1060

Open

josuah requested changes Jul 3, 2024

View reviewed changes

epc-ake force-pushed the camera_interface_esp32s3 branch from a705d85 to 3bd5053 Compare July 3, 2024 12:35

epc-ake force-pushed the camera_interface_esp32s3 branch 2 times, most recently from 119a3ec to c4e5d10 Compare July 4, 2024 08:10

uLipe requested changes Aug 6, 2024

View reviewed changes

epc-ake added 4 commits August 7, 2024 09:26

drivers: video: esp32s3: add support for cam interface

4cb56a1

Adding support for the esp32s3 camera interface. Signed-off-by: Armin Kessler <[email protected]>

drivers: video: esp32s3: remove VSync interrupt

866c2c0

Remove the VSync interrupt handle, as DMA captures VSync through the in_suc_eof event. Signed-off-by: Armin Kessler [email protected]

sample: video: esp32s3: add support for xiao_esp32s3

8ebcb45

adding xiao_esp32s3 support to video capture sample Signed-off-by: Armin Kessler [email protected]

epc-ake force-pushed the camera_interface_esp32s3 branch from 8a742c3 to 8ebcb45 Compare August 7, 2024 07:34

josuah mentioned this pull request Aug 13, 2024

drivers: video: API enhancement #72959

Open

epc-ake mentioned this pull request Aug 15, 2024

drivers: video: Add SMH option for video buffer #76982

Merged

sample: video: esp32s3: implement hal_api

045ad83

The driver now calls the `<hal/cam_hal.h>` API instead of writing directly into the registers. Signed-off-by: Armin Kessler [email protected]

kartben requested changes Aug 15, 2024

View reviewed changes

LucasTambor reviewed Aug 15, 2024

View reviewed changes

LucasTambor mentioned this pull request Aug 28, 2024

drivers: video: esp32s3: add support for cam interface #77709

Merged

carlescufi assigned sylvioalves and unassigned nordic-krch Aug 29, 2024

epc-ake closed this Sep 2, 2024

epc-ake deleted the camera_interface_esp32s3 branch September 19, 2024 09:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Camera interface esp32s3 #75331

Camera interface esp32s3 #75331

epc-ake commented Jul 2, 2024 •

edited

Loading

josuah left a comment

ASerbinski commented Jul 3, 2024

epc-ake commented Jul 4, 2024

havoc-dlt commented Aug 4, 2024 •

edited

Loading

josuah commented Aug 4, 2024

epc-ake commented Aug 5, 2024

havoc-dlt commented Aug 6, 2024 •

edited

Loading

uLipe left a comment

uLipe Aug 6, 2024

epc-ake Aug 7, 2024

uLipe Aug 6, 2024

epc-ake Aug 7, 2024

epc-ake commented Aug 15, 2024

kartben Aug 15, 2024

kartben Aug 15, 2024

kartben Aug 15, 2024

kartben Aug 15, 2024

kartben Aug 15, 2024

kartben Aug 15, 2024

kartben Aug 15, 2024

epc-ake Aug 15, 2024

kartben Aug 15, 2024

LucasTambor Aug 15, 2024

epc-ake Aug 15, 2024

LucasTambor Aug 15, 2024

epc-ake commented Aug 15, 2024

epc-ake commented Sep 2, 2024

		@@ -0,0 +1,5 @@
		CONFIG_DMA=y
		CONFIG_VIDEO_ESP32S3=y

Camera interface esp32s3 #75331

Camera interface esp32s3 #75331

Conversation

epc-ake commented Jul 2, 2024 • edited Loading

Pull Request Summary

Open Topics for Discussion

1. Using External Memory

2. ESP32-HAL

3. Devicetree structure:

4. Testing

josuah left a comment

Choose a reason for hiding this comment

ASerbinski commented Jul 3, 2024

epc-ake commented Jul 4, 2024

havoc-dlt commented Aug 4, 2024 • edited Loading

josuah commented Aug 4, 2024

epc-ake commented Aug 5, 2024

havoc-dlt commented Aug 6, 2024 • edited Loading

uLipe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epc-ake commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epc-ake commented Aug 15, 2024

epc-ake commented Sep 2, 2024

epc-ake commented Jul 2, 2024 •

edited

Loading

havoc-dlt commented Aug 4, 2024 •

edited

Loading

havoc-dlt commented Aug 6, 2024 •

edited

Loading