I’m trying to make a parallel command group and I need to specify the order that the parallel commands run in. E.g. I expect the following to print “Hello World” repeatedly, not "WorldHello ":
new ParallelCommandGroup(
new RunCommand(() -> System.out.print("Hello "),
new RunCommand(() -> System.out.println("World")
);
But looking closely, the implementation of ParallelCommandGroup uses a HashMap, not a LinkedHashMap, which does not guarantee the iteration order of the commands to be the same as the insertion order. So this would not work for my case. Then I looked at ParallelRaceGroup and ParallelDeadlineGroup, and even ScheduleCommand, and I found that they all use either HashMap or HashSet which does not guarantee order. The master map in CommandScheduler does use a LinkedHashMap, so I came up with this solution:
new InstantCommand(() -> {
new RunCommand(() -> System.out.print("Hello ").schedule();
new RunCommand(() -> System.out.println("World").schedule();
});
It works but I don’t like this solution because now I can’t use the the features in the parallel command groups to cancel these commands.
No, I do not want to run the next command when the first finishes. I gave a simple case, but in the actual code both need to run at the same time. As an actual example, in an auto, I’d like to have one command to update the robot state odometry, and the next command to use that calculation to drive.
@jdao’s solution would not work because the lambdas will only run a single time, not once every loop.
My suggestion is to do this in the drive subsystem itself. You can use the periodic function of a subsystem to update odometry automatically for you, no need for a command to do it.
I’ve thought about that. It would definitely make this situation work, but I don’t see that as an inherent action of the subsystem. E.g. I might not want it to run while the robot is disabled. I’d rather be able to control when it gets run
I mean, I get what you’re saying, but that’s not really what parallel means… parallel commands should be completely isolated from each other so they can truly run in parallel. If there’s a dependency between them, then one NEEDS to run before the other, which implies sequential behavior.
Given your specific example, I agree with @Julian_C. You’re much better off doing it in the periodic method of the subsystem, which is exactly how the WPILib example does it. This way your odometry update will happen every loop, and you can just use it as-needed. The subsystem periodic methods are called by the scheduler immediately before the “active” commands run, so that will guarantee your order without having to worry about it explicitly
Is there another reason why you would want it to be a command and not as part of a periodic call? The pose is similar to getting a sensor reading from the robot and I don’t see any reason why you would need it to stop updating. Also stopping and starting it again may introduce strange behavior (e.g. angle when it stopped was 0 and when you start it again its now 180 and the robot overcompensates). Another way of doing this would be to reset your pose every time you need to restart the pose updates.
You could design your commands to pass in a delayTime so that eventhough they don’t start at the exact time you could delay one by xx miliseconds or xx cycles in your isFinished or execute loop.
new ParallelCommandGroup(
new RunCommand(() -> System.out.print("Hello ")),
new SequentialCommandGroup(
new WaitCommand(0.1), // wait 0.1 seconds
new RunCommand(() -> System.out.println("World"))
)
);
For the record, I’m NOT recommending this approach…
This straight up doesn’t work - if the SequentialCommandGroup is runing first it will still cause “World” to be printed out before "Hello " after the 0.1 seconds
I’m now convinced that for this exact purpose, this is probably the best way to do it. I am more asking whether the non-guarantee of order in the parallel command groups is a design choice for the framework or an implementation detail. I can always make my own implementation of command groups if I need it.
There’s a reason I added the disclaimer at the bottom…
I mean, it’s a race condition… It’ll work assuming both RunCommand don’t require the same subsystem and can actually run in parallel. In that case, the scheduler will see 2 commands that need to run, a RunCommand, and a SequentialCommandGroup. It’ll run one then the other. When it runs the SeqCmdGrp, it’ll see one to run (WaitCommand). After a few cycles (0.1s), the WaitCommand will be done, and it’ll run the 2nd RunCommand. The key here is that RunCommands don’t take more than one cycle to run, so as long as your WaitCommand is longer than one cycle (0.02s), it should work fine.
Again, I’m not recommending this, because it still relies on a race condition and guesswork with the timing.
The nature of parallelism is that order literally doesn’t matter. I’m inclined to say it was an explicit design choice. At the risk of sounding argumentative, I still fail to see why this should be needed.
I’m not claiming the command-based framework is perfect, but it meets 99.<whatever>% of the need.
It’s not a race condition - The parallel command groups are never multi-threaded and never actually run in parallel. Here is the execute() function of ParallelCommandGroup:
for (Map.Entry<Command, Boolean> commandRunning : m_commands.entrySet()) {
if (!commandRunning.getValue()) {
continue;
}
commandRunning.getKey().execute();
if (commandRunning.getKey().isFinished()) {
commandRunning.getKey().end(false);
commandRunning.setValue(false);
}
}
It just loops over a list of the commands and runs them. So whatever order that the set has at the beginning, it will always run in that order.
It IS a race condition in that the order it’s executed is non-deterministic with respect to the order it’s added to the ParallelCommandGroup. I’m well aware of how the commands are run.
Here’s how my code snippet above should run (ignoring any other commands in the system)
Ah, yes, I stand corrected on that point. In my mind, I was treating it like an InstantCommand, which would run once and end. Apologies for the confusion there.
You’re correct that that order is undefined, so you could get either. I think my previous point about parallelism still applies (dependencies between the two MUST be handled sequentially)
If the order is that important to you, I would recommend creating your own class that has the behavior you want. You could probably copy most if not all of the code from ParallelCommandGroup.
I’d also recommend opening an issue on github so you can see what the WPI devs think of changing the implementations to use a LinkedHashMap and LinkedHashSet. Also, @Oblarg could comment on this as he’s the one responsible for the command framework re-write.
This isn’t something they can change this season, though, because this might cause a race condition in someone else’s code that’s already relying on the order of the HashMap/HashSet.
Might I suggest this should actually be one command? Since there is a dependency that both run at the same time, and in a specific order. This seems like one operation of the robot:
new RunCommand(() -> {
System.out.print("Hello ");
System.out.println("World");
}
Ah, yes, I stand corrected on that point. In my mind, I was treating it like an InstantCommand, which would run once and end. Apologies for the confusion there.