Skip to content

Conversation

@zimri-leisher
Copy link
Collaborator

@zimri-leisher zimri-leisher commented Mar 5, 2025

Related Issue(s) #3023
Has Unit Tests (y/n) WIP
Documentation Included (y/n) WIP

Change Description

The 0.1 version of the FpySequencer, which replicates the functionality of the existing CmdSequencer component.
Features:

  • Dispatching arbitrary commands
  • Absolute and relative delays between commands
  • Configurable timeout
  • Cancelling sequences
  • Validating and running as separate commands
  • Blocking/non-blocking running of sequences

It uses FPP state machines extensively.
~1300 lines of CPP
~550 lines of FPP

Rationale

This is the first part of the roadmap for the implementation of the advanced sequencing language.

Zimri Leisher and others added 30 commits November 25, 2024 16:39
Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed all your comments as best I could.

@@ -0,0 +1,26 @@
# Svc::FpySequencer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bocchino wants a minimal SDD.

@zimri-leisher
Copy link
Collaborator Author

Awesome, just the feedback I was looking for. Will fix.

@LeStarch
Copy link
Collaborator

LeStarch commented Apr 7, 2025

Finalizing changes:

  1. Add sequence file ID. Add it to context. -- TODO
    1. If sequence ID matches active but the returned opcode is not expected: WARN then stop sequence (wrong opcode from current sequence).
    2. If sequence ID does not match, WARN but continue (spurious response from a previous sequence).
    3. Capture sequence command id (distinguish between multiple same opcodes)
    4. Capture sequence file ID (dustingush against different runs)
  2. Add SDD w/ statement about being in development.
  3. Remove priorities.
  4. Remove test usage in Ref.
  5. No invalid sequence
  6. Make sequence failed specific to reason why
  7. Statement Index to events where approrpiate

Once done, I will review one more time and then merge as-is.

To make this prime-time we will additionally need to:

  1. Author better UTs (close to 100% coverage)
  2. Author a full SDD with documented binary format
  3. Author users guide for tool support
  4. Add to references and templates.
  5. Constant for the buffer size
  6. Failed on line
  7. Consider: fixing object ids
    These steps are a separate PR.

@zimri-leisher
Copy link
Collaborator Author

zimri-leisher commented Apr 14, 2025

Finalizing changes:

  • Add SDD w/ statement about being in development. DONE
  • Remove priorities. WONTDO -- I know I said I would remove priorities, but due to some slight changes in internal architecture it's now hard to remove them. Tim said it was okay, so I'm gonna keep them in.
  • Remove test usage in Ref. WILLCO
  • No invalid sequence DONE
  • Make sequence failed specific to reason why DONE
  • Statement Index to events where approrpiate DONE

The remainder are about the "what to do if we get an unexpected response" problem.

Here is the behavior that I think we want:

  1. If we get a response back from a different sequence, warn but do not cancel current.
    • Rationale: This can happen nominally if we cancelled the previous sequence while a cmd was executing
  2. If we get a response back from the same sequence:
    1. If the opcode or cmdIdentifierNumber (a number incrementing from 0 modulo 2^16) do not match, fail the sequence
      • Rationale: it must be a coding error if the current sequence gave us a different response. Do not proceed!
    2. Otherwise, all's well, proceed as normal.

Working on implementing this now.

@zimri-leisher
Copy link
Collaborator Author

zimri-leisher commented Apr 14, 2025

The implementation of the new, more precise command response behavior is quite complex and needs another pair of eyes. @LeStarch please review FpySequencer::cmdResponseIn_handler and the RUNNING state (specifically, the on stmtResponse_unexpected signal listener) in the FpySequencerStateMachine.fppi.

EDIT--this is ready to review now

Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need to confirm 2 more things:

  1. priorities when we discussed, @timcanham heard "priorities aren't needed in this component", and @zimri-leisher heard "Priorities can be used". Both statements were true, but created different expectations to the path forward. I am working on a resolution
  2. I see exactly what I expect from the code block for the new command response handling, except it seems to me that the m_statementsDispatched is incremented in the wrong place as it is incremented just after dispatch, resulting in a wait for the next command. What am I missing?
  3. Remove from Ref when the above two are resolved.

Very good work. We are in the final steps.

@LeStarch
Copy link
Collaborator

LeStarch commented Apr 17, 2025

Still need to confirm 2 more things:

1. `priorities` when we discussed, @timcanham heard "priorities aren't needed in this component", and @zimri-leisher heard "Priorities can be used".  Both statements were true, but created different expectations to the path forward.  I am working on a resolution

2. I see exactly what I expect from the code block for the new command response handling, except it seems to me that the m_statementsDispatched is incremented in the wrong place as it is incremented just after dispatch, resulting in a wait for the next command.  What am I missing?

3. Remove from Ref when the above two are resolved.

Very good work. We are in the final steps.

I confirmed Priorities are green-light. Thus to close out this PR

  • Address/Resolve my last round of feedback
  • Answer my ordering question
  • Remove from Ref
  • Pass CI
  • Merge
  • Celebrate 🎉🎉 🎉 🎉 🎉

@zimri-leisher
Copy link
Collaborator Author

Okay, I think I have completed all of those items.

Copy link
Collaborator

@bocchino bocchino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good to me!

  • I reviewed and approved the state machine design
  • I asked for a minimal SDD, which was provided

@LeStarch
Copy link
Collaborator

@zimri-leisher we'll need to fix the CI errors that came in when you bumped F Prime. Let me know if you need help!

@zimri-leisher
Copy link
Collaborator Author

Okay, believe they're all fixed now.


#if FW_SERIALIZABLE_TO_STRING
void StatementArgBuffer::toString(Fw::StringBase& text) const {
static const char * formatString = "(data = %p, size = %" PRI_FwSizeType ")";

Check notice

Code scanning / CodeQL

Use of basic integral type Note

formatString uses the basic integral type char rather than a typedef with size and signedness.
// Construction, initialization, and destruction
// ----------------------------------------------------------------------

FpySequencer ::FpySequencer(const char* const compName) :

Check notice

Code scanning / CodeQL

Use of basic integral type Note

compName uses the basic integral type char rather than a typedef with size and signedness.
@zimri-leisher
Copy link
Collaborator Author

@LeStarch let's get this merged!!! So excited.

@LeStarch LeStarch merged commit 6e2fb1f into nasa:devel Apr 21, 2025
44 checks passed
@LeStarch
Copy link
Collaborator

  • Versioning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants