[DRAFT] Add sequence dispatcher component #2731

zimri-leisher · 2024-05-13T22:29:49Z


*Related Issue(s)*	#2608 #2729
*Has Unit Tests (y/n)*	Y
*Documentation Included (y/n)*

Change Description

This PR adds a SeqDispatcher component to Svc which allows command sequences to be automatically distributed among CmdSequencers.

Rationale

CmdSequencers only support running a single command sequence at a time. For complex missions, this necessitates having multiple CmdSequencers. However, without SeqDispatcher, ground operators would have to manually keep track of which CmdSequencers are currently being used. SeqDispatcher automates this process.

Testing/Review Recommendations

This is ported from an internal fork of FPrime which is a few versions out of date. Special care should be taken to make sure that the code seen here is up-to-date with the rest of FPrime. Also, probably should make much more in-depth unit tests.

Future Work

Add documentation to .sdd doc

Closes #2729

zimri-leisher · 2024-05-13T22:32:16Z

Hello,
This is a very WIP PR for the SeqDispatcher. Eager to iterate on it with the maintainers and other members of the community!
At the moment all functionality works and some basic testing has been implemented. I had to make some very small changes to CmdSequencer to notify this component when a sequence starts, so that this component can keep track of which sequencers are busy.

Joshua-Anderson

This is fantastic @zimri-leisher! Looks like the exact architecture I would have recommended. I threw in a few minor comments for consideration

Joshua-Anderson · 2024-05-14T17:16:40Z

Svc/SeqDispatcher/SeqDispatcher.fpp

+
+        async input port seqRunIn: Svc.CmdSeqIn
+
+        output port seqRunOut: [types.CMD_SEQUENCERS_COUNT] Svc.CmdSeqIn


It might be worth adding sequence dispatcher custom constant (SeqDispatcherSequencerCount?) for tracking the number of command sequencers, since some projects may want to separately reserve some command sequencers for specific uses. I.e. command sequencer 0 might be used for fault protection sequence dispatch.

Sorry yeah this types.CMD_SEQUENCERS_COUNT was a leftover from our internal fork. Will do

Joshua-Anderson · 2024-05-14T17:20:37Z

Svc/SeqDispatcher/SeqDispatcher.cpp

+
+  // if the seqRunOut port at this idx isn't connected
+  if (!this->isConnected_seqRunOut_OutputPort(sequencerIdx)) {
+    this->log_WARNING_HI_INVALID_SEQUENCER(static_cast<U16>(sequencerIdx));


Instead of checking if the port is connected on use, it might make sense to check if the sequence engine is connected on initialization and asserting if not - letting users know if the topology is misconfigured as quickly as possible.

Great call! Will do

LeStarch

In general, I am impressed! I'll ask @timcanham to look over it too.

LeStarch · 2024-05-14T17:57:44Z

Svc/CmdSequencer/CmdSequencerImpl.hpp

@@ -235,6 +235,10 @@ namespace Svc {
          //! \return The log file name
          Fw::LogStringArg& getLogFileName();

+          //! Get the normal string file name
+          //! \return The normal string file name
+          Fw::String& getStringFileName();


I see this new function here, and the member variable...but not the implementation of the function in the .cpp file changes. Is that missing?

That was a mistake, adding back

LeStarch · 2024-05-14T18:01:23Z

Svc/SeqDispatcher/CMakeLists.txt

+# they might not be available when cmake tries to build this component.
+
+set(MOD_DEPS
+    common/types


I don't recall common/types in F´, nor do I see that in the PR.

Artifact from internal code layout

LeStarch · 2024-05-14T18:02:19Z

Svc/SeqDispatcher/CMakeLists.txt

+
+# Register the unit test build
+set(UT_SOURCE_FILES
+  "${CMAKE_CURRENT_LIST_DIR}/SeqDispatcherCommands.fppi"


Typically you don't need to add .fppi files to source lists. Is this a workaround for the version I have?

Not sure if we've tried without including the fppi files. Will try removing if possible

LeStarch · 2024-05-14T18:07:15Z

Svc/SeqDispatcher/SeqDispatcher.cpp

+// ======================================================================
+
+#include <FpConfig.hpp>
+#include <components/fsw/SeqDispatcher/SeqDispatcher.hpp>


Probably need to update include paths.

LeStarch · 2024-05-14T18:14:09Z

Svc/SeqDispatcher/SeqDispatcherCommands.fppi

+
+async command LOG_STATUS() opcode 1
+
+async command PING() opcode 2


What is PING for? Typically, we do ping ports as those can be tracked by health.

Hmm, internally we add them so that we can ping the component's thread. How do you hook it up to health?

LeStarch · 2024-05-14T18:15:53Z

@timcanham want to take a look, it is a sequence dispatcher!

zimri-leisher · 2024-05-14T18:37:01Z

Thanks for all the comments. Sorry that there are a lot of things that are idiomatic to our code that I didn't remove from here, but honestly part of the reason why I wanted to put this in front of you guys was to get exactly this feedback.

I have a question: where would you recommend storing the CmdSequencerState enum and CMD_SEQUENCERS_COUNT const? Should I make a new .fpp file inside of Svc/SeqDispatcher? What should I call it, if so?

Joshua-Anderson · 2024-05-14T19:22:51Z

Since CMD_SEQUENCERS_COUNT/SeqDispatcherSequencerCount is a per deployment config option, I'd add a SeqDispatcherCfg.fpp file with this constant to config/, similar to DpCfg.fpp. ~~If you wanted to share CmdSequencerState between CmdSequencer and SeqDispatcher, I might add the enum type to the same file.~~

Joshua-Anderson · 2024-05-14T19:26:46Z

Err, since we don't want folks to modify CmdSequencerState, config/ probably isn't the right home for it. Fw/Types/Types.fpp is where we store common types but this is a bit too specific for that. What about using FW::Wait instead of a custom command sequencer enum?

zimri-leisher · 2024-05-14T21:09:09Z

Alright to the SeqDispatcherCfg.fpp file, will do

With regards to using Fw::Wait, I'd prefer not to, as CmdSequencerState has three values instead of two: AVAILABLE, RUNNING_BLOCK, and RUNNING_NO_BLOCK. I also see a relatively low cost to creating a new enum for this. What I'm realizing right now as I write this is that it should just be an enum internal to the .hpp/.cpp files, because it's not used in the fpp at all.

Joshua-Anderson · 2024-05-14T21:16:56Z

@zimri-leisher my bad, I got CmdSequencerState and Svc.CmdSequencer.BlockState confnused.

For component internal enums, you can either add it in the hpp as you suggested, or add it as a standalone type to the SeqDispatcher.fpp file.

The concern we were discussing does arise for the Svc.CmdSequencer.BlockState enum. I'm not sure we want SeqDispatcher to depend on CmdSequencer, since we may want to leave open the option for folks to replace CmdSequencer with their own component.

zimri-leisher · 2024-05-14T21:25:11Z

Good point, maybe I should change the argument of the RUN command to just take a boolean: blocking or non-blocking. No need for a two-valued enum to exist when we have bools I guess :P

Joshua-Anderson · 2024-05-14T21:32:17Z

I'd recommend using an enum like FW:Wait (WAIT and NO_WAIT) instead of a bool, because that can make the sequence files more readable SeqDispatch.Run "/myseq.bin", WAIT vs SeqDispatch.Run "/myseq.bin", TRUE

I'd be curious to get folks opinion on what is the clearest.... WAIT/NO_WAIT, BLOCK/NO_BLOCK, TRUE/FALSE

timcanham · 2024-05-14T23:01:24Z

IMHO not TRUE/FALSE for the reasons @Joshua-Anderson says, WAIT/NO_WAIT seems a little more understandable in plain English.

zimri-leisher · 2024-05-16T15:25:58Z

Ok, will make these changes. However just want to mention that I'm out of office for 2 weeks starting this Friday so they might not happen until then.
I have another question which is related to this PR and also our implementation of complex logic in sequences. Not sure where best to ask it so I'll ask it here:

The idea of this PR is that you shouldn't have to know which sequencer you're running a sequence on ahead of time. However, this means that commands inside of the sequence file don't know either. This is a problem if these commands want to modify something about the sequencer--in our case, we want to change which line the sequencer is on based on some runtime criteria inside of a command.

Is there some way that a command handler can tell from which sequencer a command came from? This would solve my problem. I couldn't find a way and so came up with a really gross solution. Hoping one of you has some input on this.

Joshua-Anderson · 2024-05-16T18:56:51Z

This is a problem if these commands want to modify something about the sequencer--in our case, we want to change which line the sequencer is on based on some runtime criteria inside of a command.

The way I've seen this approached on past missions is a concept of sequence directives. Effectively these are special commands/opcodes that are only valid within a command sequence, and they dispatch a command to the command sequencer executing the sequence.

Ex: SEQ_WAIT 10 sequence diretive within a command sequence would execute a command on the command sequencer executing the sequence to delay 10 seconds until the next command.

Fprime doesn't have any concept of sequence directives yet, but it's something on our roadmap that we want to add. It probably requires a change to the sequence binary format to add a flag indicating if the "command" opcode is a standard fprime command or if it should be dispatched as a sequence directive instead.

What do you think about a sequence directive approach? Would that solve your use cases?

Have a fantastic vacation and no rush at all on this pull request!

zimri-leisher · 2024-05-16T19:00:11Z

That is kind of what I'm doing, only my solution is hackier. I made a component called GenericSequencer which only has empty commands inside of it. Whenever a real sequencer sees a command from a GenericSequencer (it checks this by deserializing the command, and checking if the opcode is equal to one of the known commands in a generic sequencer), it edits the command packet opcode to instead be the opcode from its own command.
Any pitfalls you can see with this approach? I'm kind of afraid of doing it because messing around with binary packets is not where I want to be, but as far as I can tell it's the most seamless way of doing it.

Joshua-Anderson · 2024-05-16T19:17:25Z

@zimri-leisher Yep, very similar approach. Only pitfall is that is it adds commands in your dictionary that you never want to send from the ground. Sequence directives are a slightly cleaner way of accomplishing the same thing.

I think your approach will work well and as a future improvement we can work on the design and implementation of sequence directives for fprime. I'll make sure you get tagged in the github discussions on the topic once we get started.

Zimri Leisher added 2 commits May 13, 2024 17:10

Add sequence dispatcher component

2853b2f

Add seq start port to cmd sequencer

7352ba2

zimri-leisher mentioned this pull request May 13, 2024

Distributing command sequences throughout CmdSequencers #2729

Open

Joshua-Anderson reviewed May 14, 2024

View reviewed changes

LeStarch reviewed May 14, 2024

View reviewed changes

Update author names and some include paths

629abce

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] Add sequence dispatcher component #2731

[DRAFT] Add sequence dispatcher component #2731

zimri-leisher commented May 13, 2024 •

edited

zimri-leisher commented May 13, 2024 •

edited

Joshua-Anderson left a comment

Joshua-Anderson May 14, 2024

zimri-leisher May 14, 2024

Joshua-Anderson May 14, 2024

zimri-leisher May 14, 2024

LeStarch left a comment

LeStarch May 14, 2024

zimri-leisher May 14, 2024

LeStarch May 14, 2024

zimri-leisher May 14, 2024

LeStarch May 14, 2024

zimri-leisher May 14, 2024

LeStarch May 14, 2024

zimri-leisher May 14, 2024

LeStarch May 14, 2024

zimri-leisher May 14, 2024

LeStarch commented May 14, 2024

zimri-leisher commented May 14, 2024

Joshua-Anderson commented May 14, 2024 •

edited

Joshua-Anderson commented May 14, 2024

zimri-leisher commented May 14, 2024

Joshua-Anderson commented May 14, 2024 •

edited

zimri-leisher commented May 14, 2024

Joshua-Anderson commented May 14, 2024

timcanham commented May 14, 2024

zimri-leisher commented May 16, 2024

Joshua-Anderson commented May 16, 2024 •

edited

zimri-leisher commented May 16, 2024 •

edited

Joshua-Anderson commented May 16, 2024 •

edited


		async input port seqRunIn: Svc.CmdSeqIn

		output port seqRunOut: [types.CMD_SEQUENCERS_COUNT] Svc.CmdSeqIn


		async command LOG_STATUS() opcode 1

		async command PING() opcode 2

[DRAFT] Add sequence dispatcher component #2731

Are you sure you want to change the base?

[DRAFT] Add sequence dispatcher component #2731

Conversation

zimri-leisher commented May 13, 2024 • edited

Change Description

Rationale

Testing/Review Recommendations

Future Work

zimri-leisher commented May 13, 2024 • edited

Joshua-Anderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeStarch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeStarch commented May 14, 2024

zimri-leisher commented May 14, 2024

Joshua-Anderson commented May 14, 2024 • edited

Joshua-Anderson commented May 14, 2024

zimri-leisher commented May 14, 2024

Joshua-Anderson commented May 14, 2024 • edited

zimri-leisher commented May 14, 2024

Joshua-Anderson commented May 14, 2024

timcanham commented May 14, 2024

zimri-leisher commented May 16, 2024

Joshua-Anderson commented May 16, 2024 • edited

zimri-leisher commented May 16, 2024 • edited

Joshua-Anderson commented May 16, 2024 • edited

zimri-leisher commented May 13, 2024 •

edited

zimri-leisher commented May 13, 2024 •

edited

Joshua-Anderson commented May 14, 2024 •

edited

Joshua-Anderson commented May 14, 2024 •

edited

Joshua-Anderson commented May 16, 2024 •

edited

zimri-leisher commented May 16, 2024 •

edited

Joshua-Anderson commented May 16, 2024 •

edited