TlmArchive - #464
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new passive ChangesTelemetry archive
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TelemetrySplitter
participant TlmArchive
participant AntennaDeployer
participant ArchiveFile
TelemetrySplitter->>TlmArchive: deliver telemetry packet
TlmArchive->>AntennaDeployer: query deployment state
AntennaDeployer-->>TlmArchive: return deployment boolean
TlmArchive->>ArchiveFile: append packet when not deployed
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@PROVESFlightControllerReference/Components/AntennaDeployer/AntennaDeployer.cpp`:
- Around line 89-92: Update finishDeployment() so the deployment state is
written true only for DEPLOY_RESULT_SUCCESS; preserve false for
DEPLOY_RESULT_FAILED and exhausted retries. Ensure deploymentStateGet_handler()
continues reporting readDeploymentState() so failed deployments remain eligible
for archiving and future DEPLOY commands.
In `@PROVESFlightControllerReference/Components/TlmArchive/TlmArchive.cpp`:
- Around line 29-43: Update the TlmArchive write path around comIn and
deploymentStateGet_handler so the deployment-state check and
PRE_DEPLOYMENT_TLM_PATH append execute under one atomic ownership. Serialize
access with the existing component synchronization mechanism, or pass the
deployment state together with telemetry through a single queued owner, ensuring
no archive write can occur after deployment begins.
- Around line 25-44: Update TlmArchive::comIn_handler so it no longer performs
createDirectory, file open, or file write operations synchronously; instead,
enqueue or buffer each telemetry packet for a bounded worker/component queue.
Implement the worker-side filesystem initialization and append writes,
preserving deployment-state filtering and PRE_DEPLOYMENT_TLM_PATH while ensuring
queue capacity and overflow behavior are bounded.
- Around line 33-44: Update the archive write path in the surrounding TlmArchive
method to log failures from createDirectory() and file.open() before returning.
Pass size by reference to file.write(), capture its Os::File::Status, compare
the reported byte count with the requested size, and emit a log for write
failures or short writes before closing the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bf4a10ba-600a-4164-b953-3b26604cab1a
📒 Files selected for processing (12)
PROVESFlightControllerReference/Components/AntennaDeployer/AntennaDeployer.cppPROVESFlightControllerReference/Components/AntennaDeployer/AntennaDeployer.fppPROVESFlightControllerReference/Components/AntennaDeployer/AntennaDeployer.hppPROVESFlightControllerReference/Components/CMakeLists.txtPROVESFlightControllerReference/Components/TlmArchive/CMakeLists.txtPROVESFlightControllerReference/Components/TlmArchive/TlmArchive.cppPROVESFlightControllerReference/Components/TlmArchive/TlmArchive.fppPROVESFlightControllerReference/Components/TlmArchive/TlmArchive.hppPROVESFlightControllerReference/Components/TlmArchive/docs/sdd.mdPROVESFlightControllerReference/ReferenceDeployment/Top/instances.fppPROVESFlightControllerReference/ReferenceDeployment/Top/topology.fpplib/fprime-zephyr
| bool AntennaDeployer ::deploymentStateGet_handler(FwIndexType portNum) { | ||
| (void)portNum; | ||
| return this->readDeploymentState(); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep failed deployments marked as not deployed.
finishDeployment() writes true for both DEPLOY_RESULT_SUCCESS and DEPLOY_RESULT_FAILED (Lines 226-231). After retries are exhausted, this handler reports deployed, so TlmArchive stops archiving and future DEPLOY commands are rejected.
Proposed fix
- if (result == Components::DeployResult::DEPLOY_RESULT_SUCCESS ||
- result == Components::DeployResult::DEPLOY_RESULT_FAILED) {
+ if (result == Components::DeployResult::DEPLOY_RESULT_SUCCESS) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@PROVESFlightControllerReference/Components/AntennaDeployer/AntennaDeployer.cpp`
around lines 89 - 92, Update finishDeployment() so the deployment state is
written true only for DEPLOY_RESULT_SUCCESS; preserve false for
DEPLOY_RESULT_FAILED and exhausted retries. Ensure deploymentStateGet_handler()
continues reporting readDeploymentState() so failed deployments remain eligible
for archiving and future DEPLOY commands.
ineskhou
left a comment
ineskhou
left a comment
There was a problem hiding this comment.
Great stuff! Great use of topology and ports and great architechure design. Had some comments about robustness with the file systsem since we dont want to hammer the file system too hard when we are depending on teh startup sequence
Main thing I want to see before merge is some manual testing of this, if you could provide screenshots. Especially this working as the startup sequence runs and if you can recerate a dead filesystem (but no worries if not)
ineskhou
left a comment
ineskhou
left a comment
There was a problem hiding this comment.
Great stuff! Great use of topology and ports and great architechure design. Had some comments about robustness with the file systsem since we dont want to hammer the file system too hard when we are depending on teh startup sequence
Main thing I want to see before merge is some manual testing of this, if you could provide screenshots. Especially this working as the startup sequence runs and if you can recerate a dead filesystem (but no worries if not)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
PROVESFlightControllerReference/Components/TlmArchive/TlmArchive.cpp (1)
59-78:⚠️ Potential issue | 🟠 MajorBound filesystem retries and archive growth.
Each run with a pending packet recreates the directory, retries failures on the next tick, and appends without a maximum size. If deployment never completes or the filesystem is unhealthy, this can hammer the filesystem and exhaust
//tlm; add bounded failure backoff/disable behavior and a maximum byte or record limit.This repeats the unresolved archive-lifecycle concerns from the earlier review.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PROVESFlightControllerReference/Components/TlmArchive/TlmArchive.cpp` around lines 59 - 78, Update the TlmArchive packet-write lifecycle around createDirectory, open, and write to prevent unbounded filesystem retries and archive growth: track consecutive failures and apply bounded backoff or disable archiving after the configured failure threshold, and enforce a maximum archive byte or record limit before appending. Preserve normal logging and reset retry state after successful writes, using existing TlmArchive state/configuration symbols where available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@PROVESFlightControllerReference/Components/TlmArchive/TlmArchive.fpp`:
- Around line 7-8: Move the blocking archive work currently performed by
TlmArchive.run_handler() behind an active or queued worker, preserving mailbox
draining and archive I/O without executing it on the scheduler thread. Update
TlmArchive in
PROVESFlightControllerReference/Components/TlmArchive/TlmArchive.fpp:7-8 and
remove the direct rateGroup1Hz-to-tlmArchive.run connection in
PROVESFlightControllerReference/ReferenceDeployment/Top/topology.fpp:306,
routing scheduled work through the new asynchronous path instead.
In `@PROVESFlightControllerReference/Components/TlmArchive/TlmArchive.hpp`:
- Around line 28-31: Rename the TlmArchive member fields in TlmArchive.hpp to
m_queue_mutex, m_pending_packet, m_packet_pending, and m_antennas_deployed, then
update every corresponding reference in TlmArchive.cpp while preserving
behavior.
---
Duplicate comments:
In `@PROVESFlightControllerReference/Components/TlmArchive/TlmArchive.cpp`:
- Around line 59-78: Update the TlmArchive packet-write lifecycle around
createDirectory, open, and write to prevent unbounded filesystem retries and
archive growth: track consecutive failures and apply bounded backoff or disable
archiving after the configured failure threshold, and enforce a maximum archive
byte or record limit before appending. Preserve normal logging and reset retry
state after successful writes, using existing TlmArchive state/configuration
symbols where available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b82e5631-f93e-4dee-a87a-fd6eb37fd263
📒 Files selected for processing (4)
PROVESFlightControllerReference/Components/TlmArchive/TlmArchive.cppPROVESFlightControllerReference/Components/TlmArchive/TlmArchive.fppPROVESFlightControllerReference/Components/TlmArchive/TlmArchive.hppPROVESFlightControllerReference/ReferenceDeployment/Top/topology.fpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PROVESFlightControllerReference/Components/TlmArchive/TlmArchive.cpp (1)
83-108: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnforce the file limit before appending and only account for successful writes.
A failed or short write at Lines 86-89 falls through and can add
requestedSizetom_fileSize; size-query failures at Lines 98-101 also do not count toward the failure limit. Additionally, an existing or near-limit archive can receive one more oversized packet before the cap is checked. Initialize/check the current size before append, reject packets that exceed remaining capacity, and update the counter only after a complete write.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PROVESFlightControllerReference/Components/TlmArchive/TlmArchive.cpp` around lines 83 - 108, The TlmArchive append flow must validate the current file size and enforce the archive limit before writing, rejecting packets larger than the remaining capacity, including when the archive is already near its cap. Update m_fileSize only after a complete successful write; do not account for failed or short writes, and ensure file-size query failures increment the existing failure counter through the same failure path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@PROVESFlightControllerReference/Components/TlmArchive/TlmArchive.cpp`:
- Around line 32-35: Synchronize access to m_failures and m_fileSize between
run_handler() and comIn_handler() using the existing m_queueMutex, covering
limit checks and state updates while releasing it before filesystem I/O;
alternatively, convert both fields to atomics with appropriate acquire/release
operations.
---
Outside diff comments:
In `@PROVESFlightControllerReference/Components/TlmArchive/TlmArchive.cpp`:
- Around line 83-108: The TlmArchive append flow must validate the current file
size and enforce the archive limit before writing, rejecting packets larger than
the remaining capacity, including when the archive is already near its cap.
Update m_fileSize only after a complete successful write; do not account for
failed or short writes, and ensure file-size query failures increment the
existing failure counter through the same failure path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 077c4fd3-385d-460f-953b-17d8b1f4a701
📒 Files selected for processing (3)
PROVESFlightControllerReference/Components/TlmArchive/TlmArchive.cppPROVESFlightControllerReference/Components/TlmArchive/TlmArchive.fppPROVESFlightControllerReference/Components/TlmArchive/TlmArchive.hpp
On startup with
|
| } | ||
|
|
||
| if ((currentSize > MAX_FILE_SIZE)) { | ||
| { |
There was a problem hiding this comment.
Its a soft check so no biggie but a currentSize + requested_size > MAX_FILE_SIZE is technically more correct
There was a problem hiding this comment.
yeah thats how it was, but then if that check is there the component never actually hits the file size limit and will just emit the less precise failure count error instead
There was a problem hiding this comment.
I guess you could do a latch for that, which could be cleaner. Since the size isnt hard requirement this is more of a suggestion
- Retain the mutex only for packet mailbox access - Update the design documentation for atomic state synchronization
ineskhou
left a comment
ineskhou
left a comment
There was a problem hiding this comment.
Thanks for reposnding to my comments! LGTM
ineskhou
left a comment
ineskhou
left a comment
There was a problem hiding this comment.
@hrfarmer , actually I have 2 questions specifically about downlinking the file. I am having a hard time getting the physical tlm data so I can read it. Its through UART but I think it might be related to my GDS configuration. Do you mind downlinking and posting what a file would look like? I'm also wondering why you picked .tlm file, when a .txt might be more accesible?
|
Did Testing with starting without the startup sequence.bin The sat ran and timed out |
|
Using The script was able unpack telelmtry here is the original |
ineskhou
left a comment
ineskhou
left a comment
There was a problem hiding this comment.
Changed Paramaters, watched the buffer fill up based on the changed paramater, LGTM! Don't forget to post your evidence and merge once you've fixed the CI machine
i'm testing it out right now to be sure, but the ci failure actually could be because of the component. this starts doing file system stuff right as the ci script tries to format the file system, so it could be causing some race condition/conflict that's leading to broken behavior. would explain why it seemed to sometimes work and sometimes not |
|
At this point after commenting out the run handler in 21309ec and having the radio tests immediately start working again, I'm pretty convinced the component's somehow having an adverse effect on comms, rate groups, or the filesystem thats leading to upstream breakage. Given an issue like this could be a potential game-ender for the satellites and there's only a couple of days until handoff, unless anyone feel strongly otherwise (or it turns out to somehow be a simple fix) I don't feel confident merging for this release cc @ineskhou |
2 participants
This component will store the telemetry collected before the first antenna deploy, as this information can be very useful for early troubleshooting.
This component was scoped down a lot from what I originally intended, it was functional but writes/other fs actions kept occasionally causing a software hang, which is very undesirable. I'll keep working on an expansion to this component that can constantly log telemetry so any recent missing telemetry could be downlinked, and if the software hangs can be fixed, could go on the second partition.