Scheduler execute-isFinished order causing unwanted behavior

[nimrod46]

Hello ppl, throughout my years here we kept using somewhat the same simple logic to stop a motor using the command-based approach:

execute: 
    setSpeed(1)

isFinished:
    return isBallIdentified()

But this approach always introduced the same basic problem: The way command schedule works is first to execute the command and only then checking isFinished which causes the motor to stutter in the best case, I am not sure why this is implemented like this, maybe I always missed something there.

I think it's very reasonable to assume that a command which is scheduled to run while its isFinished condition is already met will only be initialized without executing at all.

Of course, the problem is a bit more complicated than that, in recent years you introduce some major improvements which made this "bug" even more noticeable such as whileActiveContinuous, parallelDeadlineGroup (which also execute and then checks isFinished), and new features to come such as #4009.

Hoping to hear your thoughts about it!

[Starlight220]

Originally (ie when the command lifecycle was first devised in v1), the thought was that initialize would contain only initialization logic; perhaps fill the role of a quasi-constructor that can reset the command state without allocating a new object. Similarly, only execute would contain the functionality (ie what the command actually does), with the assumption that the command end state is definitely not reached until the command actually did something (ie an execute call) so there's no point in checking isFinished before execute.

This assumption made sense in commands v1, where all commands were team-written. Nowadays, with InstantCommand, StartEndCommand, etc. in widespread use (and controlling motors with MotorSafety disabled), this assumption might need correction. These past few days I've run into this myself with my team's code, and having to resort to tricks like the following to implement "preconditions" for commands:

new ConditionalCommand(
  commandThatDoesSomething,
  new InstantCommand(), // aka "do nothing"
  precondition
)

Due to how InstantCommand only has a meaningful initialize, passing it through the scheduler queue might be a waste. Maybe add an isFinished call after calling initialize and if it returns true, immediately call end and not add the command to the execution queue? This might be a relatively big behavioral break though, so I'm not sure.

[Oblarg]

I'm not sure I understand the complaint here; if you don't want the command to run at all in certain cases then you can add a condition to the trigger.

[nimrod46]

I'm not sure I understand the complaint here; if you don't want the command to run at all in certain cases then you can add a condition to the trigger.

@Oblarg My point is that you never want to run the command if the isFinished condition is reached not even once, maybe it made sense to make sure execute is called once before isFinished is getting checked but today when you have built-in solutions for writing repeating commands (i.e using whileActiveContinuous) I think it's only causing problems, sure you can add checks to all relevant commands but no one wants to code like this.

If the desired behavior is really to call execute once regardless of its isFinished state I think adding a call to execute in the initialize makes much more sense than hiding this behavior behind and affecting every type of command.

[Starlight220]

I think that the unless() decorator (added in #4244) sufficiently provides an answer for the use case OP mentioned, and we're not looking to confusingly break a core part of how command-based has worked since its conception. So I'm closing this; if there's anything I'm missing, reopen this or open a new issue.