nasa / cFE

The Core Flight System (cFS) Core Flight Executive (cFE)
Apache License 2.0
408 stars 200 forks source link

Naming convention for macros in cfe_mission_cfg and cfe_platform_cfg #142

Closed skliper closed 4 years ago

skliper commented 4 years ago

The cfe_mission_cfg.h and cfe_platform_cfg.h files contain numerous macros for tuning CFE to the jobs that need to be performed.

The two files have different scope:

It is important that the scope of these definitions is well defined and used consistently throughout the code. A naming convention should be introduced so that it is clear when reading the code whether the macro has mission-level or platform-level scope.

skliper commented 4 years ago

Imported from trac issue 111. Created by jphickey on 2015-09-08T10:03:23, last modified: 2019-03-05T14:58:28

skliper commented 4 years ago

Trac comment by sstrege on 2016-09-17 15:21:25:

CCB needs to agree on a naming convention, then trivial search and replace. Will update when naming convention is finalized.

skliper commented 4 years ago

Trac comment by jphickey on 2016-11-08 11:58:00:

The original proposal here (well over a year ago) was to use the following prefixes:

I still think this naming convention will work well. IIRC the issue at the time is that it ends up modifying //a lot// of files (about 60) and it was intermixed with some actual code changes when it was first proposed. On this ticket we could implement the name change as a change set by itself that does not modify any code besides the symbol name, but it is still likely to touch a lot of files.

skliper commented 4 years ago

Trac comment by jphickey on 2016-11-08 12:44:08:

I took the liberty of hacking out a quick bash script to perform the search/replace that I described above in my previous comment. To get an idea of the changes involved in doing this I have pushed it to babelfish.

It is commit [changeset:35a2d07] on branch trac-111-proposed-naming-convention

This ONLY does a straight name replacement on the various macros. It doesn't change usage in any way. It builds just fine for me but be aware that the names are also referenced in some apps and in the PSP so it requires a corresponding search/replace changeset in those areas as well.

skliper commented 4 years ago

Trac comment by glimes on 2016-11-08 14:17:08:

Current crop of cfe-next are all going into CFE 6.6

skliper commented 4 years ago

Trac comment by cdknight on 2016-11-08 14:32:05:

I have one request regarding backward-compatibility, I suggest having a master switch to change the old macros to generate compiler errors. Initially it can be used with a compile-time define to test folks' code, but in a later release, the switch should be enabled by default to break. (Is there a more elegant way to do this?)

{{{

ifndef CFE_DEPRICATE_6_5_MACROS

define CFE_CPU_ID CFE_PLATFORM_CPU_ID

define CFE_CPU_NAME CFE_PLATFORM_CPU_NAME

/ and all the rest of the #define's... /

else / ! CFE_DEPRICATE_6_5_MACROS /

define CFE_CPU_ID ...depricated, use CFE_PLATFORM_CPU_ID... / ... intentional to cause compile error /

define CFE_CPU_NAME ...depricated, use CFE_PLATFORM_CPU_NAME...

/ and all the rest of the #define's... /

endif / CFE_DEPRICATE_6_5_MACROS /

}}}

skliper commented 4 years ago

Trac comment by jphickey on 2016-11-08 14:56:32:

I agree on the comment regarding deprecated names. On the OSAL side there is a build macro called OSAL_OMIT_DEPRECATED that, when //set//, all backward-compatibility mappings are removed/skipped from the header file.

The idea is that by default, if nothing is done, the backward compatibility macros are enabled such that old code will continue to build. But when it is desired to check that code is not using any deprecated name, the compatibility layer can be taken out and therefore any reference to the old name will trigger a compiler error.

I think this should have a similar macro, the only question is what that macro should be named ... on one hand I think it might be preferable to have only one such switch in the system, which would theoretically disable anything that the CCB considers deprecated. The alternative is to have a dedicated switch per "feature" but I think that might make too many switches in the long run and might be harder to deal with.

skliper commented 4 years ago

Trac comment by sstrege on 2016-11-22 12:54:39:

I am also in agreement on the comment regarding depreciated names and in agreement on, if nothing is done, the backward compatibility macros are enabled.

Since the OSAL already has a built in macro, I recommend the macro on the cFE side be named similarly i.e. CFE_OMIT_DEPRECATED. I also like having one switch vs. a dedicated switch per feature. The deprecated code really needs to be cleaned up ASAP. This switch should not be enabling too many depreciated features.

skliper commented 4 years ago

Trac comment by cdknight on 2016-11-22 13:52:21:

I recommend that the Bamboo build system have a build where the macros are deprecated.

skliper commented 4 years ago

Trac comment by jphickey on 2016-12-05 20:41:15:

Update: Commit [changeset:e8d2067] includes the compatibility layer. I am able to compile existing application code when using this translation. By adding a -DCFE_OMIT_DEPRECATED_6_6 the compiler errors out upon any usage of the old macro name (and apps which have not been updated will fail to compile).

skliper commented 4 years ago

Trac comment by glimes on 2017-01-31 13:25:59:

Merged 05-dec-2016, paperwork lagged.

skliper commented 4 years ago

Trac comment by jhageman on 2019-03-05 14:58:28:

Milestone renamed