CCSDS Electronic Data Sheet (EDS) integration

[skliper]

Integrate Consultative Committee for Spacecraft Data Systems (CCSDS) Electronic Data Sheets (EDS) into the cFS tool chain and code base. An EDS is a formal machine readable (XML) specification of interfaces for both hardware and software components. The primary use cases are to support:
a) Exchange of unambiguous interface definitions in a standard format between organizations (As CCSDS is an international standards body, organizations includes other international agencies, and industry partners)
b) Automatic generation of interface code, system models, system tests, and mission operational databases
c) Run-time support for Plug-and-Play systems

There are two CCSDS standards documents that specify the schema and terms to be used in the data sheets for cFS

1. XML Specification for Electronic Data Sheets for Onboard Devices and Software Components (CCSDS Blue Book 876.0)
2. Specification for Dictionary of Terms for Electronic Data Sheets for Onboard Components (CCSDS Magenta Book 876.1)

This ticket is intended to address EDS use at the cFS message layer (Software Bus). Another major use is at the device layer (hardware component) which is the target of major studies at the European Space Agency, and the China Academy of Space Technology. The device layer use case will be a future cFS ticket.

[skliper]

Imported from trac issue 176. Created by jwilmot on 2016-10-24T14:37:39, last modified: 2019-03-05T14:58:28

[skliper]

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

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

[skliper]

Trac comment by jphickey on 2017-07-10 17:08:29:

As a prerequisite to using any sort of generated header files or source code, it is highly beneficial to adopt a consistent naming convention for all identifiers. The GHAPS project did some work in this area and documented the recommended conventions.

Note this is only concerning identifier names and not anything specifically related to EDS, it is just being proposed to establish a consistent practice which will have benefits going forward (with or without EDS).

The document is in commit [changeset:c281e47] (markdown format)

I will also also attach a PDF copy of the document to this ticket for review.

[skliper]

Trac comment by jphickey on 2017-07-16 23:36:52:

commit [changeset:32cba48] illustrates the enumerator naming convention applied to 6 select enums in CFE ES and CFE EVS.

These 6 specific enums were chosen because they are part of the external messaging API. There are plenty of additional macros which are only used internally that are left as-is for now, but these would be good candidates to fix as well.

The existing #define statements are removed and replaced with an enum. The enums are defined in a new file called CFE_ES_typedefs.h and CFE_EVS_typedefs.h respectively. The idea here is to completely separate the types that are used for external messaging from the types and definitions that are for internal use only (existing headers are something of a mixture).

[skliper]

Trac comment by jphickey on 2017-07-24 10:46:52:

Furthermore - there are commands such as DeleteCDS in CFE_ES. Converting this to all uppercase based on a simple CamelCase rule produces DELETE_C_D_S.

The rule could be extended to handle multiple uppercase chars in a row and treat that as one word, but again that is not a reversible operation. This is another example of why it is generally better to avoid conversions and just preserve names as is where possible.

[skliper]

Trac comment by jphickey on 2017-07-24 10:27:33:

FYI the name convention for the #define macros for command codes might warrant a bit more discussion and maybe another section in the document. These currently end in a _CC suffix, for instance:

{{{
#define CFE_EVS_NO_OPERATION_CC 0
}}}

Based on the suggested naming convention this should be changed to "Noop". The question is with respect to capitalization -- should it be made all uppercase as in CFE_EVS_NOOP_CC or should we preserve case and call it CFE_EVS_Noop_CC

I tend to prefer the case-preserved approach as it is easier particularly for multi-word command names. For instance CFE_ES has a "QueryOne" command, which would translate to either CFE_ES_QueryOne_CC or CFE_ES_QUERY_ONE_CC.

My preference would be the former i.e. CFE_ES_QueryOne_CC as this minimizes name changes between the actual command name (QueryOne) and the symbol, and the logic to do this in a script is very simple - just add the prefix and suffix. The logic to convert to all uppercase is of course possible as well, but it requires separating into words based on uppercase chars and adding extra underscores, and it is harder to go back to the original name again, particularly if the command name actually has an underscore in it.

[skliper]

Trac comment by jphickey on 2017-07-24 16:49:14:

Commit [changeset:8728ebf] illustrates how this rule would be applied to the CC macros for ES and EVS.

[skliper]

Trac comment by sstrege on 2017-08-07 13:13:12:

On behalf of stashakk who writes:

-----Original Message-----
From: Tashakkor, Scott B (MSFC-ES52)
Sent: Thursday, July 27, 2017 8:50 AM
To: Strege, Susanne L. (GSFC-5820) [email protected]
Subject: Re: cFS CCB Meeting Minutes - 7-25-17

After reading through the meeting notes and looking at the ticket. I DO AGREE. The purpose and rationale are sound to why.

As for the specifics, that to me is more personal preference (because it is more stylistic and controlling a style is hard because everyone will do it differently). So my personal opinion/thoughts (which I know others will differ):

I have never liked the use of macros. They overly complicate code and very wrong things can be done with them, the language has the functionality already, so I feel that we should use it. I understand the purpose of going to a naming convention, but sometimes shortening things will always have conflicts, for instance with "msg" that has multiple contexts, or even things like ntp (network time protocol or nuclear thermal propulsion). So at some point collisions will happen with shortening. My biggest thing would be to keep a centralized list (in code or a document like cFS_IdentifierNamingConvention that continually keeps track of this information.

[skliper]

Trac comment by jphickey on 2017-08-22 11:24:09:

Updated commit available for review in [changeset:8ae2f8d]

** Note this is all-inclusive and replaces all previous changesets on this ticket ** (except for the proposal document itself)

This change set focuses on updating all enumeration and command names according the proposal, across all core applications. As a result it is a fairly large changeset but the changes should be all straightforward and mechanical in nature, effectively the result of doing a global search and replace.

Compatibility macros are included to preserve existing application code that may depend on the old symbol names.

[skliper]

Trac comment by jphickey on 2017-08-22 11:29:29:

Also pushed a branch ic-2017-08-22 which merges this and the #230 changeset, to get bamboo build and test results

[skliper]

Trac comment by jphickey on 2017-08-22 16:39:03:

Per CCB review on 2017-08-22: The "ic-2017-08-22" branch on bamboo was modified to include additional CFS applications.

The results are viewable at this link:
https://babelfish.arc.nasa.gov/bamboo/browse/CFS-CFSCFE106/latest

The net result is that this specific change set did not break any apps ... BUT,

• This creates additional cppcheck failures which needed to be quarantined - some of which are potentially significant. Probably worth reviewing those and fixing them.
• Some apps are incorrectly relying on implicit inclusion of cfe_platform_cfg.h
• Some app source files have encoding issues or bad UTF-8 chars that cause other tools like "meld" to fail when attempting to read these files. This may be a bug in the other tool, but we should at least standardize the encoding in our source files (recommend plain 7-bit ascii)

[skliper]

Trac comment by jphickey on 2017-08-22 16:43:55:

Additional notes on [changeset:8ae2f8d]:

In the new "extern_typedefs.h" file the content is conditional on:
#ifdef CFE_EDS_ENABLED_BUILD

If that is defined then the EDS tool generated file will be pulled in, which entirely replaces the content of this header file.

I am proposing adding this to the 6.6 release for two reasons:

1. it future-proofs the header a bit more
2. it gives a clue to anyone editing this file or adding something to it, that this file is not a purely normal header file, and that it may be sourced directly from EDS in a future version.

Ideally for 6.6 release I think we should include the EDS XML files in the source tree even if they are not directly used. After all this is part of the reason for adding the extern_typedefs.h to begin with -- to have a direct 1:1 map between an EDS XML file and a header file that has the C version of the same content. When you actually see the C header file side by side with the corresponding XML file things make a lot more sense.

[skliper]

Trac comment by jphickey on 2017-09-01 00:31:46:

FYI - commit [changeset:fc652ab] contains the proposed EDS XML files for inclusion in CFE 6.6

In this commit is only new files, it does not change any existing files or build scripts. This is simply to include the proposed SOIS EDS XML files with the CFE core.

They should describe basically what the FSW is sending/receiving from external entities.

[skliper]

Trac comment by sstrege on 2017-09-05 12:22:36:

Approve commit fc652ab

[skliper]

Trac comment by jphickey on 2017-09-07 10:32:57:

Additional commits are ready for CCB review:

1. commit [changeset:d124688] branch trac-176-public-cfe-time-copy-macro

   This is a simple change that moves the CFE_TIME_Copy macro into a public header rather than hiding it inside cfe_time_utils.h which only TIME internal source can get to. It is useful for other entities that need to efficiently copy times between structures which could potentially be of a different type.

2. commit [changeset:b54a2b7] branch trac-176-sizeof-operator-instances

   This is an extension of the naming convention that has already been approved and started in previous commits. It is intended to "future proof" the code a little better for type name changes where they are warranted. As discussed previously in the CCB it is often preferable to use an instance rather than a type name as the subject for sizeof(). If/when the type of an object changes (regardless of whether the object actually changes type or if is just a name change) using an instance will "just work" whereas code referencing a specific type name will break, and in not-so-obvious ways and it may not even trigger a compiler error or warning.

   In fact when doing this change, there were a couple instance in UT where the wrong object had been used, and this is very obvious when using the instance name but not obvious at all when using the type name.

[skliper]

Trac comment by jphickey on 2017-09-07 14:35:28:

Message dispatch consistency commits that are ready for CCB review:

The objective of this set of commits is to make "message dispatch" operations more consistent across all CFE core applications. The pattern rules suggested are as follows:

1. Every interface message defined in the external interface (be it EDS or otherwise) should have a separate dedicated handler function. This includes no-op and send housekeeping commands.

2. The message handler functions should all have a similar prototype, which is:

{{{
int32 MessageHandler(const MessageType_t *data);
}}}

Where MessageHandler is a function name based on the command name, and MessageType_t is a type name also based on the command name that represents the full message to be handled.

The data argument should be a pointer to the base of the message. This is an area where there was significant inconsistency -- some applications passed a pointer to the payload, whereas others passed the base pointer.

The return type is an int32, ideally using the standard CFE return codes where CFE_SUCCESS means that the message was handled. This was also inconsistent across applications, where EVS as an example had returned boolean where TRUE meant the message was handled and FALSE meant that it was an error. Also note that TBL had an existing dispatch paradigm with its own return code here, so it is still a bit of an oddball, but it can still use the same basic prototype.

Each core application has been updated separately, so they can be code reviewed independently:

ES: Commit [changeset:423eadb] branch trac-176-es-message-dispatcher-pattern
EVS: Commit [changeset:a87adfa] branch trac-176-evs-message-dispatcher-pattern
SB: Commit [changeset:80304dd] branch trac-176-sb-message-dispatcher-pattern
TBL: Commit [changeset:0d8cf4f] branch trac-176-tbl-message-dispatcher-pattern
TIME: Commit [changeset:cf98441] branch trac-176-time-message-dispatcher-pattern

NOTE: aside from changing the prototype and names in the code and modifying where certain checks are done, this tries hard to maintain the //current behavior// as much as possible. In particular, only ES and EVS actually verified the length of incoming messages at all. This is not changed by these commits.

[skliper]

Trac comment by jphickey on 2017-09-07 14:57:42:

A few additional important notes on the message dispatcher changes above

1. These change a few type names, so it depends on having the sizeof() use instance names rather than type names (one of the earlier commits). Without this, code will break due to changed type names, but as long as this commit is merged first, all is well.

2. The diffs are not as big as they look, particularly in cases where indentation had to be adjusted due to moving an "if" statement or similar. Due to this I recommend viewing the diffs with the "ignore whitespace" option enabled, as this produces a diff which is //much// more understandable and clearly shows what has changed (often very little).

[skliper]

Trac comment by glimes on 2017-09-12 11:22:17:

Commit [changeset:fc652ab] now included in ic-2017-09-12 and in test,
leaving ticket state unchanged to track additional work.

( I'd change it to "work in progress" but that would change ownership
from Joe to Me -- better to leave it in his name! Wish we had thought
about that when we set up the Action list ;)

[skliper]

Trac comment by jphickey on 2017-09-13 14:08:50:

Setting it to in_work as suggested.

I also went ahead and rebased this changeset to the latest development branch per above. Commit IDs in the previous comments updated accordingly.

[skliper]

Trac comment by jphickey on 2017-09-14 17:23:58:

One more naming convention update....

Commit [changeset:1ed4c8d] branch trac-176-fix-struct-names

This updates the names of the Telemetry data structures to be consistent and more in line with the naming convention, and ensures consistency between the code and EDS.

For instance, the housekeeping telemetry packet may have been Hk_Packet (SB), HkPacket (ES), TlmPkt (EVS). These are all now HousekeepingTlm. Ditto for all other telemetry message definitions.

This is likely the last change set relating to naming convention issues, unless we decide to fix the instance names as well (this only addresses type names).

[skliper]

Trac comment by jphickey on 2017-09-15 10:12:36:

Review note:

1. all the above commits are merged into a branch ic-2017-09-13 which is built by bamboo and the results are here: https://babelfish.arc.nasa.gov/bamboo/browse/CFS-CFSCFE111. Build 10 (as of this writing) is the latest/current build.

2. the previous bamboo build (9) pointed out a warning in the EVS changeset, which I fixed. The commit id's above all reflect the latest/current version which matches build 10 (the warning is indeed fixed in build 10).

[skliper]

Trac comment by glimes on 2017-09-15 12:08:12:

Joe -- build 10 (of commit [changeset:b684218]) came up fairly clean,
but build 11 (of commit [changeset:56430ca]) had compilation errors:

{{{
cfe/fsw/cfe-core/src/inc/cfe_es_msg.h:1596: error: redefinition of typedef 'CFE_ES_OneAppTlm_t'
cfe/fsw/cfe-core/src/inc/cfe_time_msg.h:1144: error: redefinition of typedef 'CFE_TIME_ToneDataCmd_t'
}}}

Missing {{{#ifndef}}} guards somewhere?

[skliper]

Trac comment by jphickey on 2017-09-15 12:28:54:

Replying to [comment:22 glimes]:

Joe -- build 10 (of commit [changeset:b684218]) came up fairly clean,
but build 11 (of commit [changeset:56430ca]) had compilation errors:

Thanks for the heads up. It looks like this was related to the final commit that did the struct name fixes, where it was adding compatibility typedefs. Basically, 2 cases where the name ended up NOT changing, so the typedef ended up as a duplicate. For some reason, my compiler doesn't care about typedef'ing something to the same name, but whatever compiler bamboo uses does.

I pushed a revised commit ([changeset:1ed4c8d]) that removes the duplicates. Build 12 looks better.

[skliper]

Trac comment by glimes on 2017-09-15 12:33:51:

Reviewing the component commits of the merges. By the way, thanks for organizing
this in bite-sized chunks, it makes it easier to focus on individual intented
changes, then later to focus on any potential interactions.

I'm up to one of the earlier merges, and time is moving along, so I'll stash
my review up to this point (and will continue reviewing).

Commit [changeset:d124688] {{{Trac #207: Make CFE_TIME_Copy macro public}}}

recommend accept.

Commit [changeset:b54a2b7] {{{Trac #207: prefer sizeof(instance) rather than type}}}

recommend accept.

This change is beneficial, but not without risk; for example:

{{{
CFE_PSP_MemSet(CFE_EVS_GlobalData.EVS_LogPtr->LogEntry, 0,

•              CFE_PLATFORM_EVS_LOG_MAX * sizeof(CFE_EVS_Packet_t));
  

•              sizeof(CFE_EVS_GlobalData.EVS_LogPtr->LogEntry));
  

}}}

While this removes the redundant knowledge of the declaration
of LogEntry (reducing risk associated with ANY change to the
data type), it adds the risk of doing the wrong thing if LogEntry
changes from an array to a pointer.

Thought for later: is there a way to write an {{{assert}}} that correctly
differentiates between an array and a pointer, which works when the array
happens to have the same size as a pointer?

[skliper]

Trac comment by glimes on 2017-09-15 12:50:56:

The message dispatch changes look bulky but are actually not complicated ... I still need to scan them (due dilligence) but expect that my only meaningful comment will be the one just below.

Commit [changeset:80304dd] {{{Trac #207: Update SB message dispatch pattern}}}

recommend accept.

Note that command handling functions (like {{{CFE_SB_SendHKTlmCmd}}} ) that do not have parameters, and thus do not reference the incoming message, will corretly trigger static analysis warnings, as failure to reference a parameter can indicate problems.

It might be prudent to inject this line into these functions, to document to maintainers, compilers, and static analysis that this is intentional:

{{{
(void) data; /* this command has no parameters. */
}}}

[skliper]

Trac comment by jphickey on 2017-09-22 10:41:39:

REBASE

The commits above have been rebased to the baseline that was approved at the 2017-09-15 CCB meeting. This is the updated list of commits that are still awaiting CCB review and approval:

commit [changeset:88bb580] branch trac-176-es-message-dispatcher-pattern
commit [changeset:b8168db] branch trac-176-evs-message-dispatcher-pattern
commit [changeset:7968598] branch trac-176-time-message-dispatcher-pattern
commit [changeset:5718e96] branch trac-176-tbl-message-dispatcher-pattern
commit [changeset:55dfb35] branch trac-176-sb-message-dispatcher-pattern
commit [changeset:aee56d1] branch trac-176-fix-struct-names

[skliper]

Trac comment by jphickey on 2017-09-22 16:22:59:

REVISED AGAIN....

Per CCB discussion held on 2017-09-22, additional changes were made to clean up the status code situation. Instead of introducing essentially duplicate status codes for each service, a generic
set of status codes is introduced. The message dispatcher change sets are all revised to use the generic status codes where applicable.

The set of commits as it stands now:
Status Codes: commit [changeset:69edaa0] branch trac-176-common-status-codes
ES: commit [changeset:4737d6a] branch trac-176-es-message-dispatcher-pattern
EVS: commit [changeset:3244d92] branch trac-176-evs-message-dispatcher-pattern
TIME: commit [changeset:478ffdb] branch trac-176-time-message-dispatcher-pattern
TBL: commit [changeset:b91baff] branch trac-176-tbl-message-dispatcher-pattern
SB: commit [changeset:0f46da2] branch trac-176-sb-message-dispatcher-pattern

All the above commits should be "good to go" based on the agreement that it is ready for merge after making the status code changes.

Finally the last commit which is still outstanding:
commit [changeset:27cc676] branch trac-176-fix-struct-names

[skliper]

Trac comment by glimes on 2017-09-25 17:03:40:

Most recent round of reviewed changes has been integrated,
up to but not including the change Joe just made ready for review.

[skliper]

Trac comment by sstrege on 2017-09-26 11:05:08:

Recommend/approve commit 27cc676

[skliper]

Trac comment by glimes on 2017-09-26 12:27:37:

recommend approve for [changeset:27cc676] in general, but we need to
resolve the following problems turned up by Bamboo when testing
the merge [changeset:88e078a] ...

{{{
cfe/fsw/cfe-core/unit-test/ut_bsp_stubs.c:225: error: conflicting types for 'CFE_PSP_WriteToCDS'
cfe/fsw/cfe-core/unit-test/ut_bsp_stubs.c:716: error: conflicting types for 'CFE_PSP_MemCpy'
}}}

[skliper]

Trac comment by jphickey on 2017-09-29 17:07:24:

While integrating the current development branch with the GHAPS EDS tool, another type issue was spotted. Note this is at least partly due to the stricter type checking that we are now implementing.

In the EVS subsystem, two commands shared a payload structure, which were technically the same binary format, but in reality had different labels for their single "enum" field. In the binary world these were both uint8 so it never caused an issue. However when actually type checking this, it became apparent that the two command payloads really needed to be separately defined.

Commit [changeset:d547d5c] branch trac-176-fixup-evs-mode-mismatch fixes this and makes it correct.

[skliper]

Trac comment by sstrege on 2017-10-03 12:05:05:

Recommend/approve commit [changeset:d547d5c]

[skliper]

Trac comment by glimes on 2017-10-03 12:37:34:

Recommend approval of [changeset:d547d5c]

[skliper]

Trac comment by jphickey on 2017-10-19 16:03:50:

Changing the milestone here to cfe-next.

All the stuff we agreed to for CFE 6.6 has been merged, but there are still more changesets to do before we can claim EDS integration. The remainder will be a target for the next CFE release.

[skliper]

Trac comment by jphickey on 2017-10-19 16:41:33:

Putting this pack on 6.6 for record keeping. This ticket will be closed, and I've opened #243 to track the continuing work for the future release.

[skliper]

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

Milestone renamed