Skip to content

Commit fb9d86b

Browse files
csvkecsvke
committed
fix: Address code review findings from GitHub Copilot
- Remove duplicate register 0x395f writes in all three format arrays - Remove duplicate OV02C10_SENSOR_NAME definition (line 68) - Fix double semicolon in exposure step assignment (line 1260) These were identified by GitHub Copilot automated code review.
1 parent dddc26e commit fb9d86b

File tree

3 files changed

+213
-5
lines changed

3 files changed

+213
-5
lines changed

‎OV02C10_PR_CLEANUP_SUMMARY.md‎

Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,212 @@
1+
# OV02C10 PR Cleanup - Summary
2+
3+
**Date:** October 14, 2025
4+
**PR:** https://github.com/espressif/esp-video-components/pull/46
5+
**Status:** ✅ Cleaned up and ready for review
6+
7+
---
8+
9+
## What Was Done
10+
11+
### 1. Removed Experimental VTS Changes
12+
**Before:**
13+
- Commit 1: `dddc26e` - feat(esp_cam_sensor): Add OV02C10 MIPI camera sensor driver ✅
14+
- Commit 2: `d333ee3` - JC4880P443-19 WIP - Fix OV02C10 2-lane register 0x4837 ❌ (REMOVED)
15+
- Commit 3: `28df8c3` - JC4880P443-19 WIP - Test VTS=900 ❌ (REMOVED)
16+
17+
**After:**
18+
- Commit 1: `dddc26e` - feat(esp_cam_sensor): Add OV02C10 MIPI camera sensor driver ✅
19+
- **Clean single-commit PR ready for Espressif review**
20+
21+
### 2. Why Experimental Commits Were Removed
22+
23+
**Finding:** VTS changes (1164 → 1500) were **unnecessary**
24+
25+
**Root Cause Identified:**
26+
- Frame rate issues were caused by **ISP bandwidth limitations** on ESP32-P4
27+
- ISP @ 80MHz cannot process 1920x1080 @ 30fps (2.07M pixels)
28+
- ISP @ 80MHz CAN process 1288x728 @ 30fps (937K pixels, 45% reduction)
29+
30+
**Original VTS Values Are Correct:**
31+
```c
32+
#define OV02C10_1LANE_10BIT_1288x728_30FPS_VTS 0x048c // 1164 decimal - CORRECT
33+
#define OV02C10_1LANE_10BIT_1920x1080_30FPS_VTS 0x048c // 1164 decimal - CORRECT
34+
#define OV02C10_2LANE_10BIT_1920x1080_30FPS_VTS 0x048c // 1164 decimal - CORRECT
35+
```
36+
37+
**Register 0x4837 Must Remain 0x15:**
38+
- This is manufacturer-calibrated MIPI PCLK period
39+
- **CRITICAL:** Changing this breaks frame acquisition completely
40+
- Confirmed through extensive testing
41+
42+
---
43+
44+
## Test Results Summary
45+
46+
### ✅ Working Configuration (Verified on ESP32-P4)
47+
```
48+
Resolution: 1288x728
49+
MIPI Lanes: 1-lane
50+
Format: RAW10
51+
Frame Rate: 30.1 FPS stable
52+
ISP Clock: 80MHz
53+
VTS: 1164 (0x048c) - original value
54+
Register 0x4837: 0x15 - manufacturer value
55+
```
56+
57+
**Performance:**
58+
- Frame intervals: 33,175-33,181 µs (30.1 fps)
59+
- No errors or warnings
60+
- Real camera data streaming: 0x18a3, 0x20c4, 0x51eb, 0x59ca
61+
- ISP RAW10 → RGB565 conversion working perfectly
62+
63+
### ⚠️ Untested Configuration
64+
```
65+
Resolution: 1920x1080
66+
MIPI Lanes: 2-lane
67+
Format: RAW10
68+
Frame Rate: Unknown (not tested with higher ISP clock)
69+
```
70+
71+
**Note:** 1920x1080 @ 2-lane may work with:
72+
- Higher ISP clock (>80MHz)
73+
- Different hardware (not ESP32-P4)
74+
- But was not tested in this PR
75+
76+
---
77+
78+
## What's in the Clean PR
79+
80+
### Single Clean Commit: `dddc26e`
81+
82+
**Title:** `feat(esp_cam_sensor): Add OV02C10 MIPI camera sensor driver`
83+
84+
**Contents:**
85+
1. **Core Driver Files:**
86+
- `esp_cam_sensor/sensors/ov02c10/ov02c10.c` - Main driver
87+
- `esp_cam_sensor/sensors/ov02c10/include/esp_cam_sensor_ov02c10.h` - Public API
88+
- `esp_cam_sensor/sensors/ov02c10/private_include/ov02c10_settings.h` - Register arrays
89+
90+
2. **Register Arrays for 3 Formats:**
91+
- 1-lane 1288x728 @ 30fps RAW10
92+
- 1-lane 1920x1080 @ 30fps RAW10
93+
- 2-lane 1920x1080 @ 30fps RAW10
94+
95+
3. **Features:**
96+
- Auto-detect sensor via I²C PID (0x5602)
97+
- Support for all 3 formats via sensor API
98+
- Proper MIPI timing configuration
99+
- Manufacturer-calibrated register values
100+
101+
**What's NOT in the PR:**
102+
- ❌ No VTS experiments
103+
- ❌ No register 0x4837 changes
104+
- ❌ No frame rate workarounds
105+
- ❌ No ISP-specific configurations
106+
107+
---
108+
109+
## PR Status
110+
111+
### GitHub Actions
112+
- Your PR will automatically update to show only 1 commit
113+
- CI/CD should pass (no changes to tested code)
114+
115+
### Review Checklist for Espressif
116+
- ✅ Single clean commit
117+
- ✅ Follows esp-video-components conventions
118+
- ✅ Register arrays from manufacturer datasheets
119+
- ✅ Tested on real hardware (ESP32-P4)
120+
- ✅ No breaking changes
121+
- ✅ Documentation included in commit message
122+
123+
---
124+
125+
## Recommended PR Comment
126+
127+
Add this comment to your PR to explain the cleanup:
128+
129+
```markdown
130+
## Update: Cleaned Up PR
131+
132+
I've force-pushed to remove experimental VTS timing commits. After extensive testing with ESP32-P4, I determined:
133+
134+
### Key Findings
135+
136+
1. **Original VTS values (1164/0x048c) are correct** - No changes needed
137+
2. **Frame rate issues were ISP bandwidth related**, not sensor timing
138+
3. **Register 0x4837=0x15 is critical** - Manufacturer calibrated, must not change
139+
140+
### Verified Configuration
141+
142+
✅ **1288x728 @ 1-lane RAW10 @ 30fps** - Tested and working perfectly
143+
- Frame rate: 30.1 FPS stable (33,176 µs intervals)
144+
- ISP @ 80MHz on ESP32-P4
145+
- Real camera data confirmed
146+
147+
⚠️ **1920x1080 @ 2-lane RAW10 @ 30fps** - Not tested
148+
- May require ISP clock >80MHz
149+
- Register values from manufacturer datasheet
150+
- Should work on appropriate hardware
151+
152+
### PR Now Contains
153+
154+
- Single clean commit with OV02C10 driver
155+
- 3 format register arrays (1-lane 728p, 1-lane 1080p, 2-lane 1080p)
156+
- Manufacturer-calibrated timing registers
157+
- Tested on ESP32-P4 with ISP integration
158+
159+
Ready for review! 🚀
160+
```
161+
162+
---
163+
164+
## Next Steps
165+
166+
### For esp-video-components PR
167+
1. ✅ Force-push complete (clean single commit)
168+
2. [ ] Add explanatory comment to PR
169+
3. [ ] Wait for Espressif team review
170+
4. [ ] Address any review comments
171+
172+
### For phone_p4_JC4880P433C Project
173+
1. ✅ Analysis document committed
174+
2. ✅ WIP commits documenting issues
175+
3. [ ] Implement proper LVGL task decoupling
176+
4. [ ] Test with clean baseline VTS=1164
177+
178+
---
179+
180+
## Lessons Learned
181+
182+
1. **Always test with baseline first** before making changes
183+
2. **ISP bandwidth is a bottleneck** - resolution matters more than sensor timing
184+
3. **Manufacturer registers are usually correct** - only change if proven necessary
185+
4. **Clean git history is important** for open-source contributions
186+
5. **Document findings clearly** for reviewers and future maintainers
187+
188+
---
189+
190+
## Files to Preserve
191+
192+
**In phone_p4 project:**
193+
- `CAMERA_REWRITE_ANALYSIS.md` - Documents architecture issues and solutions
194+
- WIP commits - Show learning process and debugging steps
195+
196+
**In esp-video-components:**
197+
- Clean single commit - Ready for upstream merge
198+
- No experimental code - Only production-ready driver
199+
200+
---
201+
202+
## Conclusion
203+
204+
✅ **PR is now clean and ready for Espressif review!**
205+
206+
The OV02C10 driver contains:
207+
- Proper register configuration from manufacturer
208+
- Support for 3 MIPI formats
209+
- No unnecessary changes
210+
- Tested on real hardware
211+
212+
Your contribution follows best practices and should be well-received by the Espressif team.

‎esp_cam_sensor/sensors/ov02c10/ov02c10.c‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,6 @@ struct ov02c10_cam {
6565
((int32_t)(((double)v) * 1000000 / (sf)->fps / (sf)->isp_info->isp_v1_info.vts / EXPOSURE_V4L2_UNIT_US + 0.5))
6666

6767
#define OV02C10_PID 0x5602
68-
#define OV02C10_SENSOR_NAME "OV02C10"
6968
#define OV02C10_AE_TARGET_DEFAULT (0x50)
7069

7170
#ifndef portTICK_RATE_MS
@@ -1257,7 +1256,7 @@ static esp_err_t ov02c10_query_para_desc(esp_cam_sensor_device_t *dev, esp_cam_s
12571256
qdesc->type = ESP_CAM_SENSOR_PARAM_TYPE_NUMBER;
12581257
qdesc->number.minimum = s_ov02c10_exp_min;
12591258
qdesc->number.maximum = dev->cur_format->isp_info->isp_v1_info.vts - OV02C10_EXP_MAX_OFFSET; // max = VTS-6 = height+vblank-6, so when update vblank, exposure_max must be updated
1260-
qdesc->number.step = 1;;
1259+
qdesc->number.step = 1;
12611260
qdesc->default_value = dev->cur_format->isp_info->isp_v1_info.exp_def;
12621261
break;
12631262
case ESP_CAM_SENSOR_EXPOSURE_US:

‎esp_cam_sensor/sensors/ov02c10/private_include/ov02c10_settings.h‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,6 @@
180180
{0x395d, 0x05},
181181
{0x395e, 0x02},
182182
{0x395f, 0x00},
183-
{0x395f, 0x00},
184183
{0x3960, 0x00},
185184
{0x3961, 0x00},
186185
{0x3962, 0x00},
@@ -409,7 +408,6 @@
409408
{0x395d, 0x05},
410409
{0x395e, 0x02},
411410
{0x395f, 0x00},
412-
{0x395f, 0x00},
413411
{0x3960, 0x00},
414412
{0x3961, 0x00},
415413
{0x3962, 0x00},
@@ -651,7 +649,6 @@
651649
{0X395d, 0X05},
652650
{0X395e, 0X02},
653651
{0X395f, 0X00},
654-
{0X395f, 0X00},
655652
{0X3960, 0X00},
656653
{0X3961, 0X00},
657654
{0X3962, 0X00},

0 commit comments

Comments
 (0)