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 #16, Convert LC state macros to enums #48

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

Fix #16, Convert LC state macros to enums #48

wants to merge 1 commit into from

Conversation

thnkslprpt
Copy link
Contributor

@thnkslprpt thnkslprpt commented Nov 2, 2022 •
edited
Loading

Checklist

Describe the contribution

Testing performed
GitHub CI actions (incl. Build + Run, Unit Tests etc.) all passing successfully.

Expected behavior changes
No impact on behavior. Enums improve type-safety and ease debugging.

Contributor Info
Avi @thnkslprpt

@dzbaker dzbaker requested a review from jphickey November 3, 2022 18:30
jphickey
jphickey requested changes Nov 3, 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.

For messages and tables we need to stick with types that have a well-defined bit length because these are interface structures.

Otherwise, I like making enum's, but we need to adhere to the name convention, because in the future the intent is to generate these header files from a command/data dictionary.

fsw/src/lc_msgdefs.h Outdated
LC_STATE_PASSIVE, /**< \brief LC Application State Pasive */
LC_STATE_DISABLED, /**< \brief LC Application State Disabled */
LC_STATE_FROM_CDS /**< \brief Used for reset processing, not valid state */
} LC_AppState_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

Type name should be LC_AppState_Enum_t and labels should all start with LC_AppState_ prefix (instead of LC_STATE_)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

fsw/src/lc_msgdefs.h Outdated
LC_APSTATE_DISABLED, /**< \brief Actionpoint state disabled */
LC_APSTATE_PERMOFF, /**< \brief Actionpoint state permanently off, see #LC_SET_AP_PERMOFF_CC */
LC_ACTION_NOT_USED = 255 /**< \brief Actionpoint unused, not valid command argument */
} LC_APState_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar typedef and name here (see cFE naming convention document)

Copy link
Contributor Author

@thnkslprpt thnkslprpt Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Updated.
Renamed to LC_ActionPointState... (rather than LC_ApState... to avoid confusion with LC_AppState....

fsw/src/lc_tbl.h Outdated
uint16 RPNEquation[LC_MAX_RPN_EQU_SIZE]; /**< \brief Reverse Polish Equation that
specifies when this actionpoint
should fail */
LC_APState_t DefaultState; /**< \brief Default state for this AP (enumerated)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should stay uint8 because it is a table (enums are not defined length)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.
Forgot to consider that aspect.

fsw/src/lc_msg.h Outdated
uint16 APNumber; /**< \brief Which actionpoint(s) to change */
uint16 NewAPState; /**< \brief New actionpoint state */
uint16 APNumber; /**< \brief Which actionpoint(s) to change */
LC_APState_t NewAPState; /**< \brief New actionpoint state */
Copy link
Contributor

Choose a reason for hiding this comment

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

this should stay uint16 because it is a message (enums are not defined length)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.
Thanks - forgot to consider that aspect.

fsw/src/lc_tbl.h Outdated
uint8 CurrentState; /**< \brief Current state of this actionpoint */
uint8 ActionResult; /**< \brief Result for the last sample of this actionpoint */

LC_APState_t CurrentState; /**< \brief Current state of this actionpoint */
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, it needs to be a defined length in this context (uint8)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 3, 2022
View reviewed changes
fsw/src/lc_app.c
@@ -831,7 +831,7 @@
** May need to override the restored application state
*/

#if LC_STATE_WHEN_CDS_RESTORED != LC_STATE_FROM_CDS
#if LC_STATE_WHEN_CDS_RESTORED != LC_AppState_FROM_CDS

Check notice

Code scanning / CodeQL-coding-standard

Conditional compilation

Use of conditional compilation must be kept to a minimum.
@thnkslprpt thnkslprpt force-pushed the fix-16-convert-state-macros-to-enums branch from 0a658cd to ad7c0e7 Compare November 5, 2022 09:24
@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-16-convert-state-macros-to-enums branch 5 times, most recently from fe4d7f2 to 9147dfc Compare March 12, 2023 11:39
thnkslprpt
thnkslprpt commented Mar 12, 2023
View reviewed changes
fsw/inc/lc_msgdefs.h Outdated
@@ -361,7 +371,7 @@

#ifndef LC_OMIT_DEPRECATED
#define LC_SET_AP_PERMOFF_CC LC_SET_AP_PERM_OFF_CC
#define LC_ACTION_NOT_USED LC_APSTATE_NOT_USED
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 note this additional change due to the rebase to incorporate your recent changes in #74

@thnkslprpt thnkslprpt force-pushed the fix-16-convert-state-macros-to-enums branch 3 times, most recently from 1fec42a to fbb0dcc Compare March 14, 2023 13:20
@thnkslprpt thnkslprpt force-pushed the fix-16-convert-state-macros-to-enums branch 2 times, most recently from 604bb1d to cce59de Compare April 7, 2023 00:40
@thnkslprpt thnkslprpt force-pushed the fix-16-convert-state-macros-to-enums branch 2 times, most recently from 06f1b3b to 3c2b663 Compare April 22, 2023 09:19
@thnkslprpt thnkslprpt force-pushed the fix-16-convert-state-macros-to-enums branch 2 times, most recently from 7558be6 to 4d4b42d Compare May 5, 2023 03:26
@thnkslprpt thnkslprpt force-pushed the fix-16-convert-state-macros-to-enums branch from 4d4b42d to 27324be Compare May 19, 2023 03:38
@thnkslprpt thnkslprpt force-pushed the fix-16-convert-state-macros-to-enums branch from 27324be to 712a7ac Compare June 1, 2023 22:27
@thnkslprpt thnkslprpt force-pushed the fix-16-convert-state-macros-to-enums branch from 712a7ac to b4a92c1 Compare June 1, 2023 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jphickey jphickey Awaiting requested review from jphickey

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Equuleus
Development

Successfully merging this pull request may close these issues.

LC: Consider making states into enums
3 participants
@thnkslprpt @jphickey @dzbaker