search

nasa / cFE

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

Suspicious implementation of SHORT_FORMAT mode in EVS_SendPacket() #95

Closed skliper closed 4 years ago

skliper commented 4 years ago

This code sequence occurs within the {{{EVS_SendPacket()}}} function:

{{{ / (LSW) Is the intent to write the event text to the log but not the SB msg ??? /

if (CFE_EVS_GlobalData.EVS_TlmPkt.Payload.MessageFormatMode == CFE_EVS_SHORT_FORMAT) { / Send an empty message if short format is enabled / EVS_PktPtr->Payload.Message[0] = '\0';

/ (LSW) This is pointless -- why bother to send a buffer with an empty string ??? /

} }}}

It appears that someone (LSW?) already noticed the strangeness here some time prior to the 6.4.1 release.

The intent here may have been to send only the Event ID and omit the actual string, since most event ID's have fixed strings with them.

However, the length of the actual packet (in the CCSDS header) is never adjusted, so although the first character of the message payload is overwritten with a NUL, the full message payload is still sent on the software bus so there is absolutely no benefit to doing this in the current form.

skliper commented 4 years ago

Imported from trac issue 64. Created by jphickey on 2015-06-04T12:54:04, last modified: 2019-03-05T14:58:28

skliper commented 4 years ago

Trac comment by jphickey on 2015-06-04 12:54:27:

Candidate for CCB discussion

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 sstrege on 2017-10-04 18:57:35:

Commit [changeset:0a4ff97] (branch: sls-ic-trac64-merge) is ready for CCB review

skliper commented 4 years ago

Trac comment by cdknight on 2017-10-05 10:48:56:

recommend approve

skliper commented 4 years ago

Trac comment by jphickey on 2017-10-05 11:43:54:

My concerns are:

A nitpick: I think overwriting the first char of the message with a null is unnecessary (line 366-367) and I'd rather see this taken out.

Bigger issue: how will ground systems and/or other subscribers handle this size change?

Currently there is no precedent (as far as I know) for dynamically-sized telemetry messages. Basically, a MsgId is associated with a certain C structure which defines its contents, and the size is either right or wrong. (of course for commands it is the combination of MsgId and Command Code - but this still maps to a certain C data type).

So this would be an entirely new paradigm where the message length must be included in the decision making process, and there is more than one valid value for it, which is a major difference.

Here is my recommendation:

Using a different MsgId for short format avoids triggering indeterminate behavior in apps/tools/ground systems that haven't been updated to deal with short format events, and therefore better for backward compatibility.

For instance, if an app subscribes to the event message stream today, it is probably expecting that the "Message" portion of the contents will be valid. If we suddenly start sending shorter packets this will break existing code in unknown, possibly bad ways. But if it were a different MsgId then it will get no events at all, rather than getting events which do not match its expectation.

If the app/tool actually wants short format event messages then it could subscribe specifically to the short format stream, or even both streams to get either one (and that would assume the app/tool has been updated to deal with both possible formats).

skliper commented 4 years ago

Trac comment by glimes on 2017-10-05 11:57:04:

It is possible to work with messages within CFS whose final member is an array of characters; setting up the receiver to properly honor the end of the packet is a few easy lines of code.

Whether or not such messages are useful is a mission-level decision; and I am aware of one mission (AOS: a CFS-on-a-UAV project) where the sheer convenience of being able to fling around messages with variable length text strings at the end has been significantly beneficial.

It is certain that, if you define the message layout as being variable, that's just one more property of the data type on which senders and receivers have to agree.

(that said, I've not yet reviewed this change; I'm commenting specifically on precedent for dynamic message sizing. grain of salt and all that.)

skliper commented 4 years ago

Trac comment by jphickey on 2017-10-05 12:09:30:

I agree it is possible for subscribers to deal with the variable length field, and maybe even useful.

My main concern is that this hasn't been part of the contract in the past. So a separate MsgId would avoid backward compatibility woes with existing code that does not deal with variable length packets.

We would also need some protocol agreement for short frames i.e. if you are expecting a structure of size (X) and got a structure of size (X - 10), does this mean:

a) it is an entirely different frame with a different definition, or b) a truncated frame, where e.g. if you pad in a known value (e.g. nulls) you can reconstitute the original frame or at least something that is usable when cast to the expected definition.

Option (b) is viable, but it reduces the robustness of the size check. For instance you do not know if the frame is validly truncated or if it is entirely bogus. I am less comfortable with (a), as this just muddies the water too much.

skliper commented 4 years ago

Trac comment by sstrege on 2017-10-05 13:52:28:

I agree a protocol agreement needs to be defined for short frames. Ground systems need to know what to expect when EVS is configured to send short event messages. There is an existing requirement that specifies the protocol requirements for short frames:

cEVS3103.5 If the SB Format Mode is set to Short, the cFE shall generate an SB Event Message formatted as specified in the cFE User's Guide containing the spacecraft time, Processor ID, Application ID, Event ID, and Event Type.

I like the idea of having a separate MID and payload definition for short frames

skliper commented 4 years ago

Trac comment by jphickey on 2017-10-13 22:25:17:

Final version of this fix is ready for review: branch trac-64-complete commit [changeset:d49d4ef]

This cleans up the event generator code and consolidates the duplication between EVS_SendEvent, EVS_SendEventWithAppID, and EVS_SendTimedEvent into a common function, which replaces EVS_SendPacket.

The new function, called EVS_GenerateEventTelemetry() handles all of the necessary logging and message generation for events in a common way. It generates a long message for logging and record keeping purposes, and optionally a short message for the software bus if configured to do so.

skliper commented 4 years ago

Trac comment by cdknight on 2017-10-15 11:53:15:

recommend approve, looks good!

skliper commented 4 years ago

Trac comment by glimes on 2017-10-17 11:57:55:

recommend approve.

skliper commented 4 years ago

Trac comment by sstrege on 2017-10-17 12:28:58:

recommend approve

skliper commented 4 years ago

Trac comment by acudmore on 2017-10-20 12:14:49:

Nice clean up of code.. recommend approve

skliper commented 4 years ago

Trac comment by glimes on 2017-10-24 15:45:42:

commit [changeset:d49d4ef] integrated to development.

skliper commented 4 years ago

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

Milestone renamed