New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STM32: Fix usart irq index #5962

Merged
merged 18 commits into from Jan 31, 2018

Conversation

Projects
None yet
5 participants
@bcostm
Contributor

bcostm commented Jan 29, 2018

Description

  • There was an issue with the serial irq index used in the STM32 serial api implementation. On some platforms the indexes was wrong and this results to wrong usart access.
  • The issue has been flagged in #5916 and #5931
  • A fix has been found by @marcemmers (thanks again!) and done for STM32L0 devices in PR #5917. But this PR has been closed due to this one covering all STM32 devices.

Status

READY

Migrations

NO

Todos

  • Tests

@marcemmers can you please test it in your application for the STM32L0 ?

irq_handler(serial_irq_ids[id], TxIrq);
int8_t id = get_uart_index(uart_name);
if (id >= 0) {

This comment has been minimized.

@0xc0170

0xc0170 Jan 29, 2018

Member

what happens if uart_name is out of defined BASES ? I can see that get uart index returns -1 if not there, but this is not handled here, can it happen?

This comment has been minimized.

@bcostm

bcostm Jan 29, 2018

Contributor

If we have well written all the usarts available in the device it should not happen... Before this it was worse because in some case we could access out of the arrays (uart_handlers[id] and serial_irq_ids[id]).

This comment has been minimized.

@bcostm

bcostm Jan 31, 2018

Contributor

An assert has been added in commit b6efdd5 to check this index.

@@ -92,43 +101,43 @@ static void uart3_8_irq(void)
#if defined(TARGET_STM32F091RC)

This comment has been minimized.

@marcemmers

marcemmers Jan 29, 2018

Contributor

I was wondering if the target necessary defines were still necessary? Haven't looked at the specific processor yet.

This comment has been minimized.

@bcostm

bcostm Jan 29, 2018

Contributor

yes it is required because the IRQ vector name is different and also the __HAL_GET_PENDING_IT macro is available only on F09x devices

@marcemmers

This comment has been minimized.

Contributor

marcemmers commented Jan 29, 2018

I will test on the L073, will let you know what i find.

@marcemmers

This comment has been minimized.

Contributor

marcemmers commented Jan 29, 2018

This worked fine for me on the L073RZ-Nucleo. Have tested all available uarts both on 9600 and 115k.

Did notice this however:

// Warning: the list of UART/USART in this function must be aligned with the list
// written in serial_init function.
int8_t get_uart_index(UARTName uart_name)

Why not remove the increment numbering from serial_init and also use get_uart_index instead? So:

#if defined(USART1_BASE)
    if (obj_s->uart == UART_1) {
        __HAL_RCC_USART1_FORCE_RESET();
        __HAL_RCC_USART1_RELEASE_RESET();
        __HAL_RCC_USART1_CLK_ENABLE();
    }
#endif

    obj_s->index = get_uart_index(obj_s->uart);    
    MBED_ASSERT(obj_s->index >= 0);

instead of

#if defined(USART1_BASE)
    if (obj_s->uart == UART_1) {
        __HAL_RCC_USART1_FORCE_RESET();
        __HAL_RCC_USART1_RELEASE_RESET();
        __HAL_RCC_USART1_CLK_ENABLE();
        obj_s->index = IndexNumber;
    }
    IndexNumber++;
#endif
@bcostm

This comment has been minimized.

Contributor

bcostm commented Jan 29, 2018

Yep I think you're right and it should work. It's even better that way ! 👍

I'll send a new commit with your proposal.

And the assert adresses this remark:

what happens if uart_name is out of defined BASES ? I can see that get uart index returns -1 if not there, but this is not handled here, can it happen?

@bcostm bcostm force-pushed the bcostm:fix_usart_irq_index branch from aa7f519 to b6efdd5 Jan 29, 2018

@0xc0170 0xc0170 added needs: work and removed needs: review labels Jan 30, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 30, 2018

@bcostm Please check travis failures

@bcostm

This comment has been minimized.

Contributor

bcostm commented Jan 30, 2018

Should be ok now with commit eb4b339

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 30, 2018

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Jan 30, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 30, 2018

Build : SUCCESS

Build number : 1009
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5962/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 31, 2018

/morph uvisor-test

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jan 31, 2018

@cmonr cmonr merged commit f907012 into ARMmbed:master Jan 31, 2018

19 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@bcostm bcostm deleted the bcostm:fix_usart_irq_index branch Mar 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment