I2s rework

[YruamaLairba]

Hi, I'm currently rewriting stm32_i2s crates, and i try to reflect my change and implement an example here in parallel for validation.

I'm currently stuck on a requirement i have. Using I2s in slave mode require to read a pin in alternate mode and in a generic way. I don't know how to implement that, so i will open an issue for that.

[burrbull]

I'm currently stuck on a requirement i have. Using I2s in slave mode require to read a pin in alternate mode and in a generic way. I don't know how to implement that, so i will open an issue for that.

Possibly with_input could help you

stm32f4xx-hal/src/gpio/convert.rs

Line 271 in 7e50031

pub fn with_input<R>(&mut self, f: impl FnOnce(&mut Pin<P, N, Input>) -> R) -> R {

https://github.com/stm32-rs/stm32h7xx-hal/blob/master/examples/gpio_with_input.rs

[YruamaLairba]

Possibly with_input could help you

stm32f4xx-hal/src/gpio/convert.rs

Line 271 in 7e50031

pub fn with_input<R>(&mut self, f: impl FnOnce(&mut Pin<P, N, Input>) -> R) -> R {

https://github.com/stm32-rs/stm32h7xx-hal/blob/master/examples/gpio_with_input.rs

Unfortunatly, this is not a solution since it reconfigure the pin. The problem will come with I2s in duplex mode where the WS pin is shared between 2 i2s peripheral. If the first peripheral act as master, reconfiguring the pin will disconnect the WS signal from the WS pin, making it impossible to read.

[burrbull]

Then what you mean saying "read in generic way"?
I thought you mean to change mode on input.

[YruamaLairba]

Then what you mean saying "read in generic way"?

I mean i can use that property on a Pin passed as generic parameter. The easiest way would to implement a custom trait, but this may not necessary if this propetery can be implemented through a generic description of pins.

More concretly, if you look i2s:: Pins trait, you will see i want to be able to read the WS pin. Since this trait is implemented for a generic set of pins, i need to add something on pins that can operate as WS pin.

[YruamaLairba]

I tryed to progress by creating a WsPin trait and trying to implement on the concerned pins. Unfortunatly, i can't find a way to implement this trait for all concerned pins without manually implementing for each concerned pin.

i wanted to reuse existent stuff by mixing generic and macro, but trying to replicate the implementation block for each SPI doesn't work, it produce a "conflicting implementation" of the trait
https://github.com/YruamaLairba/stm32f4xx-hal/blob/d59d93b8d533f445d276b835ed7766fc8cee5df6/src/i2s.rs#L117

[burrbull]

DO you need to change WS pin mode from alternate to input for reading or NOT?

[burrbull]

Instead of using SetAlternate you could just force WS pin (or all of them) to be in Alternate mode
See
f7b6268

https://github.com/stm32-rs/stm32f4xx-hal/blob/i2s_rework/src/i2s.rs

[YruamaLairba]

DO you need to change WS pin mode from alternate to input for reading or NOT?

I need to read WS pin without changing it's physical mode.

In the code, I do pointer castings for 2 purpose:

• to read the pin input state without changing gpio HAL. _is_input is private, i can't use it outside it's module.
• to present in public API a type that reflect the actual physical mode.

I tried an approach using a pin wrapper, it's seems simpler.

[YruamaLairba]

Hi, now i have a working example. i still have to typestate the driver to avoid meaningless operation, and maybe re-implement i2s transfer to fix existing examples.

What do you think of my hack around WsPin ? I fear it's a bit unclear

[burrbull]

What do you think of my hack around WsPin ? I fear it's a bit unclear

Instead of new WsPin<P, N> you could use Pin<P, N, Ws + implement gpio::marker::Readable + gpio::marker::Interruptable for Ws mode. This gives you hal::Input and ExtiPin implementations for free.

[YruamaLairba]

What do you think of my hack around WsPin ? I fear it's a bit unclear

Instead of new WsPin<P, N> you could use Pin<P, N, Ws + implement gpio::marker::Readable + gpio::marker::Interruptable for Ws mode. This gives you hal::Input and ExtiPin implementations for free.

I didn't understood right away your suggestion. You suggest to create a new mode called Ws ? At first look, it's seems a very good idea, i get same functionnality with less code and unsafe block :)

[burrbull]

I didn't understood right away your suggestion. You suggest to create a new mode called Ws ? At first look, it's seems a very good idea, i get same functionnality with less code and unsafe block :)

Yes, I suggest to create new MODE + I suggest to reuse existent structure for this:

stm32f4xx-hal/src/i2s.rs

Line 26 in 7e50031

pub type Ws = spi::Nss;

[YruamaLairba]

This works, i needed to change visibility of gpio::marker to pub(crate)

[burrbull]

As spi::Nss and i2s::Ws have different behavior I suggest to split them on different structures and duplicate PinA implementations:

stm32f4xx-hal/src/gpio/alt.rs

Line 309 in 7e50031

<spi::Nss, SPI2> for [PB9<5>, PB12<5>],

[YruamaLairba]

As spi::Nss and i2s::Ws have different behavior I suggest to split them on different structures and duplicate PinA

Maybe it worth to do the same with Ck and Sd, this will remove dependency to the spi hal module, what do you think ?

[burrbull]

As spi::Nss and i2s::Ws have different behavior I suggest to split them on different structures and duplicate PinA

Maybe it worth to do the same with Ck and Sd, this will remove dependency to the spi hal module, what do you think ?

I'm OK with this.

[YruamaLairba]

@burrbull new version of stm32_v12x have been published. What things need to be done to finish this pull request ? should I synchronize this branch with master ?

[YruamaLairba]

i resynchronized with master and updated CHANGELOG. I don't understand why there is a conflict

[burrbull]

LGTM. Except million commits. Could you squash them before merge?

[YruamaLairba]

@burrbull Is that you wanted ?

[burrbull]

bors r+

[bors]

Build succeeded:

• ci (stm32f401, stable)
• ci (stm32f405, stable)
• ci (stm32f407, stable)
• ci (stm32f410, stable)
• ci (stm32f411, stable)
• ci (stm32f412, stable)
• ci (stm32f413, stable)
• ci (stm32f415, stable)
• ci (stm32f417, stable)
• ci (stm32f423, stable)
• ci (stm32f427, stable)
• ci (stm32f429, stable)
• ci (stm32f437, stable)
• ci (stm32f439, stable)
• ci (stm32f446, stable)
• ci (stm32f469, stable)
• ci (stm32f479, stable)
• Rustfmt