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

Move SB route lookup (including insert method) to a module #928

Closed
skliper opened this issue Sep 30, 2020 · 6 comments · Fixed by #947 or #975
Closed

Move SB route lookup (including insert method) to a module #928

skliper opened this issue Sep 30, 2020 · 6 comments · Fixed by #947 or #975
Assignees
Labels
enhancement Priority: Mission Feature or bug related to stakeholder needs
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2020

Is your feature request related to a problem? Please describe.
Various implementations possible for message and route tables, all with associated advantages and issues.

Describe the solution you'd like
Provide the capability to replace the core implementation.

Describe alternatives you've considered
None.

Additional context
For implementations that support large MsgIds, or prefer smaller memory footprint at the cost of performance. Hashes, searches, etc.

Requester Info
Jacob Hageman - NASA/GSFC (per stakeholder request for alternate implementation)

@skliper skliper added enhancement Priority: Mission Feature or bug related to stakeholder needs labels Sep 30, 2020
@skliper skliper added this to the 7.0.0 milestone Sep 30, 2020
@skliper skliper self-assigned this Oct 5, 2020
@skliper
Copy link
Contributor Author

skliper commented Oct 5, 2020

Abstracting out MsgId/Route relation proposal - CFE_SB_GetRoutingTblPtr could use MsgId, with all the relation logic underneath the hood. CFE_SB_GetDestPtr could use route pointer. For the most part hide CFE_SB_GetRoutingTblIdx... shouldn't use direct access except when looping for reporting, and then could just loop the table. Also move all the related utility functions. Replace CFE_SB_RouteIdxPop_Unsync with a generic CFE_SB_AddRoute given MsgId, and CFE_SB_RouteIdxPush_Unsync is dead code for now (we no longer remove routes). CFE_SB_IsValidRouteIdx may actually go away (don't expose index directly).

cfe_sb_task.c

  • CFE_SB_EnableRouteCmd - get route, get dest
  • CFE_SB_DisableRouteCmd - get route, get dest
  • CFE_SB_SendRtgInfo - loop through routes, not keys
  • CFE_SB_SendMapInfo - loop through routes, not keys

cfe_sb_priv.c

  • CFE_SB_DuplicateSubscribeCheck - use route pointer (used in one place, could get route first)

cfe_sb_init.c

  • CFE_SB_InitMsgMap - TBD, at minimum move into module

cfe_sb_api.c - straight forward using same methods above, use MsgId to get route pointer..

Move the message map internal to the module... may pull some of the configuration options into the module (don't allow configuration of the message map size, it's based on supported message IDs).

EDIT - worth noting every one of these functions is internal to SB, but modularizing really makes them an interface...

@skliper
Copy link
Contributor Author

skliper commented Oct 5, 2020

Not planning on fixing with this change - PipeId is used as an index in multiple places, whitespace/alignment is tragic, excessive comments (in my opinion). Likely spawn follow-on PRs to clean up.

EDIT - also violates declare variables at the start of the function, fixing some where I see them but not a careful scrub

@jphickey
Copy link
Contributor

jphickey commented Oct 5, 2020

Old issue #100 is for making Pipe IDs abstract/safe (which must include fixing use as index, etc).

Might be worth considering for this cycle? Since I've converted so many other resources to use abstract IDs already, seems like a natural thing to include. Could be done after the current efforts are completed, if desired.

@skliper
Copy link
Contributor Author

skliper commented Oct 6, 2020

Could be done after the current efforts are completed, if desired.

Low priority but likely possible, and a desirable change for consistency.

@astrogeco
Copy link
Contributor

astrogeco commented Oct 7, 2020

CCB 2020-10-06

Two implementation options:

  • Option 1: Hard option, doing it "the right way"

    • Redesign the interface using a new module
    • Abstract the relationship between routes and message IDs
  • Option 2: Easy option

    • Small fixes to the current design if we don't make it "interface safe"
    • Problems with maintenance if the routing table changes

CCB votes for the structured API method (Option 1)
@skliper can you double check the accuracy of these notes?

@skliper
Copy link
Contributor Author

skliper commented Oct 7, 2020

I think the key to option 1 is to keep direct access to the resource (routing table in this case) on one side of the interface. Option 2 was allowing direct access on both sides in the short term, since that's how it's done currently since today routing is an internal capability of SB. Making routing a module with various possible implementations benefits from a more defined interface between routing and SB, which is option 1. Even doing it the right way shouldn't be that much more work, I'm just lazy enough to want confirmation that the harder option is worth it from the CCB/open source point of view.

skliper added a commit to skliper/cFE that referenced this issue Oct 21, 2020
- Removed all index/pointer accesses of message map and routing table
- All references now via IDs and APIs
- Note SB still owns destination logic (unchanged linear linked list)
- Limited whitespace fixes for readability
- Resolved observed instances of variables not declared at the
  start of functions
- Cleaned comments
- Resolved potential double locks in CFE_SB_SendPrevSubs
- Route and message write to file no longer guaranteed in msgid
  order to maintain performance for large msgid space implementations
- Removed unused CFE_SB_FindGlobalMsgIdCnt
- Clarified CFE_PLATFORM_SB_MAX_MSG_IDS config param description
- Eliminated potential race in CFE_SB_PipeDeleteFull
- Individual destination removal debug events no longer reported
  during a CFE_SB_PipeDeleteFull
skliper added a commit to skliper/cFE that referenced this issue Oct 21, 2020
- Implementation for direct map and unsorted routing table
- Includes full coverage tests
- Removed msg key and route stack concepts from direct map
skliper added a commit to skliper/cFE that referenced this issue Oct 21, 2020
- Linking SBR with SB unit test, not stubbed
- Confirms matching functionality (with updates for
  intended changes)
skliper added a commit to skliper/cFE that referenced this issue Oct 21, 2020
- Implementation for direct map and unsorted routing table
- Includes full coverage tests
- Removed msg key and route stack concepts from direct map
skliper added a commit to skliper/cFE that referenced this issue Oct 21, 2020
- Linking SBR with SB unit test, not stubbed
- Confirms matching functionality (with updates for
  intended changes)
astrogeco added a commit that referenced this issue Oct 21, 2020
Fix #928 and #929 - Modularize software bus routing, add msg map hash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Priority: Mission Feature or bug related to stakeholder needs
Projects
None yet
3 participants