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

Simplify handling/checking of event type in commands and use a defined mask #1438

Open
skliper opened this issue Apr 28, 2021 · 0 comments
Open

Comments

@skliper
Copy link
Contributor

skliper commented Apr 28, 2021

Is your feature request related to a problem? Please describe.
Inconsistent handling/checking of event type when input by command.

There is no type mask defined, so some places it's constructed and other's hardcoded:

/* Event Type bit masks */
#define CFE_EVS_DEBUG_BIT 0x0001
#define CFE_EVS_INFORMATION_BIT 0x0002
#define CFE_EVS_ERROR_BIT 0x0004
#define CFE_EVS_CRITICAL_BIT 0x0008

Internal functions EVS_EnableTypes and EVS_DisableTypes both silently mask and use a variable with a constructed value where the value should be const, fragile since each individual value is used (twice) so easy to get out of sync:

void EVS_EnableTypes(EVS_AppData_t *AppDataPtr, uint8 BitMask)
{
uint8 EventTypeBits = (CFE_EVS_DEBUG_BIT | CFE_EVS_INFORMATION_BIT | CFE_EVS_ERROR_BIT | CFE_EVS_CRITICAL_BIT);
/* Enable selected event type bits from bitmask */
AppDataPtr->EventTypesActiveFlag |= (BitMask & EventTypeBits);
} /* End EVS_EnableTypes */

void EVS_DisableTypes(EVS_AppData_t *AppDataPtr, uint8 BitMask)
{
uint8 EventTypeBits = (CFE_EVS_DEBUG_BIT | CFE_EVS_INFORMATION_BIT | CFE_EVS_ERROR_BIT | CFE_EVS_CRITICAL_BIT);
/* Disable selected event type bits from bitmask */
AppDataPtr->EventTypesActiveFlag &= ~(BitMask & EventTypeBits);
} /* End EVS_DisableTypes */

All the command handlers that actually use the above helpers already check against a hard coded value and report out of range bit mask:

if (CmdPtr->BitMask == 0x0 || CmdPtr->BitMask > 0x0F)

if (CmdPtr->BitMask == 0x0 || CmdPtr->BitMask > 0x0F)

if (CmdPtr->BitMask == 0x0 || CmdPtr->BitMask > 0x0F)

if (CmdPtr->BitMask == 0x0 || CmdPtr->BitMask > 0x0F)

Describe the solution you'd like
At minimum define a mask (near bit definitions) and use it. Helpers could check the range and return an error, then handler could just report on failure.

Describe alternatives you've considered
Could leave the check in the handler and remove the helpers since they'd just be an | or &~, not really worth a helper for this approach.

Additional context
Code review

Requester Info
Jacob Hageman - NASA/GSFC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant