Skip to content

Fix UdpRssiPack startup race in negotiated RX buffer setup#1135

Merged
bengineerd merged 4 commits into
pre-releasefrom
udp-rssi-startup-race
Mar 16, 2026
Merged

Fix UdpRssiPack startup race in negotiated RX buffer setup#1135
bengineerd merged 4 commits into
pre-releasefrom
udp-rssi-startup-race

Conversation

@bengineerd

Copy link
Copy Markdown
Contributor

Description
This branch replaces the fixed startup delay in pyrogue.protocols.UdpRssiPack with an explicit readiness-based flow for programming the UDP RX buffer count.

The underlying issue is that RSSI parameter negotiation completes asynchronously, and only the client side was waiting for link-open before using curMaxBuffers(). That meant the server side could read and apply the pre-negotiation buffer depth too early. The old time.sleep(0.25) masked this race, but it did not actually solve it.

What changed:

  • Removed the blind startup delay approach
  • Kept the existing client-side synchronous behavior:
    • if wait=True and client mode, _start() waits for RSSI open before applying negotiated RX buffer depth
  • Changed the server-side behavior:
    • _start() no longer tries to apply negotiated RX buffer depth immediately
    • instead, it defers the setRxBufferCount(curMaxBuffers()) call until RSSI actually reaches open state
  • Added small internal state to ensure the negotiated RX buffer count is applied once and cleaned up correctly on stop

Why:

  • curMaxBuffers() is only stable after RSSI negotiation
  • on the server side, blocking _start() until open can deadlock when both endpoints are managed under the same Root
  • so the correct fix is:
    • synchronous wait on the client side
    • deferred update on the server side

Result:

  • removes the fixed sleep
  • avoids the server-side race
  • avoids introducing same-process startup deadlock

…ssiPack

- Introduced threading to manage RX buffer updates on the server side.
- Added methods to apply negotiated RX buffer count and arm server RX buffer updates.
- Improved handling of RX buffer configuration to prevent deadlocks during negotiation.
- Updated _start and _stop methods to reflect new RX buffer management logic.
@bengineerd bengineerd requested a review from slacrherbst March 10, 2026 19:01
@bengineerd bengineerd marked this pull request as ready for review March 10, 2026 20:08
@bengineerd bengineerd requested a review from ruck314 March 16, 2026 16:19
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 13.63636% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 29.55%. Comparing base (455967d) to head (af8aed6).
⚠️ Report is 792 commits behind head on pre-release.

Files with missing lines Patch % Lines
python/pyrogue/protocols/_Network.py 13.63% 19 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@              Coverage Diff              @@
##           pre-release    #1135    +/-   ##
=============================================
  Coverage        29.54%   29.55%            
=============================================
  Files               65       68     +3     
  Lines             6644     6964   +320     
  Branches          1015      973    -42     
=============================================
+ Hits              1963     2058    +95     
- Misses            4527     4753   +226     
+ Partials           154      153     -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bengineerd bengineerd merged commit 87ee6ff into pre-release Mar 16, 2026
6 checks passed
@bengineerd bengineerd deleted the udp-rssi-startup-race branch March 16, 2026 19:13
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