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 #1157, increment sequence if indicated #1275

Closed
wants to merge 1 commit into from

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Apr 1, 2021

Describe the contribution
Do not restrict sequence increment to TLM only, since CMD packets can be locally generated too (e.g. SCH, TIME).

Fixes #1157

Testing performed
Build and sanity check CFE, run all unit tests.

Expected behavior changes
All transmitted messages will get their sequence count incremented if transmitted with parameter IncrementSequenceCount set to true. Previously, for some reason, this flag was only honored on TLM packets.

System(s) tested on
Ubuntu 20.04

Additional context
Thus far I've not found a real reason/explanation as to why this flag would be ignored for CMD packets. If there is a good reason, it should be documented. Otherwise for the sake of consistency it should be done for all messages.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

Do not restrict sequence increment to TLM only, since
CMD packets can be locally generated too (e.g. SCH, TIME).
@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Apr 1, 2021
@jphickey
Copy link
Contributor Author

jphickey commented Apr 1, 2021

Tagging @acudmore and @jwilmot in case they can offer some history on this subject.

As it stands we handle CMD vs. TLM packets inconsistently - basically we ignore the "IncrementSequenceCount" flag if it is a command - without really any explanation/justification as to why we would ignore it. I'm suggesting removing this to make it consistent, but there may be some history I'm not aware of.

My recommendation would be - if there is a use case where the counter shouldn't be incremented, why can't the caller just set this parameter false ...?

@astrogeco
Copy link
Contributor

astrogeco commented Apr 7, 2021

CCB:2021-04-07

  • This has a history and we need to be careful changing this
  • In the previous builds there was no option to increment the sequence count so the behavior was implicit
  • Could be related to command authentication
  • Needs a splinter
  • Document the "unexpected" behavior so it is obvious

@astrogeco astrogeco added CCB:Splinter Item needs a focused splinter meeting and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Apr 7, 2021
@skliper skliper added this to the cFS-Draco milestone Aug 6, 2021
@skliper skliper removed this from the Draco milestone Mar 18, 2022
@astrogeco astrogeco added the CCB:Ignore Pull Request can be ignored. Will be re-examined at by next CCB. label May 27, 2022
@jphickey jphickey closed this Jan 17, 2024
@jphickey jphickey deleted the fix-1157-cmd-seq branch January 17, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Ignore Pull Request can be ignored. Will be re-examined at by next CCB. CCB:Splinter Item needs a focused splinter meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto increment sequence on CMD packets
3 participants