add I2S camera driver #3219

rursprung · 2025-03-05T21:51:41Z

this is an attempt at rebasing #1646 to the latest version. all credits go to @Dominaezzz for the original implementation.
this isn't ready for actual review yet as it hasn't been tested yet (the first attempt at testing it didn't work on ESP32-CAM) and there are some open TODOs (see the commit-msg).

note: there'll be force-pushes to this branch until this PR is ready.

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

I have updated existing examples or added new ones (if applicable).
I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
My changes were added to the CHANGELOG.md in the proper section.
~~I have added necessary changes to user code to the Migration Guide.~~ (only new code => not needed)
My changes are in accordance to the esp-rs developer guidelines

Extra:

I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

see the commit-msg for further details.

Description

see the commit-msg for further details.

Testing

tested by me on an ESP32-CAM (with the diff shown below to make it work on that) and by @Dominaezzz on his device (for which the qa-test is written)

esp-hal/src/i2s/master/mod.rs

Dominaezzz · 2025-03-05T22:25:58Z

I ran the example and the I2C bit works. The DMA streaming bit doesn't because the CPU was too slow, suspicious.

esp-hal/src/i2s/master/camera.rs

rursprung · 2025-03-06T06:27:54Z

thanks a lot for testing this! i've resolved your immediate feedbacks which should get the example running for you OOTB.
as a next step i'll try to get it working on my ESP32-CAM and then clean up the PR and mark it ready for review

rursprung · 2025-03-07T07:35:41Z

i now got this to work on my ESP32-CAM board as well! the following changes are needed for that in the example:

diff --git a/qa-test/src/bin/i2s_camera.rs b/qa-test/src/bin/i2s_camera.rs
index 08d761af..cf867786 100644
--- a/qa-test/src/bin/i2s_camera.rs
+++ b/qa-test/src/bin/i2s_camera.rs
@@ -2,13 +2,13 @@
 #![feature(iter_from_coroutine)]

 //! This shows how to continuously receive data from a Camera via I2S,
-//! using a "Makerfabs ESP32 3.2" TFT LCD with Camera"
+//! using an AI Thinker ESP32-CAM board.
 //!
 //! This example reads a JPEG from an OV2640 and writes it to the console as
 //! hex.
 //!
 //! Pins used:
-//! XCLK    GPIO32
+//! XCLK    GPIO0
 //! SIOD    GPIO26
 //! SIOC    GPIO27
 //! PCLK    GPIO22
@@ -48,6 +48,11 @@ use esp_hal::{
     main,
     time::Rate,
     Blocking,
+    gpio::{
+        Output,
+        Level,
+        OutputConfig,
+    },
 };
 use esp_println::println;

@@ -62,7 +67,10 @@ fn main() -> ! {

     let cam_siod = peripherals.GPIO26;
     let cam_sioc = peripherals.GPIO27;
-    let cam_xclk = peripherals.GPIO32;
+    let cam_xclk = peripherals.GPIO0;
+
+    /// power down pin - must be pulled low for the camera to run!
+    let _cam_pwdn = Output::new(peripherals.GPIO32, Level::Low, OutputConfig::default());

     let camera = Camera::new(
         peripherals.I2S0,

this is still marked as a draft because i have a few TODOs left for which i do however need input:

add missing docs for DataFormat
add missing example to docs
should the qa-test example be adapted to the ESP32-CAM instead? which board is more prevalent?

rursprung

once i have your input on this i'll update the PR and then it should be hopefully ready to merge. it'd be great if you could already review the rest of it - thanks!

esp-hal/src/i2s/master/camera.rs

qa-test/src/bin/i2s_camera.rs

esp-hal/src/i2s/master/mod.rs

rursprung · 2025-03-09T19:06:56Z

FYI: i've left this under i2s::master since a lot of the code is based on traits which are present in there. i'm not sure how you'd want to best refactor this with an i2s::slave (duplicate code? move things up to i2s? does that even make sense?).
note that i'm far out of my waters with this - it's the first time i'm doing anything with i2s (i just wanted to get some pictures from that camera and wanted to use rust for it since i'm always advocating for it 😅)

Dominaezzz · 2025-03-09T19:25:22Z

FYI: i've left this under i2s::master since a lot of the code is based on traits which are present in there. i'm not sure how you'd want to best refactor this with an i2s::slave (duplicate code? move things up to i2s? does that even make sense?).

You should be able to reference the traits in i2s::master from a different file. If visibility is an issue feel free to use pub(crate) on whatever.

note that i'm far out of my waters with this - it's the first time i'm doing anything with i2s (i just wanted to get some pictures from that camera and wanted to use rust for it since i'm always advocating for it 😅)

I can understand, this pretty close to the finish line, the only major issue is the file location.

esp-hal/src/i2s/master/camera.rs

rursprung · 2025-03-09T20:40:48Z

You should be able to reference the traits in i2s::master from a different file. If visibility is an issue feel free to use pub(crate) on whatever.

this is not that simple since these traits are in i2s::master::private - i doubt that i should make that pub(crate), that'd defeat its purpose? so either these are "master-only" (which doesn't seem to be the case) or maybe they shouldn't be in private?

Dominaezzz · 2025-03-09T20:45:00Z

private means private to the crate (esp_hal), rather than private to the file.

Though I agree, some refactoring needs doing.

rursprung · 2025-03-10T18:55:58Z

as discussed on matrix i've left it under i2s::master for the time being.

i've incorporated all your review feedback (thanks!) except for the with_data_pins comment - see my comment above as well as on your lcd_cam::cam PR (#3237 (comment)): i think using with_d[0..7] methods would be a much worse API (& developer experience) compared to this or a struct (as used in lcd_cam::cam so far). i hope a maintainer can chip in on this discussion and then i'll adapt to whatever the outcome of it is (even if it's the ugly API 🤷)

esp-hal/src/i2s/master/camera.rs

this implements the I2S camera functionality. it has been tested using an AI Thinker ESP32-CAM board with an OV2640 camera as well as a Makerfabs ESP32 3.2" TFT LCD with Camera with the same camera model. the qa-test is based on the latter board. this connection can be found on ESP32 boards. note that this is independent of `lcd_cam::cam` module which is a different kind of camera. `DataFormat::DualChannel32` is not currently available as part of this PR since it's also not present in `esp32-camera` and it's thus unclear what its behaviour would be and whether that should be supported. while this is currently under `i2s::master` it should theoretically belong to `i2s::slave` - but it relies heavily on traits which need to be moved elsewhere so that they're accessible (currently they're under `i2s::master::private`). this refactoring will be done at a later stage by the maintainers :) Co-Authored-By: Ralph Ursprung <[email protected]>

this offers two HTTP endpoints: - `/image`: returns a single JPEG image - `/stream`: returns a `multipart/x-mixed-replace` stream of JPEGs currently all settings (e.g. resolution) are hardcoded and nothing can be changed by the user. this is a very far cry from what `esp32-camera` offers. if you need something productive you might be better off using the [`CameraWebServer` Arduino example]. areas needing improvement: - don't use `Vec<u8>` for image but stream it directly to HTTP (but still in an HTTP-agnostic way if ever possible) - allow configuring settings (via HTTP endpoint) - refactor and move generic things out: in the best case there's a generic camera trait available somewhere centrally which is implemented in HALs/BSPs and applications don't need to care about them. - add documentation - everything else which might need a cleanup currently this is based on a fork of `esp-hal` until [esp-hal#3219] is merged. [`CameraWebServer` Arduino example]: https://github.com/espressif/arduino-esp32/tree/master/libraries/ESP32/examples/Camera/CameraWebServer [esp-hal#3219]: esp-rs/esp-hal#3219

rursprung · 2025-03-16T07:26:49Z

i've now created an example application which can use this: https://github.com/rursprung/esp32-cam-test/

it'd be great if this PR would get approved & merged so that it eventually lands in the next release (and i can switch away from the git dependency), thanks!

esp-hal/src/i2s/master/camera.rs

Dominaezzz · 2025-03-16T15:16:53Z

esp-hal/src/i2s/master/camera.rs

+        mut self,
+        mut buf: BUF,
+        len: usize,
+    ) -> Result<CameraTransfer<'d, I2S, BUF>, Error> {


When this fails, along with the error, the camera and the buffer should be returned.

Like Result<CameraTransfer<'d, I2S, BUF>, (Error, Self, BUF)>

Dominaezzz · 2025-03-16T15:19:37Z

esp-hal/src/i2s/master/camera.rs

+}
+
+/// Represents the camera interface.
+pub struct Camera<'d, I2S: DmaEligible> {


The I2S generic parameter should be erased, like this

esp-hal/esp-hal/src/i2s/parallel.rs

Lines 237 to 244 in 7c159b6

pub struct I2sParallel<'d, Dm>

where

Dm: DriverMode,

{

instance: PeripheralRef<'d, AnyI2s>,

tx_channel: ChannelTx<'d, Dm, PeripheralTxChannel<AnyI2s>>,

_guard: PeripheralGuard,

}

Also, do the guard thing.

rursprung · 2025-03-16T15:33:02Z

esp-hal/src/i2s/master/camera.rs

+        i2s: impl Peripheral<P = I2S> + 'd,
+        config: Config,
+        channel: impl Peripheral<P = CH> + 'd,
+    ) -> Self {


note that this returns Self instead of Result<Self, ConfigError> because - at this point - there is no error which can happen. should i have an empty ConfigError enum just to fulfill the dev. guide? adding an enum entry later would anyway be a breaking change for consumers, so we can as well switch to a Result later if it turns out that there are errors

should i have an empty ConfigError enum just to fulfill the dev. guide?

Yes please. Adding variants to #[non_exhaustive] enums is not considered a breaking change.

bugadani reviewed Mar 5, 2025

View reviewed changes

esp-hal/src/i2s/master/mod.rs Outdated Show resolved Hide resolved

Dominaezzz reviewed Mar 5, 2025

View reviewed changes

esp-hal/src/i2s/master/camera.rs Show resolved Hide resolved

Dominaezzz reviewed Mar 5, 2025

View reviewed changes

esp-hal/src/i2s/master/camera.rs Outdated Show resolved Hide resolved

rursprung force-pushed the i2s-cam branch from feb9db6 to fc079f9 Compare March 6, 2025 06:27

rursprung force-pushed the i2s-cam branch from fc079f9 to 464c9fe Compare March 7, 2025 07:33

rursprung commented Mar 7, 2025

View reviewed changes

esp-hal/src/i2s/master/camera.rs Outdated Show resolved Hide resolved

esp-hal/src/i2s/master/camera.rs Outdated Show resolved Hide resolved

qa-test/src/bin/i2s_camera.rs Show resolved Hide resolved

rursprung commented Mar 7, 2025

View reviewed changes

esp-hal/src/i2s/master/mod.rs Show resolved Hide resolved

rursprung mentioned this pull request Mar 7, 2025

provide full-fledged example for ESP camera #3157

Open

rursprung force-pushed the i2s-cam branch 2 times, most recently from 9a8bcb8 to 7fc1f89 Compare March 9, 2025 19:05

rursprung force-pushed the i2s-cam branch from 7fc1f89 to b570634 Compare March 9, 2025 19:21

rursprung force-pushed the i2s-cam branch from b570634 to a1e1848 Compare March 9, 2025 19:26

rursprung marked this pull request as ready for review March 9, 2025 19:41

Dominaezzz reviewed Mar 9, 2025

View reviewed changes

move mod i2s::master to own folder

aa00357

rursprung force-pushed the i2s-cam branch 2 times, most recently from 0a2a762 to 1430d79 Compare March 10, 2025 18:55

Dominaezzz reviewed Mar 10, 2025

View reviewed changes

esp-hal/src/i2s/master/camera.rs Outdated Show resolved Hide resolved

rursprung force-pushed the i2s-cam branch 2 times, most recently from c7ecece to d616104 Compare March 12, 2025 07:30

rursprung commented Mar 12, 2025

View reviewed changes

esp-hal/src/i2s/master/camera.rs Outdated Show resolved Hide resolved

bugadani reviewed Mar 12, 2025

View reviewed changes

esp-hal/src/i2s/master/camera.rs Outdated Show resolved Hide resolved

rursprung force-pushed the i2s-cam branch from d616104 to f0f2b9f Compare March 12, 2025 09:00

rursprung force-pushed the i2s-cam branch from f0f2b9f to a988b9c Compare March 12, 2025 10:02

rursprung requested review from bugadani and Dominaezzz March 16, 2025 07:28

Dominaezzz reviewed Mar 16, 2025

View reviewed changes

esp-hal/src/i2s/master/camera.rs Show resolved Hide resolved

Dominaezzz reviewed Mar 16, 2025

View reviewed changes

esp-hal/src/i2s/master/camera.rs Show resolved Hide resolved

Dominaezzz reviewed Mar 16, 2025

View reviewed changes

rursprung commented Mar 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add I2S camera driver #3219

add I2S camera driver #3219

rursprung commented Mar 5, 2025 •

edited

Loading

Dominaezzz commented Mar 5, 2025

rursprung commented Mar 6, 2025

rursprung commented Mar 7, 2025

rursprung left a comment

rursprung commented Mar 9, 2025

Dominaezzz commented Mar 9, 2025

rursprung commented Mar 9, 2025

Dominaezzz commented Mar 9, 2025

rursprung commented Mar 10, 2025

rursprung commented Mar 16, 2025 •

edited

Loading

Dominaezzz Mar 16, 2025

Dominaezzz Mar 16, 2025

rursprung Mar 16, 2025

bugadani Mar 19, 2025

	pub struct I2sParallel<'d, Dm>
	where
	Dm: DriverMode,
	{
	instance: PeripheralRef<'d, AnyI2s>,
	tx_channel: ChannelTx<'d, Dm, PeripheralTxChannel<AnyI2s>>,
	_guard: PeripheralGuard,
	}

add I2S camera driver #3219

Are you sure you want to change the base?

add I2S camera driver #3219

Conversation

rursprung commented Mar 5, 2025 • edited Loading

Submission Checklist 📝

Extra:

Pull Request Details 📖

Description

Testing

Dominaezzz commented Mar 5, 2025

rursprung commented Mar 6, 2025

rursprung commented Mar 7, 2025

rursprung left a comment

Choose a reason for hiding this comment

rursprung commented Mar 9, 2025

Dominaezzz commented Mar 9, 2025

rursprung commented Mar 9, 2025

Dominaezzz commented Mar 9, 2025

rursprung commented Mar 10, 2025

rursprung commented Mar 16, 2025 • edited Loading

Dominaezzz Mar 16, 2025

Choose a reason for hiding this comment

Dominaezzz Mar 16, 2025

Choose a reason for hiding this comment

rursprung Mar 16, 2025

Choose a reason for hiding this comment

bugadani Mar 19, 2025

Choose a reason for hiding this comment

rursprung commented Mar 5, 2025 •

edited

Loading

rursprung commented Mar 16, 2025 •

edited

Loading