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 #285, Refactor OSAL code selection #427

Closed

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Apr 20, 2020

Describe the contribution
Use separate source files and CMake-based source selection based on feature configuration, rather than using the C preprocessor for including/excluding different OSAL function groups.

Refactor all implementation units to provide a separate header file for each functional group/subsystem. Remove "static" declaration on internal helper functions so they can be invoked from unit test.

Fix #285 (primary)
Also Fix #214 and fix #195 (trivial fixes rolled in as part of refactoring) and fix #432 (separate commit)

Testing performed
Build and execute CFE for VxWorks, POSIX and RTEMS. Sanity check of CFE functions. Confirm all unit tests passing and coverage test of VxWorks is working (lcov shows 100% for covered modules).

Expected behavior changes
No impact to runtime code. Changes build system considerably, however.

  • No more user-maintained osconfig.h file - now replaced by a cmake configuration file
  • Break up low level implementation into small, separate subsystem units, with a separate header file for each one.

System(s) tested on
Ubuntu 18.04 LTS 64 bit (build host, native test)
i686-rtems4.11 cross build using QEMU
ppc-vxworks6.9 cross build using MCP750

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

@jphickey
Copy link
Contributor Author

Force-push to rebase onto latest "master" branch rather than integration-candidate, now showing as one commit...

@jphickey jphickey force-pushed the fix-285-code-selection branch 2 times, most recently from 88690d7 to 2f67435 Compare April 21, 2020 19:26
@jphickey jphickey marked this pull request as ready for review April 21, 2020 19:41
@jphickey
Copy link
Contributor Author

Force pushed to squash all fixes so far. Tested on MCP750 and looks good there too. This is ready for CCB review.

@jphickey jphickey added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Apr 21, 2020
@skliper
Copy link
Contributor

skliper commented Apr 22, 2020

20200422 CCB - Covered concepts and patterns, line-by-line and official approval will be off-line. Note will require rebases and careful merge to avoid conflicts.

@skliper skliper added CCB - 20200422 and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Apr 22, 2020
@jphickey
Copy link
Contributor Author

Question - Just added enhancement #434 for the separate issue, which made me think rather than adding a separate config option for every debug feature, maybe we should also rename OSAL_CONFIG_DEBUG_PERMISSIVE_MODE to be simply OSAL_CONFIG_DEBUG_MODE so that other debug-related items could be grouped with it rather than making a new option for each debug feature. I can foresee having other debug niceties be added in the future.

As this PR is already changing these configs, maybe it should preemptively rename this one and extend its scope to be applicable to these other debug features, or is it preferable to have more fine-grained options?

@skliper
Copy link
Contributor

skliper commented Apr 22, 2020

I prefer to continue to support custom configuration of the individual options, but have no problem with a general "wrapper" switch to enable them all.

@astrogeco
Copy link
Contributor

@skliper based on your email I added the fast track label, do you want to put this in the 20200415 IC? I'll probably merge that to master by Monday April 27

@jphickey
Copy link
Contributor Author

I don't think we can do that due to cross dependencies. There were already other items in the 2020-04-22 CCB which introduced changes that need to be reconciled/merged with this change set.

I recommend to follow the normal cycle, just do so as expediently as we can.

@astrogeco astrogeco added the CCB:Approved Indicates code review and approval by community CCB label Apr 23, 2020
@astrogeco
Copy link
Contributor

@jphickey merged everything else, might need to work with @skliper and @dmknutsen on #423 and #421

Use separate source files and CMake-based source selection
based on feature configuration, rather than using the C
preprocessor for including/excluding different OSAL function
groups.

Refactor all implementation units to provide a separate header
file for each functional group/subsystem.  Remove "static"
declaration on internal helper functions so they can be invoked
from unit test.
@jphickey jphickey changed the base branch from integration-candidate to master April 29, 2020 14:00
@jphickey
Copy link
Contributor Author

FYI - rebased to the 2020-04-15 baseline. Had to change the base branch back to master too (as it is!).

@astrogeco astrogeco changed the base branch from master to integration-candidate April 29, 2020 17:48
@astrogeco astrogeco changed the base branch from integration-candidate to master April 29, 2020 17:49
@astrogeco astrogeco changed the base branch from master to integration-candidate April 29, 2020 20:02
@astrogeco astrogeco changed the base branch from integration-candidate to master April 29, 2020 20:02
jphickey added a commit that referenced this pull request Apr 29, 2020
Fix #285, Refactor OSAL to avoid inclusion of C files
Manually merged to address merge conflicts
@astrogeco astrogeco changed the base branch from master to integration-candidate April 29, 2020 21:37
astrogeco pushed a commit that referenced this pull request Apr 29, 2020
@astrogeco
Copy link
Contributor

Merged in 7a7e6d1

@astrogeco astrogeco closed this Apr 29, 2020
jphickey added a commit that referenced this pull request Apr 30, 2020
Fix #285, Refactor OSAL to avoid inclusion of C files
Manually merged to address merge conflicts
jphickey added a commit that referenced this pull request Apr 30, 2020
Fix #285, Refactor OSAL to avoid inclusion of C files
Manually merged to address merge conflicts
jphickey added a commit that referenced this pull request Apr 30, 2020
Fix #285, Refactor OSAL to avoid inclusion of C files
Manually merged to address merge conflicts
jphickey added a commit to jphickey/osal that referenced this pull request Apr 30, 2020
Fix nasa#285, Refactor OSAL to avoid inclusion of C files
Manually merged to address merge conflicts
@skliper skliper added this to the 5.1.0 milestone Jun 1, 2020
jphickey pushed a commit to jphickey/osal that referenced this pull request Aug 10, 2022
jphickey pushed a commit to jphickey/osal that referenced this pull request Aug 10, 2022
issue nasa#427, adding travis.yml for cppcheck on flight
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
4 participants