Team2890/allwpilib
4
0
Fork 0
mirror of https://github.com/wpilibsuite/allwpilib synced 2026-06-28 02:11:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

[commands] Fix compose-while-scheduled issue and test all compositions (#5581)

Browse Source
This commit is contained in:
Ryan Blue
2023-12-23 15:12:13 -05:00
committed by GitHub
parent aeb1a4aa33
commit ef1cb3f41e
20 changed files with 146 additions and 83 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandScheduler.java
View File

@@ -585,7 +585,7 @@ public final class CommandScheduler implements Sendable, AutoCloseable {
*/
public void registerComposedCommands(Command... commands) {
var commandSet = Set.of(commands);
requireNotComposed(commandSet);
requireNotComposedOrScheduled(commandSet);
var exception = new Exception("Originally composed at:");
exception.fillInStackTrace();
for (var command : commands) {
@@ -617,7 +617,7 @@ public final class CommandScheduler implements Sendable, AutoCloseable {
}
/**
* Requires that the specified command hasn't been already added to a composition.
* 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.
@@ -635,7 +635,7 @@ public final class CommandScheduler implements Sendable, AutoCloseable {
}
/**
* Requires that the specified commands not have been already added to a composition.
* 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.
@@ -644,6 +644,34 @@ public final class CommandScheduler implements Sendable, AutoCloseable {
requireNotComposed(commands.toArray(Command[]::new));
}
/**
* 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);
}
}
/**
* Check if the given command has been composed.
*

23
wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp
View File

@@ -477,6 +477,29 @@ void CommandScheduler::RequireUngrouped(
}
}
void CommandScheduler::RequireUngroupedAndUnscheduled(const Command* command) {
if (IsScheduled(command)) {
throw FRC_MakeError(frc::err::CommandIllegalUse,
"Commands that have been scheduled individually may "
"not be added to another composition!");
}
RequireUngrouped(command);
}
void CommandScheduler::RequireUngroupedAndUnscheduled(
std::span<const std::unique_ptr<Command>> commands) {
for (auto&& command : commands) {
RequireUngroupedAndUnscheduled(command.get());
}
}
void CommandScheduler::RequireUngroupedAndUnscheduled(
std::initializer_list<const Command*> commands) {
for (auto&& command : commands) {
RequireUngroupedAndUnscheduled(command);
}
}
void CommandScheduler::InitSendable(wpi::SendableBuilder& builder) {
builder.SetSmartDashboardType("Scheduler");
builder.AddStringArrayProperty(

2
wpilibNewCommands/src/main/native/cpp/frc2/command/ConditionalCommand.cpp
View File

@@ -12,7 +12,7 @@ ConditionalCommand::ConditionalCommand(std::unique_ptr<Command>&& onTrue,
std::unique_ptr<Command>&& onFalse,
std::function<bool()> condition)
: m_condition{std::move(condition)} {
CommandScheduler::GetInstance().RequireUngrouped(
CommandScheduler::GetInstance().RequireUngroupedAndUnscheduled(
{onTrue.get(), onFalse.get()});
m_onTrue = std::move(onTrue);

2
wpilibNewCommands/src/main/native/cpp/frc2/command/ParallelCommandGroup.cpp
View File

@@ -63,7 +63,7 @@ Command::InterruptionBehavior ParallelCommandGroup::GetInterruptionBehavior()
void ParallelCommandGroup::AddCommands(
std::vector<std::unique_ptr<Command>>&& commands) {
CommandScheduler::GetInstance().RequireUngrouped(commands);
CommandScheduler::GetInstance().RequireUngroupedAndUnscheduled(commands);
if (isRunning) {
throw FRC_MakeError(frc::err::CommandIllegalUse,

2
wpilibNewCommands/src/main/native/cpp/frc2/command/ParallelDeadlineGroup.cpp
View File

@@ -62,7 +62,7 @@ Command::InterruptionBehavior ParallelDeadlineGroup::GetInterruptionBehavior()
void ParallelDeadlineGroup::AddCommands(
std::vector<std::unique_ptr<Command>>&& commands) {
CommandScheduler::GetInstance().RequireUngrouped(commands);
CommandScheduler::GetInstance().RequireUngroupedAndUnscheduled(commands);
if (!m_finished) {
throw FRC_MakeError(frc::err::CommandIllegalUse,

2
wpilibNewCommands/src/main/native/cpp/frc2/command/ParallelRaceGroup.cpp
View File

@@ -50,7 +50,7 @@ Command::InterruptionBehavior ParallelRaceGroup::GetInterruptionBehavior()
void ParallelRaceGroup::AddCommands(
std::vector<std::unique_ptr<Command>>&& commands) {
CommandScheduler::GetInstance().RequireUngrouped(commands);
CommandScheduler::GetInstance().RequireUngroupedAndUnscheduled(commands);
if (isRunning) {
throw FRC_MakeError(frc::err::CommandIllegalUse,

2
wpilibNewCommands/src/main/native/cpp/frc2/command/RepeatCommand.cpp
View File

@@ -9,7 +9,7 @@
using namespace frc2;
RepeatCommand::RepeatCommand(std::unique_ptr<Command>&& command) {
CommandScheduler::GetInstance().RequireUngrouped(command.get());
CommandScheduler::GetInstance().RequireUngroupedAndUnscheduled(command.get());
m_command = std::move(command);
m_command->SetComposed(true);
AddRequirements(m_command->GetRequirements());

2
wpilibNewCommands/src/main/native/cpp/frc2/command/SequentialCommandGroup.cpp
View File

@@ -62,7 +62,7 @@ Command::InterruptionBehavior SequentialCommandGroup::GetInterruptionBehavior()
void SequentialCommandGroup::AddCommands(
std::vector<std::unique_ptr<Command>>&& commands) {
CommandScheduler::GetInstance().RequireUngrouped(commands);
CommandScheduler::GetInstance().RequireUngroupedAndUnscheduled(commands);
if (m_currentCommandIndex != invalid_index) {
throw FRC_MakeError(frc::err::CommandIllegalUse,

2
wpilibNewCommands/src/main/native/cpp/frc2/command/WrapperCommand.cpp
View File

@@ -9,7 +9,7 @@
using namespace frc2;
WrapperCommand::WrapperCommand(std::unique_ptr<Command>&& command) {
CommandScheduler::GetInstance().RequireUngrouped(command.get());
CommandScheduler::GetInstance().RequireUngroupedAndUnscheduled(command.get());
m_command = std::move(command);
m_command->SetComposed(true);
// copy the wrapped command's name

38
wpilibNewCommands/src/main/native/include/frc2/command/CommandScheduler.h
View File

@@ -374,7 +374,7 @@ class CommandScheduler final : public wpi::Sendable,
void OnCommandFinish(Action action);
/**
* Requires that the specified command hasn't been already added to a
* Requires that the specified command hasn't already been added to a
* composition.
*
* @param command The command to check
@@ -383,7 +383,7 @@ class CommandScheduler final : public wpi::Sendable,
void RequireUngrouped(const Command* command);
/**
* Requires that the specified commands not have been already added to a
* Requires that the specified commands have not already been added to a
* composition.
*
* @param commands The commands to check
@@ -392,7 +392,7 @@ class CommandScheduler final : public wpi::Sendable,
void RequireUngrouped(std::span<const std::unique_ptr<Command>> commands);
/**
* Requires that the specified commands not have been already added to a
* Requires that the specified commands have not already been added to a
* composition.
*
* @param commands The commands to check
@@ -401,6 +401,38 @@ class CommandScheduler final : public wpi::Sendable,
*/
void RequireUngrouped(std::initializer_list<const Command*> commands);
/**
* Requires that the specified command has not 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.
*/
void RequireUngroupedAndUnscheduled(const Command* 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.
*/
void RequireUngroupedAndUnscheduled(
std::span<const std::unique_ptr<Command>> commands);
/**
* 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.
*/
void RequireUngroupedAndUnscheduled(
std::initializer_list<const Command*> commands);
void InitSendable(wpi::SendableBuilder& builder) override;
private:

6
wpilibNewCommands/src/main/native/include/frc2/command/SelectCommand.h
View File

@@ -56,7 +56,8 @@ class SelectCommand : public CommandHelper<Command, SelectCommand<Key>> {
m_defaultCommand.SetComposed(true);
for (auto&& command : foo) {
CommandScheduler::GetInstance().RequireUngrouped(command.second.get());
CommandScheduler::GetInstance().RequireUngroupedAndUnscheduled(
command.second.get());
command.second.get()->SetComposed(true);
}
@@ -77,7 +78,8 @@ class SelectCommand : public CommandHelper<Command, SelectCommand<Key>> {
: m_selector{std::move(selector)} {
m_defaultCommand.SetComposed(true);
for (auto&& command : commands) {
CommandScheduler::GetInstance().RequireUngrouped(command.second.get());
CommandScheduler::GetInstance().RequireUngroupedAndUnscheduled(
command.second.get());
command.second.get()->SetComposed(true);
}

47
wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/CommandGroupErrorTest.java
View File

@@ -1,47 +0,0 @@
// Copyright (c) FIRST and other WPILib contributors.
// Open Source Software; you can modify and/or share it under the terms of
// the WPILib BSD license file in the root directory of this project.
package edu.wpi.first.wpilibj2.command;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertThrows;
import org.junit.jupiter.api.Test;
class CommandGroupErrorTest extends CommandTestBase {
@Test
void commandInMultipleGroupsTest() {
MockCommandHolder command1Holder = new MockCommandHolder(true);
Command command1 = command1Holder.getMock();
MockCommandHolder command2Holder = new MockCommandHolder(true);
Command command2 = command2Holder.getMock();
new ParallelCommandGroup(command1, command2);
assertThrows(
IllegalArgumentException.class, () -> new ParallelCommandGroup(command1, command2));
}
@Test
void commandInGroupExternallyScheduledTest() {
MockCommandHolder command1Holder = new MockCommandHolder(true);
Command command1 = command1Holder.getMock();
MockCommandHolder command2Holder = new MockCommandHolder(true);
Command command2 = command2Holder.getMock();
new ParallelCommandGroup(command1, command2);
assertThrows(
IllegalArgumentException.class, () -> CommandScheduler.getInstance().schedule(command1));
}
@Test
void redecoratedCommandErrorTest() {
Command command = new InstantCommand();
assertDoesNotThrow(() -> command.withTimeout(10).until(() -> false));
assertThrows(IllegalArgumentException.class, () -> command.withTimeout(10));
CommandScheduler.getInstance().removeComposedCommand(command);
assertDoesNotThrow(() -> command.withTimeout(10));
}
}

10
wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/MultiCompositionTestBase.java
View File

@@ -13,11 +13,11 @@ import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
interface MultiCompositionTestBase<T extends Command> extends SingleCompositionTestBase<T> {
T compose(Command... members);
abstract class MultiCompositionTestBase<T extends Command> extends SingleCompositionTestBase<T> {
abstract T compose(Command... members);
@Override
default T composeSingle(Command member) {
T composeSingle(Command member) {
return compose(member);
}
@@ -63,7 +63,7 @@ interface MultiCompositionTestBase<T extends Command> extends SingleCompositionT
@MethodSource
@ParameterizedTest(name = "interruptible[{index}]: {0}")
default void interruptible(
void interruptible(
@SuppressWarnings("unused") String name,
InterruptionBehavior expected,
Command command1,
@@ -103,7 +103,7 @@ interface MultiCompositionTestBase<T extends Command> extends SingleCompositionT
@MethodSource
@ParameterizedTest(name = "runsWhenDisabled[{index}]: {0}")
default void runsWhenDisabled(
void runsWhenDisabled(
@SuppressWarnings("unused") String name,
boolean expected,
Command command1,

3
wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/ParallelCommandGroupTest.java
View File

@@ -14,8 +14,7 @@ import static org.mockito.Mockito.verify;
import org.junit.jupiter.api.Test;
class ParallelCommandGroupTest extends CommandTestBase
implements MultiCompositionTestBase<ParallelCommandGroup> {
class ParallelCommandGroupTest extends MultiCompositionTestBase<ParallelCommandGroup> {
@Test
void parallelGroupScheduleTest() {
try (CommandScheduler scheduler = new CommandScheduler()) {

3
wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/ParallelDeadlineGroupTest.java
View File

@@ -14,8 +14,7 @@ import static org.mockito.internal.verification.VerificationModeFactory.times;
import java.util.Arrays;
import org.junit.jupiter.api.Test;
class ParallelDeadlineGroupTest extends CommandTestBase
implements MultiCompositionTestBase<ParallelDeadlineGroup> {
class ParallelDeadlineGroupTest extends MultiCompositionTestBase<ParallelDeadlineGroup> {
@Test
void parallelDeadlineScheduleTest() {
try (CommandScheduler scheduler = new CommandScheduler()) {

3
wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/ParallelRaceGroupTest.java
View File

@@ -16,8 +16,7 @@ import static org.mockito.Mockito.verify;
import org.junit.jupiter.api.Test;
class ParallelRaceGroupTest extends CommandTestBase
implements MultiCompositionTestBase<ParallelRaceGroup> {
class ParallelRaceGroupTest extends MultiCompositionTestBase<ParallelRaceGroup> {
@Test
void parallelRaceScheduleTest() {
try (CommandScheduler scheduler = new CommandScheduler()) {

3
wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/RepeatCommandTest.java
View File

@@ -10,8 +10,7 @@ import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import org.junit.jupiter.api.Test;
class RepeatCommandTest extends CommandTestBase
implements SingleCompositionTestBase<RepeatCommand> {
class RepeatCommandTest extends SingleCompositionTestBase<RepeatCommand> {
@Test
void callsMethodsCorrectly() {
try (CommandScheduler scheduler = new CommandScheduler()) {

3
wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/SelectCommandTest.java
View File

@@ -13,8 +13,7 @@ import java.util.HashMap;
import java.util.Map;
import org.junit.jupiter.api.Test;
class SelectCommandTest extends CommandTestBase
implements MultiCompositionTestBase<SelectCommand<Integer>> {
class SelectCommandTest extends MultiCompositionTestBase<SelectCommand<Integer>> {
@Test
void selectCommandTest() {
try (CommandScheduler scheduler = new CommandScheduler()) {

3
wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/SequentialCommandGroupTest.java
View File

@@ -12,8 +12,7 @@ import static org.mockito.Mockito.verify;
import org.junit.jupiter.api.Test;
class SequentialCommandGroupTest extends CommandTestBase
implements MultiCompositionTestBase<SequentialCommandGroup> {
class SequentialCommandGroupTest extends MultiCompositionTestBase<SequentialCommandGroup> {
@Test
void sequentialGroupScheduleTest() {
try (CommandScheduler scheduler = new CommandScheduler()) {

39
wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/SingleCompositionTestBase.java
View File

@@ -5,17 +5,19 @@
package edu.wpi.first.wpilibj2.command;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.EnumSource;
import org.junit.jupiter.params.provider.ValueSource;
public interface SingleCompositionTestBase<T extends Command> {
T composeSingle(Command member);
public abstract class SingleCompositionTestBase<T extends Command> extends CommandTestBase {
abstract T composeSingle(Command member);
@EnumSource(Command.InterruptionBehavior.class)
@ParameterizedTest
default void interruptible(Command.InterruptionBehavior interruptionBehavior) {
void interruptible(Command.InterruptionBehavior interruptionBehavior) {
var command =
composeSingle(
new WaitUntilCommand(() -> false).withInterruptBehavior(interruptionBehavior));
@@ -24,9 +26,38 @@ public interface SingleCompositionTestBase<T extends Command> {
@ValueSource(booleans = {true, false})
@ParameterizedTest
default void runWhenDisabled(boolean runsWhenDisabled) {
void runWhenDisabled(boolean runsWhenDisabled) {
var command =
composeSingle(new WaitUntilCommand(() -> false).ignoringDisable(runsWhenDisabled));
assertEquals(runsWhenDisabled, command.runsWhenDisabled());
}
@Test
void commandInOtherCompositionTest() {
var command = new InstantCommand();
new WrapperCommand(command) {};
assertThrows(IllegalArgumentException.class, () -> composeSingle(command));
}
@Test
void commandInMultipleCompositionsTest() {
var command = new InstantCommand();
composeSingle(command);
assertThrows(IllegalArgumentException.class, () -> composeSingle(command));
}
@Test
void composeThenScheduleTest() {
var command = new InstantCommand();
composeSingle(command);
assertThrows(
IllegalArgumentException.class, () -> CommandScheduler.getInstance().schedule(command));
}
@Test
void scheduleThenComposeTest() {
var command = new RunCommand(() -> {});
CommandScheduler.getInstance().schedule(command);
assertThrows(IllegalArgumentException.class, () -> composeSingle(command));
}
}