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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix #60, Apply consistent Event ID names to common events #61

wants to merge 1 commit into from

Conversation

thnkslprpt
Copy link
Contributor

@thnkslprpt thnkslprpt commented Oct 21, 2022 •
edited
Loading

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

chillfig
chillfig requested changes Nov 1, 2022
View reviewed changes
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.

May need some comment updates about DS_RESET_INF_EID and its type now being INFORMATION instead of previously DEBUG.

fsw/src/ds_events.h Outdated
@@ -292,7 +292,7 @@
* counters command. The command is used primarily to clear counters
* that have already been examined.
*/
#define DS_RESET_CMD_EID 33
#define DS_RESET_INF_EID 33
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 287 may need a change from Type: DEBUG to Type: INFORMATION

fsw/src/ds_msgdefs.h Outdated
@@ -70,7 +70,7 @@
* \par Command Verification
* Evidence of success may be found in the following telemetry:
* - #DS_HkPacket_t.CmdAcceptedCounter will reset to zero
* - The #DS_RESET_CMD_EID debug event message will be sent
* - The #DS_RESET_INF_EID debug event message will be sent
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 #DS_RESET_INF_EID debug event message will be sent
* - The #DS_RESET_INF_EID informational event message will be sent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Justin. I've added the updated comments in now.

@thnkslprpt thnkslprpt force-pushed the fix-60-apply-consistent-event-id-names branch from a532820 to 92153d5 Compare November 1, 2022 03:03
chillfig
chillfig approved these changes Nov 1, 2022
View reviewed changes
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.

Approved.

The Build and Run workflow in DS is breaking, but due to a null argument in CFE_EVS_SendEvent() in /fsw/src/ds_file.c (https://github.com/nasa/DS/actions/runs/3366470370/jobs/5582995262). This pull request only updates Event ID names and does not seem to influence this error/null argument. @dzbaker what do you think?

This workflow also breaks for DS #51 which doesn't directly change /fsw/src/ds_file.c

@thnkslprpt
Copy link
Contributor Author

thnkslprpt commented Nov 2, 2022 •
edited
Loading

Approved.

The Build and Run workflow in DS is breaking, but due to a null argument in CFE_EVS_SendEvent() in /fsw/src/ds_file.c (https://github.com/nasa/DS/actions/runs/3366470370/jobs/5582995262). This pull request only updates Event ID names and does not seem to influence this error/null argument. @dzbaker what do you think?

This workflow also breaks for DS #51 which doesn't directly change /fsw/src/ds_file.c

Yeah this seems like a separate issue that is only now being triggered in the warnings.
A simple fix might be something like:
image
This change results in build + run passing again without any issues.
Could also have 'UNKNOWN' or just an empty string in place of 'NULL'.
I can open an issue and submit that change if you think it's suitable.

@dzbaker
Copy link
Contributor

dzbaker commented Nov 2, 2022

Approved.
The Build and Run workflow in DS is breaking, but due to a null argument in CFE_EVS_SendEvent() in /fsw/src/ds_file.c (https://github.com/nasa/DS/actions/runs/3366470370/jobs/5582995262). This pull request only updates Event ID names and does not seem to influence this error/null argument. @dzbaker what do you think?
This workflow also breaks for DS #51 which doesn't directly change /fsw/src/ds_file.c

Yeah this seems like a separate issue that is only now being triggered in the warnings. A simple fix might be something like: image This change results in build + run passing again without any issues. Could also have 'UNKNOWN' or just an empty string in place of 'NULL'. I can open an issue and submit that change if you think it's suitable.

Yes @thnkslprpt, that would be great if you could submit that fix and issue. I'll merge it in with the CCB:FastTrack label so we can try this one again.

@thnkslprpt thnkslprpt mentioned this pull request Nov 2, 2022
Closed
2 tasks
@thnkslprpt
Copy link
Contributor Author

Yes @thnkslprpt, that would be great if you could submit that fix and issue. I'll merge it in with the CCB:FastTrack label so we can try this one again.

No worries mate.
#64

@dzbaker
Copy link
Contributor

dzbaker commented Nov 2, 2022

Yes @thnkslprpt, that would be great if you could submit that fix and issue. I'll merge it in with the CCB:FastTrack label so we can try this one again.

No worries mate. #64

Thanks! Can you rebase this (and #62/#59) with main now that the fix has been merged?

@thnkslprpt thnkslprpt force-pushed the fix-60-apply-consistent-event-id-names branch from 92153d5 to 87bbcd3 Compare November 2, 2022 11:40
@thnkslprpt
Copy link
Contributor Author

Thanks! Can you rebase this (and #62/#59) with main now that the fix has been merged?

OK I think that's done now Dylan (for this PR and #62).

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

@dzbaker
Copy link
Contributor

dzbaker commented Mar 9, 2023

Hey @thnkslprpt, would you be able to resolve the conflicts? Thanks!

@thnkslprpt thnkslprpt force-pushed the fix-60-apply-consistent-event-id-names branch 2 times, most recently from b5e2772 to 900bf5d Compare March 14, 2023 10:13
@thnkslprpt thnkslprpt force-pushed the fix-60-apply-consistent-event-id-names branch 2 times, most recently from 06b321f to 6b72267 Compare April 6, 2023 23:01
@thnkslprpt thnkslprpt force-pushed the fix-60-apply-consistent-event-id-names branch from 6b72267 to 012ec36 Compare April 6, 2023 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chillfig chillfig chillfig approved these changes

Assignees

@chillfig chillfig

Labels
enhancement
Projects
None yet
Milestone
Equuleus
Development

Successfully merging this pull request may close these issues.

Inconsistent Event ID naming
3 participants
@thnkslprpt @dzbaker @chillfig