Make mode manager test more reliable - #460
Conversation
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe safe-mode command-loss integration test now declares its watchdog hardware requirement, detects reboot completion using a timestamped framework-version event, and polls until the boot count reflects the reboot. ChangesWatchdog reboot test
Estimated code review effort: 2 (Simple) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 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/mode_manager_test.py`:
- Around line 599-601: Update the reboot_start initialization in
mode_manager_test.py to use a UTC-aware current timestamp via
datetime.now(timezone.utc), while preserving the existing TimeType.set_datetime
call and TB_DONT_CARE time base.
🪄 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: 9a1c45ea-8c27-4f38-a29e-a7146a7fcb92
📒 Files selected for processing (1)
PROVESFlightControllerReference/test/int/mode_manager_test.py
| reboot_start: TimeType = TimeType().set_datetime( | ||
| datetime.now(), time_base=TimeType.TimeBase("TB_DONT_CARE") | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the return type of set_datetime and how TimeBase is initialized in fprime_gds.
python -c '
try:
from fprime_gds.common.models.serialize.time_type import TimeType
from datetime import datetime, timezone
t = TimeType()
res = t.set_datetime(datetime.now(timezone.utc), time_base=0)
print("set_datetime returns:", res)
print("TimeBase type:", type(TimeType.TimeBase))
# Check if TimeBase can be instantiated with a string
try:
tb = TimeType.TimeBase("TB_DONT_CARE")
print("TimeBase string instantiation succeeded:", tb)
except Exception as e:
print("TimeBase string instantiation failed:", type(e).__name__, "-", e)
try:
val = TimeType.TimeBase["TB_DONT_CARE"].value
print("Bracket notation succeeded. Value:", val)
except Exception as e2:
print("Bracket notation failed:", e2)
except ImportError as e:
print("Could not import TimeType:", e)
'Repository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and locate the TimeType implementation.
git ls-files PROVESFlightControllerReference/test/int/mode_manager_test.py
fd -a 'time_type.py' .
fd -a 'TimeType.py' .
fd -a '*time*type*' . | head -200
# Inspect the relevant test lines.
python3 - <<'PY'
from pathlib import Path
path = Path('PROVESFlightControllerReference/test/int/mode_manager_test.py')
lines = path.read_text().splitlines()
for start in (585, 595, 620):
end = min(len(lines), start + 35)
print(f"\n--- {path}:{start+1}-{end} ---")
for i in range(start, end):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 541
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find definitions/usages related to TimeType and TimeBase in the repository.
rg -n "class TimeType|def set_datetime|TimeBase|TB_DONT_CARE" PROVESFlightControllerReference -g '*.py'Repository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 1626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path('PROVESFlightControllerReference/test/int/mode_manager_test.py')
lines = path.read_text().splitlines()
# Print the surrounding region and any nearby references to reboot_start.
for start in (590, 600, 610, 620, 630):
end = min(len(lines), start + 20)
print(f"\n--- {path}:{start+1}-{end} ---")
for i in range(start, end):
print(f"{i+1:4d}: {lines[i]}")
print("\n--- reboot_start references ---")
for i, line in enumerate(lines, 1):
if "reboot_start" in line:
print(f"{i:4d}: {line}")
PYRepository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 3639
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Compare the surrounding test patterns for TimeType initialization and timezone handling.
python3 - <<'PY'
from pathlib import Path
for file in [
'PROVESFlightControllerReference/test/int/camera_handler_test.py',
'PROVESFlightControllerReference/test/int/tmp112_test.py',
'PROVESFlightControllerReference/test/int/drv2605_test.py',
'PROVESFlightControllerReference/test/int/rtc_test.py',
'PROVESFlightControllerReference/test/int/radio_test.py',
'PROVESFlightControllerReference/test/int/veml6031_test.py',
'PROVESFlightControllerReference/test/int/reset_manager_test.py',
'PROVESFlightControllerReference/test/int/pico_temp_test.py',
]:
path = Path(file)
if not path.exists():
continue
text = path.read_text().splitlines()
print(f"\n=== {file} ===")
for i, line in enumerate(text, 1):
if 'TimeType.TimeBase("TB_DONT_CARE")' in line or 'datetime.now()' in line or 'datetime.now(timezone.utc)' in line:
print(f"{i:4d}: {line}")
PYRepository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 2042
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate any local references to fprime_gds packaging/docs and TimeType usage patterns.
rg -n "fprime_gds|set_datetime\(|TimeType\.TimeBase\(\"TB_DONT_CARE\"\)" -g '!**/.git/**' .Repository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 10900
🌐 Web query:
fprime_gds TimeType set_datetime return value TimeType.TimeBase TB_DONT_CARE
💡 Result:
In the F Prime (F´) framework, TimeType is a serializable class used to store timestamp information [1]. The value TB_DONT_CARE is defined as a constant (specifically 0xFFFF) within the TimeBase enumeration [2][3][4], which is used to indicate that a sequence or operation should run regardless of the flight software's specific time base [3]. While the F Prime documentation and source code references for the TimeType class (in both C++ and Python GDS) include various setter methods—such as set for time values or setTimeBase [5]—there is no standard method named set_datetime that returns TB_DONT_CARE as a value. The confusion likely stems from the fact that TimeType objects are associated with time bases, and TB_DONT_CARE is a valid value for the time base field itself [2][4]. If you are encountering code that references a return value of TB_DONT_CARE, it is likely referring to the state or configuration of the TimeBase property rather than the return value of a specific set_datetime method. In F Prime, the TimeBase enumeration includes: - TB_NONE: No time base established [2][4] - TB_PROC_TIME: Processor cycle time [2][4] - TB_WORKSTATION_TIME: Workstation time [2][4] - TB_SC_TIME: Spacecraft time [4] - TB_FPGA_TIME: FPGA/hardware clock [4] - TB_DONT_CARE: A wildcard value used for sequences (0xFFFF) [2][3][4]
Citations:
- 1: https://fprime.jpl.nasa.gov/latest/docs/user-manual/gds/gds-test-api-guide/
- 2: https://nasa.github.io/fprime/UsersGuide/dev/configuring-fprime.html
- 3: https://nasa.github.io/fprime/UsersGuide/gds/seqgen.html
- 4: https://fprime.jpl.nasa.gov/devel/docs/reference/api/cpp/html/class_time_base.html
- 5: https://fprime.jpl.nasa.gov/latest/docs/reference/api/cpp/html/_time_8hpp_source.html
🌐 Web query:
fprime_gds Python TimeType set_datetime TimeBase TB_DONT_CARE enum
💡 Result:
In the F' (F Prime) framework, TimeBase.TB_DONT_CARE is an enumeration member used to indicate that a specific time base requirement should be ignored, typically during sequence execution [1][2][3]. Key details regarding TB_DONT_CARE and the TimeType system include: 1. Usage in Sequencing: TB_DONT_CARE (often with a value of 0xFFFF or 65535) allows sequences to execute regardless of the current flight software time base [1][3]. Other common time bases include TB_PROC_TIME, TB_WORKSTATION_TIME, and TB_SC_TIME [2][4]. 2. TimeType in GDS: The fprime_gds.common.models.serialize.time_type.TimeType class is used to store and manipulate timestamps within the GDS Python environment [5][6]. 3. Initialization: When initializing a TimeType object, the time_base argument expects an integer corresponding to a valid TimeBase enum value [5]. 4. Comparison and Math: While TimeType objects support rich comparison and mathematical operations, operations between objects with different time bases or contexts generally preserve the attributes of the left operand [7][8]. There is no specific method named set_datetime in the standard Python TimeType class provided by fprime_gds; however, the class includes a get_datetime method used to convert a TimeType into a Python datetime object [5]. The time is typically set during the instantiation of the TimeType object or via internal helper methods [5].
Citations:
- 1: https://fprime.jpl.nasa.gov/devel/docs/user-manual/gds/seqgen/
- 2: https://fprime.jpl.nasa.gov/latest/docs/reference/api/cpp/html/class_time_base.html
- 3: https://fprime.jpl.nasa.gov/latest/docs/reference/api/cpp/html/_time_base_enum_ac_8hpp_source.html
- 4: https://fprime.jpl.nasa.gov/v4.2.2/docs/reference/api/cpp/html/class_time_base.html
- 5: https://nasa.github.io/fprime/UsersGuide/dev/testAPI/markdown/time_type.html
- 6: https://fprime.jpl.nasa.gov/latest/docs/user-manual/gds/gds-test-api-guide/
- 7: https://fprime.jpl.nasa.gov/devel/docs/user-manual/gds/gds-test-api-guide/
- 8: https://nasa.github.io/fprime/UsersGuide/dev/testAPI/user_guide.html
🌐 Web query:
"TimeType.set_datetime" fprime_gds returns self
💡 Result:
In the fprime-gds framework, the TimeType class does not have a method named set_datetime [1][2]. The documentation for fprime.common.models.serialize.time_type.TimeType lists get_datetime(tz=None) as the method for retrieving a Python datetime object, but it does not include a corresponding set_datetime method [1]. Furthermore, TimeType objects are generally designed to hold timestamp information (seconds and microseconds) and support arithmetic and comparison operations [3][4]. Internal methods such as _TimeTypeset_float exist for internal helper purposes but are not intended for general modification of existing TimeType instances [1]. The confusion may arise from comparing this to other libraries (such as PHP's DateTime::setTime [5]) or misremembering the API structure.
Citations:
- 1: https://nasa.github.io/fprime/UsersGuide/dev/testAPI/markdown/time_type.html
- 2: https://nasa.github.io/fprime/v1.5/UsersGuide/api/python/fprime-gds/html/api/index.html
- 3: https://fprime.jpl.nasa.gov/latest/docs/user-manual/gds/gds-test-api-guide/
- 4: https://fprime.jpl.nasa.gov/devel/docs/user-manual/gds/gds-test-api-guide/
- 5: https://www.php.net/datetime.settime.php
Use a UTC-aware timestamp here. assert_event(..., start=...) should be anchored with datetime.now(timezone.utc) to avoid timezone-dependent event matching.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 600-600: datetime.datetime.now() called without a tz argument
(DTZ005)
🤖 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/test/int/mode_manager_test.py` around lines
599 - 601, Update the reboot_start initialization in mode_manager_test.py to use
a UTC-aware current timestamp via datetime.now(timezone.utc), while preserving
the existing TimeType.set_datetime call and TB_DONT_CARE time base.
Sources: Coding guidelines, Linters/SAST tools
…zy increment The FrameworkVersion reboot-detection assert fires during topology startup, before StartupManager's first 1Hz run tick performs the lazy boot-count increment (StartupManager.cpp get_boot_count(true) behind the m_boot_count==0 guard). Querying GET_BOOT_COUNT immediately after reboot detection therefore races the increment and can read the pre-reboot count — the failure seen in run 29894110572 (query at TB_PROC_TIME 4.8s, count still 2). Keep the +1 assertion (it distinguishes exactly-one reboot from a boot loop, which a startup-EVR assert cannot) but retry the read for up to 30s until the increment lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Second flake in test_safe_09, seen on PR #470's CI (run 29983198334): CommandLossDetected asserted fine, but the immediate history scrape ran before EnteringSafeMode (emitted ~40ms later in the same firmware code path) reached the GDS. Use assert_event with a timeout so the check waits for the event instead of racing it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion (#470) * fix(startup): harden boot count persistence against hard-reset corruption HWIL evidence (integration-uart runs 29894110572 / 29979002023, PR #460): the boot count file was read back as 0x02FE191005000001 after a sequence of watchdog-commanded hard resets, and the next boot persisted exactly garbage+1 - the increment machinery works, but nothing guards against a torn flash write poisoning the count forever. Four changes to StartupManager: - Corruption guard: values > 1,000,000 are treated as a failed read (mirrors the existing quiescence-file 0xFF-fill guard) and reported via new WARNING_HI BootCountCorrupted with the raw value. - GET_BOOT_COUNT is now read-only: the unconditional write-back on every query was the largest source of boot-count write traffic, and each write is a window for a reset to tear the file. - Increment retry: if the first-tick persist fails (e.g. filesystem not ready), run_handler retries each 1Hz tick until it sticks - the increment is delayed, not lost. Failure warning de-duplicated per streak. - Atomic persist: write temp file then Os::FileSystem::rename over the target; littlefs renames are atomic so a mid-update reset leaves either the old or new file, never a torn one. Regression seam: test_safe_09_command_loss_triggers_safe_mode_and_reboot asserts boot count == initial+1 across a watchdog reset (with the retry poll from PR #460). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(startup): flush before close in file write helper; zero-init m_boot_count Addresses CodeRabbit review on #470: Os::File::close() is void and cannot report a deferred flush failure, so the write helper now requires file.flush() == OP_OK before reporting SUCCESS - persist_boot_count no longer renames a temp file whose data may not have reached storage. Also gives m_boot_count an in-class zero initializer; run_handler's first-tick guard reads it before any assignment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: correct filesystem — flight FS is FAT (ELM FatFs), not littlefs The persist design claimed littlefs-atomic renames; the flight filesystem is FAT (CONFIG_FAT_FILESYSTEM_ELM, zephyr,fstab,fatfs). FAT renames are not guaranteed power-cut atomic. The write-then-rename design still closes the observed torn-in-place-write window (data fully flushed before it replaces the old file); the residual worst case is a missing file, which reads as a failed read and re-initializes visibly instead of propagating silent garbage. Comments and SDD updated to state this accurately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2 participants
Description
I suspect that the 60 second sleep is maybe too long or short or something. This PR replaces the sleep with an event assert that will, hopefully at a minimum, speed up tests since it can escape prior to 60 seconds of the reset happens sonner than 60 seconds.