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

Use a better time representation in OS_stat call #429

Closed
jphickey opened this issue Apr 20, 2020 · 3 comments · Fixed by #723, #735, #750 or #767
Closed

Use a better time representation in OS_stat call #429

jphickey opened this issue Apr 20, 2020 · 3 comments · Fixed by #723, #735, #750 or #767
Assignees
Milestone

Comments

@jphickey
Copy link
Contributor

Is your feature request related to a problem? Please describe.
The OS_stat call currently returns the file time as an int32 member within the os_fstat_t structure, as defined here:

int32 FileTime;

This isn't really documented in the API but the field is a traditional UNIX-style timestamp, which is seconds elapsed since Jan 1 1970 UTC.

This type of timestamp suffers from the "year 2038" bug, where the int32 value rolls over and becomes negative. Although this is 18 years from now, at the timescales of space software development cycles, it is entirely possible that coding being developed now will still be in service at the time this happens, so it should be fixed sooner rather than later.

Describe the solution you'd like
There are two fixes needed:

  1. use the OS_time_t representation as used in OS_GetLocalTime and OS_SetLocalTime. This is just for consistency - shouldn't use a different representation of time as the other API calls do.
  2. Fix the OS_time_t to accommodate larger timestamp values and/or use a different epoch (latter would be risk but keep the structure the same size).

Additional context
Discussion regarding use of this field in nasa/cFE#519

Requester Info
Joseph Hickey, Vantage Systems, Inc.

@skliper skliper added the bug label Dec 9, 2020
@skliper skliper added this to the 6.0.0 milestone Dec 9, 2020
@jphickey
Copy link
Contributor Author

Two potential approaches:

  1. Expand the "seconds" field inside OS_time_t to be 64 bits instead of 32 bits

Advantage: Should be mostly backward compatible. Code reading "seconds" and "microsecs" fields should still work as expected.
Disadvantage: Makes the OS_time_t structure bigger - 16 bytes instead of 8, wasting 4 bytes. The backward compatibility could also be viewed as a disadvantage because application code can still be using uint32 when reading the seconds field and get a silent truncation down to 32 bits, thereby still having a year 2038 bug.

  1. Change OS_time_t to be a single 64 bit microseconds counter.

Advantage: Keeps the same size (8) and resolution (microseconds) but expands the useful range to over 100,000 years. Also makes adding and subtracting times much simpler as simple arithmetic just works and no need to worry about carrying over microseconds to seconds.
Disadvantage: Code directly reading the seconds/microseconds field will break (but see 1 above - could be an advantage as it forces developers to look at the code and make sure they are using wide enough types to not have a year 2038 issue). Also can be mitigated with some macros to get seconds/microseconds from the structure. Will still require an app/code change, but simple to fix.

My preference leans toward the option 2 above, because of the simplicity it allows with respect to time arithmetic.

@jphickey
Copy link
Contributor Author

After some more experimentation, I am still leaning toward (2) above. It will break the API by changing the format of this structure, but the net result will be more robust and easier to use.

To minimize breakage it needs to be done in 3 steps:

Step 1 - Add static inline functions to serve as Accessors for OS_time_t values. There should be (at least):

  • Access whole seconds from OS_time_t: int64 OS_TimeGetTotalSeconds(OS_time_t tm) - replaces direct access to the .seconds member.
  • Access microseconds (fractional part only) from OS_time_t: uint32 OS_TimeGetMicrosecondsPart(OS_time_t tm) - replaces direct access to the .microsecs member.
  • Compose/Create an OS_time_t from whole seconds + fractional part: OS_time_t OS_TimeInterval(int64 seconds, int32 nanoseconds) - replaces direct initialization of OS_time_t value. Note - Suggesting going with nanoseconds here rather than microseconds, since it still fits in 32 bit value, and the extra resolution might be useful someday even if initially it just gets multiplied by 1000.

Not strictly necessary initially but recommend adding for completeness, as it allows use of more optimized conversions depending on how OS_time_t gets ultimately defined:

  • Get fractional part as fixed-point 32 bit value: uint32 OS_TimeGetSubseconds32Part(OS_time_t tm) - intended to be compatible with/usable as CFE TIME "subseconds" i.e. units of 1/2^32.
  • Get fractional part in other standard units: uint32 OS_TimeGetNanosecondsPart(OS_time_t tm), uint32 OS_TimeGetMillisecondsPart(OS_time_t tm), etc.
  • Get whole interval in other standard units: int64 OS_TimeGetTotalNanoseconds(OS_time_t tm), int64 OS_TimeGetTotalMicroseconds(OS_time_t tm), int64 OS_TimeGetTotalMilliseconds(OS_time_t tm), etc.
  • Add and Subtract time values: OS_time_t OS_TimeAdd(OS_time_t time1, OS_time_t time2), OS_time_t OS_TimeSubtract(OS_time_t time1, OS_time_t time2)

Step 2 - Scrub all refs in CFE/Apps to use the accessors above rather than accessing OS_time_t fields directly (outside OSAL).

Step 3 - Change OSAL OS_time_t definition to be more optimal - So long as apps are using the accessors they should be sufficiently insulated from the specific definition of OS_time_t so it can change however it needs to without breaking code.

jphickey added a commit to jphickey/osal that referenced this issue Dec 31, 2020
Do not access members of OS_time_t directly, instead
use conversion/accessor inline functions to get the
desired value.
jphickey added a commit to jphickey/osal that referenced this issue Dec 31, 2020
Do not access members of OS_time_t directly, instead
use conversion/accessor inline functions to get the
desired value.
jphickey added a commit to jphickey/osal that referenced this issue Dec 31, 2020
Do not access members of OS_time_t directly, instead
use conversion/accessor inline functions to get the
desired value.
jphickey added a commit to jphickey/osal that referenced this issue Dec 31, 2020
Do not access members of OS_time_t directly, instead
use conversion/accessor inline functions to get the
desired value.

Update the "stat" structure output by OS_stat to use
the OS_time_t struct instead of int32, and update
the OS_stat implemention to transfer the full resolution
if it supports it (POSIX.1-2008 or newer).
jphickey added a commit to jphickey/osal that referenced this issue Dec 31, 2020
Do not access members of OS_time_t directly, instead
use conversion/accessor inline functions to get the
desired value.

Update the "stat" structure output by OS_stat to use
the OS_time_t struct instead of int32, and update
the OS_stat implemention to transfer the full resolution
if it supports it (POSIX.1-2008 or newer).
jphickey added a commit to jphickey/osal that referenced this issue Jan 4, 2021
Add test cases to coverage test to validate all basic
OS_time_t access operations and conversions.
jphickey added a commit to jphickey/osal that referenced this issue Jan 4, 2021
Use a single 64-bit tick counter as OS_time_t, rather than
a split 32 bit seconds + 32 bit microseconds counter.

This benefits in several ways:

- increases the timing precision by 10x (0.1us ticks)
- increases the representable range by 400x (+/-14000 yrs)
- simplifies addition/subtraction (no carry over)
- avoids "year 2038" bug w/32-bit timestamps
@jphickey
Copy link
Contributor Author

jphickey commented Jan 4, 2021

Regarding the PRs for this - this currently has two PRs linked to close it.

First is #723, which represents "step 1" in my previous comment above and has no special dependencies and does not break anything.

Second is #735, which represents "step 3" in my previous comment above. This has to be merged with the other PRs that resolve its dependencies.

Submitted separately so the CCB can choose how to merge them (play it slow or just put them all in at once).

jphickey added a commit to jphickey/osal that referenced this issue Jan 7, 2021
Add OS_TimeAssembleFromMilliseconds and OS_TimeAssembleFromMicroseconds
for a complete set of conversion routines in both directions.
astrogeco added a commit to astrogeco/osal that referenced this issue Jan 13, 2021
jphickey added a commit to jphickey/osal that referenced this issue Jan 14, 2021
Use a single 64-bit tick counter as OS_time_t, rather than
a split 32 bit seconds + 32 bit microseconds counter.

This benefits in several ways:

- increases the timing precision by 10x (0.1us ticks)
- increases the representable range by 400x (+/-14000 yrs)
- simplifies addition/subtraction (no carry over)
- avoids "year 2038" bug w/32-bit timestamps
astrogeco added a commit that referenced this issue Jan 25, 2021
jphickey pushed a commit to jphickey/osal that referenced this issue Aug 10, 2022
jphickey pushed a commit to jphickey/osal that referenced this issue Aug 10, 2022
Fix nasa#415, nasa#429
Reviewed and approved at 2019-12-18 CCB
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment