[Proposal] I2S/SAI traits - take 3

rust-embedded / embedded-hal

A Hardware Abstraction Layer (HAL) for embedded systems

Apache License 2.0

2.01k stars 202 forks source link

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

Open maximeborges opened 2 years ago

maximeborges commented 2 years ago

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 (https://github.com/rust-embedded/embedded-hal/pull/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

eldruin commented 2 years ago

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 #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 commented 1 year ago

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 commented 1 year ago

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.