Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i2s support? #205

Closed
VorpalBlade opened this issue Jan 14, 2023 · 14 comments
Closed

i2s support? #205

VorpalBlade opened this issue Jan 14, 2023 · 14 comments

Comments

@VorpalBlade
Copy link

I'm currently considering using rust for my next ESP32 project. I would like to use IDF and std, so I was looking at this HAL (as opposed to https://github.com/esp-rs/esp-hal).

However, I can only find references to i2s in the no-std HAL, not in this HAL. As this is something I need for my next project, I'm curious why this is? Is it just not implemented (yet)?

@ivmarkov
Copy link
Collaborator

It is not implemented yet. You can be the first to implement it!

@maximeborges
Copy link
Contributor

maximeborges commented Jan 30, 2023

I had some working things in here master...maximeborges:esp-idf-hal:feature/i2s-impl based on my fork of embedded-hal rust-embedded/embedded-hal@master...maximeborges:embedded-hal:feature/sai, but I'm on way too many projects at the moment and this one is not high in the priority list currently, and this code need quite a bit more love before being ready to merge I think.

If you want to take over or just get some of my stuff working, you can ping me

Also @eldruin proposed to move the SAI traits from embedded-hal to https://github.com/eldruin/embedded-i2s-rs (see rust-embedded/embedded-hal#426 (comment))

@VorpalBlade
Copy link
Author

I might look at this a bit later during the year, my hobby project where I need this probably won't happen before the summer at least.

@dacut
Copy link
Contributor

dacut commented Mar 14, 2023

I've been looking into some of this recently; I've been hacking on an epaper display that uses parallel I2S on the ESP32 (mainly used for LCDs and cameras). The example C code they provide uses private 4.x APIs like periph_module_enable and the multiplexing GPIO functionality via gpio_matrix_out.

The gory details (at least for ESP32) at in section 12.5 of the ESP32 Technical Reference Manual[PDF] discussing LCD and camera interfacing.

The I2S system was redesigned in the 5.0 release, but seems to have omitted support for this kind of functionality: the struct defining the I2S setup can only deal with a single GPIO for input or output.

I'm curious how many others this affects. Obviously it's critical for me to have the parallel support (the 5.x C driver is unusable for this board); but I wouldn't waste the effort trying to make my driver more generic than it needs to be if nobody else needs it.

@VorpalBlade
Copy link
Author

I'm curious how many others this affects. Obviously it's critical for me to have the parallel support (the 5.x C driver is unusable for this board); but I wouldn't waste the effort trying to make my driver more generic than it needs to be if nobody else needs it.

My plan is to use i2s for sound (microphone as well as speaker), so I would not need this sort of functionality.

@dacut
Copy link
Contributor

dacut commented Mar 22, 2023

I am currently working on this, initially targeting compatibility with the ESP-IDF v5.0 driver. You can follow my progress on this fork/branch: https://github.com/dacut/esp-idf-hal/tree/i2s

@dacut
Copy link
Contributor

dacut commented Mar 24, 2023

At this point, my fork has support for standard mode, but I have yet to test it. I will be assembling a test jig to do some verification this weekend.

@ivmarkov, you might know the answer to this question. There are a few 5.0 headers that aren't being bindgen-ed in esp-idf-sys, so I'm having to pass

[[package.metadata.esp-idf-sys.extra_components]]
bindings_header = "bindings.h"

in esp-idf-hal's Cargo.toml, with this bindings.h header. This is probably not ideal. I'm assuming I need to do this directly in esp-idf-sys's bindings.h, but I'm not sure where the preprocessor defines (e.g. ESP_IDF_COMP_SOC_ENABLED) come from. They don't look like Kconfigs, and both Google and grep are failing me here...

Nevermind. I was barking up the wrong tree. ESP_IDF_COMP_DRIVER_ENABLED is the (existing) component I needed. I've created a pull request for esp-idf-sys.

@dacut
Copy link
Contributor

dacut commented Mar 26, 2023

Additional fixes have been pushed to my branch. There is test code available over in my test-esp-sound repository.

@ivmarkov
Copy link
Collaborator

@dacut I need the i2s functionality too in the meantime, so I'm willing to work on merging your code - if you open a PR.
The code looks to be in a quite good state already, but changes most likely will be necessary, so if you open a PR, we can discuss there.

Some initial thoughts by only quickly looking at the driver code: your treatment of i2s pins seems not to be compatible with how the other drivers treat pins - unless I'm missing something, I can't pass a & 'd mut ref to a pin peripheral in the driver and have the pin peripheral available once the driver is dropped. We might also need to fix how the RX callback is passed to the driver. While using & dyn is a definite improvement compared to how other drivers do it, passing a *mut raw pointer reference makes the whole function unsafe due to lifetime annotations missing, and the fn is not marked as unsafe. Yet, by life-timing the dyn reference we might be able to fix it.

But as I said - by PR-ing the driver we might have more fruitful conversation, with references in the code, etc.

@dacut
Copy link
Contributor

dacut commented Mar 29, 2023

Hi @imarkov,

It’s not quite ready for review yet, so I was hesitant to open a PR, but will do so in a draft state when I get a chance over the next couple days or so. As you note, there are a few things in there that aren’t right (like the *mut for a callback), because I found I had coded myself into a corner earlier.

Didn’t mean for this to be the end-all-be-all.

Also have more hardware on the way to fix up my test bed for verification.

Thanks!

@dacut dacut mentioned this issue Apr 7, 2023
@dacut
Copy link
Contributor

dacut commented Apr 7, 2023

Life got a bit busy with some family health issues and travel over the last couple weeks, but for those following along, I've now created a PR over here: #232

@dacut
Copy link
Contributor

dacut commented Jun 27, 2023

#232 and its cleanup PR, #269, have been merged. Go drop some beats!

@ivmarkov, this issue can be closed.

@Vollbrecht
Copy link
Collaborator

Vollbrecht commented Jun 27, 2023

#232 and its cleanup PR, #269, have been merged. Go drop some beats!

After your nice work here, we are all interested in your uber project that is utilizing the driver, so we can use it as an example 😸

@dacut
Copy link
Contributor

dacut commented Jun 27, 2023

@Vollbrecht My journey here started with trying to get this e-paper display running with a Rust backend. However, this device uses a strange I2S mode, parallel I2S, that isn't well documented (and I've not seen how to port this to the 5.x SDK yet). You can see how someone else wrote a C/C++ driver for this, and the C existing demo code relies on the 4.4 SDK.

So I have a bit more work to do (probably outside of esp-idf-hal).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

5 participants