PSA: Command groups can self-cancel when using `ProxyCommand` or `asProxy`

This has impacted a few teams so far this year, and has been a common topic of discussion on the FRC discord - but there’s no thread about it here, so I’m making one for visibility:

ProxyCommand interacts in a subtle/surprising way with the Command-based requirement system, which guards against simultaneous access to the same Subsystem instance by two different commands. This can result in command groups containing ProxyCommands self-cancelling under certain circumstances:

A command group containing a ProxyCommand will self-cancel if the proxied Command shares a requirement with the rest of the group. This occurs because the proxied command (which runs directly on the scheduler) will demand the Subsystem resource that the command group is currently reserving, interrupting the group that originally scheduled it.

ProxyCommand was originally intended to allow a command group to interact with a command running on a different subsystem, without pulling that subsystem into the group’s requirements. It works by scheduling the “proxied” command normally, and piping the runtime state of that command back to the group.

Teams have been using the fact that ProxyCommand can create the proxied command at runtime from a supplier to use it instead to accomplish deferred construction of a command conditioned on runtime state. This works in isolation, but if used in a command group it is very likely to encounter the behavior described above.

For more information, see the open github ticket to add a DeferredCommand that safely adds the functionality teams want from ProxyCommand.

5 Likes

I’ve definitely foot-gunned a few times this season with ProxyCommand.

How do you plan to handle potential requirement differences between the declared DeferredCommand and the command it generates via Supplier?

Here’s a motivating example if anybody is interested.

1 Like

We don’t. The requirements of the actual command provided by the Supplier will be ignored, and the requirements declared on the DeferredCommand itself will be canonical.

This means you’re stuck with an extra bit of verbose injection if you use the class directly, but the addition of a new subsystem factory .defer() that declares this as a requirement implicitly will work correctly “by default.”

3 Likes

That’s the reason that non-proxied deferring was deprecated, and is a good question and point of discussion for future plans of DeferredCommand.

We (WPILib) can decide stuff, but it’s exactly this type of thing that feedback/input from teams is important: most of us aren’t strongly affiliated with teams at the moment, and we don’t experience the rushed style of in-season development or encounter use-cases that teams do. Thus, it sometimes happens that we make design decisions that aren’t always well-tuned. Frankly, I’d say this was one such decision. That’s why input from teams/the community is so important – it helps us tune things better for what teams need and improve developer/team experience.

So, any input would be valuable: on GitHub, on other platforms (CD, discord) it’s also very helpful and perhaps more suited for discussion but has a chance of being missed.

In this specific case, DeferredCommand requirements, some possible options (and others are welcome!):

  • an explicit non-defaultable requirements parameter (ie a Set rather than vararg)
  • maybe some sort of check when the supplier is polled? Though I can imagine that there’s a usecase that this sort of check would harm.
  • nothing is also an option. Personally, I’m a bit wary of it.

Opinions? If there are more ideas we’ve missed, theyre welcome.

2 Likes

Yes, this bit us. And there does not seem to be an actual alternative.

We need something like a “proxy” command, because we create Trajectories on the fly, and then want to use a pre-existing/common Command to run it.

We ended up writing our own. In places where it looks like we need a Proxy, we have the upper Command just call the methods of the “sub” command, without actually scheduling that subcommand. Works very well.

1 Like

This is what the proposed DeferredCommand would do.

3 Likes

Could you use a trajectory supplier or something? For some of our on the fly generation, the command takes a Supplier<PathPoint>. The only problem we have is when we want to have a SequentialCommandGroup where we aren’t allowed to override the initialize.

First one doesn’t really help, since the user still gets to ignore it if they want.

Second one is good, probably just a warning and not an assert/crash, though? Use cases where you don’t know what your requirements are going to be ahead of time… possible but you’re kinda asking for trouble. Users can do ConditionalCommand if they don’t know which command they want to run so that should handle most of those use cases.

Nothing as an option. IDK maybe. Better than ProxyCommand at the moment.

The user being able to ignore it might be good, tbh. In my experience, most cases of missing requirements are mistakes rather than conscious omission. Accepting a Set rather than a varargs forces the user to think about requirements, even if they do pass in an empty set. The drawback is that it’s a bit more verbose, though it shouldn’t be too bad with Set.of().

That’s another open question where community input would be valuable. But yes, I suppose there should have a way to workaround/suppress that check.

My feeling is that the framework was never designed for command requirements to mutate at runtime, and we shouldn’t enable that behavior without both good cause and certainty that it won’t cause other bugs elsewhere. At the moment I doubt both of these can be found.

If requirements don’t mutate at runtime, a runtime check is inappropriate.

1 Like

The way it does that is by breaking a pattern followed literally everywhere else that subsystem requirements are varargs. That’s not good software design IMO

1 Like

lol this happened to us. Had to make our own alternative.

1 Like

I know of at least 4 teams now that have rolled their own solution to this. It needs to be fixed for 2024.

2 Likes

However, why not change everything to function off of sets? I’m not a WPILib contributor, but that seams like it would just be just some changes to command base and then just tedious changes to the rest of commands? Certainly not fun work, but as @Starlight220 said, it would make it much harder to accidentally put in no requirements for a command.

This topic was automatically closed 365 days after the last reply. New replies are no longer allowed.