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 #972, reorganize directory structure #1203

Merged
merged 6 commits into from
Mar 11, 2021

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Mar 3, 2021

Describe the contribution
Reorganize/Refactor the CFE source code directory structure per the discussion in #972.

Important notes:

  • This uses CMake "interface libraries" to define the CFE APIs - the public interface is now defined in the cfe_app_intf and the internal interface (private types, stuff that goes only between core apps) is in cfe_internal_intf.
  • The CMD/TLM message definition headers (e.g. cfe_*_msg.h) are within the fsw/inc directory of the respective module - because these are "owned" by the module itself.
  • Any data types that are shared between CMD/TLM message and API functions should be defined in the "extern_typedefs.h" file (some things did have to move to meet this).
  • Split all public headers into two groups:
    • the "api_typedefs.h" (e.g. cfe_es_api_typedefs.h) which is intended to define the data types and constants used in the API, which in turn may be included by other headers that need to reference/build upon the data types in the respective API.
    • the actual API header (e.g. cfe_es.h) which contains the all function prototypes. It is intended to be included only from C files.

Fixes #972

Testing performed
Build and sanity test CFE
Run all unit tests

Expected behavior changes
None outside build system.

System(s) tested on
Ubuntu 20.04 (native)
Testing is not yet sufficient - Still needs more verification on other platforms and configurations (static apps, etc).

Additional context
Although this is a substantial change to the build system and directory structure, very little (if any?) of the FSW C code was actually changed.

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

@jphickey jphickey changed the base branch from main to integration-candidate March 3, 2021 15:41
@jphickey
Copy link
Contributor Author

jphickey commented Mar 3, 2021

Pushed as draft for pre-review ... The real commit is in 12412c2

This really needs a stable baseline due to the fact that it moves files around, it can be difficult to auto-merge.

Corrected: some debug/saved files I forgot to remove were still in the original push. Cleaned up now.

@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Mar 3, 2021
@jphickey jphickey force-pushed the fix-972-directory-structure branch from 3c4b9cf to 12412c2 Compare March 3, 2021 15:52
@astrogeco astrogeco added CCB:2021-03-03 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Mar 3, 2021
@jphickey
Copy link
Contributor Author

jphickey commented Mar 3, 2021

For review/summary - the directory structure of the modules dir now looks like this:

modules/
├── cfe_app_intf
│   ├── eds
│   ├── fsw
│   │   └── inc
│   └── ut-stubs
│       └── src
├── cfe_assert
│   ├── inc
│   └── src
├── cfe_internal_intf
│   ├── eds
│   ├── fsw
│   │   └── inc
│   └── ut-stubs
│       ├── inc
│       └── src
├── cfe_testcase
│   └── src
├── cfe_testrunner
│   ├── inc
│   └── src
├── es
│   ├── eds
│   ├── fsw
│   │   ├── inc
│   │   └── src
│   └── ut-coverage
├── evs
│   ├── eds
│   ├── fsw
│   │   ├── inc
│   │   └── src
│   └── ut-coverage
├── fs
│   ├── fsw
│   │   └── src
│   └── ut-coverage
├── msg
│   ├── fsw
│   │   ├── inc
│   │   └── src
│   ├── option_inc
│   └── ut-coverage
├── resourceid
│   ├── fsw
│   │   └── src
│   ├── option_inc
│   └── ut-coverage
├── sb
│   ├── eds
│   ├── fsw
│   │   ├── inc
│   │   └── src
│   └── ut-coverage
├── sbr
│   ├── fsw
│   │   └── src
│   └── ut-coverage
├── tbl
│   ├── eds
│   ├── fsw
│   │   ├── inc
│   │   └── src
│   └── ut-coverage
└── time
    ├── eds
    ├── fsw
    │   ├── inc
    │   └── src
    └── ut-coverage

@astrogeco astrogeco added the CCB:Splinter Item needs a focused splinter meeting label Mar 3, 2021
@astrogeco
Copy link
Contributor

astrogeco commented Mar 3, 2021

CCB:2021-03-03

  • This is a draft so needs some more cleanup
  • Bumps cmake minimum version to 3.5
  • Looking for suggestions on naming option_inc
  • Does this fix the missing c99 standard flag in tbl compilation?
  • Remove get_current_cflags hack
  • Instead of listing cfe-core we now list all the cfe components
  • splits up api definitions and types from module headers think about renaming *_extern_api_typedefs.h to avoid confusion with *_api_typedefs.h
    • the extern typedefs could come from an external database
  • stubs need to be defined with the library
  • Move "semiprivate" functions into a separate header.
    • Which functions should apps be allowed to call? Do we care that apps can't see these functions?
    • Didn't SB use "priv" in the file name for APIs that normal applications should not call?
  • There was an attempt to clean up internal headers
    • Should we use "internal" or "private"? What's the difference?
  • Minimal and transparent effect on apps that are written "the right way"
  • email the community to give people a heads up
  • Some projects use CCDD which will break after this change
  • Also clean up deprecated elements

@astrogeco
Copy link
Contributor

@skliper @jwilmot what other words can we use for the "fsw" directory? As our userbase and applications grow I think it would be interesting to have that directory be named something that encompasses more "project types". What about something like "rt_sw" for real-time sw?

I know this is a big paradigm change and we should honor our fsw roots though. Thoughts?

@astrogeco
Copy link
Contributor

@jphickey would it make sense for cfe_app_intf to instead be named cfe_api ?

@skliper
Copy link
Contributor

skliper commented Mar 3, 2021

@skliper @jwilmot what other words can we use for the "fsw" directory? As our userbase and applications grow I think it would be interesting to have that directory be named something that encompasses more "project types". What about something like "rt_sw" for real-time sw?

I know this is a big paradigm change and we should honor our fsw roots though. Thoughts?

The key for me is that it's uniquely identified vs test or any other code in the repo so from that context "fsw" is sufficient, and doesn't limit use. It is the core Flight Executive and core Flight System, rebranding may be a hard sell. It's not really "real-time" in the classic sense since that depends on OS, and anything I can think of generic enough to cover all uses looses it's meaning.

@jphickey
Copy link
Contributor Author

jphickey commented Mar 3, 2021

I still like "fsw" - it has heritage/history, and its obvious what it refers to (i.e. its one of those acronyms that really doesn't need a definition). Given the number of times the word "flight" is referred to in both code and docs (and the overall system name!), this will probably never change.

@jphickey
Copy link
Contributor Author

jphickey commented Mar 3, 2021

@jphickey would it make sense for cfe_app_intf to instead be named cfe_api ?

I'm fine with that. What about cfe_internal_intf -- should this be simpler, like e.g. cfe_core maybe? (I originally hesitated from calling it cfe core because this used to refer to the public intf, not the internal one).

@astrogeco
Copy link
Contributor

@jphickey would it make sense for cfe_app_intf to instead be named cfe_api ?

I'm fine with that. What about cfe_internal_intf -- should this be simpler, like e.g. cfe_core maybe? (I originally hesitated from calling it cfe core because this used to refer to the public intf, not the internal one).

Yeah I was thinking about "internal" but couldn't come up with anything. Maybe private? cfe_core is an easy to overload term so I don't like it

@jphickey
Copy link
Contributor Author

jphickey commented Mar 3, 2021

Yeah I was thinking about "internal" but couldn't come up with anything. Maybe private? cfe_core is an easy to overload term so I don't like it

This is a tough one because "private" (at least to me) implies localized to a single module, which this is not (i.e. not shared at all). I sometimes refer to this as "quasi-private" - but I wanted a term that conveys it is shared within the group, but not widely shared beyond the group.

@astrogeco
Copy link
Contributor

This is a tough one because "private" (at least to me) implies localized to a single module, which this is not (i.e. not shared at all). I sometimes refer to this as "quasi-private" - but I wanted a term that conveys it is shared within the group, but not widely shared beyond the group.

What if we use private then use "local" for the module-specific stuff?

@skliper
Copy link
Contributor

skliper commented Mar 3, 2021

This is a tough one because "private" (at least to me) implies localized to a single module, which this is not (i.e. not shared at all). I sometimes refer to this as "quasi-private" - but I wanted a term that conveys it is shared within the group, but not widely shared beyond the group.

What if we use private then use "local" for the module-specific stuff?

cfe_local? or cfe_scope... local_scope, restricted? internal... where's my thesaurus.

@astrogeco
Copy link
Contributor

cfe_local?

or cfe-core_local or just core_local?.

Also what is our guidance for underscores vs dashes again?

@astrogeco
Copy link
Contributor

Separate question, do we want to drop the cfe_* prefix or replace it with core on these directory names? It feels redundant since everything will be under the cfe "repo folder"

@jphickey
Copy link
Contributor Author

jphickey commented Mar 3, 2021

Separate question, do we want to drop the cfe_* prefix or replace it with core on these directory names? It feels redundant since everything will be under the cfe "repo folder"

I do concur about the redundant-ness of the directory name - HOWEVER - there are other patterns involved here. The directory defines a CMake target of the same name - this is a common pattern in all modules. (i.e. so "es" defines an "es" target, etc). This becomes apparent when linking, where one has to list the interface in "target_link_libraries" - in this context the "cfe_" prefix makes sense. That is, one does target_link_libraries(${my_app} cfe_api). I suppose a target named core_api would be OK too though.

Significant reorganization of the CFE core directory
and header file structure.

All modules become separate subdirectories under fsw/modules.

Additionally, the interfaces to CFE core (public and internal)
are also separated into modules.

CMake "interface libraries" and related constructs are used to
manage the include paths to all the separate modules.
This updates the cppcheck static analysis rule to run via a script
which takes into account the new directory structure.

Oddly this reported new (valid) issues in FSW code which have also
been corrected.

Notably the wrapper script also contains a special provision to
run static analysis on TIME only with SERVER mode enabled.  The
default logic of cppcheck would run all combos and it is not valid
to have both of these defined.
@jphickey jphickey force-pushed the fix-972-directory-structure branch from aca9147 to a53428c Compare March 9, 2021 17:01
@jphickey jphickey marked this pull request as ready for review March 9, 2021 17:11
@jphickey
Copy link
Contributor Author

jphickey commented Mar 9, 2021

Build checks passing, should be basically ready for its final review and merge.

@jphickey jphickey closed this Mar 10, 2021
@jphickey jphickey deleted the fix-972-directory-structure branch March 10, 2021 14:43
@jphickey jphickey restored the fix-972-directory-structure branch March 10, 2021 14:46
@jphickey jphickey reopened this Mar 10, 2021
@jphickey jphickey closed this Mar 10, 2021
@jphickey jphickey reopened this Mar 10, 2021
@astrogeco
Copy link
Contributor

astrogeco commented Mar 10, 2021

Notes from CCB Splinter 2021-03-10

  • Open new issue to update file diagrams in documentation
  • For Section 4.2.1, Does it make sense to put tables in fsw for apps?
    • The idea is that users will customize what is in that directory
  • How are PerfIDs scoped? Are they globally unique or just processor specific?
    • Should we increase the number space ?
  • Can we put the CI changes into their own commit?
  • Add ci_lab and psp related PRs to "additional context" and add "dependency" label

The userguide build needs to include fsw/inc from all modules
Create a separate "core_internal.h" header file for prototypes
that were only intended for CFE core use.
@astrogeco astrogeco removed the CCB:Splinter Item needs a focused splinter meeting label Mar 11, 2021
@astrogeco astrogeco merged commit 9bc7bfd into nasa:main Mar 11, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request Mar 11, 2021
astrogeco added a commit to astrogeco/cFS that referenced this pull request Mar 12, 2021
@jphickey jphickey deleted the fix-972-directory-structure branch April 9, 2021 18:01
@skliper skliper added this to the 7.0.0 milestone Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor directory structure so it makes sense again (fsw contains non-fsw)
3 participants