[Proposal] I2S traits

[eldruin]

I added some I2S traits to kick off the discussion started in #203.
The traits accept two words for left and right and are otherwise similar to SPI.

An interleaved version can be found in the i2s-interleaved branch. The data should be passed interleaved. e.g. [left0,right0,left1,right1,...]

A version of these traits for the embedded-hal 0.2.x version can be found in the i2s-v0.2.x branch.

There is another MCU implementation in stm32f4xx-hal

TODO:

• Demo microcontroller HAL implementation:
  • stm32h7xx-hal - discussion issue
  • [WIP] stm32f4xx-hal
• Bit-banging HAL implementation supporting 16-bit and 32-bit packets
  • Working example tested using both MAX98357A and PCM5102A
• Working example of saw wave
  • Uses driver: https://github.com/maxekman/cs43l22
• Full-duplex trait implementation
  • stm32h7xx-hal - discussion issue
• Changelog entry.
• Remove try_ prefix
• Move files according to [Reorganization Proposal 1] Move traits into blocking/nonblocking modules #278 after that is merged

Thoughts?
cc. @maxekman

[rust-highfive]

r? @therealprof

(rust_highfive has picked a reviewer for you, use r? to override)

[maxekman]

Thanks @eldruin for opening the proposal. I also got contacted by @jkristell about drivers for the CS43L22 (which uses I2S) which he also had been experimenting with. Maybe @jkristell also has some input to the discussion?

[therealprof]

Do we need LW and RW separate for some reason?

[maxekman]

For PCM mode it’s not needed at all, for the other modes it’s not technically needed but could be semantically useful. For the other modes first left then right is written to the data register, or for Philips mode it’s synced to the WS clock. But I’m very new to this and have little practical knowledge. I guess the final decision depends on what abstraction level is the right one for the embedded-hal lib. The more high level left/right channel semantics (or even multichannel) can definitely be left to individual drivers to supply IMO.

[eldruin]

Separate LW and RW would allow different types for the buffers as long as they can be converted to a W iterator. I would rather not require them to be the same type if it is not necessary.

[maxekman]

Just out of curiosity, when would it make sense to have different types for the left and right channel? To my understanding they would always be interleaved when written and an application would most likely write both a left and right word at the same time.

[ryankurte]

Separate LW and RW would allow different types for the buffers as long as they can be converted to a W iterator.

Is there a situation in which this is useful? from my extremely limited i2s experience the left and right channels are always interleaved and usually going to be run at the same sampling rate / bit depth?

Afaik we usually have separate methods for iterator based approaches, rather than trying to be generic over both. For example, something like fn try_write<W>(&mut self, left_words: &[W], right_words: &[W]) -> Result<(), Self::Error> vs. fn try_write_iter<W, I: Iterator<W>>(&mut self, left_words: I, right_words: I)

[eldruin]

I thought separate types would improve support for 1-channel "mono" communication. e.g. maybe one wants to have a struct that generates numbers on the fly for the left channel and just loop over a zero-filled buffer for the right channel.

However, I just had a look at the multichannel description (TDM, although it is not standard) and it seems anything higher level than a raw buffer would not be a good fit.
I will update this PR soon.

[maxekman]

Sounds good. The new interface looks much cleaner BTW.
I'll try to play with an implementation of it in the stm32f4xx-hal crate I get some time, mostly for me to learn the ecosystem better.

[maxekman]

Now when I have some more knowledge with I2S the separate left and right words might not be such a bad idea after all. What supports this is the fact that a full frame is considered transmitted only after both left and right has been sent. The word size can be 16, 24 or 32 bits. In the STM32F4 there is a special status flag (CHSIDE) which can be used together with TXE to determine which word to send next. It would be good to be able to take advantage of that.

I have also read the data sheet for the CS4385 which is a 8 channel DAC. In TDM mode it can receive all 8 channels over a SAI interface, which is basically a more modular version of I2S. In that case it can send 256 bits after only one WS trigger, but that behavior is not possible with standard I2S. My conclusion is that it’s perfectly ok for the HAL to focus on providing a 2 channel abstraction, which is all that standard I2S will be able to do.

Could you revert back the last change @eldruin? Sorry for the extra rounds 🙂

[eldruin]

@maxekman Very interesting :) Sure, no problem. I just reverted the last change in the i2s branch. The interleaved version can be found in the i2s-interleaved branch.

[maxekman]

Thanks @eldruin, will give it a go later today.

[maxekman]

I would like to bring up another topic; clock control.

I know that setting up a device etc is outside the scope of the embedded-hal project, but I would like to gather the opinions on clock setup for I2S. Is there a way it can be done in the scope of embedded-hal?

The clock setup plays such a major role for I2S often involving both a master clock for external ADCs and a word clock for the channels in addition to the ordinary serial clock. Any other thoughts on this topic?

[maxekman]

BTW, I have started work on the CS43L22 driver, which could be an example/reference implementation. I'll hopefully get it into the stm32f407g-disc crate eventually (https://github.com/stm32-rs/stm32f407g-disc).

[maxekman]

I have now also started to experiment with a I2S HAL implementation for the stm32f4xx-hal crate (https://github.com/stm32-rs/stm32f4xx-hal). My initial tests will be hardcoded only for the I2S 3 for the f407 which is what the 407g-disc board uses for audio out. Later all f4xx should be supported with the macro system that is used in those crates.

[maxekman]

I have working sound output from the F4 discovery board now! The next step will be to do the same thing but via this HAL proposal and see how that feels. Will feedback any discoveries on the way.

[maxekman]

I have published an early draft of a MCU HAL implementation: stm32-rs/stm32f4xx-hal#145

If you want you can update the todo in the description @eldruin.

[maxekman]

I have also published a WIP of a driver implementation for the CS43L22 DAC found on the STM32F4 Discovery board: https://github.com/maxekman/cs43l22

@eldruin You can add this to the description also if you want.

[maxekman]

Here is a running example of a simple frequency sweep of a inverted saw wave: https://github.com/maxekman/stm32f407g-disc/blob/dac-support-with-cs43l22/examples/audio.rs

[eldruin]

Cool stuff @maxekman! I updated the description accordingly.

[eldruin]

I backported this PR to embedded-hal 0.2.x for testing ease in the i2s-v0.2.x branch.
I wrote an initial bit-banging implementation of the I2S traits here.
Next I will test it on hardware and then iterate adding further capabilities.

[maxekman]

@eldruin That is cool, do you have some DAC chip to test with?

[maxekman]

@eldruin Do you think I should also implement the v2.x version of the HAL for stm32f4xx-hal? I'm not familiar with the timelines for when the current master of embedded-hal will be released. I did hear something that it may be released as v1..

[eldruin]

Yes, the current embedded-hal master state will be released as v1.0 but I am not sure when that will happen (see #177). Maybe it would make sense to release this in a 0.2.x release to get more testing done before 1.0.
What do you think @therealprof?

I have a couple I2S DACs but had no chance to try them so far. I want to test my implementation with a MAX98357A which uses standard I2S (the only mode I implemented so far). If you want give it a try as well that would be great :)

[maxekman]

@eldruin Sounds good, I'll work on the v0.2.x HAL implementation first then, in any case.

[RaoulHC]

Is it just the stm32f4xx-hal implementation that's needed for this to go in? I've been playing around with audio on the stm32f407g-disc too, so it would be good to see this get merged and not use a selection of half finished branches. Happy to help that go in if you're no longer working on it @maxekman, as I see there hasn't been movement in a while.

[maxekman]

@RaoulHC I would love to finish this, I might be able to do it today. I have indeed not worked on this in a while due to the day job. If I don't get anything up soon, feel free to help out.

[maxekman]

@eldruin Could you rebase and push your i2s-v0.2.x branch (and possibly the i2s also)? Thanks!

[eldruin]

@maxekman I have rebased both branches now. FYI, I think it would be best to release this as part of embedded-hal version 1.1.0 to avoid getting anything else on the way of 1.0.0. Version 1.1.0 could be quickly released after 1.0 and if this is ready by then, I believe it could be merged.

[maxekman]

@eldruin Are you suggesting that I drop the work towards v0.2.x and completely focuses on v1.x?

I have splited all work in v0.2 and v1.0 branches for stm32f4xx-hal, stm32f407g-disco and my CSxxx DAC driver, which means we are flexible with it either way.

[eldruin]

@eldruin Are you suggesting that I drop the work towards v0.2.x and completely focuses on v1.x?

That is not what I meant. I just wanted to let you know that this does not need to be done before the 1.0 release.
Regarding the i2s-v0.2.x version, I only added it to ease testing and development of I2S support since using the i2s branch required changing everything to be 1.0-compatible first and that was a lot of work.
As of now, the embedded-hal v0.2.x branch is only receiving some maintenance fixes and I2S should only be merged to the master (1.x) branch after the 1.0 release.
Does this clarify?

[maxekman]

Yep, thanks for the clarification @eldruin!

[maxekman]

Got some progress on the ref implementation / POC today but could not get any sensible audio to play using the trait version (the non-trait version send(u16) works and plays the saw tooth) from the example in stm32f407g-disco (stm32-rs/stm32f407g-disc#16). Must be something with the generics that is not done correctly (I'm pretty new to Rust and the type system). Never mind, it was just a buffer underrun because I waited for transmission after filling the buffer, instead of waiting for empty buffer before transmission. Switched order and it works now.

[maxekman]

Maybe this is ready to be merged soon? I have not been able to finish my reference implementation but @samcrow did finish a similar implementation in stm32-rs/stm32f4xx-hal#265.

[eldruin]

@maxekman cool!
What would be left would be implementing the traits in this PR on top of stm32-rs/stm32f4xx-hal#265. I think it should be fairly trivial but maybe some details come up then.

[samcrow]

I implemented some of these proposed traits in a branch of stm32f4xx-hal. The implementation is in another library: https://github.com/samcrow/stm32_i2s/blob/ehal1/src/ehal.rs . The corresponding version of stm32f4xx-hal, with an example that uses the WriteIter trait, is here: https://github.com/samcrow/stm32f4xx-hal/blob/ehal1-i2s/examples/i2s-audio-out-hal.rs.

The process was simple. Some notes:

1. What should Write::try_write, WriteIter::try_write_iter, and Read::try_read do when left_words and right_words have different lengths? I decided to use the minimum of the two lengths, and ignore any samples at the end of the longer channel, just because that was the easiest way to implement it.
2. I didn't implement FullDuplex because the driver does not yet support full-duplex mode.

[YruamaLairba]

Hi, i just discovered this I2S proposal. samcrow mentionned it because i'm rewriting I2s implementation for stm32f4xx. Is the discussion still open ? I have remarks, comments and suggestions.

The first is the following: i see many times the terms "slave" to refer to the other I2s device. I think referring in this way is an error, "master" and "slave" just define who provide clocks, and the "master" is not even necessarily one of the I2s interface implied in data communication.

[eldruin]

This discussion is still open. I updated this PR to how we do things now in #385 so as not to break any implementations using this branch.
I also removed any "slave" mention in the documentation.
Thank you very much so far everybody and let's continue the discussion over at #385