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

CFE_TIME_GetTime() should not return a structure #45

Open
skliper opened this issue Sep 30, 2019 · 8 comments
Open

CFE_TIME_GetTime() should not return a structure #45

skliper opened this issue Sep 30, 2019 · 8 comments
Assignees

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

In general it is not a good idea to return a structure from a function because compilers do not all perform this the same way. Some compilers/ABIs are reasonably efficient at this (as gcc seems to be), but others are not so efficient and will do extra copies of the structure.

@skliper skliper self-assigned this Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Imported from trac issue 14. Created by jphickey on 2014-12-30T22:06:05, last modified: 2019-04-01T16:46:45

@skliper skliper assigned jphickey and unassigned skliper Sep 30, 2019
@skliper skliper added this to the 7.0.0 milestone Jun 3, 2020
@skliper
Copy link
Contributor Author

skliper commented Jun 3, 2020

I suggest we fix this for 7.0. Also problems in SB that I'm addressing.

@skliper
Copy link
Contributor Author

skliper commented Aug 31, 2020

Need to resolve how to handle this. Deprecate the old and and add one with a new name? Or breaking change as part of 7.0 and just fix it to be consistent?

@skliper skliper added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Aug 31, 2020
@jphickey
Copy link
Contributor

I had assumed this would be address as part of a more comprehensive CFE TIME cleanup, as there are so many other issues with this subsystem (#78, #109, #123, and probably more?).

With respect to this particular API we should probably first discuss whether there is still a need/justification for the CFE_MISSION_TIME_CFG_DEFAULT_TAI/CFE_MISSION_TIME_CFG_DEFAULT_UTC platform config options. It is another one of those things that is never really tested - the default has TAI so that is likely the only thing verified. It also seems like apps that depend on the notion of civil/calendar time would need to know whether they are getting UTC or TAI so they'd be better off calling CFE_TIME_GetTAI or CFE_TIME_GetUTC directly. I am not sure what the CFE_TIME_GetTime() is supposed to be for, really.

Perhaps add CFE_TIME_GetCurrentTAI() and CFE_TIME_GetCurrentUTC() that fill the specified output buffer and return int32, and deprecate all current time getter APIs that return a structure directly?

@CDKnightNASA
Copy link
Contributor

Hmm, seems like an opportune time to make cFE Time a modular structure as with cFE msg?

@skliper
Copy link
Contributor Author

skliper commented Aug 31, 2020

Good points. Not a critical change for 7.0, makes sense to me to include as part of the comprehensive cleanup (and possible modularization) post 7.0.

@skliper skliper removed this from the 7.0.0 milestone Aug 31, 2020
@astrogeco astrogeco added CCB-20200902 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Sep 2, 2020
@astrogeco
Copy link
Contributor

CCB 2020-09-02 Will include in bigger time services update

@skliper
Copy link
Contributor Author

skliper commented Apr 20, 2021

Additionally analyze all CFE_TIME_SysTime_t pass by value cases and resolve. Used in a handful of APIs:

cfe_evs.h:CFE_Status_t CFE_EVS_SendTimedEvent(CFE_TIME_SysTime_t Time, uint16 EventID, uint16 EventType, const char *Spec, ...)
cfe_fs.h:CFE_Status_t CFE_FS_SetTimestamp(osal_id_t FileDes, CFE_TIME_SysTime_t NewTimestamp);
cfe_msg.h:CFE_Status_t CFE_MSG_SetMsgTime(CFE_MSG_Message_t *MsgPtr, CFE_TIME_SysTime_t Time);
cfe_time.h:CFE_TIME_SysTime_t CFE_TIME_GetTime(void);
cfe_time.h:CFE_TIME_SysTime_t CFE_TIME_GetTAI(void);
cfe_time.h:CFE_TIME_SysTime_t CFE_TIME_GetUTC(void);
cfe_time.h:CFE_TIME_SysTime_t CFE_TIME_GetMET(void);
cfe_time.h:CFE_TIME_SysTime_t CFE_TIME_GetSTCF(void);
cfe_time.h:CFE_TIME_SysTime_t CFE_TIME_Add(CFE_TIME_SysTime_t Time1, CFE_TIME_SysTime_t Time2);
cfe_time.h:CFE_TIME_SysTime_t CFE_TIME_Subtract(CFE_TIME_SysTime_t Time1, CFE_TIME_SysTime_t Time2);
cfe_time.h:CFE_TIME_Compare_t CFE_TIME_Compare(CFE_TIME_SysTime_t TimeA, CFE_TIME_SysTime_t TimeB);
cfe_time.h:CFE_TIME_SysTime_t CFE_TIME_MET2SCTime(CFE_TIME_SysTime_t METTime);
cfe_time.h:void CFE_TIME_ExternalMET(CFE_TIME_SysTime_t NewMET);
cfe_time.h:void CFE_TIME_ExternalGPS(CFE_TIME_SysTime_t NewTime, int16 NewLeaps);
cfe_time.h:void CFE_TIME_ExternalTime(CFE_TIME_SysTime_t NewTime);
cfe_time.h:void CFE_TIME_Print(char *PrintBuffer, CFE_TIME_SysTime_t TimeToPrint);

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

No branches or pull requests

4 participants