stm32/machine_i2s: Add support for I2S on H7 MCUs (WIP) #8270

dpgeorge · 2022-02-07T13:39:24Z

Master TX is tested, in blocking and non-blocking IO mode.

To do:

test master RX mode
test asyncio mode

(Note: this supports I2S using the SPI peripheral. More sophisticated I2S support can be achieved using the SAI peripheral, but that's much more involved.)

miketeachman

Great that I2S is getting ported to another STM32 processor! I have a few comments and questions about the proposed changes.

miketeachman · 2022-02-09T04:27:17Z

ports/stm32/machine_i2s.c

+        const pin_af_obj_t *pin_af = pin_find_af(self->sd, AF_FN_I2S, self->i2s_id);
+        if (pin_af != NULL) {
+            if ((pin_af->type == AF_PIN_TYPE_I2S_SDI
+                    && (self->mode == I2S_MODE_MASTER_RX || self->mode == I2S_MODE_SLAVE_RX))


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...

miketeachman · 2022-02-09T04:31:13Z

ports/stm32/machine_i2s.c

+        if (!(pin_af != NULL && (
+            pin_af->type == AF_PIN_TYPE_I2S_SD
+            #if defined(STM32H7)
+            || pin_af->type == AF_PIN_TYPE_I2S_SDO || pin_af->type == AF_PIN_TYPE_I2S_SDI


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.

miketeachman · 2022-02-09T04:37:34Z

ports/stm32/machine_i2s.c

@@ -566,6 +565,21 @@ STATIC bool i2s_init(machine_i2s_obj_t *self) {
        const pin_af_obj_t *af = pin_find_af(self->sd, AF_FN_I2S, self->i2s_id);
        GPIO_InitStructure.Alternate = (uint8_t)af->idx;
        HAL_GPIO_Init(self->sd->gpio, &GPIO_InitStructure);
+
+        #if defined(STM32H7)


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?

miketeachman · 2022-02-09T04:38:20Z

ports/stm32/machine_i2s.c

@@ -790,7 +811,9 @@ STATIC void machine_i2s_init_helper(machine_i2s_obj_t *self, size_t n_pos_args,
    init->MCLKOutput = I2S_MCLKOUTPUT_DISABLE;
    init->AudioFreq = args[ARG_rate].u_int;
    init->CPOL = I2S_CPOL_LOW;
+    #if !defined(STM32H7)


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.

miketeachman · 2022-02-09T04:39:58Z

ports/stm32/machine_i2s.c

@@ -584,13 +584,19 @@ STATIC bool i2s_init(machine_i2s_obj_t *self) {

    if (HAL_I2S_Init(&self->hi2s) == HAL_OK) {
        // Reset and initialize Tx and Rx DMA channels
+        uint32_t pm_size;
+        if (self->hi2s.Init.DataFormat == I2S_DATAFORMAT_16B) {


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:

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) {

"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).

dpgeorge · 2022-03-25T03:44:18Z

See #8451 for passive I2S support. This PR would need to be rebased/reworked on top of that, and the multitest can then be used to test things.

dpgeorge · 2022-03-25T07:17:32Z

Rebased on #8451. Active and passive TX and RX now work on H7, but only 32-bit stereo has been tested.

Signed-off-by: Damien George <damien@micropython.org>

In particular, it is called by the constructor if the instance already exists. So if the previous instance was deinit'd then it will be deinit'd a second time. Signed-off-by: Damien George <damien@micropython.org>

Signed-off-by: Damien George <damien@micropython.org>

Master TX is tested, in blocking and non-blocking IO mode. Signed-off-by: Damien George <damien@micropython.org>

Signed-off-by: Damien George <damien@micropython.org>

Better alphablend features

projectgus · 2024-03-07T23:52:13Z

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

dpgeorge added the port-stm32 label Feb 7, 2022

miketeachman reviewed Feb 9, 2022

View reviewed changes

dpgeorge force-pushed the stm32-i2s-h7 branch from c02cf4c to 9762766 Compare March 25, 2022 07:16

dpgeorge added 8 commits March 26, 2022 00:09

stm32/machine_i2s: Support I2S passive mode.

fbbe29b

Signed-off-by: Damien George <damien@micropython.org>

stm32/machine_i2s: Set FullDuplexMode to disabled on F4.

f4afbff

Signed-off-by: Damien George <damien@micropython.org>

stm32/machine_i2s: Allow I2S.deinit to be called multiple times.

86eb6bd

In particular, it is called by the constructor if the instance already exists. So if the previous instance was deinit'd then it will be deinit'd a second time. Signed-off-by: Damien George <damien@micropython.org>

tests/multi_machine: Add a multi-test for machine.I2S.

6c41da4

Signed-off-by: Damien George <damien@micropython.org>

stm32/machine_i2s: Fix 16-bit stereo i2s_frame_map.

3c1ef0f

Signed-off-by: Damien George <damien@micropython.org>

stm32/machine_i2s: Add support for I2S on H7 MCUs.

7a84fc4

Master TX is tested, in blocking and non-blocking IO mode. Signed-off-by: Damien George <damien@micropython.org>

stm32/dma: Provide 16 and 32 bit I2S DMA structs.

6c6f15c

Signed-off-by: Damien George <damien@micropython.org>

stm32/machine_i2s: Correctly set the DMA transfer size.

9c9a930

Signed-off-by: Damien George <damien@micropython.org>

dpgeorge force-pushed the stm32-i2s-h7 branch from 9762766 to 9c9a930 Compare March 25, 2022 13:13

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Aug 17, 2023

Merge pull request micropython#8270 from makermelissa/better-alphablend

d0f313c

Better alphablend features

Gadgetoid mentioned this pull request Feb 29, 2024

global: Remove the STATIC macro. #13763

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stm32/machine_i2s: Add support for I2S on H7 MCUs (WIP) #8270

stm32/machine_i2s: Add support for I2S on H7 MCUs (WIP) #8270

dpgeorge commented Feb 7, 2022 •

edited

Loading

miketeachman left a comment

miketeachman Feb 9, 2022

dpgeorge Feb 9, 2022

miketeachman Feb 9, 2022 •

edited

Loading

dpgeorge Feb 9, 2022

miketeachman Feb 9, 2022

dpgeorge Feb 9, 2022

miketeachman Feb 10, 2022

miketeachman Feb 9, 2022

dpgeorge Feb 9, 2022

miketeachman Feb 9, 2022

dpgeorge Feb 9, 2022

miketeachman Feb 10, 2022

dpgeorge Feb 10, 2022

dpgeorge Feb 10, 2022

dpgeorge Feb 12, 2022

miketeachman Feb 12, 2022

miketeachman Feb 12, 2022

dpgeorge Feb 14, 2022

dpgeorge Mar 25, 2022

dpgeorge commented Mar 25, 2022

dpgeorge commented Mar 25, 2022

projectgus commented Mar 7, 2024

stm32/machine_i2s: Add support for I2S on H7 MCUs (WIP) #8270

Are you sure you want to change the base?

stm32/machine_i2s: Add support for I2S on H7 MCUs (WIP) #8270

Conversation

dpgeorge commented Feb 7, 2022 • edited Loading

miketeachman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miketeachman Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgeorge commented Mar 25, 2022

dpgeorge commented Mar 25, 2022

projectgus commented Mar 7, 2024

dpgeorge commented Feb 7, 2022 •

edited

Loading

miketeachman Feb 9, 2022 •

edited

Loading