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.

Sign up for GitHub

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

Jump to bottom

Fix #51, remove mid #59

Merged
merged 1 commit into from
Nov 3, 2022
Merged

Fix #51, remove mid #59

merged 1 commit into from
Nov 3, 2022

Conversation

chillfig
Copy link
Contributor

@chillfig chillfig commented Oct 11, 2022 •
edited

Checklist (Please check before submitting)

Describe the contribution

Testing performed
Steps taken to test the contribution:

  • Printed out Filter Table Packet and DS AppData Hash Table Entries after adding a message ID to the Table using OS_printf.
  • Printed out Filter Table Packet Entries and DS AppData Hash Table Entries after removing that same message ID from the Table using OS_printf.
  • Observed that the Filter Table entry with the indicated message ID was reset to 0.
  • Observed that the DS AppData Hash Table Entry for the removed, indicated message ID was reset.

Expected behavior changes
Allows users to remove message IDs from the filter table.

System(s) tested on

  • Ubuntu 18.04

Additional context
N/A

Third party code
If included, identify any third party code and provide text file of license

Contributor Info - All information REQUIRED for consideration of pull request
Justin Figueroa, Vantage Systems

@chillfig chillfig self-assigned this Oct 11, 2022
@chillfig chillfig force-pushed the remove_mid branch 2 times, most recently from b3fbe1a to 7dff414 Compare October 11, 2022 00:13
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 11, 2022
View reviewed changes
fsw/src/ds_table.c Fixed Show fixed Hide fixed
fsw/src/ds_cmds.c Fixed Show fixed Hide fixed
fsw/src/ds_cmds.c
/* */
/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */

void DS_CmdRemoveMID(const CFE_SB_Buffer_t *BufPtr)

Check notice

Code scanning / CodeQL-coding-standard

Long function without assertion

All functions of more than 10 lines should have at least one assertion.
fsw/src/ds_table.c Fixed Show fixed Hide fixed
fsw/src/ds_cmds.c
/* */
/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */

void DS_CmdRemoveMID(const CFE_SB_Buffer_t *BufPtr)

Check notice

Code scanning / CodeQL-coding-standard

Function too long

DS_CmdRemoveMID has too many lines (79, while 60 are allowed).
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 11, 2022
View reviewed changes
fsw/src/ds_cmds.c Fixed Show fixed Hide fixed
@chillfig chillfig force-pushed the remove_mid branch 5 times, most recently from 13484db to 0ce3175 Compare October 25, 2022 17:45
@chillfig chillfig marked this pull request as ready for review October 25, 2022 17:46
chillfig
chillfig commented Oct 25, 2022
View reviewed changes
fsw/src/ds_cmds.c
pPacketEntry->MessageID = CFE_SB_ValueToMsgId(0);

/* Create new hash table as well */
DS_TableCreateHash();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jphickey Would you please let me know your thoughts on using DS_TableCreateHash() here to address the DS Hash Table in removing a message ID, instead of discretely removing a link? As part of this PR, some work went towards doing the latter, but the former seemed easier. This is a concern since DS_CmdAddMID also adds to the Hash Table:

DS/fsw/src/ds_cmds.c

Lines 1415 to 1416 in 49d6a09

/* Add the message ID to the hash table as well */
HashTableIndex = DS_TableAddMsgID(DS_AddMidCmd->MessageID, FilterTableIndex);

@chillfig chillfig changed the title Fix #51, remove mid draft Fix #51, remove mid Oct 25, 2022
@dzbaker
Copy link
Contributor

dzbaker commented Oct 27, 2022

CCB 27 October 2022: Approved pending review and workflow failure resolution.

jphickey
jphickey approved these changes Oct 28, 2022
View reviewed changes
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.

Looks good, just one minor comment

fsw/src/ds_cmds.c Outdated
*/
pPacketEntry = &DS_AppData.FilterTblPtr->Packet[FilterTableIndex];

pPacketEntry->MessageID = CFE_SB_ValueToMsgId(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Only comment here - it would be preferable to use the defined CFE_SB_INVALID_MSG_ID constant here

@chillfig chillfig force-pushed the remove_mid branch 2 times, most recently from 4323e70 to f9c2910 Compare October 31, 2022 18:29
@chillfig
Copy link
Contributor Author

chillfig commented Oct 31, 2022 •
edited

CCB 27 October 2022: Approved pending review and workflow failure resolution.

Regarding the "Build and Run / Build and run with startup msg verification / Build and run app, confirm startup message" failing check, it seems to fail when I run the latest merged commit (commit SHA: 49d6a09) on DS with the addition of a comment change on top of it: https://github.com/chillfig/DS/actions/runs/3363700817/jobs/5577190604#logs. The failing "Unit Test and Coverage" check is failing as expected due to new code being added. New unit tests will be created to resolve that check.

@dzbaker dzbaker mentioned this pull request Nov 2, 2022
Open
2 tasks
@chillfig
Copy link
Contributor Author

chillfig commented Nov 2, 2022

@dzbaker This pull request is now rebased off of #64.

@dzbaker dzbaker merged commit db99b7f into nasa:main Nov 3, 2022
@dmknutsen dmknutsen added this to the Draco milestone Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jphickey jphickey jphickey approved these changes

Assignees

@chillfig chillfig

Labels
CCB:Approved draco-rc4
Projects
None yet
Milestone
Draco
Development

Successfully merging this pull request may close these issues.

Command to remove MIDs from the filter table
4 participants
@chillfig @dzbaker @jphickey @dmknutsen