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 #1021, resolve discrepancies between task API and unit tests #1037

Merged

Conversation

jphickey
Copy link
Contributor

Describe the contribution
Ensures correlation between the unit-tests and documented return values for the OSAL task API.

Fixes #1021

Testing performed
Build and run unit tests, re-run cross reference script to confirm

Expected behavior changes
All documented return values are tested or noted that they cannot be tested in functional test

System(s) tested on
Ubuntu

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey force-pushed the fix-1021-task-api-retcodes branch 2 times, most recently from 965a1fd to 11fe719 Compare May 21, 2021 15:05
@jphickey jphickey added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label May 25, 2021
Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

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

OS_TaskCreate - should test NULL stack address case (allocate from heap)
OS_TaskInstallDeleteHandler - check that delete handler actually gets called
OS_TaskDelay - add a check the delay actually delays? we depend on this working all over the place but I never see it actually checked
OS_TaskPriority - add a check that setting priority is actually respected
OS_TaskGetId - automate this check, pass in the pointer to the ID?
OS_TaskFindIdBySystemData - needs test

@skliper
Copy link
Contributor

skliper commented May 26, 2021

I suggest we hold off on this one until @jphickey and I have a chance to discuss

@jphickey
Copy link
Contributor Author

Here's my take:

  • OS_TaskSetPriority: No real easy way to check this in the current API, we would have to confine multiple tasks to the same CPU. As long as tasks are allowed to float among SMP CPUs, a lower priority and a higher priority task can run at the same time. Once we can assign affinity then the priority should be more verifiable. (except when permissive mode is used....)
  • OS_TaskFindIdBySystemData: As noted in the comment, there is no way to get the "system data" that this depends on. This is only used by the PSP, where that info was acquired externally/outside of the OSAL API, to do the reverse lookup. Hence why the test only checks the errors right now.
  • OS_TaskGetId: This is checking the returned value matches the expected ID. I am not sure what is being requested on this one.
  • OS_TaskDelay: I suppose we can add a call to OS_gettime before and after, then look at the difference

@jphickey
Copy link
Contributor Author

jphickey commented May 26, 2021

Commit 2d7d575 adds a check for OS_TaskCreate with NULL stack (dynamically allocate), checks that the task delete handler was invoked, and confirms that OS_TaskDelay actually delayed by comparing with OS_GetLocalTime.

@jphickey jphickey force-pushed the fix-1021-task-api-retcodes branch from 1870d32 to 2d7d575 Compare May 26, 2021 15:16
@skliper
Copy link
Contributor

skliper commented May 26, 2021

@astrogeco @ejtimmon Since priority/scheduling testing is pretty much impossible to verify in a generic sense (without system/configuration knowledge), how about extending the OSAL example work to include a sample test that could confirm the MCP750 specific system setup (single core, whatever task scheduling is used)? So this wouldn't be a built-in open-source OSAL unit test, but could be used to show things work as expected on the MCP750. Would be great if this could eventually be extended to include a multi-cpu test, linux and RTEMS... but will still be system/configuration dependent. I'll mark/report testing of priorities and scheduling as a project/distribution test responsibility and not confirmed generically using OSAL unit tests.

@skliper
Copy link
Contributor

skliper commented May 26, 2021

Concur w/ all responses, fixes. Good to go. Note OS_TaskGetId comment was stale, so no longer applicable.

@jphickey
Copy link
Contributor Author

If we resurrect the tasking-example as has been discussed, that might be a good place to demonstrate priorities.

I also thought we could check semaphore priorities. That is, if we create a low priority task first, let it block, then create a high priority task, and have it block on the same sem. If we then give the sem, only the high priority task should wake up. If we give it a second time then the low priority task should wake up. This would confirm priority-based semaphores which IIRC we are using, if the sem was configured as a FIFO then it would fail this test.

That test case could theoretically be done in the current tests but we would likely have to bypass it when OSAL_CONFIG_DEBUG_PERMISSIVE_MODE is enabled.

Also, the OS_task_prop_t structure has a "priority" member in it, which we can check. However, this still gets set even if OSAL_CONFIG_DEBUG_PERMISSIVE_MODE is enabled. Might want to modify this so it reflects the real priority as programmed into the OS Scheduler, then we can use this field to gauge the behavior to expect from other calls.

@astrogeco astrogeco added the CCB:Approved Indicates code review and approval by community CCB label May 26, 2021
Ensures correlation between the unit-tests and documented return
values for the OSAL task API.

Also adds the following test cases:
- Call OS_TaskCreate with a NULL stack pointer
- Confirm OS_TaskDeleteHandler function was invoked
- Confirm OS_TaskDelay interval
@jphickey jphickey force-pushed the fix-1021-task-api-retcodes branch from 2d7d575 to a987a20 Compare May 26, 2021 17:52
@jphickey
Copy link
Contributor Author

Re-Pushed to correct commit message typo and squash commits where necessary

@skliper
Copy link
Contributor

skliper commented May 26, 2021

I also thought we could check semaphore priorities....

Good suggestion! Could you submit a new issue?

That test case could theoretically be done in the current tests but we would likely have to bypass it when OSAL_CONFIG_DEBUG_PERMISSIVE_MODE is enabled.

Isn't there a set of user level priorities that do still work in permissive? Can we stay in that range?

Also, the OS_task_prop_t structure has a "priority" member in it, which we can check. However, this still gets set even if OSAL_CONFIG_DEBUG_PERMISSIVE_MODE is enabled. Might want to modify this so it reflects the real priority as programmed into the OS Scheduler, then we can use this field to gauge the behavior to expect from other calls.

Good suggestion/update! Could you submit a new issue?

@jphickey jphickey changed the title Fix #1021, resolve descrepancies between task API and unit tests Fix #1021, resolve discrepancies between task API and unit tests May 27, 2021
@astrogeco astrogeco removed the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label May 27, 2021
@jphickey
Copy link
Contributor Author

Submitted issue #1059 for above

@astrogeco astrogeco changed the base branch from main to integration-candidate June 2, 2021 01:47
@astrogeco astrogeco merged commit f3c3692 into nasa:integration-candidate Jun 2, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request Jun 2, 2021
Update nasa/osal#1058

Includes:

nasa/osal#1044, resolve discrepancies between mutex API and unit tests
nasa/osal#1038, resolve discrepancies between queue API and unit tests
nasa/osal#1037, resolve discrepancies between task API and unit tests
nasa/osal#1051, resolve discrepancies between timebase API and unit tests

nasa/osal#1030, check misc API return codes
nasa/osal#1039, Rename CodeQL cFE Build and add Duplicate Job
nasa/osal#1049, filter only whole words for keyword match
astrogeco added a commit to nasa/cFS that referenced this pull request Jun 2, 2021
Update nasa/osal#1058

Includes:

nasa/osal#1044, resolve discrepancies between mutex API and unit tests
nasa/osal#1038, resolve discrepancies between queue API and unit tests
nasa/osal#1037, resolve discrepancies between task API and unit tests
nasa/osal#1051, resolve discrepancies between timebase API and unit tests

nasa/osal#1030, check misc API return codes
nasa/osal#1039, Rename CodeQL cFE Build and add Duplicate Job
nasa/osal#1049, filter only whole words for keyword match
@jphickey jphickey deleted the fix-1021-task-api-retcodes branch June 8, 2021 18:22
astrogeco added a commit to nasa/cFS that referenced this pull request Jun 8, 2021
- #263, bundle

Combines:

- nasa/cFE#1584, v6.8.0-rc1+dev642
- nasa/osal#1058, v5.1.0-rc1+dev500
- nasa/sch_lab#82, v2.4.0-rc1+dev36

Includes:

- nasa/cFE#1580, Update ES verify errors to match test
- nasa/cFE#1578, Resolve mismatched endforeach of CMakeList.txt
- nasa/cFE#1579, Improve event filter documentation
- nasa/cFE#1576, EVS/FS documentation cleanup
- nasa/cFE#1575, Remove shell file subtype and clarify scope
- nasa/cFE#1582, Simplify file open/truncate logic
- nasa/cFE#1567, Remove deprecated elements
- nasa/cFE#1556, Replace Header Content Type magic number
- nasa/cFE#1553, Remove unused EVS LogMode defines
- nasa/cFE#1570, Document cleanup from SB/MSG/SBR review, Remove unused CFE_SB_NO_SUBSCRIBERS
- nasa/cFE#1565, Add CFE_SB_DestinationD tag and use for pointers
- nasa/cFE#1564, Use CFE_MSG_SequenceCount_t for seqcnt type
- nasa/cFE#1562, Document CFE_ES_PoolCreateEx NumBlockSizes error handling
- nasa/cFE#1560, Documentation cleanup in TBL/TIME
- nasa/cFE#1558, Document TblName as app specific
- nasa/cFE#1563, use OSAL script to generate API guide
- nasa/cFE#1557, Include verify headers to validate config
- nasa/cFE#1555, add doxygen aliases for OSAL parameter/retvals

- nasa/osal#1033, include doxygen targets locally
- nasa/osal#1031, Document OS_ObjectIdToArrayIndex as public
- nasa/osal#1040, resolve discrepancies between binsem API and unit tests
- nasa/osal#1029, add missing clock retcode tests
- nasa/osal#1046, resolve discrepancies between common API and unit tests
- nasa/osal#1041, resolve discrepancies between countsem API and unit tests
- nasa/osal#1045, resolve discrepancies between dir API and unit tests
- nasa/osal#1043, resolve discrepancies between module API and unit tests
- nasa/osal#1044, resolve discrepancies between mutex API and unit tests
- nasa/osal#1038, resolve discrepancies between queue API and unit tests
- nasa/osal#1037, resolve discrepancies between task API and unit tests
- nasa/osal#1051, resolve discrepancies between timebase API and unit tests
- nasa/osal#1030, check misc API return codes
- nasa/osal#1039, Rename CodeQL cFE Build and add Duplicate Job
- nasa/osal#1049, filter only whole words for keyword match
- nasa/osal#962, update OSAL Config Guide link

- nasa/sch_lab#81, exit the main loop if init fails

- HOTFIX cFS-bundle, Use new osalguide logfile to fix documentation build
workflow errors,
astrogeco added a commit to nasa/cFS that referenced this pull request Jun 8, 2021
- #263, bundle

Combines:

- nasa/cFE#1584, v6.8.0-rc1+dev642
- nasa/osal#1058, v5.1.0-rc1+dev500
- nasa/sch_lab#82, v2.4.0-rc1+dev36

Includes:

- nasa/cFE#1580, Update ES verify errors to match test
- nasa/cFE#1578, Resolve mismatched endforeach of CMakeList.txt
- nasa/cFE#1579, Improve event filter documentation
- nasa/cFE#1576, EVS/FS documentation cleanup
- nasa/cFE#1575, Remove shell file subtype and clarify scope
- nasa/cFE#1582, Simplify file open/truncate logic
- nasa/cFE#1567, Remove deprecated elements
- nasa/cFE#1556, Replace Header Content Type magic number
- nasa/cFE#1553, Remove unused EVS LogMode defines
- nasa/cFE#1570, Document cleanup from SB/MSG/SBR review, Remove unused CFE_SB_NO_SUBSCRIBERS
- nasa/cFE#1565, Add CFE_SB_DestinationD tag and use for pointers
- nasa/cFE#1564, Use CFE_MSG_SequenceCount_t for seqcnt type
- nasa/cFE#1562, Document CFE_ES_PoolCreateEx NumBlockSizes error handling
- nasa/cFE#1560, Documentation cleanup in TBL/TIME
- nasa/cFE#1558, Document TblName as app specific
- nasa/cFE#1563, use OSAL script to generate API guide
- nasa/cFE#1557, Include verify headers to validate config
- nasa/cFE#1555, add doxygen aliases for OSAL parameter/retvals

- nasa/osal#1033, include doxygen targets locally
- nasa/osal#1031, Document OS_ObjectIdToArrayIndex as public
- nasa/osal#1040, resolve discrepancies between binsem API and unit tests
- nasa/osal#1029, add missing clock retcode tests
- nasa/osal#1046, resolve discrepancies between common API and unit tests
- nasa/osal#1041, resolve discrepancies between countsem API and unit tests
- nasa/osal#1045, resolve discrepancies between dir API and unit tests
- nasa/osal#1043, resolve discrepancies between module API and unit tests
- nasa/osal#1044, resolve discrepancies between mutex API and unit tests
- nasa/osal#1038, resolve discrepancies between queue API and unit tests
- nasa/osal#1037, resolve discrepancies between task API and unit tests
- nasa/osal#1051, resolve discrepancies between timebase API and unit tests
- nasa/osal#1030, check misc API return codes
- nasa/osal#1039, Rename CodeQL cFE Build and add Duplicate Job
- nasa/osal#1049, filter only whole words for keyword match
- nasa/osal#962, update OSAL Config Guide link

- nasa/sch_lab#81, exit the main loop if init fails

- HOTFIX cFS-bundle, Use new osalguide logfile to fix documentation build
workflow errors,

Co-authored-by: Joseph Hickey <jphickey@users.noreply.github.com>
Co-authored-by: Jake Hageman skliper <skliper@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: Ross Peters <rosspeters6@users.noreply.github.com>
@skliper skliper added this to the 6.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.

Unit test status code discrepancies in osapi-task.h
3 participants