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 #710, Specify ProcessorID in targets.cmake #773

Merged

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Jul 9, 2020

Describe the contribution

Allow explicitly setting of the processor ID in targets.cmake

Testing performed
Build and sanity-check CFE
Build and run all unit tests.
Confirm that CFE_PSP_GetProcessorId() now returns the expected value.

Expected behavior changes
The TGTx_PROCESSOR_ID setting in targets.cmake will be passed to the final build/link of CFE core as the CPU ID. If unspecified then the CMake index value is used instead (backward compatible).

System(s) tested on
Ubuntu 20.04

Additional context
Minimal/simple fix that at least should address part of the problem. Note this value isn't passed to elf2cfetbl for table builds at the moment, this only affects the CFE runtime value of processor ID.

At least on pc-linux one can still provide the value on the command line which takes precedence over anything in the build.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey force-pushed the fix-710-specify-processorid branch from 640f84c to fe47a3e Compare July 9, 2020 14:58
@jphickey jphickey changed the title Fix 710 specify processorid Fix #710, Specify ProcessorID in targets.cmake Jul 9, 2020
@jphickey
Copy link
Contributor Author

jphickey commented Jul 9, 2020

Ping @excaliburtb - this is a very minimalist approach to start with - if you like this we can potentially merge it.

Going forward I'm starting to like the idea of going fully name-based and remove the index numbers from targets.cmake.

something like:

set(MISSION_CPUNAMES cpu1 cpu2)

set(cpu1_PROCESSOR_ID 1234)
set(cpu1_APPLIST ...)
set(cpu1_PLATFORM ...)

set(cpu2_PROCESSOR_ID 5678)
set(cpu2_APPLIST ...)
set(cpu2_PLATFORM ...)

Obviously this is a bigger change to do, but it would actually clean up a bunch of implementation aspects if we did it this way, and it would probably be more logical from a user perspective too.

@excaliburtb
Copy link

yes, that works and addresses my immediate problem.

When we were doing our own cmake script mods, we basically took the approach of, if a cpu list of names exists, process the list, otherwise use the index approach

so the read_targetconfig function would look like:

  if (NOT DEFINED TGT_CPUIDS)
    set(TGT_CPUIDS)
    while(1)
      math(EXPR TGTID "${TGTID} + 1")
      if (NOT DEFINED TGT${TGTID}_NAME)
        break()
      endif()
      if (NOT DEFINED TGT${TGTID}_SYSTEM)
        set(TGT${TGTID}_SYSTEM "cpu${TGTID}")
        set(TGT${TGTID}_SYSTEM "${TGT${TGTID}_SYSTEM}" PARENT_SCOPE)
      endif()
      if (NOT DEFINED TGT${TGTID}_PLATFORM)
        set(TGT${TGTID}_PLATFORM "default" "${TGT${TGTID}_NAME}")
        set(TGT${TGTID}_PLATFORM "${TGT${TGTID}_PLATFORM}" PARENT_SCOPE)
      endif()
      LIST(APPEND TGT_CPUIDS "${TGTID}")
    endwhile()
  endif()

  foreach(TGTID IN LISTS TGT_CPUIDS)

    if (SIMULATION)
    .. (etc... )

something like that would keep it backwards compatible but, as you say, may end up keeping up some of the implementation aspects you were hoping to get rid of without names.

Add support for a TGTx_PROCESSOR_ID directive, which allows
one to set the default value returned by CFE_PSP_GetProcessorId()
function, rather than assuming the index value from CMake.
@jphickey jphickey force-pushed the fix-710-specify-processorid branch from 77e5a06 to 2868acc Compare July 9, 2020 15:43
@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Jul 9, 2020
@jphickey
Copy link
Contributor Author

jphickey commented Jul 9, 2020

Thanks! So if this at least is a step in the right direction I've squashed it and marked at as CCB ready. I can't foresee this breaking anything.

In the meantime I will work on the name-based approach.

@jphickey jphickey marked this pull request as ready for review July 9, 2020 15:49
@jphickey jphickey requested a review from skliper July 9, 2020 15:49
@jphickey
Copy link
Contributor Author

jphickey commented Jul 9, 2020

Note to reviewers - This is related to #710 and #265, but does not fully address either one of them, so I intentionally did NOT link this PR to those issues because it doesn't address all aspects of either issue, this just addresses the ability to set a processor ID explicitly rather than assuming the sequential number.

@excaliburtb
Copy link

thanks for the support!

@skliper
Copy link
Contributor

skliper commented Jul 10, 2020

@astrogeco Could you merge this into IC? Priority.

@jphickey
Copy link
Contributor Author

See also follow-on PR in #776 - this takes a stab at name-based config described above - seems to work nicely.

@astrogeco astrogeco changed the base branch from master to integration-candidate July 15, 2020 13:20
@astrogeco astrogeco merged commit 1039f28 into nasa:integration-candidate Jul 15, 2020
@astrogeco astrogeco removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Aug 18, 2020
@skliper skliper added this to the 6.8.0 milestone Aug 21, 2020
@jphickey jphickey deleted the fix-710-specify-processorid branch October 14, 2020 03:27
@skliper skliper linked an issue Sep 24, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:FastTrack Priority: Mission Feature or bug related to stakeholder needs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify/clean up spacecraft/cpu names/ids
5 participants