-
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
Remove CFE_SB_SendPrevSubs in favor of CFE_SBR_ForEachRouteId API #963
Comments
@CDKnightNASA - thoughts on this? can we get rid of |
Are the requirements/use cases still met? CFE_SB_SendPrevSubs was to inform SBN that subscriptions existed prior to SBN startup so that SBN could forward those to peers. SBN is started after cFE so subscriptions do already exist. The interface needs to be robust so that no subscriptions are lost and avoid any potential subscribe/unsubscribe race conditions. If CFE_SBR_ForEachRouteId does this then CFE_SB_SendPrevSubs is redundant and should be removed. |
@jwilmot The implementation (as I understand it) enables reporting (to avoid missing new subscriptions), then gets all prev subs which would work with either method. If done in the opposite order, subscriptions can be missed either using |
There was an unsubscribe "SBN_ProcessLocalUnsub" and "SBN_RemoveAllSubsFromPeer" in an older codebase. In the past, SBN would dynamically and reliably subscribe and unsubscribe to peers as informed by the local cFE SB. The idea was to have consistent state across all peers. Looks like something got lost over the years. The expected behavior is broken if there is no way for SBN to remove a subscription. |
I'm not saying SBN doesn't have the capability to unsubscribe, there's just no message from SB that says a message was unsubscribed. |
Is there a SB housekeeping CC/Tlm struct that contains current (as of the HK response) subscription information? I'd think the ground would want a way to query what subscriptions were active. I suggest SBN could use a HK command/response... |
Ah, reviewing the code, looks like the model is that if GND wanted to see what the routing table contained, they dump it to a file and downlink the file. :\ Theoretically, SBN could use that route, although it's a bit gross... The |
Maybe add an API to register a callback function for new subscriptions? This isn't targeted for Caelum, but seems like a good path forward to reduce complexity in the core. |
So is the ForEachRouteId() ready for primetime? Sounds like I should switch SBN and you'll mark the SendPRevSubs and ALLSUBS bus messages deprecated, yes? |
Yes, ForEachRouteId is good to go. |
Is your feature request related to a problem? Please describe.
CFE_SB_SendPrevSubs holds locks that aren't that useful, sufficient logic required, creates bus traffic, etc.
Describe the solution you'd like
SBN could just use CFE_SBR_ForEachRouteId, not really a true "public" API... but seems like fair use in this case. Would be outside of SB lock, but likely OK based on design (enables subscription reporting first, then check all previous subs).
Describe alternatives you've considered
At least remove all the locking/unlocking. It doesn't help.
Additional context
Related to #947 work.
Requester Info
Jacob Hageman - NASA/GSFC
Ping @CDKnightNASA
The text was updated successfully, but these errors were encountered: