Repository navigation
Nesting CommandGroups breaks scheduler #81
Description
For our Autonomous routine, we have a custom CommandGroup that contains another custom CommandGroup that we inject at runtime.
While the Scheduler is building the CommandRunner and transversing through the commands, it assumes any inner CommandGroups it encounters must be one of the three built-in sequencing helpers and (rightfully) calls getCommands to grab the command list.
However, the Scheduler breaks when you pass in a custom CommandGroup, i.e. one which you extend yourself, because the commands are actually located under getRoot().getCommands(). Meanwhile, getCommands() returns null.
public class NestingCommandGroup extends CommandGroup {
public NestingCommandGroup() {
sequentially(InnerCommandGroup(), ...);
}
}
public class InnerCommandGroup extends CommandGroup {
public InnerCommandGroup() {
...
}
}The following test case should fail with a NullPointerException.
@Test
public void shouldExecuteInnerNamedCommand() {
Command c = NestingCommandGroup();
scheduler.submit(c);
scheduler.execute(0);
assertThat(...);
}And this is the debugger tree, if it helps.
Activity
rhauch commentedon Sep 18, 2016
Our original design intended that CommandGroups are generic containers for parallel or sequential groups of commands, and that they are created by subclassing them and using the
sequentiallyandsimultaneouslymethods that can, in fact, be nested. See this command group subclass for an example. Have you tried using thesequentiallyandsimultaneouslymethods rather than creating your own custom InnerCommandGroup?rhauch commentedon Oct 26, 2016
@rothso, any thoughts on my previous comment about using
sequentiallyandsimultaneouslymethods rather than creating your own custom InnerCommandGroup?rothso commentedon Oct 27, 2016
Hey again, really sorry about the delay!
Thank you for linking to the AutoChevalDeFrise class, it's very helpful to see how the mthods are meant to be used. Compared to my current approach, I have something similar but instead of a subclass for each Autonomous mode, I have a ScoringAutonomous class (sorry for the Kotlin, the constructor logic can be found in the
init{}block) because our logic for driving up to and away from the defenses is all the same aside from a few parameters (like the gate, target goal, etc.) which we provide in the constructor.One of these parameters is the defense crossing strategy. For handling the actual crossing action, I've extracted that into separate classes like CrossCheval. This is then provided in the constructor. We can't inline commands because the defenses vary, so our best option was to resort to the strategy pattern. I'm then composing the defense crossing command as if it were any plain old Command, visible at line 47.
If I do something like this, the entire command sequence runs fine (back to Java):
The composition occurs flawlessly. At this point I'm forced to make
crossChevalCommanda into a class because it gets used in teleop too. So the new code is refactored into this:Both groups of commands follow the exact same nesting structure, but now the Cheval command no longer runs. Likewise, if I were to include ScoringAutonomous as part of a larger CommandGroup, then it would not run either. The child commands are stored in slightly different places in the
CommandGroupobject depending on if I subclass or if I instantiated using the static methods. The Scheduler only checks therootof the command being submitted; for all nested CommandGroups, it jumps instead tocommands. A nested CommandGroup instantiated as a subclass still has its commands underroot.This seems like a bug, because you would expect both examples to behave the same since in both cases an instance of CommandGroup is provided. The discrepancy shows a contradiction of the Liskov substitution principle, since a particular subclass (of
Commandnow) unexpectedly does not behave like otherCommands.A fix could be to change the CommandGroup class to store the child commands of its subclasses under the
commandsmember rather thanroot.commandsso it matches the internal structure of the other static sequential/simultaneous CommandGroups, which seems to be the structure understood by the Scheduler. I'm not sure if something is restricting this approach, but I could submit a PR that slightly modifies the CommandGroup and Scheduler classes to potentially get rid of the need for therootmiddleman.This is a minor issue, but I do believe it prevents someone from creating a reusable CommandGroup to use in other CommandGroups without doing some odd tricks and it's definitely a frustrating issue to run into because the code itself doesn't throw any errors; it fails silently. Either way, I really appreciate your time and patience.
rhauch commentedon Oct 27, 2016
@rothso, our original goal was to simplify the design and to prefer composition over subclassing. For example, if a CommandGroup needs to be reused, you can always create a method that creates it when required.
Regardless of our original intent and biases, I do agree it would be better if
CommandGroupcould be extended more naturally, since it makes sense that in some cases subclassing might be preferred. We do want to evolve Strongback to be as usable as possible. Go ahead an create a PR with the proposed change; that'll be the easiest way to evaluate your proposal.Thanks for your feedback and offer to help!
rothso commentedon Oct 28, 2016
Oh! Now I think I'm finally understanding this.
So rather than subclassing CommandGroup for the commands like CrossCheval, I should instead create a class like CrossingCommands and have a static method return the command created through the helper methods, similar to what you're doing in the Hardware module?
So this means I was not forced to create a subclass after all. Your approach makes a lot of sense! I can definitely see the merits of avoiding subclassing and preferring these types of methods instead.
Funny, it turns out the premise behind this issue was totally wrong. I really appreciate you walking me through this, it's been incredibly enlightening. Hopefully the PR that's up can still help someone who may have a use case that truly requires subclassing (holding state, maybe? though I would imagine CommandGroups are intended to be stateless).
rhauch commentedon Oct 28, 2016
@rothso You're PR looks good and we'll merge shortly. I do agree this change will be cleaner for people that want to subclass, and we shouldn't restrict that option.
Thanks again for your contribution! Keep up the great work!
rothso commentedon Oct 28, 2016
Thank you so much!