XCLK_FREQ_OVERRIDE + other small stuff #20

PhosphorosVR · 2025-11-11T21:50:41Z

Add general version configuration to FaceFocusVR board files
Added XCLK_FREQ_OVERRIDE and logging
Moved XCLK_FREQ out of configs, default now 20MHz
Adjusted camera XCLK frequency for OV2640/OV3660
Reduced OV2640 XCLK to 20MHz for FaceFocusVR boards (~50fps)
Tweaked JPEG quality and LED timer/channel settings (didnt fix NO SOI Error)

- Added Logging for it - Moved XCLK_FREQ away from konfigs, default set to 230MHz - updated version numbers for facefocusvr boards

…nflicts (didnt help lol)

…ocusvr boards

…be better for sensor

Copilot

Pull Request Overview

This PR refactors camera XCLK frequency configuration to support per-sensor overrides and resolves LEDC timer/channel conflicts. The main changes include replacing the global CONFIG_CAMERA_USB_XCLK_FREQ with sensor-specific override options (CONFIG_CAMERA_XCLK_FREQ_OVERRIDE_OV2640 and CONFIG_CAMERA_XCLK_FREQ_OVERRIDE_OV3660), implementing override logic in CameraManager, and moving LED PWM from TIMER_0/CHANNEL_0 to TIMER_1/CHANNEL_1 to avoid conflicts with camera XCLK.

Replaced global USB XCLK config with sensor-specific overrides for OV2640/OV3660
Implemented runtime XCLK frequency override logic based on detected camera sensor
Moved LED PWM to TIMER_1/CHANNEL_1 to avoid hardware conflict with camera XCLK (TIMER_0)

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
sdkconfig	Removed USB_XCLK_FREQ config, added override configs for OV2640/OV3660
main/Kconfig.projbuild	Removed USB XCLK configuration, added sensor-specific override options with help text
boards/sdkconfig.base_defaults	Removed default USB_XCLK_FREQ setting
boards/seed_studio/xiao_esp32s3	Updated to use new override configs (OV2640: 23MHz)
boards/project_babble/project_babble	Updated to use new override configs (OV2640: 23MHz)
boards/facefocusvr/face	Updated to use new override configs (OV2640: 20MHz, OV3660: 24MHz), added version 1.1.0
boards/facefocusvr/eye_R	Updated to use new override configs (OV2640: 20MHz, OV3660: 24MHz), added version 1.1.0
boards/facefocusvr/eye_L	Updated to use new override configs (OV2640: 20MHz, OV3660: 24MHz), added version 1.1.0
components/CameraManager/CameraManager/CameraManager.cpp	Implemented sensor-specific XCLK override logic, added logging, adjusted JPEG quality, introduced USB default XCLK of 20MHz
components/LEDManager/LEDManager/LEDManager.cpp	Changed LED PWM from TIMER_0/CHANNEL_0 to TIMER_1/CHANNEL_1 to avoid conflict with camera

Copilot · 2025-11-11T21:54:02Z

main/Kconfig.projbuild

+        range 0 40000000
+        help
+            If non-zero, overrides the selected XCLK frequency specifically when an OV2640 sensor
+            is detected. Use 0 to disable and fall back to CAMERA_USB_XCLK_FREQ / CAMERA_WIFI_XCLK_FREQ.


The help text references CAMERA_USB_XCLK_FREQ, which was removed in this PR. Since USB mode now uses a hardcoded default of 20MHz (see CAMERA_USB_XCLK_FREQ_DEFAULT in CameraManager.cpp), the help text should be updated to reflect that it falls back to the USB default (20MHz) or CAMERA_WIFI_XCLK_FREQ depending on the mode.

Copilot · 2025-11-11T21:54:02Z

main/Kconfig.projbuild

+        range 0 40000000
+        help
+            If non-zero, overrides the selected XCLK frequency specifically when an OV3660 sensor
+            is detected. Use 0 to disable and fall back to CAMERA_USB_XCLK_FREQ / CAMERA_WIFI_XCLK_FREQ.


The help text references CAMERA_USB_XCLK_FREQ, which was removed in this PR. Since USB mode now uses a hardcoded default of 20MHz (see CAMERA_USB_XCLK_FREQ_DEFAULT in CameraManager.cpp), the help text should be updated to reflect that it falls back to the USB default (20MHz) or CAMERA_WIFI_XCLK_FREQ depending on the mode.

Copilot · 2025-11-11T21:54:02Z

components/CameraManager/CameraManager/CameraManager.cpp

+    const auto pid = temp_sensor->id.PID;
+    int desired_freq = 0;
+#ifdef CONFIG_CAMERA_XCLK_FREQ_OVERRIDE_OV2640
+    if (pid == OV2640_PID && CONFIG_CAMERA_XCLK_FREQ_OVERRIDE_OV2640 > 0)


The override logic does not prevent applying overrides to OV5640 sensors. If an OV5640 is detected, it gets its frequency set on line 212, but then the override logic (lines 224-262) will still execute. If OV5640's PID happens to match OV2640_PID or OV3660_PID, an unintended override could be applied. While unlikely due to different PIDs, the logic should explicitly exclude OV5640 from the override check (e.g., add && pid != OV5640_PID) or restructure to use else-if to prevent processing OV5640 twice.

Copilot · 2025-11-11T21:54:03Z

components/CameraManager/CameraManager/CameraManager.cpp

+    }
+#endif
+#ifdef CONFIG_CAMERA_XCLK_FREQ_OVERRIDE_OV3660
+    if (pid == OV3660_PID && CONFIG_CAMERA_XCLK_FREQ_OVERRIDE_OV3660 > 0)


The override logic does not prevent applying overrides to OV5640 sensors. If an OV5640 is detected, it gets its frequency set on line 212, but then the override logic (lines 224-262) will still execute. If OV5640's PID happens to match OV2640_PID or OV3660_PID, an unintended override could be applied. While unlikely due to different PIDs, the logic should explicitly exclude OV5640 from the override check (e.g., add && pid != OV5640_PID) or restructure to use else-if to prevent processing OV5640 twice.

Copilot · 2025-11-11T21:54:03Z

components/LEDManager/LEDManager/LEDManager.cpp

+    uint32_t duty = (state == LED_ON) ? ((50 * 255) / 100) : 0;
+    ESP_ERROR_CHECK_WITHOUT_ABORT(ledc_set_duty(LEDC_LOW_SPEED_MODE, LEDC_CHANNEL_1, duty));
+    ESP_ERROR_CHECK_WITHOUT_ABORT(ledc_update_duty(LEDC_LOW_SPEED_MODE, LEDC_CHANNEL_1));


The indentation is inconsistent with the surrounding code. This line should be indented to align with the other statements inside the if block (lines 176-177).

Suggested change

uint32_t duty = (state == LED_ON) ? ((50 * 255) / 100) : 0;

ESP_ERROR_CHECK_WITHOUT_ABORT(ledc_set_duty(LEDC_LOW_SPEED_MODE, LEDC_CHANNEL_1, duty));

ESP_ERROR_CHECK_WITHOUT_ABORT(ledc_update_duty(LEDC_LOW_SPEED_MODE, LEDC_CHANNEL_1));

uint32_t duty = (state == LED_ON) ? ((50 * 255) / 100) : 0;

ESP_ERROR_CHECK_WITHOUT_ABORT(ledc_set_duty(LEDC_LOW_SPEED_MODE, LEDC_CHANNEL_1, duty));

ESP_ERROR_CHECK_WITHOUT_ABORT(ledc_update_duty(LEDC_LOW_SPEED_MODE, LEDC_CHANNEL_1));

Good catch by copilot, though, I'm not enforcing any formatting just yet. I probably should

Copilot · 2025-11-11T21:54:03Z

components/LEDManager/LEDManager/LEDManager.cpp

    ESP_LOGI(LED_MANAGER_TAG, "Updating external LED duty to %u%% (raw %lu)", dutyPercent, dutyCycle);

    // Apply to LEDC hardware live
    // We configured channel 0 in setup with LEDC_LOW_SPEED_MODE


The comment states 'channel 0' but the code now uses CHANNEL_1 (line 190). The comment should be updated to 'channel 1' to match the current implementation.

Suggested change

// We configured channel 0 in setup with LEDC_LOW_SPEED_MODE

// We configured channel 1 in setup with LEDC_LOW_SPEED_MODE

lorow · 2025-12-13T19:05:49Z

components/CameraManager/CameraManager/CameraManager.cpp

      .frame_size = FRAMESIZE_240X240, // QQVGA-UXGA, For ESP32, do not use sizes above QVGA when not JPEG. The performance of the ESP32-S series has improved a lot, but JPEG mode always gives better frame rates.

-  .jpeg_quality = 8, // 0-63, for OV series camera sensors, lower number means higher quality // Below 6 stability problems
+  .jpeg_quality = 10, // 0-63, for OV series camera sensors, lower number means higher quality // Below 6 stability problems


Why do we wanna lower the quality once more? Were there any problems with it at 8 for facefocus boards or?

lorow · 2025-12-13T19:06:21Z

components/CameraManager/CameraManager/CameraManager.cpp

  // 20000000 max freq on ESP32-CAM
  // 24000000 optimal freq on ESP32-S3 // 23MHz same fps
  int xclk_freq_hz = CONFIG_CAMERA_WIFI_XCLK_FREQ;
+  ESP_LOGI(CAMERA_MANAGER_TAG, "Base WIFI XCLK freq (from config): %d Hz", xclk_freq_hz);


lorow · 2025-12-13T19:09:30Z

components/CameraManager/CameraManager/CameraManager.cpp

+  // Use code default for UVC if no per-sensor override applies
+  constexpr int CAMERA_USB_XCLK_FREQ_DEFAULT = 20000000; // 20 MHz default
+  xclk_freq_hz = CAMERA_USB_XCLK_FREQ_DEFAULT;


is there a reason to define CAMERA_USB_XCLK_FREQ_DEFAULT instead of just directly assigning 20000000 to xclk_freq_hz ?

lorow · 2025-12-13T19:14:22Z

components/CameraManager/CameraManager/CameraManager.cpp

+  // Minimal per-sensor override logic (only if enabled via Kconfig and different)
+  if (temp_sensor)
+  {
+    const auto pid = temp_sensor->id.PID;


good catch with the check, but since we're checking for the camera_id a couple of lines above, we will never hit it. We will crash right away if things failed to grab the sensor.

Also, I'm genuinely not sure why we're applying the overrides here like that. Can't we check if they're defined and > 0 right in the beginning and if so - override the base XCLK variable and then let the system handle everything on its own?

We could make this even cleaner by defining a method for it - like int determineXCLKFrequency(int cameraPID) {}

What do you think?

lorow · 2025-12-13T19:15:11Z

components/LEDManager/LEDManager/LEDManager.cpp

        .speed_mode = LEDC_LOW_SPEED_MODE,
        .duty_resolution = resolution,
-        .timer_num = LEDC_TIMER_0,
+        .timer_num = LEDC_TIMER_1, // moved to TIMER_1 to avoid conflict with camera XCLK (TIMER_0)


good catch, thank you!

lorow · 2025-12-13T19:15:45Z

components/LEDManager/LEDManager/LEDManager.cpp

+    uint32_t duty = (state == LED_ON) ? ((50 * 255) / 100) : 0;
+    ESP_ERROR_CHECK_WITHOUT_ABORT(ledc_set_duty(LEDC_LOW_SPEED_MODE, LEDC_CHANNEL_1, duty));
+    ESP_ERROR_CHECK_WITHOUT_ABORT(ledc_update_duty(LEDC_LOW_SPEED_MODE, LEDC_CHANNEL_1));


Good catch by copilot, though, I'm not enforcing any formatting just yet. I probably should

lorow · 2025-12-13T19:18:10Z

Sorry it took me so long to reviewing your PR, a couple of things happened, but we're back on track. I've left some questions, I'm not sure if I'm sold on the current implementation of overrides BUT I do think it's a good idea to have two separate and clearly defined freq for the different sensors. Also, once more, good catch with the timers, thank you!

PhosphorosVR added 14 commits November 4, 2025 15:23

Add general version configuration to facefocusvr board files

31017a8

Merge branch 'EyeTrackVR:main' into main

264afc5

- Added XCLK_FREQ_OVERRIDE

71b1417

- Added Logging for it - Moved XCLK_FREQ away from konfigs, default set to 230MHz - updated version numbers for facefocusvr boards

Merge branch 'main' of https://github.com/PhosphorosVR/OpenIris-ESPIDF

1ea3444

Adjust camera JPEG quality and LED timer/channel settings to avoid co…

cd34c4d

…nflicts (didnt help lol)

Adjust camera JPEG quality and LED timer/channel settings to avoid co…

58aacc8

…nflicts (didnt help lol)

ahhhh

7015cde

Merge branch 'main' of https://github.com/PhosphorosVR/OpenIris-ESPIDF

8d5e9ef

Merge branch 'main' of https://github.com/PhosphorosVR/OpenIris-ESPIDF

2f196a8

Merge branch 'main' of https://github.com/PhosphorosVR/OpenIris-ESPIDF

c74e2fd

Adjust camera XCLK frequency settings for OV2640 and OV3660 for facef…

c359f9b

…ocusvr boards

Reduce OV2640 XCLK frequency to 20MHz ~50fps still enough but should …

889d4ce

…be better for sensor

- sdkconfig for babble

23e28af

Updated sdkconfig for Project Babble

81f9849

Copilot AI review requested due to automatic review settings November 11, 2025 21:50

Copilot started reviewing on behalf of PhosphorosVR November 11, 2025 21:51 View session

Copilot finished reviewing on behalf of PhosphorosVR November 11, 2025 21:52

Copilot AI reviewed Nov 11, 2025

View reviewed changes

PhosphorosVR marked this pull request as draft December 4, 2025 02:35

lorow requested changes Dec 13, 2025

View reviewed changes

	// We configured channel 0 in setup with LEDC_LOW_SPEED_MODE
	// We configured channel 1 in setup with LEDC_LOW_SPEED_MODE

XCLK_FREQ_OVERRIDE + other small stuff #20

Are you sure you want to change the base?

XCLK_FREQ_OVERRIDE + other small stuff #20

Uh oh!

Conversation

PhosphorosVR commented Nov 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorow commented Dec 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants