nasa / cFE

The Core Flight System (cFS) Core Flight Executive (cFE)
Apache License 2.0
408 stars 200 forks source link

Time Services need cleanup relative to requirements #302

Open skliper opened 4 years ago

skliper commented 4 years ago

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.

skliper commented 4 years ago

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

skliper commented 4 years ago

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 commented 4 years ago

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

skliper commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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

skliper commented 3 years ago

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.

thnkslprpt commented 1 year ago

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):

thnkslprpt commented 1 year ago

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

This is addressed here: