Skip to content

Fix antenna producer initialization and ground-normal propagation - #1672

Open
dhutererprats wants to merge 4 commits into
developfrom
feature/bsk-1671--antenna-producer-initialization
Open

dhutererprats wants to merge 4 commits into
developfrom
feature/bsk-1671--antenna-producer-initialization

Conversation

@dhutererprats

@dhutererprats dhutererprats commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Description

A groundLocation → simpleAntenna → linkBudget chain could classify unwritten antenna outputs as a space-to-space link during reset, disabling atmosphere and adding an erroneous ground pointing loss. SimpleAntenna also omitted the ground normal required for atmospheric slant paths.

Defer link placement and atmospheric-table initialization until execution, after both antenna messages have been written. When atmosphere is enabled, also wait for a nonzero ground antenna position so that a misordered antenna cannot cache sea-level conditions. Links without atmosphere do not wait for this position. While waiting, the module publishes zero service and logs one warning per reset.

Forward GroundStateMsgPayload.rHat_LP_N through AntennaLogMsgPayload.nHat_LP_N.

The atmospheric integration formulas remain unchanged.

Commits:

  1. Forward the ground surface normal through simpleAntenna.
  2. Initialize linkBudget from published producer outputs, with producer-chain regression tests.
  3. Add release-note and known-issue entries, with permanent documentation of readiness checks and error timing.
  4. Add scheduler-level integration tests using real producers and downstream consumers.

Behavior changes for reviewers:

  • Reset now writes a zeroed link-budget output, so the output reads as written before the first update. downlinkHandling treats this as no service.
  • Placement errors, including the unsupported two-ground-station case, now appear on the first update instead of during InitializeSimulation.
  • Lookup-table generation moves from Reset to the first update with ready inputs.
  • After a later Reset, previously written antenna positions cannot be distinguished from new ones, so producers must still run before linkBudget.

Verification

Prepared against origin/develop at 016955f, built with Vizard, OpNav, and MuJoCo enabled.

The full-suite results below cover the production fixes before the later documentation and scheduler-test additions. Those additions do not change production C++ code.

  • Full native suite: 843 passed.
  • Full Python suite: 14,331 passed and 41 skipped.
  • New coverage includes both antenna orders, tilted normals, aligned and offset boresights, sea-level and 1,624 m stations, delayed messages, antennas running before groundLocation, altitude changes across resets, zero service while waiting, space-to-space operation, and links without atmosphere.
  • Quantitative checks cover FSPL, Gaussian pointing loss, receiver CNR, and atmospheric agreement with correctly initialized written-payload references.
  • Against unmodified develop, 32 of the 34 producer-chain cases fail. With the reset-publication-only workaround, 28 fail. For a 1,624 m station at 20 GHz and 0.7 rad elevation, develop disables atmosphere (0 dB) and that workaround caches sea level (0.67146325 dB). This PR gives the reference 0.33555029 dB.
  • The 8 wrong-order cases fail when the ground-position wait is disabled.
  • Targeted Sphinx, Breathe, and Doxygen rendering passes, and linkBudget.h compiles on its own.
  • Downstream check: a real ground-station configuration (1,624 m, 2.2 GHz and 8.2 GHz receivers, with and without a Python noise-calibration model between the antenna and linkBudget) gives a first usable atmospheric loss and CNR identical to an explicitly primed reference for ordered, antenna-first, and delayed-antenna schedules, both link input orders, and a reset after an altitude change. Reinitializing linkBudget after the producers run leaves the result unchanged.

Follow-up validation after the documentation and scheduler-test additions:

  • 199 focused Python tests and 100 focused native tests passed, as recorded in the scheduler-test review comment.
  • The six new scheduler cases also passed a separate local run. They exercise ordered startup, delayed antennas, and delayed ground updates with both antenna input orders, using a 1,624 m station and a reset after a 2,000 m altitude change. Assertions cover the first usable tick, zero service and preserved storage while waiting, message timing, and downstream data transfer.
  • The updated documentation passed targeted Sphinx, Breathe, and Doxygen rendering and cross-reference checks, as recorded in the documentation review.

Run the targeted native checks with:

cmake --build dist3 \
  --target test_antennaProducerChain test_linkBudgetAtmosphere \
  --parallel 3

ctest --test-dir dist3 \
  -R 'LinkBudget|AntennaProducerChain' \
  --output-on-failure

Validation was performed on macOS arm64. Windows and Linux builds have not been run locally.

The downstream check uses a separate guarded Python noise-calibration adapter. That adapter fix is outside this Basilisk PR.

Documentation

Document producer execution order, deferred initialization, the ground-position wait, reset behavior, and ground-normal propagation. Explain why isWritten() alone does not establish usable service and when placement and atmospheric lookup errors are reported. Link the release-note and known-issue snippets to the permanent initialization guide.

@dhutererprats
dhutererprats requested a review from a team as a code owner November 9, 2026 15:24
@dhutererprats
dhutererprats marked this pull request as draft November 9, 2026 16:06
@schaubh schaubh self-assigned this Nov 9, 2026
@schaubh schaubh added bug Something isn't working documentation Improvements or additions to documentation labels Nov 9, 2026
@schaubh schaubh added this to Basilisk Nov 9, 2026
@schaubh schaubh moved this to 👀 In review in Basilisk Nov 9, 2026

schaubh commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Added the scheduler-level integration test in 3dd3876d2a following the critical review.

The review agreed with issue #1671 and the deferred-initialization/ground-normal solution. The remaining testing suggestion was to commit a regression using the actual simulation scheduler: the native producer-chain tests call lifecycle methods directly at time zero, and the downstream scheduling check described in the PR was not part of the committed suite.

test_antennaProducerScheduling.py adds six cases using real spacecraft, groundLocation, simpleAntenna, linkBudget, downlinkHandling, and storage modules through SimulationBaseClass:

  • Ordered startup, delayed antennas, and delayed ground updates, each with both antenna input orders.
  • Consumers continue running for two ticks while the selected producer task is disabled. Link service stays zero and storage is preserved.
  • On resumption, the first ready tick supplies the expected link results and downstream service.
  • Reinitializing the same simulation after an altitude change exercises old, already-written antenna messages and verifies that the first ordered tick rebuilds the atmosphere from the new geometry.
  • Assertions cover CNR, atmospheric and pointing loss, ground position/normal, message timestamps, elapsed time, and storage accounting.

The primed reference shares the link-budget formulas but supplies geometry independently; the existing numerical tests remain responsible for independent RF/atmospheric formula validation.

Validation on macOS:

  • 199 focused Python tests passed, including all six new cases.
  • 100 focused native tests passed.
  • The test docstrings parse as valid reStructuredText, and commit hooks passed.
  • Review probes deliberately reversed ground/antenna order, antenna/link order, and link/downlink order. Each variant failed all six cases, confirming sensitivity to scheduling errors. These probes did not change the committed file.

This commit adds only the integration test. Production behavior is unchanged.

@schaubh

schaubh commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Added the scheduler-level integration test in 3dd3876d2a following the critical review.

Pushed a commit to address this.

@schaubh
schaubh force-pushed the feature/bsk-1671--antenna-producer-initialization branch from 3dd3876 to 58f7ef5 Compare November 9, 2026 19:21

@schaubh schaubh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The review feedback was to document both lifecycle compatibility changes in the permanent module guide, so downstream users can update their readiness checks and error handling:

  • Reset now publishes a zero-filled output. After InitializeSimulation(), isWritten() can therefore be true before any usable link budget exists; consumers must also check the receiving antenna's CNR and link bandwidth.
  • Antenna-placement validation and atmospheric lookup-table initialization now occur on the first UpdateState with ready inputs. Applications handling those errors must cover ExecuteSimulation() or direct UpdateState calls. Unlinked antenna inputs are still reported during Reset.

The latest branch already addresses this feedback in 5b0fdabc2e. The files modified for this documentation follow-up are:

  • src/simulation/communication/linkBudget/linkBudget.rst: adds both compatibility notes under “Initialization and Execution Order,” including the implications for consumers and exception handling.
  • docs/source/Support/bskReleaseNotesSnippets/1671-antenna-producer-initialization.rst: links the release note to that permanent guide section.
  • docs/source/Support/bskKnownIssuesSnippets/1671-antenna-producer-initialization.rst: adds the same guide reference to the known-issue entry.

The targeted Sphinx/Breathe/Doxygen build and rendered cross-reference checks passed. The documentation was squashed into the existing documentation commit; the scheduler integration test remains a separate commit. This documentation concern is resolved.

@schaubh schaubh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Howdy @dhutererprats , thanks for the PR. Looks good overall. I found two small improvements I wanted to address. Review comments are posted below and I already pushed changes to address this. If you are ok with my changes, feel free to merge and close this PR. I'm approving it at this point.

@dhutererprats
dhutererprats marked this pull request as ready for review November 10, 2026 23:19

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

linkBudget initializes from unwritten antenna messages and simpleAntenna omits the ground normal

2 participants