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.

Sign up for GitHub

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

Jump to bottom

Tri-state USART TX output if load due to powered down peripheral is detected #12760

Merged
merged 1 commit into from May 10, 2023
Merged

Tri-state USART TX output if load due to powered down peripheral is detected #12760

merged 1 commit into from May 10, 2023

Conversation

SteveCEvans
Copy link
Member

Fixes: #12723

This PR monitors the state of the TX line when full duplex connections are used. When no data is being transmitted the TX line is set to be an input with pullup and when new data is to be transmitted the line is checked. It should always be high unless it is being excessively loaded. If the line is found to be pulled low, it is left tri-stated and the data discarded.

Note:
This build sets debug[0] to debug[3] to indicate the status of UARTs 1 - 4 respectively.

Value Meaning
1 Normal Operation
2 Powered down peripheral detected

This debug will be removed once fix is tested/proven.

@SteveCEvans SteveCEvans added this to the 4.5 milestone May 3, 2023
@SteveCEvans SteveCEvans self-assigned this May 3, 2023
@github-actions

This comment has been minimized.

Sign in to view
@blckmn
Copy link
Member

blckmn commented May 4, 2023 •
edited

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> FAIL
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

@SteveCEvans
Copy link
Member Author

GPS unit on port 4 isn't working with this change... investigating.

@SteveCEvans
Copy link
Member Author

This is due to the TX output being disabled using the UART_IT_TXE interrupt rather than UART_IT_TC interrupt which happens before the data has been fully transmitted.

ledvinap
ledvinap approved these changes May 5, 2023
View reviewed changes
Copy link
Contributor

@ledvinap ledvinap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • this PR may break hardware with pulldown on TX pin
  • it is implemented very low level, overhed will be non-trivial
  • it can be optimized if UART supports write(void*, len) semantics consistently

src/main/drivers/at32/serial_uart_at32bsp.c Outdated Show resolved Hide resolved
src/main/drivers/serial_uart.c
@@ -267,6 +267,12 @@ static void uartWrite(serialPort_t *instance, uint8_t ch)
{
uartPort_t *uartPort = (uartPort_t *)instance;

// Check if the TX line is being pulled low by an unpowered peripheral
if (uartPort->checkUsartTxOutput && !uartPort->checkUsartTxOutput(uartPort)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is called for every byte, so may be worth optimizing - possibly by testing uart->txPinState == TX_PIN_MONITOR first

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also used for VCP in which case uartPort->checkUsartTxOutput won't be defined and uart->txPinState won't be valid.

I think that a better optimisation of the serial code will be to remove the DMA support which isn't used, but has many checks being performed.

@SteveCEvans
Copy link
Member Author

GPS issue resolved. Tested and working on STM32 and AT32 processors. Debug now removed.

@SteveCEvans SteveCEvans force-pushed the uart_tx_powerdown branch 2 times, most recently from 79ee004 to 3d9174c Compare May 5, 2023 19:53
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link

github-actions bot commented May 5, 2023

Do you want to test this code? Here you have an automated build:
Assets
WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

@sugaarK sugaarK merged commit 4dc04d6 into betaflight:master May 10, 2023
19 checks passed
haslinghuis pushed a commit to haslinghuis/betaflight that referenced this pull request May 10, 2023
@SteveCEvans @haslinghuis
Tri-state USART TX output if load due to powered down peripheral is d…
af8124d 
…etected (betaflight#12760)

Tri-state USART TX output if loaded due to powered down peripheral being detected
@haslinghuis haslinghuis mentioned this pull request May 10, 2023
Merged
@SteveCEvans
Copy link
Member Author

For the record, see below evidence of the timing of MSP traffic being sent to a WS VTX. The top trace is high when data is being transmitted, and it can be seen that updates are occurring at the expected 12Hz.

uartTxMonitor() is only being called once transmission of a block of data has completed, so the transmission completion interrupt is only occurring infrequently and not adding significant load to the interrupt handling.

image

haslinghuis added a commit that referenced this pull request May 11, 2023
@haslinghuis @SteveCEvans
[4.4.2] Tri-state USART TX output if load due to powered down periphe…
ca12a4c 
…ral is d… (#12782)

Tri-state USART TX output if load due to powered down peripheral is detected (#12760)

Tri-state USART TX output if loaded due to powered down peripheral being detected

Co-authored-by: Steve Evans <SteveCEvans@users.noreply.github.com>
@TheIsotopes TheIsotopes mentioned this pull request May 12, 2023
Closed
@SteveCEvans SteveCEvans mentioned this pull request May 13, 2023
Closed
AkankshaJjw pushed a commit to AkankshaJjw/betaflight that referenced this pull request May 29, 2023
@SteveCEvans @AkankshaJjw
Tri-state USART TX output if load due to powered down peripheral is d…
5906065 
…etected (betaflight#12760)

Tri-state USART TX output if loaded due to powered down peripheral being detected
@SteveCEvans SteveCEvans mentioned this pull request Jun 26, 2023
Merged
haslinghuis added a commit to haslinghuis/betaflight that referenced this pull request Sep 25, 2023
@haslinghuis
Revert "Tri-state USART TX output if load due to powered down periphe…
3354870 
…ral is detected (betaflight#12760)"

This reverts commit 4dc04d6.
haslinghuis added a commit to haslinghuis/betaflight that referenced this pull request Sep 26, 2023
@haslinghuis
Revert "Tri-state USART TX output if load due to powered down periphe…
5ed45d7 
…ral is detected (betaflight#12760)"

This reverts commit 4dc04d6.
@ledvinap ledvinap mentioned this pull request Sep 27, 2023
Closed
davidbitton pushed a commit to davidbitton/betaflight that referenced this pull request Feb 5, 2024
@SteveCEvans @davidbitton
Tri-state USART TX output if load due to powered down peripheral is d…
5c48e7b 
…etected (betaflight#12760)

Tri-state USART TX output if loaded due to powered down peripheral being detected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ledvinap ledvinap ledvinap approved these changes

@haslinghuis haslinghuis haslinghuis approved these changes

@ctzsnooze ctzsnooze ctzsnooze approved these changes

@sugaarK sugaarK sugaarK approved these changes

@blckmn blckmn Awaiting requested review from blckmn

@KarateBrot KarateBrot Awaiting requested review from KarateBrot

@klutvott123 klutvott123 Awaiting requested review from klutvott123

Assignees

@SteveCEvans SteveCEvans

Labels
RN: BUGFIX
Projects
Finalizing Firmware 4.5 Release
Status: COMPLETED
Milestone
4.5
Development

Successfully merging this pull request may close these issues.

Sending data to unpowered HDZero VTX causes FC to crash to DFU mode.
6 participants
@SteveCEvans @blckmn @ledvinap @haslinghuis @ctzsnooze @sugaarK