Skip to content
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

F4_HAL/uart: Fix data loss when using DMA Rx with hardware flow control. #12

Conversation

dlech
Copy link

@dlech dlech commented Dec 6, 2020

Data loss can occur when using DMA Rx + hardware flow control.

Consider the following situation where a device is receiving UART data that consists of one byte that gives the size of the data followed by "size" bytes:

  • MCU requests to read one byte from UART by calling HAL_UART_Receive_IT()
  • The remote may or may not have already sent the byte, but it doesn't matter, this works correctly.
  • After the UART_Receive_IT() callback, the MCU requests to read "size" bytes by calling HAL_UART_Receive_DMA().
  • If the remote has not sent the next byte yet, then all is well, but it could have sent the next byte already, which is waiting in the DR register. The RTS line will be high which prevents the remote from sending any more data, so no overrun will occur. But since __HAL_UART_CLEAR_OREFLAG() reads the DR register, this byte will be lost and the DMA read will read one extra byte from the incoming stream causing the algorithm to get out of sync.

This adds a check to see if flow control is enabled and only calls __HAL_UART_CLEAR_OREFLAG() if flow control is disabled. This should prevent breaking users who aren't using flow control and may already depend on this behavior.

@dlech
Copy link
Author

dlech commented Dec 6, 2020

FYI, this is a variation of part of the fixes from bluekitchen/btstack@bef4d37. Disabling IRQs is probably also needed, but I'm not sure the way it is done in the linked commit won't cause problems for existing users of MicroPython (i.e. interrupts may already be disabled).

@dpgeorge
Copy link
Member

dpgeorge commented Dec 7, 2020

This looks OK to me. I assume you have tested it and it fixed the issue with lost chars on DMA RX?

Disabling IRQs is probably also needed

All F4 MCUs have a bit-band region (every bit is assigned a 32-bit word alias), and this can be used to make atomic changes to bits in peripheral registers. I think this would be a good way to solve that issue of SET_BIT not being atomic.

Eg:

#define PERIPH_BB_REG_ADDR(reg, bit_pos) (__IO uint32_t *)(PERIPH_BB_BASE + ((uintptr_t)&(reg) - PERIPH_BASE) * 32 + (bit_pos) * 4)
#define SET_BIT_PERIPH_BB(reg, bit_pos) (*(PERIPH_BB_REG_ADDR((reg), (bit_pos)) = 1)

{
    ...
    SET_BIT_PERIPH_BB(huart->Instance->CR1, USART_CR1_PEIE_Pos);
    ...
}

Note: the above macros are untested!

@dpgeorge
Copy link
Member

dpgeorge commented Dec 7, 2020

Also, can you please retarget this to the new working branch work-F0-1.9.0+F4-1.16.0+F7-1.7.0+H7-1.6.0+L0-1.11.2+L4-1.8.1+WB-1.10.0 ?

dlech added 2 commits March 12, 2021 13:07
Data loss can occur when using DMA Rx + hardware flow control.

Consider the following situation where a device is receiving UART
data that consists of one byte that gives the size of the data followed
by "size" bytes:

- MCU requests to read one byte from UART by calling HAL_UART_Receive_IT()
- The remote may or may not have already sent the byte, but it doesn't
  matter, this works correctly.
- After the UART_Receive_IT() callback, the MCU requests to read "size"
  bytes by calling HAL_UART_Receive_DMA().
- If the remote has not sent the next byte yet, then all is well, but
  it could have sent the next byte already, which is waiting in the DR
  register. The RTS line will be high which prevents the remote from
  sending any more data, so no overrun will occur. But since
  __HAL_UART_CLEAR_OREFLAG() reads the DR register, this byte will be
  lost and the DMA read will read one extra byte from the incoming
  stream causing the algorithm to get out of sync.

This adds a check to see if flow control is enabled and only calls
__HAL_UART_CLEAR_OREFLAG() if flow control is disabled. This should
prevent breaking users who aren't using flow control and may already
depend on this behavior.
This changes writes to the CR1 and CR3 registers to use bit-band access
to atomically modify the registers. This is needed since the interrupt
handlers also modify these same registers and an interrupt could occur
in the time between when the registers is read and when the modify value
is written back.

This change is not made in the init functions since interrupts will not
be enabled yet and it is more efficient to set more than one bit at a
time.
@dlech dlech force-pushed the f4-hal-uart-dma-rx-rts branch from 9cc7d02 to 74cc9b3 Compare March 12, 2021 21:00
@dlech dlech changed the base branch from work-F0-1.9.0+F4-1.16.0+F7-1.7.0+H7-1.6.0+L0-1.11.2+L4-1.8.1+WB-1.6.0 to work-F0-1.9.0+F4-1.16.0+F7-1.7.0+G4-1.3.0+H7-1.6.0+L0-1.11.2+L4-1.8.1+WB-1.10.0 March 12, 2021 21:01
@dlech
Copy link
Author

dlech commented Mar 12, 2021

Hi @dpgeorge. I have rebased and added the suggested bit-band macros. This has been tested and confirmed to fix the DMA issue.

dlech added a commit to pybricks/micropython that referenced this pull request Mar 12, 2021
This pulls in fixes to the MicroPython stm32lib needed for BTStack.

This patch can be dropped once micropython/stm32lib#12 is merged.
dlech added a commit to pybricks/micropython that referenced this pull request Apr 26, 2021
This pulls in fixes to the MicroPython stm32lib needed for BTStack.

This patch can be dropped once micropython/stm32lib#12 is merged.
dlech added a commit to pybricks/micropython that referenced this pull request Jun 24, 2021
This pulls in fixes to the MicroPython stm32lib needed for BTStack.

This patch can be dropped once micropython/stm32lib#12 is merged.
dlech added a commit to pybricks/micropython that referenced this pull request Sep 1, 2021
This pulls in fixes to the MicroPython stm32lib needed for BTStack.

This patch can be dropped once micropython/stm32lib#12 is merged.
laurensvalk pushed a commit to pybricks/micropython that referenced this pull request Oct 27, 2021
This pulls in fixes to the MicroPython stm32lib needed for BTStack.

This patch can be dropped once micropython/stm32lib#12 is merged.
@dpgeorge dpgeorge merged commit 74cc9b3 into micropython:work-F0-1.9.0+F4-1.16.0+F7-1.7.0+G4-1.3.0+H7-1.6.0+L0-1.11.2+L4-1.8.1+WB-1.10.0 Nov 26, 2021
@dpgeorge
Copy link
Member

Sorry this got lost, now merged.

@dlech dlech deleted the f4-hal-uart-dma-rx-rts branch November 26, 2021 16:26
laurensvalk pushed a commit to pybricks/micropython that referenced this pull request Dec 23, 2021
This pulls in fixes to the MicroPython stm32lib needed for BTStack.

This patch can be dropped once micropython/stm32lib#12 is merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants