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

Fix #1843, Add Time Clock Test #1860

Merged

Conversation

zanzaben
Copy link
Contributor

@zanzaben zanzaben commented Aug 20, 2021

Describe the contribution
Fixes #1843
Adds test to cover Clock APIs.

Testing performed
Build and run unit test

Expected behavior changes
No impact to behavior

System(s) tested on
Ubuntu 20.04

Contributor Info - All information REQUIRED for consideration of pull request
Alex Campbell GSFC

@zanzaben zanzaben force-pushed the fix1843_Time_Clock_Func_Test branch from a1ab28e to 80f083f Compare August 23, 2021 19:09
@zanzaben zanzaben force-pushed the fix1843_Time_Clock_Func_Test branch from 80f083f to 914d9b3 Compare August 24, 2021 15:25
@zanzaben zanzaben marked this pull request as ready for review August 24, 2021 15:26
@zanzaben zanzaben changed the title WIP Fix #1843, Add Time Clock Test Fix #1843, Add Time Clock Test Aug 24, 2021
@zanzaben zanzaben added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Aug 24, 2021
@astrogeco astrogeco removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Aug 25, 2021
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should improve the values being logged here, otherwise looks fine.


if (state >= 0)
{
UtAssert_BOOL_TRUE(ClockFlagCheck(ClockInfo, CFE_TIME_FLAG_CLKSET));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the "ClockFlagCheck" helper here. This seems to be hiding info, or at least will result in the log file having less detail than it could.

What about doing something like:

UtAssert_UINT32_EQ(ClockInfo, ClockInfo | CFE_TIME_FLAG_CLKSET);

The reason I like this is that it will show the raw value for ClockInfo value. In comparison, calling the ClockFlagCheck function will only display the result (true or false) and neither the raw value nor the bit being tested will be shown in the log.

(FWIW, I acknowledge that this does not show the bit being tested either, could also do UtAssert_UINT32_EQ(ClockInfo & CFE_TIME_FLAG_CLKSET, CFE_TIME_FLAG_CLKSET) but then this only shows the flag, not the raw value - depends on which is more interesting).

At the end of the day, could also introduce a new macro to test a bit field, that logs both the raw value and the bit being tested (not a bad idea, really).

@astrogeco
Copy link
Contributor

astrogeco commented Aug 25, 2021

CCB:2021-08-25 APPROVED with CHANGES

  • See jphickey's comment, display the raw value of ClockInfo

@astrogeco astrogeco added the CCB:Approved Indicates code review and approval by community CCB label Aug 25, 2021
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I still think a macro that works with bitmasks is a good addition, so I wrote nasa/osal#1135

@astrogeco astrogeco changed the base branch from main to integration-candidate August 25, 2021 21:57
@astrogeco astrogeco merged commit 9197270 into nasa:integration-candidate Aug 25, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request Aug 25, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request Sep 1, 2021
**Combines**

nasa/cFE#1885,              v6.8.0-rc1+dev980
nasa/osal#1138,             v5.1.0-rc1+dev598
nasa/cFS-GroundSystem#195,  v2.2.0-rc1+dev63

**Includes**

*cFE*

nasa/cFE#1870, Add SB API test cases
nasa/cFE#1869, Add ES API test cases
nasa/cFE#1872, Add TBL API test cases
nasa/cFE#1871, Add FS API test cases
nasa/cFE#1860, Add Time Clock Test
nasa/cFE#1862, EVS coverage test
nasa/cFE#1876, SB test improvements
nasa/cFE#1865, CFE_TBL_Modified: Test CRC, updated flag
nasa/cFE#1881, Improve EVS code coverage
nasa/cFE#1877, add call to CFE_ES_ExitChildTask
nasa/cFE#1902, Incorrect OSAL Format in Users Guide Reference
nasa/cFE#1884, Improve FS coverage
nasa/cFE, Improve MSG branch coverage
nasa/cFE#1891, Improve resource ID branch coverage
nasa/cFE#1894, Improve SBR branch coverage
nasa/cFE#1896, Fix #1895, Improve TIME branch coverage
nasa/cFE#1904, Improve TBL code coverage
nasa/cFE#1864, Support custom PSP directory
nasa/cFE#1913, Update time tests to use bitmask check macros
nasa/cFE#1923, remove extra word in comment

*osal*

nasa/osal#1136, add bitmask assert macros

*cFS-GroundSystem*

nasa/cFS-GroundSystem#190, Fix #189, Virtualenv and Pipenv .gitignore support
nasa/cFS-GroundSystem#194, Fix doc, comment, and message typos

Co-authored-by: Jacob Hageman           <skliper@users.noreply.github.com>
Co-authored-by: Joseph Hickey           <jphickey@users.noreply.github.com>
Co-authored-by: Alex Campbell           <zanzaben@users.noreply.github.com>
Co-authored-by: Ariel Adams             <ArielSAdamsNASA@users.noreply.github.com>
Co-authored-by: Jose F Martinez Pedraza <pepepr08@users.noreply.github.com>
Co-authored-by: Avi                     <thnkslprpt@users.noreply.github.com>
Co-authored-by: Paul                    <pavll@users.noreply.github.com>
@skliper skliper added this to the 7.0.0 milestone Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TIME functional - add test of CFE_TIME_GetClockState and CFE_TIME_GetClockInfo
4 participants