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

Add cmake function to add cpus by function rather than assume consecutive cpu ids. #265

Closed
skliper opened this issue Sep 30, 2019 · 12 comments · Fixed by #776
Closed

Add cmake function to add cpus by function rather than assume consecutive cpu ids. #265

skliper opened this issue Sep 30, 2019 · 12 comments · Fixed by #776
Labels
enhancement Priority: Mission Feature or bug related to stakeholder needs
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

Currently, the cmake rules require that cpus are built in order and will stop looking for cpu targets when a cpu idx is not found.

Propose that a cmake function can be executed from the targets.cmake file which explicitly adds the cpus to be built for the mission.

@skliper skliper added this to the 6.8.0 milestone Sep 30, 2019
@skliper skliper self-assigned this Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Imported from trac issue 234. Created by tbrain on 2018-03-30T13:08:24, last modified: 2019-07-03T12:58:32

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by sseeger on 2018-05-28 13:38:55:

Thomas,

Just to be clear are you wanting something in targets.cmake that looks like

SET(TARGETS cpu1 cpu3 test123)

Which will then tell cmake to build TGT_CPU1 TGT_CPU3 and TGT_TEST123?

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-03-04 18:03:43:

Owner - please provide updates/responses.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-07-03 12:58:32:

Moved unfinished 6.7.0 tickets to next release

@skliper skliper removed their assignment Sep 30, 2019
@skliper skliper removed this from the 6.8.0 milestone Mar 2, 2020
@skliper
Copy link
Contributor Author

skliper commented Jul 7, 2020

Ping @excaliburtb - no action in a while, I'll tag as a mission priority.

@skliper skliper added the Priority: Mission Feature or bug related to stakeholder needs label Jul 7, 2020
@skliper skliper added this to the 7.0.0 milestone Jul 7, 2020
@skliper
Copy link
Contributor Author

skliper commented Jul 7, 2020

@jphickey thoughts?

@excaliburtb
Copy link

excaliburtb commented Jul 7, 2020

The actual requirement is to be able to specify multiple targets in targets.cmake

set(TGT1_NAME)
set(TGT2_NAME)

but if I have an environmental or variable-based conditional do:
if(DEFINED foo)
// build TGT1
else()
// build TGT2
endif()

Last I looked, this was an impossible setup because read_targetconfig looks for sequential targets. Also, one might want to build just TGT3 for some reason. Right now, one has to rename the TGT3 configurations. Not impossible but definitely inconvenient.

@jphickey
Copy link
Contributor

jphickey commented Jul 7, 2020

FWIW, the CMake build system was designed to view the build as a "system" - a set of services/apps that are deployed across one or more CPUs (maybe or maybe not discrete/separate) that collectively implement your spacecraft operation software. In that view it shouldn't be "different" depending on an environment variable.

The thought was hardware differences should be abstracted by the PSP, allowing the same set of high level apps to run, with only perhaps different configuration files/tables as you moved to a different host system type. It wasn't anticipated that there might be a fully different CPU definition. Or if there was, that one of these two approaches could work:

  • it could be built but optionally not deployed/ignored; OR
  • if it really had to be configured differently, that an entirely separate "defs" directory (and targets.cmake file) would be used to describe the alternative configuration (this does work but requires to redefine the mission/platform config too).

As adding/changing CPUs has implications on the MSGID assignment/allocations, there is a mission config component to redefining a CPU - so not really something that should be done via environment variable IMO.

What would be fairly trivial to do, however, would be to allow one to configure the CPU number that is returned by default from CFE_PSP_GetProcessorId() - i.e. something like TGTx_NUMBER where it would match the CMake target number by default if unspecified, but can present a different number at runtime if you so choose. That is probably easy to do.

@jphickey
Copy link
Contributor

jphickey commented Jul 7, 2020

If we had the opportunity to re-think the targets.cmake file entirely, I would do it again using name-based lists rather than number-based. That is, have a list of CPU names, then define <name>_APPLIST, <name>_PLATFORM, <name>_SYSTEM, etc.

That might be still worth considering, but it would have a major impact on backward compatibility.

@skliper
Copy link
Contributor Author

skliper commented Jul 8, 2020

I'd say fair game for 7.0 if stakeholders are on board.

@excaliburtb
Copy link

Just to clarify a bit.. when i build a specific target, I keep all the configuration that goes along with it because we have already diverged from the cpu<N>_*.h default approach to configurations and tables and have switched to using named targets instead. All our organization is based on <name>_*.h or <name>/*.h. I am also seeing several other projects take the same approach.

As far as having multiple target_defs to piece up the build, that approach has two failings. 1.) It greatly increases the integration efforts to keep everything in sync. and 2.) it doesn't take into account the problems faced with heterogenous targets where not all tool chains are available to all users of missions or, even simply, tools like continuous integration.

I consider target_defs to be an integration point between flight computers of a single vehicle and we almost always run into a situation where we need to build a single piece of the configuration, rather than the whole thing. My point being, users are already doing this but they are having to alter the cmake scripts to do it. So, yes, name-based lists would be much better than index-based.

jphickey added a commit to jphickey/cFE that referenced this issue Jul 10, 2020
Remove requirement to define a sequential/numbered list of targets.
Instead, user defines a list of target names, followed by a set of
properties associated with each name.

This makes it much simpler to consistently define CPUs but selectively
add/remove them from the build based on other criteria.
@jphickey
Copy link
Contributor

For stakeholders of this, see PR #776 (above) which is a follow on to my previous PR #773. It's a bit more of a change but it seems to work nicely from my testing so far. Looking for feedback. Thanks!

@jphickey jphickey linked a pull request Jul 10, 2020 that will close this issue
astrogeco added a commit that referenced this issue Sep 4, 2020
Fix #265, implement cmake name based targets
astrogeco added a commit that referenced this issue Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Priority: Mission Feature or bug related to stakeholder needs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants