Fix #663, more consistent use of data types

[jphickey]

Describe the contribution
Define and Use data types more consistently across CFE.

Replace many uses of generic uint16/uint32 with a more purpose-specific typedef. Replace all sizes with size_t across the API.

Fixes #663

Testing performed
Build and sanity test CFE

Expected behavior changes
None, but does affect API. The API changes should be largely backward compatible, however.

System(s) tested on
Ubuntu 20.04, 64-bit

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

EDIT: Also fixed #843

[jphickey]

Pushed a draft/preview commit as discussed in the 2020-11-04 CCB. This has all the API changes but the unit test updates are not complete.

[jphickey]

General summary of what's being proposed:

• Use size_t for all in-memory object sizes, EXCEPT for:

  • CMD/TLM messages - where CFE_ES_MemOffset_Atom_t is used
  • Interface with PSP, where uint32 must be used until PSP API is updated (could do that too)

• Use sizeof() more consistently - particularly on strings on the stack in message handlers. It is more type correct (because value is a size_t, not int) and it adheres to the DRY principle - we want the actual size of the string, whatever it really is - don't assume it was declared to a certain size.

• Use ES-defined typedefs for Task Priority, Stack Pointer, etc.

• Anywhere a constant is used for these types, wrap it in a macro that converts the type, which makes the type conversion explicit when reading the code.

[jphickey]

Also note that this sweep also caught a bunch of issues in the old SB message API - which if its being deprecated, we can just ignore those. But we should decide whether the new MSG API should use size_t or keep its separate CFE_MSG_Size_t ... my thought is if we are going to use size_t, lets use it everywhere. The same justification for not putting a size typedef in CFE should apply to the module(s) (and vice versa).

[skliper]

...issues in the old SB message API

I also noticed a bunch of type issues (maybe some of the same?), fixed what I saw in #998. Mostly using uint16 for message size all over, which is a bug since it can't represent the full size CCSDS message. Agree that size_t could replace CFE_MSG_Size_t. The less one-off types the better in my mind. Maybe get #998 in first, then fix?

Could we use a more descriptive name than CFE_ES_MemOffset_Atom_t? Really it's a conversion instead of the atomic type, isn't it? I think of it as the native to tlm conversion for size_t...

Is the CDS offset type just for the PSP? If so I say we get that fixed also. It stands out as unique.

[skliper]

This will conflict with #998, but I like your change better (using size_t for the verify command length).

I would prefer separating the concept of the atomic type (which could be native) with the conversion to fit in telemetry. I'd also prefer the default behavior to use 64 bit's for memory addresses... if a user wants to use just 32 bit they can, but I'd rather default to consistent sized messages across 64/32 and valid data in both.

[jphickey]

...issues in the old SB message API

I also noticed a bunch of type issues (maybe some of the same?), fixed what I saw in #998. Mostly using uint16 for message size all over, which is a bug since it can't represent the full size CCSDS message. Agree that size_t could replace CFE_MSG_Size_t. The less one-off types the better in my mind. Maybe get #998 in first, then fix?

Yes, I think the best approach would be to embrace size_t everywhere, for all in-memory objects. There was also some random casting of sizes to uint16 here and there (EVS had one, I think - fixed in this PR). And yes, I concur with finishing up the pending changes first.

Could we use a more descriptive name than CFE_ES_MemOffset_Atom_t? Really it's a conversion instead of the atomic type, isn't it? I think of it as the native to tlm conversion for size_t...

Yes, perhaps we should bring up/revisit the whole "_Atom_t" suffix thing in general.... the genesis of the type suffixes was to facilitate automation/tool-chaining of these types - and use a different suffix for enums, strings, structures, etc.

It is currently documented in https://github.com/nasa/cFE/blob/main/docs/cFS_IdentifierNamingConvention.md#typedefs (unfortunately just noticed that github doesn't render the table data in this MD file correctly either -- another issue?).

At any rate, this is the pattern that I still try to follow for CMD/TLM types. I still think its a useful thing to have a different suffix, maybe "Atom" is confusing and we could do something else. It is intended to convey that it is a scalar, as opposed a compound type (struct/string/array) etc. Maybe "Scalar"?

Is the CDS offset type just for the PSP? If so I say we get that fixed also. It stands out as unique.

Not just CDS - all the memory blocks incl. reserved area and RAM disk are done with uint32 sizes - and these are especially problematic because they pass by pointer - not by value - so any change would NOT be backward compatible.

Because any change to this API would not be backward compatible - my recommendation is to rethink/redesign this API and make it more future proof, such as:

void * CFE_PSP_GetBlockPointer(CFE_PSP_BlockId_t BlockId);
size_t CFE_PSP_GetBlockSize(CFE_PSP_BlockId_t BlockId);

Then the PSP can also provide separate constants for BlockId corresponding to the ramdisk, reserved area, cds, etc. The important thing is we can add/extend these constants without breaking or changing the general API. The current calls have a separate dedicated call for each memory section. So making it more extensible we can more easily have separate sections for ES/EVS/TIME (I think there is another issue related to this)

[astrogeco]

Can we change the word "proposed" here? Feels like its incomplete...

[astrogeco]

CCB 2020-11-18 APPROVED

• Related and depends on Fix #635, Reduce use of uint32, add more OSAL typedefs osal#654
• Removes implicit type conversions
• Need separate discussion about _atom suffix nomenclature since it doesn't actually mean "atomic"
• Will hold a little less in CDS size for 64-bit platforms

[skliper]

Are all _Atom_t types fixed size (appropriate for cmd/tlm messages)? If so and we continue with this pattern, then I'm fine with it. In my mind it was just the "scalar" type, but without the fixed size restriction.