FpySequencer Fix no response from validate, refactor to prevent this class of issu…#5315
FpySequencer Fix no response from validate, refactor to prevent this class of issu…#5315zimri-leisher wants to merge 5 commits into
Conversation
…e, work on improving handling of path truncation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // override the getSequenceBaseDir() helper to return a settable member. | ||
| class TestFpySequencer : public FpySequencer { | ||
| public: | ||
| TestFpySequencer(const char* compName) : FpySequencer(compName), m_baseDirOverride(Fpy::SEQ_BASE_DIR) {} |
|
|
||
| // returns the base directory against which sequence file paths are resolved. | ||
| // defaults to the SEQ_BASE_DIR config constant; overridable in tests/deployments | ||
| const char* FpySequencer::getSequenceBaseDir() const { |
| this->m_computedCRC.init(); | ||
|
|
||
| // concat the seq base dir prefix to the seq file path | ||
| const char* baseDir = this->getSequenceBaseDir(); |
| Svc_FpySequencer_SequencerStateMachine::Signal signal, | ||
| const Svc::FpySequencer_SequenceExecutionArgs& value) { | ||
| this->m_sequenceArgs = value.get_buffer(); | ||
| this->m_sequenceExecArgs = value; |
| Svc_FpySequencer_SequencerStateMachine::Signal signal, | ||
| const Svc::FpySequencer_SequenceExecutionArgs& value) { | ||
| this->m_sequenceArgs = value.get_buffer(); | ||
| this->m_sequenceExecArgs = value; |
| // also, the filePath string in SequenceExecutionArgs is guaranteed | ||
| // to be truncated to FileNameStringSize chars, so it will not | ||
| // be truncated by this assignment | ||
| this->m_fullSequenceFilePath = this->m_sequenceExecArgs.get_filePath(); |
|
|
||
| if (fileStatus != Os::File::OP_OK) { | ||
| this->log_WARNING_HI_FileReadError(readStage, this->m_sequenceFilePath, static_cast<I32>(fileStatus)); | ||
| this->log_WARNING_HI_FileReadError(readStage, this->m_fullSequenceFilePath, static_cast<I32>(fileStatus)); |
|
|
||
| if (actualReadLen < expectedReadLen) { | ||
| this->log_WARNING_HI_EndOfFileError(readStage, this->m_sequenceFilePath); | ||
| this->log_WARNING_HI_EndOfFileError(readStage, this->m_fullSequenceFilePath); |
Coverage report — base
|
| Module | Line | Δ | Function | Δ | Branch | Δ |
|---|---|---|---|---|---|---|
Svc/FpySequencer |
86.46 | -0.11 | 98.08 | -0.49 | 76.52 | -0.23 |
Os/Posix |
62.40 | +0.40 | 84.21 | +0.00 | 44.10 | +0.23 |
Modules without UTs
CFDP/Checksum/GTest, Drv/ByteStreamDriverModel, Drv/Interfaces, Drv/LinuxGpioDriver, Drv/LinuxI2cDriver, Drv/LinuxSpiDriver, Drv/LinuxUartDriver, Drv/Ports, Drv/Ports/DataTypes, FppTestProject/FppTest/interfaces, FppTestProject/FppTest/topology/async, FppTestProject/FppTest/topology/components/Comp, FppTestProject/FppTest/topology/components/Framework, FppTestProject/FppTest/topology/components/Receiver, FppTestProject/FppTest/topology/components/Sender, FppTestProject/FppTest/topology/guarded, FppTestProject/FppTest/topology/ports, FppTestProject/FppTest/topology/sync, FppTestProject/FppTest/topology/top_ports, FppTestProject/FppTest/topology/types, Fw/Cmd, Fw/Com, Fw/Comp, Fw/FilePacket/GTest, Fw/Fpy, Fw/Interfaces, Fw/Obj, Fw/Port, Fw/Ports/CompletionStatus, Fw/Ports/Ready, Fw/Ports/Signal, Fw/Ports/SuccessCondition, Fw/Prm, Fw/SerializableFile/test/TestSerializable, Fw/Sm, Fw/Test, Fw/Types/GTest, Os/Models, Svc/Cycle, Svc/DpPorts, Svc/Fatal, Svc/FatalHandler, Svc/FileDownlinkPorts, Svc/FprimeProtocol, Svc/Interfaces, Svc/PassiveConsoleTextLogger, Svc/Ping, Svc/PolyIf, Svc/Ports/CommsPorts, Svc/Ports/FilePorts, Svc/Ports/OsTimeEpoch, Svc/Ports/TlmPacketizerPorts, Svc/Ports/VersionPorts, Svc/Sched, Svc/Seq, Svc/Subtopologies/CdhCore, Svc/Subtopologies/ComCcsds, Svc/Subtopologies/ComFprime, Svc/Subtopologies/ComLoggerTee, Svc/Subtopologies/DataProducts, Svc/Subtopologies/FileHandling, Svc/Types/TlmPacketizerTypes, Svc/WatchDog, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, cmake/test/data/TestDeployment/TestBuildAutocoder, cmake/test/data/TestDeployment/TestChainedAutocoder, cmake/test/data/TestDeployment/TestHeaderAutocoder, cmake/test/data/TestDeployment/TestTargetAutocoder, cmake/test/data/test-fprime-library/TestLibrary/TestComponent, cmake/test/data/test-fprime-library2/TestLibrary2/TestComponent
| // * the block state | ||
| // * the sequence runtime args | ||
| // See the struct member docstrings for more info | ||
| Svc::FpySequencer_SequenceExecutionArgs m_sequenceExecArgs; |
There was a problem hiding this comment.
to begin running the sequence, we need these three args (filename, block state and seq runtime args). Store them all in a single struct so updates to them can be "atomic"
|
|
||
| // the full (including the SEQ_BASE_DIR prefix) path | ||
| // to the sequence file. Note: not updated until validate() is run | ||
| Fw::FileNameString m_fullSequenceFilePath; |
There was a problem hiding this comment.
the file name provided in the sequence exec args is not the "final" filename that we actually try to open. we need to prefix with seq base dir first.
| action setSequenceBlockState: SequenceExecutionArgs | ||
| @ sets the arguments to pass to the sequence | ||
| action setSequenceArguments: SequenceExecutionArgs | ||
| @ sets the sequence execution arguments (seq path, block state, seq args) |
There was a problem hiding this comment.
this is a clear example of how i made the args "atomic"--you can only update all three at once
| // loads the sequence in memory, and does header/crc/integrity checks. | ||
| // return SUCCESS if sequence is valid, FAILURE otherwise | ||
| Fw::Success FpySequencer::validate() { | ||
| FW_ASSERT(this->m_sequenceFilePath.length() > 0); |
There was a problem hiding this comment.
honestly no idea how this assert got in here. it was a clear mistake. the sequencer should not crash if the path length is zero. this has been here for a while.
It is impossible to send an empty string arg in fprime-gds, which I think is the wrong behavior, but was in practice preventing this from occurring.
| stateDiagram-v2 | ||
| direction LR | ||
| state "IDLE | ||
| entry / clearBreakpoint, clearSequenceFile |
There was a problem hiding this comment.
state machine sdd hadn't been updated in a while
|
@LeStarch @thomas-bc whenever you get a sec, this PR is ready for review |
|
TODO make sure we can call abs paths EDIT--updated description to make it clear that you will have to prefix "../../" etc out of the SEQ_BASE_DIR prefix if you want to call something outside of seq base dir. See desc for rationale |
zimri-leisher
changed the title
LeStarch
left a comment
LeStarch
left a comment
There was a problem hiding this comment.
I am not sure I agree with this change. It pushes the need to understand internal details of this component (how full paths are constructed, what command arguments look like) through a port interface and into any user of that port.
This means that any time this component is adjusted, remote components need to be adjusted in parallel to ensure that they remain in-sync with this component.
It would be much better, in my opinion, to send out the full-file path on the seq start port as that is really the expected contract of that port. There are several other ways that could be explored to accomplish this:
- Compute the full file path where it was, store the path and the status, check status in validate and emit an error.
- Consider only "starting a sequence" if it passes validate
- Allow for errors that can occur before validate runs (like we did before).
| // we report the raw operator-supplied path rather than m_fullSequenceFilePath | ||
| // because this action runs in the VALIDATING entry, before validate() resolves | ||
| // the base dir prefix into m_fullSequenceFilePath | ||
| this->seqStartOut_out(0, this->m_sequenceExecArgs.get_filePath(), this->m_sequenceExecArgs.get_runArgsBuf()); |
There was a problem hiding this comment.
I think this breaks encapsulation.
The expected contract of this port is that it includes the sequence file path that was run. The remote receiver should not need to know the internals of how FpySequencer constructs the file path in order to have that information. Nor should the remote component need to know how to handle operator commands to this particular component.
|
Fair point. I think I was imagining a future in which all sequence ecosystem components would use the SEQ_BASE_DIR prefix, so everyone would be "speaking the same language" when it comes to sequence paths.
I'll consider these options again. Another option which I think might be the best, is a new "seqRunReject" port back to the SeqDispatcher. |
3 participants
Change Description
Important note: this makes it somewhat more difficult to call sequences at absolute paths, because there is no option to turn off the suffix. However, I think this is okay, as it's a compile time constant, so as long as you know what FSW you're running, you know the path prefix, and you can always just back out of the prefix directory.
One small other change was adding single quotes around file names in event format strings, so that you can differentiate between the path and the event msg.
Also, updated the state machine SDD
AI Usage (see policy)
Used for writing tests and some final edits. Mostly human written (a rarity for me these days)