-
Notifications
You must be signed in to change notification settings - Fork 202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Suspicious implementation of SHORT_FORMAT mode in EVS_SendPacket() #95
Comments
Imported from trac issue 64. Created by jphickey on 2015-06-04T12:54:04, last modified: 2019-03-05T14:58:28 |
Trac comment by jphickey on 2015-06-04 12:54:27: Candidate for CCB discussion |
Trac comment by glimes on 2016-11-08 14:17:08: Current crop of cfe-next are all going into CFE 6.6 |
Trac comment by sstrege on 2017-10-04 18:57:35: Commit [changeset:0a4ff97] (branch: sls-ic-trac64-merge) is ready for CCB review |
Trac comment by cdknight on 2017-10-05 10:48:56: recommend approve |
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). |
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.) |
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 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. |
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 I like the idea of having a separate MID and payload definition for short frames |
Trac comment by jphickey on 2017-10-13 22:25:17: Final version of this fix is ready for review: branch 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 |
Trac comment by cdknight on 2017-10-15 11:53:15: recommend approve, looks good! |
Trac comment by glimes on 2017-10-17 11:57:55: recommend approve. |
Trac comment by sstrege on 2017-10-17 12:28:58: recommend approve |
Trac comment by acudmore on 2017-10-20 12:14:49: Nice clean up of code.. |
Trac comment by glimes on 2017-10-24 15:45:42: commit [changeset:d49d4ef] integrated to development. |
Trac comment by jhageman on 2019-03-05 14:58:28: Milestone renamed |
Added a second CPU to the build configuration to ensure the build system is functioning properly with Travis CI for multiple targets. Resolves: nasa#95
After adding the second CPU (by uncommenting CPU2 from targets.cmake), added in necessary scr and header files for cpu2. Combined, this should make cpu2 in the exe directory. This build configuration if successful will ensure the build system is functioning properly with Travis CI for multiple targets. Resolves: nasa#95
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.
The text was updated successfully, but these errors were encountered: