-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DRAFT] Add sequence dispatcher component #2731
base: devel
Are you sure you want to change the base?
Conversation
Hello, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fantastic @zimri-leisher! Looks like the exact architecture I would have recommended. I threw in a few minor comments for consideration
|
||
async input port seqRunIn: Svc.CmdSeqIn | ||
|
||
output port seqRunOut: [types.CMD_SEQUENCERS_COUNT] Svc.CmdSeqIn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry yeah this types.CMD_SEQUENCERS_COUNT was a leftover from our internal fork. Will do
|
||
// 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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great call! Will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I am impressed! I'll ask @timcanham to look over it too.
@@ -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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a mistake, adding back
# they might not be available when cmake tries to build this component. | ||
|
||
set(MOD_DEPS | ||
common/types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recall common/types
in F´, nor do I see that in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Artifact from internal code layout
|
||
# Register the unit test build | ||
set(UT_SOURCE_FILES | ||
"${CMAKE_CURRENT_LIST_DIR}/SeqDispatcherCommands.fppi" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically you don't need to add .fppi
files to source lists. Is this a workaround for the version I have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we've tried without including the fppi files. Will try removing if possible
Svc/SeqDispatcher/SeqDispatcher.cpp
Outdated
// ====================================================================== | ||
|
||
#include <FpConfig.hpp> | ||
#include <components/fsw/SeqDispatcher/SeqDispatcher.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably need to update include paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
|
||
async command LOG_STATUS() opcode 1 | ||
|
||
async command PING() opcode 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is PING for? Typically, we do ping ports as those can be tracked by health.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, internally we add them so that we can ping the component's thread. How do you hook it up to health?
@timcanham want to take a look, it is a sequence dispatcher! |
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 |
Since CMD_SEQUENCERS_COUNT/SeqDispatcherSequencerCount is a per deployment config option, I'd add a |
Err, since we don't want folks to modify CmdSequencerState, |
Alright to the With regards to using Fw::Wait, I'd prefer not to, as CmdSequencerState has three values instead of two: |
@zimri-leisher my bad, I got CmdSequencerState and For component internal enums, you can either add it in the hpp as you suggested, or add it as a standalone type to the The concern we were discussing does arise for the |
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 |
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 I'd be curious to get folks opinion on what is the clearest.... WAIT/NO_WAIT, BLOCK/NO_BLOCK, TRUE/FALSE |
IMHO not TRUE/FALSE for the reasons @Joshua-Anderson says, WAIT/NO_WAIT seems a little more understandable in plain English. |
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. 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. |
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: 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! |
That is kind of what I'm doing, only my solution is hackier. I made a component called |
@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. |
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