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 #1755, separate variable for OSAL status #1756

Merged

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Aug 3, 2021

Describe the contribution
Ensure a separate stack variable, always named "OsStatus", is used to hold the result of an OSAL API call. Do not use the same variable that is used to store a CFE status code.

This also fixes a few return code translations, where some functions were returning the OSAL status code (perhaps unintentionally) because it was stored in the same variable.

Fixes #1755

Testing performed
Build and sanity check CFE, run all tests

Expected behavior changes
Nothing significant, however some APIs which returned an OSAL status will now translate to CFE status.

System(s) tested on
Ubuntu

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

@zanzaben
Copy link
Contributor

zanzaben commented Aug 3, 2021

several functional tests will also need to be updated for this.

@astrogeco astrogeco added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Aug 11, 2021
@astrogeco
Copy link
Contributor

@jphickey check build fails

Ensure a separate stack variable, always named "OsStatus", is used
to hold the result of an OSAL API call.  Do not use the same variable
that is used to store a CFE status code.
Ensure a separate stack variable, always named "PspStatus", is used
to hold the result of a PSP API call.  Do not use the same variable
that is used to store a CFE status code.
@jphickey
Copy link
Contributor Author

The build failure here was a bogus warning about a "possible uninitialized value" - in fact there is no way to get to that case statement without OsStatus getting initialized to something. It appears gcc 9 optimizer (CI build) is not smart enough to see that detail, but gcc 10 optimizer (on my local machine) is - this is why I didn't see the warning myself.

At any rate, I added an explicit init of OsStatus and also rebased to main, should be good to go now.

@astrogeco astrogeco removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Aug 18, 2021
@astrogeco
Copy link
Contributor

astrogeco commented Aug 18, 2021

CCB:2021-08-18

  • If it works let's merge it soon
  • Follow on work for Draco would be to redefine these new Status types as actual types instead of int32

@astrogeco astrogeco added the CCB:Approved Indicates code review and approval by community CCB label Aug 18, 2021
@astrogeco astrogeco changed the base branch from main to integration-candidate August 18, 2021 16:29
@astrogeco astrogeco merged commit 47eb39d into nasa:integration-candidate Aug 18, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request Aug 18, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request Aug 20, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request Aug 21, 2021
**Combines**

nasa/cFE# v6.8.0-rc1+dev933
nasa/osal# v5.1.0-rc1+dev594

**Includes**

*osal*

nasa/osal#1131, add test case types similar to NA

*cFE*

nasa/cFE#1803, Add software bus tests
nasa/cFE#1756, separate variable for OSAL status
nasa/cFE#1809, increase SB pool max size bucket
nasa/cFE#1842, Add Null check for CFE_ResourceId_FindNext
nasa/cFE#1828, Improve TBL coverage tests
nasa/cFE#1833, Clean up Message ID Functional Test #1824, Add missing cases for msg id func tests
nasa/cFE#1832, Combine SB Set/Get message characteristics group #1831, Consolidate msg get/set doxygen group
nasa/cFE#1836, Adding coverage tests to cFE TIME
nasa/cFE#1848, enable strict resource id w/OMIT_DEPRECATED
nasa/cFE#1845, HOTFIX IC-20210819, type correction TBL coverage test
nasa/cFE#1806, Add test for ES BackgroundWakeup
nasa/cFE#1813, Success Test for CFE_ES_RestartApp
nasa/cFE#1814, Subscribe to Message Limit Greater Than CFE_PLATFORM_SB_DEFAULT_MSG_LIMIT
nasa/cFE#1811, Success Test for CFE_ES_GetMemPoolStats
nasa/cFE#1822, Group MSG APIs documentation by header type
nasa/cFE#1816, add retroactive CFE status asserts
nasa/cFE#1854, remove unused CFE_TBL_ERR_BAD_APP_ID
nasa/cFE#1855, correct syslog message in pool create
nasa/cFE#1853, remove unused CFE_ES_POOL_BOUNDS_ERROR
nasa/cFE#1859, remove unused CFE_TBL_ERR_FILE_NOT_FOUND
nasa/cFE#1856, Check error ctr to TransmitMsg test
nasa/cFE#1857, End Child Task requirement remove error code
nasa/cFE#1782, Add functional tests for resource misc

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: Niall Mullane           <nmullane@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>
@jphickey jphickey deleted the fix-1755-osstatus-variable branch August 23, 2021 17:55
@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.

Improve consistency when working with OSAL status codes
4 participants