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)*	Y

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

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

Svc/SeqDispatcher/SeqDispatcher.fpp

Svc/SeqDispatcher/SeqDispatcher.cpp

LeStarch

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

Svc/CmdSequencer/CmdSequencerImpl.hpp

Svc/SeqDispatcher/CMakeLists.txt

Svc/SeqDispatcher/SeqDispatcher.cpp

Svc/SeqDispatcher/SeqDispatcherCommands.fppi

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.

…onnections on init

zimri-leisher · 2024-07-25T03:19:54Z

Well, I took my time, but I'm back and gonna get this merged. Just pushed some changes to allow the code to build, and incorporated all of your feedback so far.

What's the best way to add my name to the allowed words list? It's failing on spell check lol

thomas-bc · 2024-07-25T16:31:45Z

@zimri-leisher you can add words that aren't recognized to the https://github.com/nasa/fprime/blob/devel/.github/actions/spelling/expect.txt

zimri-leisher · 2024-07-28T02:17:49Z

Any help on this error?

/workspaces/fprime/fprime/build-fprime-automatic-native-ut/F-Prime/Svc/SeqDispatcher/SeqDispatcherTesterBase.cpp:1170:5: error: no matching function for call to ‘Fw::String::String(Fw::String&)’

Looks like I can fix it by removing explicit from this line in String.hpp:

thomas-bc · 2024-07-30T01:15:21Z

@zimri-leisher could using the FileNameString type help?

zimri-leisher · 2024-08-07T05:14:35Z

It seems the problem was that the (previously unused) CmdSeqIn port took in a string like ref filename: Fw.String instead of filename: string size X
Once I fixed it, the unit test compilation errors went away.

Svc/SeqDispatcher/SeqDispatcher.cpp

+}
+
+//! Handler for input port seqRunIn
+void SeqDispatcher::seqRunIn_handler(NATIVE_INT_TYPE portNum,


Svc/SeqDispatcher/SeqDispatcher.cpp

+                                    const U32 cmdSeq,
+                                    const Fw::CmdStringArg& fileName,
+                                    Fw::Wait block) {
+  FwIndexType idx = this->getNextAvailableSequencerIdx();


Svc/SeqDispatcher/SeqDispatcher.cpp

LeStarch · 2024-08-16T02:17:15Z

Framework failure caused by: #2835. Will rerun.

zimri-leisher · 2024-08-18T00:39:42Z

Okay, ready for review again.

LeStarch

Looks very good! An EVR could be made high instead of low because it is a problem with a direct user command.

Tests could be more complete, and SDD is blank.

Other (very minor style points):

Typically in compound if clauses we put () around each sub-phrase. e.g. if ((A) || (B)). for clarity. Your code is perfectly clear, so no real issue here.
Typically we don't return bool but rather opt for an enumeration that describes the issue. This is truly a "did work" or "did not work", so it is ok.

None of this is a blocker. Good work! I'd like another set of eyes....let me see if @timcanham will review. In the mean-time, consider that EVR to be made high.

Svc/Seq/Seq.fpp

Svc/SeqDispatcher/SeqDispatcherEvents.fppi

Svc/SeqDispatcher/docs/sdd.md

zimri-leisher · 2024-08-28T23:17:55Z

I made the changes to the SDD file and event.
I agree, the unit tests are a bit lacking, I would like to add a couple more.
One question: would we like to retroactively come up with requirements for this component?

Svc/CmdSequencer/Sequence.cpp

Svc/SeqDispatcher/SeqDispatcher.cpp

+void SeqDispatcher::LOG_STATUS_cmdHandler(
+    const FwOpcodeType opCode,          /*!< The opcode*/
+    const U32 cmdSeq) {                   /*!< The command sequence number*/
+  for(FwIndexType idx = 0; idx < SeqDispatcherSequencerPorts; idx++) {


LeStarch · 2024-08-29T16:15:40Z

Known file CI failure. I captured the failing random seed, and re-ran. We have an issue to look into these failures.

timcanham · 2024-08-30T04:07:53Z

@zimri-leisher Do you plan to release ./docs/sdd.md in a future release?

timcanham

@zimri-leisher This looks really great! Minor comments only. If you don't wish to make any changes, I'll approve.

Svc/SeqDispatcher/SeqDispatcherEvents.fppi

Svc/SeqDispatcher/SeqDispatcher.hpp

Svc/SeqDispatcher/SeqDispatcherEvents.fppi

Svc/SeqDispatcher/SeqDispatcher.cpp

Svc/SeqDispatcher/test/ut/SeqDispatcherTester.cpp

garthwatney

Overall looks very good. I have included my review comments.

Svc/SeqDispatcher/SeqDispatcher.cpp

Svc/SeqDispatcher/SeqDispatcher.hpp

LeStarch · 2024-09-03T17:29:23Z

@zimri-leisher I noted some things that could be future work, some things that would be nice to do now. Would you please note those items you intend to close-out for this PR, and which you'd prefer to see as a future work item?

That way I know what to confirm is done before merging and what I should track as future work.

Thanks for all the hard work!

…ert instead of warning for runSeq

zimri-leisher · 2024-09-08T22:46:41Z

Okay, I fixed most of the new comments. Any pointers for the unit tests?

Svc/SeqDispatcher/SeqDispatcher.cpp

+    // directly commanded that specific sequencer
+
+    // warn because this may be unintentional
+    this->log_WARNING_LO_UnexpectedSequenceStarted(static_cast<U16>(portNum), fileName);


Svc/SeqDispatcher/SeqDispatcher.cpp

+    return;
+  }
+
+  this->runSequence(idx, fileName, Fw::Wait::NO_WAIT);


Svc/SeqDispatcher/SeqDispatcher.cpp

+  // no available sequencers
+  if (idx == -1) {
+    this->log_WARNING_HI_NoAvailableSequencers();
+    this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR);


Svc/SeqDispatcher/SeqDispatcher.cpp

+  // no available sequencers
+  if (idx == -1) {
+    this->log_WARNING_HI_NoAvailableSequencers();
+    this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR);


Svc/SeqDispatcher/SeqDispatcher.cpp

+    return;
+  }
+
+  this->runSequence(idx, fileName, block);


Svc/SeqDispatcher/SeqDispatcher.cpp

+    return;
+  }
+
+  this->runSequence(idx, fileName, block);


Svc/SeqDispatcher/SeqDispatcher.cpp

LeStarch · 2024-09-18T00:55:05Z

I am going to merge and take action for adding rules testing!

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

Closed

Joshua-Anderson reviewed May 14, 2024

View reviewed changes

Svc/SeqDispatcher/SeqDispatcher.fpp Outdated Show resolved Hide resolved

Svc/SeqDispatcher/SeqDispatcher.cpp Outdated Show resolved Hide resolved

LeStarch reviewed May 14, 2024

View reviewed changes

Update author names and some include paths

629abce

Get fully compiling, move consts/enums to correct places, check for c…

04f23eb

…onnections on init

zimri-leisher changed the title ~~[DRAFT] Add sequence dispatcher component~~ Add sequence dispatcher component Jul 25, 2024

zimri-leisher added 2 commits July 28, 2024 01:49

Add spelling exceptions

71df861

Get unit tests almost compiling...

5668d92

Fix string type in port, call component init in test

5af7b1b

github-advanced-security bot found potential problems Aug 16, 2024

View reviewed changes

Svc/SeqDispatcher/SeqDispatcher.cpp Fixed Show fixed Hide fixed

Svc/SeqDispatcher/SeqDispatcher.cpp Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Aug 16, 2024

View reviewed changes

Svc/SeqDispatcher/SeqDispatcher.cpp Fixed Show fixed Hide fixed

Svc/SeqDispatcher/SeqDispatcher.cpp Fixed Show fixed Hide fixed

zimri-leisher added 2 commits August 18, 2024 00:33

UpperCamel events, add warning for unexpected seq start

4add94c

remove init method, add check for connected to getNextAvailableIdx

a8c6596

LeStarch approved these changes Aug 28, 2024

View reviewed changes

Svc/Seq/Seq.fpp Show resolved Hide resolved

Svc/SeqDispatcher/SeqDispatcherEvents.fppi Outdated Show resolved Hide resolved

Svc/SeqDispatcher/docs/sdd.md Show resolved Hide resolved

LeStarch requested review from timcanham and gjwatney August 28, 2024 22:52

zimri-leisher added 2 commits August 28, 2024 18:19

Merge branch 'nasa:devel' into 2608-seq-dispatcher

c7b324c

Update sdd, change event from low to high, static cast a portnum

f7c4108

github-advanced-security bot found potential problems Aug 29, 2024

View reviewed changes

Svc/CmdSequencer/Sequence.cpp Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Aug 29, 2024

View reviewed changes

Svc/SeqDispatcher/SeqDispatcher.cpp Dismissed Show dismissed Hide dismissed

Svc/SeqDispatcher/SeqDispatcher.cpp Dismissed Show dismissed Hide dismissed

github-advanced-security bot found potential problems Aug 29, 2024

View reviewed changes

timcanham assigned zimri-leisher Aug 30, 2024

timcanham reviewed Aug 30, 2024

View reviewed changes

garthwatney approved these changes Aug 30, 2024

View reviewed changes

Svc/SeqDispatcher/SeqDispatcher.cpp Show resolved Hide resolved

Svc/SeqDispatcher/SeqDispatcher.cpp Outdated Show resolved Hide resolved

Svc/SeqDispatcher/SeqDispatcher.hpp Show resolved Hide resolved

Add state diagram, add more warnings, fix wrong header types, use ass…

28b021e

…ert instead of warning for runSeq

github-advanced-security bot found potential problems Sep 9, 2024

View reviewed changes

Svc/SeqDispatcher/SeqDispatcher.cpp Dismissed Show dismissed Hide dismissed

github-advanced-security bot found potential problems Sep 9, 2024

View reviewed changes

Svc/SeqDispatcher/SeqDispatcher.cpp Dismissed Show dismissed Hide dismissed

LeStarch approved these changes Sep 18, 2024

View reviewed changes

LeStarch merged commit b0716ad into nasa:devel Sep 18, 2024
35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sequence dispatcher component #2731

Add sequence dispatcher component #2731

zimri-leisher commented May 13, 2024 •

edited

Loading

zimri-leisher commented May 13, 2024 •

edited

Loading

Joshua-Anderson left a comment

LeStarch left a comment

LeStarch commented May 14, 2024

zimri-leisher commented May 14, 2024

Joshua-Anderson commented May 14, 2024 •

edited

Loading

Joshua-Anderson commented May 14, 2024

zimri-leisher commented May 14, 2024

Joshua-Anderson commented May 14, 2024 •

edited

Loading

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

Loading

zimri-leisher commented May 16, 2024 •

edited

Loading

Joshua-Anderson commented May 16, 2024 •

edited

Loading

zimri-leisher commented Jul 25, 2024 •

edited

Loading

thomas-bc commented Jul 25, 2024

zimri-leisher commented Jul 28, 2024

thomas-bc commented Jul 30, 2024

zimri-leisher commented Aug 7, 2024

LeStarch commented Aug 16, 2024

zimri-leisher commented Aug 18, 2024

LeStarch left a comment

zimri-leisher commented Aug 28, 2024 •

edited

Loading

LeStarch commented Aug 29, 2024

timcanham commented Aug 30, 2024

timcanham left a comment

garthwatney left a comment

LeStarch commented Sep 3, 2024

zimri-leisher commented Sep 8, 2024

LeStarch commented Sep 18, 2024

Add sequence dispatcher component #2731

Add sequence dispatcher component #2731

Conversation

zimri-leisher commented May 13, 2024 • edited Loading

Change Description

Rationale

Testing/Review Recommendations

Future Work

zimri-leisher commented May 13, 2024 • edited Loading

Joshua-Anderson left a comment

Choose a reason for hiding this comment

LeStarch left a 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 Loading

Joshua-Anderson commented May 14, 2024

zimri-leisher commented May 14, 2024

Joshua-Anderson commented May 14, 2024 • edited Loading

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 Loading

zimri-leisher commented May 16, 2024 • edited Loading

Joshua-Anderson commented May 16, 2024 • edited Loading

zimri-leisher commented Jul 25, 2024 • edited Loading

thomas-bc commented Jul 25, 2024

zimri-leisher commented Jul 28, 2024

thomas-bc commented Jul 30, 2024

zimri-leisher commented Aug 7, 2024

LeStarch commented Aug 16, 2024

zimri-leisher commented Aug 18, 2024

LeStarch left a comment

Choose a reason for hiding this comment

zimri-leisher commented Aug 28, 2024 • edited Loading

LeStarch commented Aug 29, 2024

timcanham commented Aug 30, 2024

timcanham left a comment

Choose a reason for hiding this comment

garthwatney left a comment

Choose a reason for hiding this comment

LeStarch commented Sep 3, 2024

zimri-leisher commented Sep 8, 2024

LeStarch commented Sep 18, 2024

zimri-leisher commented May 13, 2024 •

edited

Loading

zimri-leisher commented May 13, 2024 •

edited

Loading

Joshua-Anderson commented May 14, 2024 •

edited

Loading

Joshua-Anderson commented May 14, 2024 •

edited

Loading

Joshua-Anderson commented May 16, 2024 •

edited

Loading

zimri-leisher commented May 16, 2024 •

edited

Loading

Joshua-Anderson commented May 16, 2024 •

edited

Loading

zimri-leisher commented Jul 25, 2024 •

edited

Loading

zimri-leisher commented Aug 28, 2024 •

edited

Loading