Skip to content

Conversation

@m-aleem
Copy link
Contributor

@m-aleem m-aleem commented Jul 3, 2025

Related Issue(s) #3783, #3458
Has Unit Tests (y/n) Yes - existing
Documentation Included (y/n) Yes, existing doc updated

Change Description

For #3783:

  1. Remove FW_USE_TIME_BASE and FW_USE_TIME_CONTEXT flags and update corresponding documentation
  2. Move TimeBase enum to FpConfig.fpp and update all references that use these enums (prepend TimeBase::) Note that this also requires a corresponding FPP update CppWriter corresponding to making TimeBase an FPP enum fpp#756
  3. Add FPP Struct for TimeValue and update Time class to use this data structure to store time value
  4. Update FW_RAW_TIME_HANDLE_MAX_SIZE from 32 to 56 (due to build errors)

--

For #3458:

  1. Removed time context comparison in time compare method
  2. Updated FpySequencer to ignore differences in Fw.Time context

Rationale

#3783, #3458

Testing/Review Recommendations

Verified all UTs passed with FPP updates

Future Work

N/A

@m-aleem m-aleem requested a review from LeStarch July 3, 2025 21:45
@m-aleem m-aleem marked this pull request as ready for review July 3, 2025 21:47
@m-aleem
Copy link
Contributor Author

m-aleem commented Jul 3, 2025

@LeStarch This is going to fail CI until we get the FPP updates as discussed. I've opened that PR for Rob in nasa/fpp#756 but other than that the code updates here should be ready for review.

If either of the below are issues, lmk and we can discuss if further changes are needed:

De/Serialization Notes

For reference, here is the (de)serialization logic changes:

  1. Serialize time base, return status if status is not OK
  • Note the old implementation wraps this in a if/endif FW_USE_TIME_BASE flag
  • The old implementation casts TimeBase to a FwTimeBaseStoreType while the new implementation does buffer.serialize(this->m_timeBase)
  • In the new implementation, m_timeBase is a TimeBase type which is generated by an FPP enum of explicit size FwTimeBaseStoreType
  1. Serialize time context, return status if status is not OK
  • Note the old implementation wraps this in a if/endif FW_USE_TIME_CONTEXT flag
  1. Serialize seconds, return status if status is not OK
  2. Serialize useconds, return status if status is not OK

Other Notes

As mentioned in the summary I had to Update FW_RAW_TIME_HANDLE_MAX_SIZE from 32 to 56 (due to build errors/not enough size assertion)

In file included from /Users/aleem/Documents/fprime/git/fprime/Svc/OsTime/test/RawTimeTester/RawTimeTester.cpp:3:
/Users/aleem/Documents/fprime/git/fprime/Os/Delegate.hpp:93:19: error: static assertion failed due to requirement 'sizeof(Svc::RawTimeTester) <= sizeof (aligned_new_memory)': Handle size not large enough
   93 |     static_assert(sizeof(Implementation) <= sizeof(aligned_new_memory), "Handle size not large enough");
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/aleem/Documents/fprime/git/fprime/Svc/OsTime/test/RawTimeTester/RawTimeTester.cpp:9:30: note: in instantiation of function template specialization 'Os::Delegate::makeDelegate<Os::RawTimeInterface, Svc::RawTimeTester, unsigned char[32]>' requested here
    9 |         return Os::Delegate::makeDelegate<RawTimeInterface, Svc::RawTimeTester, RawTimeHandleStorage>(
      |                              ^
/Users/aleem/Documents/fprime/git/fprime/Os/Delegate.hpp:93:42: note: expression evaluates to '56 <= 32'
   93 |     static_assert(sizeof(Implementation) <= sizeof(aligned_new_memory), "Handle size not large enough");
      |                   ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
[77/1159] Building CXX object F-Prime/Os/CMakeFiles/Os_Queue.dir/Queue.cpp.o
ninja: build stopped: subcommand failed.
[ERROR] CMake erred with return code 1

Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor change which will help F Prime be more clear. Thanks!

@m-aleem m-aleem changed the title Update F' Time handling with FPP struct and clean up TIME flags Update F' Time handling with FPP struct and clean up TIME flags, and remove Time Context comparison Jul 7, 2025
@zimri-leisher
Copy link
Collaborator

Love this change! Can't wait for Time not to be a special case.

@m-aleem m-aleem requested review from LeStarch July 14, 2025 18:26
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@LeStarch LeStarch merged commit ef5f255 into nasa:devel Jul 21, 2025
71 checks passed
@m-aleem m-aleem deleted the devel-3783-v2 branch July 22, 2025 18:01
@thomas-bc thomas-bc added Breaking Changes / Needs Release Notes Need to add instructions in the release notes for updates. tmp: Needs Release Note labels Aug 6, 2025
@thomas-bc thomas-bc mentioned this pull request Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking Changes / Needs Release Notes Need to add instructions in the release notes for updates.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants