Rtc conflict fix - #500
Conversation
…-core-reference into RTC_test_fix
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe RTC manager clears alarms during timebase changes and rejects alarm creation under process time. Integration tests add bounded event checks, alarm-ID cancellation checks, replacement-alarm cleanup, and timebase conflict coverage. ChangesRTC alarm timebase handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🔵 Low · up to The PR’s RTC conflict test may pass without reliably validating the rejection and restoration sequence because it uses the wrong timebase parameter and reuses an earlier event timestamp. The change is otherwise mergeable with explicit owner follow-up to correct these bounded test-validation gaps. Sequence Diagram(s)sequenceDiagram
participant Test
participant RtcManager
participant RTC
Test->>RtcManager: set spacecraft time
Test->>RtcManager: set alarm
RtcManager->>RTC: create alarm
Test->>RtcManager: change timebase
RtcManager->>RTC: clear alarm
Test->>RtcManager: set alarm under process time
RtcManager-->>Test: return execution error
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 190-197: Update the error branch in parameterUpdated after
rtc_alarm_set_time fails so it logs AlarmHardwareError, emits
log_ACTIVITY_HI_TimeBaseChanged(timeBase), and only then returns. Preserve the
existing success-path event behavior without duplicating the notification.
In `@PROVESFlightControllerReference/test/int/rtc_test.py`:
- Around line 496-503: Replace the dict(...) constructor assigned to
alarm_time_data with an equivalent dictionary literal containing the existing
Year, Month, Day, Hour, Minute, and Second entries. Run make fmt to apply the
required Ruff formatting.
- Around line 512-513: Update the cleanup call to ALARM_CANCEL in rtc test
teardown to pass the required U16 alarm ID, using the configured ID 0 or the ID
captured from the initial AlarmSet event, so the retained alarm is reliably
cancelled before later tests run.
- Around line 372-374: Update the AlarmCanceled assertion in the RTC test to
separate validation of alarm_canceled_evt.args from the ID comparison, ensuring
the error message never indexes args[0] unless an argument exists. Preserve the
expected alarm_id equality check after confirming the argument list is
non-empty.
- Around line 311-313: Update the event-window timestamp initialization using
TimeType.set_datetime to call datetime.now(timezone.utc) instead of the
host-local naive datetime.now(). Ensure the timezone import is available, then
run make fmt.
🪄 Autofix
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: d2bf789d-6a0b-4ebf-bf50-8a397fc47586
📒 Files selected for processing (2)
PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.cppPROVESFlightControllerReference/test/int/rtc_test.py
| 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
set -euo pipefail
rg -n -C 2 'set_datetime\(' PROVESFlightControllerReference/test/int
rg -n -C 2 'datetime\.now\(timezone\.utc\)' PROVESFlightControllerReference/test/intRepository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 15158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== rtc_test.py imports and nearby helper =="
sed -n '1,80p;255,515p' PROVESFlightControllerReference/test/int/rtc_test.py
echo
echo "== locate TimeType definition/usages =="
rg -n "class TimeType|def set_datetime|TimeBase|TB_DONT_CARE|TB_UTC|TB_" .Repository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 24805
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TimeType source in installed package =="
python3 - <<'PY'
import inspect
from fprime_gds.common.models.serialize.time_type import TimeType
print("module:", TimeType.__module__)
print(inspect.getsourcefile(TimeType))
print("\nset_datetime source:")
try:
print(inspect.getsource(TimeType.set_datetime))
except Exception as e:
print(type(e).__name__, e)
print("\nTimeBase source:")
try:
print(inspect.getsource(TimeType.TimeBase))
except Exception as e:
print(type(e).__name__, e)
print("\nTimeBase values:")
print(list(TimeType.TimeBase))
for val in TimeType.TimeBase:
print(val, val.value, val.name)
PY
echo
echo "== related await_event predicate behavior =="
rg -n -C 3 'def await_event|time_base|TimeBase|TB_' fprime-gds-common-proves-core-reference fprime_gds 2>/dev/null || trueRepository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 354
🌐 Web query:
fprime_gds common models serialize time_type TimeType set_datetime TimeBase TB_DONT_CARE TB_UTC
💡 Result:
In the F' (F Prime) architecture, TimeType is a serializable class located in fprime_gds.common.models.serialize.time_type used to parse, store, and format timestamp information for telemetry and events [1][2]. Regarding your query on TimeType and time base values: 1. TimeType and set_datetime: The TimeType class provides methods to manage timestamp data [1]. While standard Python datetime conversion is often handled through getters like get_datetime [1], the internal values (seconds and microseconds) are typically managed during object initialization or via internal helpers [1]. Users primarily interact with TimeType through arithmetic operations and comparison operators supported by the GDS Integration Test API [2]. 2. TimeBase Enumerations (TB_DONT_CARE, TB_UTC): The TimeBase enumeration defines the source or clock system for a timestamp [3][4]. - TB_DONT_CARE: Defined as 0xFFFF (or 65535), this value is used primarily in sequences to indicate that the sequence should execute regardless of the current flight software time base [3][4][5]. - TB_UTC: Note that standard F' TimeBase configurations typically include values like TB_NONE, TB_PROC_TIME, TB_WORKSTATION_TIME, and TB_SC_TIME [4][6][5]. While specific project implementations may extend or map time bases differently, TB_UTC is not a standard, built-in member of the default TimeBase enumeration in the core F' framework [4][5]. If you are looking to set a specific time base, you would typically provide the desired TimeBase enum value during the construction of a TimeType object [1]. The system uses these bases to ensure synchronization between ground-side sequences and flight software state [3].
Citations:
- 1: https://nasa.github.io/fprime/UsersGuide/dev/testAPI/markdown/time_type.html
- 2: https://fprime.jpl.nasa.gov/latest/docs/user-manual/gds/gds-test-api-guide/
- 3: https://fprime.jpl.nasa.gov/devel/docs/user-manual/gds/seqgen/
- 4: https://fprime.jpl.nasa.gov/devel/docs/reference/api/cpp/html/class_time_base.html
- 5: https://github.com/nasa/fprime/blob/devel/default/config/FpConfig.fpp
- 6: https://fprime.jpl.nasa.gov/latest/docs/reference/api/cpp/html/_time_base_enum_ac_8hpp_source.html
🌐 Web query:
github NASA fprime gds TimeType set_datetime TB_UTC source
💡 Result:
In the NASA F Prime (F´) Ground Data System (GDS), the TimeType class is used to store and manage timestamps for telemetry and events [1][2]. While you referenced "TB_UTC," the F Prime GDS implementation typically uses defined TimeBase constants to categorize time sources [1]. The set_datetime method is a member function of the TimeType class used to initialize or update a timestamp from a Python datetime object [1][3]. Example usage found in the F Prime GDS source code [3]: cmd_data.time.set_datetime(datetime.datetime.now, TimeType.TimeBase("TB_WORKSTATION_TIME")) Key details regarding TimeType and set_datetime: 1. Functionality: The set_datetime method allows developers to convert a standard Python datetime object into an F Prime TimeType instance, requiring a specified TimeBase [1]. 2. TimeBase: TimeBase constants (such as TB_WORKSTATION_TIME or others defined in the system) indicate the source or domain of the time [1][3]. 3. Location: The source code for the TimeType class is located in the fprime-gds repository, typically under fprime_gds/common/models/serialize/time_type.py [1][4]. Please note that results referencing TB_UTC often appear in other software projects (like libzypp) unrelated to NASA's F Prime [5][6]. Within F Prime, ensure you are using the specific TimeBase constants defined in your F Prime deployment's configuration or GDS headers [1][4].
Citations:
- 1: https://nasa.github.io/fprime/UsersGuide/dev/testAPI/markdown/time_type.html
- 2: https://fprime.jpl.nasa.gov/latest/docs/user-manual/gds/gds-test-api-guide/
- 3: https://github.com/nasa/fprime-gds/blob/devel/src/fprime_gds/common/pipeline/standard.py
- 4: https://github.com/nasa/fprime-gds
- 5: https://opensuse.github.io/libzypp/classzypp_1_1Date.html
- 6: https://opensuse.github.io/libzypp/Date_8cc_source.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect fprime-gds package/source if present =="
python3 - <<'PY'
import shutil, urllib.request, sys
urls = [
"https://raw.githubusercontent.com/nasa/fprime-gds/devel/src/fprime_gds/common/models/serialize/time_type.py",
]
for url in urls:
try:
data = urllib.request.urlopen(url, timeout=20).read().decode("utf-8", "replace")
except Exception as e:
print(f"FETCH_FAILED {url}: {type(e).__name__}: {e}")
continue
lines = data.splitlines()
print(f"--- {url} ({len(lines)} lines) ---")
for i, line in enumerate(lines, 1):
if "class TimeBase" in line or "TB_UTC" in line or "TB_DONT_CARE" in line or "TB_SC_TIME" in line or "TB_PROC_TIME" in line or "def set_datetime" in line:
start=max(1,i-8); end=min(len(lines),i+16)
for j in range(start,end+1):
print(f"{j:4}: {lines[j-1]}")
print()
PY
echo
echo "== list installed fprime-gds location without importing =="
python3 - <<'PY'
import sys, os, zipimport
print("sys.path:")
for p in sys.path[:10]:
print(" ", p)
for spec in pkg_resources_pkgutil := []:
pass
try:
import pkg_resources
for p in pkg_resources.working_set.by_key["fprime-gds"].metadata.resource_listdir("fprime_gds/common/models/serialize") if "fprime-gds" in {k.lower(): True for k in []} and "pkg_resources" in dir() else []:
if "time_type" in p:
print(p)
except Exception as e:
print("pkg_resources check skipped:", type(e).__name__, e)
try:
import fprime_gds
print("fprime_gds imported from", fprime_gds.__file__)
except Exception as e:
print("fprime_gds import failed:", type(e).__name__, e)
PYRepository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 704
🌐 Web query:
site:nasa.github.io/fprime UsersGuide dev testAPI time_type TimeType set_datetime TB_DONT_CARE
💡 Result:
In the F Prime (F´) framework, TB_DONT_CARE is a member of the TimeBase enumeration, defined with a value of 0xFFFF [1][2]. It is used primarily in sequencing to indicate that a sequence can be executed regardless of the current flight software time base [1]. Regarding the context of your query: 1. TimeType and Test API: The TimeType class is a serializable object used for storing and comparing timestamps (seconds and microseconds) within F Prime [3][4]. While TimeType handles time data, the TB_DONT_CARE constant belongs specifically to the TimeBase enumeration used by the system to manage time synchronization and sequencing [1][2]. 2. Functionality: When a sequence specifies TB_DONT_CARE as its time base, the system bypasses time base validation checks that would otherwise prevent the sequence from running if the flight software time base did not match the expected source [1][5]. 3. Configuration: The constant is defined in the F Prime configuration (typically in FpConfig.h) alongside other time base types like TB_PROC_TIME and TB_WORKSTATION_TIME [2][6][7]. It is distinct from FW_CONTEXT_DONT_CARE, which serves a similar "don't care" purpose for time contexts in sequences [5][8][6]. There is no standard method named set_datetime that takes TB_DONT_CARE as an argument in the public IntegrationTestAPI or TimeType class [3][9]. Time management in the Test API is generally handled through TimeType instances or helper methods like get_latest_time [4][9]. If you are encountering a set_datetime method, it may be a custom implementation or specific to a localized test script [3].
Citations:
- 1: https://nasa.github.io/fprime/v2.0.1/UsersGuide/gds/seqgen.html
- 2: https://nasa.github.io/fprime/v3.4.2/UsersGuide/dev/configuring-fprime.html
- 3: https://nasa.github.io/fprime/UsersGuide/dev/testAPI/markdown/time_type.html
- 4: https://nasa.github.io/fprime/UsersGuide/dev/testAPI/user_guide.html
- 5: https://nasa.github.io/fprime/UsersGuide/api/c++/html/sequence_8cpp_source.html
- 6: https://nasa.github.io/fprime/UsersGuide/api/c++/html/fp_config_8h.html
- 7: https://nasa.github.io/fprime/v3.4.0/UsersGuide/api/c++/html/fp_config_8h.html
- 8: https://nasa.github.io/fprime/v3.3.1/UsersGuide/api/c++/html/fp_config_8h_source.html
- 9: https://nasa.github.io/fprime/UsersGuide/dev/testAPI/markdown/integration_test_api.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== fetch source with insecure SSL (remote docs/source unavailable to command, use deterministic local evidence instead) =="
curl -kL -sS --max-time 20 "https://raw.githubusercontent.com/nasa/fprime-gds/devel/src/fprime_gds/common/models/serialize/time_type.py" | sed -n '1,180p' || true
echo
echo "== repository ruff/python lint config for timezone warnings =="
rg -n "DTZ005|use-datetime-default|datetime-default|flake8-datetimez|ruff|lint" -g 'pyproject.toml' -g 'Makefile' -g '*.ini' . || trueRepository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 6419
🌐 Web query:
github nasa fprime-gds time_type.py TimeBase TB_UTC set_datetime
💡 Result:
In the F Prime (F´) GDS, the TimeType class (found in fprime_gds.common.models.serialize.time_type) is used to handle timestamping data [1]. There is no method named set_datetime in the TimeType class [1]. The TimeType object is initialized using the constructor init(time_base=0, time_context=0, seconds=0, useconds=0) [1]. While the class includes a method called get_datetime(tz=None) to convert a TimeType object into a Python datetime object [1], it does not have a corresponding setter to update the TimeType instance from a datetime object. Regarding TimeBase and TB_UTC: 1. TimeBase: This is an enumeration defining the clocks in the F´ system (e.g., TB_NONE, TB_PROC_TIME, TB_WORKSTATION_TIME, TB_SC_TIME, TB_DONT_CARE) [2][3]. 2. TB_UTC: This is generally not a standard TimeBase enum value in the base F´ framework [2][3]. Time bases are typically defined by the project in FpConfig.fpp [3]. For time manipulation within the GDS test API, the TimeType class supports basic arithmetic (addition, subtraction) and comparisons directly with other TimeType objects or floating-point numbers [4]. If you need to construct a TimeType object, you generally provide the seconds and microseconds directly during instantiation [1]. [1][4][2]
Citations:
- 1: https://nasa.github.io/fprime/UsersGuide/dev/testAPI/markdown/time_type.html
- 2: https://fprime.jpl.nasa.gov/latest/docs/reference/api/cpp/html/class_time_base.html
- 3: https://fprime.jpl.nasa.gov/devel/docs/user-manual/framework/configuring-fprime/
- 4: https://fprime.jpl.nasa.gov/devel/docs/user-manual/gds/gds-test-api-guide/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate TimeType definitions across repo =="
rg -n "class TimeType|def set_datetime|TB_UTC|TB_DONT_CARE" . --glob '!**/.git/**' || true
echo
echo "== all TimeType constructors/assignments =="
rg -n "TimeType\(|\.set_datetime\(" . --glob '!**/.git/**' || true
echo
echo "== check whether TB_UTC is defined in git-tracked files =="
rg -n "TB_UTC" $(git ls-files) 2>/dev/null || true
echo
echo "== inspect repository pyproject for DTZ005, if present =="
python3 - <<'PY'
from pathlib import Path
for p in [Path("pyproject.toml"), Path("ruff.toml"), Path(".ruff.toml")]:
if p.exists():
print(f"--- {p} ---")
txt = p.read_text()
for i, line in enumerate(txt.splitlines(), 1):
if "DTZ005" in line or "use-datetime-default" in line or "timedeltas-only-conversion" in line or "datetime-default" in line:
print(f"{i}: {line}")
PYRepository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 5142
🌐 Web query:
github nasa fprime-gds src/fprime_gds/common/models/serialize/time_type.py TB_UTC
💡 Result:
In the F Prime (F´) framework, TB_UTC is not a standard member of the fprime_gds.common.models.serialize.time_type.TimeType class or the core Fw::Time system. The F Prime time system primarily utilizes time bases such as TB_NONE, TB_WORKSTATION_TIME, and TB_PROC_TIME [1][2]. The confusion regarding TB_UTC often arises from its presence in other unrelated software projects, such as the libzypp library, which uses an enumeration containing TB_UTC for date and time formatting [3][4][5]. Within the F Prime GDS Python codebase, time is managed using the TimeType class [6]. While developers might occasionally define custom time bases or work with external time formats, TB_UTC is not a built-in F Prime constant. If you encounter TB_UTC in an F Prime context, it is likely a custom extension or an error resulting from a misidentification of external library constants. For standard F Prime operations, time bases are typically set using the defined enumerations in the Fw::Time component, and the GDS handles these via the serialization logic in fprime_gds.common.models.serialize.time_type [6][2].
Citations:
- 1: https://github.com/nasa/fprime-gds/blob/devel/src/fprime_gds/common/pipeline/standard.py
- 2: https://github.com/nasa/fprime/blob/589871a957802bb0ea96a98122418a514440cb5b/Fw/Time/Time.cpp
- 3: https://opensuse.github.io/libzypp/classzypp_1_1Date.html
- 4: https://opensuse.github.io/libzypp/functions_eval_t.html
- 5: https://opensuse.github.io/libzypp/functions_t.html
- 6: https://nasa.github.io/fprime/UsersGuide/dev/testAPI/markdown/time_type.html
Use timezone-aware UTC timestamps for event windows.
These test timestamps are used after UTC-based RTC alarm setup, but datetime.now() is host-local and naive. Use datetime.now(timezone.utc) before constructing the TimeType event window. Run make fmt after the update.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 312-312: 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/rtc_test.py` around lines 311 - 313,
Update the event-window timestamp initialization using TimeType.set_datetime to
call datetime.now(timezone.utc) instead of the host-local naive datetime.now().
Ensure the timezone import is available, then run make fmt.
Sources: Coding guidelines, Linters/SAST tools
| assert alarm_canceled_evt.args and alarm_canceled_evt.args[0].val == alarm_id, ( | ||
| f"AlarmCanceled id {alarm_canceled_evt.args[0].val} did not match expected {alarm_id}" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not index an unchecked event argument list.
If AlarmCanceled has no arguments, the assertion message evaluates alarm_canceled_evt.args[0] and raises IndexError. Split the presence check from the ID comparison.
Proposed fix
- assert alarm_canceled_evt.args and alarm_canceled_evt.args[0].val == alarm_id, (
- f"AlarmCanceled id {alarm_canceled_evt.args[0].val} did not match expected {alarm_id}"
- )
+ assert alarm_canceled_evt.args, "AlarmCanceled did not include an alarm ID"
+ assert alarm_canceled_evt.args[0].val == alarm_id, (
+ f"AlarmCanceled id {alarm_canceled_evt.args[0].val} did not match expected {alarm_id}"
+ )📝 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.
| assert alarm_canceled_evt.args and alarm_canceled_evt.args[0].val == alarm_id, ( | |
| f"AlarmCanceled id {alarm_canceled_evt.args[0].val} did not match expected {alarm_id}" | |
| ) | |
| assert alarm_canceled_evt.args, "AlarmCanceled did not include an alarm ID" | |
| assert alarm_canceled_evt.args[0].val == alarm_id, ( | |
| f"AlarmCanceled id {alarm_canceled_evt.args[0].val} did not match expected {alarm_id}" | |
| ) |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 372-374: Assertion should be broken down into multiple parts
(PT018)
🤖 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/rtc_test.py` around lines 372 - 374,
Update the AlarmCanceled assertion in the RTC test to separate validation of
alarm_canceled_evt.args from the ID comparison, ensuring the error message never
indexes args[0] unless an argument exists. Preserve the expected alarm_id
equality check after confirming the argument list is non-empty.
Source: Linters/SAST tools
| alarm_time_data = dict( | ||
| Year=alarm_time.year, | ||
| Month=alarm_time.month, | ||
| Day=alarm_time.day, | ||
| Hour=alarm_time.hour, | ||
| Minute=alarm_time.minute, | ||
| Second=alarm_time.second, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use a dictionary literal.
Ruff C408 flags this redundant dict(...) call. Replace it with a dictionary literal and run make fmt. As per coding guidelines, Python code must use Ruff through make fmt.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 503-503: use jsonify instead of json.dumps for JSON output
Context: json.dumps(alarm_time_data)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.16.1)
[warning] 496-503: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
🤖 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/rtc_test.py` around lines 496 - 503,
Replace the dict(...) constructor assigned to alarm_time_data with an equivalent
dictionary literal containing the existing Year, Month, Day, Hour, Minute, and
Second entries. Run make fmt to apply the required Ruff formatting.
Sources: Coding guidelines, Linters/SAST tools
| # Clean up: cancel the alarm | ||
| fprime_test_api.send_command(f"{rtcManager}.ALARM_CANCEL") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the required alarm ID during cleanup.
ALARM_CANCEL_cmdHandler requires a U16 ID. Line 513 sends no argument. The command can fail before it cancels the retained first alarm, which leaks state into later tests.
Pass the configured alarm ID (0) or capture the first AlarmSet event ID and use it here.
Proposed fix
# Clean up: cancel the alarm
- fprime_test_api.send_command(f"{rtcManager}.ALARM_CANCEL")
+ fprime_test_api.send_command(f"{rtcManager}.ALARM_CANCEL", [0])📝 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.
| # Clean up: cancel the alarm | |
| fprime_test_api.send_command(f"{rtcManager}.ALARM_CANCEL") | |
| # Clean up: cancel the alarm | |
| fprime_test_api.send_command(f"{rtcManager}.ALARM_CANCEL", [0]) |
🤖 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/rtc_test.py` around lines 512 - 513,
Update the cleanup call to ALARM_CANCEL in rtc test teardown to pass the
required U16 alarm ID, using the configured ID 0 or the ID captured from the
initial AlarmSet event, so the retained alarm is reliably cancelled before later
tests run.
There was a problem hiding this comment.
Looks like coderabbit is right about the U16, is this passing without its paramaer?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 '\bALARM_CANCEL\b|ALARM_CANCEL_cmdHandler\b' .Length of output: 19125
@ineskhou, the test can pass without the parameter, but the cleanup command is not valid.
ALARM_CANCEL requires ID: U16 in RtcManager.fpp. The handler also requires U16 ID.
Line 513 only calls send_command. It does not assert the command response or AlarmCanceled. Therefore, the test can finish even if command deserialization or dispatch rejects the empty argument list. In that case, the first alarm remains configured.
Use [0] here. The component sets its alarm with ID 0, and the existing tests already use that ID. An additional AlarmCanceled assertion would verify the cleanup.
🧬 Code Graph Analysis Results
PROVESFlightControllerReference/test/int/common.py:56-109
proves_send_and_assert_command accepts args with a default of [], then passes that list to fprime_test_api.send_and_assert_command. Commands invoked without parameters therefore execute with an empty argument list.
PROVESFlightControllerReference/test/int/rtc_test.py:55-75
set_time constructs a JSON time payload and calls TIME_SET with one argument: [time_data_str].
PROVESFlightControllerReference/test/int/rtc_test.py:78-109
uplink_sequence_and_await_completion generates a sequence using generateSequence(..., 0xFFFF, cont=True), where 0xFFFF is passed as the sequence generator parameter.
You are interacting with an AI system.
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 573-594: Update the alarm conflict test around ALARM_SET and
TIMEBASE_PRM_SET to first assert that the initial ALARM_SET successfully created
the alarm, then select TB_PROC_TIME and assert TimeBaseChanged before checking
cancellation and rejection behavior. Wrap the processor-time conflict assertions
in try/finally, and move the existing restoration to TB_SC_TIME into finally so
cleanup occurs even when an assertion fails.
🪄 Autofix
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: af123be4-d418-45c7-bf92-32d8134b4fdd
📒 Files selected for processing (2)
PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.cppPROVESFlightControllerReference/test/int/rtc_test.py
💤 Files with no reviewable changes (1)
- PROVESFlightControllerReference/Components/Drv/RtcManager/RtcManager.cpp
| fprime_test_api.send_command(f"{rtcManager}.ALARM_SET", [alarm_time_data_str]) | ||
|
|
||
| # Switch to proc time to make sure it is canceled | ||
| proves_send_and_assert_command( | ||
| fprime_test_api, f"{rtcManager}.TIMEBASE_PRM_SET", ["TB_SC_TIME"] | ||
| ) | ||
|
|
||
| # make sure that it is gone | ||
| fprime_test_api.send_command(f"{rtcManager}.ALARM_LIST") | ||
| fprime_test_api.assert_event(f"{rtcManager}.AlarmNotSet", start=start, timeout=10) | ||
|
|
||
| # Make sure we cannot set while in proc time | ||
| fprime_test_api.send_command(f"{rtcManager}.ALARM_SET", [alarm_time_data_str]) | ||
| fprime_test_api.assert_event(f"{rtcManager}.AlarmNotSet", start=start, timeout=10) | ||
|
|
||
| # Set time back to RTC to avoid ruining other tests | ||
| proves_send_and_assert_command( | ||
| fprime_test_api, f"{rtcManager}.TIMEBASE_PRM_SET", ["TB_SC_TIME"] | ||
| ) | ||
| fprime_test_api.assert_event( | ||
| f"{rtcManager}.TimeBaseChanged", start=start, timeout=10 | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Establish the conflict state before asserting its result.
Line 573 does not confirm that ALARM_SET created an alarm. A rejected or unprocessed command leaves no alarm, so Line 582 can pass without testing cleanup.
Line 577 selects TB_SC_TIME, not TB_PROC_TIME. The test therefore does not execute the processor-time conflict path.
After selecting processor time, assert TimeBaseChanged. Wrap the conflict checks in try/finally and move the existing spacecraft-time restoration into finally. A failed assertion must not leave shared hardware in processor time.
Proposed fix
fprime_test_api.send_command(f"{rtcManager}.ALARM_SET", [alarm_time_data_str])
+ fprime_test_api.assert_event(
+ f"{rtcManager}.AlarmSet", start=start, timeout=10
+ )
- fprime_test_api, f"{rtcManager}.TIMEBASE_PRM_SET", ["TB_SC_TIME"]
+ fprime_test_api, f"{rtcManager}.TIMEBASE_PRM_SET", ["TB_PROC_TIME"]🤖 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/rtc_test.py` around lines 573 - 594,
Update the alarm conflict test around ALARM_SET and TIMEBASE_PRM_SET to first
assert that the initial ALARM_SET successfully created the alarm, then select
TB_PROC_TIME and assert TimeBaseChanged before checking cancellation and
rejection behavior. Wrap the processor-time conflict assertions in try/finally,
and move the existing restoration to TB_SC_TIME into finally so cleanup occurs
even when an assertion fails.
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 573-576: Update the RTC test command/assertion sequences around
ALARM_SET, TimeBaseChanged, and AlarmNotSet so each command captures a fresh
event-window timestamp immediately before sending it. Use the matching timestamp
for each subsequent assert_event call, including the final restoration
assertion, instead of reusing the initial start value.
🪄 Autofix
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: 066775ac-c694-463d-b69c-9eb2305f85a2
📒 Files selected for processing (1)
PROVESFlightControllerReference/test/int/rtc_test.py
|
|
||
| # Set an alarm | ||
| fprime_test_api.send_command(f"{rtcManager}.ALARM_SET", [alarm_time_data_str]) | ||
| fprime_test_api.assert_event(f"{rtcManager}.AlarmSet", start=start, timeout=10) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Capture a new event-window timestamp for each command.
start is captured before ALARM_SET and reused by later TimeBaseChanged and AlarmNotSet assertions. If assert_event matches any event after start, the final assertion at Lines 595-597 can match the earlier transition at Lines 579-581 instead of the restoration command. Capture start immediately before each command with a corresponding event assertion.
🤖 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/rtc_test.py` around lines 573 - 576,
Update the RTC test command/assertion sequences around ALARM_SET,
TimeBaseChanged, and AlarmNotSet so each command captures a fresh event-window
timestamp immediately before sending it. Use the matching timestamp for each
subsequent assert_event call, including the final restoration assertion, instead
of reusing the initial start value.
ineskhou
left a comment
ineskhou
left a comment
There was a problem hiding this comment.
Good guard ALARM_SET, good testing cancellation great additions to test suite! Small comments and would love to see some alarm demo if you had chance
| # Assert that we receive an AlarmTriggered event within 10 seconds | ||
| fprime_test_api.await_event(f"{rtcManager}.AlarmTriggered", timeout=10) | ||
| fprime_test_api.assert_event( | ||
| f"{rtcManager}.AlarmTriggered", start=start, timeout=10 |
There was a problem hiding this comment.
Why are you using assert event everywhere instead of await event? await allows us to be more flexible with waiting for commands to come back from my understanding, but is the timing importtant in this case?
|
|
||
| # Switch to proc time to make sure it is canceled | ||
| proves_send_and_assert_command( | ||
| fprime_test_api, f"{rtcManager}.TIMEBASE_PRM_SET", ["TB_SC_TIME"] |
There was a problem hiding this comment.
Based on the comment should we swap to TB_PROC_TIME instead of TB_SC_TIME?
| } | ||
|
|
||
| // Cancel any running sequences, as the change in reported time may impact their behavior | ||
| for (FwIndexType i = 0; i < this->getNum_cancelSequences_OutputPorts(); i++) { |
There was a problem hiding this comment.
side note but we dont check if the timebase has changed from the current timebase here i think, so even if we go from proc to proc we cancel alarms. i think it makes more sense to check, but u could make the argument this is expected behavior roo
| # Clean up: cancel the alarm | ||
| fprime_test_api.send_command(f"{rtcManager}.ALARM_CANCEL") |
There was a problem hiding this comment.
Looks like coderabbit is right about the U16, is this passing without its paramaer?
4 participants
Pull Request Title (e.g., Feature: Add user authentication)
Description
Prevent conflicts with the RTC alarm and timebase parameter
Related Issues/Tickets
#497
How Has This Been Tested?
Screenshots / Recordings (if applicable)
Checklist
Further Notes / Considerations