Fix #629, Define BSP type in sample config

[skliper]

Describe the contribution
Update of OSAL_SYSTEM_BSPNAME to OSAL_SYSTEM_BSPTYPE in sample toolchains.
Fix #629

Testing performed
Steps taken to test the contribution:

1. Readme build steps

Expected behavior changes
Builds out of the box with sample config

System(s) tested on

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

Additional context
None

Third party code
None

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

[skliper]

Priority request @astrogeco @jphickey, master is currently broken. Consider for fasttrack.

[skliper]

@dmknutsen - ping... to address the issue you identified

[jphickey]

The changes to the "toolchain-cpuX" files are probably all that is needed here.

[jphickey]

This is important for the SIMULATION=native mode to work.

[skliper]

Can it be improved such that it only overwrites what isn't supplied? Seems like overriding all three settings when one isn't supplied isn't very friendly to user configuration.

[jphickey]

It only sets it if not already set and not crosscompiling. It shouldn't overwrite anything.

[skliper]

Either way, I'll take out. Seems like it would make sense to just default what isn't defined, vs set all three if one of the two is missing. Not very friendly/intuitive from my perspective. If I supply something, I'd expect it to be used.

[jphickey]

Part of this dates back that in previous CFE builds (before recent merges) only CFE_SYSTEM_PSPNAME and OSAL_SYSTEM_OSTYPE was required. That's why this "if" statement is only checking that those two are defined. The OSAL build later confirms that OSAL_SYSTEM_BSPTYPE and OSAL_SYSTEM_OSTYPE are defined (that's the error observed in #629).

It is true that it doesn't handle some but not all being specified but most of the time that's an error anyway, because there is not a default for cross compile. And for non-cross (native) there is only one right answer anyway which is pc-linux/posix.

I would propose rather than fixing that -- the whole thing could be generally simplified a bit -- we really should only need to specify a PSP type. We can't mix and match BSP/PSPs and OSs - although an OS can have more than one possible BSP/PSP, the reverse isn't true - each PSP is written for a specific OS. So by specifying a PSP that in turn can dictate what OS layer to compile, so the toolchain only needs to specify the PSP to use and everything else should fall from that.

But the "default" in arch_build.cmake still needs to be there, to handle the case where not crosscompiling and just building for the host machine, though. In this case no toolchain file is used and therefore these variables will be unset (rightfully).

[skliper]

I'm all for simplification, and definitely support a default. It was only the override I had concerns with, and that it looked to need a refresh based on the recent changes.

[jphickey]

Submitted #632. Looking at the code this might be fairly trivial/easy to do.

[jphickey]

Also for process improvement -- why wasn't an issue like this caught by the CI? I personally don't usually test with "toolchain-cpu1.cmake" cross compile - I either build with SIMULATION=native or cross compile for either RTEMS or VxWorks

[skliper]

CI doesn't do a plain "make prep". Certainly open to improving it.

[jphickey]

Also worth noting that the implicit use of a name-based toolchain-<cpuname>.cmake file was a backward compatibility hack to begin with -- I'd rather one specified the TGTx_SYSTEM variable instead and explicitly specified the toolchain to use for cross compile.

[skliper]

Removed arch_build change, note also submitted #633 to address toolchain hack.

[skliper]

#634 is a better solution, closing this as duplicate

[jphickey]

Maybe still worth discussing - not closing this entirely --

It is still incorrect that these example toolchain-cpu<x>.cmake files are setting a OSAL_SYSTEM_BSPNAME which is incorrect and not actually used anywhere.

If we go with the #634 proposal then then we should remove these lines entirely.

[skliper]

I vote for remove entirely w/ #634.

[astrogeco]

CCB 20200422 - Will close this PR and delete lines in cfe PR #634