Fix #610, Add vxworks7 support

[ghost]

Describe the contribution
This is a more Joe-approved vxworks7 addition without the copy of the code.

Fix #610

Testing performed
None yet. I'm going to bed. But I wanted to look at it with the CCB. I will try and do testing before the CCB.

Contributor Info - All information REQUIRED for consideration of pull request
Steven Seeger, Embedded Flight Systems, Inc.

[jphickey]

Getting there ... we have to fix the CI build failure which is probably caused by forking files.

I still think this can be done without forking anything except the dir stuff.

[jphickey]

BTW - to comment on the process a bit - we first start with an issue before submitting a PR. Recommend to basically take the source code comment here:

https://github.com/klystron78/osal/blob/378578829317e3c23e518529b55254918357669e/src/os/vxworks/src/vxworks7/os-impl-common.c#L61-L66

And submitting an issue about this first (i.e. "on vxworks7 such and such does not work") then submitting the PR that addresses it.

[jphickey]

FYI - The allocator macro I was referring to in CCB can be seen here for counting sems:

osal/src/os/vxworks/inc/os-impl-countsem.h

Line 37 in 4f7e201

VX_COUNTING_SEMAPHORE(cmem);

Similar macro exists for binary sem, mutex, etc. It would seem possible (maybe even probable?) that wind river would offer a similar macro to statically allocate a task TCB?

[ghost]

FYI - The allocator macro I was referring to in CCB can be seen here for counting sems:

osal/src/os/vxworks/inc/os-impl-countsem.h

Line 37 in 4f7e201

VX_COUNTING_SEMAPHORE(cmem);

Similar macro exists for binary sem, mutex, etc. It would seem possible (maybe even probable?) that wind river would offer a similar macro to statically allocate a task TCB?

I don't see one. From taskLib.h:

17 29nov18,gws made windTcb definition private, added windTcb opaque definition,
18 made TCB inline functions private,
19 added VX_WIND_TCB_SIZE for VX_TASK, VX_TASK_INITIALIZE (F11200)

VX_TASK declares space for a task and stack, and it uses VX_WIND_TCB_SIZE to size a character array for the tcb.

[jphickey]

Can you check if vxworks 6.9 provides a VX_WIND_TCB_SIZE macro? It would seem it does not, from the sounds of it.

Which is actually (possibly) a good thing -- because you can probably make a local macro:

#ifdef VX_WIND_TCB_SIZE /* vxworks7+ provides this abstract size */
typedef char[VX_WIND_TCB_SIZE]  OS_VxWorks_TCB_t;
#else  /* no abstract size, use WIND_TCB directly */
typedef WIND_TCB  OS_VxWorks_TCB_t;
#endif

Then just declare the struct based off OS_VxWorks_TCB_t ....

[skliper]

Any update on this? We need to wrap up so we can progress with certification activities.

[jphickey]

Still got some stuff to resolve here. Unit test is broken (see Travis-CI) and I think we can further simplify.

[ghost]

I've done a rebase and pushed this for consideration. I've updated with Joe's latest comments.

I don't know anything about travis-ci or what the problem is. This would be a good time to learn. Since I did a rebase and force push it might make sense to close this pull request and open a new one?

[jphickey]

I've done a rebase and pushed this for consideration. I've updated with Joe's latest comments

This looks like a straight rebase to me -- I don't see any differences in the way the struct is defined from this commit to the previous one. Did you push the whole set of updates you've made? May want to double check.

I don't know anything about travis-ci or what the problem is.

For the travis error click the "details" link then click the build that failed - in this case all 4 have failed so just pick one. Then scroll down the logs to see this build issue:

In file included from /home/travis/build/nasa/osal/src/unit-test-coverage/vxworks/adaptors/src/ut-adaptor-tasks.c:33:0:

/home/travis/build/nasa/osal/src/os/vxworks/inc/os-impl-tasks.h:45:2: error: #error build system must define OSAL_VXWORKS6 or OSAL_VXWORKS7

 #error build system must define OSAL_VXWORKS6 or OSAL_VXWORKS7

  ^~~~~

If you implement my previous comment it should fix this issue.... so make sure you've pushed everything.

Since I did a rebase and force push it might make sense to close this pull request and open a new one?

No need - we update branches all the time prior to merge. Only need a new PR if it was reverted/rejected post merge for some reason.

[jphickey]

@klystron78 - Correction - actually the commits in this PR are still showing to be from Sep 20, which may explain why they look the same to me. Are you sure you pushed?

[ghost]

@klystron78 - Correction - actually the commits in this PR are still showing to be from Sep 20, which may explain why they look the same to me. Are you sure you pushed?

Yes I pushed. The error in your previous comment was probably due to the fact that I first had a bad push due to mixing up commit order in an interactive rebase locally. I backed that out and pushed.

I am about to make sure all this runs on the target hardware.

[jphickey]

Yes I pushed.

Github disagrees --- I'm still seeing the original commits and still on the old baseline from Sept. I even re-fetched this PR to my local repo and see the same. I also don't see the normal log messages that appear in the PR when you force push that says you updated it.

[ghost]

Yes I pushed.

Github disagrees --- I'm still seeing the original commits and still on the old baseline from Sept. I even re-fetched this PR to my local repo and see the same. I also don't see the normal log messages that appear in the PR when you force push that says you updated it.

Sorry just noticed that the branch for the PR is vxworks7-2. Not rebase/vxworks7. I will update to that branch.

[jphickey]

Somehow it looks like typecasts got into this one whereas they were not in there before. Note that OSAL PR #654 (currently in IC) updates a lot of these so it better if we do NOT include them here or else we'll get merge conflicts.

Requesting that the typecast updates to fix warnings at least be a separate PR.... otherwise looks better now. I will also pull this to my local machine and test it (haven't done yet).

Have you confirmed coverage testing and all on this?

[ghost]

Somehow it looks like typecasts got into this one whereas they were not in there before. Note that OSAL PR #654 (currently in IC) updates a lot of these so it better if we do NOT include them here or else we'll get merge conflicts.

Requesting that the typecast updates to fix warnings at least be a separate PR.... otherwise looks better now. I will also pull this to my local machine and test it (haven't done yet).

So it's required that I have the typecasts and changes in here to avoid compiler warnings (which are errors due to Werror=conversion, on by default in vxworks7) so it seemed to me that this was acceptable to have vxworks7 support. I can take all that out (I made a separate conversion branch the vxworks7 work is on top of) but then I'd have to update my toolchain or change the osal's CMakeLists to avoid conversion errors. What do you prefer?

Have you confirmed coverage testing and all on this?

I have only run this on vxworks7. I don't have access to run on 6.

[ghost]

Ok this still needs some work. I forgot I had Wno-conversion in my toolchain file, so I've missed some typecasts in the conversion branch. Should I just remove that branch from this entirely and not have any typecasts at all? I can leave Wno-conversion in my toolchain file.

[jphickey]

So it's required that I have the typecasts and changes in here to avoid compiler warnings (which are errors due to Werror=conversion, on by default in vxworks7) so it seemed to me that this was acceptable to have vxworks7 support.

I don't consider a warning option to be in the critical path of getting VxWorks 7 to work like the other stuff is. Preference is for focused PRs that can be individually reviewed and tested.

I'm mainly just requesting that this warning resolution stuff be split into a separate PR. It kinda requires a different review and testing and one can just as easily add -Wno-conversion for the time being which is really the way gcc defaults to in the current builds.

CCB can then decide if we want to build with -Wconversion and apply it across the board to all supported plaforms if we do.

Main thing is to be consistent - we should build all platforms with -Wconversion or without it and make it explicit - not be dependent on compiler defaults. It's really just luck/chance that all cross compilers thus far have been gcc based and they do not include -Wconversion with -Wall like clang does.

Nothing in that should hold up the rest of the VxWorks 7 stuff though. So hence a separate PR is what I recommend.

Have you confirmed coverage testing and all on this?

I have only run this on vxworks7. I don't have access to run on 6.

Coverage testing runs on the native host. Build with SIMULATION=native ENABLE_UNIT_TESTS=true and then run make test and make lcov.

[jphickey]

So it's required that I have the typecasts and changes in here to avoid compiler warnings (which are errors due to Werror=conversion, on by default in vxworks7) so it seemed to me that this was acceptable to have vxworks7 support.

Another way to look at it -- the same problem (probably) currently exists and can be reproduced in VxWorks 6.9 if you add -Wconversion to the command line. Or even on native/POSIX if you add this flag.

So the warnings aren't really a VxWorks 6/7 issue at all - its a compiler settings issue. The fact that VxWorks 7 uses a different compiler with different defaults just muddies that up, because we didn't set it explicitly.

[ghost]

Pushed without the Wconv stuff.

[jphickey]

FYI - I went ahead and wrote a separate issue for the -Wconversion stuff -- see #660. A future PR for that would be associated with that issue number but I recommend waiting until we discuss in CCB and decide which way to go.

[jphickey]

Looks good to me now, at least from inspection point of view.

Still need to make sure coverage test is working and covering what it is supposed to (not clear if the directory implementation might need a coverage update due to use of the POSIX version now).

[skliper]

With the addition of vxworks7 support to the open source repo, we need a way to confirm it works in CI or at minimum a manual setup/run (that hopefully could be implemented in a runner or similar). Is there a way to build and run the OSAL functional tests using the vxworks SDK? If this isn't possible, at the very minimum we need a setup available locally to build/test (what we do for vxworks 6.9).

[astrogeco]

CCB 20201202 will revisit next week

• need some updates to coverage check based on comments
• need to set up the SDK and have a lab setup for testing
  • Some discussion about setting up a hardware sim. See https://labs.windriver.com/vxworks-sdk/#read

[jphickey]

OK! The code changes look good now.

Only remaining issue is that this needs a squash merge up to the current "main" branch. I ran into a build/dependency issue when I tried to build this locally.

Recommendation is to fetch the latest main, create/checkout a new branch on main, then do git merge --squash vxworks7-2 ... fix conflicts and build/confirm ... then git push $fork $new_branch:vxworks7-2 to update this PR.

[jphickey]

Also when writing a new commit message for the squashed branch - please use the basic commit message template with:

Fix #610,  <short description>

<long description>

This should be the pattern used for all commits.

[jphickey]

Looks good!

[astrogeco]

CCB 2020-12-09 APPROVED

• We need to test this with VxWorks 6
• Need test setup for VxWorks 7

[astrogeco]

@jphickey can you help me with this conflict? feel free to merge into integration-candidate when fixed

[astrogeco]

@jphickey can you help me with this conflict? feel free to merge into integration-candidate when fixed

I think I fixed them look at line 237 in src/os/vxworks/src/os-impl-tasks.c in #690