Conversation
|
So is modifying the RUN going to be a separate PR? Why not just do it in this PR, I think they are related. New RUN_ARGS cmd for SeqDispatcher and update existing RUN cmd for FpySeq |
| } | ||
|
|
||
| // Store args for pushArgsToStack action | ||
| this->m_pendingSeqArgs = buffer; |
There was a problem hiding this comment.
There is a cleaner way of passing around the pending sequence arguments without adding new state.
I described it in an earlier comment:
Important: you're going to have to add the SeqArgs struct to the SequenceExecutionArgs struct, and pass it in on line 370 here. You should probably add a new SM action called "pushArgsToStack" or something, which happens when cmd_RUN is received,
| # validate does not take as input a block state, only a path | ||
| on cmd_VALIDATE do { setGoalState_VALID, setSequenceFilePath } enter VALIDATING | ||
| # run takes both path and block state | ||
| on cmd_RUN do { setGoalState_RUNNING, setSequenceFilePath, setSequenceBlockState } enter VALIDATING |
There was a problem hiding this comment.
pushArgsToStack should go here, and it should take an argument, of type SequenceExecutionArgs, and the arg buf should be added to that struct
just copy what's already done with setSequenceFilePath or setSequenceBlockState.
| constant SequenceArgumentsMaxSize = 512 No newline at end of file | ||
| @ Note: This must fit within FW_CMD_ARG_BUFFER_MAX_SIZE along with other command arguments | ||
| @ Total serialized size: fileName (~44 bytes) + Fw::Wait (4 bytes) + SeqArgs (8 + buffer_size) | ||
| @ With FW_CMD_ARG_BUFFER_MAX_SIZE ~= 500, we limit this to 400 bytes |
There was a problem hiding this comment.
FW_CMD_ARG_BUFFER_MAX_SIZE is now an FPP constant. Just use it directly to calculate the value here. That way it will change if the cmd arg buf size changes.
| # prio: lower than sm sig and CANCEL | ||
| async command RUN_VALIDATED( | ||
| $block: BlockState @< Return command status when complete or not | ||
| ) \ |
There was a problem hiding this comment.
This command needs an argument buf too. And that would mean that we'd need a RUN_VALIDATED_ARGS command. However, I think it would be okay to skip this for now, because this command is not used frequently.
Please make a github issue to track adding the args buf to this command when we fix the GDS.
|
|
||
| Fpy::StackSizeType availableSpace = Fpy::MAX_STACK_SIZE - this->m_runtime.stack.size; | ||
|
|
||
| if (args.get_size() > availableSpace) { |
There was a problem hiding this comment.
The way this PR is currently implemented, sendSignal_result_failure does not do what you expect it does here. It actually would do nothing, and the FSW would try to run the sequence. Could be quite bad. This is because, when this action is called, you're in the RUN state. Homework for you: make sure you know which signals do what in the RUN state. Many signals won't do anything.
Once you make the modification I mentioned regarding the SequenceExecutionArgs, this function will just set the m_pendingSeqArgs blindly. See action_setSequenceFilePath.
Then, the size check will go in the FpySequencer::validate method. You can just return Fw::Success::FAILURE.
Please also add a unit test for this case.
|
Port buffers approved. |
3 participants
Change Description
Add a U8 array to CmdSeqIn to allow pass through arguments from SeqDisp to CmdSequencer / FpySequencer / etc
Rationale
This is a feature to pass through arguments from the SequenceDispatcher to Sequencers (current and future) that support it.
Testing/Review Recommendations
Ensure SequenceDispatch functionality is working, both on fprime and fpy side.
These changes should not break / altar core functionality.
Future Work
This is a chain of pull requests to support arguments in sequences. Future work includes:
AI Usage (see policy)
Claude Code v2.1.92 sonnet used to autocomplete and find instances where CmdSeqIn changes were required.
FPP coding was done by hand and claude was used to autocomplete C++ calls.
All lines of code were human reviewed.
@LeStarch @zimri-leisher