Add OsTime component #3355

kubiak-jpl · 2025-03-10T21:08:55Z


*Related Issue(s)*	#3335
*Has Unit Tests (y/n)*	y
*Documentation Included (y/n)*	y

Change Description

Creates a Time provides that uses the underlying OSAL RawTime interfaces to generate timestamps.

The RawTime layer only provides time intervals so the OsTime class needs to be passed a pair of epoch times, an Fw::Time and Os::RawTime object that correspond to the same moment in time. OsTime calculates the offset from the epoch RawTime as an Fw::TimeInterval, then adds this interval to the epoch Fw::Time object to calculate the final Fw::Time object.

Rationale

Provide a generic Time provider that can use any RawTime implementation. Original rationale is for microcontroller platforms which cannot use the existing PosixTime or ChronoTime components.

Testing/Review Recommendations

Provided unit tests are fairly barebones and just show the general behavior. It is expected that the underlying RawTime implementations will be more throughly tested.

How should overflow of the Fw::Time be handled, if at all?

Future Work

Possible extensions:

EVR on RawTime errors. These are not implemented because EVRs themselves require a Time call.

LeStarch · 2025-03-12T00:36:38Z

Svc/OsTime/docs/sdd.md

+|-----------------|--------------------------------------------------------------------------------------------------------------------|--------------|
+| SVC-OS-TIME-001 | `Svc::OsTime` shall return current system time as an `Fw::Time` objects in response to the `timeGetPort` port call | Unit Test    |
+| SVC-OS-TIME-002 | `Svc::OsTime` shall return ZERO_TIME as an `Fw::Time` object if the RawTime layer returns an error code            | Unit Test    |
+


Would you add a section for configuration or setup or usage? I get what you are trying to do, but I a write-up of what I need to call in the topology to set this up would be nice.

Sure. Is there an existing component with a documentation structure you'd prefer I'd use here?

I updated the sdd in the latest comment

Svc/OsTime/OsTime.cpp

LeStarch · 2025-03-12T00:39:16Z

@thomas-bc would you be willing to review too? You are the author of RawTime.

Svc/OsTime/OsTime.cpp

Svc/OsTime/OsTime.hpp

Svc/OsTime/OsTime.cpp

+OsTime ::~OsTime() {}
+
+void OsTime::set_epoch(const Fw::Time& fw_time, const Os::RawTime& os_time) {
+    m_epoch_fw_time = fw_time;


Svc/OsTime/OsTime.cpp

+
+void OsTime::set_epoch(const Fw::Time& fw_time, const Os::RawTime& os_time) {
+    m_epoch_fw_time = fw_time;
+    m_epoch_os_time = os_time;


Svc/OsTime/OsTime.cpp

+// ----------------------------------------------------------------------
+
+void OsTime ::timeGetPort_handler(FwIndexType portNum, Fw::Time& time) {
+    time = Fw::ZERO_TIME;


Svc/OsTime/OsTime.cpp

kubiak-jpl · 2025-03-13T16:46:53Z

@thomas-bc I noticed the MacOS unit test is failing, probably because the UTs rely on sleeping and time checks. I though 100 ms was generous enough, but I guess not. Is there a recommended way to stub out the OSAL layer for components UTs like this?

thomas-bc · 2025-03-13T18:10:06Z

@kubiak-jpl I would have thought 100ms be more than enough too... Running your tests on an M4 macOS the tests start failing when approaching 15-10ms threshold, but the GitHub VMs are busy bees... also not sure how much more precision we can expect from std::sleep_for + wake up time

I'm going to re-run your tests in CI once and see if that was transient or if we need to rethink a little. What are you thinking of stubbing the OSAL layer for here?

thomas-bc · 2025-03-13T18:16:10Z

Svc/OsTime/OsTime.cpp

+OsTime ::~OsTime() {}
+
+void OsTime::set_epoch(const Fw::Time& fw_time, const Os::RawTime& os_time) {
+    m_epoch_fw_time = fw_time;


Suggested change

m_epoch_fw_time = fw_time;

(void) m_epoch_fw_time = fw_time;

To appease CI warnings, and in the other uses of time_x = time_y below

That approach gives me compiler errors on my machine

[ 77%] Building CXX object F-Prime/Svc/OsTime/CMakeFiles/Svc_OsTime.dir/OsTime.cpp.o /home/kubiak/source/fprime/fprime-kubiak/Svc/OsTime/OsTime.cpp: In member function ‘void Svc::OsTime::set_epoch(const Fw::Time&, const Os::RawTime&)’: /home/kubiak/source/fprime/fprime-kubiak/Svc/OsTime/OsTime.cpp:31:30: error: invalid use of ‘void’ 31 | (void) m_epoch_fw_time = fw_time; | ^~~~~~~ /home/kubiak/source/fprime/fprime-kubiak/Svc/OsTime/OsTime.cpp:32:30: error: invalid use of ‘void’ 32 | (void) m_epoch_os_time = os_time; | ^~~~~~~ /home/kubiak/source/fprime/fprime-kubiak/Svc/OsTime/OsTime.cpp:33:28: error: invalid use of ‘void’ 33 | (void) m_epoch_valid = true; | ^~~~ /home/kubiak/source/fprime/fprime-kubiak/Svc/OsTime/OsTime.cpp: In member function ‘virtual void Svc::OsTime::timeGetPort_handler(FwIndexType, Fw::Time&)’: /home/kubiak/source/fprime/fprime-kubiak/Svc/OsTime/OsTime.cpp:60:19: error: invalid use of ‘void’ 60 | (void) time = m_epoch_fw_time;

My bad sorry, looks like the syntax should be the following for return values of the assignment operator

(void) (m_epoch_fw_time = fw_time);

That's pretty ugly 😮‍💨

m_epoch_valid = true; probably doesn't need a cast

We can also just dismiss the warning

I'd prefer to dismiss the warnings. I'm not really sure what it even means to check the return type of an assignment outside of using the assigned variable

thomas-bc · 2025-03-13T18:42:22Z

Svc/OsTime/test/ut/OsTimeTester.cpp

+      ASSERT_NE(time_600ms, Fw::ZERO_TIME);
+
+      const double time_600 = time_600ms.getSeconds() + (time_600ms.getUSeconds() / (1000.*1000.));
+      ASSERT_NEAR(0.6, time_600, 0.1);


For the tests that are causing issues, maybe rather than ASSERT_NEAR, you could e.g.

ASSERT_GT(time_600, 0.6) // should be deterministically true ASSERT_LT(time_600, 0.6 * 1.5) // give some extra time to the slow github runners

I tried something similar to this. Let's see if it passes CI.

I think the ideal solution would be for a set a OSAL implementations that are designed for unit testing. Like a UtRawTime implementation that I could pass Fw::Time structures to for unit testing, but that would probably be a bigger discussion outside of the PR

kubiak-jpl · 2025-03-13T19:24:55Z

@LeStarch @thomas-bc

I made a few more substantial changes since the initial commit

I added a setEpoch port call so that another component could set the new epoch time. I'm thinking of a time sync command handler here
Added an Os::Mutex and a ScopeLock to protect the epoch variables

Svc/OsTime/OsTime.cpp

+// Handler implementations for user-defined typed input ports
+// ----------------------------------------------------------------------
+
+void OsTime ::timeGetPort_handler(FwIndexType portNum, Fw::Time& time) {


Svc/OsTime/OsTime.cpp

+        return;
+    }
+
+    time = m_epoch_fw_time;


Svc/OsTime/OsTime.cpp

+// Handler implementations for user-defined typed input ports
+// ----------------------------------------------------------------------
+
+void OsTime ::timeGetPort_handler(FwIndexType portNum, Fw::Time& time) {


Svc/OsTime/OsTime.cpp

+    time.add(elapsed.getSeconds(), elapsed.getUSeconds());
+}
+
+void OsTime ::setEpoch_handler(FwIndexType portNum, const Fw::Time& fw_time, const Os::RawTime& os_time) {


Svc/OsTime/OsTime.hpp

+
+    Fw::Time m_epoch_fw_time;
+    Os::RawTime m_epoch_os_time;
+    bool m_epoch_valid;


LeStarch

Need to fix UTs on mac. It would be best to define the port outside of the component as that keeps components from having module-link dependencies on one-another.

LeStarch · 2025-03-13T21:05:51Z

Svc/OsTime/OsTime.fpp

+
+    @ A pair of timestamps representing an Epoch time in 
+    @ an Fw::Time and Os::RawTime object
+    port OsTimeEpoch(


For module independence, ports should typically be defined outside of the component.

Sure. Where would you like me to add it?

There's an Svc/Ports/ module that this would likely fall under nicely.

Svc/OsTime/OsTime.cpp

+
+void OsTime::set_epoch(const Fw::Time& fw_time, const Os::RawTime& os_time) {
+    Os::ScopeLock lock(m_epoch_lock);
+    m_epoch_fw_time = fw_time;


Svc/OsTime/OsTime.cpp

+void OsTime::set_epoch(const Fw::Time& fw_time, const Os::RawTime& os_time) {
+    Os::ScopeLock lock(m_epoch_lock);
+    m_epoch_fw_time = fw_time;
+    m_epoch_os_time = os_time;


Svc/OsTime/OsTime.cpp

+void OsTime ::timeGetPort_handler(FwIndexType portNum, Fw::Time& time) {
+    Os::ScopeLock lock(m_epoch_lock);
+
+    time = Fw::ZERO_TIME;


Svc/OsTime/OsTime.cpp

+}
+
+void OsTime ::setEpoch_handler(FwIndexType portNum, const Fw::Time& fw_time, const Os::RawTime& os_time) {
+    set_epoch(fw_time, os_time);


Svc/OsTime/OsTime.cpp

+}
+
+void OsTime ::setEpoch_handler(FwIndexType portNum, const Fw::Time& fw_time, const Os::RawTime& os_time) {
+    set_epoch(fw_time, os_time);


…::Time objects

kubiak-jpl · 2025-03-14T15:44:05Z

@thomas-bc @LeStarch I reworked the unit tests to remove timing dependencies by adding a RawTimeTester module under the OsTime test directory. I suspect this could be more broadly useful though

kubiak-jpl and others added 4 commits March 5, 2025 16:23

OsTime proof of concept

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

cf6226b

Updated documentation and unit tests

b156ba9

Add no epoch test

Loading
Loading status checks…

8a8586f

Merge branch 'devel' into ostime-rawtime

Loading
Loading status checks…

75c3361

LeStarch requested changes Mar 12, 2025

View reviewed changes

LeStarch requested a review from thomas-bc March 12, 2025 00:38

github-advanced-security bot found potential problems Mar 12, 2025

View reviewed changes

Svc/OsTime/OsTime.cpp Dismissed Show dismissed Hide dismissed

Svc/OsTime/OsTime.cpp Fixed Show fixed Hide fixed

Svc/OsTime/OsTime.hpp Fixed Show resolved Hide resolved

github-advanced-security bot found potential problems Mar 12, 2025

View reviewed changes

Svc/OsTime/OsTime.cpp Fixed Show fixed Hide fixed

Svc/OsTime/OsTime.cpp Fixed Show fixed Hide fixed

Svc/OsTime/OsTime.cpp Fixed Show resolved Hide resolved

github-advanced-security bot found potential problems Mar 12, 2025

View reviewed changes

thomas-bc reviewed Mar 13, 2025

View reviewed changes

kubiak-jpl added 6 commits March 13, 2025 12:05

Add kubiak to expect.txt

df244a9

Updates to OsTime to support setEpoch port and data structure locking

7249bba

Address analyzer warnings

Loading
Loading status checks…

bc4c324

Fix one more CI warning

Loading
Loading status checks…

d3fd013

Remove use of void

76e6fab

Rework timing assertions

Loading
Loading status checks…

060e7a0

kubiak-jpl and others added 2 commits March 13, 2025 12:27

Fix spelling of epoch

Loading
Loading status checks…

e2262f5

Merge branch 'devel' into ostime-rawtime

Loading
Loading status checks…

06f9b54

github-advanced-security bot found potential problems Mar 13, 2025

View reviewed changes

Svc/OsTime/OsTime.cpp

return;

}

time = m_epoch_fw_time;

Check warning

Code scanning / CodeQL

Unchecked return value Warning

The return value of non-void function

operator=

Loading
is not checked.

github-advanced-security bot found potential problems Mar 13, 2025

View reviewed changes

LeStarch requested changes Mar 13, 2025

View reviewed changes

Move OsTimeEpoch port inside a Ports directory

Loading
Loading status checks…

ea22a6e

github-advanced-security bot found potential problems Mar 13, 2025

View reviewed changes

kubiak-jpl added 2 commits March 14, 2025 08:41

Update OsTime tests to use RawTimeTester

330b31b

Increase config FW_RAW_TIME_HANDLE_MAX_SIZE to 32 bytes to support Fw…

Loading
Loading status checks…

35e25a2

…::Time objects

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OsTime component #3355

Add OsTime component #3355

kubiak-jpl commented Mar 10, 2025 •

edited

Loading

LeStarch Mar 12, 2025

kubiak-jpl Mar 12, 2025

kubiak-jpl Mar 13, 2025

LeStarch commented Mar 12, 2025

kubiak-jpl commented Mar 13, 2025

thomas-bc commented Mar 13, 2025 •

edited

Loading

thomas-bc Mar 13, 2025 •

edited

Loading

kubiak-jpl Mar 13, 2025

thomas-bc Mar 13, 2025

thomas-bc Mar 13, 2025

kubiak-jpl Mar 13, 2025

thomas-bc Mar 13, 2025

kubiak-jpl Mar 13, 2025

kubiak-jpl commented Mar 13, 2025

LeStarch left a comment

LeStarch Mar 13, 2025

kubiak-jpl Mar 13, 2025

thomas-bc Mar 13, 2025

kubiak-jpl commented Mar 14, 2025

Add OsTime component #3355

Are you sure you want to change the base?

Add OsTime component #3355

Conversation

kubiak-jpl commented Mar 10, 2025 • edited Loading

Change Description

Rationale

Testing/Review Recommendations

Future Work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeStarch commented Mar 12, 2025

kubiak-jpl commented Mar 13, 2025

thomas-bc commented Mar 13, 2025 • edited Loading

thomas-bc Mar 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubiak-jpl commented Mar 13, 2025

LeStarch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubiak-jpl commented Mar 14, 2025

kubiak-jpl commented Mar 10, 2025 •

edited

Loading

thomas-bc commented Mar 13, 2025 •

edited

Loading

thomas-bc Mar 13, 2025 •

edited

Loading