Repository navigation
Fix antenna producer initialization and ground-normal propagation - #1672
dhutererprats wants to merge 4 commits into
Conversation
|
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.
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:
This commit adds only the integration test. Production behavior is unchanged. |
Pushed a commit to address this. |
3dd3876 to
58f7ef5
Compare
schaubh
left a comment
There was a problem hiding this comment.
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:
Resetnow publishes a zero-filled output. AfterInitializeSimulation(),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
UpdateStatewith ready inputs. Applications handling those errors must coverExecuteSimulation()or directUpdateStatecalls. Unlinked antenna inputs are still reported duringReset.
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
left a comment
There was a problem hiding this comment.
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.
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:
Behavior changes for reviewers:
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.
Follow-up validation after the documentation and scheduler-test additions:
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-failureValidation 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.