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 #777 deprecate sb #998

Merged
merged 3 commits into from
Nov 23, 2020

Conversation

skliper
Copy link
Contributor

@skliper skliper commented Nov 4, 2020

Describe the contribution
Fix #777 - see issue for deprecated elements

Testing performed
Built and ran, enabled telemetry, checked noops. Unit tests built and passed.

Expected behavior changes
None

System(s) tested on

  • Hardware: cFS Dev Server
  • OS: Ubuntu 18.04
  • Versions: Bundle main + this commit

Additional context
#777

Third party code
None

Contributor Info - All information REQUIRED for consideration of pull request
Jacob Hageman - NASA/GSFC

@skliper skliper added CCB:Ready Ready for discussion at the Configuration Control Board (CCB) Priority: Mission Feature or bug related to stakeholder needs labels Nov 4, 2020
@astrogeco astrogeco added CCB:Splinter Item needs a focused splinter meeting CCB-20201104 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Nov 4, 2020
@astrogeco astrogeco requested review from jphickey, a user and ejtimmon November 4, 2020 17:45
@astrogeco
Copy link
Contributor

CCB 2020-11-04

  • Replaces old refrences with new ones. Also updated the App dev guide language.
  • Tables also use the new APIs
  • Hope to review asynchronously or have a splinter

Updates the unit tests, unit test support, and
stubs to replace deprecated SB APIs with MSG APIs.
Update documentation to replace deprecated SB
APIs with MSG APIs
Update the core software from the deprecated SB
APIs to the MSG APIs.
@skliper skliper marked this pull request as ready for review November 4, 2020 21:50
@skliper skliper requested a review from zanzaben November 4, 2020 21:51
@skliper
Copy link
Contributor Author

skliper commented Nov 4, 2020

Ready for review - 8514efa unit test updates, 1da6b3c doc updates, 34e5510 core updates.

int32 Status;
CFE_SB_MsgPtr_t EVS_MsgPtr; /* Pointer to SB message */
int32 Status;
CFE_MSG_Message_t *MsgPtr; /* Pointer to SB message */
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this comment should be change to not mention SB?

Suggested change
CFE_MSG_Message_t *MsgPtr; /* Pointer to SB message */
CFE_MSG_Message_t *MsgPtr; /* Pointer to message */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Planning to address as part of #1009 cleanup.

@skliper
Copy link
Contributor Author

skliper commented Nov 13, 2020

@acudmore @jphickey @ejtimmon Please review so I can address #1009 and move forward with the standard pattern implementation.

Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

I've reviewed, and I'm honestly a little reluctant to go forth with this - we definitely have more to discuss here! I'm really not convinced we should be removing the CFE_SB_Msg_t type. There is an important distinction between a message in its raw form and a message as its being transported via SB - like the analogy of a letter vs. an envelope going through the post office to be delivered to its destination. MSG lib should be dealing with letters while SB deals with envelopes. They are related, but different.

My concern is that this change set is undoing a lot of that distinction that was already there. I suppose we can put it back in #1009, or maybe we should hold off on at least the final commit in this change set.

FWIW - I am in agreement with the removal of the "clear" flag on init (first commit) and the removal of most "manipulator" routines - again with the analogy of an envelope, once it is "sealed" you can look at its basic stuff such as the address/delivery info (msgid, size) but not the stuff inside (timestamps, cmd codes, payload). So it makes sense that the SB functions that get/set internal message fields should be deprecated in favor of the MSG versions. But when dealing with messages in their delivery form we should still have an SB level API for that that works with SB defined types.

| Command Code | CFE_MSG_SetFcnCode | Command Only |
| Checksum | CFE_MSG_GenerateChecksum | Command Only |
| Time | CFE_SB_TimeStampMsg | Telemetry Only |
| Time | CFE_MSG_SetMsgTime | Telemetry Only |
Copy link
Contributor

Choose a reason for hiding this comment

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

The "applicability" column is a bit confusing now - Since the MSG module allows users to customize their headers - a user can now invent their own headers that have a checksum on every packet type and/or timestamps on commands, etc.

So "applicability" is always implementation defined now ... whats listed here is applicability when using the included "msg" implementation.

CFE_SB_CLEAR_DATA);
CFE_MSG_Init((CFE_MSG_Message_t *) SAMPLE_AppData.BigPktPtr,
SAMPLE_BIG_TLM_MID,
SAMPLE_BIGPKT_MSGLEN);
Copy link
Contributor

Choose a reason for hiding this comment

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

Casting here is not desirable. We really shouldn't be documenting typecasts like this as the "recommended" way of doing things. I recognize that this was a preexisting issue in the docs though - and its just updated here.

Maybe this will also be affected by #1009 -- but we really should have a pattern that does not require casting when calling the basic APIs.

fsw/cfe-core/src/evs/cfe_evs_utils.c Show resolved Hide resolved
@jphickey
Copy link
Contributor

@acudmore @jphickey @ejtimmon Please review so I can address #1009 and move forward with the standard pattern implementation.

Sorry for the delay I was under the impression we might have an actual splinter meeting about this, but I'm fine to do it all via github as well.

@skliper
Copy link
Contributor Author

skliper commented Nov 16, 2020

@astrogeco can we move forward with this one? Maybe create a new issue for the documentation and fix it later.

jphickey added a commit that referenced this pull request Nov 23, 2020
Fix #777, Use MSG APIs - Core software
Fix #777, Use MSG APIs - Docs
Fix #777, Use MSG APIs - Unit tests
astrogeco pushed a commit to astrogeco/cFE that referenced this pull request Nov 23, 2020
Fix nasa#777, Use MSG APIs - Core software
Fix nasa#777, Use MSG APIs - Docs
Fix nasa#777, Use MSG APIs - Unit tests

See nasa#998 for more details
@astrogeco astrogeco merged commit 7b9db48 into nasa:integration-candidate Nov 23, 2020
@skliper skliper linked an issue Jan 7, 2021 that may be closed by this pull request
@skliper skliper deleted the fix777-deprecate-sb branch February 1, 2021 22:07
@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:FastTrack CCB:Splinter Item needs a focused splinter meeting conflicts Priority: Mission Feature or bug related to stakeholder needs
Projects
None yet
4 participants