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 #380, Add OS_TimeBase Api Functional Tests #481

Conversation

yammajamma
Copy link
Contributor

Fix #380, Add OS_TimeBase Api Functional Tests

Describe the contribution
Added OS_TimeBase Api Functional Tests for OS_TimeBaseCreate, OS_TimeBaseSet, OS_TimeBaseDelete, OS_TimeBaseGetIdByName, OS_TimeBaseGetInfo, OS_TimeBaseGetFreeRun

Testing performed
Steps taken to test the contribution:

  1. Standard build and ran unit tests.

System(s) tested on
cFS Dev Server
OS: Ubuntu 18.04
Versions: OSAL 5.0.11.0

Contributor Info - All information REQUIRED for consideration of pull request
Yasir Khan
NASA GSFC

@astrogeco astrogeco added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label May 27, 2020
@astrogeco
Copy link
Contributor

CCB 2020-05-27 APPROVED

@astrogeco astrogeco added CCB-20200527 and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels May 27, 2020
++TimerSyncCount;
return TimerSyncRetVal;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Note this sync function is going to immediately return 0, which causes a "spin loop" in the timebase. There is some protection against this but its really not something you should do, its just to protect against locking the CPU.

objid = 0x00000000;
actual = OS_TimeBaseCreate(&objid, "maxTimeBase", 0);
UtAssert_True(actual == expected, "OS_TimeBaseCreate() (%ld) == OS_SUCCESS", (long)actual);

Copy link
Contributor

Choose a reason for hiding this comment

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

The OS_MAX_TIMEBASES is a configurable parameter. Could we turn this into a loop based on OS_MAX_TIMEBASES so that this test case still works if the user changes this value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I read in the OS_MAX_TIMEBASES, so shouldn't it automatically always read in the max value if the user reconfigured it it before this point?

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea is to confirm that one can create entities all the way up to OS_MAX_TIMEBASES (i.e. all OS_SUCCESS), then attempt to create one more which should return an error. The point at which this happens is user configurable so this would have to be converted into a loop. This is currently hardcoding 5 timebase objects, so it will fail with OS_MAX_TIMEBASES set to anything other than 5.

@CDKnightNASA
Copy link
Contributor

I realize this has been developed and approved but I am wondering if this could leverage the macros in https://github.com/nasa/osal/blob/master/src/unit-tests/inc/ut_os_support.h (namely: UT_NOMINAL(), UT_RETVAL(), ...)

@astrogeco
Copy link
Contributor

astrogeco commented May 27, 2020

I realize this has been developed and approved but I am wondering if this could leverage the macros in https://github.com/nasa/osal/blob/master/src/unit-tests/inc/ut_os_support.h (namely: UT_NOMINAL(), UT_RETVAL(), ...)

Let's merge this and open a new PR specifically for using the macros

Edit: New PR #484

@astrogeco
Copy link
Contributor

@yammajamma I switched this to draft. Can you address @jphickey comments and remove the draft status once done?

@yammajamma
Copy link
Contributor Author

@astrogeco Yep, working on that right now.

@astrogeco astrogeco changed the base branch from master to integration-candidate June 2, 2020 19:03
@yammajamma yammajamma marked this pull request as ready for review June 2, 2020 23:13
@astrogeco astrogeco added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Approved Indicates code review and approval by community CCB labels Jun 3, 2020
@astrogeco astrogeco requested a review from jphickey June 3, 2020 13:55
@astrogeco
Copy link
Contributor

@jphickey do these look good?

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

@astrogeco astrogeco merged commit ff44e9f into nasa:integration-candidate Jun 8, 2020
@skliper skliper added this to the 5.1.0 milestone Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TimeBase API missing functional tests
5 participants