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 #503, better events for CFE_TBL_Load() #606

Merged

Conversation

CDKnightNASA
Copy link
Contributor

@CDKnightNASA CDKnightNASA commented Apr 14, 2020

Describe the contribution
Addresses #503 but this is a draft, still need UT code updates. Simplified changes for CFE_TBL_Load() to make the code a bit easier to follow (hey, events generated when results are off-nominal, instead of at the end?)

Partially addresses #607

Testing performed
Initial build/run, UT not updated yet.

Expected behavior changes
Internal CFE_TBL_LoadFromFile() API changed slightly to add AppName as a parameter. Return value from LoadFromFile() no longer relevant for event generation.

System(s) tested on
Debian 9

Contributor Info - All information REQUIRED for consideration of pull request
[email protected]

@CDKnightNASA
Copy link
Contributor Author

Updated the unit tests, removing "draft" status from pull request.

@CDKnightNASA CDKnightNASA marked this pull request as ready for review April 14, 2020 13:34
fsw/cfe-core/src/inc/cfe_error.h Outdated Show resolved Hide resolved
fsw/cfe-core/src/inc/cfe_tbl_events.h Show resolved Hide resolved
fsw/cfe-core/unit-test/tbl_UT.c Outdated Show resolved Hide resolved
fsw/cfe-core/src/tbl/cfe_tbl_api.c Outdated Show resolved Hide resolved

/* Initialize return pointer to NULL */
WorkingBufferPtr = NULL;

/* Verify access rights and get a valid Application ID for calling App */
Status = CFE_TBL_ValidateAccess(TblHandle, &ThisAppId);

if (Status == CFE_SUCCESS)
if (Status != CFE_SUCCESS)
Copy link
Contributor

Choose a reason for hiding this comment

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

This routine mixes use of status and returns throughout that make's it hard to follow. Could it be refactored to clarify flow? Consider doing all the checks first to decide what work to do (or error to return), then do the work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's quite a lot of conditional logic (whether loading the table from a file or memory block, whether the load is partial, etc) so can't do a lot "up-front". I've refactored to do fail=>event+return to reduce deeply-nested if struct of original code. Unsure if it could be further refactored without changing/removing behavior.

@CDKnightNASA CDKnightNASA added enhancement CCB:Ready Ready for discussion at the Configuration Control Board (CCB) cFE-TBL Table services labels Apr 21, 2020
@astrogeco astrogeco changed the title fix #503 - better events for CFE_TBL_Load() Fix #503, better events for CFE_TBL_Load() Apr 21, 2020
@astrogeco
Copy link
Contributor

CCB 20200422 - Approve and open a new ticket to refactor and reduce complexity. There's lots of return statements and we probably don't want to add more.

@skliper
Copy link
Contributor

skliper commented Apr 22, 2020

#600 is the table load refactor, I'll hijack and include the complexity reduction effort

@astrogeco astrogeco added CCB - 20200422 CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Apr 23, 2020
@astrogeco astrogeco changed the base branch from master to integration-candidate April 27, 2020 23:02
@astrogeco astrogeco merged commit a4d087e into nasa:integration-candidate Apr 28, 2020
@jphickey
Copy link
Contributor

This is breaking the integration-candidate build for me, it is using "ThisAppId" variable uninitialized:

/home/joe/code/cfecfs/github/cfe/fsw/cfe-core/src/tbl/cfe_tbl_api.c: In function ‘CFE_TBL_Load’:
/home/joe/code/cfecfs/github/cfe/fsw/cfe-core/src/tbl/cfe_tbl_api.c:687:5: error: ‘ThisAppId’ is used uninitialized in this function [-Werror=uninitialized]
  687 |     CFE_ES_GetAppName(AppName, ThisAppId, OS_MAX_API_NAME);

@astrogeco
Copy link
Contributor

@CDKnightNASA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB cFE-TBL Table services enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CFE_TBL_Load should produce clearer message when it has a file header mismatch
4 participants