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

Fix #429, add OS_time_t access functions #723

Merged
merged 3 commits into from
Jan 13, 2021

Conversation

jphickey
Copy link
Contributor

Describe the contribution

Initially the first commit just adds access functions to convert/extract different units from an OS_time_t value - so that other code in CFE/PSP/Apps can be updated to use the access functions and thereby not break when the internal time definition changes. There will then be a second commit that updates the actual time format.

It also uses OS_time_t in the "stat" structure used by the file module rather than having this be a int32.

Minor detail - also updates the pointer argument to OS_SetLocalTime() to be const

Fixes #429

Testing performed
Build and sanity check CFE

Expected behavior changes
Initially just a set of new access functions - no impact to behavior
Eventually will change the internal format of OS_time_t which will break any code that is not using the access functions.
Prototype change of OS_SetLocalTime() should be backward compatible.

System(s) tested on
Ubuntu 20.04
RTEMS 4.11.3

Additional context
Initial commit which just adds the access functions and updates OSAL can be merged at any time, it does not have any dependencies and does not break anything.

Second commit will depend on nasa/cFE#1051 and nasa/PSP#227 (may be a separate PR depending on how review/merge cycles go)

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Dec 31, 2020
@jphickey
Copy link
Contributor Author

This initial change can be reviewed and merged at any time; does not depend on any other change an does not break anything, but we need to get these access functions in place first in order to fix CFE/PSP code.

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
Copy link
Contributor Author

Amended to also include a conversion from "subseconds" (2^-32) back to OS_time_t - since the opposite was provide, should provide conversion for both directions here.

static inline OS_time_t OS_TimeAssembleFromSubseconds(int64 seconds, uint32 subseconds)

This can replace CFE_TIME_Micro2SubSecs and CFE_TIME_Sub2MicroSecs but without the bugs noted in nasa/cFE#1051 (comment)

@skliper
Copy link
Contributor

skliper commented Jan 4, 2021

Did you add unit tests and stubs for all the new APIs?

@jphickey
Copy link
Contributor Author

jphickey commented Jan 4, 2021

Did you add unit tests and stubs for all the new APIs?

There are no stubs because these are all static inline access functions. Most are exercised by existing tests. However I will add a few more test cases to the clock coverage test to make sure they all get called.

Add test cases to coverage test to validate all basic
OS_time_t access operations and conversions.
@astrogeco
Copy link
Contributor

CCB 2021-01-06 APPROVED

@jphickey
Copy link
Contributor Author

jphickey commented Jan 6, 2021

Two comments regarding recent review:

  1. I forgot to mention, that while this adds individual access functions get time in units of milli/micro/nano/sub seconds, it only provides creation function for nano and subseconds - not milli/micro/whole seconds. I figured these were trivial, but in retrospect, maybe we should provide all of them for completeness?
  2. Regarding the unit test of File Time in the OS_stat() call - the purpose of this test is to make sure that the time returned by the system fstat() call (in struct stat) gets translated into the OS_stat_t structure returned by OSAL. The input is a fixed value - always the same. The test case just makes sure the same value appears in the output struct, too. But because only a POSIX 2008 build will copy the full resolution - only this is verified in the test case, so the test case doesn't need conditional logic. I think verifying the seconds made it through is enough, as we don't have a specific requirement for high-res timestamps at this time.

@astrogeco astrogeco added CCB-20210106 and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Jan 6, 2021
Add OS_TimeAssembleFromMilliseconds and OS_TimeAssembleFromMicroseconds
for a complete set of conversion routines in both directions.
@jphickey
Copy link
Contributor Author

jphickey commented Jan 7, 2021

Update - commit f09c57e adds two more conversions to create an OS_time_t from milli- and microseconds. Just the same as the OS_TimeAssembleFromNanoseconds with different units. I decided this was probably worthwhile since all the conversions from an OS_time_t to these units were provided, we should also provide the full set in the opposite direction, too, even if the conversion is pretty trivial.

@astrogeco astrogeco changed the base branch from main to integration-candidate January 12, 2021 19:01
@astrogeco
Copy link
Contributor

@jphickey please check conflicts

astrogeco added a commit to astrogeco/cFS that referenced this pull request Jan 13, 2021
@astrogeco astrogeco merged commit b30e58d into nasa:integration-candidate Jan 13, 2021
@astrogeco
Copy link
Contributor

@jphickey please check conflicts

Nevermind, I think I got it. See b30e58d and CI run at https://github.com/nasa/cFS/pull/174/checks?sha=1a5438081bd92d28fa59b5d3042ea24629d38763

@jphickey jphickey deleted the fix-429-expand-ostimet branch January 27, 2021 14:09
@skliper skliper added this to the 6.0.0 milestone Sep 24, 2021
jphickey pushed a commit to jphickey/osal that referenced this pull request Aug 10, 2022
jphickey pushed a commit to jphickey/osal that referenced this pull request Aug 10, 2022
Fix nasa#723, CFE_EVS_Register const correct and report truncation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use a better time representation in OS_stat call
3 participants