Introduction of downlinkHandling module to communication architecture#1323
Introduction of downlinkHandling module to communication architecture#1323dhutererprats wants to merge 10 commits intodevelopfrom
Conversation
dhutererprats
force-pushed
the
downlinkHandling
branch
2 times, most recently
from
f6af0cf to
3be776c
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3be776cccd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void DownlinkHandling::customWriteMessages(uint64_t CurrentClock) | ||
| { | ||
| this->downlinkOutMsg.write(&this->downlinkOutBuffer, this->moduleID, CurrentClock); |
There was a problem hiding this comment.
When this module is turned off through nodeStatusInMsg (dataStatus <= 0), DataNodeBase skips evaluateDataModel, so downlinkOutBuffer is never refreshed for that step; however customWriteMessages still publishes the previous buffer values. In that scenario, downstream consumers can read stale non-zero throughput/link fields even though the node is disabled and nodeDataOutMsg has been zeroed, which can mislead mode logic and telemetry analysis.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This was real: when nodeStatusInMsg.deviceCmd <= 0, DataNodeBase skips evaluateDataModel(), so downlinkOutBuffer could remain stale while still being published.
I fixed this by explicitly clearing downlinkOutBuffer inside customWriteMessages() when dataStatus <= 0 before writing downlinkOutMsg. This prevents stale non-zero link/throughput diagnostics from being emitted while the node is off.
I also added a regression test (test_downlink_disabling_node_clears_diagnostics) that toggles the node from ON to OFF in one run and verifies the downlink diagnostics are zeroed after disable.
dhutererprats
force-pushed
the
downlinkHandling
branch
2 times, most recently
from
5ba35e8 to
edec469
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edec469cda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| *dataUsageMsg = this->nodeDataOutMsg.zeroMsgPayload; | ||
|
|
||
| double computedTimeStep = currentTime - this->previousTime; // [s] |
There was a problem hiding this comment.
computedTimeStep is calculated from previousTime, but previousTime is only updated inside evaluateDataModel() when the node is active. When nodeStatusInMsg disables this model for multiple ticks, re-enabling it makes the next active step integrate the entire off-duration as if downlink was running, which inflates removedBitsThisStep and all cumulative bit counters in a single cycle. Updating the timing state while disabled (or resetting/clamping on re-enable) avoids this off-time throughput corruption.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, this was valid.
previousTime was only advanced in evaluateDataModel(), so disabled periods could be lumped into the first re-enabled step.
Fixed by advancing timing while disabled in customWriteMessages():
previousTime = CurrentClock * NANO2SECcurrentTimeStep = 0.0
I also added a regression test (test_downlink_reenable_does_not_integrate_disabled_downtime) that disables for multiple ticks, re-enables, and verifies no off-time burst in timestep/cumulative removal, so this issue should be fixed now
dhutererprats
force-pushed
the
downlinkHandling
branch
2 times, most recently
from
72e48b4 to
76f68ab
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76f68abd32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this->setDataNameFromStorageSelection(selection, this->nodeDataName, sizeof(this->nodeDataName)); | ||
| std::strncpy(dataUsageMsg->dataName, this->nodeDataName, sizeof(dataUsageMsg->dataName) - 1); |
There was a problem hiding this comment.
Selecting a target partition across multiple storageUnitInMsgs is unsafe because the emitted DataNodeUsageMsgPayload is keyed only by dataName; if two connected storage units share a partition name (common with default "STORED DATA"), both units will consume the same negative baud command. This silently drains non-selected buffers and corrupts downlink/removal accounting in multi-storage scenarios, since downstream storage routing matches by name only (DataStorageUnitBase::messageInStoredData).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I addressed it by adding a uniqueness guard in downlinkHandling before applying removal:
- if the selected
dataNameis not unique across linkedstorageUnitInMsgs, removal is forced to zero for that step and aBSK_WARNINGis emitted in that case to make the configuration issue visible. - this will prevent ambiguous negative-baud routing from draining multiple storage units unintentionally
I also added a regression test:
test_downlink_ambiguous_multi_storage_name_blocks_removal
And updated the module docs to describe this safeguard and the recommendation to keep partition names globally unique across linked storage units.
dhutererprats
force-pushed
the
downlinkHandling
branch
from
76f68ab to
db4f933
Compare
dhutererprats
force-pushed
the
downlinkHandling
branch
from
db4f933 to
b6d1d20
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
dhutererprats
force-pushed
the
downlinkHandling
branch
from
b6d1d20 to
1ffe531
Compare
1 participant
bsk-xxxx)Description
This PR introduces the
downlinkHandlingcommunication module and all supporting interfaces, tests, and documentation required to use and validate it in Basilisk.The module maps link quality from
linkBudgetinto realistic downlink outcomes, including:CNR1/CNR2and antenna RX stateCore implementation and integration work includes:
New downlink message payload and diagnostics field updates:
src/architecture/msgPayloadDefC/DownlinkHandlingMsgPayload.hNew/hardened downlinkHandling module implementation and interface:
src/simulation/communication/downlinkHandling/downlinkHandling.hsrc/simulation/communication/downlinkHandling/downlinkHandling.cppsrc/simulation/communication/downlinkHandling/downlinkHandling.isrc/simulation/communication/downlinkHandling/Custom.cmakeUnit test coverage:
src/simulation/communication/downlinkHandling/_UnitTest/test_downlinkHandling.pyDocumentation and figures:
src/simulation/communication/downlinkHandling/downlinkHandling.rstsrc/simulation/communication/downlinkHandling/_Documentation/Images/DownlinkHandlingFlow.svgsrc/simulation/communication/downlinkHandling/_Documentation/Images/DownlinkHandlingReliabilityChain.svgRelease note snippet:
docs/source/Support/bskReleaseNotesSnippets/downlinkHandling.rstCommit organization is by feature layer:
Verification
Validation commands for this branch:
cmake --build dist3 -j4.venv/bin/pytest -q src/simulation/communication/downlinkHandling/_UnitTest/test_downlinkHandling.pyTest coverage includes:
REMOVE_ATTEMPTEDvsREMOVE_DELIVERED_ONLY)CNR1/CNR2, antenna RX states)Documentation
Documentation delivered in:
src/simulation/communication/downlinkHandling/downlinkHandling.rstIt includes:
C/N0,Eb/N0, BER, PER, retry-limited ARQ, throughput scaling)Future work