search

nasa / fprime

F´ - A flight software and embedded systems framework
https://fprime.jpl.nasa.gov
Apache License 2.0
9.95k stars 1.28k forks source link

[DRAFT] Add sequence dispatcher component #2731

Open zimri-leisher opened 1 month ago

zimri-leisher commented 1 month ago
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 commented 1 month ago

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.

LeStarch commented 1 month ago

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

zimri-leisher commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

zimri-leisher commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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.