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

ES Does Not Check CFE_PSP_MemRead8 Return Code #120

Closed
skliper opened this issue Sep 30, 2019 · 5 comments
Closed

ES Does Not Check CFE_PSP_MemRead8 Return Code #120

skliper opened this issue Sep 30, 2019 · 5 comments
Labels

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

The ES API functions need to properly alert users of errors. The CFE_ES_CalculateCRC API function will not alert users if an error code was returned from the call to the CFE_PSP_MemRead8.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Imported from trac issue 89. Created by sstrege on 2015-08-19T15:33:04, last modified: 2019-03-01T15:27:58

@skliper skliper self-assigned this Sep 30, 2019
@skliper skliper added the bug label Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-12-08 11:59:50:

In the current development branch, in cfe_es_api.c, CFE_ES_CalculateCRC() function, the code is:

{{{
for ( i = 0 ; i < DataLength ; i++, BufPtr++)
{
if (CFE_PSP_MemRead8((cpuaddr)BufPtr, &ByteValue) != CFE_PSP_SUCCESS)
{
ByteValue = 0;
}
Index = ( ( Crc ^ ByteValue) & 0x00FF);
Crc = ( (Crc >> 8 ) & 0x00FF) ^ CrcTable[Index];
}
}}}

The return code of CFE_PSP_MemRead8 is indeed being checked, but it is handled silently, not reporting this error to the caller.

The API returns the CRC directly, and therefore does not have a provision for returning an error. To report this to the caller would require an API change which would in turn break backward compatibility.

Since CFE_PSP_MemRead8 is extremely unlikely to fail, since it only operates on local RAM on any platform we actually have implemented thus far. Therefore I am not sure this meets the cost/benefit trade off of a breaking API change.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2016-10-18 15:07:03:

Remove my name from many tickets (where it was applied by default), so others can tell that there are tickets that need some attention.

@skliper skliper removed their assignment Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Nov 4, 2019

nasa/PSP#10 - CFE_PSP_MemRead8 is targeted for deprecation.

@jphickey
Copy link
Contributor

jphickey commented Nov 4, 2019

Note that the current master branch no longer has the API call in question here. It was removed prior to github migration, and replaced with a direct memory read. It is no longer calling CFE_PSP_MemRead8 at all.

Therefore I would recommend closing this as invalid, regardless with what happens with nasa/PSP#10, as this is no longer an issue in CFE_ES either way.

@skliper skliper added the invalid label Nov 4, 2019
@skliper skliper closed this as completed Nov 4, 2019
@skliper skliper removed the bug label Dec 30, 2019
jphickey pushed a commit that referenced this issue Jan 9, 2020
Fix #117, #119
Reviewed and approved at 2019-12-18 CCB
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants