You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "JianyuWang0623 (via GitHub)" <gi...@apache.org> on 2026/04/10 10:10:56 UTC
[PR] xtensa/esp32s3: refactor cam driver to use cam_ll HAL layer [nuttx]
JianyuWang0623 opened a new pull request, #18703:
URL: https://github.com/apache/nuttx/pull/18703
## Summary
As suggested by @eren-terzioglu in https://github.com/apache/nuttx/pull/18692#discussion_r3062857373,
refactor the ESP32-S3 CAM driver to use Espressif's `cam_ll_*` HAL abstraction
layer instead of direct `putreg32`/`getreg32` register manipulation.
Replace all raw register accesses in `esp32s3_cam.c` with `cam_ll_*` inline
functions from the `esp_hal_cam` component in `esp-hal-3rdparty`. This reduces
maintenance burden and aligns with Espressif's recommended driver architecture.
Changes:
- Add `lcd_cam_dev_t *hw` pointer to the driver private struct
- Use `cam_ll_start/stop/reset/fifo_reset` for CAM control
- Use `cam_ll_get_interrupt_status/clear_interrupt_status` for ISR handling
- Use `cam_ll_set_recv_data_bytelen` for DMA buffer length configuration
- Use `cam_ll_select_clk_src/set_group_clock_coeff` for clock setup
- Use `cam_ll_enable_vsync_filter/set_vsync_filter_thres` for VSYNC filtering
- Use `cam_ll_enable_vsync_generate_eof/enable_rgb_yuv_convert` for capture config
- Use struct field access for interrupt enable (`cam_ll_enable_interrupt`
requires `__DECLARE_RCC_ATOMIC_ENV` which is not available in NuttX)
- Add `esp_hal_cam` include paths to `hal.mk`
Net result: 68 insertions, 155 deletions — eliminates all `putreg32`/`getreg32`
calls from the CAM driver.
## Impact
- No functional change — the driver behavior is identical, only the register
access method changes from raw addresses to the vendor HAL layer.
- No impact on build process, documentation, security, or compatibility.
- Only affects `arch/xtensa/src/esp32s3/esp32s3_cam.c` and `hal.mk`.
## Testing
Host: Ubuntu 22.04 x86\_64, xtensa-esp32s3-elf-gcc
Target: ESP32-S3 (lckfb-szpi-esp32s3, ESP32-S3-WROOM-1-N16R8, PSRAM 8MB OCT)
with GC0308 DVP camera and ST7789 LCD
Build and flash:
```
$ cd nuttx
$ make -j$(nproc) flash ESPTOOL_PORT=/dev/ttyUSB0
...
Generated: nuttx.bin
...
Hard resetting via RTS pin...
```
Runtime test — continuous preview:
```
nsh> camera 0
nximage_listener: Connected
nximage_initialize: Screen resolution (320,240)
Start video this mode is eternal. (Non stop, non save files.)
```
Runtime test — mirrored preview:
```
nsh> camera 0 -m
nximage_listener: Connected
nximage_initialize: Screen resolution (320,240)
Start video this mode is eternal. (Non stop, non save files.)
```
Verified:
- LCD preview displays correct colors (RGB565X byte-swap path active)
- Hardware mirror (`-m`) works correctly
- MMAP buffers allocated with GDMA alignment (3 video buffers in RING mode)
- Repeated start/stop cycles — no crash, no resource leak
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] xtensa/esp32s3: refactor cam driver to use cam_ll HAL layer [nuttx]
Posted by "eren-terzioglu (via GitHub)" <gi...@apache.org>.
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 <wa...@xiaomi.com>
> 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 <wa...@xiaomi.com>
>
> 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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] xtensa/esp32s3: refactor cam driver to use cam_ll HAL layer [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #18703:
URL: https://github.com/apache/nuttx/pull/18703
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] xtensa/esp32s3: refactor cam driver to use cam_ll HAL layer [nuttx]
Posted by "JianyuWang0623 (via GitHub)" <gi...@apache.org>.
JianyuWang0623 commented on PR #18703:
URL: https://github.com/apache/nuttx/pull/18703#issuecomment-4222823945
@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 <wa...@xiaomi.com>
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 <wa...@xiaomi.com>
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
}
/**
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] xtensa/esp32s3: refactor cam driver to use cam_ll HAL layer [nuttx]
Posted by "eren-terzioglu (via GitHub)" <gi...@apache.org>.
eren-terzioglu commented on PR #18703:
URL: https://github.com/apache/nuttx/pull/18703#issuecomment-4222783922
Looks clearer a lot, nice work.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org