Skip to content

Fix #503, better events for CFE_TBL_Load()#606

Merged
astrogeco merged 1 commit intonasa:integration-candidatefrom
CDKnightNASA:fix-503-tbl_events
Apr 28, 2020
Merged

Fix #503, better events for CFE_TBL_Load()#606
astrogeco merged 1 commit intonasa:integration-candidatefrom
CDKnightNASA:fix-503-tbl_events

Conversation

@CDKnightNASA
Copy link
Contributor

@CDKnightNASA CDKnightNASA commented Apr 14, 2020 •
edited by skliper
Loading

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 CDKnightNASA requested a review from skliper April 14, 2020 01:31
@CDKnightNASA CDKnightNASA self-assigned this Apr 14, 2020
@CDKnightNASA CDKnightNASA linked an issue Apr 14, 2020 that may be closed by this pull request
CFE_TBL_Load should produce clearer message when it has a file header mismatch #503
Closed
@CDKnightNASA
Copy link
Contributor Author

CDKnightNASA commented Apr 14, 2020

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
skliper
skliper reviewed Apr 14, 2020
View reviewed changes
fsw/cfe-core/src/tbl/cfe_tbl_api.c
Status = CFE_TBL_ValidateAccess(TblHandle, &ThisAppId);

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

@skliper skliper Apr 14, 2020

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

@CDKnightNASA CDKnightNASA Apr 14, 2020

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 linked an issue Apr 21, 2020 that may be closed by this pull request
CFE_TBL should send events instead of using syslog #607
Open
@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

astrogeco commented Apr 22, 2020

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

jphickey commented Apr 29, 2020

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

astrogeco commented Apr 29, 2020

@CDKnightNASA

@skliper skliper mentioned this pull request May 1, 2020
Merged
@jphickey jphickey mentioned this pull request May 7, 2020
Closed
@skliper skliper removed a link to an issue May 12, 2020
CFE_TBL should send events instead of using syslog #607
Open
@skliper skliper mentioned this pull request May 12, 2020
Open
@skliper skliper added this to the 6.8.0 milestone Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@skliper skliper skliper left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@CDKnightNASA CDKnightNASA

Labels

CCB:Approved Indicates code review and approval by community CCB cFE-TBL Table services enhancement

Projects

None yet

Milestone

Bootes

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

@CDKnightNASA @astrogeco @skliper @jphickey