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 #705, Use cFE defines to size arrays #899

Merged

Conversation

skliper
Copy link
Contributor

@skliper skliper commented Sep 22, 2020

Describe the contribution
Fixes #705, also a similar issue in TBL
Change from OS_MAX* defines to CFE defines for array sizing

Testing performed
Built and ran unit tests, passed. Visually confirmed sizes matched.

Expected behavior changes
No impact as long as the sizes are the same. Now scoped appropriately such that OS's can be configured differently and won't break cFE (as long as the cFE defines are max of the set)

System(s) tested on

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

Additional context
None

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) CCB:FastTrack bug labels Sep 22, 2020
@skliper skliper added this to the 7.0.0 milestone Sep 22, 2020
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.

Seems like there is more work to do here.... ?

  • As the structures (CFE_ES_AppInfo_t) are actually part of the IPC interface (msg), the definition of these shouldn't be in cfe_es.h but rather in cfe_es_msg.h where all other structures used within messages are defined.
  • Anywhere that the Name, EntryPoint, or FileName members are filled should use CFE_SB_MessageStringSet- which is similar to strncpy but takes both a source and dest length. It is intended for this use case where the local/platform and global/mission sizes might be different and this handles padding/termination issues.

@skliper
Copy link
Contributor Author

skliper commented Sep 22, 2020

Seems like there is more work...

I suggest new issue(s) since at least the second change sounds like a useful one to get documented separately, the old one just covered the array sizing.

@CDKnightNASA
Copy link
Contributor

FYI given this development, I'm wondering of the wisdom of having return status int32's intermixing CFE and OSAL return status...I realize that'll mean a lot of duplication where we remap OSAL status to CFE status with the same name, but in a lot of cases I'm guessing CFE can simplify the return status.

@astrogeco
Copy link
Contributor

CCB 2020-09-23 write up extra issues based on conversation. @jphickey to update function calls.

@skliper skliper marked this pull request as draft September 23, 2020 17:13
@skliper
Copy link
Contributor Author

skliper commented Sep 23, 2020

FYI given this development, I'm wondering of the wisdom of having return status int32's intermixing CFE and OSAL return status...I realize that'll mean a lot of duplication where we remap OSAL status to CFE status with the same name, but in a lot of cases I'm guessing CFE can simplify the return status.

Is this in reference to a different issue? This just changed array sizes...

@CDKnightNASA
Copy link
Contributor

FYI given this development, I'm wondering of the wisdom of having return status int32's intermixing CFE and OSAL return status...I realize that'll mean a lot of duplication where we remap OSAL status to CFE status with the same name, but in a lot of cases I'm guessing CFE can simplify the return status.

Is this in reference to a different issue? This just changed array sizes...

Just saying, philosophically we are "divorcing" array sizes from OSAL defs, while we continue to be "married" to the OSAL return codes. :) A general philosophical matter but one I think is a bit of conflict.

@astrogeco astrogeco added CCB-20200930 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) bug labels Sep 30, 2020
jphickey added a commit to jphickey/cFE that referenced this pull request Sep 30, 2020
Also check/ensure null termination of output
jphickey added a commit to jphickey/cFE that referenced this pull request Sep 30, 2020
Also check/ensure null termination of output
@jphickey
Copy link
Contributor

@skliper - My suggested update to fix the output sizes is here: skliper/cFE@fix705-rm-os-dependence...jphickey:fix-705-outarray-size

If you want, you can either merge this into your PR or I can submit a new PR that has both.

To summarize, I decided against using CFE_SB_MessageStringSet() because it (intentionally) fills the output buffer completely, foregoing a null terminator if it doesn't fit. But due to the usage patterns of these structs we should always ensure null termination. So continuing to use the C library strncpy() routine is no worse, we just have to make sure to use sizeof() rather than assume a specific macro/symbol.

@skliper skliper marked this pull request as ready for review September 30, 2020 16:17
@skliper skliper added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Sep 30, 2020
@astrogeco astrogeco added conflicts and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Sep 30, 2020
@astrogeco
Copy link
Contributor

CCB 2020-09-30:

  • There are some cases where we'll get unit test failures, changeing cFE defined OS_size

APPROVED

@astrogeco
Copy link
Contributor

@skliper can you clear the conflicts?

Also check/ensure null termination of output
@skliper skliper force-pushed the fix705-rm-os-dependence branch from ca3fca3 to 590025a Compare October 2, 2020 13:39
@skliper skliper removed the conflicts label Oct 2, 2020
@skliper
Copy link
Contributor Author

skliper commented Oct 2, 2020

@astrogeco conflicts resolved.

@astrogeco astrogeco changed the base branch from main to integration-candidate October 2, 2020 13:54
@astrogeco astrogeco dismissed jphickey’s stale review October 2, 2020 13:55

Reviewed at CCB

@astrogeco astrogeco merged commit 7e755c3 into nasa:integration-candidate Oct 2, 2020
@skliper skliper deleted the fix705-rm-os-dependence branch February 1, 2021 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CFE_ES_OneAppTlm_Payload_t telemetry struct contains non mission-scoped-sized array
4 participants