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

Avoid data loss with serial interrupt used at high baudrates #4734

Merged
merged 2 commits into from Jul 24, 2017

Conversation

Projects
None yet
7 participants
@LMESTM
Contributor

LMESTM commented Jul 11, 2017

Description

There was a hidden bug in the uart irq handler that was seen when reaching high baud rates as described in details in #4722 . The RXE flags shouldn't be erased in the uart_irq as erasing the flag is actually done when reading the data in the data register. At low baudrate, this has usually not effect, but at igh baud rates, this can actually erase a new newly arrived data, not yet read by application. This would cause data loss at application layer.

The 2nd commit of this PR consist in using TXE (room in byte FIFO for posting next data) flag instead of TC (transmission complete), in order to improve overall performance. Also this is now in line with TxIrq definition from SerialBAse.h : TxIrq for transmit buffer empty

Status

READY

Related PRs

Dependency to #4707

  • The #4707 needs to be merged before this PR (which will need rebase) => OK, MERGED

Testing

  • Tests
    This has been tested by @RobMeades in the scope of #4722
    Unfortunately I could not find mbed tests for checking serial interrupt - this would be good enhancement on test side.

@LMESTM LMESTM changed the title from Do not erwase flasgs in stm32 uart irq handler to Do not erase flags in stm32 uart irq handler Jul 11, 2017

@LMESTM LMESTM changed the title from Do not erase flags in stm32 uart irq handler to Avoid data loss with serial interrupt used at high baudrates Jul 11, 2017

@0xc0170 0xc0170 added the needs: CI label Jul 11, 2017

@kjbracey-arm

Seems correct to me. Only thing that makes me nervous is the question "what if the IRQ handler doesn't transfer data"?

Previously (at least on this platform) they could have gotten away with just not reading - it would have meant the data being thrown away. Or they could have gotten away with not writing - the transmit empty interrupt would clear and not retrigger. The UARTSerial IRQ handlers take great care to try to clear the interrupt condition, but other users might not.

So could break some ST-specific applications, but any portable applications should be correctly clearing already.

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Jul 13, 2017

I agree that possible application that were not enabling / disabling interrupts on a need basis may encounter issues and will need update, not sure how to avoid this risk :-(

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 13, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 13, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 14, 2017

@LMESTM Can you please resolve the conflicts with a rebase?

LMESTM added some commits Jul 10, 2017

STM32: Serial - do no clear RXNE flag
The RXNE flag is getting cleared when reading Data Register so it should
not be cleared here. Especially in case of high data rate, another byte of
data could have received during irq_handler call and clearing the flag
would read and discard this data which would be lost for application.
STM32: Serial - use TXE as tx_irq instead of TC
TXE indicates that a byte can be written to UART register for sending,
while TC indicates that last byte was completely sent. So the TXE flag
can be used in case of interrupt based Serial communication, to allow
faster and efficient application buffer emptying.

Also TXE flag will be erased from the interrupt when writing to register.
In case there is nothing to write in the register, the application is
expected to disable the interrupt.

@LMESTM LMESTM force-pushed the LMESTM:stm32_uart_irq branch to b5cbaaa Jul 17, 2017

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Jul 17, 2017

@0xc0170 - rebased

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 17, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 17, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 820

Test failed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Jul 17, 2017

tests-mbedmicro-rtos-mbed-signals,tests-mbedmicro-rtos-mbed-mail failed in connection process.

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 18, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 824

All builds and test passed!

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Jul 19, 2017

@0xc0170 please don't merge - this needs rework as suggested in #4780
hmm - after sutying the remarks, I don't think there is a need for rework after all - see #4780 (comment)

Waiting for confirmation from @betzw anyway

@0xc0170 0xc0170 added needs: work and removed ready for merge labels Jul 20, 2017

@betzw

This comment has been minimized.

Contributor

betzw commented Jul 21, 2017

OK for me!

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Jul 21, 2017

@betzw Thanks for your quick feedback !

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Jul 21, 2017

@0xc0170 thanks for waiting - nothing has changed, so this can be moved back to ready_for_merge :-)

@0xc0170 0xc0170 added ready for merge and removed needs: work labels Jul 21, 2017

@theotherjimmy theotherjimmy merged commit 9e443e9 into ARMmbed:master Jul 24, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment