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

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.

HS_RESET_INF_EID and/or comments mentioning it may need an update before merging. Comments suggest it is of DEBUG type.

@@ -320,7 +320,7 @@
* This event message is issued when a reset counters command has
* been received.
*/
#define HS_RESET_DBG_EID 24
#define HS_RESET_INF_EID 24
Copy link
Contributor

@chillfig chillfig Nov 1, 2022

Choose a reason for hiding this comment

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

Why does the event name change from DBG to INF? Shouldn't line 316 also change to indicate Informational type?

This line may also need an update if the change is warranted.
https://github.com/thnkslprpt/HS/blob/e90051e221c462b8cf0409253a677e0df8459947/fsw/src/hs_msgdefs.h#L113

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's really a matter of interpretation whether to list some of these as DBG or INF.
I will update the comments to align with the common Event ID type as per the updates.

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 mate.

@thnkslprpt thnkslprpt force-pushed the fix-50-apply-consistent-event-id-names branch from e90051e to e8bb8ac Compare November 1, 2022 02:52
@thnkslprpt thnkslprpt requested review from chillfig and removed request for dmknutsen November 1, 2022 02:52
@thnkslprpt thnkslprpt force-pushed the fix-50-apply-consistent-event-id-names branch from e8bb8ac to 3d39202 Compare November 1, 2022 03:00
@@ -110,7 +110,7 @@
* Successful execution of this command may be verified with
* the following telemetry:
* - #HS_HkPacket_t.CmdCount will be cleared
* - The #HS_RESET_DBG_EID debug event message will be
* - The #HS_RESET_INF_EID debug event message will be
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* - The #HS_RESET_INF_EID debug event message will be
* - The #HS_RESET_INF_EID informational event message will be

@dzbaker dzbaker added this to the Fornax milestone Nov 21, 2022
@dzbaker dzbaker modified the milestones: Fornax, Equuleus Dec 7, 2022
@thnkslprpt thnkslprpt force-pushed the fix-50-apply-consistent-event-id-names branch from 3d39202 to 0652ca2 Compare March 12, 2023 05:02
@dzbaker dzbaker requested a review from chillfig March 30, 2023 18:39
@thnkslprpt thnkslprpt force-pushed the fix-50-apply-consistent-event-id-names branch from 0652ca2 to 051741b Compare March 31, 2023 02:34
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.

Changes look consistent to me.

@thnkslprpt thnkslprpt force-pushed the fix-50-apply-consistent-event-id-names branch 2 times, most recently from 0a7a50d to 05a5cf1 Compare April 6, 2023 21:28
@thnkslprpt thnkslprpt force-pushed the fix-50-apply-consistent-event-id-names branch from 05a5cf1 to 98da814 Compare May 5, 2023 02:54
@thnkslprpt thnkslprpt force-pushed the fix-50-apply-consistent-event-id-names branch from 98da814 to 5fb454f Compare November 1, 2023 17:36
@thnkslprpt thnkslprpt force-pushed the fix-50-apply-consistent-event-id-names branch from 5fb454f to 36b5b19 Compare January 14, 2024 22:13
@dzbaker dzbaker merged commit e5d77d4 into nasa:main Jul 15, 2024
17 checks passed
@thnkslprpt thnkslprpt deleted the fix-50-apply-consistent-event-id-names branch July 16, 2024 09:27
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
3 participants