Update Commands.java Add looseSequence() method #6639
Conversation
added looseSequence()
|
Needs unit tests and ports to other languages, but I think this is clearly a good feature - maybe we should extend to the other command group types? |
|
I'm not a fan of the naming; "loose" is loosely defined (pun intended). |
|
I think this is a common enough composition to warrant it, especially because we want to be absolutely sure that if a user is attempting a loose sequence, they reliably proxy all of the commands in the sequence. The biggest I like "loose", personally, but am flexible on naming. Agree on using |
|
|
|
I think I prefer |
|
Personally, I would be okay with |
|
We can play with other words to describe the Root words or derivatives that may fit better such as |
|
@tom131313 I like |
separatedSequence proxyAll
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Commands.java
Outdated
Show resolved
Hide resolved
…/Commands.java Co-authored-by: Jade <[email protected]>
It looks like in the current WPILib only the |
Huh interesting I would say they should be or defer should have it removed for consistencies sake but thats a seperate pr problem |
|
DeferredCommand accepts a |
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Commands.java
Outdated
Show resolved
Hide resolved
documentation
|
Overall, good work! Just don't want us to forget that this still needs unit tests and ports to other languages (including robotpy/robotpy-commands-v2 after this is merged). I'm also still a little iffy about the name- People might think there's some time gap between the commands- but it's still pretty good. The paragraph in the doc comment helps a ton, though- For the (few) people that do read it, there isn't any ambiguity. It might be nice though to add a brief note in the first sentence of the doc comment since some things only show the first sentence:
That's in #5606, which isn't merged yet. |
|
Re: naming, we could also try |
|
How about I think it's better to not bring more terms into the mess when we have plenty of terms already. The relation to |
We do already have a good term that we can use but in a negative way. So another name suggestion and cautionary tale: Commands can be executed in a sequence by a few ways such as a command triggers another command or timing paces a sequence such as press button "A" to schedule a command then press button "B" to schedule the next command or watch the clock to schedule a command then later on the clock schedule the next command. The The cautionary tale, of course, is the interrupt behavior of the |
new javadoc and new name for ungroupedSequence
I did this. I could have removed the |
I tried an |
|
@Starlight220 While I agree we shouldn't add too many new identifiers, I think a single new one here is appropriate because of the fundamentally abstract/complicated nature of The ideal situation would be one where It's a matter of weighing discoverability against absolute consistency. @tom131313 I did indeed mean parallel compositions - particularly the ones other than |
The problem was if the two (or more) parallel commands have a subsystem in common, then the results are bizarre, unexpected and seemingly wrong. I can add my |
|
@tom131313 You could enforce the disjoint requirements at construction-time, probably? This also suggests We also should probably enforce that the proxied commands all have at least one requirement for all of these, or else people might double-compose them and that can break badly. |
There was a problem hiding this comment.
add a brief note in the first sentence of the doc comment since some things only show the first sentence
I did this. I could have removed the
<p>as that seems to do the same thing in VSCode but I assumed you meant more words were needed before thecr/lf. And you'll notice a major overhaul to the whole method.
Thanks! Specifically, I meant a more detailed description in the first sentence (before the first period), but I'm glad to see you still added that detail!
You could enforce the disjoint requirements at construction-time, probably?
That's how a normal ParallelCommandGroup does it. (See ParallelCommandGroup.java#L51)
We also should probably enforce that the proxied commands all have at least one requirement for all of these, or else people might double-compose them and that can break badly.
Could you elaborate more on this? (Specifically what you mean by double-compose, and an example of bad usage would also be nice)
| * | ||
| * @param commands the commands to include in the series | ||
| * @return the command to run the series of commands | ||
| * @see #sequence() use sequence() to invoke group sequence behavior |
There was a problem hiding this comment.
| * @see #sequence() use sequence() to invoke group sequence behavior | |
| * @see #sequence(Command...) use sequence() to invoke group sequence behavior |
The parameter type(s) of a method must be specified. (See https://docs.oracle.com/javase/7/docs/technotes/tools/windows/javadoc.html#see)
disjointParallel
|
disjoint commands
| * | ||
| * @param commands the commands to include in the series | ||
| * @return the command to run the series of commands repeatedly | ||
| * @see #sequence(Command...) use sequenceRepeatedly() to invoke repeated group sequence behavior |
There was a problem hiding this comment.
Is sequence(Command...) this the right method to link to?
There was a problem hiding this comment.
Changed it to a better reference.
| * @see #parallel(Command...) use parallel() to invoke group parallel behavior | ||
| */ | ||
| public static Command disjointParallel(Command... commands) { | ||
| new ParallelCommandGroup(commands); // check parallel constraints |
There was a problem hiding this comment.
You should implement the check for disjoint requirements separately (probably in a private static convenience method)- It's self-documenting and (more importantly) will not register the commands as composed.
There was a problem hiding this comment.
You should implement the check for disjoint requirements separately (probably in a private static convenience method)- It's self-documenting and (more importantly) will not register the commands as composed.
I agree that my proposed validation that has a side-effect of registering the commands that then must be unregistered is a bit stinky but it appears to save duplicating a lot of code. Tracing the deadline validation, which may be the worst case, needs most of the following code pasted here from ParallelDeadlineGroup.java and CommandScheduler.java.
// from ParallelDeadlineGroup.java
public final void setDeadline(Command deadline) {
@SuppressWarnings("PMD.CompareObjectsWithEquals")
boolean isAlreadyDeadline = deadline == m_deadline;
if (isAlreadyDeadline) {
return;
}
if (m_commands.containsKey(deadline)) {
throw new IllegalArgumentException(
"The deadline command cannot also be in the other commands!");
}
addCommands(deadline);
m_deadline = deadline;
}
/**
* Adds the given commands to the group.
*
* @param commands Commands to add to the group.
*/
public final void addCommands(Command... commands) {
if (!m_finished) {
throw new IllegalStateException(
"Commands cannot be added to a composition while it's running");
}
CommandScheduler.getInstance().registerComposedCommands(commands);
for (Command command : commands) {
if (!Collections.disjoint(command.getRequirements(), m_requirements)) {
throw new IllegalArgumentException(
"Multiple commands in a parallel group cannot require the same subsystems");
}
m_commands.put(command, false);
m_requirements.addAll(command.getRequirements());
m_runWhenDisabled &= command.runsWhenDisabled();
if (command.getInterruptionBehavior() == InterruptionBehavior.kCancelSelf) {
m_interruptBehavior = InterruptionBehavior.kCancelSelf;
}
}
}
// from CommandScheduler.java
private final Map<Command, Exception> m_composedCommands = new WeakHashMap<>();
/**
* Register commands as composed. An exception will be thrown if these commands are scheduled
* directly or added to a composition.
*
* @param commands the commands to register
* @throws IllegalArgumentException if the given commands have already been composed, or the array
* of commands has duplicates.
*/
public void registerComposedCommands(Command... commands) {
Set<Command> commandSet;
try {
commandSet = Set.of(commands);
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException(
"Cannot compose a command twice in the same composition! (Original exception: "
+ e
+ ")");
}
requireNotComposedOrScheduled(commandSet);
var exception = new Exception("Originally composed at:");
exception.fillInStackTrace();
for (var command : commands) {
m_composedCommands.put(command, exception);
}
}
/**
* Requires that the specified command hasn't already been added to a composition, and is not
* currently scheduled.
*
* @param command The command to check
* @throws IllegalArgumentException if the given command has already been composed or scheduled.
*/
public void requireNotComposedOrScheduled(Command command) {
if (isScheduled(command)) {
throw new IllegalArgumentException(
"Commands that have been scheduled individually may not be added to a composition!");
}
requireNotComposed(command);
}
/**
* Requires that the specified commands have not already been added to a composition, and are not
* currently scheduled.
*
* @param commands The commands to check
* @throws IllegalArgumentException if the given commands have already been composed or scheduled.
*/
public void requireNotComposedOrScheduled(Collection<Command> commands) {
for (var command : commands) {
requireNotComposedOrScheduled(command);
}
}
/**
* Requires that the specified command hasn't already been added to a composition.
*
* @param commands The commands to check
* @throws IllegalArgumentException if the given commands have already been composed.
*/
public void requireNotComposed(Command... commands) {
for (var command : commands) {
var exception = m_composedCommands.getOrDefault(command, null);
if (exception != null) {
throw new IllegalArgumentException(
"Commands that have been composed may not be added to another composition or scheduled "
+ "individually!",
exception);
}
}
}
/**
* Requires that the specified commands have not already been added to a composition.
*
* @param commands The commands to check
* @throws IllegalArgumentException if the given commands have already been composed.
*/
public void requireNotComposed(Collection<Command> commands) {
requireNotComposed(commands.toArray(Command[]::new));
}
/**
* Whether the given commands are running. Note that this only works on commands that are directly
* scheduled by the scheduler; it will not work on commands inside compositions, as the scheduler
* does not see them.
*
* @param commands the command to query
* @return whether the command is currently scheduled
*/
public boolean isScheduled(Command... commands) {
return m_scheduledCommands.containsAll(Set.of(commands));
}| * @throws IllegalArgumentException if the deadline command is also in the otherCommands argument | ||
| */ | ||
| public static Command disjointDeadline(Command deadline, Command... otherCommands) { | ||
| new ParallelDeadlineGroup(deadline, otherCommands); // check parallel deadline constraints |
There was a problem hiding this comment.
Same as disjointParallel(): You should implement the check for disjoint requirement separately (probably in a private static convenience method)- It's self-documenting and (more importantly) will not register the commands as composed.
| */ | ||
| public static Command disjointDeadline(Command deadline, Command... otherCommands) { | ||
| new ParallelDeadlineGroup(deadline, otherCommands); // check parallel deadline constraints | ||
| return deadline(deadline, proxyAll(otherCommands)); |
There was a problem hiding this comment.
Does the deadline command need to be proxied?
There was a problem hiding this comment.
Does the
deadlinecommand need to be proxied?
Maybe but doing that here doesn't help.
If the deadline is an individual command then "no" it doesn't need an asProxy() since it's going to end the group and release the requirements at that time with no possibility to release the requirements earlier.
If the deadline is a composite group command then "yes" it needs the Proxy treatment because the earlier part of the group could end and those requirements released early for the default command to run while the later part of the deadline runs with different requirements.
The Proxy for the deadline as a grouped command has to be added when that group is made (not when used or referenced later) because after its creation later additional wrapping groupings don't know about the inner groups.
This points out again that the Proxy has to be carefully considered minimally applying it starting from the inner commands to the outer commands.
Edit after more thought that I might not understand the objective of this disjoint work and I gave a somewhat wrong answer above.
I have mostly been thinking about the disjoint of a single group. The deadline portion doesn't need a proxy if an individual command and also race would not need proxies for individual commands but for groups would be useful. I now see that to assure disjointedness without much thought by the user, everything in sight needs to have proxy - groups and individual commands in all the various wrappers - race, parallel, sequence, deadline. This is because any of these constructs could be used in combination within any other and the only way to assure independence is make everything independent.
Likely this results in a lot of redundant and inefficient wrappings (of wrappings of wrappings). So we're back to "hand-tuning" the requirements by judicious addition of asProxy() if you want thoughtful efficiency.
So yes - proxy everything if that's the objective and I now assume it is.
disjoints
Adding the Otherwise a composite command has to be wrapped in the And if the We can cover the case of adding I see my comments on the |
|
The Next step? @Override
public void execute() {
if (m_ended) {
m_ended = false;
m_command.initialize();
m_delayFirstExecuteIterations = 0; //<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
}
if (m_delayFirstExecuteIterations < 3) { //<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
m_delayFirstExecuteIterations++; //<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
return; //<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
}//<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
m_command.execute();
if (m_command.isFinished()) {
// restart command
m_command.end(false);
m_ended = true;
}
} |
Could you elaborate more on this? (What is the code being run (including any added or changed method definitions), what is the expected behavior, and what is the actual behavior?) |
This or equivalents fail: Maybe it happens because the Proxied command (say |
|
Thanks for noticing that bug! It turns out to be a fairly involved bug, and I think the cleanest fix would be #6593. This is what happens during the call to
Note that both |
added more disjoint methods updated javadocs
add proxyAll() restricted to commands with requirements
I added this check for restriction by changing the I believe this satisfies all the comments on this PR except possibly the validation scheme that registers then unregisters composed commands. Changing that is a daunting task for me to do elegantly so I'll leave that for others again to decide how to do it. I think a complete set of Java [The |
added
Commands.looseSequence()Comparable to
Commands.sequence()to create aSequentialCommandGroupEach command is run individually by proxy. The requirements of each command are only for the duration of that command and are not required for the entire group process. This allows the possibility of a subsystem default command to run within the sequential group.
Note that the name
looseSequencerefers to a loose coupling of commands or loosening of the subsystem requirements for the duration of the group. The sequence is still determined by the order of the parameters.