Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update Commands.java Add looseSequence() method #6639

Closed
wants to merge 11 commits into from
Closed

Update Commands.java Add looseSequence() method #6639

Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7a8cd5b
Update Commands.java
tom131313 May 20, 2024
8b36b83
Update Commands.java
tom131313 May 21, 2024
d5d39d3
Update wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command…
tom131313 May 21, 2024
0f28942
Update Commands.java
tom131313 May 21, 2024
5c858d3
Update Commands.java
tom131313 May 22, 2024
59e77bb
Update Commands.java
tom131313 May 23, 2024
c1576a2
Update Commands.java
tom131313 May 23, 2024
1fb1c9a
Update Commands.java
tom131313 May 23, 2024
0082985
Update Commands.java
tom131313 May 30, 2024
5963974
Merge branch 'wpilibsuite:main' into looseSequence
tom131313 May 31, 2024
4272774
Update Commands.java
tom131313 May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Commands.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,22 @@ public static Command sequence(Command... commands) {
return new SequentialCommandGroup(commands);
}

/**
* Runs individual commands in a series without grouped behavior.
*
* <p>Each command is run independently by proxy. The requirements of
* each command are reserved only for the duration of that command and
* are not reserved for an entire group process as they are in a
* grouped sequence.
*
* @param commands the commands to include in the series
* @return the command to run the series of commands
* @see #sequence(Command...) use sequence() to invoke group sequence behavior
*/
public static Command disjointSequence(Command... commands) {
return sequence(proxyAll(commands));
}

/**
* Runs a group of commands in series, one after the other. Once the last command ends, the group
* is restarted.
Expand All @@ -236,6 +252,24 @@ public static Command repeatingSequence(Command... commands) {
return sequence(commands).repeatedly();
}

/**
* Runs individual commands in a series without grouped behavior; once the last command ends, the series is restarted.
*
* <p>Each command is run independently by proxy. The requirements of
* each command are reserved only for the duration of that command and
* are not reserved for an entire group process as they are in a
* grouped sequence.
*
* @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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is sequence(Command...) this the right method to link to?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to a better reference.

* @see #disjointSequence(Command...)
* @see Command#repeatedly()
*/
public static Command repeatingDisjointSequence(Command... commands) {
return disjointSequence(commands).repeatedly();
}

/**
* Runs a group of commands at the same time. Ends once all commands in the group finish.
*
Expand All @@ -246,6 +280,23 @@ public static Command repeatingSequence(Command... commands) {
public static Command parallel(Command... commands) {
return new ParallelCommandGroup(commands);
}

/**
* Runs individual commands at the same time without grouped behavior and ends once all commands finish.
*
* <p>Each command is run independently by proxy. The requirements of
* each command are reserved only for the duration of that command and
* are not reserved for an entire group process as they are in a
* grouped parallel.
*
* @param commands the commands to run in parallel
* @return the command to run the commands in parallel
* @see #parallel(Command...) use parallel() to invoke group parallel behavior
*/
public static Command disjointParallel(Command... commands) {
new ParallelCommandGroup(commands); // check parallel constraints
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
  }

return parallel(proxyAll(commands));
}

/**
* Runs a group of commands at the same time. Ends once any command in the group finishes, and
Expand Down Expand Up @@ -273,6 +324,24 @@ public static Command deadline(Command deadline, Command... otherCommands) {
return new ParallelDeadlineGroup(deadline, otherCommands);
}

/**
* Runs individual commands at the same time without grouped behavior; when the deadline command ends the otherCommands are cancelled.
*
* <p>Each otherCommand is run independently by proxy. The requirements of
* each command are reserved only for the duration of that command and are
* not reserved for an entire group process as they are in a grouped deadline.
*
* @param deadline the deadline command
* @param otherCommands the other commands to include and will be cancelled when the deadline ends
* @return the command to run the deadline command and otherCommands
* @see #deadline(Command, Command...) use deadline() to invoke group parallel deadline behavior
* @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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return deadline(deadline, proxyAll(otherCommands));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the deadline command need to be proxied?

Copy link
Author

@tom131313 tom131313 May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the deadline command 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.

}

private Commands() {
throw new UnsupportedOperationException("This is a utility class");
}
Expand Down