eren-terzioglu commented on PR #18703: URL: https://github.com/apache/nuttx/pull/18703#issuecomment-4222883758
> @eren-terzioglu Thanks again for the suggestion — I've been refactoring the ESP32-S3 CAM driver to use the `cam_ll_*` HAL layer as you recommended. > > During testing I found a bug in `cam_ll_start()` in `esp-hal-3rdparty` (`components/esp_hal_cam/esp32s3/include/hal/cam_ll.h`): > > The current code writes `cam_update` before `cam_start`: > > ```c > dev->cam_ctrl.cam_update = 1; > dev->cam_ctrl1.cam_start = 1; > ``` > > `cam_update` is a self-clearing latch — it captures the current register values at the moment it's written. Since `cam_start` is still 0 when `cam_update` fires, the hardware latches `cam_start=0`. The CAM only appears to work because a previous start may have left `cam_start` high. On a clean first start, this causes the capture to silently fail (symptom: screen flickering / no frame output). > > The fix is to swap the order — set `cam_start=1` first, then trigger `cam_update` to latch it: > > ```c > dev->cam_ctrl1.cam_start = 1; > dev->cam_ctrl.cam_update = 1; > ``` > > Does `espressif/esp-hal-3rdparty` accept external PRs to `release/master.b`? If so I'll submit one. Otherwise, here's the patch: > > ```diff > $ git show > commit 44ec73727cb0bfefb587246d48095c662e372fd4 (HEAD) > Author: wangjianyu3 <[email protected]> > Date: Fri Apr 10 18:14:30 2026 +0800 > > hal/cam_ll: fix cam_ll_start register write order > > cam_update is a self-clearing latch that captures the current value of > cam_start. Writing cam_update before cam_start means the latch fires > while cam_start is still 0, so the CAM never actually starts on the > first call — it only works if a previous start left cam_start high. > > Swap the order: set cam_start=1 first, then trigger cam_update to latch > it. This matches the hardware intent documented in the TRM. > > Signed-off-by: wangjianyu3 <[email protected]> > > diff --git a/components/esp_hal_cam/esp32s3/include/hal/cam_ll.h b/components/esp_hal_cam/esp32s3/include/hal/cam_ll.h > index 2b58c45f76f..9f6619cf3dd 100644 > --- a/components/esp_hal_cam/esp32s3/include/hal/cam_ll.h > +++ b/components/esp_hal_cam/esp32s3/include/hal/cam_ll.h > @@ -443,8 +443,8 @@ static inline void cam_ll_set_data_wire_width(lcd_cam_dev_t *dev, uint32_t width > __attribute__((always_inline)) > static inline void cam_ll_start(lcd_cam_dev_t *dev) > { > - dev->cam_ctrl.cam_update = 1; > dev->cam_ctrl1.cam_start = 1; > + dev->cam_ctrl.cam_update = 1; // self clear, latches cam_start=1 > } > > /** > ``` It is fine to open there but we have to ask it into `esp-idf` team about it. Not sure is it added for some reason. Decision is up to you but merge process will take time because a different team works on it. I will ask the reason to responsible team. Thanks for the report. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
