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.

Sign up for GitHub

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

Jump to bottom

fix#2316 - CFE_TIME_Print() calls strftime() #2356

Merged
merged 1 commit into from
Jun 1, 2023
Merged

fix#2316 - CFE_TIME_Print() calls strftime() #2356

merged 1 commit into from
Jun 1, 2023

Conversation

CDKnightNASA
Copy link
Contributor

@CDKnightNASA CDKnightNASA commented May 26, 2023 •
edited
Loading

Checklist (Please check before submitting)

Describe the contribution
CFE_TIME_Print() calls strftime to format times and returns a status code rather than creating a syslog entry. Note that this may be expanded to allow this to be configurable or parameterized (although EVS may just call strftime directly.)

Also collapsed epoch defines to SECONDS and MICROS for more performance and simpler configuration.

Testing performed
Standard build and UT tests updated.

Expected behavior changes
Function returns CFE_Status_t status.
NOTE strftime uses time_t which may be 32- or 64-bit and may be signed or unsigned, depending on the platform.

System(s) tested on
Ubuntu 22.04LTS

Additional context
https://en.wikipedia.org/w/index.php?title=Time_t&oldid=450752800

Third party code
None.

Contributor Info - All information REQUIRED for consideration of pull request
[email protected]

@CDKnightNASA CDKnightNASA self-assigned this May 26, 2023
@CDKnightNASA CDKnightNASA marked this pull request as draft May 26, 2023 14:57
@CDKnightNASA
Copy link
Contributor Author

Switching this to draft as I consider whether we can, instead, just deprecate and point folks at strftime directly.

github-advanced-security[bot]
github-advanced-security bot found potential problems May 26, 2023
View reviewed changes
modules/core_api/ut-stubs/src/cfe_time_stubs.c
UT_GenStub_AddParam(CFE_TIME_Print, char *, PrintBuffer);
UT_GenStub_AddParam(CFE_TIME_Print, CFE_TIME_SysTime_t, TimeToPrint);

UT_GenStub_Execute(CFE_TIME_Print, Basic, UT_DefaultHandler_CFE_TIME_Print);

return UT_GenStub_GetReturnValue(CFE_TIME_Print, CFE_Status_t);

Check warning

Code scanning / CodeQL

Uses of recursion

The function CFE_TIME_Print is indirectly recursive via this call to [UT_Stub_GetReturnValuePtr](1).
modules/core_api/ut-stubs/src/cfe_time_stubs.c
{
UT_GenStub_SetupReturnBuffer(CFE_TIME_Print, CFE_Status_t);

Check warning

Code scanning / CodeQL

Uses of recursion

The function CFE_TIME_Print is indirectly recursive via this call to [UT_Stub_RegisterReturnType](1).
modules/core_api/ut-stubs/src/cfe_time_stubs.c
@@ -305,12 +305,16 @@
* Generated stub function for CFE_TIME_Print()
* ----------------------------------------------------
*/
void CFE_TIME_Print(char *PrintBuffer, CFE_TIME_SysTime_t TimeToPrint)
CFE_Status_t CFE_TIME_Print(char *PrintBuffer, CFE_TIME_SysTime_t TimeToPrint)

Check notice

Code scanning / CodeQL

Long function without assertion

All functions of more than 10 lines should have at least one assertion.
modules/time/fsw/src/cfe_time_api.c
@@ -563,150 +565,28 @@
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
void CFE_TIME_Print(char *PrintBuffer, CFE_TIME_SysTime_t TimeToPrint)
CFE_Status_t CFE_TIME_Print(char *PrintBuffer, CFE_TIME_SysTime_t TimeToPrint)

Check notice

Code scanning / CodeQL

Long function without assertion

All functions of more than 10 lines should have at least one assertion.
@CDKnightNASA CDKnightNASA force-pushed the fix-2316-time_fmt branch 2 times, most recently from 5100004 to 861a42f Compare May 26, 2023 16:39
@CDKnightNASA CDKnightNASA marked this pull request as ready for review May 26, 2023 16:54
@dzbaker dzbaker added the CCB:Approved Indicates code review and approval by community CCB label Jun 1, 2023
dzbaker added a commit to nasa/cFS that referenced this pull request Jun 1, 2023
@dzbaker
IC: Caelum-rc4+dev52
eea5d66 
*Combines:*

cFE v7.0.0-rc4+dev334
osal v6.0.0-rc4+dev217

**Includes:**

*cFS*
- #680
- #676

*cFE*
- nasa/cFE#2350
- nasa/cFE#2354
- nasa/cFE#2355
- nasa/cFE#2366
- nasa/cFE#2364
- nasa/cFE#2356
- nasa/cFE#2361

*osal*
- nasa/osal#1393

Co-authored by: Daniel Knutsen <[email protected]>
Co-authored by: Justin Figueroa <[email protected]>
Co-authored by: Chris Knight <[email protected]>
This was referenced Jun 1, 2023
Merged
Merged
@dzbaker dzbaker merged commit 7c27c0e into nasa:main Jun 1, 2023
dzbaker added a commit to nasa/cFS that referenced this pull request Jun 1, 2023
@dzbaker
IC: Caelum-rc4+dev52
dd8dbfb 
*Combines:*

cFE v7.0.0-rc4+dev334
osal v6.0.0-rc4+dev217

**Includes:**

*cFS*
- #680
- #676

*cFE*
- nasa/cFE#2350
- nasa/cFE#2354
- nasa/cFE#2355
- nasa/cFE#2366
- nasa/cFE#2364
- nasa/cFE#2356
- nasa/cFE#2361

*osal*
- nasa/osal#1393

Co-authored by: Daniel Knutsen <[email protected]>
Co-authored by: Justin Figueroa <[email protected]>
Co-authored by: Chris Knight <[email protected]>
@dzbaker dzbaker mentioned this pull request Jun 26, 2023
Closed
dzbaker added a commit that referenced this pull request Jun 26, 2023
@dzbaker
Fix#2388, Revert "Merge pull request #2356 from CDKnightNASA/fix-2316…
a682f18 
…-time_fmt"

This reverts commit 7c27c0e, reversing
changes made to b5b90b4.
@dzbaker dzbaker mentioned this pull request Jun 26, 2023
Merged
2 tasks
dzbaker added a commit that referenced this pull request Jun 26, 2023
@dzbaker
Merge pull request #2389 from nasa/Fix2388
600a209 
Fix#2388, Revert "Merge pull request #2356 from CDKnightNASA/fix-2316-time_fmt"
@dzbaker
Copy link
Collaborator

dzbaker commented Jun 26, 2023

@CDKnightNASA Would you be able to re-open this PR so we can tag it for Equuleus-rc2 (based on last week's discussion, we decided to hold off the change until after this release)?

@CDKnightNASA
Copy link
Contributor Author

@CDKnightNASA Would you be able to re-open this PR so we can tag it for Equuleus-rc2 (based on last week's discussion, we decided to hold off the change until after this release)?

I think I'll have to create a new PR? I don't see a way to re-open this PR as it's marked as merged.

dzbaker added a commit to dzbaker/cFE that referenced this pull request Jul 11, 2023
@dzbaker
Fix nasa#2316, Re-introduce change from PR nasa#2356.
7fc5f88
@dzbaker dzbaker mentioned this pull request Jul 11, 2023
Open
2 tasks
@dzbaker dzbaker linked an issue Jul 11, 2023 that may be closed by this pull request
create CFE_TIME_PrintFmt(); #2316
Open
@dzbaker
Copy link
Collaborator

dzbaker commented Jul 11, 2023

@CDKnightNASA Would you be able to re-open this PR so we can tag it for Equuleus-rc2 (based on last week's discussion, we decided to hold off the change until after this release)?

I think I'll have to create a new PR? I don't see a way to re-open this PR as it's marked as merged.

I went ahead and created #2390, which reverts the revert, and assigned it to Equuleus-rc2.

@dmknutsen dmknutsen added this to the Equuleus milestone Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jphickey jphickey Awaiting requested review from jphickey

Assignees

@CDKnightNASA CDKnightNASA

Labels
CCB:Approved Indicates code review and approval by community CCB Equuleus-rc1
Projects
None yet
Milestone
Equuleus
Development

Successfully merging this pull request may close these issues.

create CFE_TIME_PrintFmt();
3 participants
@CDKnightNASA @dzbaker @dmknutsen