STM32L0: fix uart irq index

[marcemmers]

Description

Issue flagged here: #5916

Status

READY

Migrations

NO

[mbed-ci]

Automatic CI verification build not done, please verify manually.

[0xc0170]

@ARMmbed/team-st-mcd Please review

[0xc0170]

@marcemmers Can you please sign https://os.mbed.com/contributor_agreement/?

[marcemmers]

@0xc0170 is it enough to connect my mbed account with signed agreement to this github account?

[bcostm]

Thanks for the PR.

I looked carefully in all L0 devices on mbed and here is the status concerning indexes. The indexes are chosen at serial object init inside the serial_api.c file common to all STM32 devices.

The status is the following:

For L072/L073 devices
USART1 = index 0
USART2 = index 1
USART4 = index 2
USART5 = index 3
LPUART1 = index 4

For L053 devices
USART1 = index 0
USART2 = index 1
LPUART1 = index 2

For L011/L031 devices
USART2 = index 0
LPUART1 = index 1

Looking at your PR, I see 2 problems:

• wrong USART2 index for L011/L031 which should be 0 instead of 1.
• wrong LPUART1 index for L011/L031 which should be 1 and for L053 should be 2.
  I think this could be managed using #if defined(TARTGET_xxx)

Then, there is another problem, related to USART4 and USART5 sharing the same IRQ vector for L072/L073 devices. See my fix in PR #5907. Basically you have to create a common function uart4_5_irq (instead of 2 functions uart4_irq and uart5_irq) which will call uart_irq(2) and uart_irq(3) sequentially. And you have also to assign the uart4_5_irq function to USART4/5 (vector = (uint32_t)&uart4_5_irq;)

[marcemmers]

That does explain the reason for the lpuart having index 2 assigned. It was probably tested on a L053 before and that worked just fine.

To be honest I don't really like the solution with the target defines because that might cause the same problems later on when adding new devices. In the newest commit I have tried to solve this by using the same numbering scheme as used in the general STM serial_api.c

The only thing I am not really sure of is if which option would be better, where the latter lets uart_irq() take care of which irq was the cause:

#if defined(USART4_BASE) || defined(USART5_BASE)
static void uart4_5_irq(void)
{
#if defined(USART4_BASE)
    if (USART4->ISR & (UART_FLAG_TXE | UART_FLAG_RXNE | UART_FLAG_ORE)) {
        uart_irq(USART4_BASE);
    }
#endif

#if defined(USART5_BASE)
    if (USART5->ISR & (UART_FLAG_TXE | UART_FLAG_RXNE | UART_FLAG_ORE)) {
        uart_irq(USART5_BASE);
    }
#endif
}
#endif

#if defined(USART4_BASE) || defined(USART5_BASE)
static void uart4_5_irq(void)
{
#if defined(USART4_BASE)
    uart_irq(USART4_BASE);
#endif
#if defined(USART5_BASE)
    uart_irq(USART5_BASE);
#endif
}
#endif

Edit: Also, there is a possible issue with the LPUART1 using the same irq vector as AES en RNG in some STM32L0xx microcontrollers. This is not solved in this scope.

[bcostm]

LGTM

To be honest I don't really like the solution with the target defines

Same for me. The less we add targets in the source files the better it is...

In the newest commit I have tried to solve this by using the same numbering scheme as used in the general STM serial_api.c

Yes it is a nicer way of doing it. The only drawback I can see is that there is an additional function call in the interrupt, and I don't know if it may have an impact on critical communications ?

The only thing I am not really sure of is if which option would be better

I think the first solution you chose looks a little bit faster to execute but not sure...

there is a possible issue with the LPUART1 using the same irq vector as AES en RNG

I looked in the RNG code and I don't see the use of interrupt. So it should be ok. But I agree that it could be a problem the day it will be available...

Another point: I think it would be nice to move the uart_pointer_get_index function you have created in a common file so that we can use it for ALL STM32 devices. But this can be done later.

[marcemmers]

I discussed the changes with a collegae of mine and he also mentioned the extra function call. This could be resolved by adding the inline keyword as long as the function is only used once.

The IAR compiler already does this by itself when optimization is set to high but i don't know about other compilers.

[bcostm]

FYI while working on refactoring the usart index, I noticed there is also an issue with the indexes in the serial_get_irq_n function...

[0xc0170]

can you align the new changes please (see https://docs.mbed.com/docs/mbed-os-handbook/en/latest/cont/code_style/)

[0xc0170]

@bcostm Are you happy with the changes as they are? It's not clear based on the conversation that this fixes the issue referenced above (#5916)

[marcemmers]

@0xc0170 AFAIK @bcostm is still working on fixing the issue for the whole STM line making this PR obsolete. See #5931

I will check the alignment more carefully next time.

[bcostm]

Yes I have finished to change the irq index for all other STM32 series. I am going to send my PR. But I didn't touch the L0. You can merge this PR.

[0xc0170]

Thank you for answers.

I'll close this PR (can reopen if needed)

[0xc0170]

You can merge this PR.

I missed this comment, I can reopen or you can cherry-pick this change and include it in the PR to fix all STM32? Let me know