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

CFE_TBL_Load should produce clearer message when it has a file header mismatch #503

Closed
CDKnightNASA opened this issue Jan 30, 2020 · 7 comments · Fixed by #606
Closed

CFE_TBL_Load should produce clearer message when it has a file header mismatch #503

CDKnightNASA opened this issue Jan 30, 2020 · 7 comments · Fixed by #606
Assignees
Milestone

Comments

@CDKnightNASA
Copy link
Contributor

Is your feature request related to a problem? Please describe.
CFE_TBL_LoadFromFile compares the application's name + table name with the string in the header of the file. If you configure the cfe_es_startup.scr with a different application name than what is expected in the header, you get an obtuse "Fail to load Tbl '.' from '/cf/.tbl'" not making it clear that it's purely a mismatch of the header.

Describe the solution you'd like
At minimum it should indicate that the matter is a mis-match of the header and the expected header (which means that the file is there, for example). Better would be to report the actual contents of the header and the expected value. The EID for the error is unique so CFE_TBL_Load can produce an event with more specific text.

Describe alternatives you've considered
Could have CFE_TBL_LoadFromFile generate the event, but that's an "internal" fn so probably not.

Additional context
Add any other context about the feature request here.

Requester Info
[email protected]

@CDKnightNASA CDKnightNASA self-assigned this Jan 30, 2020
@CDKnightNASA
Copy link
Contributor Author

Note that CFE_TBL_Load writes to syslog instead of generating events...I'd like to discuss at the CCB whether this is correct behavior or whether the table code should be generating events. The description for WriteToSysLog states:

"This routine writes a formatted string to the cFE system log. This can be used to record very low-level errors that can't be reported using the Event Services. This function is used in place of printf for flight software. It should be used for significant startup events, critical errors, and conditionally compiled debug software."

I wouldn't classify loading tables as "significant startup events, critical errors..."

@astrogeco
Copy link
Contributor

@CDKnightNASA we just created the "ready for CCB" label for cases like this :). Can you add the label to this issue?

@CDKnightNASA CDKnightNASA added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Feb 11, 2020
@astrogeco astrogeco added this to the 6.8.0 milestone Feb 12, 2020
@astrogeco
Copy link
Contributor

CCB 20200212 - Good idea, need to think through implementation: events VS sys_log and best way to handle it.

@CDKnightNASA
Copy link
Contributor Author

  • I need to confirm that ES can/does log to syslog if configured or if it's otherwise unable to generate event messages on the bus.

@CDKnightNASA CDKnightNASA removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Feb 12, 2020
@astrogeco astrogeco added the CCB:Approved Indicates code review and approval by community CCB label Feb 17, 2020
@astrogeco astrogeco added IC - 20200226 and removed IC - 20200226 CCB:Approved Indicates code review and approval by community CCB labels Feb 25, 2020
@skliper
Copy link
Contributor

skliper commented Apr 8, 2020

@CDKnightNASA will you have time to work this one soon, or should I remove the current milestone? Happy to go either way, just likely won't get in this release cycle if not addressed soon.

Or is the syslog/event discussion a good candidate for a new issue, and this one could be closed with just updating the current message?

@CDKnightNASA
Copy link
Contributor Author

working on it now

@CDKnightNASA
Copy link
Contributor Author

Some comments: No EVS does not send events to SysLog if EVS can't publish to the SB. (In fact, EVS does not even check the return value of CFE_SB_SendMsg()--I am thinking I should file a bug against EVS that it should check the return value and, if not CFE_SUCCESS, should report an error to SysLog? About the only error that is likely from SendMsg() is a CFE_SB_BUF_ALOC_ERR, but it is possible!

Second - the logic for CFE_TBL_Load() is that it logs to syslog in the main line and then at the end of the function, it checks the status and if it's not success, then it reports an event depending on the value of status. Seems clunky and difficult to debug. I'm going to remove that logic, make Load() purely send events, and generally clean things up.

One other comment -- a lot of the messages report the contents of the Status variable as a hex number. Given that the Status is set to a #define decimal value, I'm very confused why hex.

CDKnightNASA added a commit to CDKnightNASA/cFE that referenced this issue Apr 10, 2020
CDKnightNASA added a commit to CDKnightNASA/cFE that referenced this issue Apr 14, 2020
@CDKnightNASA CDKnightNASA linked a pull request Apr 14, 2020 that will close this issue
CDKnightNASA added a commit to CDKnightNASA/cFE that referenced this issue Apr 14, 2020
CDKnightNASA added a commit to CDKnightNASA/cFE that referenced this issue Apr 14, 2020
CDKnightNASA added a commit to CDKnightNASA/cFE that referenced this issue Apr 21, 2020
astrogeco added a commit that referenced this issue Apr 28, 2020
Fix #503, better events for CFE_TBL_Load()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants