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.

Sign up for GitHub

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

Jump to bottom

Fix #1677, Add Message API Functional Test #1740

Closed
wants to merge 0 commits into from
Closed

Fix #1677, Add Message API Functional Test #1740

wants to merge 0 commits into from

Conversation

paulober
Copy link
Contributor

Describe the contribution

Contributor Info - All information REQUIRED for consideration of pull request
Paul Oberosler, Individual

@paulober paulober marked this pull request as draft July 30, 2021 16:52
skliper
skliper requested changes Jul 30, 2021
View reviewed changes
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.

Good work! See requested changes... those values can all be zero (they aren't pointers).

modules/msg/fsw/src/cfe_msg_ccsdsext.c Outdated
@@ -90,7 +90,7 @@ CFE_Status_t CFE_MSG_GetEDSVersion(const CFE_MSG_Message_t *MsgPtr, CFE_MSG_EDSV
*-----------------------------------------------------------------*/
CFE_Status_t CFE_MSG_SetEDSVersion(CFE_MSG_Message_t *MsgPtr, CFE_MSG_EDSVersion_t Version)
{
if (MsgPtr == NULL || (Version > (CFE_MSG_EDSVER_MASK >> CFE_MSG_EDSVER_SHIFT)))
if (MsgPtr == NULL || Version == NULL || (Version > (CFE_MSG_EDSVER_MASK >> CFE_MSG_EDSVER_SHIFT)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Version can be 0...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good work! See requested changes... those values can all be zero (they aren't pointers).

Thanks...

Sure, I'll change that back quickly.

modules/msg/fsw/src/cfe_msg_ccsdsext.c Outdated Show resolved Hide resolved
modules/msg/fsw/src/cfe_msg_ccsdsext.c Outdated Show resolved Hide resolved
@skliper
Copy link
Contributor

skliper commented Jul 30, 2021

Commit message typo - it should reference #1677.

@paulober
Copy link
Contributor Author

Commit message typo - it should reference #1677.

I also noticed, I will push it immediately

@paulober paulober force-pushed the fix-1677-functional-tests-for-message-apis branch 2 times, most recently from cb1ff23 to e8164f5 Compare July 30, 2021 19:33
@paulober paulober marked this pull request as ready for review July 30, 2021 19:35
@paulober paulober requested a review from skliper July 30, 2021 19:35
@paulober
Copy link
Contributor Author

paulober commented Jul 30, 2021 •
edited
Loading

I have not yet managed to run a MemoryProfiler with the unit test to find out what checksum an empty message with id 1 has. Could someone help me with this? (for the ut of GenerateChecksum)
These are the lines

@paulober paulober force-pushed the fix-1677-functional-tests-for-message-apis branch from 6eef28a to f50791a Compare August 1, 2021 09:55
@paulober paulober closed this Aug 1, 2021
@paulober paulober force-pushed the fix-1677-functional-tests-for-message-apis branch from f50791a to cc8c9a1 Compare August 1, 2021 10:05
@skliper
Copy link
Contributor

skliper commented Aug 2, 2021

Duplicate of #1745

@skliper skliper marked this as a duplicate of #1745 Aug 2, 2021
@skliper
Copy link
Contributor

skliper commented Aug 2, 2021

I have not yet managed to run a MemoryProfiler with the unit test to find out what checksum an empty message with id 1 has. Could someone help me with this? (for the ut of GenerateChecksum)
These are the lines

See comments on #1745 - really GenerateChecksum just inserts the correct value into the message, so verification is just to run ValidateChecksum before and after and make sure it passes after. There actually is no GetChecksum API anymore since we couldn't come up with a use-case from an application context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skliper skliper Awaiting requested review from skliper

Assignees
No one assigned
Labels
duplicate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add functional tests for cFE Message header APIs
2 participants
@paulober @skliper