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

Deprecate/Delete CFE2FSSeconds and FS2CFESeconds #519

Closed
skliper opened this issue Feb 19, 2020 · 8 comments · Fixed by #670 or #692
Closed

Deprecate/Delete CFE2FSSeconds and FS2CFESeconds #519

skliper opened this issue Feb 19, 2020 · 8 comments · Fixed by #670 or #692
Assignees
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Feb 19, 2020

Is your feature request related to a problem? Please describe.
CFE2FSSeconds and FS2CFESeconds never clearly required/defined.

Describe the solution you'd like
Remove unrequired functionality.

Describe alternatives you've considered
Define explicit use case, requirement, and testing within cFE context.

Additional context
See #302 and #518

Requester Info
Jacob Hageman - NASA/GSFC

@ejtimmon
Copy link
Contributor

FYI - FM uses CFE_TIME_FS2CFESeconds. SCH also uses it in unit tests.

@skliper
Copy link
Contributor Author

skliper commented Apr 20, 2020

@ejtimmon are they required? Note it just applies a compile time defined conversion factor, which doesn't seem all that useful. Seems like it would need to be managed per reset/seed of FS time, and/or if mission time changes. What's the actual use case? Does FS get local file creation time and try to convert it?

@jphickey
Copy link
Contributor

How is these functions different than CFE_TIME_GetUTC()? Wouldn't the "filesystem time" typically be UTC based like anything else is?

@ejtimmon
Copy link
Contributor

@skliper It's used in the GetFileInfo command to get the file time. The sequence is OS_stat -> OS_FILESTAT_TIME() -> CFE_TIME_FS2CFESeconds.

@skliper
Copy link
Contributor Author

skliper commented Apr 20, 2020

Well, that makes things complicated. OSAL just uses stat to set FileTime when OS_FileStat is called. Seems like any hard coded conversion could be misleading, and depends on OS_SetLocalTime use and the mission time management scheme. CFE2FSSeconds/FS2CFESeconds are likely to be more often wrong than right if I understand things correctly (any hard reset or mission time updates can change the conversion factor). Could report files being created in the future depending on how missions manage local time.

Seems to me like a better approach is to not convert, and allow the mission to manage local time if they want? Basically they'd need to keep local time in sync with system time, and then the file timestamps would make more sense... otherwise you'd either need active updates to the conversion whenever time management/resets happen, or you'd need to manage the file timestamp all over the place (any writes, etc).

@jphickey
Copy link
Contributor

This is somewhat broken in other ways, in particular it is still using just a 32-bit integer for time storage. Any OS implementation should (hopefully) have moved beyond this by now because of the 2038 bug. On space mission timescales we may very well be risking that issue already (code written now may very well be flying in 2038).

Since the file time is accessed through a OS_FILESTAT_TIME macro it should be possible to replace this with at least OS_time_t. Then there should be conversion functions that map/convert between OS_time_t (e.g. UNIX time) and CFE_SysTime_t (MET). CFE TIME should (mostly?) provide this already, I think....

@skliper
Copy link
Contributor Author

skliper commented Apr 20, 2020

@ejtimmon Since CFE2FS/FS2CFE conversion functions aren't really implemented in a way that works (other than recompiling whenever the offset changes), and ideally no conversion would be needed (either convert internally or keep system and local time in sync), could we go ahead and deprecate these?

@ejtimmon
Copy link
Contributor

@skliper Yes, go ahead. I'll make a ticket against FM - will probably remove the conversion and make a note in the user guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants