[cmd] Add DeferredCommand

[rzblue]

Closes #5150
Allows commands to be constructed at runtime without proxying.

Requirements must be specified explicitly, even if there are no requirements. Java uses a Set<Subsystem>. Not sure how to accomplish this in c++. The reason for this is that requirements must be known before runtime (and therefore before the contained command is constructed, and users will likely forget to include the proper requirements.

The other potential tripping point is that the supplier must create a new command each time it's called, because this is considered a command composition, so we don't want the supplied commands being used for other compositions or being scheduled directly.

Still needs c++ tests, will finish those up this weekend.

[Gold856]

Requirements must be specified explicitly, even if there are no requirements. Java uses a Set<Subsystem>. Not sure how to accomplish this in c++.

I think you've done it already. Most commands like RunCommand have a constructor with a default argument for requirements:

explicit RunCommand(std::function<void()> toRun, std::span<Subsystem* const> requirements = {});

which I think is used when requirements aren't specified. You haven't specified any default arguments for requirements. so a compilation error should happen if requirements aren't specified. At the minimum, teams would need to pass {} for requirements.

The other potential tripping point is that the supplier must create a new command each time it's called, because this is considered a command composition, so we don't want the supplied commands being used for other compositions or being scheduled directly.

I think creating new commands is already standard practice.

Constructors that take a CommandPtr would also be nice. #5155 modified ProxyCommand constructors to accept CommandPtr.

[Starlight220]

The other potential tripping point is that the supplier must create a new command each time it's called, because this is considered a command composition, so we don't want the supplied commands being used for other compositions or being scheduled directly.

I think creating new commands is already standard practice.

Perhaps add a note in documentation that for selecting one of a preallocated set of commands, SelectCommand should be used?

---

Also, deprecate the deferring ProxyCommand overload, with a note pointing to DeferredCommand and a possible mention of combining the two.

[KangarooKoala]

The other potential tripping point is that the supplier must create a new command each time it's called, because this is considered a command composition, so we don't want the supplied commands being used for other compositions or being scheduled directly.

Couldn't we mark the commands as not composed after we're done with them via CommandScheduler.removeComposedCommand() or Command.SetComposed(false)?

[rzblue]

Couldn't we mark the commands as not composed after we're done with them via CommandScheduler.removeComposedCommand() or Command.SetComposed(false)?

Not sure about this. Let me think. @Starlight220 any thoughts?

Also, deprecate the deferring ProxyCommand overload, with a note pointing to DeferredCommand and a possible mention of combining the two.

I'm not sure about this. Combining them would result in the same behavior, but combining the need for requirements to be explicitly specified for DeferredCommand combined with the confusion with ProxyCommand requirements seems like a recipe for even more confusion.

[rzblue]

Constructors that take a CommandPtr would also be nice. #5155 modified ProxyCommand constructors to accept CommandPtr.

What would be the point of this? EDIT: Ah, you mean constructors that take a CommandPtr supplier. yeah, i can do that

[Gold856]

Discussion in #5150 also mentions subsystem factories to make requirement handling easier for teams.

Resolves #5150. Can you link #5150?

[Starlight220]

Couldn't we mark the commands as not composed after we're done with them via CommandScheduler.removeComposedCommand() or Command.SetComposed(false)?

Unmarking commands isn't something we've done before, and I think it would just cause even more confusion due to race conditions. On the other hand, we usually also don't make commands this late into runtime.
I think it would be simpler to not unmark, but I'm not entirely sure.

[rzblue]

Just found a somewhat serious issue with this, as well as with the command library in general.

If you add a command to a composition after that command has been scheduled directly with the scheduler, there will be no errors thrown and the command will happily be run by both the composition and the command scheduler. (This of course ignores requirements, but that's a separate issue.

We haven't run into this because typically commands are constructed prior to runtime, but that isn't requirement.

This should be fixed for the library as a whole- we can check if the command is scheduled with the scheduler as part of the pre-composition check.

Test demonstrating the issue:

    @Test
    void testScheduleThenCompose() {
        assert DriverStation.isEnabled();
        AtomicInteger i = new AtomicInteger();
        Command command = Commands.run(() -> {
            System.out.println(i.incrementAndGet());
        });
        command.schedule();
        Command command2 = new SequentialCommandGroup(command); // This line should throw
        command2.schedule();
        CommandScheduler.getInstance().run();
        assertEquals(i, 1);
    }

[KangarooKoala]

I like Ryan's suggestion to check if a command is already scheduled in registerComposedCommands(), because the symmetry with the requireNotComposed() check in schedule() makes it seem quite robust at preventing commands from being composed and scheduled.

Also, should discussion continue in a new issue?

[rzblue]

Opened #5581 to fix

[rzblue]

Command* constructors have been removed. The null handling has also been removed, since CommandPtr cannot hold null.

[Starlight220]

The null handling has also been removed, since CommandPtr cannot hold null.

CommandPtrs can still be invalid / hold null as a result of a use-after-move, or if they're initialized with a null unique_ptr.

[rzblue]

CommandPtrs can still be invalid / hold null as a result of a use-after-move

CommandPtr handles this by throwing an exception, and this is bad behavior anyway

if they're initialized with a null unique_ptr.

This ends up being handled with the same checking as above- I can't even unwrap the CommandPtr to check if the pointer is valid, the unwrap function triggers CommandPtr's null check

TLDR; CommandPtr cannot hold a null pointer and be used at all.

[WarrenReynolds]

Is there any example code of how a DeferCommand can be used in C++? A google search has terminated with me ending up here.

[Starlight220]

Here's an example of a command factory that prints the game data fetched when the command is scheduled.

CommandPtr PrintGameData() {
  return frc2::cmd::Defer([] { return frc2::cmd::Print(frc::DriverStation::GetGameData()); });
}

I hope this helps.
Apparently there indeed hasn't been docs added for Defer yet.