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 #52, Apply consistent Event ID names to common events #53

Merged

Conversation

thnkslprpt
Copy link
Contributor

@thnkslprpt thnkslprpt commented Oct 21, 2022

Checklist

Describe the contribution

Testing performed
Only GitHub CI actions.

Expected behavior changes
No impact on code behavior (no logic changes).
Consistent Event ID names for the events which are common to all/most cFS components and apps will improve consistency and ease make code review/debugging easier.

Contributor Info
Avi Weiss @thnkslprpt

Copy link
Contributor

@chillfig chillfig left a comment

Choose a reason for hiding this comment

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

CS_RESET_INF_EID has header comment in its definition that suggest it is Debug type. May just need an update on the header comment to change from Type: DEBUG to Type: INFORMATION

@@ -60,7 +60,7 @@
*
* This event message is issued when a reset counters command has been received.
*/
#define CS_RESET_DBG_EID 3
#define CS_RESET_INF_EID 3
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this event name change from using DBG to INF?

Line 57 may need an update to indicate this event is informational type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be updated now Justin.

@thnkslprpt thnkslprpt force-pushed the fix-52-apply-consistent-event-id-names branch from d3335ce to 6ab5451 Compare November 1, 2022 02:54
Copy link
Contributor

@chillfig chillfig 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!

@thnkslprpt
Copy link
Contributor Author

Looks good!

Cheers mate.

Copy link
Contributor

@chillfig chillfig left a comment

Choose a reason for hiding this comment

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

I missed this. fsw/src/cs_table_processing.c/line 112 needs a semi-colon.

fsw/src/cs_table_processing.c Outdated Show resolved Hide resolved
@thnkslprpt thnkslprpt force-pushed the fix-52-apply-consistent-event-id-names branch from 6ab5451 to 7bdfb22 Compare November 1, 2022 03:54
@dzbaker dzbaker added this to the Fornax milestone Nov 21, 2022
@dzbaker dzbaker modified the milestones: Fornax, Equuleus Dec 7, 2022
@chillfig
Copy link
Contributor

chillfig commented Mar 9, 2023

CCB:2023.03.09: Putting on hold until naming conventions for event, etc are merged.

@thnkslprpt thnkslprpt force-pushed the fix-52-apply-consistent-event-id-names branch from 7bdfb22 to 57d7b56 Compare March 12, 2023 05:09
@thnkslprpt thnkslprpt force-pushed the fix-52-apply-consistent-event-id-names branch from 57d7b56 to a5f0cf4 Compare April 1, 2023 23:32
@thnkslprpt thnkslprpt force-pushed the fix-52-apply-consistent-event-id-names branch 2 times, most recently from de94f05 to c31a276 Compare November 1, 2023 17:34
@thnkslprpt thnkslprpt force-pushed the fix-52-apply-consistent-event-id-names branch from c31a276 to ed928b0 Compare April 19, 2024 22:21
@jphickey
Copy link
Contributor

It looks like in the recent rebase, some updates got merged incorrectly, causing the workflow failure

@jphickey jphickey force-pushed the fix-52-apply-consistent-event-id-names branch from ed928b0 to ba3051c Compare July 11, 2024 14:14
@jphickey
Copy link
Contributor

Rebased to latest mainline and manually fixed up some merge overlaps re: payload member. Workflows now pass and this should be good to go.

@thnkslprpt
Copy link
Contributor Author

Thanks Joe.

@dzbaker dzbaker merged commit f958cc0 into nasa:main Jul 15, 2024
16 checks passed
@thnkslprpt thnkslprpt deleted the fix-52-apply-consistent-event-id-names branch July 16, 2024 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent Event ID naming
4 participants