Skip to content

AP_Networking: make PPP much more robust #30274

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Jun 8, 2025

this fixes two key issues with PPP:

  • if we lost the link (eg. serial cable unplugged) then it did not properly restart the link when the cable is plugged back in

  • if using hardware flow control and the UART write buffer fills up then we would sleep with the TCPIP lock held, which means if the main thread tries to send an IPv4 packet then it would block on the TCPIP lock, and we would watchdog

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances PPP reliability by adding link keepalive, timeout-based restarts, lock-aware output handling, and gateway interface promotion.

  • Define LCP echo interval and failure threshold in lwIP options to auto-restart dead links
  • Implement PPP_LINK_TIMEOUT_MS, skip output during termination, and unlock TCPIP before sleeping in ppp_output_cb
  • Introduce netif_promote and refactor ppp_loop for timeout detection, PPPERR_PEERDEAD handling, and interface recreation

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
libraries/AP_Networking/config/lwipopts.h Added LCP_ECHOINTERVAL and LCP_MAXECHOFAILS to generate LCP echoes
libraries/AP_Networking/AP_Networking_PPP.cpp Added link timeout, conditional unlock in output, new status case, netif_promote, and rebuild logic in ppp_loop
Comments suppressed due to low confidence (1)

libraries/AP_Networking/AP_Networking_PPP.cpp:315

  • [nitpick] The idle timeout branch isn't covered by existing tests; consider adding a unit or integration test to verify that PPP negotiations restart after PPP_LINK_TIMEOUT_MS elapses without traffic.
} else if (now_ms - last_read_ms > PPP_LINK_TIMEOUT_MS) {

@rmackay9
Copy link
Contributor

rmackay9 commented Jun 8, 2025

Great to see this. Sorry, it's just so easy to request a review by Co-pilot

this fixes two key issues with PPP:

 - if we lost the link (eg. serial cable unplugged) then it did not
   properly restart the link when the cable is plugged back in

 - if using hardware flow control and the UART write buffer fills up
   then we would loop sleeping with the TCPIP lock held, which means if
   the main thread tries to send an IPv4 packet then it could block on
   the TCPIP lock, and we would watchdog. This is now limited to 1ms max
@tridge tridge force-pushed the pr-PPP-robustness branch from 8d61c36 to 498e3e0 Compare June 8, 2025 04:17
@timtuxworth
Copy link
Contributor

timtuxworth commented Jun 8, 2025

I haven't been able to demonstrate problems repeatably, but I've definitely experienced reliability issues with PPP, especially in cases of companion computers which can take longer to boot than the FC. I'll be looking forward to trying this, I have a couple of vehicles that I can test this one that are flyable without any changes.

This is great thanks @tridge !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants