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

Fix #50, Mark end of valid MsgId subscriptions #51

Merged

Conversation

skliper
Copy link
Contributor

@skliper skliper commented Jul 23, 2020

Describe the contribution
Fix #50

  • Added a TO_UNUSED entry at the end of the subscription list
  • Added break in subscription loop when TO_UNUSED found

Testing performed
Normal run, confirmed no 0x0 duplicate subscription message, confirmed nominal hk telemetry still works with GroundSystem

Expected behavior changes
No more subscriptions on the unused table slots (no MsgId 0 subscriptions).

System(s) tested on

  • Hardware: cFS Dev 3
  • OS: Ubuntu 18.04
  • Versions: bundle

Additional context
None

Third party code
None

Contributor Info - All information REQUIRED for consideration of pull request
Jacob Hageman - NASA/GSFC

@skliper skliper added bug Something isn't working CCB:FastTrack labels Jul 23, 2020
@skliper skliper self-assigned this Jul 23, 2020
@astrogeco
Copy link
Contributor

CCB 2020-07-29, Suggestion: Consolidate unused entries in table. Some potential issues with equality check using TO_UNUSED

@skliper
Copy link
Contributor Author

skliper commented Jul 29, 2020

Switch to use the compare msgid function.

@skliper
Copy link
Contributor Author

skliper commented Jul 29, 2020

Updated to use MsgId Equal.

@skliper skliper requested a review from jphickey July 29, 2020 17:07
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with it now using CFE_SB_MsgId_Equal()....

HOWEVER - one difference is that the previous implementation did allow "holes" in the list i.e. a valid entry, a TO_UNUSED entry, another valid entry, etc. This could have been used for "reserved" entries that could be turned on/off based on whether an app was loaded, without changing the positions of other entries.

This will now stop at the first TO_UNUSED entry - so holes/reserved slots are no longer possible.

As long as folks are OK with that, I'm fine with it -- I don't see why someone would need to leave gaps/holes, just that it is a change worth noting.

@skliper
Copy link
Contributor Author

skliper commented Jul 30, 2020

Good note, and no holes approach is fine by me.

@astrogeco astrogeco changed the base branch from master to main August 4, 2020 17:38
@astrogeco astrogeco changed the base branch from main to integration-candidate August 4, 2020 17:38
@astrogeco astrogeco merged commit 4d56060 into nasa:integration-candidate Aug 4, 2020
@skliper skliper added this to the 2.4.0 milestone Aug 21, 2020
@skliper skliper deleted the fix50-msgid0-subscribe branch February 1, 2021 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CCB:FastTrack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subscribes to MsgId 0x0 256-actual_msgids times
3 participants