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

cfe_es_start: fix CFE_ES_MainTaskSyncDelay() parameter #2562

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Nodraak
Copy link
Contributor

@Nodraak Nodraak commented May 14, 2024

CFE_ES_MainTaskSyncDelay() expects a TimeOutMilliseconds parameter.

Checklist (Please check before submitting)

Describe the contribution
A clear and concise description of what the contribution is.

  • Include explicitly what issue it addresses [e.g. Fixes #X]

Testing performed
Steps taken to test the contribution:

  1. Build steps '...'
  2. Execution steps '...'

Expected behavior changes
A clear and concise description of how this contribution will change behavior and level of impact.

  • API Change: xxx (if applicable)
  • Behavior Change: xxx (if applicable)
  • Or no impact to behavior

System(s) tested on

  • Hardware: [e.g. PC, SP0, MCP750]
  • OS: [e.g. Ubuntu 18.04, RTEMS 4.11, VxWorks 6.9]
  • Versions: [e.g. cFE 6.6, OSAL 4.2, PSP 1.3 for mcp750, any related apps or tools]

Additional context
Add any other context about the contribution here.

Third party code
If included, identify any third party code and provide text file of license

Contributor Info - All information REQUIRED for consideration of pull request
Full name and company/organization/center of all contributors ("Personal" if individual work)

  • Note CLAs apply to only software contributions.

CFE_ES_MainTaskSyncDelay() expects a `TimeOutMilliseconds` parameter.
@Nodraak
Copy link
Contributor Author

Nodraak commented May 14, 2024

Hi,

This change is pretty self explanatory, so I will not explain it too much, but here is how I found it:

I was debugging some issue in my FSW unrelated to cFS (which turned out to be a pointer issue when calling memset()) and suspected a stack overflow. After increasing the cFS app stack from 8 KB to 15 KB, I tried to add -fsanitize=address -fno-omit-frame-pointer to my compilation options and noticed the SW being stuck. GDB quickly pointed me to this line and I noticed the very high timeout (30 000 000 msec = 8 hours).
I assume that the cause is that the Address sanitizer breaks the initialization of a cFS app (?), which makes this call block until the timeout expires.
At this point, I had noticed and fixed my memset() mistake, so I did not investigated further.

All of the above was compiled and run on Ubuntu 22.04.4 LTS using gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0. The latest upstream commit before we forked cFS is the following one:

commit 192f2ea3061bfe722b8ed1d72b7adecbf36d4f6e
Merge: c23e8dd d3cfb84
Author: dzbaker <[email protected]>
Date:   Thu Mar 30 17:05:11 2023 -0400

    Merge pull request #664 from nasa/integration-candidate
    
    IC: Caelum-rc4+dev44

Note: I quickly grep'ed for all CFE_ES_MainTaskSyncDelay() and did not find the same issue elsewhere.

@dzbaker
Copy link
Collaborator

dzbaker commented May 16, 2024

CCB 16 May 2024: Approved pending creation of issue.

@dzbaker
Copy link
Collaborator

dzbaker commented May 16, 2024

Hi @Nodraak. Thanks for the contribution! Would you be able to fill out a CLA (Contributor License Agreement) and email it to [email protected] and copy [email protected]?

@dzbaker dzbaker added the CCB:PendingCLA External contribution pending CLA confirmation label May 21, 2024
@dzbaker
Copy link
Collaborator

dzbaker commented Jul 3, 2024

Hi @Nodraak. Thank you again for your contribution! I haven't seen a CLA yet; would you be able to fill one out? Let me know if you have any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:PendingCLA External contribution pending CLA confirmation CCB:Provisionally-Approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants