Nesting CommandGroups breaks scheduler

[rothso]

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.

[rhauch]

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 sequentially and simultaneously methods that can, in fact, be nested. See this command group subclass for an example. Have you tried using the sequentially and simultaneously methods rather than creating your own custom InnerCommandGroup?

[rhauch]

@rothso, any thoughts on my previous comment about using sequentially and simultaneously methods rather than creating your own custom InnerCommandGroup?

[rothso]

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):

// Robot.java
CommandGroup crossChevalCommand = sequentially(
        simultaneously(
            new RaiseAntlerSnorfler(antlerSnorfler),
            new DriveTimed(drive, CROSS_SPEED_ENTRANCE, CROSS_DELAY_SECONDS)
        ),
        new DriveTimed(drive, CROSS_SPEED_MIDWAY, 1.75 /* ?? */),
        new LowerAntlers(antlerSnorfler)
);
Strongback.submit(new ScoringAutonomous(..., crossChevalCommand, ...);

The composition occurs flawlessly. At this point I'm forced to make crossChevalCommand a into a class because it gets used in teleop too. So the new code is refactored into this:

// CrossChevalCommand.java
public class CrossChevalCommand extends CommandGroup {

  public CrossChevalCommand() {
    sequentially(
        simultaneously(
            new RaiseAntlerSnorfler(antlerSnorfler),
            new DriveTimed(drive, CROSS_SPEED_ENTRANCE, CROSS_DELAY_SECONDS)
        ),
        new DriveTimed(drive, CROSS_SPEED_MIDWAY, 1.75 /* ?? */),
        new LowerAntlers(antlerSnorfler)
    );
  }
}

// Robot.java
CommandGroup crossChevalCommand = new CrossChevalCommand();
Strongback.submit(new ScoringAutonomous(..., crossChevalCommand, ...);

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 CommandGroup object depending on if I subclass or if I instantiated using the static methods. The Scheduler only checks the root of the command being submitted; for all nested CommandGroups, it jumps instead to commands. A nested CommandGroup instantiated as a subclass still has its commands under root.

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 Command now) unexpectedly does not behave like other Commands.

A fix could be to change the CommandGroup class to store the child commands of its subclasses under the commands member rather than root.commands so 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 the root middleman.

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]

@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 CommandGroup could 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]

Our original goal was to simplify the design and to prefer composition over subclassing

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?

public class CrossingCommands {

  public static Command crossCheval() {
    return runSequentially(
        runSimultaneously(
            new RaiseAntlerSnorfler(antlerSnorfler),
            new DriveTimed(drive, CROSS_SPEED_ENTRANCE, CROSS_DELAY_SECONDS)
        ),
        new DriveTimed(drive, CROSS_SPEED_MIDWAY, 1.75 /* ?? */),
        new LowerAntlers(antlerSnorfler)
    );
  }
}

// And later
Strongback.submit(new Autonomous(..., CrossingCommands.crossCheval(), ...);

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]

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

Thank you so much!