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 #2378, refactor SB to support additional use cases #2381

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

jphickey
Copy link
Contributor

Checklist (Please check before submitting)

Describe the contribution
Cleans up the internal SB implementation so it can better support future enhancements such as message integrity, additional header fields and timestamping.

Fixes #2378

Testing performed
Full suite of tests on SB implementation

Expected behavior changes
API behavior is preserved, fully backward compatible

System(s) tested on
Debian

Additional context
Replaces #2367 based on code review and discussion. This PR represents the refactoring change, along with some other refinements. The routing change will be a separate PR.

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

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL-coding-standard found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

* Not invoked outside of this unit
*
*-----------------------------------------------------------------*/
void CFE_SB_MessageTxn_GetEventDetails(const CFE_SB_MessageTxn_State_t *TxnPtr, const CFE_SB_PipeSetEntry_t *ContextPtr,

Check warning

Code scanning / CodeQL-security

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 127 lines.
@jphickey jphickey force-pushed the fix-2378-sb-refactor branch from c4141f9 to 0d27889 Compare December 7, 2023 15:51
Cleans up the internal SB implementation so it can better support future
enhancements such as message integrity, additional header fields and
timestamping.
@jphickey jphickey force-pushed the fix-2378-sb-refactor branch from 0d27889 to 550e7f7 Compare February 26, 2024 17:39
@jphickey
Copy link
Contributor Author

Rebased this PR to current main branch

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL-coding-standard found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@jphickey jphickey force-pushed the fix-2378-sb-refactor branch from 550e7f7 to f20e60a Compare April 18, 2024 23:25
@jphickey
Copy link
Contributor Author

The unused variable static analysis warning is fixed, but code coverage is now complaining.

The "missed branches" that it is reporting are mostly due to the fact that the switch statements in the code only have cases for the values that actually occur, and thus it is impossible to reach the switch with a value that doesn't have a case for it. While this would normally be a good thing, gcov/lcov flags it as an uncovered branch. I will look into this tomorrow to see if I can resolve it somehow.

@jphickey jphickey force-pushed the fix-2378-sb-refactor branch 2 times, most recently from 329695d to ea02a67 Compare April 19, 2024 21:43
@jphickey
Copy link
Contributor Author

Most "switch" cases now fixed (code coverage-wise) by adding a default case that does nothing, and always going through it. Looking at other modules there are still some code coverage lines that could potentially be fixed up before merging.

Restructure some switch statements and add a "default" case so they
are not flagged as having a missing branch in the coverage test.
@dzbaker
Copy link
Collaborator

dzbaker commented May 2, 2024

CCB 2 May 2024: Approved pending BVT run.

dzbaker added a commit to nasa/cFS that referenced this pull request May 6, 2024
*Combines:*

cFE equuleus-rc1+dev141
osal equuleus-rc1+dev66
PSP equuleus-rc1+dev42

**Includes:**

*cFS*
- #751
- #762

*cFE*
- nasa/cFE#2537
- nasa/cFE#2381
- nasa/cFE#2551

*osal*
- nasa/osal#1460

*PSP*
- nasa/PSP#430

Co-authored by: Justin Figueroa <[email protected]>
Co-authored by: Joseph Hickey <[email protected]>
@dzbaker dzbaker mentioned this pull request May 6, 2024
2 tasks
@dzbaker dzbaker merged commit 64b6837 into nasa:main Jul 2, 2024
22 checks passed
@dzbaker dzbaker added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Provisionally-Approved labels Jul 25, 2024
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 enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Software bus refactoring needed to prepare for future needs
3 participants