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 #759 - removes GetLastSenderId() #764

Closed

Conversation

CDKnightNASA
Copy link
Contributor

@CDKnightNASA CDKnightNASA commented Jun 26, 2020

Fixes #759
Fixes #608

Removes CFE_SB_GetLastSenderId(), which is supremely broken.

Testing performed
sb_UT passes

Expected behavior changes
Removes the CFE_SB_GetLastSenderId() API.

System(s) tested on
Debian 10.3

Contributor Info - All information REQUIRED for consideration of pull request
Christopher.D.Knight@nasa.gov

@CDKnightNASA CDKnightNASA added this to the 6.8.0 milestone Jun 26, 2020
@CDKnightNASA CDKnightNASA self-assigned this Jun 26, 2020
@skliper skliper modified the milestones: 6.8.0, 7.0.0 Jun 29, 2020
@skliper
Copy link
Contributor

skliper commented Jun 29, 2020

Likely needs a requirements update.

@CDKnightNASA
Copy link
Contributor Author

Likely needs a requirements update.

Done. I can squash prior to CCB if that's preferred.

Looking at https://github.com/nasa/cFE/blob/master/docs/cfe%20requirements.docx I have some architectural questions about the SB in general in a multi-processor environment. Perhaps we can discuss at a CCB meeting or have a splinter.

@skliper
Copy link
Contributor

skliper commented Jun 30, 2020

@dmknutsen Heads up this change (assuming stakeholders accept it) will need to be reflected in JIRA, with impacts to test cases.

@jphickey
Copy link
Contributor

The requirement necessitates a different discussion than the API. Reading that requirement text/rationale, it actually seems reasonable to me, but the problem is the implementation didn't match, so I'm not sure it should be removed.

The CFE_SB_GetLastSenderId() didn't actually implement the function described or fulfill this requirement. It was tracking/returning the wrong thing and was not adequate for the rationale described in the requirement text. That's why we need to remove/deprecate this existing API - it doesn't match.

Is it OK to keep a requirement but note that it is not met in the current version(s)? This wouldn't really be a change - as CFE_SB_GetLastSenderId() has never met this requirement - so in theory every CFE version to date has not actually met this requirement.

There are three things we need to do:

  1. Remove/deprecate the existing API as it doesn't correctly implement/fulfill a requirement
  2. Revalidate/discuss the requirement itself and determine if it is still applicable or needs an update
  3. Implement whatever was the outcome of (2) above.

This PR should only do item 1, nothing else at this time. There is certainly a requirements aspect to this, but I wouldn't jump to the conclusion of removing the existing requirement.

@jphickey
Copy link
Contributor

Also, if my understanding is correct, the requirements CSV file is an export from a Jira repository maintained internally by GSFC - so we should not change/modify the CSV file directly anyway.

I'm good with commit 2a17ed2 alone.

@astrogeco
Copy link
Contributor

@CDKnightNASA want to discuss today?

@astrogeco astrogeco added CCB:Ready Ready for discussion at the Configuration Control Board (CCB) CCB-20200701 labels Jul 1, 2020
@astrogeco
Copy link
Contributor

astrogeco commented Jul 1, 2020

CCB 2020-07-01: Will have a design discussion later. Needs some more conversations about Omit_deprecated, certification, etc.

Add a short documentation change explaining that the function does not work as expected.

@CDKnightNASA CDKnightNASA removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Jul 1, 2020
@CDKnightNASA
Copy link
Contributor Author

replaced by #784

dmknutsen added a commit to dmknutsen/cFE that referenced this pull request May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants