allwpilib icon indicating copy to clipboard operation
allwpilib copied to clipboard

scheduling from Command.end?

Open truher opened this issue 10 months ago • 7 comments

in Command.end(), theres a comment that advises against using this method to schedule another command, suggesting andThen(), which I think would only work for a static sequence. I had in mind to use the end() method to do ad hoc command sequencing, and it seems like it should work fine. is there a reason not to?

truher avatar Apr 23 '24 03:04 truher

What is scheduling from end going to gain you compared to using factory methods? You might be looking for select or either

spacey-sooty avatar Apr 23 '24 04:04 spacey-sooty

maybe? how do I use select() or either() to assemble an indefinite series of arbitrary commands?

truher avatar Apr 23 '24 04:04 truher

Something like .run(select(...)) could work?

spacey-sooty avatar Apr 23 '24 05:04 spacey-sooty

i see, you mean to put the selection logic in the command selector. how do you trap the command completion to select the next one? I think my main question is actually the reasoning behind the comment. why say that chaining commands through end() is a bad idea if it seems to work fine?

truher avatar Apr 23 '24 05:04 truher

I presume that it isn't recommended as it adds a side effect to the command that is essentially hidden. When you use a factory command you explicitly state that after this command ends this command should run. Compared to end where you are implicitly moving on to it which could cause unintuitive behaviour using something like factory methods as well.

spacey-sooty avatar Apr 23 '24 05:04 spacey-sooty

ok thanks. maybe this is more of a question about canon rather than the specific Command.end idea. I'll ask over on CD.

truher avatar Apr 23 '24 14:04 truher

Looks like that line was originally added in #2450 (to resolve #2441). However, the behavior that needed to be fixed (scheduling a command with shared requirements from end() would cause the original command to be canceled, calling end() again in an infinite loop) has been fixed in #5470, so we probably don't need that comment anymore.

Regarding https://www.chiefdelphi.com/t/canonical-command-chaining/464156 (which I'm guessing is where you asked on CD):

  • Overriding Command.end() (or equivalently, wrapping in onEnd()) to get a callback when the command finishes should work, though if you run into bugs a) please file a bug report to help us improve! and b) try polling Command.isScheduled() periodically to detect when it is no longer scheduled.
  • Depending on your framework, you also might be able to use cmd.andThen(Commands.defer(() -> getNextCommand(), Set.of(<requirements>))) instead of wrapping in cmd.onEnd(() -> scheduleNextCommand()) or overriding cmd.end() to schedule the next command, but I suspect that would run into memory issues since it would create subcommands but not be able to garbage collect old commands (since those are wrappers around the subcommands). bovlb's CD suggestion of andThen(Commands.runOnce(() -> scheduleNextCommand())) wouldn't run into those memory issues, though since onEnd() should work, there wouldn't be an advantage compared to onEnd(() -> scheduleNextCommand()).
  • I would advise against using onCommandFinish() and/or onCommandInterrupt(), since even if you get both the finish and interrupt combination, you still will not be notified of a command ending within a composition (since the callbacks are managed by CommandScheduler, while command compositions manage the composed commands themselves).

KangarooKoala avatar Apr 23 '24 16:04 KangarooKoala