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

stm32: fix buffered uart flush #2416

Merged
merged 5 commits into from
Jan 20, 2024

Conversation

andresv
Copy link
Contributor

@andresv andresv commented Jan 8, 2024

usart_v4 uses internal FIFO and therefore actually all bytes are not yet sent out although state.tx_buf.is_empty().

Code to demonstrate the problem:

let t = Mono::now();

pin.set_high();
rs485_tx.write_all(&tx_data).await.ok();
rs485_tx.flush().await.ok();
pin.set_low();

let diff = (Mono::now() - t).to_micros();
defmt::info!("tx {} us", diff);

Here red line shows pin. So flush finishes although in reality 9 bytes were not yet sent out from the FIFO.
Screenshot 2024-01-08 at 14 48 33

With this patch flush finishes right after TX is done.
Screenshot 2024-01-08 at 17 36 21

STM32G0 reference manual

Page 1008:

  1. Write the data to send in the USART_TDR register. Repeat this for each data to be transmitted in case of single buffer.
  • When FIFO mode is disabled, writing a data to the USART_TDR clears the TXE flag.
  • When FIFO mode is enabled, writing a data to the USART_TDR adds one data to the TXFIFO. Write operations to the USART_TDR are performed when TXFNF flag is set. This flag remains set until the TXFIFO is full.
  1. When the last data is written to the USART_TDR register, wait until TC = 1.
  • When FIFO mode is disabled, this indicates that the transmission of the last frame is complete.
  • When FIFO mode is enabled, this indicates that both TXFIFO and shift register are empty.

TODO

  • Currently flush would return 1 byte time too early for non usart_v4 implementation. Final byte is written to register and after that flush can see that tx_buf is empty and it returns Ready although actually final bytes is still being written to the wire.
  • Test on a board that uses usart_v4 (STM32G081).
  • Test on a board that uses usart_v2 (STM32F411).
  • Figure out why on STM32F411 with first transfer TC flag is set right after first byte is written to DR. Consecutive transfers works as expected. 0a6f4c9

@andresv
Copy link
Contributor Author

andresv commented Jan 9, 2024

@Dirbaio test fails and by clicking I got:

run failed: Failed to get repo token: could not refresh installation id 37913974's token: received non 2xx response status "500 Internal Server Error" when fetching https://api.github.com/app/installations/37913974/access_tokens

@Dirbaio
Copy link
Member

Dirbaio commented Jan 10, 2024

bender run

@andresv andresv force-pushed the stm32-fix-buffered-uart-flush branch from 2d7ac24 to ded2bc0 Compare January 10, 2024 07:53
@andresv andresv marked this pull request as draft January 19, 2024 10:39
@andresv andresv force-pushed the stm32-fix-buffered-uart-flush branch from ded2bc0 to 61439a8 Compare January 19, 2024 13:26
@jamesmunns
Copy link
Contributor

This is off topic, but just noting embassy-rp has the same issue: https://github.com/embassy-rs/embassy/blob/main/embassy-rp/src/uart/buffered.rs#L381-L391

There's no interrupt based way to tell when the sending is done, you have to busy poll the "tx idle" flag to determine when the FIFO is really empty.

@andresv
Copy link
Contributor Author

andresv commented Jan 19, 2024

I started to test my fix on STM32F411 and it does not work in there. It seems TC flag is not cleared.

RM says this about TC:

It is cleared by a software sequence (a read from the USART_SR register followed by a write to the USART_DR register). The TC bit can also be cleared by writing a '0' to it. This clearing sequence is recommended only for multibuffer communication.

I am currently figuring out how to do it properly.

@andresv andresv marked this pull request as ready for review January 19, 2024 18:31
usart_v4 uses internal FIFO and therefore actually all bytes are not yet sent out although state.tx_buf.is_empty()
usart_v4 uses TC interrupt to see if all bytes are sent out from the FIFO and waker is called from this interrupt. This minimises unnecessary wakeups during sending.
Byte was written to TDR and right after that waker was called. This means `flush` would see that `tx_buf` is empty and can return Ready although actually hardware was still writing this last byte to the wire.
With this change non `usart_v4 ` variants would also use TC interrupt to check when last byte was sent out.
unwraps take more space because of panics
There is one caveat. For some reason with first send using usart_v1/usart_v2 TC flag appears right after first byte from buffer is written to DR. Consecutive transfers work as expected - TC flag appears when last byte is fully transferred to wire.
@Dirbaio Dirbaio force-pushed the stm32-fix-buffered-uart-flush branch from 0a6f4c9 to ec2e3de Compare January 19, 2024 23:15
@Dirbaio Dirbaio enabled auto-merge January 19, 2024 23:16
@Dirbaio Dirbaio added this pull request to the merge queue Jan 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 19, 2024
@Dirbaio Dirbaio added this pull request to the merge queue Jan 20, 2024
@Dirbaio Dirbaio removed this pull request from the merge queue due to a manual request Jan 20, 2024
@Dirbaio Dirbaio added this pull request to the merge queue Jan 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 20, 2024
@Dirbaio Dirbaio added this pull request to the merge queue Jan 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 20, 2024
@Dirbaio Dirbaio added this pull request to the merge queue Jan 20, 2024
Merged via the queue into embassy-rs:main with commit 43b6258 Jan 20, 2024
@andresv andresv deleted the stm32-fix-buffered-uart-flush branch March 6, 2024 13:46
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.

3 participants