cFE icon indicating copy to clipboard operation
cFE copied to clipboard

Time Services need cleanup relative to requirements

Open skliper opened this issue 6 years ago • 10 comments

Per CCB on 3/27/19, Time Services goes way beyond it's requirements. Need to re-evaluate configuration options and reduce mission specific code.

As part of the cleanup, factor out duplicate code. Specifically referenced at code review in command handling.

  • CFE_TIME_SetTimeCmd, CFE_TIME_SetMETCmd, CFE_TIME_SetStcfCmd, and CFE_TIME_SetLeapSeconds are all basically the same logic, etc.
  • break of files #1322, specific note from CFS-43 was the lack of a cfe_time_tone.h
  • CFE_TIME_ToneSendGPS and CFE_TIME_ToneSendTime are basically the same logic, refactor
  • #1536
  • #1535
  • Mutually exclusive defines could just be a boolean (but will likely go away w/ refactor), note cfe_time_verify.h is excessively complex at this point https://github.com/nasa/cFE/blob/84ba9a9974794e239b989cdc4e2359216e44fab0/modules/time/fsw/src/cfe_time_verify.h#L43-L51

skliper avatar Sep 30 '19 19:09 skliper

Imported from trac issue 271. Created by jhageman on 2019-04-01T09:44:50, last modified: 2019-05-23T16:43:54

skliper avatar Sep 30 '19 19:09 skliper

Suggestions - Explicitly define TAT pulse and 1Hz pulse, the system should handle various configurations (any combination of 'virtual/internal' and external/API, synced or not)

TAT pulse source selection (SetSource) should be generalized to 'virtual' or passed to PSP, or get triggered via API (generalize implementation of cTIME2010 which currently just does PRIMARY/SECONDARY) 1Hz pulse source selection (new) should be generalized to 'virtual' aka timer based or passed to PSP (expect trigger via API), or based off of Tone Pulse (synced, 1Hz loop triggered by Tone Pulse) MET source selection (new) should be generalized to 'virtual' or passed to PSP, or passed in via API 1Hz pulse when external shall flywheel after mission config elapsed time without a pulse (missing requirement)

Really TIME ifdefs need to be removed, and TIME should just do what's requested when requested.

Note #518 will fix some of the requirements but not all, so revisit after that update.

skliper avatar Feb 19 '20 15:02 skliper

See cTIME2013 - need to simplify configuration to adjust SCTF when requested (regardless of mode).

skliper avatar Feb 19 '20 17:02 skliper

SetSourceCmd - not well defined requirement or implementation. Suggest updates per the 1Hz comment above (sort of tied 1Hz cycle to Tone but needs work).

skliper avatar Feb 19 '20 18:02 skliper

CFE_TIME_Locaal1HzISR related design also needs cleanup and implementation is not consistent with CFE_TIME_Tone1HzISR. Wouldn't expect an "ISR" to be exposed in an API, Tone1HzISR has an ExternalTone API wrapper, etc. See also #551 which only fixed the duplicate prototype, but bigger issues should be addressed as part of refactor.

skliper avatar May 05 '20 11:05 skliper

Also noted in code review, inconsistent parameter names used in the APIs (Time1/2, TimeA/B). Improve consistency.

skliper avatar May 11 '21 19:05 skliper

Also noted in code review, consider merge of external events (CFE_TIME_ExternalMET, CFE_TIME_ExternalGPS, CFE_TIME_ExternalTime). Although might be simpler to just include all three and support selection (internal/virtual/external MET, OS/GPS/Time (and relation to tone), etc) instead of #ifdefs all over. They are APIs, you don't have to use them.

skliper avatar May 11 '21 19:05 skliper

Per CCB on 3/27/19, Time Services goes way beyond it's requirements. Need to re-evaluate configuration options and reduce mission specific code.

As part of the cleanup, factor out duplicate code. Specifically referenced at code review in command handling.

  • CFE_TIME_SetTimeCmd, CFE_TIME_SetMETCmd, CFE_TIME_SetStcfCmd, and CFE_TIME_SetLeapSeconds are all basically the same logic, etc. ...

This first point is addressed here (excl. CFE_TIME_SetLeapSeconds() which does not have enough common logic to justify being included):

  • https://github.com/nasa/cFE/pull/2340

thnkslprpt avatar May 20 '23 04:05 thnkslprpt

Also noted in code review, inconsistent parameter names used in the APIs (Time1/2, TimeA/B). Improve consistency.

This is addressed here:

  • https://github.com/nasa/cFE/pull/2342

thnkslprpt avatar May 20 '23 05:05 thnkslprpt

  • CFE_TIME_ToneSendGPS and CFE_TIME_ToneSendTime are basically the same logic, refactor

The above is addressed here:

  • https://github.com/nasa/cFE/pull/2658

thnkslprpt avatar Jun 24 '25 14:06 thnkslprpt