-
Notifications
You must be signed in to change notification settings - Fork 204
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
Fix #2362, Add source routing APIs to SB #2367
base: main
Are you sure you want to change the base?
Conversation
Still have some (known) coverage and test workflow issues to solve in here, but wanted to get this going for CCB review today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
4408c1c
to
9a81cb6
Compare
Add two new APIs that allow more control over the message routing. These allow the caller to directly specify the route (MsgId) and size of the content, and the message will be delivered based on the passed in values vs. the values inside the message itself. This restructures the existing/historical API calls to use the same underlying mechanism. Send/Receive actions have a transaction object associated and this tracks the status and events. Common event reporting is done based on this transaction object.
9a81cb6
to
aff55c3
Compare
I don't understand the driver behind adding complexity to SB to handle blobs of data that aren't self identifiable, this doesn't seem like a "natural" fit for pub/sub. Why not just wrap it with CCSDS if pub/sub is necessary, or implement point-to-point if it isn't? Simple tools for simple jobs vs trying to expand SB to do everything. Is this introducing a pattern of extending a core service beyond original intent, and if so what are the bounds? There's no longer embedded size, identification, common handling supported by binary blob messages, it's not clear why such an extended change makes sense in the open source context. Maybe just add a library that does wrapping/unwrapping for you and can be used by anyone who needs to send blobs, or some other way to avoid such major rewrites? My concern is now receivers need to know if it's a blob or not, which none of the standard receivers handle, so as soon as this is introduced there's an interoperability issue all over (not every subscribed MID can be checked for size or whatever). |
Yeah, arbitrary data blobs are not really the emphasis/driving need behind this. Its more about the routing flexibility. We already had the notion of the message itself as well as its "envelope" -- the The fact that this permitted the message to be a binary blob was a side benefit - if nothing else, proving that delivery of a random binary blob works ensures that the delivery is being done based only on its envelope information. This opens up a whole bunch of possibilities for future features:
If the concern has to do with the (possibility) of sending a binary blob and confusing a subscriber, we could conceivably restrict content to "Valid messages" even with this new API, and add some checks that the message attempting to be sent seems like an actual message. I think all we could do is confirm that the |
While I get you could do these things all within SB using the SB internal "envelope" concept, I don't agree with such a temporary nature or adding complexity to SB to do things that also could require changes to every sender/receiver. This concept seems to tie directly with SB, where it's dependent on these new APIs, only SB has the envelope information, it's gone as soon as delivered, etc. If the envelope is part of the message (wrapped) that dependency is eliminated, the CCSDS spacepacket header is already an envelope and you can layer as needed to add all the functionality above yet maintain independences from SB "envelope" magic. Wrapping w/ additional protocols (or encapsulating protocols in CCSDS space packets) has been common practice and eliminates the strict dependency on SB to get this all done. You could keep the SB/MSG independence and just update MSG to handle whatever envelope/wrapper information is needed and it can be customized/localized to the specific projects that need it. If folks really want this to work seamlessly across the common apps I just don't see how adding APIs and requiring special handling on the receiving side is the preferred approach. |
Checklist (Please check before submitting)
Describe the contribution
Add two new APIs that allow more control over the message routing. These allow the caller to directly specify the route (MsgId) and size of the content, and the message will be delivered based on the passed in values vs. the values inside the message itself.
This restructures the existing/historical API calls to use the same underlying mechanism. Send/Receive actions have a transaction object associated and this tracks the status and events. Common event reporting is done based on this transaction object.
Fixes #2362
Testing performed
Build and run CFE including functional test app
Build and run sanity tests
Expected behavior changes
Adds two new APIs. Existing APIs should be backward compatible, with some possible changes to the expected event IDs that might be generated under some off-nominal conditions.
System(s) tested on
Debian
Additional context
The new API allows the passed object to be any arbitrary data -- it does not need to be a CFE/CCSDS message at all. Therefore this could be more suitable for things like CFDP PDU packets, etc. as it would not need to be forced to look like a CMD/TLM message.
Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.