-
Couldn't load subscription status.
- Fork 664
Add sequence and parallel groups that run the default commands of idle subsystems #8293
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
base: main
Are you sure you want to change the base?
Conversation
|
This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this 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.
Eventually this will also need tests and C++.
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.
Why can't this be a (private) factory method in NonBlockingCommands wrapping around Commands.parallel()?
This class violates the command method lifecycle invariants- For example, scheduling cmd.deadlineFor(new RunDefaultsCommand(requirements)) (with no other commands involving requirements running) will cause the default commands to be initialized, executed, then initialized again without a call to end() once the deadline finishes. Another example is a default command finishing normally (which is generally not advised but does happen) but other default commands still running, causing the finished default command to have execute() called at unexpected times.
| allReqs.addAll(command.getRequirements()); | ||
| } | ||
| var group = new SequentialCommandGroup(); | ||
| group.addRequirements(); |
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 does this no-op here for?
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.
Oops, I think it got left there for some reason - thanks
Currently, standard command groups (like Commands.sequence and Commands.parallel) require all subsystems used by any of their children for the entire duration of the group's execution. This prevents the default commands of "idle" subsystems (those required by the group but not by the currently running child command) from running.
This new class provides factory methods that fix this behavior by ensuring default commands run on any subsystem that is not in use by the active command(s).
These commands are designed to bring most of the appeal of proxying(having default commands run within command groups) without the compromise of undefined cancellation behavior and requirements conflicts.