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

Refactor directory structure so it makes sense again (fsw contains non-fsw) #972

Closed
skliper opened this issue Oct 27, 2020 · 16 comments · Fixed by #1203 or #1222
Closed

Refactor directory structure so it makes sense again (fsw contains non-fsw) #972

skliper opened this issue Oct 27, 2020 · 16 comments · Fixed by #1203 or #1222
Assignees
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Oct 27, 2020

Is your feature request related to a problem? Please describe.
fsw is misleading in that it includes unit tests, etc but then there are also fsw implementations within modules...

Describe the solution you'd like
Implement a flattened/sensible directory structure. Needs discussion.

Describe alternatives you've considered

  • cfe/
    • CMakeLists.txt for the repo
    • fsw/
      • CMakeLists.txt for the fsw
      • sbr/
        • CMakeLists.txt for sbr (and so on for CMake files)
      • msg/
      • core/
        • es/
        • ...
      • public_inc/
    • unit-test/
      • stubs/
      • coverage/
      • functional/
    • eds/
    • docs/
    • ... and so on

Additional context
#947 (review)

Requester Info
Jacob Hageman - NASA/GSFC

ping @acudmore

EDIT - updated for Gerardo comment

@skliper skliper added CCB:Ready Ready for discussion at the Configuration Control Board (CCB) enhancement labels Oct 27, 2020
@skliper
Copy link
Contributor Author

skliper commented Oct 27, 2020

Also related to #626 (utilize cmake libraries) that could help clean up includes/dependencies.

@astrogeco
Copy link
Contributor

astrogeco commented Oct 27, 2020

I would also suggest a unit-test directory with functional, stubs, and coverage subdirectories

@astrogeco astrogeco added CCB-20201028 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Oct 28, 2020
@astrogeco
Copy link
Contributor

CCB 2020-10-28 APPROVED

@ghost
Copy link

ghost commented Oct 28, 2020

Something like the simplified tree in the original comment would be great.

@astrogeco
Copy link
Contributor

I assume "core" contains the base services (es, evs, etc.). What do we get from nesting those as opposed to keeping them directly under "fsw"

@skliper
Copy link
Contributor Author

skliper commented Oct 28, 2020

I assume "core" contains the base services (es, evs, etc.). What do we get from nesting those as opposed to keeping them directly under "fsw"

The "core" services all use one CMakeList.txt, and become one cfe-core library in the build context. The individual modules each have their own CMakeLists.txt and can be removed/replaced via configuration. Eventually everything could follow the module approach and we could break out cfe-core into it's elements, but we aren't there yet (and it's not critical for Caelum).

@jphickey
Copy link
Contributor

jphickey commented Oct 28, 2020

Need to consider how this affects the build system and the way it finds components. In particular the component-level CMakeLists.txt files need to reside at the top level of the module they define.

I have two possible suggestions for consideration. The first is a "lighter" version that still preserves the current core grouping and wouldn't substantially impact the general build scripts or user understanding of the layout:

.
├── cmake
│   ├── sample_defs
│   └── target
│       ├── CMakeLists.txt
│       ├── inc
│       └── src
├── core
│   ├── CMakeLists.txt
│   ├── eds
│   ├── fsw
│   │   ├── es
│   │   ├── evs
│   │   ├── fs
│   │   ├── sb
│   │   ├── tbl
│   │   └── time
│   ├── mission_inc
│   ├── private_inc
│   └── unit-test
│       ├── coverage
│       ├── functional
│       └── stubs
├── docs
└── modules
    ├── cfe_assert
    │   ├── CMakeLists.txt
    │   ├── inc
    │   └── src
    ├── cfe_testcase
    │   ├── CMakeLists.txt
    │   └── src
    ├── cfe_testrunner
    │   ├── CMakeLists.txt
    │   ├── inc
    │   └── src
    └── msg
        ├── CMakeLists.txt
        ├── mission_inc
        ├── src
        └── unit-test
            ├── coverage
            ├── functional
            └── stubs

In this approach the MISSION_CORE_MODULES would be modified to contain core instead of cfe-core but it should otherwise work as long as the corresponding search path is updated, I don't think it would require much script modification other than to adjust the paths.

The second is a more full-blown modularization, which is what I think we should strive toward but is definitely a bigger change not only in the scripting but also user mindset:

.
├── cmake
│   ├── sample_defs
│   └── target
│       ├── inc
│       └── src
├── docs
└── modules
    ├── cfe_assert
    │   ├── CMakeLists.txt
    │   ├── inc
    │   └── src
    ├── cfe_testcase
    │   ├── CMakeLists.txt
    │   └── src
    ├── cfe_testrunner
    │   ├── CMakeLists.txt
    │   ├── inc
    │   └── src
    ├── es
    │   ├── CMakeLists.txt
    │   ├── eds
    │   ├── fsw
    │   │   ├── inc
    │   │   └── src
    │   └── unit-test
    │       ├── coverage
    │       ├── functional
    │       └── stubs
    ├── evs
    │   ├── CMakeLists.txt
    │   ├── eds
    │   ├── fsw
    │   │   ├── inc
    │   │   └── src
    │   └── unit-test
    │       ├── coverage
    │       ├── functional
    │       └── stubs
    ├── fs
    │   ├── CMakeLists.txt
    │   ├── eds
    │   ├── fsw
    │   │   ├── inc
    │   │   └── src
    │   └── unit-test
    │       ├── coverage
    │       ├── functional
    │       └── stubs
    ├── msg
    │   ├── CMakeLists.txt
    │   ├── eds
    │   ├── fsw
    │   │   ├── inc
    │   │   └── src
    │   └── unit-test
    │       ├── coverage
    │       ├── functional
    │       └── stubs
    ├── sb
    │   ├── CMakeLists.txt
    │   ├── eds
    │   ├── fsw
    │   │   ├── inc
    │   │   └── src
    │   └── unit-test
    │       ├── coverage
    │       ├── functional
    │       └── stubs
    ├── tbl
    │   ├── CMakeLists.txt
    │   ├── eds
    │   ├── fsw
    │   │   ├── inc
    │   │   └── src
    │   └── unit-test
    │       ├── coverage
    │       ├── functional
    │       └── stubs
    └── time
        ├── CMakeLists.txt
        ├── eds
        ├── fsw
        │   ├── inc
        │   └── src
        └── unit-test
            ├── coverage
            ├── functional
            └── stubs

In this approach the MISSION_CORE_MODULES would list out all individual core modules directly, i.e. instead of cfe-core it would have es evs sb tbl time fs and we would have to do some work to deal with the current places that core apps call internal functions or reference internal data structures in other core apps - either promote them to public or come up with a different way to handle it. (this is the current fsw/cfe-core/src/inc/private dir - which I'd like to see go away).

Correction - removed core/inc from second - all include files would be in respective modules in the full-modular approach.

@astrogeco
Copy link
Contributor

Uh-oh we're approaching taxonomy territory!

@skliper
Copy link
Contributor Author

skliper commented Oct 28, 2020

I'd prefer an approach that separates fsw from non-fsw. Your modules directory mixes that concept in both options (as does the current structure). I think we would benefit from an obvious difference.

@jphickey
Copy link
Contributor

jphickey commented Oct 28, 2020

Yes - The reality is that all modules may have both fsw and non-fsw components. This is true of all apps and libraries. Ideally - in a general sense - when a user adds modules (of any kind) to their build, they want to add the FSW stuff as all supporting stuff - tests, documentation, etc as one logical unit.

I don't see why we'd want to treat the core modules any differently - they are just other modules.

So the very general form/template would be:

  • CMakeLists.txt at top level for overall app build recipe (builds FSW plus any supporting stuff)
  • eds dir for CMD/TLM definitions
  • fsw dir for actual code that is executing on the flight processor - divided into inc for public API and src for source files
  • unit-test dir for the tests (could be simplified to just test)
  • docs dir for documentation (not shown earlier, but we should probably have it!)

Edit: Clarified to add "may" into first sentence.

@astrogeco
Copy link
Contributor

I think @jphickey's second option is actually not that bad, the fsw separation is inherent inside each module and I like how it encapsulates everything related to each module inside a single directory, similar to how the apps are implemented. That being said, should we really consider cfe_assert, cfe_testcase , and cfe_testrunner as modules? More broadly, what makes something a module?

@jphickey
Copy link
Contributor

@astrogeco - good catch, actually cfe_testcase as it exists today is just a holding place for some ES functional tests - this would be under es/unit-test/functional. So that module goes away in this structure.

cfe_assert (a lib) and cfe_testrunner (an app) are both separately compiled and loaded modules - hence why they are separate. You would add these to cfe_es_startup.scr when running the tests, leave them out for normal FSW deployment. I think they should stay as separate entities, as they are today. They could even be forked into entirely separate repos, but they are so small/simple that seems more work and management effort than its worth. Easier to keep them grouped with CFE core.

I also prefer the second approach - i.e. fully modular - and as I said before, it would take some work to make the CFE core apps play nice in this more isolated arrangement. I think we are close to having this be workable ... Mainly just have to fix some aspects of the ER log and cleanup code that calls directly into other modules and bypasses the public API. Worst case we just move them to public headers but document that they aren't intended to be directly referenced outside CFE itself. But all in all I don't think it would take a whole lot of work to get it running with this fully modularized directory structure.

@astrogeco
Copy link
Contributor

From a taxonomy perspective, those three items don't have a fsw component which makes them outliers. I want to make sure that when people talk about modules everyone agrees on what they are. Having non-fsw items inside the modules directory is confusing.

@jphickey
Copy link
Contributor

Correct - they are test support modules, not FSW modules - although they could be loaded into a FSW environment for test purposes when you want to do that.

Personally I don't think the term "modules" implies FSW - nowhere did we say that every module needs to have a FSW component. Some have FSW, some do not, depending on their purpose.

But that's my opinion - if you wanted to use a separate parent dir that's fine - but I don't think it adds any value, since we already designate what is "fsw" and what is not inside the modules themselves. It is somewhat redundant to designate "fsw" in the parent dir as well.

@astrogeco
Copy link
Contributor

I agree that we never said modules have to have a FSW component. Regardless of what we end up with I want to make sure we define and lay out things in a way that makes sense. So from a definition and educational perspective I'd like to have a statement that says "Modules are composed of such and such, a typical module contains these types of things. If you want to add X functionality you should create a module. If you want to add Y functionality you should create some_other_thing"

@skliper
Copy link
Contributor Author

skliper commented Nov 3, 2020

I concur w/ the second @jphickey option. I think in the short term we can keep the "cfe core" concept for private includes/API bypass. This could all benefit from #626, interface libraries. We will need a spot for ut_support, and possibly consider the task library concept for general task implementations.

Also the target inc/src seems closer to fsw than cmake, would this make more sense to be not under cmake? I could go either way since these are processed, but it feels like we are hiding these.

jphickey added a commit to jphickey/cFE that referenced this issue Mar 3, 2021
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.
jphickey added a commit to jphickey/cFE that referenced this issue Mar 3, 2021
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.
jphickey added a commit to jphickey/cFE that referenced this issue Mar 3, 2021
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.
jphickey added a commit to jphickey/cFE that referenced this issue Mar 9, 2021
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.
jphickey added a commit to jphickey/cFE that referenced this issue Mar 9, 2021
Updates the application developer guide to describe the directory
structure and file naming conventions
jphickey added a commit to jphickey/cFE that referenced this issue Mar 9, 2021
jphickey added a commit to jphickey/cFE that referenced this issue Mar 9, 2021
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 added a commit to jphickey/cFE that referenced this issue Mar 9, 2021
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 added a commit to jphickey/cFE that referenced this issue Mar 9, 2021
jphickey added a commit to jphickey/cFE that referenced this issue Mar 10, 2021
The userguide build needs to include fsw/inc from all modules
jphickey added a commit to jphickey/cFE that referenced this issue Mar 10, 2021
Create a separate "core_internal.h" header file for prototypes
that were only intended for CFE core use.
astrogeco added a commit that referenced this issue Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants