To_proc_time cmd added#455
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:
📝 WalkthroughWalkthroughRtcManager tracks proc-time mode, falls back to uptime-based time when RTC access or conversion fails, supports ChangesRTC proc-time management
Zephyr submodule reference
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant RtcManager
participant RTC
participant Uptime
Test->>RtcManager: set RTC time
RtcManager->>RTC: read RTC time
RTC-->>RtcManager: return RTC value
Test->>RtcManager: issue TO_PROC_TIME
RtcManager->>Uptime: read uptime
Uptime-->>RtcManager: return proc time
RtcManager-->>Test: emit timebase event
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
There was a problem hiding this comment.
I like this initial take, here's a suggested design:
Use F Prime's built-in enum TimeBase. Store the selected timebase as an F Prime parameter so it can be stored over multiple boots. No F Prime commands necessary, the parameter will be all you need to set and store the chosen timebase. The parameter default should be TB_SC_TIME. Check the timebase once near the beginning of timeGetPort_handler() after the variable sets but before the RTC device ready check. If the timebase is TB_PROC_TIME, return the proc time.
Also to consider... Since this component supports fewer time bases than the upstream F Prime TimeBase enum we may want to have our own TimeBase enum declared in the FPP that only copies the upstream enum but removes the unsupported timebases like TB_WORKSTATION_TIME.
What do you think?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/Drv/RtcManager/RtcManager.cpp`:
- Around line 52-54: Update timeGetPort_handler to check m_ProcTimeSet before
any RTC readiness or read logic; when set, return the current proc-time/uptime
value immediately and preserve the flag so RTC handling cannot overwrite it.
- Around line 171-179: In the proc-time initialization block, remove the unused
proc_time object and the useconds_since_boot calculation. Keep the uptime
conversion, log_ACTIVITY_HI_procTimeSet call, and m_ProcTimeSet update
unchanged.
In `@PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.fpp`:
- Around line 61-64: Rename the `procTimeSet` event to `ProcTimeSet` in the
RtcManager component, and update its generated logging call in `RtcManager.cpp`
from `log_ACTIVITY_HI_procTimeSet` to `log_ACTIVITY_HI_ProcTimeSet` so all
references remain consistent.
🪄 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: 686300f0-9451-47e5-9fc5-094b892c2de2
📒 Files selected for processing (3)
PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.cppPROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.fppPROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.hpp
| event procTimeSet( | ||
| seconds: U32 @< Uptime in seconds | ||
| ) severity activity high id 17 format "Proc time set, current uptime: {}" | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use PascalCase for event names to match existing conventions.
The existing events in this component (e.g., TimeSet, DeviceNotReady, AlarmSet) use PascalCase. Consider renaming procTimeSet to ProcTimeSet for consistency. Note that if you change this, you will also need to update the auto-generated logging call in RtcManager.cpp to log_ACTIVITY_HI_ProcTimeSet.
♻️ Proposed refactor
- event procTimeSet(
+ event ProcTimeSet(
seconds: U32 @< Uptime in seconds
) severity activity high id 17 format "Proc time set, current uptime: {}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| event procTimeSet( | |
| seconds: U32 @< Uptime in seconds | |
| ) severity activity high id 17 format "Proc time set, current uptime: {}" | |
| event ProcTimeSet( | |
| seconds: U32 @< Uptime in seconds | |
| ) severity activity high id 17 format "Proc time set, current uptime: {}" |
🤖 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/Drv/RtcManager/RtcManager.fpp`
around lines 61 - 64, Rename the `procTimeSet` event to `ProcTimeSet` in the
RtcManager component, and update its generated logging call in `RtcManager.cpp`
from `log_ACTIVITY_HI_procTimeSet` to `log_ACTIVITY_HI_ProcTimeSet` so all
references remain consistent.
collinc04
changed the title
I like the idea of using a timebase to survive multiple boots, I think this is a good idea |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.cpp (3)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository member-naming convention.
Rename
m_ProcTimeSetto an underscore-separated form such asm_proc_time_setinRtcManager.hppandRtcManager.cpp. Based on learnings, F Prime component members underPROVESFlightControllerReference/Componentsuse them_plus underscore-separated convention.🤖 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/Drv/RtcManager/RtcManager.cpp` at line 21, Rename the RtcManager member m_ProcTimeSet to m_proc_time_set throughout RtcManager.hpp and RtcManager.cpp, including its declaration, initializer, and all references, while preserving its existing behavior.Source: Learnings
52-62: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not make RTC fallback permanently sticky.
After any RTC failure sets
m_ProcTimeSet, Line 53 bypasses all future RTC probes, so the successful-read path at Line 99 cannot restore RTC mode. Keep explicit proc-time selection separate from transient fallback, or retry RTC before honoring the fallback state.Also applies to: 75-78, 91-99
🤖 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/Drv/RtcManager/RtcManager.cpp` around lines 52 - 62, Update the RTC time-selection logic in RtcManager so a transient RTC readiness or read failure does not permanently bypass future RTC probes through m_ProcTimeSet. Separate explicit proc-time configuration from fallback state, or retry the RTC before honoring fallback; ensure the successful RTC path can clear or supersede the fallback state and restore RTC mode.
176-185: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist the selected timebase.
m_ProcTimeSetis initialized tofalseandTO_PROC_TIMEonly sets that volatile atomic flag, so a proc-time selection is lost on reset. Store the selectedFw::Time::TimeBasein an F Prime parameter withTB_SC_TIMEas the default, and use the runtime flag only for transient fallback if needed.🤖 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/Drv/RtcManager/RtcManager.cpp` around lines 176 - 185, Persist the selected timebase by adding an F Prime parameter initialized to Fw::Time::TB_SC_TIME, then update TO_PROC_TIME_cmdHandler to store Fw::Time::TB_PROC_TIME through that parameter instead of relying solely on m_ProcTimeSet. Use m_ProcTimeSet only for transient fallback behavior where required, and ensure timebase initialization restores the persisted parameter value after reset.
🤖 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.
Outside diff comments:
In `@PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.cpp`:
- Line 21: Rename the RtcManager member m_ProcTimeSet to m_proc_time_set
throughout RtcManager.hpp and RtcManager.cpp, including its declaration,
initializer, and all references, while preserving its existing behavior.
- Around line 52-62: Update the RTC time-selection logic in RtcManager so a
transient RTC readiness or read failure does not permanently bypass future RTC
probes through m_ProcTimeSet. Separate explicit proc-time configuration from
fallback state, or retry the RTC before honoring fallback; ensure the successful
RTC path can clear or supersede the fallback state and restore RTC mode.
- Around line 176-185: Persist the selected timebase by adding an F Prime
parameter initialized to Fw::Time::TB_SC_TIME, then update
TO_PROC_TIME_cmdHandler to store Fw::Time::TB_PROC_TIME through that parameter
instead of relying solely on m_ProcTimeSet. Use m_ProcTimeSet only for transient
fallback behavior where required, and ensure timebase initialization restores
the persisted parameter value after reset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 18522e3c-8807-49ed-8e68-dde1157ae383
📒 Files selected for processing (3)
PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.cppPROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppilib/fprime-zephyr
collinc04
changed the title
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 (4)
PROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppi (1)
17-21: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftAlign the Beacon fields with the LoRa component contract.
The supplied
lib/fprime-zephyr/fprime-zephyr/Drv/LoRa/LoRa.fppcontract exposesDATA_RATE,CODING_RATE,BANDWIDTH_TX,BANDWIDTH_RX, andTRANSMIT, but does not establish the referencedModule*channels. Packet generation/build will fail unless the pinned submodule provides those generated names. Also, expose TX and RX bandwidth separately rather than collapsing them intoModuleBandwidth.🤖 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/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppi` around lines 17 - 21, Update the LoRa config fields in ReferenceDeploymentPackets to use the LoRa contract’s established DATA_RATE, CODING_RATE, BANDWIDTH_TX, BANDWIDTH_RX, and TRANSMIT channels; replace ModuleBandwidth with separate TX and RX bandwidth fields and remove the unsupported Module* references.PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.cpp (3)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse underscore-separated member naming.
Rename
m_ProcTimeSettom_proc_time_setconsistently in the declaration and implementation. Based on learnings, F Prime component members underPROVESFlightControllerReference/Componentsuse them_plus underscore-separated naming convention.🤖 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/Drv/RtcManager/RtcManager.cpp` at line 21, Rename the member m_ProcTimeSet to m_proc_time_set consistently across its declaration, initializer, and all implementation references within RtcManager.Source: Learnings
52-62: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAllow RTC recovery after an automatic fallback.
Once any RTC failure sets
m_ProcTimeSet = true, the next invocation returns at Lines 53-56 before attempting another RTC read. Consequently, the successful-conversion path at Lines 99-103 is unreachable, so a transient RTC failure permanently selects proc time untilTIME_SET.Keep explicit proc-time selection separate from transient fallback state, or retry RTC reads while in fallback mode.
Also applies to: 75-75, 91-95, 99-103
🤖 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/Drv/RtcManager/RtcManager.cpp` around lines 52 - 62, The RtcManager time-update flow permanently treats transient RTC failure as explicit proc-time mode via m_ProcTimeSet. Separate the automatic fallback state from explicit proc-time selection, or allow RTC reads while fallback is active, and ensure the successful RTC conversion path can clear the fallback and restore RTC-based time without requiring TIME_SET.
20-21: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist the selected timebase across reboots.
m_ProcTimeSetis initialized tofalse, soTO_PROC_TIMEreverts to RTC mode after every reboot. Store the selection in F Prime’s persistent parameter contract, initialize the default from that parameter (TB_SC_TIME), and update it from bothTO_PROC_TIMEandTIME_SET.🤖 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/Drv/RtcManager/RtcManager.cpp` around lines 20 - 21, Update RtcManager’s m_ProcTimeSet handling to use the F Prime persistent parameter contract, defaulting from the TB_SC_TIME parameter instead of false. Persist updates whenever TO_PROC_TIME or TIME_SET changes the selected timebase, while preserving the existing RTC and processor-time behavior.
🤖 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/Drv/RtcManager/RtcManager.cpp`:
- Line 46: Update the timeGetPort_handler definition in RtcManager.cpp to use
FwIndexType for portNum, matching the override declaration in RtcManager.hpp and
the generated virtual method signature.
---
Outside diff comments:
In `@PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.cpp`:
- Line 21: Rename the member m_ProcTimeSet to m_proc_time_set consistently
across its declaration, initializer, and all implementation references within
RtcManager.
- Around line 52-62: The RtcManager time-update flow permanently treats
transient RTC failure as explicit proc-time mode via m_ProcTimeSet. Separate the
automatic fallback state from explicit proc-time selection, or allow RTC reads
while fallback is active, and ensure the successful RTC conversion path can
clear the fallback and restore RTC-based time without requiring TIME_SET.
- Around line 20-21: Update RtcManager’s m_ProcTimeSet handling to use the F
Prime persistent parameter contract, defaulting from the TB_SC_TIME parameter
instead of false. Persist updates whenever TO_PROC_TIME or TIME_SET changes the
selected timebase, while preserving the existing RTC and processor-time
behavior.
In
`@PROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppi`:
- Around line 17-21: Update the LoRa config fields in ReferenceDeploymentPackets
to use the LoRa contract’s established DATA_RATE, CODING_RATE, BANDWIDTH_TX,
BANDWIDTH_RX, and TRANSMIT channels; replace ModuleBandwidth with separate TX
and RX bandwidth fields and remove the unsupported Module* references.
🪄 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: 42a3d8a5-35ad-4e05-b41c-e3804df75d57
📒 Files selected for processing (2)
PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.cppPROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppi
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.cpp (3)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse lower_snake_case for the new member state.
Rename
m_ProcTimeSetand its declaration/usages tom_proc_time_setto match the establishedm_plus underscore-separated member naming convention in this component path.Based on learnings, PROVES Flight Controller components use underscore-style member names such as
m_transmit_enable_ticks.🤖 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/Drv/RtcManager/RtcManager.cpp` at line 21, Rename the member state m_ProcTimeSet to m_proc_time_set throughout its declaration, initializer, and all usages in the RtcManager implementation, preserving its existing behavior and following the component’s lower_snake_case naming convention.Source: Learnings
52-66: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep RTC fallback recoverable.
The failure branches set
m_ProcTimeSet = true, but the early return at Lines 53-57 then prevents every future RTC readiness/read/conversion attempt. Consequently, the reset at Line 99 cannot restore RTC mode unlessTIME_SETis issued. Keep explicitTO_PROC_TIMEselection separate from transient fallback, or avoid setting this persistent flag in the failure branches.🐛 Proposed direction
if (!device_is_ready(this->m_dev)) { this->log_CONSOLE_RtcNotReady(); - this->m_ProcTimeSet = true; time.set(TimeBase::TB_PROC_TIME, 0, seconds_since_boot, useconds_since_boot); return; } if (rc != 0) { this->log_CONSOLE_RtcGetTimeFailed(rc); - this->m_ProcTimeSet = true; time.set(TimeBase::TB_PROC_TIME, 0, seconds_since_boot, useconds_since_boot); return; } if (errno == ERANGE) { this->log_CONSOLE_RtcInvalidTime(); - this->m_ProcTimeSet = true; time.set(TimeBase::TB_PROC_TIME, 0, seconds_since_boot, useconds_since_boot); return; }Also applies to: 70-95
🤖 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/Drv/RtcManager/RtcManager.cpp` around lines 52 - 66, Keep m_ProcTimeSet reserved for explicit TO_PROC_TIME selection and stop setting it in RTC failure branches, including the readiness, read, and conversion handling around the affected code. Preserve the uptime fallback for each failure, but allow subsequent calls to retry RTC readiness/read/conversion and let the existing reset at the TIME_SET path restore RTC mode.
52-66: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport automatic proc-time fallback transitions.
The new
TimeBaseandProcTimeSetevents are emitted only byTO_PROC_TIME_cmdHandler. Device-not-ready, RTC-read failure, and invalid-time fallback paths change the returned timebase without emitting either event, so downstream telemetry cannot distinguish an operator-selected switch from an automatic RTC failure.Emit the transition event once when fallback changes the active source.
Also applies to: 70-95
🤖 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/Drv/RtcManager/RtcManager.cpp` around lines 52 - 66, Update the automatic fallback paths in RtcManager, including the device-not-ready handling and the RTC-read/invalid-time paths around the affected code, to emit the same TimeBase and ProcTimeSet transition events used by TO_PROC_TIME_cmdHandler when the active source changes to processor time. Emit the transition only on the state change, preserving the existing time assignment and avoiding duplicate events on subsequent calls already in processor-time mode.PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.fpp (1)
34-36: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist the selected timebase across boots.
TO_PROC_TIMEonly sets the in-memorym_ProcTimeSetflag, which isfalseafter construction and stays false on subsequent boots. Add a persisted supported-timebase parameter with defaultTB_SC_TIME, or demonstrate that the selection is stored/restored elsewhere before relying on proc-time operation.🤖 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/Drv/RtcManager/RtcManager.fpp` around lines 34 - 36, The TO_PROC_TIME handling in RtcManager must persist the selected timebase across boots instead of relying only on the in-memory m_ProcTimeSet flag. Add a persisted supported-timebase parameter defaulting to TB_SC_TIME, update it when TO_PROC_TIME selects processor time, and restore m_ProcTimeSet from that parameter during initialization; alternatively, reuse an existing storage/restore mechanism if one already persists this selection.
🤖 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.
Outside diff comments:
In `@PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.cpp`:
- Line 21: Rename the member state m_ProcTimeSet to m_proc_time_set throughout
its declaration, initializer, and all usages in the RtcManager implementation,
preserving its existing behavior and following the component’s lower_snake_case
naming convention.
- Around line 52-66: Keep m_ProcTimeSet reserved for explicit TO_PROC_TIME
selection and stop setting it in RTC failure branches, including the readiness,
read, and conversion handling around the affected code. Preserve the uptime
fallback for each failure, but allow subsequent calls to retry RTC
readiness/read/conversion and let the existing reset at the TIME_SET path
restore RTC mode.
- Around line 52-66: Update the automatic fallback paths in RtcManager,
including the device-not-ready handling and the RTC-read/invalid-time paths
around the affected code, to emit the same TimeBase and ProcTimeSet transition
events used by TO_PROC_TIME_cmdHandler when the active source changes to
processor time. Emit the transition only on the state change, preserving the
existing time assignment and avoiding duplicate events on subsequent calls
already in processor-time mode.
In `@PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.fpp`:
- Around line 34-36: The TO_PROC_TIME handling in RtcManager must persist the
selected timebase across boots instead of relying only on the in-memory
m_ProcTimeSet flag. Add a persisted supported-timebase parameter defaulting to
TB_SC_TIME, update it when TO_PROC_TIME selects processor time, and restore
m_ProcTimeSet from that parameter during initialization; alternatively, reuse an
existing storage/restore mechanism if one already persists this selection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cbd345c4-b26f-4ef2-8447-b1e962dbb708
📒 Files selected for processing (2)
PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.cppPROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.fpp
|
I'd like to note that I have separated this from the beaconing packet change, work will continue on that one in the fprime-zephyr repo |
There was a problem hiding this comment.
There's a piece on the ticket:
Audit time usage Document where time matters and what TIME_SET changes, including:
Command sequences (cmdSeq, payloadSeq, safeModeSeq) — canceled on time set
AuthenticationRouter comm-loss timer (switched by another PR)
I think Detumble also falls into that category. Can you audit and explain what happens to all the components that depend on time when this time set hits?
There was a problem hiding this comment.
Following up on this one, I think we need to cancel running sequences when we set the time, otherwise unexpected things may happen.
I also have a board without an RTC battery so am unable to test these toggle changes. Do you mind uploading verification evidence like I did on the hardcoded startup component?
| // Check device readiness | ||
| if (!device_is_ready(this->m_dev)) { | ||
| this->log_CONSOLE_RtcNotReady(); | ||
| this->m_ProcTimeSet = true; // proc time flag |
There was a problem hiding this comment.
My vision for this component was more something that didn't change the behavior of the RTC bring up, but something the operator could physcially comamnd. That way we don't have to audit the behavoir of every failing case and how this flag would change it/ less changes are always better.
That being said totally open to it, if tested for these cases. It could fix issues if we've been having timebase issues with flashing back and forth between timebases, and I think if our sat completly runs on monotonic time, that would be fine and safer than a flaky RTC.
I think the other thing is, the RTC does fail on the first few seconds of startup, you can observe monotonic time for a bit before the RTC kicks on. Do these cases that do not allow the RTC to retry account for that or does every run run on monotinic time?
There was a problem hiding this comment.
My general thought was that if we implement a command toggle for it, then it should stay that way if it fails unless commanded otherwise so that we don't switch back to a potentially bad RTC.
I did remove the flag set on the device not ready case in the bring up, because otherwise it would boot up in monotonic time unless otherwise set
There was a problem hiding this comment.
But what if we have flaky events? one wrong I2C value could set us to monotonic. I also think the behavoir of the RTC is that we start on monotonic on boot, so if the RTC is not working we are on monotonic time. At least that's what I've observe on my board in the past.
If you want to keep latched implementation of the component, I think we at least want events in these places so that we know why the time is set to monotonic. I'd also want that more visible on the sdd
| ) | ||
|
|
||
| @ TO_PROC_TIME command to use proc time instead of RTC time | ||
| sync command TO_PROC_TIME() |
There was a problem hiding this comment.
Could be good to have a testcase for this commadn to show we can toggle it on and off no problem. We could push back getting the test in after the code freeze, but we should make a ticket if thats the case
There was a problem hiding this comment.
I made one, forgot to push oops 😓
There was a problem hiding this comment.
Could use some more verification though? It only checks for event emission, what do you think?
There was a problem hiding this comment.
This also uses await, so It probably needs to be changed to be more in line with #407
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/int/rtc_test.py`:
- Around line 441-455: Update test_proc_toggle to capture both TimeBase results
returned by await_event: assert the initial event reports the RTC source, then
send TO_PROC_TIME and assert the subsequent event reports the PROC source.
Preserve the existing timing setup and timeouts.
🪄 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: b5323cc7-7181-4c7c-9439-f4c2a5f09c46
📒 Files selected for processing (1)
PROVESFlightControllerReference/test/int/rtc_test.py
ineskhou
left a comment
ineskhou
left a comment
There was a problem hiding this comment.
Sorry hope these comments aren't too much of a mess bascially:
- Up to you as designer to choose to latch to monotonic for errors or not (you can also ask for a trade in the discord if you want), but if you do I'd want more clear events and documentation for it in the sdd.
- Looking for manual testing (see startup manager)
- Want checking that other parts of the system that use the RTC will not be negatavly affected by these timebase changes.
- Few nits (spelling, extra variable, ect)
But you've put a lot of work in this and it looks great! Thanks for staying in there through all these reviews
| fprime_test_api.send_command(f"{rtcManager}.TO_PROC_TIME") | ||
|
|
||
| # Assert that we receive a TimeBase event within 10 seconds | ||
| fprime_test_api.await_event(f"{rtcManager}.TimeBase", timeout=10) |
There was a problem hiding this comment.
Could be good to have this test assert what the timebase is that is being set (PROC vs RTC) and/or actually check the physcial timebase. Checking the event doesn't let us know if the system of the sat is on PROC time. At minimum before merge, woudl want manual evidence on this happening on a board
That being said, test updates will not block merge.
| def test_proc_toggle(fprime_test_api: IntegrationTestAPI, start_gds): | ||
| """Test for events emitted by proc time toggle""" | ||
|
|
||
| # Set time to Curiosity landing on Mars (7 minutes of terror! https://youtu.be/Ki_Af_o9Q9s) |
| seconds: U32 @< Uptime in seconds | ||
| ) severity activity high id 17 format "Proc time set, current uptime: {}" | ||
|
|
||
| @ Timebase even fires when the timebase changes and indicates the current timebase |
| // Check device readiness | ||
| if (!device_is_ready(this->m_dev)) { | ||
| this->log_CONSOLE_RtcNotReady(); | ||
| this->m_ProcTimeSet = true; // proc time flag |
There was a problem hiding this comment.
But what if we have flaky events? one wrong I2C value could set us to monotonic. I also think the behavoir of the RTC is that we start on monotonic on boot, so if the RTC is not working we are on monotonic time. At least that's what I've observe on my board in the past.
If you want to keep latched implementation of the component, I think we at least want events in these places so that we know why the time is set to monotonic. I'd also want that more visible on the sdd
There was a problem hiding this comment.
Following up on this one, I think we need to cancel running sequences when we set the time, otherwise unexpected things may happen.
I also have a board without an RTC battery so am unable to test these toggle changes. Do you mind uploading verification evidence like I did on the hardcoded startup component?
4 participants
To_Proc_Time command added
Description
Added a command to switch between timebases
Interactions
Does not interfere with command loss timer as that has been moved to the 10hz rate group.
Cancels all queued sequences, just like time_set does
Related Issues/Tickets
How Has This Been Tested?
Screenshots / Recordings (if applicable)
Checklist
Further Notes / Considerations