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

SoarProto v1.3.3 - UART Patch #10

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

SoarProto v1.3.3 - UART Patch #10

wants to merge 4 commits into from

Conversation

cjchanx
Copy link
Member

@cjchanx cjchanx commented Jul 26, 2023

Buggy HAL-ness has finally come to bite us... the optimal solution is to DMA it up with a wrapped version of https://github.com/MaJerle/stm32-usart-uart-dma-rx-tx

Based on: https://community.st.com/t5/stm32-mcu-products/stm32-uart-interrupt-stops-working-after-1-hour/m-p/99650/highlight/true#M15016

Note: Not sure if we want to consider this edge case in the current patch, if we ever get two of the UART_INTERRUPT_ARM_FAIL events in a row somehow there's gonna be another error that causes a potentially infinite loop... though it shouldn't happen if HAL does some weirdness with partially arming the IT then there could be weird things.

It looks like decreasing the baudrate will at least help this patch with losing less byte(s). The HAL_UART_Transmit() polling does annoyingly lock it while modifying the handle, but at least it is sensible enough to unlock it before actually doing the blocking transfer.


How to get better UART?

Short-Term Improvements
One immediate improvement/solution that can be made is to have the UART Task handle re-arming the interrupt instead of the ProtocolTask. Since the UART Task is responsible for all polling transmit (it should be anyway), there is no chance for the lock to be acquired in that context, only downside is that it requires the UART Task to support it which involves individual board changes -- edit, since the lock is only in a very temporary section in HAL_UART_Transmit() this will just make the re-arm slower.

The short term improvement is to just decrease baud -- if used in conjunction with this patch --

--> The annoying to deal with improvement is probably removing the HAL lock, we specifically only allow one task to transmit and one task to receive on any one UART line, so as long as the driver separates Rx and Tx variables properly (which it looks like it mostly does) that'll solve all our problems too. The only concern is if HAL_Lock is actually useful in any other context on the system and the fact that any codegen will overwrite it.

Medium-Term Improvements
Switching UART transmit to HAL_UART_Transmit_IT will fix our problems since there won't be a task context holding the HAL lock, this does require a decent sized rework to the UART task though, unless we go about it a gross way by just blocking the task until the TxCplt is called, but if needed it's quite doable. -- edit: this would still trigger some limited cases, since the HAL_LOCK is still used in this context :(

Long-Term Improvements
Writing a superior UART driver based on wrapping https://github.com/MaJerle/stm32-usart-uart-dma-rx-tx in a reusable way

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.

1 participant