Fix #707, Resolve highest MsgID of 0xFFFF bug

[skliper]

Describe the contribution
Changes Message Key from uint16 to uint32 to avoid rollover and system hang
Fix #707
Fix #414

Testing performed
Steps taken to test the contribution:

1. Set CFE_PLATFORM_SB_HIGHEST_VALID_MSGID to 0xFFFF
2. Built (SIMULATION=native) and ran, confirmed startup
3. CI - https://travis-ci.com/github/skliper/cFS/builds/166553342

Expected behavior changes
Full message ID range available

System(s) tested on

• Hardware: cFS Dev 3
• OS: Ubuntu 18.04
• Versions: Bundle w/ this change

Additional context
Identified/resolved by JSC

Third party code
None

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

[skliper]

Ping @tngo67

[skliper]

@astrogeco requesting fast-track.

[CDKnightNASA]

What about just making it "int", as it's only used as an index into an array and would be more compiler optimization friendly?

[jphickey]

Concur that the MsgKey should always be at least the size of MsgId, bigger is OK but smaller is not. However it should also be documented that at least one MsgId value must be considered "reserved" or invalid, and hence it is not possible to use every conceivable MsgId value in your routing table. It is possible that we have another bug lurking in the routing table if we allow a valid route to be configured for CFE_SB_INVALID_MSG_ID

[skliper]

What about just making it "int", as it's only used as an index into an array and would be more compiler optimization friendly?

Minimum size for int is 16 bits. Could go unsigned long? Happy to go either way, I just went fixed size since it was fixed size. Looking at the code in general fixed sizes are used even for iterators... I checked our standard and it doesn't care either way.

[jphickey]

What about just making it "int", as it's only used as an index into an array and would be more compiler optimization friendly?

Minimum size for int is 16 bits. Could go unsigned long? Happy to go either way, I just went fixed size since it was fixed size. Looking at the code in general fixed sizes are used even for iterators... I checked our standard and it doesn't care either way.

I agree, in this case the fundamental issue is that MsgKey didn't have enough bits. The algorithm requires that the key has enough bits to hold at least (1 + CFE_PLATFORM_SB_HIGHEST_VALID_MSGID). This means we need to ensure it actually has 32 bits in it, if we want to support a highest msgid of 0xFFFF or greater.

I think in the original implementation, there was an assumption that if the lookup table grew beyond 2^16 entries, it would use some other more memory efficient algorithm like a hash table, rather than a direct lookup table. Even at 2^16 this is probably wasting a good chunk of memory.

[skliper]

Added note about the table being + 1.

[skliper]

@astrogeco please fast track this one

[astrogeco]

@astrogeco please fast track this one

Will merge right after CCB

Resolved