What is the purpose of the slave mode macros, I2S_MODE_SLAVE_RX, I2S_MODE_SLAVE_TX?

I have another patch for preliminary I2S slave support on stm32, and this is needed for that to work. I should put the slave PR up before this one...

Can SDO and SDI pins work in either direction? if not, it would be good to validate the SDO and SDI versus RX and TX mode.

Yes, SDO and SDI can be swapped to work for either in or out.

Is there any other way to code this besides putting a processor macro into the file. e.g use a feature support macro in mpconfigboard.h files? Sometimes I find that processor macros seem to grow in micropython files, which tend to undermine code comprehension. Not a big deal, and more of a question on my part to learn the best practice to follow in micropython.

I agree these things grow, but it's hard to make it clean. The HAL is supposed to help there, and it does to some extent.

What's going on here is that the H7 (and maybe G4?) support SDI and SDO pins, and they can be swapped. So instead of #if defined(STM32H7) it could be #if MCU_SUPPORTS_SDI_SDO (probably use a better name than that). Does that sound reasonable?

Thanks for explaining the issue. Maybe just leave it as-is, and then consider refactoring if other processor variant(s) drive the use of additional macro logic?

similar comment as above about processor macro vs feature macro

The H7 HAL just doesn't have this entry in the Init struct. So somehow need to deal with a HAL that is not 100% cross-MCU compatible.

Is this a bug fix or DMA optimization for 32-bit audio?

This is a bug fix! I'm surprised it works on other MCUs without this...

Can you describe how the bug manifests? I assume this bug was discovered during the H7 port. Thanks.

When using 32-bit RX (which is always the case, it's always 32-bit for incoming data from a mic) the values read by the DMA were always 0. And the DMA filled its buffer twice as fast. This makes sense because the DMA was doing only 16-bit reads from the I2S data register, and two such reads are needed per 32-bit sample.

Also I'm pretty sure 32-bit TX was partially broken. It did make sound on the speaker I used for testing, but it made "better" sound when I fixed this bug.

I think you are right, I think the original code is correct for F4 and F7.

It's on H7 that the I2S data register (which now has separate TXDR and RXDR registers) behaves differently.

That makes sense to me now.

Here is an implementation idea: An alternative to overriding the dma data width sizes in the dma init struct (eg dma_init_struct_i2s in dma.c) with the new function dma_init_with_size() is to modify the init structs in dma.c.

For example:
const dma_descr_t dma_I2S_2_TX = { DMA1_Stream3, DMA_CHANNEL_0, dma_id_3, &dma_init_struct_i2s };

would change to:
const dma_descr_t dma_I2S_2_TX_16 = { DMA1_Stream3, DMA_CHANNEL_0, dma_id_3, &dma_init_struct_i2s_16 };
and
const dma_descr_t dma_I2S_2_TX_32 = { DMA1_Stream3, DMA_CHANNEL_0, dma_id_3, &dma_init_struct_i2s_32 };

There would be separate init structs for 16-bit vs 32-bit sample sizes. These init structs would have macros for F4/F7/H7/etc to conditionally compile for uniqueness in the processor families, as it does now.

in machine_i2s.c, the code that looks like this:

        // configure DMA streams
        if (self->mode == I2S_MODE_MASTER_RX) {
            self->dma_descr_rx = &dma_I2S_1_RX;
        } else {
            self->dma_descr_tx = &dma_I2S_1_TX;
        }

would change to:

        // configure DMA streams
        if (self->mode == I2S_MODE_MASTER_TX && get_dma_bits(self->mode, self->bits) ==16  ) {
            self->dma_descr_rx = &dma_I2S_1_TX_16;
        } else if  (self->mode == I2S_MODE_MASTER_TX && get_dma_bits(self->mode, self->bits) ==32  ){
            self->dma_descr_tx = &dma_I2S_1_TX_32;
        } else if RX {
            self->dma_descr_rx = &dma_I2S_1_RX_32;    --- always 32 bits
        }

If this works, it has the advantage of keeping all the dma data width information in one place, dma.c -- which might aid in self-documenting the code.

Another consideration for the H7 are the transformations that happen in machine_i2s.c code:

1. transform for 32-bit sample TX:

   micropython/ports/stm32/machine_i2s.c

   Line 246 in 2ea21ab

   STATIC void reformat_32_bit_samples(int32_t *sample, uint32_t num_samples) {

2. "cherry-picking" transform for RX:

   micropython/ports/stm32/machine_i2s.c

   Line 308 in 2ea21ab

   int8_t r_to_a_mapping = i2s_frame_map[f_index][i];

You mentioned that master TX has been tested, so that should eliminate item 1 as a concern. Hopefully the transform in item 2 will work as-is for the H7.

Yes I like your idea about having multiple DMA descriptor structs, for 16 and 32 bit transfers. I'll try that out.

And will test again all combinations of TX, RX and bit size on H7.

I changed the DMA config so it uses separate 16 and 32 bit descriptor structs.

And I tested the byte transformations, they were wrong on the H7, and are now fixed (at least for 32-bit stereo).