[commands] Clarified error messages for parallel composition commands #7161

Daniel1464 · 2024-10-04T00:27:07Z

Addresses Issue #6353.

…wpilibsuite#6353)

github-actions · 2024-10-04T00:27:20Z

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

Daniel1464 · 2024-10-04T01:32:14Z

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

robotpy/robotpy-commands-v2#73

rzblue · 2024-10-04T15:39:45Z

/format

rzblue · 2024-10-04T15:50:14Z

You'll need to install and run wpiformat yourself, the PR format command will not work if the PR is based on your main branch. For the future, it's best to make a branch for each PR.

KangarooKoala

We should only include the subsystems that are actually conflicting, since if we just report which ones the added command requires, we aren't actually communicating information that couldn't be found in other ways.

…between commands in a parallel group

Starlight220 · 2024-10-05T19:18:45Z

wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/ParallelCommandGroup.java

+      var currentRequirements = getRequirements();
+      if (!Collections.disjoint(command.getRequirements(), currentRequirements)) {
+        String requirementsStr = command.getRequirements()
+          .stream()


The Stream API is really inefficient (for these use cases) and is generally avoided in the WPILib codebase

I feel like it’s probably fine to use the stream api here, even if it’s slow, because this code will only run when an error happens anyways

Keeping the Java and C++ implementations similar will make it easier to maintain, though.

KangarooKoala

It'd be good to extract the logic into a utility function, but personally I'm fine with this PR getting merged as it is to avoid further delays. Actual WPILib maintainers will need to weigh in, though.

Starlight220 · 2024-11-06T06:05:46Z

wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/ParallelDeadlineGroup.java

        throw new IllegalArgumentException(
-            "Multiple commands in a parallel group cannot require the same subsystems");
+          String.format(
+            "Command %s could not be added to this ParallelCommandGroup"


I suggest avoiding the word ParallelCommandGroup as that class is only one of the ones relevant. "Parallel composition" would be more accurate.

Daniel1464 · 2024-11-07T02:54:30Z

Alright just resolved the naming issues brought up by @Starlight220 and changed the method to a utility method.

KangarooKoala

I'm a bit iffy about some things:

ensureDisjointRequirements() doesn't feel the best to me (maybe checkDisjointRequirements() would be nicer?). Ultimately I personally am fine if it stays with the name, but I would like a better name if we can think of one.
I'm not the happiest about C++ having a fairly different implementation- It's largely similar enough that I think okay, but it's suboptimal. The lack of a retainAll() equivalent is problematic, though.

KangarooKoala · 2024-11-07T14:34:59Z

wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Commands.java

+   * @param parallelGroup The parallel group command.
+   * @param toAdd The command that will be added to the parallel group.
+   */
+  public static void ensureDisjointRequirements(Command parallelGroup, Command toAdd) {


Where do we want to put this method? Right now Commands consists solely of command factories, and it'd be nice if we kept it that way.

Here's some other options I thought of:

Protected non-static method of Command.

Public static method of Command.

Public static method of ParallelCommandGroup (which the others use).

#6032 is somewhat relevant for the placement, since if that PR was merged we could just make it a private method of ParallelCommandGroup.

Yeah a protected non-static Command method is a great idea honestly, didn't think of that.
Currently Command doesn't have any static methods so I was hesistant to make it a public method of that, and ended up choosing Commands instead(which i didnt really like either but yeah)

…Command

KangarooKoala · 2024-11-07T17:08:35Z

wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java

+   * Throws an error if a parallel group already shares
+   * one or more requirements with a command
+   * that will be added to it.


As an FYI, this will need to be formatted (and since your main branch is a PR branch, /format on your PRs won't work). You can look at the comment-command .yml file to see what steps you should run to format. (pip3 install wpiformat==2024.45, wpiformat, and ./gradlew spotlessApply)

yeah forgot to run /format

KangarooKoala · 2024-11-07T17:20:07Z

wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java

+  protected void ensureDisjointRequirements(Command parallelGroup, Command toAdd) {
+    var sharedRequirements = new HashSet<>(parallelGroup.getRequirements());


Suggested change

protected void ensureDisjointRequirements(Command parallelGroup, Command toAdd) {

var sharedRequirements = new HashSet<>(parallelGroup.getRequirements());

protected void ensureDisjointRequirements(Command toAdd) {

var sharedRequirements = new HashSet<>(getRequirements());

Might as well use that it's non-static.

KangarooKoala · 2024-11-07T17:20:43Z

wpilibNewCommands/src/main/native/include/frc2/command/Command.h

 */
-bool RequirementsDisjoint(Command* first, Command* second);
+void EnsureDisjointRequirements(Command* parallelGroup, Command* toAdd);


We could move this into the Command class too.

wpilibNewCommands/src/main/native/cpp/frc2/command/Command.cpp

Co-authored-by: Joseph Eng <[email protected]>

Daniel1464 · 2024-11-08T16:47:34Z

Just added the requested changes

KangarooKoala

Overall, great work! Unfortunately, it can take a while to finish all of the little details.

Formatting still needs to be done, and I'm about 90% happy with the name ensureRequirementsDisjoint()- It works, but requireRequirementsDisjoint() or requireDisjointRequirements() might fit the existing naming better (CommandScheduler.requireNotComposed() and CommandScheduler.requireNotComposedOrScheduled()). The dual use of require is a bit annoying though.

@rzblue and @Starlight220- Thoughts on the removal of frc2::RequirementsDisjoint without deprecation?

Also, tests would be nice but tricky to do, so I'm not 100% sure if they're worth implementing.

[commands] Clarified error messages for parallel composition commands (…

1fcdb26

…wpilibsuite#6353)

Daniel1464 requested a review from a team as a code owner October 4, 2024 00:27

Daniel1464 closed this Oct 4, 2024

FRC_MakeError usage correction

f66ee4f

Daniel1464 reopened this Oct 4, 2024

Daniel1464 mentioned this pull request Oct 4, 2024

[commands] Clarified error messages for parallel composition commands robotpy/robotpy-commands-v2#73

Open

Added formatting corrections

2651199

KangarooKoala reviewed Oct 4, 2024

View reviewed changes

Daniel1464 and others added 4 commits October 5, 2024 10:43

Changed error message to only mention infringing/shared requirements …

e704b58

…between commands in a parallel group

Formatting part 2

148fd34

Merge branch 'wpilibsuite:main' into main

c971a81

Changed "==" to "!="

a95e41f

Daniel1464 requested a review from KangarooKoala October 5, 2024 18:23

Starlight220 reviewed Oct 5, 2024

View reviewed changes

Daniel1464 requested a review from Starlight220 October 6, 2024 21:53

Daniel1464 and others added 3 commits October 27, 2024 19:17

Added vector #include calls

aed05dc

Merge branch 'wpilibsuite:main' into main

c5432ed

Replaced stream api usage

09cce4b

KangarooKoala reviewed Nov 6, 2024

View reviewed changes

Starlight220 reviewed Nov 6, 2024

View reviewed changes

Reduced boilerplate

178ab5b

Daniel1464 requested review from KangarooKoala and Starlight220 November 7, 2024 02:54

Fixed syntax err

6712d4f

KangarooKoala reviewed Nov 7, 2024

View reviewed changes

changed ensureDisjointRequirements to a protected instance method of …

3d276de

…Command

Daniel1464 requested a review from KangarooKoala November 7, 2024 15:46

KangarooKoala reviewed Nov 7, 2024

View reviewed changes

Daniel1464 added 3 commits November 7, 2024 15:04

Standardized c++ and java usage

b5e15d3

Fixed small issue

96be9c9

Fixed small issue

e713965

Daniel1464 requested a review from KangarooKoala November 7, 2024 20:46

KangarooKoala reviewed Nov 8, 2024

View reviewed changes

wpilibNewCommands/src/main/native/cpp/frc2/command/Command.cpp Outdated Show resolved Hide resolved

Fixed minor formatting issue

05f8fc6

Co-authored-by: Joseph Eng <[email protected]>

Daniel1464 requested a review from KangarooKoala November 8, 2024 16:47

KangarooKoala reviewed Nov 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[commands] Clarified error messages for parallel composition commands #7161

[commands] Clarified error messages for parallel composition commands #7161

Daniel1464 commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

Daniel1464 commented Oct 4, 2024 •

edited

Loading

rzblue commented Oct 4, 2024

rzblue commented Oct 4, 2024

KangarooKoala left a comment •

edited

Loading

Starlight220 Oct 5, 2024

Daniel1464 Oct 6, 2024

KangarooKoala Oct 28, 2024

KangarooKoala left a comment

Starlight220 Nov 6, 2024

Daniel1464 commented Nov 7, 2024

KangarooKoala left a comment

KangarooKoala Nov 7, 2024

Daniel1464 Nov 7, 2024

KangarooKoala Nov 7, 2024

Daniel1464 Nov 7, 2024

KangarooKoala Nov 7, 2024

KangarooKoala Nov 7, 2024

Daniel1464 commented Nov 8, 2024

KangarooKoala left a comment

		protected void ensureDisjointRequirements(Command parallelGroup, Command toAdd) {
		var sharedRequirements = new HashSet<>(parallelGroup.getRequirements());

[commands] Clarified error messages for parallel composition commands #7161

Are you sure you want to change the base?

[commands] Clarified error messages for parallel composition commands #7161

Conversation

Daniel1464 commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

Daniel1464 commented Oct 4, 2024 • edited Loading

rzblue commented Oct 4, 2024

rzblue commented Oct 4, 2024

KangarooKoala left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KangarooKoala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daniel1464 commented Nov 7, 2024

KangarooKoala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daniel1464 commented Nov 8, 2024

KangarooKoala left a comment

Choose a reason for hiding this comment

Daniel1464 commented Oct 4, 2024 •

edited

Loading

KangarooKoala left a comment •

edited

Loading