Skip to content
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

SB Only Increments Message Sequence Count Where There are Subscribers #101

Closed
skliper opened this issue Sep 30, 2019 · 16 comments · Fixed by #490 or #419
Closed

SB Only Increments Message Sequence Count Where There are Subscribers #101

skliper opened this issue Sep 30, 2019 · 16 comments · Fixed by #490 or #419
Assignees
Labels
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

In the current implementation of the Software Bus, sequence counter management is handled in the routing table. The routing table only includes entries that have subscribed messages. This results in the sequence counter being incremented only if there are subscribers to a message. If a message is unsubscribed to and then resubscribed, the sequence counter will be reset to zero.

A message sequence count should reflect the production of the message vs. the receipt of a message. One use case for this implementation is to support the filtering of a message via the sequence count.

APL/SPP project implemented a solution to increment the sequence count upon production of the message regardless of whether or not there are any subscribers. The solution involved removing the sequence count table from the routing table. This exact solution may not support the new/expanded APID/message ID name space. Expanding the message ID name space will need to be considered when implementing the overall solution to increment the sequence count to reflect message production.

@skliper skliper added this to the 6.8.0 milestone Sep 30, 2019
@skliper skliper self-assigned this Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Imported from trac issue 70. Created by sstrege on 2015-06-17T14:30:51, last modified: 2019-07-08T14:26:30

@skliper skliper added the bug label Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by sstrege on 2016-08-26 13:55:09:

This is a feature of the software bus that most missions/projects have been living with unaffected. This ticket has been deferred as it is not a high priority to change the software bus implementation on how sequence counters are incremented.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2016-10-18 15:07:03:

Remove my name from many tickets (where it was applied by default), so others can tell that there are tickets that need some attention.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by cdknight on 2018-06-19 14:18:17:

I'd like to take a crack at this. I am thinking that the pipe should have a "receive count" and a sequence count generated on the "send" side.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by cdknight on 2018-10-22 14:58:42:

See [changeset:9a84396], this can be accomplished simply by allowing routes to have zero destinations. This has the benefit of only tracking sequence counts for ID's that are or were subscribed to and all new ID's start at seq id 0.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by cdknight on 2018-10-23 13:32:24:

Need to consider IsValidRouteIdx logic, events should be sent when no subs.

Should evaluate whether there's a unit test for no subs event.

SB 4305.5

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2019-02-21 16:07:26:

Looking at the old CCB notes from 2018-10-23, it looks like this change set was CCB approved but never merged.

I did a squash merge onto branch ic-jph-20190221 (to clean the history a bit) and the complete changeset is in commit [changeset:c0a3246a].

HOWEVER: this seems to trigger a failure in the SB unit test, as follows:

{{{
[ INFO] ut_stubs.c:223:Unexpected return in maximum message ID count test, i=255, exp=0x0, act=0xca000009

[ FAIL] 09.008 sb_UT.c:4621 - Test_Subscribe_API - Maximum message ID count test
}}}

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-02-25 16:26:54:

Needs unit test update

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by cdknight on 2019-02-26 15:28:35:

I realize now that there's a (bad?) side-effect of this new logic--previously when a route no longer had destinations, it was removed from the routing table. Now, even with zero destinations, the routing table entry remains--this could result in the routing table filling up with destination-less routes.

Unluckily, the routing table is also not cleared between tests (should it be?) so while my unit test did not catch this, a later test was affected by the fact that the routing table now had one entry in it already.

Options:

  1. document and don't worry about it, it's a rare case this would arise.

  2. make a compile-time option as to whether to retain destination-free routes.

  3. add a separate function to remove a destination-free route (or a function to remove all destination-free routes in the table).

  4. re-use the "oldest" routing table entry with no destinations.

  5. back out the logic and consider another tact.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-03-01 07:47:12:

CCB - review options and provide direction. Email chain indicates this was seen on JHU/APL version of to/ci that unsubscribes/resubscribes on a table change (not how GSFC does it). Seems to fall in the "different functionality" bin with associated trade-offs (bigger footprint, possibly slightly more efficient). Recommend a document update for current behavior description, and the possible alternative approach that APL used.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-04-24 12:28:02:

CCB discussion - Should follow CCSDS (resets should not happen), mark as bug

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-04-25 14:13:49:

CCB/community discussion needed on path forward options, see [comment:12 Comment 12]. CCB Agreed on 4/24/19 the counter resets don't comply with the CCSDS standard.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jwilmot on 2019-05-01 13:33:02:

The counter should increment on send as intended and per standard. It will have a performance impact on send with no subscribers but should have low impact to normal sends. APL’s implementation to remove the counter from the routing table and add a separate counter table sounds good and would decouple the routing from the sequence counter. The sequence counter table would be relatively small, (CFE_PLATFORM_SB_MAX_MSG_IDS size, default=256) and would be indexed by MsgKey (same as index into routing table). Also remove the logic to reset the counter on subscription. The extended msgID is currently handled up to that same limit. Large missions may need to change that and the CFE_SB_MsgKeyToValue logic.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-05-01 14:02:59:

Moved back to assigned to work Jonathan's suggestion. Feel free to move back to review if CCB discussion is needed, or mark for redispatch if an owner change is desired.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-07-03 12:58:32:

Moved unfinished 6.7.0 tickets to next release

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-07-08 14:25:14:

Moved ic-jph-20190221 to trac-70-seqcnt-squash [changeset:c0a3246a] and removed out of date trac-70-seqcnt [changeset:9a84396]. Still needs actions based on comment:19.

@skliper skliper assigned CDKnightNASA and unassigned skliper Sep 30, 2019
@skliper skliper removed this from the 6.8.0 milestone Nov 4, 2019
CDKnightNASA added a commit to CDKnightNASA/cFE that referenced this issue Nov 27, 2019
…ipes will never receive an "unsubscribe" notification.
skliper pushed a commit to CDKnightNASA/cFE that referenced this issue Jan 14, 2020
Maintains a sequential counter on SB messages
even when there is no subscribers.

Behavior changes:
Routes will never be deleted and pipes will never
receive an "unsubscribe" notification
CDKnightNASA added a commit to CDKnightNASA/cFE that referenced this issue Jan 18, 2020
skliper pushed a commit to CDKnightNASA/cFE that referenced this issue Jan 21, 2020
Adds logic and associated unit tests
Removes no-longer-used EID
skliper added a commit that referenced this issue Jan 21, 2020
Fix #101, Always increment sequence counter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants