Integration Candidate: 2020-05-27

[astrogeco]

Describe the contribution

1. Fix cmdUtil is not 64-bit friendly #2
2. Fix cmdUtil doesn't insert CCSDS checksum #3
3. Fix lgtm c issues #43
4. Fix Add standard type processing to cmdUtil #48
5. Fix Apply standard code style #55
6. Fix Add CCSDS V2 support #80

Testing performed
https://github.com/nasa/cFS/pull/96/checks

Expected behavior changes
PR #92 - Default behavior is the same except adds checksum and doesn't actually require fields. Adds all the packet fields, overrides, more supported data types, etc.

System(s) tested on
Ubuntu:Bionic

Additional context
Part of nasa/cFS#96

Contributor Info - All information REQUIRED for consideration of pull request
Jacob Hageman, NASA-GSFC

[astrogeco]

@skliper getting some ground system build errors in nasa/cFS#96

[skliper]

@astrogeco should be fixed.

[jphickey]

FYI - This fails to compile on a CentOS 6.9 system:

[ 75%] cc1: warnings being treated as errors
/home/joe/code/cfecfs/github-cfs-bundle/tools/cFS-GroundSystem/Subsystems/cmdUtil/cmdUtil.c: In function ‘ProcessField’:
/home/joe/code/cfecfs/github-cfs-bundle/tools/cFS-GroundSystem/Subsystems/cmdUtil/cmdUtil.c:370: error: implicit declaration of function ‘htobe16’
/home/joe/code/cfecfs/github-cfs-bundle/tools/cFS-GroundSystem/Subsystems/cmdUtil/cmdUtil.c:370: error: implicit declaration of function ‘be16toh’
Building C object tools/tblCRCTool/CMakeFiles/cfe_ts_crc.dir/cfe_ts_crc.c.o
/home/joe/code/cfecfs/github-cfs-bundle/tools/cFS-GroundSystem/Subsystems/cmdUtil/cmdUtil.c: In function ‘main’:
/home/joe/code/cfecfs/github-cfs-bundle/tools/cFS-GroundSystem/Subsystems/cmdUtil/cmdUtil.c:666: error: implicit declaration of function ‘htole16’
/home/joe/code/cfecfs/github-cfs-bundle/tools/cFS-GroundSystem/Subsystems/cmdUtil/cmdUtil.c:708: error: implicit declaration of function ‘htobe32’
/home/joe/code/cfecfs/github-cfs-bundle/tools/cFS-GroundSystem/Subsystems/cmdUtil/cmdUtil.c:710: error: implicit declaration of function ‘htole32’
/home/joe/code/cfecfs/github-cfs-bundle/tools/cFS-GroundSystem/Subsystems/cmdUtil/cmdUtil.c:752: error: implicit declaration of function ‘htobe64’
/home/joe/code/cfecfs/github-cfs-bundle/tools/cFS-GroundSystem/Subsystems/cmdUtil/cmdUtil.c:754: error: implicit declaration of function ‘htole64’
make[7]: *** [tools/cFS-GroundSystem/Subsystems/cmdUtil/CMakeFiles/cmdUtil.dir/cmdUtil.c.o] Error 1
make[6]: *** [tools/cFS-GroundSystem/Subsystems/cmdUtil/CMakeFiles/cmdUtil.dir/all] Error 2

[skliper]

FYI - This fails to compile on a CentOS 6.9 system:

Does it work with -std=gnu99 for the host?

[jphickey]

FYI - This fails to compile on a CentOS 6.9 system:

Does it work with -std=gnu99 for the host?

It needs _BSD_SOURCE ... tis the problem with non-standardized functions - not consistent.
I take it we decided against my suggestion to not use these routines and and simply write some endian neutral code in the copy function instead?

[skliper]

FYI - This fails to compile on a CentOS 6.9 system:

Does it work with -std=gnu99 for the host?

It needs _BSD_SOURCE ... tis the problem with non-standardized functions - not consistent.
I take it we decided against my suggestion to not use these routines and and simply write some endian neutral code in the copy function instead?

What makes you think that? Looks to me like the issue is still open at #95, doesn't matter to me if we go with gnu99, add all the flags, or rewrite the functions. I haven't heard from any stakeholders on the matter, and nobody has marked it as a project priority so I'm not prioritizing it. If you want to do it on your personal time go for it.

[jphickey]

Well, the fact that we are merging this version into "master" even though it doesn't really work cross platform, which is the reason I recommended against using these endian conversion functions in the first place.

I would think "the code builds on RHEL 6" is a project priority since many folks still use this OS. Furthermore, I thought there was a coding standards document that said the code should be constrained to ISO standard C (some version, we choose 9899:1999) and these functions are not within that standard.

I'm happy to implement the cross platform/generic solution in #95, but on on my personal time - sorry. Probably wouldn't take much time at all though. Very trivial to do it in standards-compliant way.

[skliper]

Well, the fact that we are merging this version into "master" even though it doesn't really work cross platform, which is the reason I recommended against using these endian conversion functions in the first place.

I would think "the code builds on RHEL 6" is a project priority since many folks still use this OS. Furthermore, I thought there was a coding standards document that said the code should be constrained to ISO standard C (some version, we choose 9899:1999) and these functions are not within that standard.

I'm happy to implement the cross platform/generic solution in #95, but on on my personal time - sorry. Probably wouldn't take much time at all though. Very trivial to do it in standards-compliant way.

There is an internal code standard constraining ISO standard C that we aren't compliant with because we use c99 (it requires c89). Not clear to me why we don't just change the host to gnu99, which is at least a standard, vs custom re-implementations of the same functions. I'd change that one line for free.

[astrogeco]

There is an internal code standard constraining ISO standard C that we aren't compliant with because we use c99 (it requires c89). Not clear to me why we don't just change the host to gnu99, which is at least a standard, vs custom re-implementations of the same functions. I'd change that one line for free.

Is this in the branch standard?

[jphickey]

There is an internal code standard constraining ISO standard C that we aren't compliant with because we use c99 (it requires c89). Not clear to me why we don't just change the host to gnu99, which is at least a standard, vs custom re-implementations of the same functions. I'd change that one line for free.

GNU99 isn't a standard. It is C99 with GNU extensions. Extensions aren't standard.

Even if you change that one line It also makes no relevance to this issue because these endian conversion functions aren't even part of GNU extensions, they are BSD extensions.

Many organization coding standards do make provisions to use newer versions of ISO standards where relevant, not limited to the revision at the time of writing. It is unfortunate if GSFC did not include such a provision, but I expect someday (maybe when c89 is 40 years obsolete not just 30 years obsolete?) that GSFC will refresh their internal standards add some sort of clause to the ISO C requirement to acknowledge the existence of newer ISO C revisions and allow their use, at which point we will be compliant.

C99 includes some important code safety items, which is why we (internally) justified using it instead of the older C89. Organizations which still insist on the obsolete c89 are doing themselves a major disservice.

However, in no event would I expect any coding standard document to permit use of compiler-specific extensions, such as gnu99. A newer revision of ISO C is a very different thing than a vendor compiler extension.

[skliper]

There is an internal code standard constraining ISO standard C that we aren't compliant with because we use c99 (it requires c89). Not clear to me why we don't just change the host to gnu99, which is at least a standard, vs custom re-implementations of the same functions. I'd change that one line for free.

Is this in the branch standard?

Yup. Note that's the flight software standard, doesn't say anything about ground software.

[skliper]

... However, in no event would I expect any coding standard document to permit use of compiler-specific extensions, such as gnu99. A newer revision of ISO C is a very different thing than a vendor compiler extension.

Well said (referring to the full comment). I'm still not convince it isn't fine to just use the gnu extensions for ground software. If you want to use a host compiler that doesn't support it feel free to custom implement. Not sure why we'd do that in the framework, seems like a waste.

[jphickey]

... However, in no event would I expect any coding standard document to permit use of compiler-specific extensions, such as gnu99. A newer revision of ISO C is a very different thing than a vendor compiler extension.

Well said (referring to the full comment). I'm still not convince it isn't fine to just use the gnu extensions for ground software. If you want to use a host compiler that doesn't support it feel free to custom implement. Not sure why we'd do that in the framework, seems like a waste.

Two reasons to not apply gnu99 here:

• We do certainly have some contingent of users that develop on MacOS, and -std=gnu99 might break them. This also potentially applies to anyone using the clang compiler instead of gcc.
• It doesn't even fix the compile problem on older toolchains, such as the version of gcc/glibc in RHEL/CentOS 6.x.

Specifying _BSD_SOURCE did fix it, but I've seen that actually create problems on other systems.

In the amount of time we've discussed it I probably could've implemented the #95 which would be plain ISO C and not rely on any of these extensions.

[astrogeco]

I wonder if the solution to all of this is to provide and support a single standard development environment using a VM or container solution.

[skliper]

I wonder if the solution to all of this is to provide and support a single standard development environment using a VM or container solution.

This is basically what we are doing with CI - Ubuntu 18.04 + scripted installs. Really depends on what the open source leadership wants to claim we support for building the ground tools.

[astrogeco]

I wonder if the solution to all of this is to provide and support a single standard development environment using a VM or container solution.

This is basically what we are doing with CI - Ubuntu 18.04 + scripted installs. Really depends on what the open source leadership wants to claim we support for building the ground tools.

Kind of but not exactly, I'll consider that done once we are able to run the same environment on a local machine as on CI