[cmd] Commands v3 framework

[SamCarlberg]

The framework fundamentally relies on the continuation API added in Java 21 (which is currently internal to the JDK). Continuations allow for call stacks to be saved to the heap and resumed later

The async framework allows command bodies to be written in an imperative style:

Mechanism drivetrain = new Drivetrain();

Command driveTenFeet =
  drivetrain.run(coroutine -> {
    drivetrain.resetEncoders();
    while (drivetrain.getDistance() < 10) {
      drivetrain.tankDrive(1, 1);
      coroutine.yield(); // <- note the yield call!
    }
    drivetrain.stop();
  }).named("Drive Ten Feet");

Which would look something like this using the current functional framework:

Subsystem drivetrain = new Drivetrain();

Comand driveTenFeet =
  drivetrain.runOnce(drivetrain::resetEncoders)
    .andThen(driveTrain.run(() -> drivetrain.tankDrive(1, 1))
    .until(() -> drivetrain.getDistance() < 10)
    .finallyDo(drivetrain::stop)
    .withName("Drive Ten Feet");

Commands could be written as thin wrappers around regular imperative methods:

class Drivetrain extends Mechanism {
  void driveDistance(Coroutine coroutine, Distance distance) {
    encoders.reset();
    while (getDistance().lt(distance)) {
      tankDrive(1, 1);
      coroutine.yield();
    }
    stop();
  }

  Command driveDistanceCommand(Distance distance) {
    return run(coroutine -> driveDistance(coroutine, distance)).named("Drive " + distance.toLongString());
  }
}

However, an async command will need to be actively cooperative and periodically call coroutine.yield() in loops to yield control back to the command scheduler to let it process other commands.

There are also some other additions like priority levels (as opposed to a blanket yes/no for ignoring incoming commands), factories requiring names be provided for commands, and the scheduler tracking all running commands and not just the highest-level groups. However, those changes aren't unique to an async framework, and could just as easily be used in a traditional command framework.

Links:
JEP 425 - Virtual Threads

Scheduler.java
ParallelGroup.java
Sequence.java

[calcmogul]

We've discussed putting a command-based rewrite in its own package instead of wpilibjN. For example, edu.wpi.first.commands3. edu.wpi.first.wpilibj2 created a bunch of confusion for which classes were where.

[SamCarlberg]

We've discussed putting a command-based rewrite in its own package instead of wpilibjN. For example, edu.wpi.first.commands3. edu.wpi.first.wpilibj2 created a bunch of confusion for which classes were where.

It's too early to be bikeshedding 😄 My goal here is to inform future command framework changes, not to have a ready-to-go implementation. (Though I do agree that wpilibjN is a bad naming scheme; I'd personally just put these in a .wpilibj.command.async package and be done with it)

[Starlight220]

Any reason for pause over suspend or yield?
The latter is preferable imo

[SamCarlberg]

Any reason for pause over suspend or yield?

It's clearer about what it does without needing to assume a baseline knowledge of multithreading concepts. And it's apt if async functions/continuations/coroutines (or wherever else you'd like to call them) are taught as pausable functions

[Starlight220]

And it's apt if async functions/continuations/coroutines (or wherever else you'd like to call them) are taught as pausable functions

I strongly disagree that "pausable functions" is the term that should be used -- it's not a term that is commonly used and will likely cause confusion.
A quick google search for "pausable functions" brings no relevant info, whereas "yield function" and "suspend function" bring relevant info (mainly centered around python and kotlin coroutines respectively).

Therefore I maintain my suggestion to name it yield -- it'll be immediately recognized as akin to Python's yield (by users with Python experience, and if students learn Python later).
My rationale for preferring yield over suspend is that Python's concurrency model is more commonly known and is more semantically similar to this than Kotlin's.

Command-based historically has a problem of being very DSL-y and magic incantations; this is a good opportunity to take a big step to using common industry terms. Let's take that step.

[shueja]

What happens if AsyncCommand.pause() is called from outside an AsyncCommand virtual thread?

[Starlight220]

With the current impl, it would suspend the entire carrier thread. Throwing could be better.

[Oblarg]

This seems like it could also support differing command interruption semantics in addition to priorities. If we have both of those features, we can very naturally represent "default commands" as a minimum-priority command with suspend or reschedule behavior on interruption.

[SamCarlberg]

Therefore I maintain my suggestion to name it yield

This is a good argument. I'll note that virtual thread pausing does not behave like yield in python or kotlin (or ruby), which allows data to be passed to an invoking function. Java also has a context-specific yield keyword (similar to var), which allows branches in switch expressions to return values to the caller. I'm a little concerned about overloading the meaning of yield with what's built into the language, but most users wouldn't already be used to it because it was added in Java 13. A yield method also cannot be called directly, it must be qualified; just yield() will not compile, but this.yield() or AsyncCommand.yield() would.

What happens if AsyncCommand.pause() is called from outside an AsyncCommand virtual thread?

As written, it would just block the calling thread. Yotam's suggestion of throwing would certainly be better, since it makes no sense to pause outside a command.

This seems like it could also support differing command interruption semantics in addition to priorities. If we have both of those features, we can very naturally represent "default commands" as a minimum-priority command with suspend or reschedule behavior on interruption.

I like this idea. How would a resume-after-interruption command look in user code?

[Oblarg]

I think we should re-purpose the InterruptBehavior enum, since its prior purpose is entirely superseded by priorities. I also think we should remove withInterruptBehavior and replace it with onInterrupt overloads.

So, it'd look like:

run(...).onInterrupt(InterruptBehavior.kSuspend);
run(...).onInterrupt(() -> { ... }, InterruptBehavior.kSuspend);

Suspended commands will be re-scheduled as soon as their requirements become available. The implementation that allows this could also allow us to eventually support queued commands.

[SamCarlberg]

I think we should re-purpose the InterruptBehavior enum, since its prior purpose is entirely superseded by priorities. I also think we should remove withInterruptBehavior and replace it with onInterrupt overloads.

Reminder that commands are interrupted by interrupting their vthread, which triggers an InterruptedException in any blocking calls in the command (Thread.sleep, Object.wait, Future.get, Lock.lock, etc), and immediately exits the run method call. I don't think we can get access to the underlying continuation layer to have it keep a vthread on ice.

Maybe we could have a pause() method on the scheduler that does a busywait? This would keep it paused internally as long as higher-priority commands are running, then automatically pick back up when it becomes the highest-priority command remaining.

This is not a complete and correct implementation, but for the sake of example:

// AsyncScheduler.java
public void pause(AsyncCommand command, Measure<Time> time) {
  long ms = (long) time.in(Milliseconds);
  boolean suspendOnInterrupt = command.interruptBehavior() == kSuspend;

  do {
    try {
      Thread.sleep(ms);
    } catch (InterruptedException e) {
      if (suspendOnInterrupt)
        continue;
      throw e;
    }
  } while (command.priority() < getHighestPriorityCommandUsingAnyOf(command.requirements()).priority());
}

[TheTripleV]

This is very similar syntax to my python coroutine command impl.

encoders.reset()
while getDistance() < distance:
  yield
  tankDrive(1, 1)
stop()

It does active cooperation with yield.

I've been mulling over doing yield based coroutines vs async/await based coroutines throughout wpilib and I think there needs to be more exploring do on which is better long-term (idk if java has async/await keywords incoming but it would be nice if the python/java apis were similar).

[SamCarlberg]

Java isn't getting async/await keywords, the best we have is Futures (more or less analogous to JavaScript promises). That's error prone since it's totally valid to call a method that returns a Future and then not await its result. A yield-based API seems less prone to unintended behavior

[SamCarlberg]

Virtual threads ended up being too prone to deadlocking and inconsistent timing, since it's at the mercy of the JVM's thread scheduler for when parked vthreads are resumed. I was consistently seeing sleep times of 20ms end up taking ~25ms, with the worst outlier at ~38ms.

I've pushed changes using Java's Continuation class that's used internally for pause/resume funcionality of virtual threads. Unfortunately, it's not a public API, so I need to do some build hacks with --add-opens to make it accessible. It also means Thread.sleep is back to being a bad decision, since it'll sleep the main execution thread, which also means no more per-command loop frequencies. But it otherwise allows for much simpler and less error-prone schedule code. Commands just need to use AsyncCommand.yield() instead of the old AsyncCommand.pause(); timing can be done with a timer, or use the withTimeout decorator

AsyncCommand cmd = someResource.run(() -> {
  var timer = new Timer();
  timer.start();
  while (!timer.hasElapsed(5.000)) {
    AsyncCommand.yield();
    doSomething();
  }
}).named("Demo");

One potential issue here is that there's no way to run cleanup logic on command interrupts. Continuations will just be kicked out of the scheduler if their command is canceled.

[Starlight220]

• I'm not sure about the protobuf/codegen/cmake changes, so if someone else can review that it'd be great.
• Suspend/resume needs some more discussion. Currently it's only in the design doc, so no need to block this PR on that. Removing it from the doc would be good, but not a PR blocker imo.
• This comment: #6518 (comment)

Other than those three points, looks good.

[Starlight220]

If someone with more experience with protobuf/cmake/codegen etc can look over these areas of this PR, that'd be great.

Other than that, this PR is good to be merged.

[Gold856]

Root CMakeLists.txt needs to have commandsv3 added as a subdirectory under the WITH_WPILIB check. Codegen looks good, if duplicative, and protobuf looks mostly good, although I would suggest having a static proto field for Mechanism. I'll also note our implementation of Protobuf clears the message beforehand, which likely makes the clears that we do here unnecessary.

[SamCarlberg]

although I would suggest having a static proto field for Mechanism

This is a deliberate choice. Mechanism and Command only make sense to be serialized in the context of a scheduler, which a static field would not be able to support.

[Gold856]

Command makes sense to not have a static proto field, but Mechanism just has a string, independent of the scheduler, so I figured it might be a better fit. I won't push too hard in either direction though. It saves, what, a handful of allocations per loop cycle?

[SamCarlberg]

Command makes sense to not have a static proto field, but Mechanism just has a string, independent of the scheduler, so I figured it might be a better fit. I won't push too hard in either direction though. It saves, what, a handful of allocations per loop cycle?

Yeah, that's negligible and I don't want people to try serializing bare mechanisms, especially if we expand the serialization to include scheduler-specific data such as default commands (which are scheduler-specific)

[Gold856]

One thing, otherwise, LGTM!