-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Scheduler #78
Scheduler #78
Conversation
* returns {@code true}. | ||
* @return {@code true} if this {@link Command} is complete; {@code false} otherwise | ||
*/ | ||
public boolean execute(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the sequence of calls?
- always call
initialize()
once - always call
firstExecute()
once, and if it returnsfalse
then:- repeatedly call
execute()
until it returns 'true'
- repeatedly call
- always call
finalize()
once
What is the difference between initialize()
and firstExecute()
? Could initialize()
return a boolean that says whether execute()
should be called (or whether the command is finished, to be symmetric with execute()
)? If so, then this would be the sequence of calls:
- always call
initialize()
once, and if it returnsfalse
then:- repeatedly call
execute()
until it returns 'true'
- repeatedly call
- always call
finalize()
once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, would it be useful to change the signature of execute
to include the integer number of milliseconds since the command was first started; e.g., execute(int elapsedMillis)
? Should initialize
be changed as well? That might make it very easy to implemented commands that timeout.
Or, how about if we add:
public default int maximumDurationInMillis() {
return 0;
}
Implementations for time-limited commands could override this and return the total duration; the CommandRunner might then track the elapsed times automatically (see below).
Overall, this is a really great start. We have to figure out the requirements, but the time-limited commands can probably easily be solved by having |
* @param command the {@link Command} to wrap | ||
* @return the {@link CommandRunner} wrapping that {@link Command} | ||
*/ | ||
public static CommandRunner ex(Command command) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure these should be static, but CommandGroup
doesn't fit the definition of Command
, so we can't use the architecture WPILib
uses. The best I've come up with is a separate add()
in Scheduler
that takes a CommandGroup
. CommandGroup
's constructor can be then be protected
and take a CommandRunner
so that any class that extends it is forced to synthesize a command group using the methods in CommandGroup
. Then we can drop their visibility to protected
.
WIP - Don't merge
I think it works, a forked command ends up getting wrapped something like three times, but I think the logic works out.