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

[Proposal] I2S/SAI traits - take 3 #426

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

maximeborges
Copy link

@maximeborges maximeborges commented Nov 10, 2022

Hey there, I've started implementing I2S on the ESP32 (well, the peripheral is called I2S but is also supporting TDM and I2S left-aligned which are not per se standard I2S), and since there is no trait here for that that works for different modes (#385 wouldn't support TDM for example) I'd like to propose one.
I called the module "SAI" for "Serial Audio Interface" which is also used by different manufacturers to qualify their "I2S" peripheral, and use generics so one implement only the modes supported by their chips, and works for different number of channel for TDM.

Still in draft, I prefer finishing testing my implementation to make sure this design makes sense before asking for a proper review, but if anyone want to take a look, any suggestion is welcome.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ryankurte (or someone else) soon.

Please see the contribution instructions for more information.

@eldruin
Copy link
Member

eldruin commented Nov 11, 2022

Thank you for your work!
I would rather encourage you to integrate these changes into the separate embedded-i2s crate and do the experimentation work over there.

@maximeborges
Copy link
Author

May I ask what is the intent in having a separate crate for I2S?

@eldruin
Copy link
Member

eldruin commented Nov 14, 2022

The motivation is twofold:

  • Make development faster since the standards needed for inclusion into embedded-hal do not apply. e.g. we can make a breaking release anytime.
  • It is possible to use the traits in real HAL implementations since no dependency upon a branch of an embedded-hal fork is necessary (this is a major reason why [Proposal] I2S traits - take 2 #385 did not move forward).

Once the traits have proved themselves, we can start the integration discussion. However, in the case of embedded-can, for example, it will be simply left as a separate project.

@ccrome
Copy link

ccrome commented Apr 16, 2023

Is this going anywhere? I do wonder why TDM API is treated differently from I2S at the API level? Can't it all be thought of as 'an audio interface with n channels', and the details for the formatting dealt with at a lower level?

There obviously needs to be some method of specifying whether the hardware goes into TDM or I2S mode, and how many channels.

@maximeborges
Copy link
Author

Is this going anywhere? I do wonder why TDM API is treated differently from I2S at the API level? Can't it all be thought of as 'an audio interface with n channels', and the details for the formatting dealt with at a lower level?

There obviously needs to be some method of specifying whether the hardware goes into TDM or I2S mode, and how many channels.

This is exactly what I did on my branch, and the code is quite generic over any type of SAI interface, and some parts are even already tested on hardware :)
Still don't really have time to wrap that up to propose a PR, but the code is available for anyone to test and review. I would gladly help anyone wanting to merge it, but cannot drive the whole process right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants