Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify OS_GetLocalTime/OS_SetLocalTime use in relation to OSAL_GETTIME_SOURCE_CLOCK #844

Closed
skliper opened this issue Mar 10, 2021 · 1 comment · Fixed by #937 or #951
Closed
Assignees
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Mar 10, 2021

Is your feature request related to a problem? Please describe.
Not really clear/consistent on OS_SetLocalTime and OS_GetLocalTime in relation to OSAL_GETTIME_SOURCE_CLOCK. Currently defaulted to CLOCK_MONOTONIC, so a OS_SetLocalTime may fail. Really need to clarify intent of the design/APIs to be more clear on how they should be used since the OSAL_GETTIME_SOURCE_CLOCK is used in both and the setting has different impacts in the different contexts.

Describe the solution you'd like
May benefit from individual configuration parameters, or maybe OS_SetLocalTime should never be CLOCK_MONOTONIC. Add a bit more context to API/configuration documentation. Use cases?

Describe alternatives you've considered
Anything to clarify/explain intent.

Additional context
Debated "bug", could switch.

Requester Info
Jacob Hageman - NASA/GSFC, OSAL code review

@skliper skliper added the CFS-38 label Mar 10, 2021
@skliper skliper added this to the 6.0.0 milestone Mar 10, 2021
@jphickey
Copy link
Contributor

I looked into the use cases of these functions:

  • OS_SetLocalTime() is not used/invoked by any framework code nor any GSFC CFS app. The only place this is called is in the tests.
  • OS_GetLocalTime() is used in a couple limited spots, but not to get the "real" time - it calls it multiple times and gets the difference between the samples to find the elapsed time between them.

In particular on pc-linux the CFE PSP uses OS_GetLocalTime() to implement the PSP timebase used for performance monitoring. In this case it is important that the clock used by this is stable over time and is NOT reset (which would break the perflog creating a discontinuity).

Given these use-cases, I think it is correct to be using MONOTONIC to implement this. However I agree it is a misnomer, as the "LocalTime" name suggests it is indeed a calendar time of some sort.

Perhaps we need a different function?

The CFE PSP use case can (and probably should) be changed to call the clock_gettime directly, then maybe this can be defined as a real time/calendar time.

jphickey added a commit to jphickey/osal that referenced this issue Mar 31, 2021
The portable clock_gettime implementation had been using CLOCK_MONOTONIC
to support its use as a PSP timebase for some platforms that used it this
way.  However with updates on the PSP side this is not required anymore.

Preference should be to use CLOCK_REALTIME as it better aligns with the
described semantics of the OSAL clock function, and makes for a better
default.  This can still be easily changed back if the user desires.
astrogeco added a commit that referenced this issue Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants