fix: UART file uplink/downlink failures (#457, #344) - #463
Conversation
Reproduces UART file uplink/downlink failures for multi-chunk transfers: uplink corrupts silently above ~3-4 chunks (separate, unresolved bug), and downlink hangs indefinitely on any transfer (root-caused to downlinkRepeater's BufferRepeater fanning out to a disabled-by-default LoRa channel that never returns its buffer). Oracle is fileManager's on-board CalculateCrc vs local zlib.crc32^0xFFFFFFFF for uplink, and byte-for-byte comparison against the GDS's own downlinked bytes for downlink. Includes a 1/3/5-chunk parametrized suite plus slow-marked 1000-chunk uplink and downlink stress cases (downlink builds its large source file via on-board AppendFile rather than the still-broken large uplink path). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gMrPNe6LwGtBS6Y7B5bo8
#457 Root cause (confirmed via HIL A/B testing): Utilities::BufferRepeater only returns a downlink buffer to fileDownlink once every enabled+connected multiOut channel has independently returned it. BufferRepeater's own CHANNEL_ENABLED default is all-channels-ENABLED, but lora.TRANSMIT defaults to DISABLED on every flight reset, so the LoRa channel's comQueue never drains its FILE buffer -> fileDownlink wedges permanently on the very first downlink attempt (matches #457 and #344). Fix: force downlinkRepeater.CHANNEL_ENABLED to [ENABLED, DISABLED, DISABLED] (UART-only) on every boot, right after loadParameters() in ReferenceDeploymentTopology.cpp, mirroring the existing lora.start(..., TransmitState::DISABLED) override pattern already used in this file for project-specific boot-time defaults. paramSet_CHANNEL_ENABLED() is private on the generated component base (command-dispatch only), so the override drives the real command path: build a Fw::CmdArgBuffer and invoke downlinkRepeater's cmdIn port directly with opcode getIdBase() + OPCODE_CHANNEL_ENABLED_SET (0x0). This is a safe-default fix only -- it does not change BufferRepeater's fan-out logic. Re-enabling LoRa downlink requires explicitly setting CHANNEL_ENABLED[1]=ENABLED in lockstep with lora.TRANSMIT ENABLED; HIL testing showed enabling TRANSMIT alone does not recover an already-wedged transfer and even fresh transfers only drain slowly against downlinkDelay's configured cadence. Verified on hardware (PROVES V5e flight controller): downlink 1 and 3 chunk cases pass cleanly from a cold boot with no manual PRM_SET (302B and 612B transfers land correctly and instantly). Uplink 1/3 chunks and downlink-of-a-5-chunk-file remain red -- separate, pre-existing uplink corruption bug above ~3-4 chunks, out of scope for this fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gMrPNe6LwGtBS6Y7B5bo8
…tion (issue #457) Bumps the fprime-zephyr submodule pin to Open-Source-Space-Foundation/fprime-zephyr branch fix/457-uart-rx-drain (commit 09fdccb), which fixes the UART RX driver's silent byte-drop-on-full-ring-buffer behavior: - schedIn_handler now drains the RX ring buffer in a bounded loop each tick (capped at ring-capacity/read-size iterations) instead of taking a single 64-byte bite, so a backlog can be fully drained in one tick instead of trickling out while more bytes queue up and overrun. - serial_cb (the RX ISR) now checks ring_buf_space_get() before reading a byte out of the hardware FIFO. Once the ring is full, it disables its own RX interrupt (uart_irq_rx_disable) instead of reading-and-dropping. For this board's USB-CDC-backed UART, that makes the underlying stack NAK the host's bulk-OUT endpoint, so unread bytes queue up in the *host's* USB stack (true end-to-end flow control) instead of being silently lost. schedIn_handler re-enables RX once draining frees ring space. - RxRingBufferOverrun (throttled warning event) + RxOverrunCount (telemetry) surface any residual drops, which should no longer occur in normal operation. RAM-neutral: RING_BUF_SIZE stays 1024 bytes; SERIAL_BUFFER_SIZE (64->248) draws from commsBufferManager's existing 1024B-per-buffer pool, adding no new allocation. Verified on hardware (PROVES V5e, RP2350/Zephyr): uplink 1/3/4/5/6/8/10 chunks and 5x5-chunk repeats pass reliably; previously uplinks above 3-4 chunks corrupted ~100% of the time. Zero RX ring-buffer overrun or CCSDS frame-desync events observed across all post-fix test sessions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WpBURCutAx8281i59nj6fo
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Summary
Fixes #457 (UART file uplink/downlink failure) and #344 (buffer-repeater downlink wedge).
downlinkRepeater.CHANNEL_ENABLEDnow defaults to UART-only at boot (ReferenceDeploymentTopology.cpp), preventing the permanent wedge caused byUtilities::BufferRepeaterwaiting on a LoRa channel that is disabled by default and never returns its buffer.fprime-zephyr'sZephyrUartDriverRX path now uses ISR-level back-pressure (uart_irq_rx_disable/enable) instead of silently dropping bytes when its software ring buffer fills, plus a bounded per-tick drain loop and overrun visibility (RxRingBufferOverrunevent /RxOverrunCounttelemetry). RAM-neutral: ring buffer size unchanged (1024B). Submodule pinned to09fdccbonOpen-Source-Space-Foundation/fprime-zephyrbranchfix/457-uart-rx-drain.Verification
Svc::FileUplink's event-ordering (Svc::FileUplink emits FileReceived before the file is durably closed, causing spurious file-not-found #461) — not a regression of this fix; the affected code path is untouched by this PR.Related follow-ups filed separately (not part of this PR)
Svc::FileUplinkemitsFileReceivedbefore durable close (pre-existing, code-confirmed)FileDownlinkCanceldoesn't recover a wedged transferCloses #457
Closes #344
🤖 Generated with Claude Code
https://claude.ai/code/session_01WpBURCutAx8281i59nj6fo